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.