Libguestfs - Feb 2023 - [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

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:
> 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
>      );
>