Re: [PATCH v2 2/2] s390x/cpumodel: Introduce dynamic feature groups

[Eduardo Habkost]

On Mon, Dec 02, 2019 at 10:15:12AM +0100, David Hildenbrand wrote:
> 
> >> Say the user has the option to select a model (zEC12, z13, z14), upper
> >> layers always want to have a model that includes all backported security
> >> features. While the host model can do that, CPU definitions can't. You
> >> can't change default models within a QEMU release, or for older releases
> >> (e.g., a z13).
> >>
> > 
> > This is a good description of the main use case we're worried
> > about in x86 too, and the main reason we have added versioned CPU
> > models.
> > 
> > I remember I was planning to use `query-cpu-model-expansion` for
> > "please give me the best configuration for this specific CPU
> > model" (which would be very similar to the approach used in this
> > series).  Now, I need to refresh my memory and try to remember
> > why I concluded this approach wouldn't work for x86.
> 
> I would be interested in that - I don't really think exposing CPU
> versions to the user is necessary here.
> 
> E.g., you can maintain the versions internally and enable the stored
> features of the fitting one with "recommended-features=on...".

I was re-reading some code and threads, and now I remember: the
main obstacle for using query-cpu-model-expansion for CPU model
version resolution in x86 is the fact that the x86 CPU models
aren't static yet.  (type=full expansion isn't useful for CPU the
use case above; type=static expansion requires static CPU models
to be useful)

I was planning to make x86 CPU models static, then I noticed we
do have lots of feature flags that depend on the current
accelerator (set by kvm_default_props) or current machine (set
by compat_props).  This breaks the rules for static CPU models.

We can still try to provide useful static CPU models in x86 in
the future (I want to).  But I don't want to make this an
obstacle for providing a CPU model update mechanism that works
for x86 (which is more urgent).

> 
> > 
> > 
> >>>
> >>> Maybe its just the interface or the name. But I find this very 
> >>> non-intuitive
> >>
> >> I'm open for suggestions.
> >>
> >>>
> >>> e.g. you wrote
> >>>
> >>>     Get the maximum possible feature set (e.g., including deprecated
> >>>     features) for a CPU definition in the configuration ("everything that
> >>>     could be enabled"):
> >>>         -cpu z14,all-features=off,available-features=on
> >>>
> >>>     Get all valid features for a CPU definition:
> >>>         -cpu z14,all-features=on
> >>>
> >>> What is the point of this? It is either the same as the one before, or it 
> >>> wont
> >>> be able to start. 
> >>
> >> valid != available, all != available. Yes, the model won't run unless
> >> you are on pretty good HW :)
> >>
> >> Maybe I should just have dropped the last example, as it seems to
> >> confuse people - it's mostly only relevant for introspection via CPU
> >> model expansion.
> >>
> >> I am open for better names. e.g. all-features -> valid-features.
> > 
> > "all" is not a meaningful name to me.  It surely doesn't mean
> > "all features in the universe", so it means a more specific set
> > of features.  How is that set defined?
> > 
> > "valid" seems clearer, but we still need a description of what
> > "valid" means exactly.
> > 
> 
> So, we have
> 
> +static S390DynFeatGroupDef s390_dyn_feature_groups[] = {
> +    /* "all" corresponds to our "full" definitions */
> +    DYN_FEAT_GROUP_INIT("all-features", ALL, "Features valid for a CPU
> definition"),
> [...]
> +};
> 
> it includes features that are not available - all features that could
> theoretically be enabled for that CPU definition.
> 
> (e.g., "vx" was introduced with z13 and cannot be enabled for the z12.
> It's part of the full model of a z13, but not of a z12)

Isn't this something already returned by device-list-properties?

-- 
Eduardo