[v2v PATCH 0/9] ensure x86-64-v2 uarch level for RHEL-9.0+ guests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 We don't specify a VCPU model in the destination domain description if the source description doesn't specify one. This is a problem for x86-64 RHEL-9.0+ guests, when using the QEMU or libvirt output modules. Those outputs default to the "qemu64" VCPU model in the absence of an explicit VCPU model specification, and "qemu64" does not satisfy the "x86-64-v2" microarchitecture level, which is required by RHEL-9.0+ guests. As a result, these guests crash during boot, after conversion. The series recognizes guests that are unable to boot on the default QEMU VCPU model, and specifies host CPU passthrough for them in the QEMU and libvirt output modules. Testing: <https://bugzilla.redhat.com/show_bug.cgi?id=2076013#c18>. Thanks, Laszlo Laszlo Ersek (9): types: introduce the "gcaps_default_cpu" field create_libvirt_xml: simplify match on (s_cpu_vendor, s_cpu_model) create_libvirt_xml: normalize match on (s_cpu_vendor, s_cpu_model) create_libvirt_xml: eliminate childless <cpu match="minimum"/> element create_libvirt_xml: restrict 'match="minimum"' <cpu> attribute production create_libvirt_xml: honor "gcaps_default_cpu" output_qemu: reflect source VCPU model to the QEMU command line output_qemu: honor "gcaps_default_cpu" convert_linux: set "gcaps_default_cpu = false" for x86_64 RHEL-9.0+ guests convert/convert_linux.ml | 9 ++++++ convert/convert_windows.ml | 1 + lib/types.ml | 3 ++ lib/types.mli | 14 +++++++++ output/create_libvirt_xml.ml | 31 +++++++++++--------- output/output_qemu.ml | 7 +++++ 6 files changed, 51 insertions(+), 14 deletions(-) base-commit: 8643970f9791b1c90dfd6a2dd1abfc9afef1fb52 -- 2.19.1.3.g30247aa5d201
Conversion will set "gcaps_default_cpu" to true iff the guest OS is capable of running on QEMU's default VCPU model. This capability will only matter for the QEMU and libvirt output modules, where, in case the capability is false *and* the source hypervisor does not specify a VCPU model, we'll have to manually present the guest OS with a VCPU that looks as close as possible to a physical CPU. (In that case, we'll specify host-passthrough.) The management applications targeted by the RHV and OpenStack output modules have their own explicit VCPU defaults, overriding the QEMU default model even in case the source hypervisor does not specify a VCPU model; those modules will ignore this capability therefore. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- lib/types.mli | 14 ++++++++++++++ convert/convert_linux.ml | 1 + convert/convert_windows.ml | 1 + lib/types.ml | 3 +++ 4 files changed, 19 insertions(+) diff --git a/lib/types.mli b/lib/types.mli index 37238cd75519..87544a0c59eb 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -251,52 +251,66 @@ val string_of_target_firmware : target_firmware -> string (** {2 Target NICs} *) type target_nics = source_nic list (** {2 Guest capabilities} *) type guestcaps = { gcaps_block_bus : guestcaps_block_type; gcaps_net_bus : guestcaps_net_type; (** Best block device and network device guest can access. These are determined during conversion by inspecting the guest (and in some cases conversion can actually enhance these by installing drivers). Thus this is not known until after conversion. *) gcaps_virtio_rng : bool; (** Guest supports virtio-rng. *) gcaps_virtio_balloon : bool; (** Guest supports virtio balloon. *) gcaps_isa_pvpanic : bool; (** Guest supports ISA pvpanic device. *) gcaps_virtio_socket : bool; (** Guest supports virtio socket. *) gcaps_machine : guestcaps_machine; (** Machine model. *) gcaps_arch : string; (** Architecture that KVM must emulate. *) gcaps_acpi : bool; (** True if guest supports acpi. *) gcaps_virtio_1_0 : bool; (** The guest supports the virtio devices that it does at the virtio-1.0 protocol level. *) + + gcaps_default_cpu : bool; + (** True iff the guest OS is capable of running on QEMU's default VCPU model. + + This capability only matters for the QEMU and libvirt output modules, + where, in case the capability is false *and* the source hypervisor does + not specify a VCPU model, we must manually present the guest OS with a + VCPU that looks as close as possible to a physical CPU. (In that case, we + specify host-passthrough.) + + The management applications targeted by the RHV and OpenStack output + modules have their own explicit VCPU defaults, overriding the QEMU default + model even in case the source hypervisor does not specify a VCPU model; + those modules ignore this capability therefore. Refer to RHBZ#2076013. *) } (** Guest capabilities after conversion. eg. Was virtio found or installed? *) and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_machine = I440FX | Q35 | Virt val string_of_guestcaps : guestcaps -> string (** {2 Guest buses} *) type target_buses = { target_virtio_blk_bus : target_bus_slot array; target_ide_bus : target_bus_slot array; target_scsi_bus : target_bus_slot array; target_floppy_bus : target_bus_slot array; } (** Mapping of fixed and removable disks to buses. As shown in the diagram below, there are (currently) four buses attached to the target VM. Each contains a chain of fixed or removable disks. Slots can also be empty. We try to assign disks to the same slot number as they would occupy on the source, although that is not always possible. diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index c2bbce03637f..93b3922cac60 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -151,52 +151,53 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = if major <= 4 then I440FX else Q35 | ("sles"|"suse-based"|"opensuse"), major -> if major < 10 then I440FX else Q35 | ("debian"|"ubuntu"|"linuxmint"|"kalilinux"), major -> if major < 4 then I440FX else Q35 (* reasonable default for all modern Linux kernels *) | _, _ -> Q35 ), true ) | _ -> Virt, true in (* Return guest capabilities from the convert () function. *) let guestcaps = { gcaps_block_bus = block_type; gcaps_net_bus = net_type; gcaps_virtio_rng = kernel.ki_supports_virtio_rng; gcaps_virtio_balloon = kernel.ki_supports_virtio_balloon; gcaps_isa_pvpanic = kernel.ki_supports_isa_pvpanic; gcaps_virtio_socket = kernel.ki_supports_virtio_socket; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; gcaps_acpi = acpi; gcaps_virtio_1_0 = virtio_1_0; + gcaps_default_cpu = true; } in guestcaps and augeas_grub_configuration () = if bootloader#set_augeas_configuration () then Linux.augeas_reload g and unconfigure_xen () = (* Remove kmod-xenpv-* (RHEL 3). *) let xenmods = List.filter_map ( fun { G.app2_name = name } -> if name = "kmod-xenpv" || String.is_prefix name "kmod-xenpv-" then Some name else None ) inspect.i_apps in Linux.remove g inspect xenmods; (* Undo related nastiness if kmod-xenpv was installed. *) if xenmods <> [] then ( (* kmod-xenpv modules may have been manually copied to other kernels. * Hunt them down and destroy them. *) let dirs = g#find "/lib/modules" in diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index b53312a9aba4..34a5044dd338 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -282,52 +282,53 @@ let convert (g : G.guestfs) _ inspect _ static_ips = Libosinfo_utils.os_support_of_osinfo_device_list (devices @ best_drv_devs) in (if q35 then Q35 else I440FX), vio10 with | Not_found -> (* Pivot on the year 2007. Any Windows version from earlier than * 2007 should use i440fx, anything 2007 or newer should use q35. * Luckily this coincides almost exactly with the release of NT 6. *) (if inspect.i_major_version < 6 then I440FX else Q35), true ) | _ -> Virt, true in (* Return guest capabilities from the convert () function. *) let guestcaps = { gcaps_block_bus = block_driver; gcaps_net_bus = net_driver; gcaps_virtio_rng = virtio_rng_supported; gcaps_virtio_balloon = virtio_ballon_supported; gcaps_isa_pvpanic = isa_pvpanic_supported; gcaps_virtio_socket = virtio_socket_supported; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; gcaps_acpi = true; gcaps_virtio_1_0 = virtio_1_0; + gcaps_default_cpu = true; } in guestcaps and configure_firstboot () = (* Note that pnp_wait.exe must be the first firstboot script as it * suppresses PnP for all following scripts. *) let tool_path = virt_tools_data_dir () // "pnp_wait.exe" in if Sys.file_exists tool_path then configure_wait_pnp tool_path else debug (f_"%s is missing. Firstboot scripts may conflict with PnP.") tool_path; (* Install VMDP unconditionally, if available, but don't * warn about it if not. *) let tool_path = virt_tools_data_dir () // "vmdp.exe" in if Sys.file_exists tool_path then configure_vmdp tool_path; (* Install QEMU Guest Agent unconditionally and warn if missing *) let qemu_ga_files = Windows_virtio.copy_qemu_ga g inspect in if qemu_ga_files <> [] then configure_qemu_ga qemu_ga_files diff --git a/lib/types.ml b/lib/types.ml index 92ed0e52513d..7ffb868b1fbf 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -383,92 +383,95 @@ let string_of_target t = target format: %s " (match t.target_file with | TargetFile s -> "[file] " ^ s | TargetURI s -> "[qemu] " ^ s) t.target_format type target_firmware = TargetBIOS | TargetUEFI let string_of_target_firmware = function | TargetBIOS -> "bios" | TargetUEFI -> "uefi" type target_nics = source_nic list type guestcaps = { gcaps_block_bus : guestcaps_block_type; gcaps_net_bus : guestcaps_net_type; gcaps_virtio_rng : bool; gcaps_virtio_balloon : bool; gcaps_isa_pvpanic : bool; gcaps_virtio_socket : bool; gcaps_machine : guestcaps_machine; gcaps_arch : string; gcaps_acpi : bool; gcaps_virtio_1_0 : bool; + gcaps_default_cpu : bool; } and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_machine = I440FX | Q35 | Virt let string_of_block_type = function | Virtio_blk -> "virtio-blk" | IDE -> "ide" let string_of_net_type = function | Virtio_net -> "virtio-net" | E1000 -> "e1000" | RTL8139 -> "rtl8139" let string_of_machine = function | I440FX -> "i440fx" | Q35 -> "q35" | Virt -> "virt" let string_of_guestcaps gcaps = sprintf "\ gcaps_block_bus = %s\n\ gcaps_net_bus = %s\n\ gcaps_virtio_rng = %b\n\ gcaps_virtio_balloon = %b\n\ gcaps_isa_pvpanic = %b\n\ gcaps_virtio_socket = %b\n\ gcaps_machine = %s\n\ gcaps_arch = %s\n\ gcaps_acpi = %b\n\ gcaps_virtio_1_0 = %b\n\ + gcaps_default_cpu = %b\n\ " (string_of_block_type gcaps.gcaps_block_bus) (string_of_net_type gcaps.gcaps_net_bus) gcaps.gcaps_virtio_rng gcaps.gcaps_virtio_balloon gcaps.gcaps_isa_pvpanic gcaps.gcaps_virtio_socket (string_of_machine gcaps.gcaps_machine) gcaps.gcaps_arch gcaps.gcaps_acpi gcaps.gcaps_virtio_1_0 + gcaps.gcaps_default_cpu type target_buses = { target_virtio_blk_bus : target_bus_slot array; target_ide_bus : target_bus_slot array; target_scsi_bus : target_bus_slot array; target_floppy_bus : target_bus_slot array; } and target_bus_slot = | BusSlotEmpty | BusSlotDisk of source_disk | BusSlotRemovable of source_removable let string_of_target_bus_slots bus_name slots = let slots = Array.mapi ( fun slot_nr slot -> sprintf "%s slot %d:\n" bus_name slot_nr ^ (match slot with | BusSlotEmpty -> "\t(slot empty)\n" | BusSlotDisk d -> string_of_source_disk d | BusSlotRemovable r -> string_of_source_removable r ^ "\n" ) ) slots in String.concat "" (Array.to_list slots) -- 2.19.1.3.g30247aa5d201
On Wed, Apr 20, 2022 at 06:23:25PM +0200, Laszlo Ersek wrote:
+ gcaps_default_cpu : bool; + (** True iff the guest OS is capable of running on QEMU's default VCPU model.
Can we say “(eg. -cpu qemu64)” here? Might be easier than trying to parse what QEMU's default VCPU means. Rest looks fine, so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 04/20/22 18:47, Richard W.M. Jones wrote:
On Wed, Apr 20, 2022 at 06:23:25PM +0200, Laszlo Ersek wrote:
+ gcaps_default_cpu : bool; + (** True iff the guest OS is capable of running on QEMU's default VCPU model.
Can we say “(eg. -cpu qemu64)” here? Might be easier than trying to parse what QEMU's default VCPU means.
I purposely avoided "qemu64", because the concept applies to "qemu-system-aarch64 -M virt", and qemu64 is not a VCPU model there. (NB in the output modules, "-cpu host" *does* make sense for the Virt machine type, and I think the libvirt domain XML fragment is arch-agnostic as well.) (Similarly, the check in convert-linux is also restricted to the x86_64 guest arch; aarch64 RHEL9 does not have this problem to my knowledge.) I can of course say '(eg. "-cpu qemu64" with the Q35 and I440FX machine types)'; so I'll do that. Thanks! Laszlo
Rest looks fine, so:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Rich.
Squash the patterns | None, None -> () | Some _, None -> () into the identical | _, None -> () We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21); the change only simplifies the code. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 36173e58cd6c..4e5bbceffabd 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -162,55 +162,57 @@ let create_libvirt_xml ?pool source inspect (match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; ); let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ]; if source.s_cpu_vendor <> None || source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in (match source.s_cpu_vendor, source.s_cpu_model with - | None, None - (* Avoid libvirt error: "CPU vendor specified without CPU model" *) - | Some _, None -> () + | _, None -> + (* This also avoids the libvirt error: + * "CPU vendor specified without CPU model". + *) + () | None, Some model -> List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) | Some vendor, Some model -> List.push_back_list cpu [ e "vendor" [] [PCData vendor]; e "model" ["fallback", "allow"] [PCData model] ] ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) ); List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] ); let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in -- 2.19.1.3.g30247aa5d201
On Wed, Apr 20, 2022 at 06:23:26PM +0200, Laszlo Ersek wrote:
Squash the patterns
| None, None -> () | Some _, None -> ()
into the identical
| _, None -> ()
We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21); the change only simplifies the code.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 36173e58cd6c..4e5bbceffabd 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -162,55 +162,57 @@ let create_libvirt_xml ?pool source inspect
(match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; );
let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
if source.s_cpu_vendor <> None || source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
(match source.s_cpu_vendor, source.s_cpu_model with - | None, None - (* Avoid libvirt error: "CPU vendor specified without CPU model" *) - | Some _, None -> () + | _, None -> + (* This also avoids the libvirt error: + * "CPU vendor specified without CPU model". + *) + () | None, Some model -> List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) | Some vendor, Some model -> List.push_back_list cpu [ e "vendor" [] [PCData vendor]; e "model" ["fallback", "allow"] [PCData model] ] ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] );
let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> As an aside, your git includes a crazy large amount of context around the changes ... Is that a configuration change? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 04/20/22 18:49, Richard W.M. Jones wrote:
On Wed, Apr 20, 2022 at 06:23:26PM +0200, Laszlo Ersek wrote:
Squash the patterns
| None, None -> () | Some _, None -> ()
into the identical
| _, None -> ()
We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21); the change only simplifies the code.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 36173e58cd6c..4e5bbceffabd 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -162,55 +162,57 @@ let create_libvirt_xml ?pool source inspect
(match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; );
let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
if source.s_cpu_vendor <> None || source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
(match source.s_cpu_vendor, source.s_cpu_model with - | None, None - (* Avoid libvirt error: "CPU vendor specified without CPU model" *) - | Some _, None -> () + | _, None -> + (* This also avoids the libvirt error: + * "CPU vendor specified without CPU model". + *) + () | None, Some model -> List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) | Some vendor, Some model -> List.push_back_list cpu [ e "vendor" [] [PCData vendor]; e "model" ["fallback", "allow"] [PCData model] ] ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] );
let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
As an aside, your git includes a crazy large amount of context around the changes ... Is that a configuration change?
It was intentional: I passed "-U26" to git-format-patch. Before I post a patch series, I review each patch in "gitk", and determine the minimal context size that makes it easy for reviewers to get a complete understanding of the change, without having to apply the series, and to re-display the patches locally with a larger context. Most of the time, the option "--function-context" (aka -W) works well for this, and that option supports even OCaml -- but only to an extent. When definitions are nested, and/or introduced with "and" (not "let"), the function boundaries are not determined well. So in that case, I start with a standard context size of 3 lines in gitk, and increase it as needed, as I traverse the patches. In this case, I arrived at "-U26" because of patch#4: create_libvirt_xml: eliminate childless <cpu match="minimum"/> element Because you really need 26 lines of context to see that the patch is correct. This doesn't tell you anything:
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index d2ca894d60bb..4a71f8bb27f5 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -180,7 +180,7 @@ let create_libvirt_xml ?pool source inspect e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
- if source.s_cpu_vendor <> None || source.s_cpu_model <> None || + if source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
but this does:
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index d2ca894d60bb..4a71f8bb27f5 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -157,53 +157,53 @@ let create_libvirt_xml ?pool source inspect
(match source.s_genid with | None -> () | Some genid -> List.push_back body (e "genid" [] [PCData genid]) );
(match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; );
let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
- if source.s_cpu_vendor <> None || source.s_cpu_model <> None || + if source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
(match source.s_cpu_model with | None -> () | Some model -> (match source.s_cpu_vendor with | None -> () | Some vendor -> List.push_back cpu (e "vendor" [] [PCData vendor]) ); List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] );
Unfortunately, I don't know of a way to attach this "useful context size" information to the individual patches; especially so that the info survive rebases. I could format each patch individually with a different -U option, but that's something I'm not ready to do :) If I could attach specially formatted notes, or commit message lines, to the patches, for driving "-U", that would be really nice. Also I usually spell out my quirky -U options in the cover letter, but this time I didn't do it. Thanks Laszlo
On Thu, Apr 21, 2022 at 05:27:06PM +0200, Laszlo Ersek wrote:
On 04/20/22 18:49, Richard W.M. Jones wrote:
On Wed, Apr 20, 2022 at 06:23:26PM +0200, Laszlo Ersek wrote:
Squash the patterns
| None, None -> () | Some _, None -> ()
into the identical
| _, None -> ()
We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21); the change only simplifies the code.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 36173e58cd6c..4e5bbceffabd 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -162,55 +162,57 @@ let create_libvirt_xml ?pool source inspect
(match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; );
let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
if source.s_cpu_vendor <> None || source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
(match source.s_cpu_vendor, source.s_cpu_model with - | None, None - (* Avoid libvirt error: "CPU vendor specified without CPU model" *) - | Some _, None -> () + | _, None -> + (* This also avoids the libvirt error: + * "CPU vendor specified without CPU model". + *) + () | None, Some model -> List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) | Some vendor, Some model -> List.push_back_list cpu [ e "vendor" [] [PCData vendor]; e "model" ["fallback", "allow"] [PCData model] ] ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] );
let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
As an aside, your git includes a crazy large amount of context around the changes ... Is that a configuration change?
It was intentional: I passed "-U26" to git-format-patch.
Before I post a patch series, I review each patch in "gitk", and determine the minimal context size that makes it easy for reviewers to get a complete understanding of the change, without having to apply the series, and to re-display the patches locally with a larger context.
Most of the time, the option "--function-context" (aka -W) works well for this, and that option supports even OCaml -- but only to an extent. When definitions are nested, and/or introduced with "and" (not "let"), the function boundaries are not determined well. So in that case, I start with a standard context size of 3 lines in gitk, and increase it as needed, as I traverse the patches.
In this case, I arrived at "-U26" because of patch#4:
create_libvirt_xml: eliminate childless <cpu match="minimum"/> element
Because you really need 26 lines of context to see that the patch is correct.
This doesn't tell you anything:
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index d2ca894d60bb..4a71f8bb27f5 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -180,7 +180,7 @@ let create_libvirt_xml ?pool source inspect e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
- if source.s_cpu_vendor <> None || source.s_cpu_model <> None || + if source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
but this does:
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index d2ca894d60bb..4a71f8bb27f5 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -157,53 +157,53 @@ let create_libvirt_xml ?pool source inspect
(match source.s_genid with | None -> () | Some genid -> List.push_back body (e "genid" [] [PCData genid]) );
(match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; );
let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
- if source.s_cpu_vendor <> None || source.s_cpu_model <> None || + if source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
(match source.s_cpu_model with | None -> () | Some model -> (match source.s_cpu_vendor with | None -> () | Some vendor -> List.push_back cpu (e "vendor" [] [PCData vendor]) ); List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] );
Unfortunately, I don't know of a way to attach this "useful context size" information to the individual patches; especially so that the info survive rebases. I could format each patch individually with a different -U option, but that's something I'm not ready to do :) If I could attach specially formatted notes, or commit message lines, to the patches, for driving "-U", that would be really nice.
Also I usually spell out my quirky -U options in the cover letter, but this time I didn't do it.
Understood - I think it does improve the ability to review patches. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
In the match on (s_cpu_vendor, s_cpu_model), the following expression is duplicated: e "model" ["fallback", "allow"] [PCData model] For eliminating this, normalize the match: match each component in separation, in the proper order, creating a tree-like match rather than a table-like one. This is done in preparation for a subsequent patch, which would otherwise duplicate even more code. We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21); the change only simplifies the code. With the elimination of the table-like pattern matching, we need not specifically mention the "CPU vendor specified without CPU model" libvirt error. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 22 ++++++++------------ 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 4e5bbceffabd..d2ca894d60bb 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -161,65 +161,61 @@ let create_libvirt_xml ?pool source inspect ); (match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; ); let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ]; if source.s_cpu_vendor <> None || source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in - (match source.s_cpu_vendor, source.s_cpu_model with - | _, None -> - (* This also avoids the libvirt error: - * "CPU vendor specified without CPU model". - *) - () - | None, Some model -> - List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) - | Some vendor, Some model -> - List.push_back_list cpu [ - e "vendor" [] [PCData vendor]; - e "model" ["fallback", "allow"] [PCData model] - ] + (match source.s_cpu_model with + | None -> () + | Some model -> + (match source.s_cpu_vendor with + | None -> () + | Some vendor -> + List.push_back cpu (e "vendor" [] [PCData vendor]) + ); + List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) ); List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] ); let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in let machine, secure_boot_required = match guestcaps.gcaps_machine, uefi_firmware with | _, Some { Uefi.flags = flags } when List.mem Uefi.UEFI_FLAG_SECURE_BOOT_REQUIRED flags -> (* Force machine type to Q35 because PC does not support * secure boot. We must remove this when we get the * correct machine type from libosinfo in future. XXX -- 2.19.1.3.g30247aa5d201
On Wed, Apr 20, 2022 at 06:23:27PM +0200, Laszlo Ersek wrote:
In the match on (s_cpu_vendor, s_cpu_model), the following expression is duplicated:
e "model" ["fallback", "allow"] [PCData model]
For eliminating this, normalize the match: match each component in separation, in the proper order, creating a tree-like match rather than a table-like one.
This is done in preparation for a subsequent patch, which would otherwise duplicate even more code.
We preserve the behavior added by commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21); the change only simplifies the code.
With the elimination of the table-like pattern matching, we need not specifically mention the "CPU vendor specified without CPU model" libvirt error.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 22 ++++++++------------ 1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 4e5bbceffabd..d2ca894d60bb 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -161,65 +161,61 @@ let create_libvirt_xml ?pool source inspect );
(match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; );
let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ];
if source.s_cpu_vendor <> None || source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in
- (match source.s_cpu_vendor, source.s_cpu_model with - | _, None -> - (* This also avoids the libvirt error: - * "CPU vendor specified without CPU model". - *) - () - | None, Some model -> - List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) - | Some vendor, Some model -> - List.push_back_list cpu [ - e "vendor" [] [PCData vendor]; - e "model" ["fallback", "allow"] [PCData model] - ] + (match source.s_cpu_model with + | None -> () + | Some model -> + (match source.s_cpu_vendor with + | None -> () + | Some vendor -> + List.push_back cpu (e "vendor" [] [PCData vendor]) + ); + List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) );
List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] );
let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in let machine, secure_boot_required = match guestcaps.gcaps_machine, uefi_firmware with | _, Some { Uefi.flags = flags } when List.mem Uefi.UEFI_FLAG_SECURE_BOOT_REQUIRED flags -> (* Force machine type to Q35 because PC does not support * secure boot. We must remove this when we get the * correct machine type from libosinfo in future. XXX --
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Commit 2a576b7cc5c3 ("v2v: -o libvirt: Don't write only <vendor> without <model> (RHBZ#1591789).", 2018-06-21) introduced a path in the code where we create a childless <cpu match="minimum"/> element. Namely, after said commit, in case source.s_cpu_vendor <> None && source.s_cpu_model = None && source.s_cpu_topology = None we no longer trigger a libvirt error; however, we do create the element <cpu match="minimum"/> without any children. Surprisingly, libvirt doesn't complain; it silently ignores and eliminates this element from the domain XML. Remove this code path by restricting the outer condition, for creating the <cpu> element, to: source.s_cpu_model <> None || source.s_cpu_topology <> None This reflects that "s_cpu_vendor" only plays a role if "s_cpu_model" is specified -- and the latter guarantees in itself that the <cpu> element will be generated with at least one child element. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index d2ca894d60bb..4a71f8bb27f5 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -157,53 +157,53 @@ let create_libvirt_xml ?pool source inspect (match source.s_genid with | None -> () | Some genid -> List.push_back body (e "genid" [] [PCData genid]) ); (match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; ); let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ]; - if source.s_cpu_vendor <> None || source.s_cpu_model <> None || + if source.s_cpu_model <> None || source.s_cpu_topology <> None then ( let cpu = ref [] in (match source.s_cpu_model with | None -> () | Some model -> (match source.s_cpu_vendor with | None -> () | Some vendor -> List.push_back cpu (e "vendor" [] [PCData vendor]) ); List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) ); List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] ); -- 2.19.1.3.g30247aa5d201
The 'match="minimum"' attribute of the <cpu> element only makes sense if the CPU model is specified. If the CPU model is not specified by "s_cpu_model", i.e., we produce the <cpu> element only due to "s_cpu_topology", then we need no attributes for <cpu> at all. Restrict 'match="minimum"' to when "s_cpu_model" is specified. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 4a71f8bb27f5..1e0242a768a0 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -159,76 +159,78 @@ let create_libvirt_xml ?pool source inspect | None -> () | Some genid -> List.push_back body (e "genid" [] [PCData genid]) ); (match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; ); let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ]; if source.s_cpu_model <> None || source.s_cpu_topology <> None then ( - let cpu = ref [] in + let cpu_attrs = ref [] + and cpu = ref [] in (match source.s_cpu_model with | None -> () | Some model -> + List.push_back cpu_attrs ("match", "minimum"); (match source.s_cpu_vendor with | None -> () | Some vendor -> List.push_back cpu (e "vendor" [] [PCData vendor]) ); List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) ); - List.push_back_list body [ e "cpu" [ "match", "minimum" ] !cpu ] + List.push_back_list body [ e "cpu" !cpu_attrs !cpu ] ); let uefi_firmware = match target_firmware with | TargetBIOS -> None | TargetUEFI -> Some (find_uefi_firmware guestcaps.gcaps_arch) in let machine, secure_boot_required = match guestcaps.gcaps_machine, uefi_firmware with | _, Some { Uefi.flags = flags } when List.mem Uefi.UEFI_FLAG_SECURE_BOOT_REQUIRED flags -> (* Force machine type to Q35 because PC does not support * secure boot. We must remove this when we get the * correct machine type from libosinfo in future. XXX *) Q35, true | machine, _ -> machine, false in let smm = secure_boot_required in (* We have the machine features of the guest when it was on the * source hypervisor (source.s_features). We have the acpi flag * which tells us whether acpi is required by this guest * (guestcaps.gcaps_acpi). And we have the set of hypervisor * features supported by the target (target_features). Combine all * this into a final list of features. *) -- 2.19.1.3.g30247aa5d201
If "s_cpu_model" is None and "gcaps_default_cpu" is "false", generate the <cpu mode="host-passthrough"/> element for libvirt. This element produces an (almost) exact copy of the host (i.e., physical) CPU for the guest, which is the best choice for guest OSes that cannot run on QEMU's default VCPU type -- considering that domains converted by virt-v2v are not expected to be migrateable without further tweaks by an administrator. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/create_libvirt_xml.ml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 1e0242a768a0..68d0a90918f3 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -158,58 +158,61 @@ let create_libvirt_xml ?pool source inspect (match source.s_genid with | None -> () | Some genid -> List.push_back body (e "genid" [] [PCData genid]) ); (match get_osinfo_id inspect with | None -> () | Some osinfo_id -> List.push_back_list body [ e "metadata" [] [ e "libosinfo:libosinfo" ["xmlns:libosinfo", "http://libosinfo.org/xmlns/libvirt/domain/1.0"] [ e "libosinfo:os" ["id", osinfo_id] []; ]; ]; ]; ); let memory_k = source.s_memory /^ 1024L in List.push_back_list body [ e "memory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "currentMemory" ["unit", "KiB"] [PCData (Int64.to_string memory_k)]; e "vcpu" [] [PCData (string_of_int source.s_vcpu)] ]; if source.s_cpu_model <> None || + not guestcaps.gcaps_default_cpu || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in (match source.s_cpu_model with - | None -> () + | None -> + if not guestcaps.gcaps_default_cpu then + List.push_back cpu_attrs ("mode", "host-passthrough"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); (match source.s_cpu_vendor with | None -> () | Some vendor -> List.push_back cpu (e "vendor" [] [PCData vendor]) ); List.push_back cpu (e "model" ["fallback", "allow"] [PCData model]) ); (match source.s_cpu_topology with | None -> () | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let topology_attrs = [ "sockets", string_of_int s_cpu_sockets; "cores", string_of_int s_cpu_cores; "threads", string_of_int s_cpu_threads; ] in List.push_back cpu (e "topology" topology_attrs []) ); List.push_back_list body [ e "cpu" !cpu_attrs !cpu ] ); let uefi_firmware = match target_firmware with | TargetBIOS -> None -- 2.19.1.3.g30247aa5d201
At the moment, QEMU is always started with the default "qemu64" VCPU model, even if the source hypervisor sets a particular VCPU model. "qemu64" is not suitable for some guest OSes. Honor "source.s_cpu_model" via the "-cpu" option, if the source specifies the VCPU model. The logic is basically copied from "lib/create_ovf.ml". Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/output_qemu.ml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/output/output_qemu.ml b/output/output_qemu.ml index f5e43705d101..385065f0579d 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -146,52 +146,58 @@ module QEMU = struct flag "-no-user-config"; flag "-nodefaults"; arg "-name" output_name; (match source.s_genid with | None -> () | Some genid -> arg_list "-device" ["vmgenid"; sprintf "guid=%s" genid; "id=vmgenid0"] ); arg_list "-machine" (machine_str :: (if smm then ["smm=on"] else []) @ ["accel=kvm:tcg"]); (match uefi_firmware with | None -> () | Some { Uefi.code } -> if secure_boot_required then arg_list "-global" ["driver=cfi.pflash01"; "property=secure"; "value=on"]; arg_list "-drive" ["if=pflash"; "format=raw"; "file=" ^ code; "readonly"]; arg_noquote "-drive" "if=pflash,format=raw,file=\"$uefi_vars\""; ); arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L)); + + (match source.s_cpu_model with + | None -> () + | Some model -> arg "-cpu" model + ); + if source.s_vcpu > 1 then ( (match source.s_cpu_topology with | None -> arg "-smp" (string_of_int source.s_vcpu) | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let args = [ sprintf "cpus=%d" source.s_vcpu; sprintf "sockets=%d" s_cpu_sockets; sprintf "cores=%d" s_cpu_cores; sprintf "threads=%d" s_cpu_threads; ] in arg_list "-smp" args ); ); (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies, we * need host-bus adapters (HBAs) between these devices and the PCI(e) root * bus. Some machine types create these HBAs automatically (despite * "-no-user-config -nodefaults"), some don't... *) let disk_cdrom_filter = function | BusSlotDisk _ | BusSlotRemovable { s_removable_type = CDROM } -> true | _ -> false and floppy_filter = -- 2.19.1.3.g30247aa5d201
If "s_cpu_model" is None and "gcaps_default_cpu" is "false", generate the "-cpu host" option for QEMU. "-cpu host" produces an (almost) exact copy of the host (i.e., physical) CPU for the guest, which is the best choice for guest OSes that cannot run on QEMU's default VCPU type -- considering that domains converted by virt-v2v are not expected to be migrateable without further tweaks by an administrator. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- output/output_qemu.ml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/output/output_qemu.ml b/output/output_qemu.ml index 385065f0579d..29adcba901a3 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -147,55 +147,56 @@ module QEMU = struct flag "-no-user-config"; flag "-nodefaults"; arg "-name" output_name; (match source.s_genid with | None -> () | Some genid -> arg_list "-device" ["vmgenid"; sprintf "guid=%s" genid; "id=vmgenid0"] ); arg_list "-machine" (machine_str :: (if smm then ["smm=on"] else []) @ ["accel=kvm:tcg"]); (match uefi_firmware with | None -> () | Some { Uefi.code } -> if secure_boot_required then arg_list "-global" ["driver=cfi.pflash01"; "property=secure"; "value=on"]; arg_list "-drive" ["if=pflash"; "format=raw"; "file=" ^ code; "readonly"]; arg_noquote "-drive" "if=pflash,format=raw,file=\"$uefi_vars\""; ); arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L)); - (match source.s_cpu_model with - | None -> () - | Some model -> arg "-cpu" model + (match source.s_cpu_model, guestcaps.gcaps_default_cpu with + | None, true -> () + | None, false -> arg "-cpu" "host" + | Some model, _ -> arg "-cpu" model ); if source.s_vcpu > 1 then ( (match source.s_cpu_topology with | None -> arg "-smp" (string_of_int source.s_vcpu) | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } -> let args = [ sprintf "cpus=%d" source.s_vcpu; sprintf "sockets=%d" s_cpu_sockets; sprintf "cores=%d" s_cpu_cores; sprintf "threads=%d" s_cpu_threads; ] in arg_list "-smp" args ); ); (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies, we * need host-bus adapters (HBAs) between these devices and the PCI(e) root * bus. Some machine types create these HBAs automatically (despite * "-no-user-config -nodefaults"), some don't... *) let disk_cdrom_filter = function | BusSlotDisk _ | BusSlotRemovable { s_removable_type = CDROM } -> true -- 2.19.1.3.g30247aa5d201
RHEL >= 9.0 on x86_64 requires the processor to support the "x86-64-v2" microarchitecture level, which the default QEMU VCPU model does not satisfy. Activate host-passthrough for such guest OSes. (Libosinfo does not track processor support, so in the future we may have to extend the expression added in this patch to other distros.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- convert/convert_linux.ml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 93b3922cac60..5660494973b1 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -139,65 +139,73 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = let { Libosinfo_utils.q35; vio10 } = Libosinfo_utils.os_support_of_osinfo_device_list devices in (if q35 then Q35 else I440FX), vio10 with | Not_found -> (* Pivot on the year 2007. Any Linux distro from earlier than 2007 * should use i440fx, anything 2007 or newer should use q35. *) (match inspect.i_distro, inspect.i_major_version with | "fedora", _ -> Q35 | ("rhel"|"centos"|"scientificlinux"|"redhat-based"|"oraclelinux"), major -> if major <= 4 then I440FX else Q35 | ("sles"|"suse-based"|"opensuse"), major -> if major < 10 then I440FX else Q35 | ("debian"|"ubuntu"|"linuxmint"|"kalilinux"), major -> if major < 4 then I440FX else Q35 (* reasonable default for all modern Linux kernels *) | _, _ -> Q35 ), true ) | _ -> Virt, true in + (* RHEL >= 9.0 on x86_64 requires the processor to support the "x86-64-v2" + * microarchitecture level, which the default QEMU VCPU model does not + * satisfy. Refer to RHBZ#2076013. + *) + let default_cpu_suffices = inspect.i_distro <> "rhel" || + inspect.i_arch <> "x86_64" || + inspect.i_major_version < 9 in + (* Return guest capabilities from the convert () function. *) let guestcaps = { gcaps_block_bus = block_type; gcaps_net_bus = net_type; gcaps_virtio_rng = kernel.ki_supports_virtio_rng; gcaps_virtio_balloon = kernel.ki_supports_virtio_balloon; gcaps_isa_pvpanic = kernel.ki_supports_isa_pvpanic; gcaps_virtio_socket = kernel.ki_supports_virtio_socket; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; gcaps_acpi = acpi; gcaps_virtio_1_0 = virtio_1_0; - gcaps_default_cpu = true; + gcaps_default_cpu = default_cpu_suffices; } in guestcaps and augeas_grub_configuration () = if bootloader#set_augeas_configuration () then Linux.augeas_reload g and unconfigure_xen () = (* Remove kmod-xenpv-* (RHEL 3). *) let xenmods = List.filter_map ( fun { G.app2_name = name } -> if name = "kmod-xenpv" || String.is_prefix name "kmod-xenpv-" then Some name else None ) inspect.i_apps in Linux.remove g inspect xenmods; (* Undo related nastiness if kmod-xenpv was installed. *) if xenmods <> [] then ( (* kmod-xenpv modules may have been manually copied to other kernels. * Hunt them down and destroy them. *) let dirs = g#find "/lib/modules" in -- 2.19.1.3.g30247aa5d201
For patches 4-9 (and so the whole series): Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 04/20/22 18:54, Richard W.M. Jones wrote:
For patches 4-9 (and so the whole series):
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Series merged as commit range e7539dc6f6d1..f28757c6d100, with the following difference on patch#1: 1: 70898075fb25 ! 1: a50b975024ac types: introduce the "gcaps_default_cpu" field @@ -18,6 +18,9 @@ Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013 Signed-off-by: Laszlo Ersek <lersek@redhat.com> + Message-Id: <20220420162333.24069-2-lersek@redhat.com> + [lersek@redhat.com: mention "-cpu qemu64" near the new field def (Rich)] + Reviewed-by: Richard W.M. Jones <rjones@redhat.com> diff --git a/lib/types.mli b/lib/types.mli --- a/lib/types.mli @@ -28,7 +31,8 @@ protocol level. *) + + gcaps_default_cpu : bool; -+ (** True iff the guest OS is capable of running on QEMU's default VCPU model. ++ (** True iff the guest OS is capable of running on QEMU's default VCPU model ++ (eg. "-cpu qemu64" with the Q35 and I440FX machine types). + + This capability only matters for the QEMU and libvirt output modules, + where, in case the capability is false *and* the source hypervisor does Thanks! Laszlo
On 04/20/22 18:23, Laszlo Ersek wrote:
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2076013
We don't specify a VCPU model in the destination domain description if the source description doesn't specify one. This is a problem for x86-64 RHEL-9.0+ guests, when using the QEMU or libvirt output modules. Those outputs default to the "qemu64" VCPU model in the absence of an explicit VCPU model specification, and "qemu64" does not satisfy the "x86-64-v2" microarchitecture level, which is required by RHEL-9.0+ guests. As a result, these guests crash during boot, after conversion.
The series recognizes guests that are unable to boot on the default QEMU VCPU model, and specifies host CPU passthrough for them in the QEMU and libvirt output modules.
Testing: <https://bugzilla.redhat.com/show_bug.cgi?id=2076013#c18>.
Thanks, Laszlo
Laszlo Ersek (9): types: introduce the "gcaps_default_cpu" field create_libvirt_xml: simplify match on (s_cpu_vendor, s_cpu_model) create_libvirt_xml: normalize match on (s_cpu_vendor, s_cpu_model) create_libvirt_xml: eliminate childless <cpu match="minimum"/> element create_libvirt_xml: restrict 'match="minimum"' <cpu> attribute production create_libvirt_xml: honor "gcaps_default_cpu" output_qemu: reflect source VCPU model to the QEMU command line output_qemu: honor "gcaps_default_cpu" convert_linux: set "gcaps_default_cpu = false" for x86_64 RHEL-9.0+ guests
convert/convert_linux.ml | 9 ++++++ convert/convert_windows.ml | 1 + lib/types.ml | 3 ++ lib/types.mli | 14 +++++++++ output/create_libvirt_xml.ml | 31 +++++++++++--------- output/output_qemu.ml | 7 +++++ 6 files changed, 51 insertions(+), 14 deletions(-)
base-commit: 8643970f9791b1c90dfd6a2dd1abfc9afef1fb52
The patches are also available here: Repo: https://github.com/lersek/virt-v2v.git Branch: vcpu-model-rhbz-2076013 Thanks Laszlo
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones