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

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

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

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

Thanks a lot!!!

David