Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 19.11.19 20:42, Eduardo Habkost wrote:
> On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote:
> > On 19.11.19 11:36, Peter Maydell wrote:
> > > On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <address@hidden> wrote:
> > > > On 19.11.19 10:22, Peter Maydell wrote:
> > > > > I don't hugely care about query-cpu-model-expansion. I
> > > > > just don't want it to have bad effects on the semantics
> > > > > of user-facing stuff like x- properties.
> > > >
> > > > IMHO, max should really include all features (yes, also the bad
> > > > x-features on arm :) ) and we should have a way to give users the
> > > > opportunity to specify "just give me the best model independent of the
> > > > accelerator" - something like a "best" model, but I don't care about the
> > > > name.
>
> I'm in agreement with Peter, here: if "max" is user-visible, it's
> better to make it provide more usable defaults.

Agreed, unless we warn the user about the model.

> > > How would "max includes all features" work if we have two
> > > x- features (or even two normal features!) which are incompatible
> > > with each other? How does it work for features which are
> >
> > I assume the "max" model should at least be consistent, see below.
> >
> > > valid for some other CPU type but not for 'max'? The design
> > > seems to assume a rather simplified system where every
> > > feature is independent and can always be applied to every
> > > CPU, which I don't think is guaranteed to be the case.
> >
> > I do agree that the use case of "max" for detecting which features can be
> > enabled for any CPU model is quite simplified and would also not work like
> > this on s390x. It really means "give me the maximum/latest/greatest you
> > can". Some examples on s390x:
> >
> > On s390x, we don't allow to enable new features for older CPU generation.
> > "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if
> > available. However, if you select a z12 (zEC12), you cannot enable "vx":
> >
> > "Feature '%s' is not available for CPU model '%s', it was introduced with
> > later models."
> >
> > Also, we have dependency checks
> > (target/s390x/cpu_models.c:check_consistency()), that at least warn on
> > inconsistencies between features (feature X requires feature Y).
> >
> > We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs.
> > z13-base.
> >
> > - z13-max: Maximum features that can be enabled on the current
> >             accel/host for a z13.
> > - z13-best: Best features that can be enabled on the current
> >             accel/host for a z13.
> > - z13-base: base feature set, independent of current
> >             accel/host for a z13. (we do have this already on s390x)
>
> We don't need to create new CPU types for that, do we?  These
> different modes could be configured by a CPU option (e.g.
> "z13,features=max"[1], "z13,features=best").

I somewhat don't like such options mixed into ordinary feature 
configuration if we can avoid it. It allows for things like

z13,features=max,features=best

The z13 model is migration-safe. So would be "z13,vx=off". 
"z13,features=best" would no longer be migration safe.

IOW, reusing an existing model along with that feels wrong, read below. 
Especially, I dislike that one config option (features=best) disables 
and enables features at the same time. Read below.

> If we do add new CPU options to tune feature defaults, libvirt
> can start using these options on query-cpu-model-expansion, and
> everybody will be happy.  No need to change defaults in the "max"
> CPU model in a way that makes it less usable just to make
> query-cpu-model-expansion work.
>
> [1] Probably we need to call it something else instead of
>      "features=max", just to avoid confusion with the "max" CPU
>      model.  Maybe "experimental-features=on",
>      "recommended-features=on"?
We already do have feature groups on s390x. So these could behave like 
"host/accelerator dependent" feature groups. I would prefer that over 
the suggestion above.

The only downside is that z13,recommended-features=on could actually 
"disable" features that are already in z13 (e.g., "vx" is part of z13, 
but if it's not available in the host we would have to disable it). I 
don't like these semantic. Maybe introducing new types of models that 
don't have any features as default could come into play.

E.g.,

z13-best => z13-raw,recommended-features=on
z13-max => z13-raw,recommended-features=on,experimental-features=on

Maybe we can find a better name for "recommended-features" and 
"experimental-features" to highlight that these are only "features 
available via the accelerator in the current host"

We could also expose:

z13-full => z13-raw,all-features=on

Which would include all features theoretically valid for a model (but 
even if not available).

--

Thanks,

David / dhildenb