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

2023-08-01 Thread Markus Armbruster
Dinah Baum  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 
> ---
>  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 

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

2023-07-30 Thread Dinah Baum
Only architectures that implement the command will return
results, others will return an error message as before.

Signed-off-by: Dinah Baum 
---
 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
+}
+
+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(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);
+
 #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' ] } }
-
 ##
 #