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

[David Hildenbrand <david@redhat.com>]

On 05.12.19 15:35, Eduardo Habkost wrote:
> 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 think, you could if you would expand "best X" to something like

-cpu X,all-features=off,featX=on,featY=on ...

The "all-features" part would need a better name as discussed. Such a
model would always have a defined feature set (all listed features) ==
static. The list could get a little longer, which is why s390x has these
static "base" features. But that's not a road blocker.

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

The static models we have (e.g., z13-base) contain a minimum set of
features we expect to be around in every environment (but doesn't have
to). It's just a way to make the featX=on,featY=on ... list shorter.

X would be expanded to e.g.,

-cpu X-base,featX=on,featY=on ...

But nothing speaks against having

-cpu X-base,featX=off,featY=on ...

A very simplistic base model would be a model without any features.
(like -cpu X,all-features=off), but then it would be set in stone.

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

We do register all feature properties for all models. So, yes, it would
have been possible if we (I) would have implemented that differently. We
could (and maybe should) still change that - only register the features
that are part of the "full" model.

-- 
Thanks,

David / dhildenb