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

[David Hildenbrand]

>> 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.
> 
> x86 has only one static CPU model, called "base", just to make
> type=static expansion work.  Having multiple "<model>-base" CPU
> models would help make the extra feature list shorter, yes.

On s390x, we also glue some magic numbers to the models (e.g., CPU ID,
IBC value, HW generation). IOW, you can't
make a z12 to a z13 just by adding features. The guest is able to
observe these magic numbers. That's also one reason we need distinct
base models.

> 
> But we would still need to decide how to handle the
> accel-specific code in x86_cpu_load_model(), including:
> * kvm_default_props/tcg_default_props;
> * x2apic special case for !kvm_irqchip_in_kernel();
> * host vendor ID special case for KVM.

What would happen right now if you would do a static expansion of e.g.,
"SandyBridge" or of "host"? wouldn't all these knobs also have to be
expanded as well?

> 
> If we include that in static expansion, it would be a large
> number of user-visible side effects for something that was
> supposed to just add/remove a tiny set of CPU features to an
> existing configuration.  If we don't, we are breaking the rules
> of static expansion (aren't we?).

I guess we would have to include - but a long list of features wouldn't
really be problematic, right? (at least when tooling just expands and
passes on whatever it gets). Same as when expanding "host".

> 
> We can still try to address this and make
> "query-cpu-model-expansion type=static ...,recommended-features=on"
> work on x86, and see it is usable by libvirt in x86.  I'm just
> worried that the interface may become complex, easy to get wrong,
> and hard to validate until full libvirt support is implemented.
> query-cpu-model-expansion is very extensible and flexible, but
> hard to explain and reason about.
> 

I don't see a real alternative to get "the best model of a specific
generation for the current accel+hw+firmware". Ack that we should
clarify all implications (and requirements) first, before taking that path.

At least the concept of feature groups should be easy to explain.

[...]
>>>> +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.
> 
> Understood.  When exactly would all-features=on be useful for
> management software?
> 

I guess "all-features=on"  would only be useful for some sort of
introspection (or testing), but not actually for something else I guess.

One interesting use case of "all-features=off/recommended-features=on"
could be

-cpu host,all-features=off,recommended-features=on

Would allow us to disable all deprecated features when starting a new VM
that we have to keep in the "host" model to keep baseline/runnability
tests working and migration of old VMs unchanged.

-- 
Thanks,

David / dhildenb