Laszlo Ersek
2023-Feb-17 07:53 UTC
[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 doesOne 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 > ); >
Richard W.M. Jones
2023-Feb-17 09:56 UTC
[Libguestfs] [PATCH v2v 1/3] v2v: Rename gcaps_default_cpu to gcaps_arch_min_version
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