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

[Eduardo Habkost]

On Wed, Nov 20, 2019 at 11:28:29AM +0100, David Hildenbrand wrote:
> 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).

I don't have a strong opinion on the specifics.  If you believe
that's the best solution, I trust your judgement.

My only point was that we don't always need to add new CPU types
for every conceivable use case, and some (but not all) problems
can be solved by simple new properties.

(To be honest, I'm now having trouble mapping these ideas to real
world problems or use cases.  What are the specific problems
we're trying to solve right now?)

-- 
Eduardo