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

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

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

Depends on the order of parameters. But I think this was clarified to my reply
in the cover letter. Will improve the overall explanation of
query-cpu-model-comparison

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

That sure makes sense, they go hand-in-hand, especially for the two commands
"virsh cpu-baseline" and "virsh cpu-compare". It's all about testing/detecting
runnable CPU models for different environments.

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

Then the question would be: Is it better to have a wrong result than any result?
As it doesn't work on s390x, there could also be a problem with other
architectures.

Especially when comparing against "host", the name after expansion would give
no identication about runnability. And only living with
"incompatible" and "identical" might also not really be enough.

While this could make sense, I'd suggest postponing such a basic comapare
function until other architectures support the basics (expanding to full) and
then see if this basic function would work for them (maybe x86 is a good
candidate once the expanded name would not be "host" anymore).

Thanks Eduardo!

David