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

[David Hildenbrand <david@redhat.com>]

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 <david@redhat.com> 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).

-- 

Thanks,

David / dhildenb