[PATCH v2 0/3] query-cpu-model-expansion: add disable-deprecated-feats arg

[PATCH v2 0/3] query-cpu-model-expansion: add disable-deprecated-feats arg

[Collin Walling]

The current implementation of query-cpu-model-expansion is lacking a way to
conditionally retrieve CPU models with properties (i.e. features) that are
flagged as deprecated set to disabled. To remedy this, a new optional boolean
"disable-deprecated-feats" argument has been added to the query.

Here is a sample QMP command that includes this new argument:

    { "execute": "query-cpu-model-expansion", "arguments": { "type": "full", "model": { "name": "host" }, "disable-deprecated-features": true}}

This patchset adds full support for this argument on s390x. A simple interface is
designed that contains an array of feature bits that are flagged for deprecation.
This list may be easily populated with more features in the future.

    void s390_get_deprecated_features(S390FeatBitmap features)
    {
        static const int feats[] = {
             /* CSSKE is deprecated on newer generations */
             S390_FEAT_CONDITIONAL_SSKE,
             S390_FEAT_BPB,
             /* Deprecated on z16 */
             S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE,
             S390_FEAT_TRANSACTIONAL_EXE
        };
        int i;

        for (i = 0; i < ARRAY_SIZE(feats); i++) {
            set_bit(feats[i], features);
        }
    }

For architectures that support model expansion but do not support the new arg,
an error message will print to stdio if this option is present.

Use case example:

Newer s390 machines may signal the end-of-support for particular CPU features,
rendering guests operating with older CPU models incapable of running on
said machines. A manual effort to disable certain CPU features would be
required.

This feature may alleviate this issue by allowing for a query of a CPU model
with deprecated features listed in the props response paired with false. The
caller of this query may use this information to explicitly disable these
features for the guest's CPU model, ensuring a safe migration to the next
generation if desired.

I have developed a way for libvirt to make use of this functionality, but I
will hold back review on those patches until a QEMU interface is agreed upon.

Collin L. Walling (3):
  cpu-models: add "disable-deprecated-feats" option to cpu model
    expansion
  target/s390x: add support for "disable-deprecated-feats" expansion
    option
  target/s390x: flag te and cte as deprecated

 qapi/machine-target.json         |  7 ++++++-
 target/arm/arm-qmp-cmds.c        |  7 +++++++
 target/i386/cpu-sysemu.c         |  7 +++++++
 target/s390x/cpu_features.c      | 17 +++++++++++++++++
 target/s390x/cpu_features.h      |  1 +
 target/s390x/cpu_models_sysemu.c | 17 ++++++++++++++---
 6 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

This optional parameter for query-cpu-model-expansion enables CPU
model features flagged as deprecated to appear in the resulting
list of properties.

This commit does not add support beyond adding a new argument
to the query. All queries with this option present will result
in an error claiming this option is not supported.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 qapi/machine-target.json         | 7 ++++++-
 target/arm/arm-qmp-cmds.c        | 7 +++++++
 target/i386/cpu-sysemu.c         | 7 +++++++
 target/s390x/cpu_models_sysemu.c | 7 +++++++
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 29e695aa06..b9da284d2d 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -285,6 +285,10 @@
 #
 # @type: expansion type, specifying how to expand the CPU model
 #
+# @disable-deprecated-feats: include CPU model features that are
+#     flagged as deprecated. If supported, these features will appear
+#     in the properties list paired with false.
+#
 # Returns: a CpuModelExpansionInfo describing the expanded CPU model
 #
 # Errors:
@@ -298,7 +302,8 @@
 ##
 { 'command': 'query-cpu-model-expansion',
   'data': { 'type': 'CpuModelExpansionType',
-            'model': 'CpuModelInfo' },
+            'model': 'CpuModelInfo',
+            '*disable-deprecated-feats': 'bool' },
   'returns': 'CpuModelExpansionInfo',
   'if': { 'any': [ 'TARGET_S390X',
                    'TARGET_I386',
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 3cc8cc738b..1010d654e3 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -100,6 +100,8 @@ static const char *cpu_model_advertised_features[] = {
 
 CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
                                                      CpuModelInfo *model,
+                                                     bool has_disable_deprecated_feats,
+                                                     bool disable_deprecated_feats,
                                                      Error **errp)
 {
     CpuModelExpansionInfo *expansion_info;
@@ -110,6 +112,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     const char *name;
     int i;
 
+    if (has_disable_deprecated_feats) {
+        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
+        return NULL;
+    }
+
     if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
         error_setg(errp, "The requested expansion type is not supported");
         return NULL;
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 3f9093d285..c15786fb66 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -196,6 +196,8 @@ out:
 CpuModelExpansionInfo *
 qmp_query_cpu_model_expansion(CpuModelExpansionType type,
                                                       CpuModelInfo *model,
+                                                      bool has_disable_deprecated_feats,
+                                                      bool disable_deprecated_feats,
                                                       Error **errp)
 {
     X86CPU *xc = NULL;
@@ -204,6 +206,11 @@ qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     QDict *props = NULL;
     const char *base_name;
 
+    if (has_disable_deprecated_feats) {
+        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
+        goto out;
+    }
+
     xc = x86_cpu_from_model(model->name, model->props, "model.props", &err);
     if (err) {
         goto out;
diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index 2d99218069..ef9fa80efd 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -210,6 +210,8 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
 
 CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
                                                       CpuModelInfo *model,
+                                                      bool has_disable_deprecated_feats,
+                                                      bool disable_deprecated_feats,
                                                       Error **errp)
 {
     Error *err = NULL;
@@ -217,6 +219,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     S390CPUModel s390_model;
     bool delta_changes = false;
 
+    if (has_disable_deprecated_feats) {
+        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
+        return NULL;
+    }
+
     /* convert it to our internal representation */
     cpu_model_from_info(&s390_model, model, "model", &err);
     if (err) {
-- 
2.43.0

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

[Collin Walling]

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);
+    }
+
     if (!qdict_size(qdict)) {
         qobject_unref(qdict);
     } else {
@@ -219,11 +227,6 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     S390CPUModel s390_model;
     bool delta_changes = false;
 
-    if (has_disable_deprecated_feats) {
-        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
-        return NULL;
-    }
-
     /* convert it to our internal representation */
     cpu_model_from_info(&s390_model, model, "model", &err);
     if (err) {
@@ -241,7 +244,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     /* convert it back to a static representation */
     expansion_info = g_new0(CpuModelExpansionInfo, 1);
     expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
-    cpu_info_from_model(expansion_info->model, &s390_model, delta_changes);
+    cpu_info_from_model(expansion_info->model, &s390_model,
+                        delta_changes, disable_deprecated_feats);
     return expansion_info;
 }
 
@@ -390,7 +394,7 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 
     baseline_info = g_new0(CpuModelBaselineInfo, 1);
     baseline_info->model = g_malloc0(sizeof(*baseline_info->model));
-    cpu_info_from_model(baseline_info->model, &model, true);
+    cpu_info_from_model(baseline_info->model, &model, true, false);
     return baseline_info;
 }
 
-- 
2.43.0

[PATCH v2 3/3] target/s390x: flag te and cte as deprecated

[Collin Walling]

Add the CONSTRAINT_TRANSACTIONAL_EXE (cte) and TRANSACTIONAL_EXE (te)
to the list of deprecated features.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 target/s390x/cpu_features.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index efafc9711c..cb4e2b8920 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -218,6 +218,9 @@ void s390_get_deprecated_features(S390FeatBitmap features)
          /* CSSKE is deprecated on newer generations */
          S390_FEAT_CONDITIONAL_SSKE,
          S390_FEAT_BPB,
+         /* Deprecated on z16 */
+         S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE,
+         S390_FEAT_TRANSACTIONAL_EXE
     };
     int i;
 
-- 
2.43.0

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Markus Armbruster]

Collin Walling <walling@linux.ibm.com> writes:

> This optional parameter for query-cpu-model-expansion enables CPU
> model features flagged as deprecated to appear in the resulting
> list of properties.
>
> This commit does not add support beyond adding a new argument
> to the query. All queries with this option present will result
> in an error claiming this option is not supported.
>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  qapi/machine-target.json         | 7 ++++++-
>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>  target/i386/cpu-sysemu.c         | 7 +++++++
>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>  4 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 29e695aa06..b9da284d2d 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -285,6 +285,10 @@
>  #
>  # @type: expansion type, specifying how to expand the CPU model
>  #
> +# @disable-deprecated-feats: include CPU model features that are
> +#     flagged as deprecated. If supported, these features will appear
> +#     in the properties list paired with false.

What's the default?

Which command result(s) does this affect?  Suggest to explain using
unabridged example QMP input and output before and after this series.

We generally avoid abbreviations in QMP names.  Let's call this
@disable-deprecated-features.

Separate sentences with two spaces for consistency, please.

> +#
>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>  #
>  # Errors:
> @@ -298,7 +302,8 @@
>  ##
>  { 'command': 'query-cpu-model-expansion',
>    'data': { 'type': 'CpuModelExpansionType',
> -            'model': 'CpuModelInfo' },
> +            'model': 'CpuModelInfo',
> +            '*disable-deprecated-feats': 'bool' },
>    'returns': 'CpuModelExpansionInfo',
>    'if': { 'any': [ 'TARGET_S390X',
>                     'TARGET_I386',
                      'TARGET_ARM',
                      'TARGET_LOONGARCH64',
                      'TARGET_RISCV' ] } }

Put a pin into this conditional: [*].

> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index 3cc8cc738b..1010d654e3 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -100,6 +100,8 @@ static const char *cpu_model_advertised_features[] = {
>  
>  CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>                                                       CpuModelInfo *model,
> +                                                     bool has_disable_deprecated_feats,
> +                                                     bool disable_deprecated_feats,
>                                                       Error **errp)
>  {
>      CpuModelExpansionInfo *expansion_info;
> @@ -110,6 +112,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      const char *name;
>      int i;
>  
> +    if (has_disable_deprecated_feats) {
> +        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
> +        return NULL;
> +    }

Reject the new argument in the ARM version, ...

> +
>      if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>          error_setg(errp, "The requested expansion type is not supported");
>          return NULL;
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 3f9093d285..c15786fb66 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -196,6 +196,8 @@ out:
>  CpuModelExpansionInfo *
>  qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>                                                        CpuModelInfo *model,
> +                                                      bool has_disable_deprecated_feats,
> +                                                      bool disable_deprecated_feats,
>                                                        Error **errp)
>  {
>      X86CPU *xc = NULL;
> @@ -204,6 +206,11 @@ qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      QDict *props = NULL;
>      const char *base_name;
>  
> +    if (has_disable_deprecated_feats) {
> +        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
> +        goto out;
> +    }

... the i386 version, ...

> +
>      xc = x86_cpu_from_model(model->name, model->props, "model.props", &err);
>      if (err) {
>          goto out;
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index 2d99218069..ef9fa80efd 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -210,6 +210,8 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>  
>  CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>                                                        CpuModelInfo *model,
> +                                                      bool has_disable_deprecated_feats,
> +                                                      bool disable_deprecated_feats,
>                                                        Error **errp)
>  {
>      Error *err = NULL;
> @@ -217,6 +219,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      S390CPUModel s390_model;
>      bool delta_changes = false;
>  
> +    if (has_disable_deprecated_feats) {
> +        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
> +        return NULL;
> +    }

... and the S390 version, but ...

> +
>      /* convert it to our internal representation */
>      cpu_model_from_info(&s390_model, model, "model", &err);
>      if (err) {

... neither the loongarch not the RISC-V version, which according to
condition [*] above also implement the command[*].  Bug?

Peeking ahead in the series, I see that you implement
@disable-deprecated-feats only for S390.

Having to reject @disable-deprecated-feats in targets that implement
query-cpu-model-expansion, but not the @disable-deprecated-feats, is
problematic:

1. If we implement query-cpu-model-expansion for another target, we need
to remember rejecting @disable-deprecated-feats.  Trap for the unwary.

2. query-qmp-schema can't tell whether the argument is supported.

You could make @query-cpu-model-expansion conditional on S390.

Since conditional arguments require 'boxed': true, you first have to
do that, like so:

    { 'command': 'query-cpu-model-expansion', 'boxed': true,
      'data': 'Foo',
      'returns': 'CpuModelExpansionInfo',
      'if': { 'any': [ 'TARGET_S390X',
                       'TARGET_I386',
                       'TARGET_ARM',
                       'TARGET_LOONGARCH64',
                       'TARGET_RISCV' ] } }

where Foo is

    { 'struct': 'Foo',
      'data': { 'type': 'CpuModelExpansionType',
                'model': 'CpuModelInfo' } }

Then add the conditional argument:

    { 'struct': 'Foo',
      'data': { 'type': 'CpuModelExpansionType',
                'model': 'CpuModelInfo' } }
                '*disable-deprecated-feats': { 'type': 'bool',
                                               'if': 'TARGET_S390X' }

Use a reasonable name instead of Foo, of course.

Disadvantages:

* More churn

* Possibly something else I can't see without trying it

Advantages:

* You don't have to reject the argument for all targets that don't
  implement it

* We can't forget to reject the argument when implementing
  query-cpu-model-expansion for another target

* query-cpu-model-expansion shows whether the argument is supported

I think you should give this a try.

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

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Daniel P. Berrangé]

On Tue, Apr 23, 2024 at 05:06:53PM -0400, Collin Walling wrote:
> This optional parameter for query-cpu-model-expansion enables CPU
> model features flagged as deprecated to appear in the resulting
> list of properties.
> 
> This commit does not add support beyond adding a new argument
> to the query. All queries with this option present will result
> in an error claiming this option is not supported.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  qapi/machine-target.json         | 7 ++++++-
>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>  target/i386/cpu-sysemu.c         | 7 +++++++
>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>  4 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 29e695aa06..b9da284d2d 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -285,6 +285,10 @@
>  #
>  # @type: expansion type, specifying how to expand the CPU model
>  #
> +# @disable-deprecated-feats: include CPU model features that are
> +#     flagged as deprecated. If supported, these features will appear
> +#     in the properties list paired with false.
> +#
>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>  #
>  # Errors:
> @@ -298,7 +302,8 @@
>  ##
>  { 'command': 'query-cpu-model-expansion',
>    'data': { 'type': 'CpuModelExpansionType',
> -            'model': 'CpuModelInfo' },
> +            'model': 'CpuModelInfo',
> +            '*disable-deprecated-feats': 'bool' },
>    'returns': 'CpuModelExpansionInfo',
>    'if': { 'any': [ 'TARGET_S390X',
>                     'TARGET_I386',

I think this is an odd design approach. Lets consider the
current output:

(QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
{
    "return": {
        "model": {
            "name": "z14-base",
            "props": {
                "aefsi": true,
                "aen": true,
                ...snip...
                "vxpd": true,
                "zpci": true
            }
        }
    }
}


If we want to inform a mgmt app of some features being deprecated,
why not just unconditionally include that info in the reply thus:


(QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
{
    "return": {
        "model": {
            "name": "z14-base",
            "props": {
                "aefsi": true,
                "aen": true,
                ...snip...
                "vxpd": true,
                "zpci": true
            }
            "deprecated-props": ["ppa15", "ri"]
        }
    }
}



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

On 4/24/24 02:19, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> This optional parameter for query-cpu-model-expansion enables CPU
>> model features flagged as deprecated to appear in the resulting
>> list of properties.
>>
>> This commit does not add support beyond adding a new argument
>> to the query. All queries with this option present will result
>> in an error claiming this option is not supported.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  qapi/machine-target.json         | 7 ++++++-
>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index 29e695aa06..b9da284d2d 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -285,6 +285,10 @@
>>  #
>>  # @type: expansion type, specifying how to expand the CPU model
>>  #
>> +# @disable-deprecated-feats: include CPU model features that are
>> +#     flagged as deprecated. If supported, these features will appear
>> +#     in the properties list paired with false.
> 
> What's the default?
> 
> Which command result(s) does this affect?  Suggest to explain using
> unabridged example QMP input and output before and after this series.
> 

Fair enough. Bool defaults to false but that's not apparent in the
description. I will add more detail.

> We generally avoid abbreviations in QMP names.  Let's call this
> @disable-deprecated-features.
> 

Okay.

> Separate sentences with two spaces for consistency, please.
> 

Understood.

>> +#
>>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>>  #
>>  # Errors:
>> @@ -298,7 +302,8 @@
>>  ##
>>  { 'command': 'query-cpu-model-expansion',
>>    'data': { 'type': 'CpuModelExpansionType',
>> -            'model': 'CpuModelInfo' },
>> +            'model': 'CpuModelInfo',
>> +            '*disable-deprecated-feats': 'bool' },
>>    'returns': 'CpuModelExpansionInfo',
>>    'if': { 'any': [ 'TARGET_S390X',
>>                     'TARGET_I386',
>                       'TARGET_ARM',
>                       'TARGET_LOONGARCH64',
>                       'TARGET_RISCV' ] } }
> 
> Put a pin into this conditional: [*].
> 
>> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
>> index 3cc8cc738b..1010d654e3 100644
>> --- a/target/arm/arm-qmp-cmds.c
>> +++ b/target/arm/arm-qmp-cmds.c
>> @@ -100,6 +100,8 @@ static const char *cpu_model_advertised_features[] = {
>>  
>>  CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>                                                       CpuModelInfo *model,
>> +                                                     bool has_disable_deprecated_feats,
>> +                                                     bool disable_deprecated_feats,
>>                                                       Error **errp)
>>  {
>>      CpuModelExpansionInfo *expansion_info;
>> @@ -110,6 +112,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>      const char *name;
>>      int i;
>>  
>> +    if (has_disable_deprecated_feats) {
>> +        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
>> +        return NULL;
>> +    }
> 
> Reject the new argument in the ARM version, ...
> 
>> +
>>      if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>>          error_setg(errp, "The requested expansion type is not supported");
>>          return NULL;
>> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
>> index 3f9093d285..c15786fb66 100644
>> --- a/target/i386/cpu-sysemu.c
>> +++ b/target/i386/cpu-sysemu.c
>> @@ -196,6 +196,8 @@ out:
>>  CpuModelExpansionInfo *
>>  qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>                                                        CpuModelInfo *model,
>> +                                                      bool has_disable_deprecated_feats,
>> +                                                      bool disable_deprecated_feats,
>>                                                        Error **errp)
>>  {
>>      X86CPU *xc = NULL;
>> @@ -204,6 +206,11 @@ qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>      QDict *props = NULL;
>>      const char *base_name;
>>  
>> +    if (has_disable_deprecated_feats) {
>> +        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
>> +        goto out;
>> +    }
> 
> ... the i386 version, ...
> 
>> +
>>      xc = x86_cpu_from_model(model->name, model->props, "model.props", &err);
>>      if (err) {
>>          goto out;
>> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
>> index 2d99218069..ef9fa80efd 100644
>> --- a/target/s390x/cpu_models_sysemu.c
>> +++ b/target/s390x/cpu_models_sysemu.c
>> @@ -210,6 +210,8 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>>  
>>  CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>                                                        CpuModelInfo *model,
>> +                                                      bool has_disable_deprecated_feats,
>> +                                                      bool disable_deprecated_feats,
>>                                                        Error **errp)
>>  {
>>      Error *err = NULL;
>> @@ -217,6 +219,11 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>      S390CPUModel s390_model;
>>      bool delta_changes = false;
>>  
>> +    if (has_disable_deprecated_feats) {
>> +        error_setg(&err, "Unsupported option 'disable-deprecated-feats'");
>> +        return NULL;
>> +    }
> 
> ... and the S390 version, but ...
> 
>> +
>>      /* convert it to our internal representation */
>>      cpu_model_from_info(&s390_model, model, "model", &err);
>>      if (err) {
> 
> ... neither the loongarch not the RISC-V version, which according to
> condition [*] above also implement the command[*].  Bug?
> 
> Peeking ahead in the series, I see that you implement
> @disable-deprecated-feats only for S390.
> 
> Having to reject @disable-deprecated-feats in targets that implement
> query-cpu-model-expansion, but not the @disable-deprecated-feats, is
> problematic:
> 
> 1. If we implement query-cpu-model-expansion for another target, we need
> to remember rejecting @disable-deprecated-feats.  Trap for the unwary.
> 
> 2. query-qmp-schema can't tell whether the argument is supported.
> 
> You could make @query-cpu-model-expansion conditional on S390.
> 
> Since conditional arguments require 'boxed': true, you first have to
> do that, like so:
> 
>     { 'command': 'query-cpu-model-expansion', 'boxed': true,
>       'data': 'Foo',
>       'returns': 'CpuModelExpansionInfo',
>       'if': { 'any': [ 'TARGET_S390X',
>                        'TARGET_I386',
>                        'TARGET_ARM',
>                        'TARGET_LOONGARCH64',
>                        'TARGET_RISCV' ] } }
> 
> where Foo is
> 
>     { 'struct': 'Foo',
>       'data': { 'type': 'CpuModelExpansionType',
>                 'model': 'CpuModelInfo' } }
> 
> Then add the conditional argument:
> 
>     { 'struct': 'Foo',
>       'data': { 'type': 'CpuModelExpansionType',
>                 'model': 'CpuModelInfo' } }
>                 '*disable-deprecated-feats': { 'type': 'bool',
>                                                'if': 'TARGET_S390X' }
> 
> Use a reasonable name instead of Foo, of course.
> 
> Disadvantages:
> 
> * More churn
> 
> * Possibly something else I can't see without trying it
> 
> Advantages:
> 
> * You don't have to reject the argument for all targets that don't
>   implement it
> 
> * We can't forget to reject the argument when implementing
>   query-cpu-model-expansion for another target
> 
> * query-cpu-model-expansion shows whether the argument is supported
> 
> I think you should give this a try.
> 
> 

Very cool. Avoiding the requirement of others architectures to catch
this conditional and handle it would definitely be nice. Thank you for
this insight. I'll play around with it and find a cleaner approach to
what I have.

I will need to reflect on Daniel's feedback as well, which suggests
simply listing deprecated features as a separate array in the QMP response.

Thanks for your feedback!

-- 
Regards,
  Collin

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

On 4/24/24 04:20, Daniel P. Berrangé wrote:
> On Tue, Apr 23, 2024 at 05:06:53PM -0400, Collin Walling wrote:
>> This optional parameter for query-cpu-model-expansion enables CPU
>> model features flagged as deprecated to appear in the resulting
>> list of properties.
>>
>> This commit does not add support beyond adding a new argument
>> to the query. All queries with this option present will result
>> in an error claiming this option is not supported.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  qapi/machine-target.json         | 7 ++++++-
>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index 29e695aa06..b9da284d2d 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -285,6 +285,10 @@
>>  #
>>  # @type: expansion type, specifying how to expand the CPU model
>>  #
>> +# @disable-deprecated-feats: include CPU model features that are
>> +#     flagged as deprecated. If supported, these features will appear
>> +#     in the properties list paired with false.
>> +#
>>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>>  #
>>  # Errors:
>> @@ -298,7 +302,8 @@
>>  ##
>>  { 'command': 'query-cpu-model-expansion',
>>    'data': { 'type': 'CpuModelExpansionType',
>> -            'model': 'CpuModelInfo' },
>> +            'model': 'CpuModelInfo',
>> +            '*disable-deprecated-feats': 'bool' },
>>    'returns': 'CpuModelExpansionInfo',
>>    'if': { 'any': [ 'TARGET_S390X',
>>                     'TARGET_I386',
> 
> I think this is an odd design approach. Lets consider the
> current output:
> 
> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
> {
>     "return": {
>         "model": {
>             "name": "z14-base",
>             "props": {
>                 "aefsi": true,
>                 "aen": true,
>                 ...snip...
>                 "vxpd": true,
>                 "zpci": true
>             }
>         }
>     }
> }
> 
> 
> If we want to inform a mgmt app of some features being deprecated,
> why not just unconditionally include that info in the reply thus:
> 
> 
> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
> {
>     "return": {
>         "model": {
>             "name": "z14-base",
>             "props": {
>                 "aefsi": true,
>                 "aen": true,
>                 ...snip...
>                 "vxpd": true,
>                 "zpci": true
>             }
>             "deprecated-props": ["ppa15", "ri"]
>         }
>     }
> }
> 
> 
> 
> With regards,
> Daniel

That's a good idea. In this way, we're not mucking up any of the CPU
model information and this makes it much more clear as to which features
are actually deprecated... I like this more.

I'll work on this.

-- 
Regards,
  Collin

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

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

On 4/24/24 13:51, Collin Walling wrote:
> On 4/24/24 04:20, Daniel P. Berrangé wrote:
>> On Tue, Apr 23, 2024 at 05:06:53PM -0400, Collin Walling wrote:
>>> This optional parameter for query-cpu-model-expansion enables CPU
>>> model features flagged as deprecated to appear in the resulting
>>> list of properties.
>>>
>>> This commit does not add support beyond adding a new argument
>>> to the query. All queries with this option present will result
>>> in an error claiming this option is not supported.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>  qapi/machine-target.json         | 7 ++++++-
>>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>> index 29e695aa06..b9da284d2d 100644
>>> --- a/qapi/machine-target.json
>>> +++ b/qapi/machine-target.json
>>> @@ -285,6 +285,10 @@
>>>  #
>>>  # @type: expansion type, specifying how to expand the CPU model
>>>  #
>>> +# @disable-deprecated-feats: include CPU model features that are
>>> +#     flagged as deprecated. If supported, these features will appear
>>> +#     in the properties list paired with false.
>>> +#
>>>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>>>  #
>>>  # Errors:
>>> @@ -298,7 +302,8 @@
>>>  ##
>>>  { 'command': 'query-cpu-model-expansion',
>>>    'data': { 'type': 'CpuModelExpansionType',
>>> -            'model': 'CpuModelInfo' },
>>> +            'model': 'CpuModelInfo',
>>> +            '*disable-deprecated-feats': 'bool' },
>>>    'returns': 'CpuModelExpansionInfo',
>>>    'if': { 'any': [ 'TARGET_S390X',
>>>                     'TARGET_I386',
>>
>> I think this is an odd design approach. Lets consider the
>> current output:
>>
>> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
>> {
>>     "return": {
>>         "model": {
>>             "name": "z14-base",
>>             "props": {
>>                 "aefsi": true,
>>                 "aen": true,
>>                 ...snip...
>>                 "vxpd": true,
>>                 "zpci": true
>>             }
>>         }
>>     }
>> }
>>
>>
>> If we want to inform a mgmt app of some features being deprecated,
>> why not just unconditionally include that info in the reply thus:
>>
>>
>> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
>> {
>>     "return": {
>>         "model": {
>>             "name": "z14-base",
>>             "props": {
>>                 "aefsi": true,
>>                 "aen": true,
>>                 ...snip...
>>                 "vxpd": true,
>>                 "zpci": true
>>             }
>>             "deprecated-props": ["ppa15", "ri"]
>>         }
>>     }
>> }
>>
>>
>>
>> With regards,
>> Daniel
> 
> That's a good idea. In this way, we're not mucking up any of the CPU
> model information and this makes it much more clear as to which features
> are actually deprecated... I like this more.
> 
> I'll work on this.
> 

Follow-up question as I look more closely to the QMP response data
structures: should the "deprecated-props" list be added to the
CpuModelInfo struct, or to the CpuModelExpansionInfo struct?

The former makes more sense to me, as the deprecated features are tied
to the actual CPU model... but unsure if other QMP commands would even
care about this info? I will begin with this approach, and if feedback
in the interim strongly sways in the other direction, then it should be
an easy change :)

-- 
Regards,
  Collin

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Markus Armbruster]

Collin Walling <walling@linux.ibm.com> writes:

> On 4/24/24 02:19, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>> 
>>> This optional parameter for query-cpu-model-expansion enables CPU
>>> model features flagged as deprecated to appear in the resulting
>>> list of properties.
>>>
>>> This commit does not add support beyond adding a new argument
>>> to the query. All queries with this option present will result
>>> in an error claiming this option is not supported.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>  qapi/machine-target.json         | 7 ++++++-
>>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>> index 29e695aa06..b9da284d2d 100644
>>> --- a/qapi/machine-target.json
>>> +++ b/qapi/machine-target.json
>>> @@ -285,6 +285,10 @@
>>>  #
>>>  # @type: expansion type, specifying how to expand the CPU model
>>>  #
>>> +# @disable-deprecated-feats: include CPU model features that are
>>> +#     flagged as deprecated. If supported, these features will appear
>>> +#     in the properties list paired with false.
>> 
>> What's the default?
>> 
>> Which command result(s) does this affect?  Suggest to explain using
>> unabridged example QMP input and output before and after this series.
>
> Fair enough. Bool defaults to false but that's not apparent in the
> description. I will add more detail.

I didn't mean to ask for example QMP in the doc comment.  I need you to
explain the new member to me.  Once I understand what the thing does, I
may have suggestions on improving the doc comment.

[...]

> Thanks for your feedback!

You're welcome!

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

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Daniel P. Berrangé]

On Wed, Apr 24, 2024 at 03:12:42PM -0400, Collin Walling wrote:
> On 4/24/24 13:51, Collin Walling wrote:
> > On 4/24/24 04:20, Daniel P. Berrangé wrote:
> >> On Tue, Apr 23, 2024 at 05:06:53PM -0400, Collin Walling wrote:
> >>> This optional parameter for query-cpu-model-expansion enables CPU
> >>> model features flagged as deprecated to appear in the resulting
> >>> list of properties.
> >>>
> >>> This commit does not add support beyond adding a new argument
> >>> to the query. All queries with this option present will result
> >>> in an error claiming this option is not supported.
> >>>
> >>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> >>> ---
> >>>  qapi/machine-target.json         | 7 ++++++-
> >>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
> >>>  target/i386/cpu-sysemu.c         | 7 +++++++
> >>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
> >>>  4 files changed, 27 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> >>> index 29e695aa06..b9da284d2d 100644
> >>> --- a/qapi/machine-target.json
> >>> +++ b/qapi/machine-target.json
> >>> @@ -285,6 +285,10 @@
> >>>  #
> >>>  # @type: expansion type, specifying how to expand the CPU model
> >>>  #
> >>> +# @disable-deprecated-feats: include CPU model features that are
> >>> +#     flagged as deprecated. If supported, these features will appear
> >>> +#     in the properties list paired with false.
> >>> +#
> >>>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
> >>>  #
> >>>  # Errors:
> >>> @@ -298,7 +302,8 @@
> >>>  ##
> >>>  { 'command': 'query-cpu-model-expansion',
> >>>    'data': { 'type': 'CpuModelExpansionType',
> >>> -            'model': 'CpuModelInfo' },
> >>> +            'model': 'CpuModelInfo',
> >>> +            '*disable-deprecated-feats': 'bool' },
> >>>    'returns': 'CpuModelExpansionInfo',
> >>>    'if': { 'any': [ 'TARGET_S390X',
> >>>                     'TARGET_I386',
> >>
> >> I think this is an odd design approach. Lets consider the
> >> current output:
> >>
> >> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
> >> {
> >>     "return": {
> >>         "model": {
> >>             "name": "z14-base",
> >>             "props": {
> >>                 "aefsi": true,
> >>                 "aen": true,
> >>                 ...snip...
> >>                 "vxpd": true,
> >>                 "zpci": true
> >>             }
> >>         }
> >>     }
> >> }
> >>
> >>
> >> If we want to inform a mgmt app of some features being deprecated,
> >> why not just unconditionally include that info in the reply thus:
> >>
> >>
> >> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
> >> {
> >>     "return": {
> >>         "model": {
> >>             "name": "z14-base",
> >>             "props": {
> >>                 "aefsi": true,
> >>                 "aen": true,
> >>                 ...snip...
> >>                 "vxpd": true,
> >>                 "zpci": true
> >>             }
> >>             "deprecated-props": ["ppa15", "ri"]
> >>         }
> >>     }
> >> }
> >>
> >>
> >>
> >> With regards,
> >> Daniel
> > 
> > That's a good idea. In this way, we're not mucking up any of the CPU
> > model information and this makes it much more clear as to which features
> > are actually deprecated... I like this more.
> > 
> > I'll work on this.
> > 
> 
> Follow-up question as I look more closely to the QMP response data
> structures: should the "deprecated-props" list be added to the
> CpuModelInfo struct, or to the CpuModelExpansionInfo struct?
> 
> The former makes more sense to me, as the deprecated features are tied
> to the actual CPU model... but unsure if other QMP commands would even
> care about this info? I will begin with this approach, and if feedback
> in the interim strongly sways in the other direction, then it should be
> an easy change :)

I hink CpuModelInfo makes more sense than CpuModelExpansionInfo.
The CpuModelExpansionInfo struct feels pretty pointless to me
in fact, since the only thing it contains is CpuModelInfo !

I think it should also be added to 'CpuDefinitionInfo', which
is the return type of 'query-cpu-defintions'.  This command already
has a 'unavailable-features' array listing features which the host
does not support. Conceptually having a 'deprecated-features' array
alongside that is a nice fit.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

On 4/25/24 09:35, Daniel P. Berrangé wrote:
> On Wed, Apr 24, 2024 at 03:12:42PM -0400, Collin Walling wrote:
>> On 4/24/24 13:51, Collin Walling wrote:
>>> On 4/24/24 04:20, Daniel P. Berrangé wrote:
>>>> On Tue, Apr 23, 2024 at 05:06:53PM -0400, Collin Walling wrote:
>>>>> This optional parameter for query-cpu-model-expansion enables CPU
>>>>> model features flagged as deprecated to appear in the resulting
>>>>> list of properties.
>>>>>
>>>>> This commit does not add support beyond adding a new argument
>>>>> to the query. All queries with this option present will result
>>>>> in an error claiming this option is not supported.
>>>>>
>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>>> ---
>>>>>  qapi/machine-target.json         | 7 ++++++-
>>>>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>>>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>>>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>>> index 29e695aa06..b9da284d2d 100644
>>>>> --- a/qapi/machine-target.json
>>>>> +++ b/qapi/machine-target.json
>>>>> @@ -285,6 +285,10 @@
>>>>>  #
>>>>>  # @type: expansion type, specifying how to expand the CPU model
>>>>>  #
>>>>> +# @disable-deprecated-feats: include CPU model features that are
>>>>> +#     flagged as deprecated. If supported, these features will appear
>>>>> +#     in the properties list paired with false.
>>>>> +#
>>>>>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>>>>>  #
>>>>>  # Errors:
>>>>> @@ -298,7 +302,8 @@
>>>>>  ##
>>>>>  { 'command': 'query-cpu-model-expansion',
>>>>>    'data': { 'type': 'CpuModelExpansionType',
>>>>> -            'model': 'CpuModelInfo' },
>>>>> +            'model': 'CpuModelInfo',
>>>>> +            '*disable-deprecated-feats': 'bool' },
>>>>>    'returns': 'CpuModelExpansionInfo',
>>>>>    'if': { 'any': [ 'TARGET_S390X',
>>>>>                     'TARGET_I386',
>>>>
>>>> I think this is an odd design approach. Lets consider the
>>>> current output:
>>>>
>>>> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
>>>> {
>>>>     "return": {
>>>>         "model": {
>>>>             "name": "z14-base",
>>>>             "props": {
>>>>                 "aefsi": true,
>>>>                 "aen": true,
>>>>                 ...snip...
>>>>                 "vxpd": true,
>>>>                 "zpci": true
>>>>             }
>>>>         }
>>>>     }
>>>> }
>>>>
>>>>
>>>> If we want to inform a mgmt app of some features being deprecated,
>>>> why not just unconditionally include that info in the reply thus:
>>>>
>>>>
>>>> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
>>>> {
>>>>     "return": {
>>>>         "model": {
>>>>             "name": "z14-base",
>>>>             "props": {
>>>>                 "aefsi": true,
>>>>                 "aen": true,
>>>>                 ...snip...
>>>>                 "vxpd": true,
>>>>                 "zpci": true
>>>>             }
>>>>             "deprecated-props": ["ppa15", "ri"]
>>>>         }
>>>>     }
>>>> }
>>>>
>>>>
>>>>
>>>> With regards,
>>>> Daniel
>>>
>>> That's a good idea. In this way, we're not mucking up any of the CPU
>>> model information and this makes it much more clear as to which features
>>> are actually deprecated... I like this more.
>>>
>>> I'll work on this.
>>>
>>
>> Follow-up question as I look more closely to the QMP response data
>> structures: should the "deprecated-props" list be added to the
>> CpuModelInfo struct, or to the CpuModelExpansionInfo struct?
>>
>> The former makes more sense to me, as the deprecated features are tied
>> to the actual CPU model... but unsure if other QMP commands would even
>> care about this info? I will begin with this approach, and if feedback
>> in the interim strongly sways in the other direction, then it should be
>> an easy change :)
> 
> I hink CpuModelInfo makes more sense than CpuModelExpansionInfo.
> The CpuModelExpansionInfo struct feels pretty pointless to me
> in fact, since the only thing it contains is CpuModelInfo !
> 

Agreed! :)

> I think it should also be added to 'CpuDefinitionInfo', which
> is the return type of 'query-cpu-defintions'.  This command already
> has a 'unavailable-features' array listing features which the host
> does not support. Conceptually having a 'deprecated-features' array
> alongside that is a nice fit.
> 

Okay. Pending review on the v3 I posted yesterday -- if those patches
look like they're on the right track, then I can add this
"deprecated-props" array to CpuDefinitionInfo as well for the next
iteration.

> 
> 
> With regards,
> Daniel

-- 
Regards,
  Collin

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

On 4/25/24 02:31, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> On 4/24/24 02:19, Markus Armbruster wrote:
>>> Collin Walling <walling@linux.ibm.com> writes:
>>>
>>>> This optional parameter for query-cpu-model-expansion enables CPU
>>>> model features flagged as deprecated to appear in the resulting
>>>> list of properties.
>>>>
>>>> This commit does not add support beyond adding a new argument
>>>> to the query. All queries with this option present will result
>>>> in an error claiming this option is not supported.
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>  qapi/machine-target.json         | 7 ++++++-
>>>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>> index 29e695aa06..b9da284d2d 100644
>>>> --- a/qapi/machine-target.json
>>>> +++ b/qapi/machine-target.json
>>>> @@ -285,6 +285,10 @@
>>>>  #
>>>>  # @type: expansion type, specifying how to expand the CPU model
>>>>  #
>>>> +# @disable-deprecated-feats: include CPU model features that are
>>>> +#     flagged as deprecated. If supported, these features will appear
>>>> +#     in the properties list paired with false.
>>>
>>> What's the default?
>>>
>>> Which command result(s) does this affect?  Suggest to explain using
>>> unabridged example QMP input and output before and after this series.
>>
>> Fair enough. Bool defaults to false but that's not apparent in the
>> description. I will add more detail.
> 
> I didn't mean to ask for example QMP in the doc comment.  I need you to
> explain the new member to me.  Once I understand what the thing does, I
> may have suggestions on improving the doc comment.
> 
> [...]
> 

Ah, I misunderstood.  The idea behind this "disable-deprecated-feats"
option is to add/override any CPU features that are flagged as
deprecated to the props dictionary (found in the CpuModelInfo struct)
paired with the value false.

For example, the csske feature on s390x is flagged as deprecated.  If a
query-cpu-model-expansion command is created with
"disable-deprecated-feats": true, then the csske feature will show up in
the props list as "csske": false.  This also overrides any user defined
features and props that would show up in the response normally. E.g. if
the same command was executed and "csske": true was provided in the
model's properties by the user, the response will still show "csske":
false since the "disable-deprecated-feats" option takes priority (there
is a discussion with David H regarding which should take precedence --
this is a flaw in this design).

In the below QMP samples I provide a static expansion on a host model.
Pay close attention to the bpb, te, cte, and csske properties.

========
 Before
========

This is how query-cpu-model-expansion may be executed today:

{
  "execute": "query-cpu-model-expansion",
  "arguments": {
    "type": "static",
    "model": {
      "name": "host"
    }
  }
}

{
  "return": {
    "model": {
      "name": "z14.2-base",
      "props": {
        "aen": true,
        "cmmnt": true,
        "aefsi": true,
        "diag318": true,
        "mepoch": true,
        "msa8": true,
        "msa7": true,
        "msa6": true,
        "msa5": true,
        "msa4": true,
        "msa3": true,
        "msa2": true,
        "msa1": true,
        "sthyi": true,
        "edat": true,
        "ri": true,
        "edat2": true,
        "etoken": true,
        "vx": true,
        "ipter": true,
        "mepochptff": true,
        "ap": true,
        "vxeh": true,
        "vxpd": true,
        "esop": true,
        "apqi": true,
        "apft": true,
        "els": true,
        "iep": true,
        "apqci": true,
        "cte": true,		<--
        "ais": true,
        "bpb": true,		<--
        "ctop": true,
        "gs": true,
        "ppa15": true,
        "zpci": true,
        "sea_esop2": true,
        "te": true,		<--
        "cmm": true
      }
    }
  }
}

Note: csske does not even show up here.

=======
 After
=======

This is with the optional "disabled-deprecated-feats":

{
  "execute": "query-cpu-model-expansion",
  "arguments": {
    "type": "static",
    "model": {
      "name": "host"
    },
    "disabled-deprecated-feats": true
  }
}

{
  "return": {
    "model": {
      "name": "z14.2-base",
      "props": {
        "aen": true,
        "cmmnt": true,
        "aefsi": true,
        "diag318": true,
        "mepoch": true,
        "msa8": true,
        "msa7": true,
        "msa6": true,
        "msa5": true,
        "msa4": true,
        "msa3": true,
        "msa2": true,
        "msa1": true,
        "sthyi": true,
        "edat": true,
        "ri": true,
        "edat2": true,
        "etoken": true,
        "vx": true,
        "ipter": true,
        "mepochptff": true,
        "ap": true,
        "vxeh": true,
        "vxpd": true,
        "esop": true,
        "apqi": true,
        "apft": true,
        "els": true,
        "iep": true,
        "apqci": true,
        "cte": false,		<-- overridden
        "ais": true,
        "bpb": false,		<-- overridden
        "ctop": true,
        "gs": true,
        "ppa15": true,
        "zpci": true,
        "sea_esop2": true,
        "te": false,		<-- overridden
	"csske": false		<-- added to the dict
        "cmm": true
      }
    }
  }
}

Now, regarding what I have proposed for v3, this option is gone entirely
and the CpuModelInfo structure has been amended to include a new
(optional) "deprecated-props" array.  As such, a lot of the headache
with requiring compliance or conditionals to satisfy other architectures
is completely gone, as well as the requirement for any additional
options to be provided on the QMP command.

For comparison's sake, here is the v3 QMP:

{
  "execute": "query-cpu-model-expansion",
  "arguments": {
    "type": "static",
    "model": {
      "name": "host"
    }
  }
}

{
  "return": {
    "model": {
      "name": "z14.2-base",
      "deprecated-props": [
        "bpb",
        "te",
        "cte",
        "csske"
      ],
      "props": {
        "aen": true,
        "cmmnt": true,
        "aefsi": true,
        "diag318": true,
        "mepoch": true,
        "msa8": true,
        "msa7": true,
        "msa6": true,
        "msa5": true,
        "msa4": true,
        "msa3": true,
        "msa2": true,
        "msa1": true,
        "sthyi": true,
        "edat": true,
        "ri": true,
        "edat2": true,
        "etoken": true,
        "vx": true,
        "ipter": true,
        "mepochptff": true,
        "ap": true,
        "vxeh": true,
        "vxpd": true,
        "esop": true,
        "apqi": true,
        "apft": true,
        "els": true,
        "iep": true,
        "apqci": true,
        "cte": true,
        "ais": true,
        "bpb": true,
        "ctop": true,
        "gs": true,
        "ppa15": true,
        "zpci": true,
        "sea_esop2": true,
        "te": true,
        "cmm": true
      }
    }
  }
}

Hope this helps to provide more context.  Please let me know if you'd
like more info.  I am leaning towards v3's design more, as it seems a
lot cleaner overall.  I would appreciate your feedback there as well if
you have the time.

Thanks!

[...]

-- 
Regards,
  Collin

Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Markus Armbruster]

Collin Walling <walling@linux.ibm.com> writes:

> On 4/25/24 02:31, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>> 
>>> On 4/24/24 02:19, Markus Armbruster wrote:
>>>> Collin Walling <walling@linux.ibm.com> writes:
>>>>
>>>>> This optional parameter for query-cpu-model-expansion enables CPU
>>>>> model features flagged as deprecated to appear in the resulting
>>>>> list of properties.
>>>>>
>>>>> This commit does not add support beyond adding a new argument
>>>>> to the query. All queries with this option present will result
>>>>> in an error claiming this option is not supported.
>>>>>
>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>>> ---
>>>>>  qapi/machine-target.json         | 7 ++++++-
>>>>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>>>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>>>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>>> index 29e695aa06..b9da284d2d 100644
>>>>> --- a/qapi/machine-target.json
>>>>> +++ b/qapi/machine-target.json
>>>>> @@ -285,6 +285,10 @@
>>>>>  #
>>>>>  # @type: expansion type, specifying how to expand the CPU model
>>>>>  #
>>>>> +# @disable-deprecated-feats: include CPU model features that are
>>>>> +#     flagged as deprecated. If supported, these features will appear
>>>>> +#     in the properties list paired with false.
>>>>
>>>> What's the default?
>>>>
>>>> Which command result(s) does this affect?  Suggest to explain using
>>>> unabridged example QMP input and output before and after this series.
>>>
>>> Fair enough. Bool defaults to false but that's not apparent in the
>>> description. I will add more detail.
>> 
>> I didn't mean to ask for example QMP in the doc comment.  I need you to
>> explain the new member to me.  Once I understand what the thing does, I
>> may have suggestions on improving the doc comment.
>> 
>> [...]
>> 
>
> Ah, I misunderstood.  The idea behind this "disable-deprecated-feats"
> option is to add/override any CPU features that are flagged as
> deprecated to the props dictionary (found in the CpuModelInfo struct)
> paired with the value false.
>
> For example, the csske feature on s390x is flagged as deprecated.  If a
> query-cpu-model-expansion command is created with
> "disable-deprecated-feats": true, then the csske feature will show up in
> the props list as "csske": false.  This also overrides any user defined
> features and props that would show up in the response normally. E.g. if
> the same command was executed and "csske": true was provided in the
> model's properties by the user, the response will still show "csske":
> false since the "disable-deprecated-feats" option takes priority (there
> is a discussion with David H regarding which should take precedence --
> this is a flaw in this design).
>
> In the below QMP samples I provide a static expansion on a host model.
> Pay close attention to the bpb, te, cte, and csske properties.

[...]

> Hope this helps to provide more context.  Please let me know if you'd
> like more info.  I am leaning towards v3's design more, as it seems a
> lot cleaner overall.  I would appreciate your feedback there as well if
> you have the time.
>
> Thanks!

Okay, I'll look at v3 then.

Thank you!