Re: [PATCH v3 2/3] qapi, target/: Enable 'query-cpu-model-expansion' on all architectures

[Markus Armbruster]

Dinah Baum <dinahbaum123@gmail.com> writes:

> Only architectures that implement the command will return
> results, others will return an error message as before.

A brief explanation why this is useful would be nice.

>
> Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>
> ---
>  cpu.c                            | 20 +++++++++++
>  include/exec/cpu-common.h        |  7 ++++
>  qapi/machine-target.json         | 60 --------------------------------
>  qapi/machine.json                | 53 ++++++++++++++++++++++++++++
>  target/arm/arm-qmp-cmds.c        |  7 ++--
>  target/arm/cpu.h                 |  7 ++++
>  target/i386/cpu-sysemu.c         |  7 ++--
>  target/i386/cpu.h                |  6 ++++
>  target/s390x/cpu.h               |  7 ++++
>  target/s390x/cpu_models_sysemu.c |  6 ++--
>  10 files changed, 110 insertions(+), 70 deletions(-)
>
> diff --git a/cpu.c b/cpu.c
> index 1c948d1161..a99d09cd47 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -292,6 +292,26 @@ void list_cpus(void)
>  #endif
>  }
>  
> +CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType 
> type,
> +                                                    CpuModelInfo *model,
> +                                                    Error **errp)
> +{
> +    /* XXX: implement cpu_model_expansion for targets that still miss it */
> +#if defined(cpu_model_expansion)
> +    return cpu_model_expansion(type, model, errp);
> +#else
> +    error_setg(errp, "Could not query cpu model information");
> +    return NULL;
> +#endif
> +}

This is vague enough to leave the user wondering what could be done to
avoid this error and by whom.

Before the patch, it's clear enough: "The command
query-cpu-model-expansion has not been found".

You could go with something like "command not supported for this
target".

The error class changes from CommandNotFound to GenericError.  Please
verify libvirt is fine with that.

> +
> +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> +                                                     CpuModelInfo *model,
> +                                                     Error **errp)
> +{
> +    return get_cpu_model_expansion_info(type, model, errp);
> +}
> +

Why do you need qmp_query_cpu_model_expansion() to become a wrapper
around the real function?

>  #if defined(CONFIG_USER_ONLY)
>  void tb_invalidate_phys_addr(hwaddr addr)
>  {
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 87dc9a752c..653f8a9d2b 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -6,6 +6,7 @@
>  #ifndef CONFIG_USER_ONLY
>  #include "exec/hwaddr.h"
>  #endif
> +#include "qapi/qapi-commands-machine.h"
>  
>  /**
>   * vaddr:
> @@ -167,4 +168,10 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>  /* vl.c */
>  void list_cpus(void);
>  
> +CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType 
> type,
> +                                                    CpuModelInfo *model,
> +                                                    Error **errp);
> +void list_cpu_model_expansion(CpuModelExpansionType type,
> +                              CpuModelInfo *model, Error **errp);
> +

The declaration of list_cpu_model_expansion() belongs to the next patch.

>  #endif /* CPU_COMMON_H */
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 3ee2f7ca6b..a658b1754b 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -139,66 +139,6 @@
>    'returns': 'CpuModelBaselineInfo',
>    'if': 'TARGET_S390X' }
>  
> -##
> -# @CpuModelExpansionInfo:
> -#
> -# The result of a cpu model expansion.
> -#
> -# @model: the expanded CpuModelInfo.
> -#
> -# Since: 2.8
> -##
> -{ 'struct': 'CpuModelExpansionInfo',
> -  'data': { 'model': 'CpuModelInfo' },
> -  'if': { 'any': [ 'TARGET_S390X',
> -                   'TARGET_I386',
> -                   'TARGET_ARM' ] } }
> -
> -##
> -# @query-cpu-model-expansion:
> -#
> -# Expands a given CPU model (or a combination of CPU model +
> -# additional options) to different granularities, allowing tooling to
> -# get an understanding what a specific CPU model looks like in QEMU
> -# under a certain configuration.
> -#
> -# This interface can be used to query the "host" CPU model.
> -#
> -# The data returned by this command may be affected by:
> -#
> -# * QEMU version: CPU models may look different depending on the QEMU
> -#   version.  (Except for CPU models reported as "static" in
> -#   query-cpu-definitions.)
> -# * machine-type: CPU model may look different depending on the
> -#   machine-type.  (Except for CPU models reported as "static" in
> -#   query-cpu-definitions.)
> -# * machine options (including accelerator): in some architectures,
> -#   CPU models may look different depending on machine and accelerator
> -#   options.  (Except for CPU models reported as "static" in
> -#   query-cpu-definitions.)
> -# * "-cpu" arguments and global properties: arguments to the -cpu
> -#   option and global properties may affect expansion of CPU models.
> -#   Using query-cpu-model-expansion while using these is not advised.
> -#
> -# Some architectures may not support all expansion types.  s390x
> -# supports "full" and "static". Arm only supports "full".
> -#
> -# Returns: a CpuModelExpansionInfo.  Returns an error if expanding CPU
> -#     models is not supported, if the model cannot be expanded, if the
> -#     model contains an unknown CPU definition name, unknown
> -#     properties or properties with a wrong type.  Also returns an
> -#     error if an expansion type is not supported.
> -#
> -# Since: 2.8
> -##
> -{ 'command': 'query-cpu-model-expansion',
> -  'data': { 'type': 'CpuModelExpansionType',
> -            'model': 'CpuModelInfo' },
> -  'returns': 'CpuModelExpansionInfo',
> -  'if': { 'any': [ 'TARGET_S390X',
> -                   'TARGET_I386',
> -                   'TARGET_ARM' ] } }
> -
>  ##
>  # @CpuDefinitionInfo:
>  #
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 192c781310..f24e19815e 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1768,3 +1768,56 @@
>  ##
>  { 'enum': 'CpuModelCompareResult',
>    'data': [ 'incompatible', 'identical', 'superset', 'subset' ] }
> +
> +##
> +# @CpuModelExpansionInfo:
> +#
> +# The result of a cpu model expansion.
> +#
> +# @model: the expanded CpuModelInfo.
> +#
> +# Since: 2.8
> +##
> +{ 'struct': 'CpuModelExpansionInfo',
> +  'data': { 'model': 'CpuModelInfo' }}
> +
> +##
> +# @query-cpu-model-expansion:
> +#
> +# Expands a given CPU model (or a combination of CPU model +
> +# additional options) to different granularities, allowing tooling to
> +# get an understanding what a specific CPU model looks like in QEMU
> +# under a certain configuration.
> +#
> +# This interface can be used to query the "host" CPU model.
> +#
> +# The data returned by this command may be affected by:
> +#
> +# * QEMU version: CPU models may look different depending on the QEMU
> +#   version.  (Except for CPU models reported as "static" in
> +#   query-cpu-definitions.)
> +# * machine-type: CPU model may look different depending on the
> +#   machine-type.  (Except for CPU models reported as "static" in
> +#   query-cpu-definitions.)
> +# * machine options (including accelerator): in some architectures,
> +#   CPU models may look different depending on machine and accelerator
> +#   options.  (Except for CPU models reported as "static" in
> +#   query-cpu-definitions.)
> +# * "-cpu" arguments and global properties: arguments to the -cpu
> +#   option and global properties may affect expansion of CPU models.
> +#   Using query-cpu-model-expansion while using these is not advised.
> +#
> +# Some architectures may not support all expansion types.  s390x
> +# supports "full" and "static". Arm only supports "full".
> +#
> +# Returns: a CpuModelExpansionInfo. Returns an error if expanding CPU

Accidental space change, line should remain

   # Returns: a CpuModelExpansionInfo.  Returns an error if expanding CPU

> +#     models is not supported, if the model cannot be expanded, if the
> +#     model contains an unknown CPU definition name, unknown
> +#     properties or properties with a wrong type.  Also returns an
> +#     error if an expansion type is not supported.
> +#
> +# Since: 2.8
> +##
> +{ 'command': 'query-cpu-model-expansion',
> +  'data': { 'type': 'CpuModelExpansionType', 'model': 'CpuModelInfo' },
> +  'returns': 'CpuModelExpansionInfo'}
> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index c8fa524002..295d690e93 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -22,6 +22,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/boards.h"
> +#include "cpu.h"
>  #include "kvm_arm.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> @@ -99,9 +100,9 @@ static const char *cpu_model_advertised_features[] = {
>      NULL
>  };
>  
> -CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> -                                                     CpuModelInfo *model,
> -                                                     Error **errp)
> +CpuModelExpansionInfo *get_cpu_model_expansion(CpuModelExpansionType type,
> +                                               CpuModelInfo *model,
> +                                               Error **errp)
>  {
>      CpuModelExpansionInfo *expansion_info;
>      const QDict *qdict_in = NULL;
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 88e5accda6..71b15d4884 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2740,6 +2740,13 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool 
> kvm_sync);
>  
>  #define cpu_list arm_cpu_list
>  
> +#ifdef CONFIG_SOFTMMU
> +CpuModelExpansionInfo *get_cpu_model_expansion(CpuModelExpansionType type,
> +                                               CpuModelInfo *model,
> +                                               Error **errp);
> +#define cpu_model_expansion get_cpu_model_expansion

What is this macro for?

> +#endif
> +
>  /* ARM has the following "translation regimes" (as the ARM ARM calls them):
>   *
>   * If EL3 is 64-bit:
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 28115edf44..c01c85839c 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -176,10 +176,9 @@ out:
>      return xc;
>  }
>  
> -CpuModelExpansionInfo *
> -qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> -                                                      CpuModelInfo *model,
> -                                                      Error **errp)
> +CpuModelExpansionInfo *get_cpu_model_expansion(CpuModelExpansionType type,
> +                                               CpuModelInfo *model,
> +                                               Error **errp)
>  {
>      X86CPU *xc = NULL;
>      Error *err = NULL;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index e0771a1043..699b987d12 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2249,6 +2249,12 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  #endif
>  
>  #define cpu_list x86_cpu_list
> +#ifdef CONFIG_SOFTMMU
> +CpuModelExpansionInfo *get_cpu_model_expansion(CpuModelExpansionType type,
> +                                               CpuModelInfo *model,
> +                                               Error **errp);
> +#define cpu_model_expansion get_cpu_model_expansion

Likewise.

> +#endif
>  
>  /* MMU modes definitions */
>  #define MMU_KSMAP_IDX   0
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index eb5b65b7d3..932f8b642a 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -804,6 +804,13 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu)
>  /* cpu_models.c */
>  void s390_cpu_list(void);
>  #define cpu_list s390_cpu_list
> +#ifdef CONFIG_SOFTMMU
> +CpuModelExpansionInfo *get_cpu_model_expansion(CpuModelExpansionType type,
> +                                               CpuModelInfo *model,
> +                                               Error **errp);
> +#define cpu_model_expansion get_cpu_model_expansion

Likewise.

> +#endif
> +
>  void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
>                               const S390FeatInit feat_init);
>  
> diff --git a/target/s390x/cpu_models_sysemu.c 
> b/target/s390x/cpu_models_sysemu.c
> index 63981bf36b..ef3845f11c 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -213,9 +213,9 @@ static void cpu_info_from_model(CpuModelInfo *info, const 
> S390CPUModel *model,
>      }
>  }
>  
> -CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> -                                                      CpuModelInfo *model,
> -                                                      Error **errp)
> +CpuModelExpansionInfo *get_cpu_model_expansion(CpuModelExpansionType type,
> +                                               CpuModelInfo *model,
> +                                               Error **errp)
>  {
>      Error *err = NULL;
>      CpuModelExpansionInfo *expansion_info = NULL;