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

[David Hildenbrand <david@redhat.com>]

On 20.11.19 15:04, Eduardo Habkost wrote:
> 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 <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).
> 
> 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.

Makes sense.

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

I mostly only care about z13-best or "z13-raw,recommended-features=on" 
for now. What's described in patch #2. Allow upper layers (or user on 
the command line) get a CPU model that has the best features supported 
by the accelerator on the current host. Esp., include new features to 
mitigate security issues.


-- 

Thanks,

David / dhildenb