Re: [Libguestfs] [PATCH v2v v2 2/3] -o libvirt: Always use host-model unless overridden by source hypervisor

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 <lersek(a)redhat.com&gt;

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/>