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

[Eduardo Habkost]

On Tue, Aug 02, 2016 at 05:04:05PM +0200, David Hildenbrand wrote:
[...]
> > 
> > > +#          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?

We could have a very minimal CPU model that is static in x86,
yes. We just need to find out "how minimal" we can really make
it.

> 
> > 
> > > +#
> > > +# @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

In this case, "host" is not migration-safe, so the results will
not be migration-safe.

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

I think we can keep it, and change it later if knowing about
migration-safety of "full" is deemed important. Being explicit
about guarantees we do _not_ give (yet) doesn't hurt, even if we
change the docs later to be explicit about extra guarantees we
didn't document before.

> 
> > 
> > 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).

Copying the name should work for the first version. It would be
less useful for <cpu model="host-model">, but we can improve it
later.

> 
[...]
> > > +#
> > > +# 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

Oops, forgot to finish this paragraph:

If people are really interested in accelerator-independent data,
we could add an additional query-cpu-definitions field stating
that. (But I doubt we will need it)

> 
> Okay, so this could be extended later than.
[...]
> > 
> > > +#
> > > +# 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?

It will probably support expanding all CPU models in the same way
(when using "full").

-- 
Eduardo