Re: [Qemu-devel] [Patch v1 25/29] qmp: add QMP interface "query-cpu-model-comparison"

[Eduardo Habkost]

On Tue, Aug 02, 2016 at 01:59:11PM +0200, David Hildenbrand wrote:
> Let's provide a standardized interface to compare two CPU models.
> 
> query-cpu-model-compare takes two models and returns what it knows about
> their compability under a certain QEMU machine QEMU has been started with.
> 
> If modelA is a subset of modelB or if both are identical, modelA will run
> in the same configuration as modelB. If modelA is however a superset of
> modelB or if both are incompatible, one can try to create a compatible one
> by "baselining" both models (follow up patch).
> 
> Acked-by: Cornelia Huck <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  include/sysemu/arch_init.h              |  3 ++
>  qapi-schema.json                        | 65 
> +++++++++++++++++++++++++++++++++
>  qmp-commands.hx                         |  6 +++
>  qmp.c                                   |  7 ++++
>  stubs/Makefile.objs                     |  1 +
>  stubs/arch-query-cpu-model-comparison.c | 12 ++++++
>  6 files changed, 94 insertions(+)
>  create mode 100644 stubs/arch-query-cpu-model-comparison.c
> 
> diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
> index 37b2e86..96d47c0 100644
> --- a/include/sysemu/arch_init.h
> +++ b/include/sysemu/arch_init.h
> @@ -38,5 +38,8 @@ 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);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 43f7969..1f8f8ec 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3149,6 +3149,71 @@
>              'model': 'CpuModelInfo' },
>    'returns': 'CpuModelExpansionInfo' }
>  
> +##
> +# @CpuModelCompareResult:
> +#
> +# An enumeration of CPU model comparation results.
> +#
> +# @incompatible: both model definition are incompatible
> +#
> +# @identical: model A == model B
> +#
> +# @superset: model A > model B
> +#
> +# @subset: model A < model B

We need to clarify what superset/subset, ">" and "<" really mean.

> +#
> +# Since: 2.8.0
> +##
> +{ 'enum': 'CpuModelCompareResult',
> +  'data': [ 'incompatible', 'identical', 'superset', 'subset' ] }

I assume implementations are free to return "incompatible" if
they still don't have any extra logic to expand CPU models and
check for supersets/subsets. If that's the case, see my
suggestion below for a generic comparison function.

> +
> +##
> +# @CpuModelCompareInfo
> +#
> +# The result of a CPU model comparison.
> +#
> +# @result: The result of the compare operation.
> +# @responsible-properties: List of properties that led to the comparison 
> result
> +#                          not being identical.
> +#
> +# @responsible-properties is a list of QOM property names that led to
> +# both CPUs not being detected as identical. For identical models, this
> +# list is empty.
> +# If a QOM property is read-only, that means there's no known way to make the
> +# CPU models identical. If the special property name "type" is included, the
> +# models are by definition not identical and cannot be made identical.
> +#
> +# Since: 2.8.0
> +##
> +{ 'struct': 'CpuModelCompareInfo',
> +  'data': {'result': 'CpuModelCompareResult',
> +           'responsible-properties': ['str']
> +          }
> +}
> +
> +##
> +# @query-cpu-model-comparison:
> +#
> +# Compares two CPU models, returning how they compare under a specific QEMU
> +# machine.
> +#
> +# Note: This interface should not be used when global properties of CPU 
> classes
> +#       are changed (e.g. via "-cpu ...").
> +#
> +# s390x supports comparing of all CPU models.

This statement is not true until patch 28/29 is applied.

> Other architectures are not
> +# supported yet.

What if we provide a generic comparison function that does like
the following pseudocode:

def basic_comparison(modela, modelb):
  if modela.name == modelb.name:
    if modela.props == modelb.props:
      return "identical", []
    else:
      #XXX: maybe add some extra logic to check if
      # modela.props is a subset or superset of modelb.props?
      return "incompatible", set(modela.props.keys(), modelb.props.keys())
  else:
    return "incompatible", ["type"]

def full_comparison(modela, modelb):
  r,p = basic_comparison(modela, modelb)
  if r == "incompatible":
    try:
      modela = expand_cpu_model(modela, "full")
      modelb = expand_cpu_model(modelb, "full")
    except:
      # in case "full" expansion mode is not supported
      return r,p
    return basic_comparison(modela, modelb)


> +#
> +# 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' }
> +
>  # @AddfdInfo:
>  #
>  # Information about a file descriptor that was added to an fd set.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7ed9528..0af2098 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3948,6 +3948,12 @@ EQMP
>      },
>  
>      {
> +        .name       = "query-cpu-model-comparison",
> +        .args_type  = "modela:q,modelb:q",
> +        .mhandler.cmd_new = qmp_marshal_query_cpu_model_comparison,
> +    },
> +
> +    {
>          .name       = "query-target",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_query_target,
> diff --git a/qmp.c b/qmp.c
> index 29fbfb8..f551019 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -614,6 +614,13 @@ 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);
> +}
> +
>  void qmp_add_client(const char *protocol, const char *fdname,
>                      bool has_skipauth, bool skipauth, bool has_tls, bool tls,
>                      Error **errp)
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 4929842..da768f0 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -1,5 +1,6 @@
>  stub-obj-y += arch-query-cpu-def.o
>  stub-obj-y += arch-query-cpu-model-expansion.o
> +stub-obj-y += arch-query-cpu-model-comparison.o
>  stub-obj-y += bdrv-next-monitor-owned.o
>  stub-obj-y += blk-commit-all.o
>  stub-obj-y += blockdev-close-all-bdrv-states.o
> diff --git a/stubs/arch-query-cpu-model-comparison.c 
> b/stubs/arch-query-cpu-model-comparison.c
> new file mode 100644
> index 0000000..d5486ae
> --- /dev/null
> +++ b/stubs/arch-query-cpu-model-comparison.c
> @@ -0,0 +1,12 @@
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "sysemu/arch_init.h"
> +#include "qapi/qmp/qerror.h"
> +
> +CpuModelCompareInfo *arch_query_cpu_model_comparison(CpuModelInfo *modela,
> +                                                     CpuModelInfo *modelb,
> +                                                     Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> -- 
> 2.6.6
> 

-- 
Eduardo