Re: [Qemu-devel] [Patch v1 24/29] qmp: add QMP interface "query-cpu-model-expansion"

[David Hildenbrand <dahi@linux.vnet.ibm.com>]

> > +# A CPU model consists of the name of a CPU definition, to which
> > +# delta changes are applied (e.g. features added/removed). Most magic values
> > +# that an architecture might require should be hidden behind the name.
> > +# However, if required, architectures can expose relevant properties.
> > +#
> > +# @name: the name of the CPU definition the model is based on
> > +# @props: #optional a dictionary of properties to be applied  
> 
> Should we make it explicit that we are talking about QOM
> properties?

Yes makes sense.

> 
> > +#
> > +# Since: 2.8.0
> > +##
> > +{ 'struct': 'CpuModelInfo',
> > +  'data': { 'name': 'str',
> > +            '*props': 'any' } }
> > +
> > +##
> > +# @CpuModelExpansionType
> > +#
> > +# An enumeration of CPU model expansion types.
> > +#
> > +# @static: Expand to a static CPU model, a combination of a static base
> > +#          model name and property delta changes. As the static base model will
> > +#          never change, the expanded CPU model will be the same, independant of
> > +#          QEMU version or compatibility machines. Therefore, the resulting  
> 
> We could be more explicit about the guarantees: "independent of
> QEMU version, machine type, machine options, and accelerator
> options".

agreed.

> 
> > +#          model can be used by tooling without having to specify a
> > +#          compatibility machine - e.g. when displaying the "host" model.
> > +#          All static CPU models are migration-safe.  
> 
> This is cool. Unfortunately we are not going to support it in x86
> very soon because we don't have any static CPU models.

Well, it's all about finding a minimum set of features that one can work with.
I assume e.g. a Phenom also always has a minimum set of features?

> 
> > +#
> > +# @full: Expand all properties. The produced model is not guaranteed to be
> > +#        migration-safe, but allows tooling to get an insight and work with
> > +#        model details.  
> 
> I wonder if we really need to document it broadly as "not
> guaranteed to be migration-safe". The returned data will be
> migration-safe (but not static) if the CPU model being expanded
> is migration-safe, won't it?

Actually I don't think so.
Imagine expanding host: featA=true, featB=false

Now, if going to another QEMU version, there might be featC known.
So -host,featA=on,featB=off will implicitly enable featC and is therefore
not be migration-safe. You would have to disable featC for compatibility
machines on the host model. Is something like that done? I don't think so
(and at least s390x won't do it right now).

But, I can simply get rid of that remark.

> 
> Also: I wonder what should be the return value for "name" when
> expansion type is "full" and we don't have any static CPU models
> (like on x86). e.g. returning:
>   { name: "host", props: { foo: "on", bar: "on", ... }
> would make the interface not directly usable for the expansion of
> <cpu mode="host-model">. Maybe that means we have to add at least
> one static CPU model to x86, too?

I'd simply copy the name then. That's what I had in mind.
(actually I don't do it on s390x because it's easier to just rely
on the output of our conversion function).

> > +
> > +
> > +##
> > +# @query-cpu-model-expansion:
> > +#
> > +# Expands the given CPU model to different granularities, allowing tooling
> > +# to get an understanding what a specific CPU model looks like in QEMU
> > +# under a certain QEMU machine.  
> 
> Maybe "expands a given CPU model (or a combination of CPU model +
> additional options) to different granularities".
> 
> > +#
> > +# Expanding CPU models is in general independant of the accelerator, except
> > +# for models like "host" that explicitly rely on an accelerator and can
> > +# vary in different configurations.  
> 
> Unfortunately this is not true in x86. Documenting it this way
> might give people the wrong expectations.
> 
> Specific guarantees from arch-specific code could be documented
> somewhere, but: where?
> 
> Maybe arch-specific guarantees should actually become
> query-cpu-definitions fields (like we have "static" and
> "migration-safe" today). If people are really interested in
> accelerator-independent data, we need

Okay, so this could be extended later than.

> 
> 
> > This interface can therefore also be used
> > +# to query the "host" CPU model.
> > +#
> > +# Note: This interface should not be used when global properties of CPU classes
> > +#       are changed (e.g. via "-cpu ...").  
> 
> We could simply enumerate all cases that could affect the return
> value. e.g.:
> 
> # The data returned by this command may be affected by:
> #
> # * QEMU version: CPU models may look different depending on the
> #   QEMU version. (Except for CPU models reported as "static"
> #   in query-cpu-definitions.)
> # * machine-type: CPU model expansion may look different depending
> #   on the machine-type. (Except for CPU models reported as "static"
> #   in query-cpu-definitions.)
> # * machine options (including accelerator): in some
> #   architectures, CPU models may look different depending on
> #   machine and accelerator options. (Except for CPU models
> #   reported as "static" in query-cpu-definitions.)
> # * "-cpu" arguments and global properties: arguments to the -cpu
> #    option and global properties may affect expansion of CPU
> #    models. Using query-cpu-model-expansion while using "-cpu"
> #    or global properties is not advised.
> 

Yes, that's better.

> 
> > +#
> > +# s390x supports expanding of all CPU models with all expansion types. Other
> > +# architectures are not supported yet.  
> 
> I think this paragraph is likely to get obsolete very soon (as
> people may forget to update it when implementing the new
> interface on other architectures). Also, the paragraph is not
> true until patch 27/29 is applied.
> 
> Maybe write it as "Some architectures may not support all
> expansion types".

Agreed. And most likely x86 won't support expanding all CPU models I assume?

> 
> Patch 27/29 could add "s390x supports both 'static' and 'full'
> expansion types". I wouldn't document it as "supports all
> expansion types" because CpuModelExpansionType may be extended in
> the future.

Agreed.


Thanks!


David