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

[Eduardo Habkost]

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.


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

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

-- 
Eduardo