1. Patchew
  2. QEMU
  3. View series

[PATCH v3 0/5] Generate x86 cpu features

Tim Wiederhake posted 5 patches 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240206134739.15345-1-twiederh@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
target/i386/cpu.c                   | 679 +-------------------------
target/i386/feature_word_info.c.inc | 710 ++++++++++++++++++++++++++++
target/i386/feature_word_info.py    |  71 +++
target/i386/feature_word_info.yaml  | 701 +++++++++++++++++++++++++++
4 files changed, 1483 insertions(+), 678 deletions(-)
create mode 100644 target/i386/feature_word_info.c.inc
create mode 100755 target/i386/feature_word_info.py
create mode 100644 target/i386/feature_word_info.yaml
[PATCH v3 0/5] Generate x86 cpu features
Posted by Tim Wiederhake 2 months, 3 weeks ago 
Synchronizing the list of cpu features and models with qemu is a recurring
task in libvirt. For x86, this is done by reading qom-list-properties for
max-x86_64-cpu and manually filtering out everthing that does not look like
a feature name, as well as parsing target/i386/cpu.c for cpu models.

This is a flawed, tedious and error-prone procedure. Ideally, qemu
and libvirt would query a common source for cpu feature and model
related information. Meanwhile, converting this information into an easier
to parse format would help libvirt a lot.

This patch series converts the cpu feature information present in
target/i386/cpu.c (`feature_word_info`) into a yaml file and adds a
script to generate the c code from this data.

v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-08/msg02005.html
v2: https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01773.html

Changes since v2:
* Rebased on top of current master
* Removed all "drive-by" changes to feature names ("vmx-ept-uc", "vmx-ept-wb",
  "kvmclock", "vmx-invept-single-context", and
  "vmx-invept-single-context-noglobals") as these needed further discussion.
* Changes to the generator script reduce the changes in formatting to the
  current feature_word_info even further to address the concern about code
  legibility. See Patch 5, "target/i386: Generate feature_word_info.c.inc" for
  all non-whitespace changes.

Tim Wiederhake (5):
  target/i386: Split out feature_word_info
  target/i386: Translate feature_word_info to yaml
  target/i386: Remove comments from feature_word_info.c.inc
  target/i386: Fix feature_word_info.c.inc formatting
  target/i386: Generate feature_word_info.c.inc

 target/i386/cpu.c                   | 679 +-------------------------
 target/i386/feature_word_info.c.inc | 710 ++++++++++++++++++++++++++++
 target/i386/feature_word_info.py    |  71 +++
 target/i386/feature_word_info.yaml  | 701 +++++++++++++++++++++++++++
 4 files changed, 1483 insertions(+), 678 deletions(-)
 create mode 100644 target/i386/feature_word_info.c.inc
 create mode 100755 target/i386/feature_word_info.py
 create mode 100644 target/i386/feature_word_info.yaml

-- 
2.43.0
Re: [PATCH v3 0/5] Generate x86 cpu features
Posted by Daniel P. Berrangé 1 month, 3 weeks ago 
On Tue, Feb 06, 2024 at 02:47:34PM +0100, Tim Wiederhake wrote:
> Synchronizing the list of cpu features and models with qemu is a recurring
> task in libvirt. For x86, this is done by reading qom-list-properties for
> max-x86_64-cpu and manually filtering out everthing that does not look like
> a feature name, as well as parsing target/i386/cpu.c for cpu models.
> 
> This is a flawed, tedious and error-prone procedure. Ideally, qemu
> and libvirt would query a common source for cpu feature and model
> related information. Meanwhile, converting this information into an easier
> to parse format would help libvirt a lot.
> 
> This patch series converts the cpu feature information present in
> target/i386/cpu.c (`feature_word_info`) into a yaml file and adds a
> script to generate the c code from this data.

Looking at this fresh, I'm left wondering why I didn't suggested
using 'QMP' to expose this information when reviewing the earlier
versions. I see Igor did indeed suggest this:

  https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg03905.html

Your commentry that "qom-list-properties" doesn't distinguish
between CPU features and other random QOM properties is bang
on the money.

I think what this highlights, is that 'qom-list-properties'
is a very poor design/fit for the problem that management apps
need to solve in this regard.

Libvirt should not need to manually exclude non-feature properties
like 'check' 'enforce' 'migratable' etc.

QEMU already has this knowledge, as IIUC, 'query-cpu-model-expansion'
can distinguish this:

query-cpu-model-expansion type=static model={'name':'Nehalem'}
{
    "return": {
        "model": {
            "name": "base",
            "props": {
                "3dnow": false,
                ...snip...
                "xtpr": false
            }
        }
    }
}

We still have the problem that we're not exposing the CPUID/MSR
leafs/register bits. So query-cpu-model-expansion isn't a fit
for the problem.

Rather than try to design something super general purpose, I'd
suggest we take a short cut and design something entirley x86
specific, and simply mark the QMP command as "unstable"
eg a 'x-query-x86-cpu-model-features', and then basically
report all the information libvirt needs there.

This is functionally equivalent to what you expose in the YAML
file, while still using QEMU's formal 'QMP' API mechanism, so
we avoid inventing a new API concept via YAML.

I think this would avoid need to have a code generator refactor
the CPU definitions too. We just need to expose the values of
the existing CPUID_xxx constants against each register.



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: [PATCH v3 0/5] Generate x86 cpu features
Posted by Tim Wiederhake 1 month, 2 weeks ago 
On Tue, 2024-03-05 at 14:17 +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 06, 2024 at 02:47:34PM +0100, Tim Wiederhake wrote:
> > > > Synchronizing the list of cpu features and models with qemu is
> > > > a
> > > > recurring
> > > > task in libvirt. For x86, this is done by reading
> > > > qom-list-properties for
> > > > max-x86_64-cpu and manually filtering out everthing that does
> > > > not
> > > > look like
> > > > a feature name, as well as parsing target/i386/cpu.c for cpu
> > > > models.
> > > > 
> > > > This is a flawed, tedious and error-prone procedure. Ideally,
> > > > qemu
> > > > and libvirt would query a common source for cpu feature and
> > > > model
> > > > related information. Meanwhile, converting this information
> > > > into an
> > > > easier
> > > > to parse format would help libvirt a lot.
> > > > 
> > > > This patch series converts the cpu feature information present
> > > > in
> > > > target/i386/cpu.c (`feature_word_info`) into a yaml file and
> > > > adds a
> > > > script to generate the c code from this data.
> > 
> > Looking at this fresh, I'm left wondering why I didn't suggested
> > using 'QMP' to expose this information when reviewing the earlier
> > versions. I see Igor did indeed suggest this:
> > 
> >   
> > https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg03905.html
> > 
> > Your commentry that "qom-list-properties" doesn't distinguish
> > between CPU features and other random QOM properties is bang
> > on the money.
> > 
> > I think what this highlights, is that 'qom-list-properties'
> > is a very poor design/fit for the problem that management apps
> > need to solve in this regard.
> > 
> > Libvirt should not need to manually exclude non-feature properties
> > like 'check' 'enforce' 'migratable' etc.
> > 
> > QEMU already has this knowledge, as IIUC, 'query-cpu-model-
> > expansion'
> > can distinguish this:
> > 
> > query-cpu-model-expansion type=static model={'name':'Nehalem'}
> > {
> >     "return": {
> >         "model": {
> >             "name": "base",
> >             "props": {
> >                 "3dnow": false,
> >                 ...snip...
> >                 "xtpr": false
> >             }
> >         }
> >     }
> > }
> > 
> > We still have the problem that we're not exposing the CPUID/MSR
> > leafs/register bits. So query-cpu-model-expansion isn't a fit
> > for the problem.
> > 
> > Rather than try to design something super general purpose, I'd
> > suggest we take a short cut and design something entirley x86
> > specific, and simply mark the QMP command as "unstable"
> > eg a 'x-query-x86-cpu-model-features', and then basically
> > report all the information libvirt needs there.
> > 
> > This is functionally equivalent to what you expose in the YAML
> > file, while still using QEMU's formal 'QMP' API mechanism, so
> > we avoid inventing a new API concept via YAML.
> > 
> > I think this would avoid need to have a code generator refactor
> > the CPU definitions too. We just need to expose the values of
> > the existing CPUID_xxx constants against each register.
> > 
> > 
> > 
> > With regards,
> > Daniel

Thank you for your feedback.

I do not see the patches and your proposed x-query-x86-cpu-model-
features QMP command being mutually exclusive.

In fact, I'd advocate for merging this patches still, as they provide a
solution (albeit not through QMP) already whereas the QMP command would
still need to be written. Additionally, there are more benefits to the
generate-code approach, as the code generator can be extended to also
generate the feature bits "#define CPUID_* (1U << ...)" in cpu.h,
removing one more source of errors. And with the generated
`feature_word_info` structure being virtually identical to the current
version, I see no downsides: If the generator does become obsolete in
the future, simply remove the python script and the yaml file, and all
that is left is the original feature_word_info code, but better
formatted.

Regards,
Tim
Re: [PATCH v3 0/5] Generate x86 cpu features
Posted by Daniel P. Berrangé 1 month, 2 weeks ago 
On Mon, Mar 11, 2024 at 12:32:03PM +0100, Tim Wiederhake wrote:
> On Tue, 2024-03-05 at 14:17 +0000, Daniel P. Berrangé wrote:
> > > Looking at this fresh, I'm left wondering why I didn't suggested
> > > using 'QMP' to expose this information when reviewing the earlier
> > > versions. I see Igor did indeed suggest this:
> > > 
> > >   
> > > https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg03905.html
> > > 
> > > Your commentry that "qom-list-properties" doesn't distinguish
> > > between CPU features and other random QOM properties is bang
> > > on the money.
> > > 
> > > I think what this highlights, is that 'qom-list-properties'
> > > is a very poor design/fit for the problem that management apps
> > > need to solve in this regard.
> > > 
> > > Libvirt should not need to manually exclude non-feature properties
> > > like 'check' 'enforce' 'migratable' etc.
> > > 
> > > QEMU already has this knowledge, as IIUC, 'query-cpu-model-
> > > expansion'
> > > can distinguish this:
> > > 
> > > query-cpu-model-expansion type=static model={'name':'Nehalem'}
> > > {
> > >     "return": {
> > >         "model": {
> > >             "name": "base",
> > >             "props": {
> > >                 "3dnow": false,
> > >                 ...snip...
> > >                 "xtpr": false
> > >             }
> > >         }
> > >     }
> > > }
> > > 
> > > We still have the problem that we're not exposing the CPUID/MSR
> > > leafs/register bits. So query-cpu-model-expansion isn't a fit
> > > for the problem.
> > > 
> > > Rather than try to design something super general purpose, I'd
> > > suggest we take a short cut and design something entirley x86
> > > specific, and simply mark the QMP command as "unstable"
> > > eg a 'x-query-x86-cpu-model-features', and then basically
> > > report all the information libvirt needs there.
> > > 
> > > This is functionally equivalent to what you expose in the YAML
> > > file, while still using QEMU's formal 'QMP' API mechanism, so
> > > we avoid inventing a new API concept via YAML.
> > > 
> > > I think this would avoid need to have a code generator refactor
> > > the CPU definitions too. We just need to expose the values of
> > > the existing CPUID_xxx constants against each register.
>
> I do not see the patches and your proposed x-query-x86-cpu-model-
> features QMP command being mutually exclusive.
> 
> In fact, I'd advocate for merging this patches still, as they provide a
> solution (albeit not through QMP) already whereas the QMP command would
> still need to be written.

I would not class this as an advantage for QEMU in merging this
series. It is defining what amounts to a new externally consumable
interface for QEMU, which is intended to be used by libvirt. Even
if it not consumed at runtime, it is still implying API guarantees.
QEMU really does not want to be adding new interfaces, as there is
a strong desire to reduce all external interaction with QEMU to
exclusively be QMP based.

>                           Additionally, there are more benefits to the
> generate-code approach, as the code generator can be extended to also
> generate the feature bits "#define CPUID_* (1U << ...)" in cpu.h,
> removing one more source of errors. And with the generated
> `feature_word_info` structure being virtually identical to the current
> version, I see no downsides: If the generator does become obsolete in
> the future, simply remove the python script and the yaml file, and all
> that is left is the original feature_word_info code, but better
> formatted.

If we external usage, then we're left with a pretty weak justification
for introducing a new custom code generator here IMHO.

In terms of the CPUID_ constants, I think what would make more sense
from QEMU's POV is to define them all as a QAPI enum.

eg

  { 'enum': 'CPUID',
    'data': [
        'fpu', 'vme', 'de', 'pse', 'tsc',
	.....
    ] }

which will result in QAPI generating constants

    CPUID_VME
    CPUID_FPU
    CPUID_DE
    ...etc

along with a constant <-> string conversion table.

The feature words could use the constants directly instead of their
string form

FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
    [FEAT_1_EDX] = {
        .type = CPUID_FEATURE_WORD,
        .features = {
	    [0] = CPUID_FPU,
    	    [1] = CPUID_VME,
	    [2] = CPUID_DE,
            ...etc...
        },
        .cpuid = {.eax = 1, .reg = R_EDX, },
        .tcg_features = TCG_FEATURES,
        .no_autoenable_flags = CPUID_HT,
    },


which I think encodes the information just as effectively as the yaml
file does.

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: [PATCH v3 0/5] Generate x86 cpu features
Posted by Tim Wiederhake 1 month, 3 weeks ago 
ping

On Tue, 2024-02-06 at 14:47 +0100, Tim Wiederhake wrote:
> Synchronizing the list of cpu features and models with qemu is a
> recurring
> task in libvirt. For x86, this is done by reading qom-list-properties
> for
> max-x86_64-cpu and manually filtering out everthing that does not
> look like
> a feature name, as well as parsing target/i386/cpu.c for cpu models.
> 
> This is a flawed, tedious and error-prone procedure. Ideally, qemu
> and libvirt would query a common source for cpu feature and model
> related information. Meanwhile, converting this information into an
> easier
> to parse format would help libvirt a lot.
> 
> This patch series converts the cpu feature information present in
> target/i386/cpu.c (`feature_word_info`) into a yaml file and adds a
> script to generate the c code from this data.
> 
> v1:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-08/msg02005.html
> v2:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01773.html
> 
> Changes since v2:
> * Rebased on top of current master
> * Removed all "drive-by" changes to feature names ("vmx-ept-uc",
> "vmx-ept-wb",
>   "kvmclock", "vmx-invept-single-context", and
>   "vmx-invept-single-context-noglobals") as these needed further
> discussion.
> * Changes to the generator script reduce the changes in formatting to
> the
>   current feature_word_info even further to address the concern about
> code
>   legibility. See Patch 5, "target/i386: Generate
> feature_word_info.c.inc" for
>   all non-whitespace changes.
> 
> Tim Wiederhake (5):
>   target/i386: Split out feature_word_info
>   target/i386: Translate feature_word_info to yaml
>   target/i386: Remove comments from feature_word_info.c.inc
>   target/i386: Fix feature_word_info.c.inc formatting
>   target/i386: Generate feature_word_info.c.inc
> 
>  target/i386/cpu.c                   | 679 +-------------------------
>  target/i386/feature_word_info.c.inc | 710
> ++++++++++++++++++++++++++++
>  target/i386/feature_word_info.py    |  71 +++
>  target/i386/feature_word_info.yaml  | 701
> +++++++++++++++++++++++++++
>  4 files changed, 1483 insertions(+), 678 deletions(-)
>  create mode 100644 target/i386/feature_word_info.c.inc
>  create mode 100755 target/i386/feature_word_info.py
>  create mode 100644 target/i386/feature_word_info.yaml
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.