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

[David Hildenbrand <david@redhat.com>]

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?

-- 
Cheers,

David / dhildenb