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

[Collin Walling]

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

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

Thanks for your feedback! Will work on the next proposal that implements
the above. If you have any strong suggestions otherwise, please let me
know :)

-- 
Regards,
  Collin