Re: [Libguestfs] [PATCH v2v 1/3] v2v: Rename gcaps_default_cpu to gcaps_arch_min_version

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