On 2/15/23 15:12, 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: Dr. David Alan Gilbert, Daniel Berrangé
> ---
> output/create_libvirt_xml.ml | 7 ++++---
> tests/test-v2v-i-ova.xml | 1 +
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> index e9c6c8c150..6eb66f2dcb 100644
> --- a/output/create_libvirt_xml.ml
> +++ b/output/create_libvirt_xml.ml
> @@ -185,15 +185,13 @@ let create_libvirt_xml ?pool source inspect
> ];
>
> 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
>
> (match source.s_cpu_model with
> | None ->
> - if guestcaps.gcaps_arch_min_version >= 1 then
> - List.push_back cpu_attrs ("mode", "host-model");
> + List.push_back cpu_attrs ("mode", "host-model");
> | Some model ->
> List.push_back cpu_attrs ("match", "minimum");
> if model = "qemu64" then
> @@ -217,6 +215,9 @@ let create_libvirt_xml ?pool source inspect
> );
>
> List.push_back_list body [ e "cpu" !cpu_attrs !cpu ]
> + )
> + else (
> + List.push_back_list body [ e "cpu" [ "mode", "host-model" ] [] ]
> );
>
> let uefi_firmware =
In effect, we're hard-coding "gcaps_default_cpu = false" (or put
differently, we permanently assume "guestcaps.gcaps_arch_min_version >=
1").
Therefore the outer condition *as well* becomes constant true (not just
the inner condition), so the outer condition too can be removed, and the
dependent logic can be unnested. And the new "else" branch is unneeded.
(Put differently, with the outer condition removed (= hardwired to
"true"), the existent logic will already do what the new else branch
would.)
Expressed as a patch on top of current master (note this is *without*
"-b", the "-b" option's output I'm going to paste below):
> diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> index 60977cf5bba5..49661c08cf46 100644
> --- a/output/create_libvirt_xml.ml
> +++ b/output/create_libvirt_xml.ml
> @@ -184,40 +184,33 @@ let create_libvirt_xml ?pool source inspect
> e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
> ];
>
> - if source.s_cpu_model <> None ||
> - not guestcaps.gcaps_default_cpu ||
> - 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
> - 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 ]
> + let cpu_attrs = ref []
> + and cpu = ref [] in
> + (match source.s_cpu_model with
> + | None ->
> + 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 ];
>
> let uefi_firmware =
> match target_firmware with
And with "git diff -b":
> diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> index 60977cf5bba5..49661c08cf46 100644
> --- a/output/create_libvirt_xml.ml
> +++ b/output/create_libvirt_xml.ml
> @@ -184,15 +184,10 @@ let create_libvirt_xml ?pool source inspect
> e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
> ];
>
> - if source.s_cpu_model <> None ||
> - not guestcaps.gcaps_default_cpu ||
> - 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
> List.push_back cpu_attrs ("mode", "host-model");
> | Some model ->
> List.push_back cpu_attrs ("match", "minimum");
> @@ -215,9 +210,7 @@ let create_libvirt_xml ?pool source inspect
> ] in
> List.push_back cpu (e "topology" topology_attrs [])
> );
> -
> - List.push_back_list body [ e "cpu" !cpu_attrs !cpu ]
> - );
> + List.push_back_list body [ e "cpu" !cpu_attrs !cpu ];
>
> let uefi_firmware =
> match target_firmware with
This formatting makes it clear that we'll always output a <cpu> element
now.
And then the test case update is OK of course:
On 2/15/23 15:12, Richard W.M. Jones wrote:
> 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/>
Thanks,
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
