Re: [PATCH v3 0/5] Generate x86 cpu features

[Tim Wiederhake <twiederh@redhat.com>]

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