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

[David Hildenbrand]

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?

--
Cheers,

David / dhildenb