On Mon, Feb 20, 2023 at 11:30:54AM +0100, Laszlo Ersek wrote:
> On 2/17/23 12:44, 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: Laszlo Ersek, Dr. David Alan Gilbert, Daniel Berrangé
> > ---
> > output/create_libvirt_xml.ml | 59 +++++++++++++++++-------------------
> > tests/test-v2v-i-ova.xml | 1 +
> > 2 files changed, 28 insertions(+), 32 deletions(-)
> >
> > diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> > index e9c6c8c150..1d77139b45 100644
> > --- a/output/create_libvirt_xml.ml
> > +++ b/output/create_libvirt_xml.ml
> > @@ -184,41 +184,36 @@ let create_libvirt_xml ?pool source inspect
> > e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
> > ];
> >
> > - 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
> > + 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");
> > - | 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 ]
> > + (match source.s_cpu_model with
> > + | None ->
> > + List.push_back cpu_attrs ("mode", "host-model");
>
> The indentation of "List.push_back" is off by one space here, as far as
> I can tell (only 1 space used instead of 2). If possible, please fix it
> up when you push the patches.
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks - fixed the indentation in my local copy.
Rich.
> Thanks!
> Laszlo
>
>
>
> > + | 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
> > | TargetBIOS -> None
> > 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/>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
