Re: [PATCH v2 2/3] target/s390x: add support for "disable-deprecated-feats" expansion option

[Collin Walling]

On 4/25/24 04:10, David Hildenbrand wrote:
> On 24.04.24 20:33, Collin Walling wrote:
>> On 4/24/24 03:24, David Hildenbrand wrote:
>>> On 23.04.24 23:06, Collin Walling wrote:
>>>> Retain a list of deprecated features disjoint from any particular
>>>> CPU model. When a query-cpu-model-expansion is provided with the
>>>> "disable-deprecated-feats" option set, the resulting properties list
>>>> will include all deprecated features paired with false. Example:
>>>>
>>>>    { ... "bpb": false, "csske": false, ...}
>>>>
>>>> It is recommended that s390 guests operate with these features
>>>> explicitly disabled to ensure compatability with future hardware.
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>    target/s390x/cpu_features.c      | 14 ++++++++++++++
>>>>    target/s390x/cpu_features.h      |  1 +
>>>>    target/s390x/cpu_models_sysemu.c | 20 ++++++++++++--------
>>>>    3 files changed, 27 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>>>> index d28eb65845..efafc9711c 100644
>>>> --- a/target/s390x/cpu_features.c
>>>> +++ b/target/s390x/cpu_features.c
>>>> @@ -212,6 +212,20 @@ void s390_feat_bitmap_to_ascii(const S390FeatBitmap 
>>>> features, void *opaque,
>>>>        };
>>>>    }
>>>>    
>>>> +void s390_get_deprecated_features(S390FeatBitmap features)
>>>> +{
>>>> +    static const int feats[] = {
>>>> +         /* CSSKE is deprecated on newer generations */
>>>> +         S390_FEAT_CONDITIONAL_SSKE,
>>>> +         S390_FEAT_BPB,
>>>> +    };
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(feats); i++) {
>>>> +        set_bit(feats[i], features);
>>>> +    }
>>>> +}
>>>> +
>>>>    #define FEAT_GROUP_INIT(_name, _group, _desc)        \
>>>>        {                                                \
>>>>            .name = _name,                               \
>>>> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
>>>> index a9bd68a2e1..661a8cd6db 100644
>>>> --- a/target/s390x/cpu_features.h
>>>> +++ b/target/s390x/cpu_features.h
>>>> @@ -69,6 +69,7 @@ void s390_add_from_feat_block(S390FeatBitmap features, 
>>>> S390FeatType type,
>>>>                              uint8_t *data);
>>>>    void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void 
>>>> *opaque,
>>>>                                   void (*fn)(const char *name, void 
>>>> *opaque));
>>>> +void s390_get_deprecated_features(S390FeatBitmap features);
>>>>    
>>>>    /* Definition of a CPU feature group */
>>>>    typedef struct {
>>>> diff --git a/target/s390x/cpu_models_sysemu.c 
>>>> b/target/s390x/cpu_models_sysemu.c
>>>> index ef9fa80efd..b002819188 100644
>>>> --- a/target/s390x/cpu_models_sysemu.c
>>>> +++ b/target/s390x/cpu_models_sysemu.c
>>>> @@ -171,7 +171,8 @@ static void qdict_add_enabled_feat(const char *name, 
>>>> void *opaque)
>>>>    
>>>>    /* convert S390CPUDef into a static CpuModelInfo */
>>>>    static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel 
>>>> *model,
>>>> -                                bool delta_changes)
>>>> +                                bool delta_changes,
>>>> +                                bool disable_deprecated_feats)
>>>>    {
>>>>        QDict *qdict = qdict_new();
>>>>        S390FeatBitmap bitmap;
>>>> @@ -201,6 +202,13 @@ static void cpu_info_from_model(CpuModelInfo *info, 
>>>> const S390CPUModel *model,
>>>>            s390_feat_bitmap_to_ascii(bitmap, qdict, 
>>>> qdict_add_disabled_feat);
>>>>        }
>>>>    
>>>> +    /* features flagged as deprecated */
>>>> +    if (disable_deprecated_feats) {
>>>> +        bitmap_zero(bitmap, S390_FEAT_MAX);
>>>> +        s390_get_deprecated_features(bitmap);
>>>> +        s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_disabled_feat);
>>>> +    }
>>>
>>> Likely, we should remove these features from the actual bitmap, such
>>> that they won't appear twice in the output? I'd expect the
>>> cpu_info_from_model() caller to handle that.
>>>
>>> Just adding them to the list as disabled is likely wrong.
>>>
>>> For example, if someone were to expend a given model with "... bpb:
>>> true" with disable-deprecated-feat=on, that should be remove from
>>> "bpb:true", and only replaced by "bpb=false" if it would be part of the
>>> CPU model we would be expanding to.
>>>
>>> Or am I missing something?
>>>
>>
>> qdict_add_disabled_feat will handle updating the feature if it already
>> exists. I placed the code to process deprecated features as the last
>> step of cpu_info_from_model to override any features that have already
>> been added to the bitmap. Whether it should be the deprecated feats that
>> take priority, or what the user requests is up in the air, however...
> 
> Yes, that's one of my concern. IIRC, if the user specifies the same 
> property multiple times, it's unclear which one will win.
> 
> "bpb=true,bpb=false" might mean that bpb=true might win.
> 
> I think this is something that this interface should sort out, so it 
> will be actually usable!
> 
>>
>> ... Daniel's suggestion to modify the QMP response to include a separate
>> list of "deprecated-props" seems like a much more efficient and readable
>> way that alleviates both your and Markus' concerns.
> 
> Would you only include properties that would apply to the current model 
> and would be set to true in the current model?
> 
> How would libvirt make use of this interface, and could it run into the 
> issue spelled out above?
> 

The way I see having a "deprecated-props" array included in the
expansion response is that it just simply signals to the user/management
app which features are deprecated. From there, the next step to set up a
CPU model with these features turned off can be performed.

I've worked on the libvirt portion for the v1 of this QEMU patch set,
and the idea is to introduce a new CPU model type called
"host-recommended". When a libvirt domain is defined with this type,
then the CPU model will become the host-model with deprecated features
turned off.

This is, of course, a big discussion to be had once the patches are
upstream. They will be an RFC since I imagine a lot of ideas will be
thrown around.

Once the QEMU portion is accepted, I'll fix up what I have for libvirt
to properly query for the deprecated features and work them into my design.

-- 
Regards,
  Collin