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

[Eduardo Habkost]

On Tue, Aug 02, 2016 at 05:15:54PM +0200, David Hildenbrand wrote:
> > > +# @CpuModelCompareResult:
> > > +#
> > > +# An enumeration of CPU model comparation results.
> > > +#
> > > +# @incompatible: both model definition are incompatible
> > > +#
> > > +# @identical: model A == model B
> > > +#
> > > +# @superset: model A > model B
> > > +#
> > > +# @subset: model A < model B  
> > 
> > We need to clarify what superset/subset, ">" and "<" really mean.
> > 
> 
> I think this is "feature subset" on the one hand and "earlier generation"
> on the other hand - at least for s390x. But it boils down to runnability I
> think: (< and > are actually quite misleading)

It sounds like we need to clarify what are the use cases and
requirements, to find out how to document the exact meaning of
"superset" and "subset".

I assume this is closely related to the semantics of
query-cpu-model-baseline (which I didn't review yet). In other
words: "superset" and "subset" means you can save a
query-cpu-model-baseline call because you already know modela or
modelb can be used as baseline, already. Is that correct?

In this case, I will get back to this while reviewing and
discussing query-cpu-model-baseline.

> 
> # @incompatible: both model definition are incompatible. A host which
> #                can run model A can't run model B and the other way around.
> #
> # @identical: model A == model B. A host which can run model A can run
> #             model B and the other way around.
> #
> # @superset: A host which can run model A can run model B, but not the
> #            other way around.
> #
> # @subset: A host which can run model B can run model A, but not the
> #          other way around.
> 
> > > +#
> > > +# Since: 2.8.0
> > > +##
> > > +{ 'enum': 'CpuModelCompareResult',
> > > +  'data': [ 'incompatible', 'identical', 'superset', 'subset' ] }  
> > 
> > I assume implementations are free to return "incompatible" if
> > they still don't have any extra logic to expand CPU models and
> > check for supersets/subsets. If that's the case, see my
> > suggestion below for a generic comparison function.
> 
> incompatible basically means, you have to baseline to create a runnable CPU
> model. Could be done, but see my last comment.
> 
> > 
> > > +
> > > +##
> > > +# @CpuModelCompareInfo
> > > +#
> > > +# The result of a CPU model comparison.
> > > +#
> > > +# @result: The result of the compare operation.
> > > +# @responsible-properties: List of properties that led to the comparison 
> > > result
> > > +#                          not being identical.
> > > +#
> > > +# @responsible-properties is a list of QOM property names that led to
> > > +# both CPUs not being detected as identical. For identical models, this
> > > +# list is empty.
> > > +# If a QOM property is read-only, that means there's no known way to 
> > > make the
> > > +# CPU models identical. If the special property name "type" is included, 
> > > the
> > > +# models are by definition not identical and cannot be made identical.
> > > +#
> > > +# Since: 2.8.0
> > > +##
> > > +{ 'struct': 'CpuModelCompareInfo',
> > > +  'data': {'result': 'CpuModelCompareResult',
> > > +           'responsible-properties': ['str']
> > > +          }
> > > +}
> > > +
> > > +##
> > > +# @query-cpu-model-comparison:
> > > +#
> > > +# Compares two CPU models, returning how they compare under a specific 
> > > QEMU
> > > +# machine.
> > > +#
> > > +# Note: This interface should not be used when global properties of CPU 
> > > classes
> > > +#       are changed (e.g. via "-cpu ...").
> > > +#
> > > +# s390x supports comparing of all CPU models.  
> > 
> > This statement is not true until patch 28/29 is applied.
> 
> Yes, will move it (also for the baseline patch),
> 
> > 
> > > Other architectures are not
> > > +# supported yet.  
> > 
> > What if we provide a generic comparison function that does like
> > the following pseudocode:
> > 
> > def basic_comparison(modela, modelb):
> >   if modela.name == modelb.name:
> >     if modela.props == modelb.props:
> >       return "identical", []
> >     else:
> >       #XXX: maybe add some extra logic to check if
> >       # modela.props is a subset or superset of modelb.props?
> >       return "incompatible", set(modela.props.keys(), modelb.props.keys())
> >   else:
> >     return "incompatible", ["type"]
> > 
> > def full_comparison(modela, modelb):
> >   r,p = basic_comparison(modela, modelb)
> >   if r == "incompatible":
> >     try:
> >       modela = expand_cpu_model(modela, "full")
> >       modelb = expand_cpu_model(modelb, "full")
> >     except:
> >       # in case "full" expansion mode is not supported
> >       return r,p
> >     return basic_comparison(modela, modelb)
> > 
> 
> While I agree that that would be nice to have, it doesn't fit for s390x right
> now: The result on s390x does not rely on features/name only, but internal 
> data
> we don't want to expose.
> 
> e.g. z13.2 and z13s are considered identically.
> 
> z13 is a subset of z13.2, although they have exactly the same
> features/properties (right now). It basically has to do with internal data
> (e.g. address bus sizes for hamax as an example)
> 
> (that's where we indictate "type" under "responsible-properties" - they can
> never be made identically, you have to baseline).

Right, I don't mean it to be enough for all architectures, but as
a basic implementation that is minimally useful when there's no
arch-specific comparison function.

-- 
Eduardo