Re: [Qemu-devel] [PATCH v3 43/49] qapi: make s390 commands depend on TARGET_S390X

[Marc-André Lureau]

Hi

On Thu, Mar 22, 2018 at 6:42 AM, Thomas Huth <address@hidden> wrote:
> On 21.03.2018 12:52, Marc-André Lureau wrote:
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> Acked-by: Cornelia Huck <address@hidden>
>> ---
>>  qapi/misc.json                          | 101 ----------------------
>>  qapi/target.json                        | 106 ++++++++++++++++++++++++
>>  include/sysemu/arch_init.h              |   7 --
>>  hw/s390x/s390-skeys.c                   |   2 +-
>>  monitor.c                               |  14 ----
>>  qmp.c                                   |  14 ----
>>  stubs/arch-query-cpu-model-baseline.c   |  13 ---
>>  stubs/arch-query-cpu-model-comparison.c |  13 ---
>>  target/s390x/cpu_models.c               |   5 +-
>>  stubs/Makefile.objs                     |   2 -
>>  10 files changed, 110 insertions(+), 167 deletions(-)
>>  delete mode 100644 stubs/arch-query-cpu-model-baseline.c
>>  delete mode 100644 stubs/arch-query-cpu-model-comparison.c
>>
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 690eeda41f..1753a81b1e 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -1821,27 +1821,6 @@
>>  { 'command': 'query-dump-guest-memory-capability',
>>    'returns': 'DumpGuestMemoryCapability' }
>>
>> -##
>> -# @dump-skeys:
>> -#
>> -# Dump guest's storage keys
>> -#
>> -# @filename: the path to the file to dump to
>> -#
>> -# This command is only supported on s390 architecture.
>> -#
>> -# Since: 2.5
>> -#
>> -# Example:
>> -#
>> -# -> { "execute": "dump-skeys",
>> -#      "arguments": { "filename": "/tmp/skeys" } }
>> -# <- { "return": {} }
>> -#
>> -##
>> -{ 'command': 'dump-skeys',
>> -  'data': { 'filename': 'str' } }
>> -
>>  ##
>>  # @object-add:
>>  #
>> @@ -2208,46 +2187,6 @@
>>            }
>>  }
>>
>> -##
>> -# @query-cpu-model-comparison:
>> -#
>> -# Compares two CPU models, returning how they compare in a specific
>> -# configuration. The results indicates how both models compare regarding
>> -# runnability. This result can be used by tooling to make decisions if a
>> -# certain CPU model will run in a certain configuration or if a compatible
>> -# CPU model has to be created by baselining.
>> -#
>> -# Usually, a CPU model is compared against the maximum possible CPU model
>> -# of a certain configuration (e.g. the "host" model for KVM). If that CPU
>> -# model is identical or a subset, it will run in that configuration.
>> -#
>> -# The result 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 comparing CPU models. s390x supports
>> -# comparing CPU models.
>> -#
>> -# Returns: a CpuModelBaselineInfo. Returns an error if comparing CPU models 
>> is
>> -#          not supported, if a model cannot be used, if a model contains
>> -#          an unknown cpu definition name, unknown properties or properties
>> -#          with wrong types.
>> -#
>> -# Since: 2.8.0
>> -##
>> -{ 'command': 'query-cpu-model-comparison',
>> -  'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
>> -  'returns': 'CpuModelCompareInfo' }
>> -
>>  ##
>>  # @CpuModelBaselineInfo:
>>  #
>> @@ -2260,46 +2199,6 @@
>>  { 'struct': 'CpuModelBaselineInfo',
>>    'data': { 'model': 'CpuModelInfo' } }
>>
>> -##
>> -# @query-cpu-model-baseline:
>> -#
>> -# Baseline two CPU models, creating a compatible third model. The created
>> -# model will always be a static, migration-safe CPU model (see "static"
>> -# CPU model expansion for details).
>> -#
>> -# This interface can be used by tooling to create a compatible CPU model out
>> -# two CPU models. The created CPU model will be identical to or a subset of
>> -# both CPU models when comparing them. Therefore, the created CPU model is
>> -# guaranteed to run where the given CPU models run.
>> -#
>> -# The result 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 baselining CPU models. s390x supports
>> -# baselining CPU models.
>> -#
>> -# Returns: a CpuModelBaselineInfo. Returns an error if baselining CPU 
>> models is
>> -#          not supported, if a model cannot be used, if a model contains
>> -#          an unknown cpu definition name, unknown properties or properties
>> -#          with wrong types.
>> -#
>> -# Since: 2.8.0
>> -##
>> -{ 'command': 'query-cpu-model-baseline',
>> -  'data': { 'modela': 'CpuModelInfo',
>> -            'modelb': 'CpuModelInfo' },
>> -  'returns': 'CpuModelBaselineInfo' }
>> -
>>  ##
>>  # @AddfdInfo:
>>  #
>> diff --git a/qapi/target.json b/qapi/target.json
>> index f277b69a2a..b07a8926d8 100644
>> --- a/qapi/target.json
>> +++ b/qapi/target.json
>> @@ -7,6 +7,8 @@
>>
>>  { 'pragma': { 'top-unit': 'target' } }
>>
>> +{ 'include': 'misc.json' }
>> +
>>  ##
>>  # @rtc-reset-reinjection:
>>  #
>> @@ -182,3 +184,107 @@
>>  ##
>>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>>    'if': 'defined(TARGET_I386)' }
>> +
>> +##
>> +# @dump-skeys:
>> +#
>> +# Dump guest's storage keys
>> +#
>> +# @filename: the path to the file to dump to
>> +#
>> +# This command is only supported on s390 architecture.
>> +#
>> +# Since: 2.5
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "dump-skeys",
>> +#      "arguments": { "filename": "/tmp/skeys" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'dump-skeys',
>> +  'data': { 'filename': 'str' },
>> +  'if': 'defined(TARGET_S390X)' }
>> +
>> +##
>> +# @query-cpu-model-comparison:
>> +#
>> +# Compares two CPU models, returning how they compare in a specific
>> +# configuration. The results indicates how both models compare regarding
>> +# runnability. This result can be used by tooling to make decisions if a
>> +# certain CPU model will run in a certain configuration or if a compatible
>> +# CPU model has to be created by baselining.
>> +#
>> +# Usually, a CPU model is compared against the maximum possible CPU model
>> +# of a certain configuration (e.g. the "host" model for KVM). If that CPU
>> +# model is identical or a subset, it will run in that configuration.
>> +#
>> +# The result 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 comparing CPU models. s390x supports
>> +# comparing CPU models.
>> +#
>> +# Returns: a CpuModelBaselineInfo. Returns an error if comparing CPU models 
>> is
>> +#          not supported, if a model cannot be used, if a model contains
>> +#          an unknown cpu definition name, unknown properties or properties
>> +#          with wrong types.
>> +#
>> +# Since: 2.8.0
>> +##
>> +{ 'command': 'query-cpu-model-comparison',
>> +  'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
>> +  'returns': 'CpuModelCompareInfo',
>> +  'if': 'defined(TARGET_S390X)' }
>> +
>> +##
>> +# @query-cpu-model-baseline:
>> +#
>> +# Baseline two CPU models, creating a compatible third model. The created
>> +# model will always be a static, migration-safe CPU model (see "static"
>> +# CPU model expansion for details).
>> +#
>> +# This interface can be used by tooling to create a compatible CPU model out
>> +# two CPU models. The created CPU model will be identical to or a subset of
>> +# both CPU models when comparing them. Therefore, the created CPU model is
>> +# guaranteed to run where the given CPU models run.
>> +#
>> +# The result 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 baselining CPU models. s390x supports
>> +# baselining CPU models.
>> +#
>> +# Returns: a CpuModelBaselineInfo. Returns an error if baselining CPU 
>> models is
>> +#          not supported, if a model cannot be used, if a model contains
>> +#          an unknown cpu definition name, unknown properties or properties
>> +#          with wrong types.
>> +#
>> +# Since: 2.8.0
>> +##
>> +{ 'command': 'query-cpu-model-baseline',
>> +  'data': { 'modela': 'CpuModelInfo',
>> +            'modelb': 'CpuModelInfo' },
>> +  'returns': 'CpuModelBaselineInfo',
>> +  'if': 'defined(TARGET_S390X)' }
>> diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
>> index 32abdfe6a1..f0ef652b2a 100644
>> --- a/include/sysemu/arch_init.h
>> +++ b/include/sysemu/arch_init.h
>> @@ -36,11 +36,4 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error 
>> **errp);
>>  CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
>> type,
>>                                                        CpuModelInfo *mode,
>>                                                        Error **errp);
>> -CpuModelCompareInfo *arch_query_cpu_model_comparison(CpuModelInfo *modela,
>> -                                                     CpuModelInfo *modelb,
>> -                                                     Error **errp);
>> -CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *modela,
>> -                                                    CpuModelInfo *modelb,
>> -                                                    Error **errp);
>> -
>>  #endif
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 76241c240e..59d28c2358 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -13,7 +13,7 @@
>>  #include "hw/boards.h"
>>  #include "hw/s390x/storage-keys.h"
>>  #include "qapi/error.h"
>> -#include "qapi/qapi-commands-misc.h"
>> +#include "qapi/target-qapi-commands.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qemu/error-report.h"
>>  #include "sysemu/kvm.h"
>> diff --git a/monitor.c b/monitor.c
>> index 4ad9225425..bd9a6950cf 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1157,19 +1157,12 @@ static void qmp_query_qmp_schema(QDict *qdict, 
>> QObject **ret_data,
>>   */
>>  static void qmp_unregister_commands_hack(void)
>>  {
>> -#ifndef TARGET_S390X
>> -    qmp_unregister_command(&qmp_commands, "dump-skeys");
>> -#endif
>>  #ifndef TARGET_ARM
>>      qmp_unregister_command(&qmp_commands, "query-gic-capabilities");
>>  #endif
>>  #if !defined(TARGET_S390X) && !defined(TARGET_I386)
>>      qmp_unregister_command(&qmp_commands, "query-cpu-model-expansion");
>>  #endif
>> -#if !defined(TARGET_S390X)
>> -    qmp_unregister_command(&qmp_commands, "query-cpu-model-baseline");
>> -    qmp_unregister_command(&qmp_commands, "query-cpu-model-comparison");
>> -#endif
>>  #if !defined(TARGET_PPC) && !defined(TARGET_ARM) && !defined(TARGET_I386) \
>>      && !defined(TARGET_S390X)
>>      qmp_unregister_command(&qmp_commands, "query-cpu-definitions");
>> @@ -4703,13 +4696,6 @@ QemuOptsList qemu_mon_opts = {
>>      },
>>  };
>>
>> -#ifndef TARGET_S390X
>> -void qmp_dump_skeys(const char *filename, Error **errp)
>> -{
>> -    error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys");
>> -}
>> -#endif
>> -
>>  #ifndef TARGET_ARM
>>  GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
>>  {
>> diff --git a/qmp.c b/qmp.c
>> index d8f80cb04e..14972b78df 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -601,20 +601,6 @@ CpuModelExpansionInfo 
>> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>      return arch_query_cpu_model_expansion(type, model, errp);
>>  }
>>
>> -CpuModelCompareInfo *qmp_query_cpu_model_comparison(CpuModelInfo *modela,
>> -                                                    CpuModelInfo *modelb,
>> -                                                    Error **errp)
>> -{
>> -    return arch_query_cpu_model_comparison(modela, modelb, errp);
>> -}
>> -
>> -CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *modela,
>> -                                                   CpuModelInfo *modelb,
>> -                                                   Error **errp)
>> -{
>> -    return arch_query_cpu_model_baseline(modela, modelb, errp);
>> -}
>
> Not sure, but couldn't these two commands be implemented on other
> architectures in the long run, too? So removing them now here seems
> somewhat counterproductive?

They would have modify the qapi ifdef and implement the qmp handler in
their target, similar to what would be done by implementing arch_query
handlers. How counterproductive is that? The benefit is that we don't
have to have stubs and "de-register" the commands.



-- 
Marc-André Lureau