Re: [PATCH v2 2/3] cpu, qapi, target/arm, i386, s390x: Generalize query-cpu-model-expansion

[Markus Armbruster <armbru@redhat.com>]

Dinah Baum <dinahbaum123@gmail.com> writes:

> This patch enables 'query-cpu-model-expansion' on all
> architectures. Only architectures that implement
> the command will return results, others will return an
> error message as before.
>
> This patch lays the groundwork for parsing a
> -cpu cpu,help option as specified in
> https://gitlab.com/qemu-project/qemu/-/issues/1480

Yes, but why does the change make sense for QMP?

Any idea how hard implementing the thing for more targets would be?
Question, not a demand!

> Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>
> ---
>  cpu.c                            | 20 ++++++++++++
>  include/exec/cpu-common.h        |  8 +++++
>  qapi/machine-target-common.json  | 51 +++++++++++++++++++++++++++++
>  qapi/machine-target.json         | 56 --------------------------------
>  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, 108 insertions(+), 67 deletions(-)
>
> diff --git a/cpu.c b/cpu.c
> index 849bac062c..daf4e1ff0d 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -292,6 +292,26 @@ void list_cpus(const char *optarg)
>  #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");

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.

> +    return NULL;
> +#endif
> +}
> +
> +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> +                                                     CpuModelInfo *model,
> +                                                     Error **errp)
> +{
> +    return get_cpu_model_expansion_info(type, model, errp);
> +}
> +
>  #if defined(CONFIG_USER_ONLY)
>  void tb_invalidate_phys_addr(target_ulong addr)
>  {
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 6feaa40ca7..ec6024dfde 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -7,6 +7,8 @@
>  #include "exec/hwaddr.h"
>  #endif
>  
> +#include "qapi/qapi-commands-machine-target-common.h"
> +
>  /**
>   * vaddr:
>   * Type wide enough to contain any #target_ulong virtual address.
> @@ -166,5 +168,11 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>  extern int singlestep;
>  
>  void list_cpus(const char *optarg);
> +typedef void (*cpu_model_expansion_func)(CpuModelExpansionType type,
> +                                         CpuModelInfo *model,
> +                                         Error **errp);
> +CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType type,
> +                                                    CpuModelInfo *model,
> +                                                    Error **errp);
>  
>  #endif /* CPU_COMMON_H */
> diff --git a/qapi/machine-target-common.json b/qapi/machine-target-common.json
> index 1e6da3177d..44713e9935 100644
> --- a/qapi/machine-target-common.json
> +++ b/qapi/machine-target-common.json
> @@ -77,3 +77,54 @@
>  ##
>  { '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 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' },

Please use the opportunity to fix the indentation.

> +  'returns': 'CpuModelExpansionInfo' }
> +

[Remainder skipped for now...]