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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|