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

[David Hildenbrand]

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