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!
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/>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
