[libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models

Jiri Denemark posted 11 patches 3 years, 4 months ago
There is a newer version of this series
[libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Jiri Denemark 3 years, 4 months ago
We already show whether a specific CPU model is usable on the current
host without modification via the 'usable' attribute of each CPU model.
But it may be useful to actually see what features are blocking each CPU
model from being usable. Especially when we already fetch the info from
QEMU and propagating it to domain capabilities XML is all we need to do.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 docs/formatdomaincaps.rst                     | 16 ++--
 src/conf/domain_capabilities.c                |  7 ++
 src/conf/schemas/domaincaps.rng               |  5 ++
 .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml  | 36 ++++-----
 .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml  | 72 +++++++++---------
 tests/domaincapsdata/qemu_4.2.0.s390x.xml     |  2 +-
 tests/domaincapsdata/qemu_4.2.0.x86_64.xml    | 36 ++++-----
 .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml  | 40 +++++-----
 .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml  | 72 +++++++++---------
 tests/domaincapsdata/qemu_5.0.0.x86_64.xml    | 40 +++++-----
 .../domaincapsdata/qemu_5.1.0-q35.x86_64.xml  | 68 ++++++++---------
 .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml  | 72 +++++++++---------
 tests/domaincapsdata/qemu_5.1.0.x86_64.xml    | 68 ++++++++---------
 .../domaincapsdata/qemu_5.2.0-q35.x86_64.xml  | 68 ++++++++---------
 .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml  | 72 +++++++++---------
 tests/domaincapsdata/qemu_5.2.0.x86_64.xml    | 68 ++++++++---------
 .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml  | 70 +++++++++---------
 .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml  | 74 +++++++++----------
 tests/domaincapsdata/qemu_6.0.0.x86_64.xml    | 70 +++++++++---------
 .../domaincapsdata/qemu_6.1.0-q35.x86_64.xml  | 70 +++++++++---------
 .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml  | 74 +++++++++----------
 tests/domaincapsdata/qemu_6.1.0.x86_64.xml    | 70 +++++++++---------
 .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml  | 70 +++++++++---------
 .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml  | 74 +++++++++----------
 tests/domaincapsdata/qemu_6.2.0.x86_64.xml    | 70 +++++++++---------
 .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml  | 70 +++++++++---------
 .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml  | 74 +++++++++----------
 tests/domaincapsdata/qemu_7.0.0.x86_64.xml    | 70 +++++++++---------
 .../domaincapsdata/qemu_7.1.0-q35.x86_64.xml  | 66 ++++++++---------
 .../domaincapsdata/qemu_7.1.0-tcg.x86_64.xml  | 70 +++++++++---------
 tests/domaincapsdata/qemu_7.1.0.x86_64.xml    | 66 ++++++++---------
 31 files changed, 907 insertions(+), 893 deletions(-)

diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst
index 6ce780fb69..81211a3fe6 100644
--- a/docs/formatdomaincaps.rst
+++ b/docs/formatdomaincaps.rst
@@ -191,10 +191,10 @@ CPUs <formatdomain.html#cpu-model-and-topology>`__.
          <feature policy='require' name='vmx'/>
        </mode>
        <mode name='custom' supported='yes'>
-         <model usable='no' deprecated='no' vendor='Intel'>Broadwell</model>
+         <model usable='no' deprecated='no' vendor='Intel' blockers='hle,rtm'>Broadwell</model>
          <model usable='yes' deprecated='no' vendor='Intel'>Broadwell-noTSX</model>
-         <model usable='no' deprecated='yes' vendor='Intel'>Haswell</model>
-         <model usable='no' deprecated='no' vendor='AMD'>EPYC-Milan</model>
+         <model usable='no' deprecated='yes' vendor='Intel' blockers='hle,rtm'>Haswell</model>
+         <model usable='no' deprecated='no' vendor='AMD' blockers='pcid,erms,invpcid,pku,fsrm,ibrs,pku'>EPYC-Milan</model>
          ...
        </mode>
      </cpu>
@@ -224,10 +224,12 @@ more details about it:
    by a dedicated ``model`` element. The ``usable`` attribute specifies whether
    the model can be used directly on the host. When usable='no' the
    corresponding model cannot be used without disabling some features that the
-   CPU of such model is expected to have. A special value ``unknown`` indicates
-   libvirt does not have enough information to provide the usability data. The
-   ``deprecated`` attribute reflects the hypervisor's policy on usage of this
-   model :since:`(since 7.1.0)`. The ``vendor`` attribute :since:`(since 8.9.0)`
+   CPU of such model is expected to have. :since:`Since 8.9.0` the ``blockers``
+   attribute contains a comma separated list of these features. A special value
+   ``unknown`` of the attribute ``usable`` indicates libvirt does not have
+   enough information to provide the usability data. The ``deprecated``
+   attribute reflects the hypervisor's policy on usage of this model
+   :since:`(since 7.1.0)`. The ``vendor`` attribute :since:`(since 8.9.0)`
    contains the vendor of the CPU model for users who want to use CPU models
    with specific vendors only. CPU models with undefined vendor will be listed
    with ``vendor='unkwnown'``.
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index b5d8288982..4a6418ca37 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -386,6 +386,13 @@ virDomainCapsCPUCustomFormat(virBuffer *buf,
         else
             virBufferAddLit(buf, " vendor='unknown'");
 
+        if (model->usable == VIR_DOMCAPS_CPU_USABLE_NO &&
+            model->blockers &&
+            *model->blockers) {
+            virBufferAsprintf(buf, " blockers='%s'",
+                              g_strjoinv(",", model->blockers));
+        }
+
         virBufferAsprintf(buf, ">%s</model>\n", model->name);
     }
 
diff --git a/src/conf/schemas/domaincaps.rng b/src/conf/schemas/domaincaps.rng
index c4cb9afeba..7783b49585 100644
--- a/src/conf/schemas/domaincaps.rng
+++ b/src/conf/schemas/domaincaps.rng
@@ -162,6 +162,11 @@
           <attribute name='vendor'>
             <text/>
           </attribute>
+          <optional>
+            <attribute name='blockers'>
+              <text/>
+            </attribute>
+          </optional>
           <text/>
         </element>
       </zeroOrMore>
diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
index dab12e5888..8ca9e8d2b2 100644
--- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
+++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
@@ -63,7 +63,7 @@
     <mode name='custom' supported='yes'>
       <model usable='yes' vendor='unknown'>qemu64</model>
       <model usable='yes' vendor='unknown'>qemu32</model>
-      <model usable='no' vendor='AMD'>phenom</model>
+      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>
       <model usable='yes' vendor='unknown'>pentium3</model>
       <model usable='yes' vendor='unknown'>pentium2</model>
       <model usable='yes' vendor='unknown'>pentium</model>
@@ -72,42 +72,42 @@
       <model usable='yes' vendor='unknown'>kvm32</model>
       <model usable='yes' vendor='Intel'>coreduo</model>
       <model usable='yes' vendor='Intel'>core2duo</model>
-      <model usable='no' vendor='AMD'>athlon</model>
+      <model usable='no' vendor='AMD' blockers='mmxext,3dnowext,3dnow'>athlon</model>
       <model usable='yes' vendor='Intel'>Westmere-IBRS</model>
       <model usable='yes' vendor='Intel'>Westmere</model>
-      <model usable='no' vendor='Intel'>Snowridge</model>
-      <model usable='no' vendor='Intel'>Skylake-Server-noTSX-IBRS</model>
-      <model usable='no' vendor='Intel'>Skylake-Server-IBRS</model>
-      <model usable='no' vendor='Intel'>Skylake-Server</model>
+      <model usable='no' vendor='Intel' blockers='clwb,sha-ni,gfni,cldemote,movdiri,movdir64b,core-capability,split-lock-detect'>Snowridge</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,pku,avx512f,avx512f,avx512f,pku'>Skylake-Server-noTSX-IBRS</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,pku,avx512f,avx512f,avx512f,pku'>Skylake-Server-IBRS</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,pku,avx512f,avx512f,avx512f,pku'>Skylake-Server</model>
       <model usable='yes' vendor='Intel'>Skylake-Client-noTSX-IBRS</model>
       <model usable='yes' vendor='Intel'>Skylake-Client-IBRS</model>
       <model usable='yes' vendor='Intel'>Skylake-Client</model>
       <model usable='yes' vendor='Intel'>SandyBridge-IBRS</model>
       <model usable='yes' vendor='Intel'>SandyBridge</model>
       <model usable='yes' vendor='Intel'>Penryn</model>
-      <model usable='no' vendor='AMD'>Opteron_G5</model>
-      <model usable='no' vendor='AMD'>Opteron_G4</model>
-      <model usable='no' vendor='AMD'>Opteron_G3</model>
+      <model usable='no' vendor='AMD' blockers='sse4a,misalignsse,xop,fma4,tbm,npt,nrip-save'>Opteron_G5</model>
+      <model usable='no' vendor='AMD' blockers='sse4a,misalignsse,xop,fma4,npt,nrip-save'>Opteron_G4</model>
+      <model usable='no' vendor='AMD' blockers='sse4a,misalignsse'>Opteron_G3</model>
       <model usable='yes' vendor='AMD'>Opteron_G2</model>
       <model usable='yes' vendor='AMD'>Opteron_G1</model>
       <model usable='yes' vendor='Intel'>Nehalem-IBRS</model>
       <model usable='yes' vendor='Intel'>Nehalem</model>
       <model usable='yes' vendor='Intel'>IvyBridge-IBRS</model>
       <model usable='yes' vendor='Intel'>IvyBridge</model>
-      <model usable='no' vendor='Intel'>Icelake-Server-noTSX</model>
-      <model usable='no' vendor='Intel'>Icelake-Server</model>
-      <model usable='no' vendor='Intel'>Icelake-Client-noTSX</model>
-      <model usable='no' vendor='Intel'>Icelake-Client</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,avx512vbmi,pku,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,la57,wbnoinvd,avx512f,avx512f,avx512f,pku'>Icelake-Server-noTSX</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,avx512vbmi,pku,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,la57,wbnoinvd,avx512f,avx512f,avx512f,pku'>Icelake-Server</model>
+      <model usable='no' vendor='Intel' blockers='avx512vbmi,pku,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,wbnoinvd,pku'>Icelake-Client-noTSX</model>
+      <model usable='no' vendor='Intel' blockers='avx512vbmi,pku,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,wbnoinvd,pku'>Icelake-Client</model>
       <model usable='yes' vendor='Intel'>Haswell-noTSX-IBRS</model>
       <model usable='yes' vendor='Intel'>Haswell-noTSX</model>
       <model usable='yes' vendor='Intel'>Haswell-IBRS</model>
       <model usable='yes' vendor='Intel'>Haswell</model>
-      <model usable='no' vendor='AMD'>EPYC-IBPB</model>
-      <model usable='no' vendor='AMD'>EPYC</model>
-      <model usable='no' vendor='Hygon'>Dhyana</model>
+      <model usable='no' vendor='AMD' blockers='sha-ni,mmxext,fxsr_opt,cr8legacy,sse4a,misalignsse,osvw,ibpb,npt,nrip-save'>EPYC-IBPB</model>
+      <model usable='no' vendor='AMD' blockers='sha-ni,mmxext,fxsr_opt,cr8legacy,sse4a,misalignsse,osvw,ibpb,npt,nrip-save'>EPYC</model>
+      <model usable='no' vendor='Hygon' blockers='mmxext,fxsr_opt,cr8legacy,sse4a,misalignsse,osvw,ibpb,npt,nrip-save'>Dhyana</model>
       <model usable='yes' vendor='Intel'>Conroe</model>
-      <model usable='no' vendor='Intel'>Cascadelake-Server-noTSX</model>
-      <model usable='no' vendor='Intel'>Cascadelake-Server</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,pku,avx512vnni,avx512f,avx512f,avx512f,pku,rdctl-no,ibrs-all,mds-no'>Cascadelake-Server-noTSX</model>
+      <model usable='no' vendor='Intel' blockers='avx512f,avx512dq,clwb,avx512cd,avx512bw,avx512vl,pku,avx512vnni,avx512f,avx512f,avx512f,pku,rdctl-no,ibrs-all,mds-no'>Cascadelake-Server</model>
       <model usable='yes' vendor='Intel'>Broadwell-noTSX-IBRS</model>
       <model usable='yes' vendor='Intel'>Broadwell-noTSX</model>
       <model usable='yes' vendor='Intel'>Broadwell-IBRS</model>
...
-- 
2.37.3
Re: [libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Daniel P. Berrangé 3 years, 4 months ago
On Tue, Oct 04, 2022 at 04:28:53PM +0200, Jiri Denemark wrote:
> We already show whether a specific CPU model is usable on the current
> host without modification via the 'usable' attribute of each CPU model.
> But it may be useful to actually see what features are blocking each CPU
> model from being usable. Especially when we already fetch the info from
> QEMU and propagating it to domain capabilities XML is all we need to do.

> diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> index dab12e5888..8ca9e8d2b2 100644
> --- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> +++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> @@ -63,7 +63,7 @@
>      <mode name='custom' supported='yes'>
>        <model usable='yes' vendor='unknown'>qemu64</model>
>        <model usable='yes' vendor='unknown'>qemu32</model>
> -      <model usable='no' vendor='AMD'>phenom</model>
> +      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>

This is an XML design anti-pattern, because it invents a data format
inside the attribute which the caller then has to further parse.

If we want to expose this, it needs to be with child elements IMHO,
but yes it will be more much more verbose.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Jiri Denemark 3 years, 4 months ago
On Tue, Oct 04, 2022 at 17:34:34 +0100, Daniel P. Berrangé wrote:
> On Tue, Oct 04, 2022 at 04:28:53PM +0200, Jiri Denemark wrote:
> > We already show whether a specific CPU model is usable on the current
> > host without modification via the 'usable' attribute of each CPU model.
> > But it may be useful to actually see what features are blocking each CPU
> > model from being usable. Especially when we already fetch the info from
> > QEMU and propagating it to domain capabilities XML is all we need to do.
> 
> > diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > index dab12e5888..8ca9e8d2b2 100644
> > --- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > +++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > @@ -63,7 +63,7 @@
> >      <mode name='custom' supported='yes'>
> >        <model usable='yes' vendor='unknown'>qemu64</model>
> >        <model usable='yes' vendor='unknown'>qemu32</model>
> > -      <model usable='no' vendor='AMD'>phenom</model>
> > +      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>
> 
> This is an XML design anti-pattern, because it invents a data format
> inside the attribute which the caller then has to further parse.
> 
> If we want to expose this, it needs to be with child elements IMHO,
> but yes it will be more much more verbose.

You're absolutely right, but that's the only option we have I'm afraid.
Mixing subelements and text nodes is a much worse anti-pattern. I wish
the model name was in an attribute, but it isn't and having

    <model usable='no' vendor='AMD'>
      <blocker name='mmxext'/>
      phenom
    </model>

is just insane :-(

Jirka
Re: [libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Daniel P. Berrangé 3 years, 4 months ago
On Tue, Oct 04, 2022 at 07:35:31PM +0200, Jiri Denemark wrote:
> On Tue, Oct 04, 2022 at 17:34:34 +0100, Daniel P. Berrangé wrote:
> > On Tue, Oct 04, 2022 at 04:28:53PM +0200, Jiri Denemark wrote:
> > > We already show whether a specific CPU model is usable on the current
> > > host without modification via the 'usable' attribute of each CPU model.
> > > But it may be useful to actually see what features are blocking each CPU
> > > model from being usable. Especially when we already fetch the info from
> > > QEMU and propagating it to domain capabilities XML is all we need to do.
> > 
> > > diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > index dab12e5888..8ca9e8d2b2 100644
> > > --- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > +++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > @@ -63,7 +63,7 @@
> > >      <mode name='custom' supported='yes'>
> > >        <model usable='yes' vendor='unknown'>qemu64</model>
> > >        <model usable='yes' vendor='unknown'>qemu32</model>
> > > -      <model usable='no' vendor='AMD'>phenom</model>
> > > +      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>
> > 
> > This is an XML design anti-pattern, because it invents a data format
> > inside the attribute which the caller then has to further parse.
> > 
> > If we want to expose this, it needs to be with child elements IMHO,
> > but yes it will be more much more verbose.
> 
> You're absolutely right, but that's the only option we have I'm afraid.
> Mixing subelements and text nodes is a much worse anti-pattern. I wish
> the model name was in an attribute, but it isn't and having
> 
>     <model usable='no' vendor='AMD'>
>       <blocker name='mmxext'/>
>       phenom
>     </model>
> 
> is just insane :-(

True, I wonder if there's a different approach to the overall problem
that would be better.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Jiri Denemark 3 years, 4 months ago
> On Tue, Oct 04, 2022 at 07:35:31PM +0200, Jiri Denemark wrote:
> > On Tue, Oct 04, 2022 at 17:34:34 +0100, Daniel P. Berrangé wrote:
> > > On Tue, Oct 04, 2022 at 04:28:53PM +0200, Jiri Denemark wrote:
> > > > We already show whether a specific CPU model is usable on the current
> > > > host without modification via the 'usable' attribute of each CPU model.
> > > > But it may be useful to actually see what features are blocking each CPU
> > > > model from being usable. Especially when we already fetch the info from
> > > > QEMU and propagating it to domain capabilities XML is all we need to do.
> > > 
> > > > diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > index dab12e5888..8ca9e8d2b2 100644
> > > > --- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > +++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > @@ -63,7 +63,7 @@
> > > >      <mode name='custom' supported='yes'>
> > > >        <model usable='yes' vendor='unknown'>qemu64</model>
> > > >        <model usable='yes' vendor='unknown'>qemu32</model>
> > > > -      <model usable='no' vendor='AMD'>phenom</model>
> > > > +      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>
> > > 
> > > This is an XML design anti-pattern, because it invents a data format
> > > inside the attribute which the caller then has to further parse.
> > > 
> > > If we want to expose this, it needs to be with child elements IMHO,
> > > but yes it will be more much more verbose.
> > 
> > You're absolutely right, but that's the only option we have I'm afraid.
> > Mixing subelements and text nodes is a much worse anti-pattern. I wish
> > the model name was in an attribute, but it isn't and having
> > 
> >     <model usable='no' vendor='AMD'>
> >       <blocker name='mmxext'/>
> >       phenom
> >     </model>
> > 
> > is just insane :-(
> 
> True, I wonder if there's a different approach to the overall problem
> that would be better.

Actually a third option just came to my mind. It's not ideal either, but
at least it would be a proper XML :-)

    <mode name='custom' supported='yes'>
      <model usable='yes' vendor='unknown'>qemu64</model>
      <model usable='no' vendor='AMD'>phenom</model>
      <blockers model='phenom'>
        <feature name='mmxext'/>
        <feature name='fxsr_opt'/>
        ...
      </blockers>
      <model ...>...</model>
      ...
    </mode>

Jirka
Re: [libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Daniel P. Berrangé 3 years, 4 months ago
On Tue, Oct 04, 2022 at 10:17:18PM +0200, Jiri Denemark wrote:
> > On Tue, Oct 04, 2022 at 07:35:31PM +0200, Jiri Denemark wrote:
> > > On Tue, Oct 04, 2022 at 17:34:34 +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Oct 04, 2022 at 04:28:53PM +0200, Jiri Denemark wrote:
> > > > > We already show whether a specific CPU model is usable on the current
> > > > > host without modification via the 'usable' attribute of each CPU model.
> > > > > But it may be useful to actually see what features are blocking each CPU
> > > > > model from being usable. Especially when we already fetch the info from
> > > > > QEMU and propagating it to domain capabilities XML is all we need to do.
> > > > 
> > > > > diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > > index dab12e5888..8ca9e8d2b2 100644
> > > > > --- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > > +++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > > @@ -63,7 +63,7 @@
> > > > >      <mode name='custom' supported='yes'>
> > > > >        <model usable='yes' vendor='unknown'>qemu64</model>
> > > > >        <model usable='yes' vendor='unknown'>qemu32</model>
> > > > > -      <model usable='no' vendor='AMD'>phenom</model>
> > > > > +      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>
> > > > 
> > > > This is an XML design anti-pattern, because it invents a data format
> > > > inside the attribute which the caller then has to further parse.
> > > > 
> > > > If we want to expose this, it needs to be with child elements IMHO,
> > > > but yes it will be more much more verbose.
> > > 
> > > You're absolutely right, but that's the only option we have I'm afraid.
> > > Mixing subelements and text nodes is a much worse anti-pattern. I wish
> > > the model name was in an attribute, but it isn't and having
> > > 
> > >     <model usable='no' vendor='AMD'>
> > >       <blocker name='mmxext'/>
> > >       phenom
> > >     </model>
> > > 
> > > is just insane :-(
> > 
> > True, I wonder if there's a different approach to the overall problem
> > that would be better.
> 
> Actually a third option just came to my mind. It's not ideal either, but
> at least it would be a proper XML :-)
> 
>     <mode name='custom' supported='yes'>
>       <model usable='yes' vendor='unknown'>qemu64</model>
>       <model usable='no' vendor='AMD'>phenom</model>
>       <blockers model='phenom'>
>         <feature name='mmxext'/>
>         <feature name='fxsr_opt'/>
>         ...
>       </blockers>
>       <model ...>...</model>
>       ...
>     </mode>

Actually, looking atr this in practice, I don't think we should be
including this information in domcapabilities at all. It gets
waaaaaaaay too verbose, even with the custom syntax in this current
patch impl. Take a look at this from one of my VMs, which uses the
qemu64 model, and thus lacks a huge number of features:

      <model fallback='forbid'>Opteron_G3</model>
      <model usable='yes' vendor='unknown'>qemu64</model>
      <model usable='yes' vendor='unknown'>qemu32</model>
      <model usable='no' vendor='AMD' blockers='fxsr-opt'>phenom</model>
      <model usable='yes' vendor='unknown'>pentium3</model>
      <model usable='yes' vendor='unknown'>pentium2</model>
      <model usable='yes' vendor='unknown'>pentium</model>
      <model usable='yes' vendor='Intel'>n270</model>
      <model usable='yes' vendor='unknown'>kvm64</model>
      <model usable='yes' vendor='unknown'>kvm32</model>
      <model usable='yes' vendor='Intel'>coreduo</model>
      <model usable='yes' vendor='Intel'>core2duo</model>
      <model usable='yes' vendor='AMD'>athlon</model>
      <model usable='no' vendor='Intel' blockers='spec-ctrl'>Westmere-IBRS</model>
      <model usable='yes' vendor='Intel'>Westmere</model>
      <model usable='no' vendor='Intel' blockers='x2apic,tsc-deadline,rdseed,sha-ni,gfni,cldemote,movdiri,movdir64b,spec-ctrl,arch-capabilities,core-capability,ssbd,3dnowprefetch,xsavec,split-lock-detect'>Snowridge</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,spec-ctrl,3dnowprefetch,xsavec'>Skylake-Server-noTSX-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,spec-ctrl,3dnowprefetch,xsavec'>Skylake-Server-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,3dnowprefetch,xsavec'>Skylake-Server</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,rdseed,spec-ctrl,3dnowprefetch,xsavec'>Skylake-Client-noTSX-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,rdseed,spec-ctrl,3dnowprefetch,xsavec'>Skylake-Client-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,rdseed,3dnowprefetch,xsavec'>Skylake-Client</model>
      <model usable='no' vendor='Intel' blockers='x2apic,tsc-deadline,avx,spec-ctrl'>SandyBridge-IBRS</model>
      <model usable='no' vendor='Intel' blockers='x2apic,tsc-deadline,avx'>SandyBridge</model>
      <model usable='yes' vendor='Intel'>Penryn</model>
      <model usable='no' vendor='AMD' blockers='fma,avx,f16c,misalignsse,3dnowprefetch,xop,fma4,tbm,nrip-save'>Opteron_G5</model>
      <model usable='no' vendor='AMD' blockers='avx,misalignsse,3dnowprefetch,xop,fma4,nrip-save'>Opteron_G4</model>
      <model usable='no' vendor='AMD' blockers='misalignsse'>Opteron_G3</model>
      <model usable='yes' vendor='AMD'>Opteron_G2</model>
      <model usable='yes' vendor='AMD'>Opteron_G1</model>
      <model usable='no' vendor='Intel' blockers='spec-ctrl'>Nehalem-IBRS</model>
      <model usable='yes' vendor='Intel'>Nehalem</model>
      <model usable='no' vendor='Intel' blockers='x2apic,tsc-deadline,avx,f16c,spec-ctrl'>IvyBridge-IBRS</model>
      <model usable='no' vendor='Intel' blockers='x2apic,tsc-deadline,avx,f16c'>IvyBridge</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,avx512vbmi,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,spec-ctrl,ssbd,3dnowprefetch,wbnoinvd,xsavec'>Icelake-Server-noTSX</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,avx512vbmi,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,spec-ctrl,ssbd,3dnowprefetch,wbnoinvd,xsavec'>Icelake-Server</model>
      <model usable='no' deprecated='yes' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,rdseed,avx512vbmi,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,spec-ctrl,ssbd,3dnowprefetch,wbnoinvd,xsavec'>Icelake-Client-noTSX</model>
      <model usable='no' deprecated='yes' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,rdseed,avx512vbmi,avx512vbmi2,gfni,vaes,vpclmulqdq,avx512vnni,avx512bitalg,avx512-vpopcntdq,spec-ctrl,ssbd,3dnowprefetch,wbnoinvd,xsavec'>Icelake-Client</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,spec-ctrl'>Haswell-noTSX-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid'>Haswell-noTSX</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,spec-ctrl'>Haswell-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm'>Haswell</model>
      <model usable='no' vendor='AMD' blockers='fma,avx,f16c,avx2,rdseed,sha-ni,rdpid,fxsr-opt,misalignsse,3dnowprefetch,osvw,topoext,perfctr-core,clzero,xsaveerptr,wbnoinvd,ibpb,amd-stibp,nrip-save,xsavec,xsaves'>EPYC-Rome</model>
      <model usable='no' vendor='AMD' blockers='fma,pcid,avx,f16c,avx2,invpcid,rdseed,sha-ni,rdpid,fsrm,fxsr-opt,misalignsse,3dnowprefetch,osvw,topoext,perfctr-core,clzero,xsaveerptr,wbnoinvd,ibpb,ibrs,amd-stibp,amd-ssbd,nrip-save,xsavec,xsaves'>EPYC-Milan</model>
      <model usable='no' vendor='AMD' blockers='fma,avx,f16c,avx2,rdseed,sha-ni,fxsr-opt,misalignsse,3dnowprefetch,osvw,topoext,ibpb,nrip-save,xsavec'>EPYC-IBPB</model>
      <model usable='no' vendor='AMD' blockers='fma,avx,f16c,avx2,rdseed,sha-ni,fxsr-opt,misalignsse,3dnowprefetch,osvw,topoext,nrip-save,xsavec'>EPYC</model>
      <model usable='no' vendor='Hygon' blockers='fma,avx,f16c,avx2,rdseed,fxsr-opt,misalignsse,3dnowprefetch,osvw,topoext,ibpb,nrip-save,xsavec'>Dhyana</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,avx512vnni,spec-ctrl,stibp,arch-capabilities,ssbd,avx512-bf16,3dnowprefetch,xsavec,rdctl-no,ibrs-all,skip-l1dfl-vmentry,mds-no,pschange-mc-no,taa-no'>Cooperlake</model>
      <model usable='yes' vendor='Intel'>Conroe</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,avx512vnni,spec-ctrl,arch-capabilities,ssbd,3dnowprefetch,xsavec,rdctl-no,ibrs-all,skip-l1dfl-vmentry,mds-no'>Cascadelake-Server-noTSX</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,avx512f,avx512dq,rdseed,avx512cd,avx512bw,avx512vl,avx512vnni,spec-ctrl,ssbd,3dnowprefetch,xsavec'>Cascadelake-Server</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,rdseed,spec-ctrl,3dnowprefetch'>Broadwell-noTSX-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,avx2,invpcid,rdseed,3dnowprefetch'>Broadwell-noTSX</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,rdseed,spec-ctrl,3dnowprefetch'>Broadwell-IBRS</model>
      <model usable='no' vendor='Intel' blockers='fma,pcid,x2apic,tsc-deadline,avx,f16c,hle,avx2,invpcid,rtm,rdseed,3dnowprefetch'>Broadwell</model>
      <model usable='yes' vendor='unknown'>486</model>


I think we need to expose this in a different way, using the CPU baseline
APIs. THese already have a VIR_CPU_BASELINE_EXPAND_FEATURES flag. We
should add a further VIR_CPU_BASELINE_BLOCKED_FEATURES flag to it.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 10/11] domain_capabilities: Add blockers attribute for CPU models
Posted by Jiri Denemark 3 years, 4 months ago
On Wed, Oct 05, 2022 at 09:07:55 +0100, Daniel P. Berrangé wrote:
> On Tue, Oct 04, 2022 at 10:17:18PM +0200, Jiri Denemark wrote:
> > > On Tue, Oct 04, 2022 at 07:35:31PM +0200, Jiri Denemark wrote:
> > > > On Tue, Oct 04, 2022 at 17:34:34 +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Oct 04, 2022 at 04:28:53PM +0200, Jiri Denemark wrote:
> > > > > > We already show whether a specific CPU model is usable on the current
> > > > > > host without modification via the 'usable' attribute of each CPU model.
> > > > > > But it may be useful to actually see what features are blocking each CPU
> > > > > > model from being usable. Especially when we already fetch the info from
> > > > > > QEMU and propagating it to domain capabilities XML is all we need to do.
> > > > > 
> > > > > > diff --git a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > > > index dab12e5888..8ca9e8d2b2 100644
> > > > > > --- a/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > > > +++ b/tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml
> > > > > > @@ -63,7 +63,7 @@
> > > > > >      <mode name='custom' supported='yes'>
> > > > > >        <model usable='yes' vendor='unknown'>qemu64</model>
> > > > > >        <model usable='yes' vendor='unknown'>qemu32</model>
> > > > > > -      <model usable='no' vendor='AMD'>phenom</model>
> > > > > > +      <model usable='no' vendor='AMD' blockers='mmxext,fxsr_opt,3dnowext,3dnow,sse4a,npt'>phenom</model>
> > > > > 
> > > > > This is an XML design anti-pattern, because it invents a data format
> > > > > inside the attribute which the caller then has to further parse.
> > > > > 
> > > > > If we want to expose this, it needs to be with child elements IMHO,
> > > > > but yes it will be more much more verbose.
> > > > 
> > > > You're absolutely right, but that's the only option we have I'm afraid.
> > > > Mixing subelements and text nodes is a much worse anti-pattern. I wish
> > > > the model name was in an attribute, but it isn't and having
> > > > 
> > > >     <model usable='no' vendor='AMD'>
> > > >       <blocker name='mmxext'/>
> > > >       phenom
> > > >     </model>
> > > > 
> > > > is just insane :-(
> > > 
> > > True, I wonder if there's a different approach to the overall problem
> > > that would be better.
> > 
> > Actually a third option just came to my mind. It's not ideal either, but
> > at least it would be a proper XML :-)
> > 
> >     <mode name='custom' supported='yes'>
> >       <model usable='yes' vendor='unknown'>qemu64</model>
> >       <model usable='no' vendor='AMD'>phenom</model>
> >       <blockers model='phenom'>
> >         <feature name='mmxext'/>
> >         <feature name='fxsr_opt'/>
> >         ...
> >       </blockers>
> >       <model ...>...</model>
> >       ...
> >     </mode>
> 
> Actually, looking atr this in practice, I don't think we should be
> including this information in domcapabilities at all. It gets
> waaaaaaaay too verbose, even with the custom syntax in this current
> patch impl. Take a look at this from one of my VMs, which uses the
> qemu64 model, and thus lacks a huge number of features:
...
> I think we need to expose this in a different way, using the CPU baseline
> APIs. THese already have a VIR_CPU_BASELINE_EXPAND_FEATURES flag. We
> should add a further VIR_CPU_BASELINE_BLOCKED_FEATURES flag to it.

Hmm, not a bad idea. And we don't even need a new flag, just a bit of
documentation and a bug fix. When you pass just a simple

    <cpu>
      <arch>x86_64</arch>
      <model>EPYC</model>
      <vendor>AMD</vendor>
    </cpu>

CPU definition to hypervisor-cpu-baseline, libvirt checks the host CPU
model for features included in EPYC CPU model and disables those that
are unavailable on the host:

    <cpu mode='custom' match='exact'>
      <model fallback='forbid'>EPYC</model>
      <vendor>AMD</vendor>
      <feature policy='disable' name='sha-ni'/>
      <feature policy='disable' name='mmxext'/>
      <feature policy='disable' name='cr8legacy'/>
      <feature policy='disable' name='sse4a'/>
      <feature policy='disable' name='misalignsse'/>
      <feature policy='disable' name='osvw'/>
      <feature policy='disable' name='monitor'/>
    </cpu>

Although it does not exactly match the list of blockers from QEMU:
sha-ni, mmxext, fxsr-opt, cr8legacy, sse4a, misalignsse, osvw.

Libvirt disables some features which are not present in QEMU's blockers
list (monitor), but this is fine as these features are included only in
libvirt's EPYC and QEMU would not enable them anyway. Explicitly
disabling them is a no-op for QEMU and helps pass libvirt internal
checks. Seeing a non-empty list of disabled features for a CPU model
marked as usable='yes' might just be a bit confusing. I guess just
documenting this should be enough.

On the other hand, the baseline CPU model does not disable some features
QEMU would need to disable (fxsr-opt in the example above) because they
are not included in the CPU model definition in libvirt. But this is a
bug and I'm a bit surprised to see it as I believe I addressed this
exact issue some time ago (although it's quite possible I'm just
thinking about similar issue somewhere else).

Jirka