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

[Eduardo Habkost]

On Tue, Aug 02, 2016 at 01:59:10PM +0200, David Hildenbrand wrote:
> Let's provide a standardized interface to expand CPU models, like the
> host model. This interface can be used by tooling to get details about a
> specific CPU model, e.g. the "host" model.
> 
> To take care of all architectures, two detail levels for an expansion
> are introduced. Certain architectures might not support all detail levels.
> While "full" will expand and indicate all relevant properties/features
> of a CPU model, "static" expands to a static base CPU model, that will
> never change between QEMU versions and therefore have the same features
> when used under different compatibility machines.
> 
> Acked-by: Cornelia Huck <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  include/sysemu/arch_init.h             |  3 ++
>  qapi-schema.json                       | 86 
> ++++++++++++++++++++++++++++++++++
>  qmp-commands.hx                        |  6 +++
>  qmp.c                                  |  7 +++
>  stubs/Makefile.objs                    |  1 +
>  stubs/arch-query-cpu-model-expansion.c | 12 +++++
>  6 files changed, 115 insertions(+)
>  create mode 100644 stubs/arch-query-cpu-model-expansion.c
> 
> diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
> index d690dfa..37b2e86 100644
> --- a/include/sysemu/arch_init.h
> +++ b/include/sysemu/arch_init.h
> @@ -35,5 +35,8 @@ int kvm_available(void);
>  int xen_available(void);
>  
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp);
> +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> +                                                      CpuModelInfo *mode,
> +                                                      Error **errp);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3f50c1d..43f7969 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3063,6 +3063,92 @@
>  ##
>  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
>  
> +##
> +# @CpuModelInfo:
> +#
> +# Virtual CPU model.
> +#
> +# A CPU model consists of the name of a CPU definition, to which
> +# delta changes are applied (e.g. features added/removed). Most magic values
> +# that an architecture might require should be hidden behind the name.
> +# However, if required, architectures can expose relevant properties.
> +#
> +# @name: the name of the CPU definition the model is based on
> +# @props: #optional a dictionary of properties to be applied

Should we make it explicit that we are talking about QOM
properties?

> +#
> +# Since: 2.8.0
> +##
> +{ 'struct': 'CpuModelInfo',
> +  'data': { 'name': 'str',
> +            '*props': 'any' } }
> +
> +##
> +# @CpuModelExpansionType
> +#
> +# An enumeration of CPU model expansion types.
> +#
> +# @static: Expand to a static CPU model, a combination of a static base
> +#          model name and property delta changes. As the static base model 
> will
> +#          never change, the expanded CPU model will be the same, 
> independant of
> +#          QEMU version or compatibility machines. Therefore, the resulting

We could be more explicit about the guarantees: "independent of
QEMU version, machine type, machine options, and accelerator
options".

> +#          model can be used by tooling without having to specify a
> +#          compatibility machine - e.g. when displaying the "host" model.
> +#          All static CPU models are migration-safe.

This is cool. Unfortunately we are not going to support it in x86
very soon because we don't have any static CPU models.

> +#
> +# @full: Expand all properties. The produced model is not guaranteed to be
> +#        migration-safe, but allows tooling to get an insight and work with
> +#        model details.

I wonder if we really need to document it broadly as "not
guaranteed to be migration-safe". The returned data will be
migration-safe (but not static) if the CPU model being expanded
is migration-safe, won't it?

Also: I wonder what should be the return value for "name" when
expansion type is "full" and we don't have any static CPU models
(like on x86). e.g. returning:
  { name: "host", props: { foo: "on", bar: "on", ... }
would make the interface not directly usable for the expansion of
<cpu mode="host-model">. Maybe that means we have to add at least
one static CPU model to x86, too?

> +#
> +# Since: 2.8.0
> +##
> +{ 'enum': 'CpuModelExpansionType',
> +  'data': [ 'static', 'full' ] }
> +
> +
> +##
> +# @CpuModelExpansionInfo
> +#
> +# The result of a cpu model expansion.
> +#
> +# @model: the expanded CpuModelInfo.
> +#
> +# Since: 2.8.0
> +##
> +{ 'struct': 'CpuModelExpansionInfo',
> +  'data': { 'model': 'CpuModelInfo' } }

I like it that the input and output of query-cpu-model-expansion
is in the same format (CpuModelInfo).

> +
> +
> +##
> +# @query-cpu-model-expansion:
> +#
> +# Expands the given CPU model to different granularities, allowing tooling
> +# to get an understanding what a specific CPU model looks like in QEMU
> +# under a certain QEMU machine.

Maybe "expands a given CPU model (or a combination of CPU model +
additional options) to different granularities".

> +#
> +# Expanding CPU models is in general independant of the accelerator, except
> +# for models like "host" that explicitly rely on an accelerator and can
> +# vary in different configurations.

Unfortunately this is not true in x86. Documenting it this way
might give people the wrong expectations.

Specific guarantees from arch-specific code could be documented
somewhere, but: where?

Maybe arch-specific guarantees should actually become
query-cpu-definitions fields (like we have "static" and
"migration-safe" today). If people are really interested in
accelerator-independent data, we need


> This interface can therefore also be used
> +# to query the "host" CPU model.
> +#
> +# Note: This interface should not be used when global properties of CPU 
> classes
> +#       are changed (e.g. via "-cpu ...").

We could simply enumerate all cases that could affect the return
value. e.g.:

# 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 expansion 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 "-cpu"
#    or global properties is not advised.


> +#
> +# s390x supports expanding of all CPU models with all expansion types. Other
> +# architectures are not supported yet.

I think this paragraph is likely to get obsolete very soon (as
people may forget to update it when implementing the new
interface on other architectures). Also, the paragraph is not
true until patch 27/29 is applied.

Maybe write it as "Some architectures may not support all
expansion types".

Patch 27/29 could add "s390x supports both 'static' and 'full'
expansion types". I wouldn't document it as "supports all
expansion types" because CpuModelExpansionType may be extended in
the future.

> +#
> +# 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.0
> +##
> +{ 'command': 'query-cpu-model-expansion',
> +  'data': { 'type': 'CpuModelExpansionType',
> +            'model': 'CpuModelInfo' },
> +  'returns': 'CpuModelExpansionInfo' }
> +
>  # @AddfdInfo:
>  #
>  # Information about a file descriptor that was added to an fd set.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c8d360a..7ed9528 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3942,6 +3942,12 @@ EQMP
>      },
>  
>      {
> +        .name       = "query-cpu-model-expansion",
> +        .args_type  = "type:s,model:q",
> +        .mhandler.cmd_new = qmp_marshal_query_cpu_model_expansion,
> +    },
> +
> +    {
>          .name       = "query-target",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_query_target,
> diff --git a/qmp.c b/qmp.c
> index b6d531e..29fbfb8 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -607,6 +607,13 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
> **errp)
>      return arch_query_cpu_definitions(errp);
>  }
>  
> +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> +                                                     CpuModelInfo *model,
> +                                                     Error **errp)
> +{
> +    return arch_query_cpu_model_expansion(type, model, 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 55edd15..4929842 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -1,4 +1,5 @@
>  stub-obj-y += arch-query-cpu-def.o
> +stub-obj-y += arch-query-cpu-model-expansion.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-expansion.c 
> b/stubs/arch-query-cpu-model-expansion.c
> new file mode 100644
> index 0000000..ae7cf55
> --- /dev/null
> +++ b/stubs/arch-query-cpu-model-expansion.c
> @@ -0,0 +1,12 @@
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "sysemu/arch_init.h"
> +#include "qapi/qmp/qerror.h"
> +
> +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> +                                                      CpuModelInfo *mode,
> +                                                      Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> -- 
> 2.6.6
> 

-- 
Eduardo