[PATCH v2v 1/3] v2v: Rename gcaps_default_cpu to gcaps_arch_min_version
Some guests require not just a specific architecture, but cannot run on qemu's default CPU model, eg. requiring x86_64-v2. Since we anticipate future guests requiring higher versions, let's encode the minimum architecture version instead of a simple boolean. This patch essentially just remaps: gcaps_default_cpu = true => gcaps_arch_min_version = 0 gcaps_default_cpu = false => gcaps_arch_min_version = 2 and updates the comments. Updates: commit a50b975024ac5e46e107882e27fea498bf425685 --- lib/types.mli | 19 +++++++++++-------- convert/convert_linux.ml | 9 +++++---- convert/convert_windows.ml | 2 +- lib/types.ml | 6 +++--- output/create_libvirt_xml.ml | 4 ++-- output/output_qemu.ml | 6 +++--- 6 files changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/types.mli b/lib/types.mli index 24a93760cf..4a2bb5b371 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -263,24 +263,27 @@ type guestcaps = { gcaps_machine : guestcaps_machine; (** Machine model. *) gcaps_arch : string; (** Architecture that KVM must emulate. *) - gcaps_virtio_1_0 : bool; - (** The guest supports the virtio devices that it does at the virtio-1.0 - protocol level. *) + gcaps_arch_min_version : int; + (** Some guest OSes require not just a specific architecture, but a + minimum version. Notably RHEL >= 9 requires at least x86_64-v2. - gcaps_default_cpu : bool; - (** 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). + If the guest is capable of running on QEMU's default VCPU model + for the architecture then this is set to [0]. This capability only matters for the QEMU and libvirt output modules, - where, in case the capability is false *and* the source hypervisor does + where, in case the capability is false {b 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.) + specify host-model.) 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. *) + + gcaps_virtio_1_0 : bool; + (** The guest supports the virtio devices that it does at the virtio-1.0 + protocol level. *) } (** Guest capabilities after conversion. eg. Was virtio found or installed? *) diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 460cbff0ed..d5c0f24dbb 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = * microarchitecture level, which the default QEMU VCPU model does not * satisfy. Refer to RHBZ#2076013 RHBZ#2166619. *) - let default_cpu_suffices = family <> `RHEL_family || - inspect.i_arch <> "x86_64" || - inspect.i_major_version < 9 in + let arch_min_version = + if family <> `RHEL_family || inspect.i_arch <> "x86_64" || + inspect.i_major_version < 9 + then 0 else 2 in (* Return guest capabilities from the convert () function. *) let guestcaps = { @@ -217,8 +218,8 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = gcaps_virtio_socket = kernel.ki_supports_virtio_socket; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = arch_min_version; gcaps_virtio_1_0 = virtio_1_0; - gcaps_default_cpu = default_cpu_suffices; } in guestcaps diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 8d3737995f..9d8d271d05 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -265,8 +265,8 @@ let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = gcaps_machine = of_virtio_win_machine_type virtio_win_installed.Inject_virtio_win.machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = 0; gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0; - gcaps_default_cpu = true; } in guestcaps diff --git a/lib/types.ml b/lib/types.ml index 98f8bc6fa5..6c019ae130 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -397,8 +397,8 @@ type guestcaps = { gcaps_virtio_socket : bool; gcaps_machine : guestcaps_machine; gcaps_arch : string; + gcaps_arch_min_version : int; gcaps_virtio_1_0 : bool; - gcaps_default_cpu : bool; } and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 @@ -426,8 +426,8 @@ let string_of_guestcaps gcaps = gcaps_virtio_socket = %b\n\ gcaps_machine = %s\n\ gcaps_arch = %s\n\ + gcaps_arch_min_version = %d\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) @@ -437,8 +437,8 @@ let string_of_guestcaps gcaps = gcaps.gcaps_virtio_socket (string_of_machine gcaps.gcaps_machine) gcaps.gcaps_arch + gcaps.gcaps_arch_min_version gcaps.gcaps_virtio_1_0 - gcaps.gcaps_default_cpu type target_buses = { target_virtio_blk_bus : target_bus_slot array; diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 60977cf5bb..e9c6c8c150 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -185,14 +185,14 @@ let create_libvirt_xml ?pool source inspect ]; if source.s_cpu_model <> None || - not guestcaps.gcaps_default_cpu || + guestcaps.gcaps_arch_min_version >= 1 || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in (match source.s_cpu_model with | None -> - if not guestcaps.gcaps_default_cpu then + if guestcaps.gcaps_arch_min_version >= 1 then List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); diff --git a/output/output_qemu.ml b/output/output_qemu.ml index b667e782ed..491906ebf9 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,9 +175,9 @@ module QEMU = struct arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L)); - (match source.s_cpu_model, guestcaps.gcaps_default_cpu with - | None, true -> () - | None, false -> arg "-cpu" "host" + (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with + | None, 0 -> () + | None, _ -> arg "-cpu" "host" | Some model, _ -> arg "-cpu" model ); -- 2.39.0
In the case where the source hypervisor doesn't specify a CPU model, previously we chose qemu64 (qemu's most basic model), except for a few guests that we know won't work on qemu64, eg. RHEL 9 requires x86_64-v2 where we use <cpu mode='host-model'/> However we recently encountered an obscure KVM bug related to this (https://bugzilla.redhat.com/show_bug.cgi?id=2168082). Windows 11 thinks the qemu64 CPU model when booted on real AMD Milan is an AMD Phenom and tried to apply an ancient erratum to it. Since KVM didn't emulate the correct MSRs for this it caused the guest to fail to boot. After discussion upstream we can't see any reason not to give all guests host-model. This should fix the bug above and also generally improve performance by allowing the guest to exploit all host features. Related: https://bugzilla.redhat.com/show_bug.cgi?id=2168082#c19 Related: https://listman.redhat.com/archives/libguestfs/2023-February/030624.html Thanks: Dr. David Alan Gilbert, Daniel Berrangé --- output/create_libvirt_xml.ml | 7 ++++--- tests/test-v2v-i-ova.xml | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index e9c6c8c150..6eb66f2dcb 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -185,15 +185,13 @@ let create_libvirt_xml ?pool source inspect ]; if source.s_cpu_model <> None || - guestcaps.gcaps_arch_min_version >= 1 || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in (match source.s_cpu_model with | None -> - if guestcaps.gcaps_arch_min_version >= 1 then - List.push_back cpu_attrs ("mode", "host-model"); + List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); if model = "qemu64" then @@ -217,6 +215,9 @@ let create_libvirt_xml ?pool source inspect ); List.push_back_list body [ e "cpu" !cpu_attrs !cpu ] + ) + else ( + List.push_back_list body [ e "cpu" [ "mode", "host-model" ] [] ] ); let uefi_firmware = diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml index da1db473e5..e5907ea1cc 100644 --- a/tests/test-v2v-i-ova.xml +++ b/tests/test-v2v-i-ova.xml @@ -10,6 +10,7 @@ <memory unit='KiB'>2097152</memory> <currentMemory unit='KiB'>2097152</currentMemory> <vcpu>1</vcpu> + <cpu mode='host-model'/> <features> <acpi/> <apic/> -- 2.39.0
On 2/15/23 15:12, Richard W.M. Jones wrote:
In the case where the source hypervisor doesn't specify a CPU model, previously we chose qemu64 (qemu's most basic model), except for a few guests that we know won't work on qemu64, eg. RHEL 9 requires x86_64-v2 where we use <cpu mode='host-model'/>
However we recently encountered an obscure KVM bug related to this (https://bugzilla.redhat.com/show_bug.cgi?id=2168082). Windows 11 thinks the qemu64 CPU model when booted on real AMD Milan is an AMD Phenom and tried to apply an ancient erratum to it. Since KVM didn't emulate the correct MSRs for this it caused the guest to fail to boot.
After discussion upstream we can't see any reason not to give all guests host-model. This should fix the bug above and also generally improve performance by allowing the guest to exploit all host features.
Related: https://bugzilla.redhat.com/show_bug.cgi?id=2168082#c19 Related: https://listman.redhat.com/archives/libguestfs/2023-February/030624.html Thanks: Dr. David Alan Gilbert, Daniel Berrangé --- output/create_libvirt_xml.ml | 7 ++++--- tests/test-v2v-i-ova.xml | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index e9c6c8c150..6eb66f2dcb 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -185,15 +185,13 @@ let create_libvirt_xml ?pool source inspect ];
if source.s_cpu_model <> None || - guestcaps.gcaps_arch_min_version >= 1 || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in
(match source.s_cpu_model with | None -> - if guestcaps.gcaps_arch_min_version >= 1 then - List.push_back cpu_attrs ("mode", "host-model"); + List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); if model = "qemu64" then @@ -217,6 +215,9 @@ let create_libvirt_xml ?pool source inspect );
List.push_back_list body [ e "cpu" !cpu_attrs !cpu ] + ) + else ( + List.push_back_list body [ e "cpu" [ "mode", "host-model" ] [] ] );
let uefi_firmware =
In effect, we're hard-coding "gcaps_default_cpu = false" (or put differently, we permanently assume "guestcaps.gcaps_arch_min_version >= 1"). Therefore the outer condition *as well* becomes constant true (not just the inner condition), so the outer condition too can be removed, and the dependent logic can be unnested. And the new "else" branch is unneeded. (Put differently, with the outer condition removed (= hardwired to "true"), the existent logic will already do what the new else branch would.) Expressed as a patch on top of current master (note this is *without* "-b", the "-b" option's output I'm going to paste below):
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 60977cf5bba5..49661c08cf46 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -184,40 +184,33 @@ let create_libvirt_xml ?pool source inspect 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 -> - if not guestcaps.gcaps_default_cpu then - List.push_back cpu_attrs ("mode", "host-model"); - | Some model -> - List.push_back cpu_attrs ("match", "minimum"); - if model = "qemu64" then - List.push_back cpu_attrs ("check", "none"); - (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 cpu_attrs = ref [] + and cpu = ref [] in + (match source.s_cpu_model with + | None -> + List.push_back cpu_attrs ("mode", "host-model"); + | Some model -> + List.push_back cpu_attrs ("match", "minimum"); + if model = "qemu64" then + List.push_back cpu_attrs ("check", "none"); + (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
And with "git diff -b":
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 60977cf5bba5..49661c08cf46 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -184,15 +184,10 @@ let create_libvirt_xml ?pool source inspect 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 -> - if not guestcaps.gcaps_default_cpu then List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); @@ -215,9 +210,7 @@ let create_libvirt_xml ?pool source inspect ] in List.push_back cpu (e "topology" topology_attrs []) ); - - List.push_back_list body [ e "cpu" !cpu_attrs !cpu ] - ); + List.push_back_list body [ e "cpu" !cpu_attrs !cpu ];
let uefi_firmware = match target_firmware with
This formatting makes it clear that we'll always output a <cpu> element now. And then the test case update is OK of course: On 2/15/23 15:12, Richard W.M. Jones wrote:
diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml index da1db473e5..e5907ea1cc 100644 --- a/tests/test-v2v-i-ova.xml +++ b/tests/test-v2v-i-ova.xml @@ -10,6 +10,7 @@ <memory unit='KiB'>2097152</memory> <currentMemory unit='KiB'>2097152</currentMemory> <vcpu>1</vcpu> + <cpu mode='host-model'/> <features> <acpi/> <apic/>
Thanks, Laszlo
As with the prior commit, prefer -cpu host for all guests (except when we have more information from the source hypervisor). Although there is the disadvantage that -cpu host is non-migratable, in practice it would be very difficult to live migrate a host launched using direct qemu commands. Note that after this change, gcaps_arch_min_version is basically an informational field. No output uses it, but it will appear in debug output and there's the possibility we might use it for a future output mode. --- output/output_qemu.ml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/output/output_qemu.ml b/output/output_qemu.ml index 491906ebf9..d1028a9cc3 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,10 +175,9 @@ module QEMU = struct arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L)); - (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with - | None, 0 -> () - | None, _ -> arg "-cpu" "host" - | Some model, _ -> arg "-cpu" model + (match source.s_cpu_model with + | None -> arg "-cpu" "host" + | Some model -> arg "-cpu" model ); if source.s_vcpu > 1 then ( -- 2.39.0
On 2/15/23 15:12, Richard W.M. Jones wrote:
As with the prior commit, prefer -cpu host for all guests (except when we have more information from the source hypervisor). Although there is the disadvantage that -cpu host is non-migratable, in practice it would be very difficult to live migrate a host launched using direct qemu commands.
Note that after this change, gcaps_arch_min_version is basically an informational field. No output uses it, but it will appear in debug output and there's the possibility we might use it for a future output mode. --- output/output_qemu.ml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/output/output_qemu.ml b/output/output_qemu.ml index 491906ebf9..d1028a9cc3 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,10 +175,9 @@ module QEMU = struct
arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
- (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with - | None, 0 -> () - | None, _ -> arg "-cpu" "host" - | Some model, _ -> arg "-cpu" model + (match source.s_cpu_model with + | None -> arg "-cpu" "host" + | Some model -> arg "-cpu" model );
if source.s_vcpu > 1 then (
Reviewed-by: Laszlo Ersek <lersek@redhat.com> (again in expectation of this being squashed / reworked into the sole version-2 patch)
On 2/17/23 08:55, Laszlo Ersek wrote:
On 2/15/23 15:12, Richard W.M. Jones wrote:
As with the prior commit, prefer -cpu host for all guests (except when we have more information from the source hypervisor). Although there is the disadvantage that -cpu host is non-migratable, in practice it would be very difficult to live migrate a host launched using direct qemu commands.
Note that after this change, gcaps_arch_min_version is basically an informational field. No output uses it, but it will appear in debug output and there's the possibility we might use it for a future output mode. --- output/output_qemu.ml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/output/output_qemu.ml b/output/output_qemu.ml index 491906ebf9..d1028a9cc3 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,10 +175,9 @@ module QEMU = struct
arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
- (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with - | None, 0 -> () - | None, _ -> arg "-cpu" "host" - | Some model, _ -> arg "-cpu" model + (match source.s_cpu_model with + | None -> arg "-cpu" "host" + | Some model -> arg "-cpu" model );
if source.s_vcpu > 1 then (
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(again in expectation of this being squashed / reworked into the sole version-2 patch)
We could even factor out the now-common 'arg "-cpu"' part, and push the "match" down into that. Laszlo
On 2/15/23 15:12, Richard W.M. Jones wrote:
Some guests require not just a specific architecture, but cannot run on qemu's default CPU model, eg. requiring x86_64-v2. Since we anticipate future guests requiring higher versions, let's encode the minimum architecture version instead of a simple boolean.
This patch essentially just remaps:
gcaps_default_cpu = true => gcaps_arch_min_version = 0 gcaps_default_cpu = false => gcaps_arch_min_version = 2
and updates the comments.
Updates: commit a50b975024ac5e46e107882e27fea498bf425685 --- lib/types.mli | 19 +++++++++++-------- convert/convert_linux.ml | 9 +++++---- convert/convert_windows.ml | 2 +- lib/types.ml | 6 +++--- output/create_libvirt_xml.ml | 4 ++-- output/output_qemu.ml | 6 +++--- 6 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/types.mli b/lib/types.mli index 24a93760cf..4a2bb5b371 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -263,24 +263,27 @@ type guestcaps = { gcaps_machine : guestcaps_machine; (** Machine model. *) gcaps_arch : string; (** Architecture that KVM must emulate. *)
- gcaps_virtio_1_0 : bool; - (** The guest supports the virtio devices that it does at the virtio-1.0 - protocol level. *) + gcaps_arch_min_version : int; + (** Some guest OSes require not just a specific architecture, but a + minimum version. Notably RHEL >= 9 requires at least x86_64-v2.
- gcaps_default_cpu : bool; - (** 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). + If the guest is capable of running on QEMU's default VCPU model + for the architecture then this is set to [0].
This capability only matters for the QEMU and libvirt output modules, - where, in case the capability is false *and* the source hypervisor does + where, in case the capability is false {b and} the source hypervisor does
One omission and one typo here: (1a) "false" should be replaced with "nonzero", (1b) there is a superfluous space character before "{b and}".
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.) + specify host-model.)
Hmm yes this is probably a leftover from commit 12473bfcb749. (2) However, I think we can be a bit clearer here: we specify host-model for libvirt, but host-passthrough for QEMU. (3) ... after reading through the series though, I've come to the same conclusion that's stated in commit message #3: gcaps_arch_min_version becomes effectively superfluous. I'd rather suggest removing it altogether then! I think that will reduce the series to a single patch, which in this particular case is good, I feel. I'll have some more comments on the other patches (I figure the updates from those will be squashed into the single version-2 patch, so they remain relevant). Laszlo
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. *) + + gcaps_virtio_1_0 : bool; + (** The guest supports the virtio devices that it does at the virtio-1.0 + protocol level. *) } (** Guest capabilities after conversion. eg. Was virtio found or installed? *)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 460cbff0ed..d5c0f24dbb 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = * microarchitecture level, which the default QEMU VCPU model does not * satisfy. Refer to RHBZ#2076013 RHBZ#2166619. *) - let default_cpu_suffices = family <> `RHEL_family || - inspect.i_arch <> "x86_64" || - inspect.i_major_version < 9 in + let arch_min_version = + if family <> `RHEL_family || inspect.i_arch <> "x86_64" || + inspect.i_major_version < 9 + then 0 else 2 in
(* Return guest capabilities from the convert () function. *) let guestcaps = { @@ -217,8 +218,8 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = gcaps_virtio_socket = kernel.ki_supports_virtio_socket; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = arch_min_version; gcaps_virtio_1_0 = virtio_1_0; - gcaps_default_cpu = default_cpu_suffices; } in
guestcaps diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 8d3737995f..9d8d271d05 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -265,8 +265,8 @@ let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = gcaps_machine = of_virtio_win_machine_type virtio_win_installed.Inject_virtio_win.machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = 0; gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0; - gcaps_default_cpu = true; } in
guestcaps diff --git a/lib/types.ml b/lib/types.ml index 98f8bc6fa5..6c019ae130 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -397,8 +397,8 @@ type guestcaps = { gcaps_virtio_socket : bool; gcaps_machine : guestcaps_machine; gcaps_arch : string; + gcaps_arch_min_version : int; gcaps_virtio_1_0 : bool; - gcaps_default_cpu : bool; } and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 @@ -426,8 +426,8 @@ let string_of_guestcaps gcaps = gcaps_virtio_socket = %b\n\ gcaps_machine = %s\n\ gcaps_arch = %s\n\ + gcaps_arch_min_version = %d\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) @@ -437,8 +437,8 @@ let string_of_guestcaps gcaps = gcaps.gcaps_virtio_socket (string_of_machine gcaps.gcaps_machine) gcaps.gcaps_arch + gcaps.gcaps_arch_min_version gcaps.gcaps_virtio_1_0 - gcaps.gcaps_default_cpu
type target_buses = { target_virtio_blk_bus : target_bus_slot array; diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 60977cf5bb..e9c6c8c150 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -185,14 +185,14 @@ let create_libvirt_xml ?pool source inspect ];
if source.s_cpu_model <> None || - not guestcaps.gcaps_default_cpu || + guestcaps.gcaps_arch_min_version >= 1 || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in
(match source.s_cpu_model with | None -> - if not guestcaps.gcaps_default_cpu then + if guestcaps.gcaps_arch_min_version >= 1 then List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); diff --git a/output/output_qemu.ml b/output/output_qemu.ml index b667e782ed..491906ebf9 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,9 +175,9 @@ module QEMU = struct
arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
- (match source.s_cpu_model, guestcaps.gcaps_default_cpu with - | None, true -> () - | None, false -> arg "-cpu" "host" + (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with + | None, 0 -> () + | None, _ -> arg "-cpu" "host" | Some model, _ -> arg "-cpu" model );
On Fri, Feb 17, 2023 at 08:53:42AM +0100, Laszlo Ersek wrote:
On 2/15/23 15:12, Richard W.M. Jones wrote:
Some guests require not just a specific architecture, but cannot run on qemu's default CPU model, eg. requiring x86_64-v2. Since we anticipate future guests requiring higher versions, let's encode the minimum architecture version instead of a simple boolean.
This patch essentially just remaps:
gcaps_default_cpu = true => gcaps_arch_min_version = 0 gcaps_default_cpu = false => gcaps_arch_min_version = 2
and updates the comments.
Updates: commit a50b975024ac5e46e107882e27fea498bf425685 --- lib/types.mli | 19 +++++++++++-------- convert/convert_linux.ml | 9 +++++---- convert/convert_windows.ml | 2 +- lib/types.ml | 6 +++--- output/create_libvirt_xml.ml | 4 ++-- output/output_qemu.ml | 6 +++--- 6 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/types.mli b/lib/types.mli index 24a93760cf..4a2bb5b371 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -263,24 +263,27 @@ type guestcaps = { gcaps_machine : guestcaps_machine; (** Machine model. *) gcaps_arch : string; (** Architecture that KVM must emulate. *)
- gcaps_virtio_1_0 : bool; - (** The guest supports the virtio devices that it does at the virtio-1.0 - protocol level. *) + gcaps_arch_min_version : int; + (** Some guest OSes require not just a specific architecture, but a + minimum version. Notably RHEL >= 9 requires at least x86_64-v2.
- gcaps_default_cpu : bool; - (** 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). + If the guest is capable of running on QEMU's default VCPU model + for the architecture then this is set to [0].
This capability only matters for the QEMU and libvirt output modules, - where, in case the capability is false *and* the source hypervisor does + where, in case the capability is false {b and} the source hypervisor does
One omission and one typo here:
(1a) "false" should be replaced with "nonzero", (1b) there is a superfluous space character before "{b and}".
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.) + specify host-model.)
Hmm yes this is probably a leftover from commit 12473bfcb749.
(2) However, I think we can be a bit clearer here: we specify host-model for libvirt, but host-passthrough for QEMU.
(3) ... after reading through the series though, I've come to the same conclusion that's stated in commit message #3: gcaps_arch_min_version becomes effectively superfluous. I'd rather suggest removing it altogether then! I think that will reduce the series to a single patch, which in this particular case is good, I feel.
Do you think it should be kept for informational purposes? I worry about losing this information for other outputs where it might matter in future. Also we can easily delete it if we find we never need it. Rich.
I'll have some more comments on the other patches (I figure the updates from those will be squashed into the single version-2 patch, so they remain relevant).
Laszlo
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. *) + + gcaps_virtio_1_0 : bool; + (** The guest supports the virtio devices that it does at the virtio-1.0 + protocol level. *) } (** Guest capabilities after conversion. eg. Was virtio found or installed? *)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 460cbff0ed..d5c0f24dbb 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = * microarchitecture level, which the default QEMU VCPU model does not * satisfy. Refer to RHBZ#2076013 RHBZ#2166619. *) - let default_cpu_suffices = family <> `RHEL_family || - inspect.i_arch <> "x86_64" || - inspect.i_major_version < 9 in + let arch_min_version = + if family <> `RHEL_family || inspect.i_arch <> "x86_64" || + inspect.i_major_version < 9 + then 0 else 2 in
(* Return guest capabilities from the convert () function. *) let guestcaps = { @@ -217,8 +218,8 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = gcaps_virtio_socket = kernel.ki_supports_virtio_socket; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = arch_min_version; gcaps_virtio_1_0 = virtio_1_0; - gcaps_default_cpu = default_cpu_suffices; } in
guestcaps diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 8d3737995f..9d8d271d05 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -265,8 +265,8 @@ let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = gcaps_machine = of_virtio_win_machine_type virtio_win_installed.Inject_virtio_win.machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = 0; gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0; - gcaps_default_cpu = true; } in
guestcaps diff --git a/lib/types.ml b/lib/types.ml index 98f8bc6fa5..6c019ae130 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -397,8 +397,8 @@ type guestcaps = { gcaps_virtio_socket : bool; gcaps_machine : guestcaps_machine; gcaps_arch : string; + gcaps_arch_min_version : int; gcaps_virtio_1_0 : bool; - gcaps_default_cpu : bool; } and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 @@ -426,8 +426,8 @@ let string_of_guestcaps gcaps = gcaps_virtio_socket = %b\n\ gcaps_machine = %s\n\ gcaps_arch = %s\n\ + gcaps_arch_min_version = %d\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) @@ -437,8 +437,8 @@ let string_of_guestcaps gcaps = gcaps.gcaps_virtio_socket (string_of_machine gcaps.gcaps_machine) gcaps.gcaps_arch + gcaps.gcaps_arch_min_version gcaps.gcaps_virtio_1_0 - gcaps.gcaps_default_cpu
type target_buses = { target_virtio_blk_bus : target_bus_slot array; diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 60977cf5bb..e9c6c8c150 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -185,14 +185,14 @@ let create_libvirt_xml ?pool source inspect ];
if source.s_cpu_model <> None || - not guestcaps.gcaps_default_cpu || + guestcaps.gcaps_arch_min_version >= 1 || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in
(match source.s_cpu_model with | None -> - if not guestcaps.gcaps_default_cpu then + if guestcaps.gcaps_arch_min_version >= 1 then List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); diff --git a/output/output_qemu.ml b/output/output_qemu.ml index b667e782ed..491906ebf9 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,9 +175,9 @@ module QEMU = struct
arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
- (match source.s_cpu_model, guestcaps.gcaps_default_cpu with - | None, true -> () - | None, false -> arg "-cpu" "host" + (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with + | None, 0 -> () + | None, _ -> arg "-cpu" "host" | Some model, _ -> arg "-cpu" model );
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 2/17/23 10:56, Richard W.M. Jones wrote:
On Fri, Feb 17, 2023 at 08:53:42AM +0100, Laszlo Ersek wrote:
On 2/15/23 15:12, Richard W.M. Jones wrote:
Some guests require not just a specific architecture, but cannot run on qemu's default CPU model, eg. requiring x86_64-v2. Since we anticipate future guests requiring higher versions, let's encode the minimum architecture version instead of a simple boolean.
This patch essentially just remaps:
gcaps_default_cpu = true => gcaps_arch_min_version = 0 gcaps_default_cpu = false => gcaps_arch_min_version = 2
and updates the comments.
Updates: commit a50b975024ac5e46e107882e27fea498bf425685 --- lib/types.mli | 19 +++++++++++-------- convert/convert_linux.ml | 9 +++++---- convert/convert_windows.ml | 2 +- lib/types.ml | 6 +++--- output/create_libvirt_xml.ml | 4 ++-- output/output_qemu.ml | 6 +++--- 6 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/types.mli b/lib/types.mli index 24a93760cf..4a2bb5b371 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -263,24 +263,27 @@ type guestcaps = { gcaps_machine : guestcaps_machine; (** Machine model. *) gcaps_arch : string; (** Architecture that KVM must emulate. *)
- gcaps_virtio_1_0 : bool; - (** The guest supports the virtio devices that it does at the virtio-1.0 - protocol level. *) + gcaps_arch_min_version : int; + (** Some guest OSes require not just a specific architecture, but a + minimum version. Notably RHEL >= 9 requires at least x86_64-v2.
- gcaps_default_cpu : bool; - (** 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). + If the guest is capable of running on QEMU's default VCPU model + for the architecture then this is set to [0].
This capability only matters for the QEMU and libvirt output modules, - where, in case the capability is false *and* the source hypervisor does + where, in case the capability is false {b and} the source hypervisor does
One omission and one typo here:
(1a) "false" should be replaced with "nonzero", (1b) there is a superfluous space character before "{b and}".
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.) + specify host-model.)
Hmm yes this is probably a leftover from commit 12473bfcb749.
(2) However, I think we can be a bit clearer here: we specify host-model for libvirt, but host-passthrough for QEMU.
(3) ... after reading through the series though, I've come to the same conclusion that's stated in commit message #3: gcaps_arch_min_version becomes effectively superfluous. I'd rather suggest removing it altogether then! I think that will reduce the series to a single patch, which in this particular case is good, I feel.
Do you think it should be kept for informational purposes? I worry about losing this information for other outputs where it might matter in future. Also we can easily delete it if we find we never need it.
Hm, OK, if it seems reasonable that other outputs might need it in the near (?) future, then it might mean less churn to keep it now. Also it doesn't incur big maintenance costs. Laszlo
Rich.
I'll have some more comments on the other patches (I figure the updates from those will be squashed into the single version-2 patch, so they remain relevant).
Laszlo
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. *) + + gcaps_virtio_1_0 : bool; + (** The guest supports the virtio devices that it does at the virtio-1.0 + protocol level. *) } (** Guest capabilities after conversion. eg. Was virtio found or installed? *)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 460cbff0ed..d5c0f24dbb 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = * microarchitecture level, which the default QEMU VCPU model does not * satisfy. Refer to RHBZ#2076013 RHBZ#2166619. *) - let default_cpu_suffices = family <> `RHEL_family || - inspect.i_arch <> "x86_64" || - inspect.i_major_version < 9 in + let arch_min_version = + if family <> `RHEL_family || inspect.i_arch <> "x86_64" || + inspect.i_major_version < 9 + then 0 else 2 in
(* Return guest capabilities from the convert () function. *) let guestcaps = { @@ -217,8 +218,8 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = gcaps_virtio_socket = kernel.ki_supports_virtio_socket; gcaps_machine = machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = arch_min_version; gcaps_virtio_1_0 = virtio_1_0; - gcaps_default_cpu = default_cpu_suffices; } in
guestcaps diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 8d3737995f..9d8d271d05 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -265,8 +265,8 @@ let convert (g : G.guestfs) _ inspect i_firmware _ static_ips = gcaps_machine = of_virtio_win_machine_type virtio_win_installed.Inject_virtio_win.machine; gcaps_arch = Utils.kvm_arch inspect.i_arch; + gcaps_arch_min_version = 0; gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0; - gcaps_default_cpu = true; } in
guestcaps diff --git a/lib/types.ml b/lib/types.ml index 98f8bc6fa5..6c019ae130 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -397,8 +397,8 @@ type guestcaps = { gcaps_virtio_socket : bool; gcaps_machine : guestcaps_machine; gcaps_arch : string; + gcaps_arch_min_version : int; gcaps_virtio_1_0 : bool; - gcaps_default_cpu : bool; } and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 @@ -426,8 +426,8 @@ let string_of_guestcaps gcaps = gcaps_virtio_socket = %b\n\ gcaps_machine = %s\n\ gcaps_arch = %s\n\ + gcaps_arch_min_version = %d\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) @@ -437,8 +437,8 @@ let string_of_guestcaps gcaps = gcaps.gcaps_virtio_socket (string_of_machine gcaps.gcaps_machine) gcaps.gcaps_arch + gcaps.gcaps_arch_min_version gcaps.gcaps_virtio_1_0 - gcaps.gcaps_default_cpu
type target_buses = { target_virtio_blk_bus : target_bus_slot array; diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml index 60977cf5bb..e9c6c8c150 100644 --- a/output/create_libvirt_xml.ml +++ b/output/create_libvirt_xml.ml @@ -185,14 +185,14 @@ let create_libvirt_xml ?pool source inspect ];
if source.s_cpu_model <> None || - not guestcaps.gcaps_default_cpu || + guestcaps.gcaps_arch_min_version >= 1 || source.s_cpu_topology <> None then ( let cpu_attrs = ref [] and cpu = ref [] in
(match source.s_cpu_model with | None -> - if not guestcaps.gcaps_default_cpu then + if guestcaps.gcaps_arch_min_version >= 1 then List.push_back cpu_attrs ("mode", "host-model"); | Some model -> List.push_back cpu_attrs ("match", "minimum"); diff --git a/output/output_qemu.ml b/output/output_qemu.ml index b667e782ed..491906ebf9 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -175,9 +175,9 @@ module QEMU = struct
arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
- (match source.s_cpu_model, guestcaps.gcaps_default_cpu with - | None, true -> () - | None, false -> arg "-cpu" "host" + (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with + | None, 0 -> () + | None, _ -> arg "-cpu" "host" | Some model, _ -> arg "-cpu" model );
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones