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

2023-02-20 Thread Richard W.M. Jones 
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 
> > 
> > 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 

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 @@
> >2097152
> >2097152
> >1
> > +  
> >
> >  
> >  

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit -

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

2023-02-20 Thread Laszlo Ersek 
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 
> 
> 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 

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 @@
>2097152
>2097152
>1
> +  
>
>  
>  

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-02-17 Thread Richard W.M. Jones 
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 

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");
+   | 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 @@
   2097152
   2097152
   1
+  
   
 
 
-- 
2.39.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs