Re: [qemu-s390x] [PATCHv2 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Eric Blake]

On 02/13/2018 11:18 AM, Viktor Mihajlovski wrote:
> The s390 CPU state can be retrieved without interrupting the
> VM execution. Extendend the CpuInfoFast union with architecture

s/Extendend/Extend/

> specific data and an implementation for s390.
>
> Return data looks like this:
>   [
>     {"thread-id":64301,"props":{"core-id":0},
>      "arch":"s390","cpu-state":"operating",
>      "qom-path":"/machine/unattached/device[0]","cpu-index":0},
>     {"thread-id":64302,"props":{"core-id":1},
>      "arch":"s390","cpu-state":"operating",
>      "qom-path":"/machine/unattached/device[1]","cpu-index":1}
> ]
>
> Currently there's a certain amount of duplication between
> the definitions of CpuInfo and CpuInfoFast, both in the
> base and variable areas, since there are data fields common
> to the slow and fast variants.

Even with the difference in spelling that I pointed out in 2/3?

> A suggestion was made on the mailing list to enhance the QAPI
> code generation to support two layers of unions. This would
> allow to specify the common fields once and avoid the duplication
> in the leaf unions.
>
> On the other hand, the slow query-cpus should be deprecated
> along with the slow CpuInfo type and eventually be removed.
> Assuming that new architectures will not be added at high
> rates, we could live with the duplication for the time being.

Yes, this part is true.

> Signed-off-by: Viktor Mihajlovski <address@hidden>
> ---
>   cpus.c           | 10 ++++++++++
>   hmp.c            | 10 ++++++++++
>   qapi-schema.json | 35 +++++++++++++++++++++++++++++------
>   3 files changed, 49 insertions(+), 6 deletions(-)

> +++ b/qapi-schema.json
> @@ -537,15 +537,26 @@
>     'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
>   
>   ##
> -# @CpuInfoS390:
> +# @CpuInfoS390Fast:
>   #
> -# Additional information about a virtual S390 CPU
> +# Additional information about a virtual S390 CPU which can be
> +# obtained without a performance penalty for a running VM
>   #
>   # @cpu-state: the virtual CPU's state
>   #
>   # Since: 2.12
>   ##
> -{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
> +{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } }
> +
> +##
> +# @CpuInfoS390:
> +#
> +# Additional information about a virtual S390 CPU, potentially expensive
> +# to obtain
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } }

This part works - but since you aren't adding any further fields, what 
is the point in having two type names?  Why not just document that 
CpuInfoS390 is always fast, whether used in CpuInfoFast or in the slower 
CpuInfo?  It's not like we get any additional compile-time safety by 
inventing a new type.

>   
>   ##
>   # @query-cpus:
> @@ -604,12 +615,24 @@
>   # @props: properties describing to which node/socket/core/thread
>   #         virtual CPU belongs to, provided if supported by board
>   #
> +# @arch: architecture of the cpu, which determines which additional fields
> +#        will be listed

Doing this leaves a stale comment in query-cpus-fast that the arch name 
is not provided; you'll need to delete that comment if this patch goes in.

> +#
>   # Since: 2.12
>   #
>   ##
> -{ 'struct': 'CpuInfoFast',
> -  'data': {'cpu-index': 'int', 'qom-path': 'str',
> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> +{ 'union': 'CpuInfoFast',
> +  'base': {'cpu-index': 'int', 'qom-path': 'str',
> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
> +           'arch': 'CpuInfoArch' },
> +  'discriminator': 'arch',
> +  'data': { 'x86': 'CpuInfoOther',
> +            'sparc': 'CpuInfoOther',
> +            'ppc': 'CpuInfoOther',
> +            'mips': 'CpuInfoOther',
> +            'tricore': 'CpuInfoOther',
> +            's390': 'CpuInfoS390Fast',
> +            'other': 'CpuInfoOther' } }

CpuInfoOther is an empty class, so it works as a placeholder for all 
other arches without having the dichotomy like the 
CpuInfoS390/CpuInfoS390Fast split.

At any rate, whether or not you merge the two CpuInfoS390 type into one, 
converting CpuInfoFast into a union (and NOT trying to inherit between 
the slow type with different field names and the new fast type) seems 
reasonable, if libvirt really wants to keep using cpu-info-fast to learn 
whether S390 vcpus are halted.

>   ##
>   # @query-cpus-fast:

So, if you fix the stale comment here, a v3 patch can have
Acked-by: Eric Blake <address@hidden>

I also think your v3 series should touch qemu-doc.texi to start the 
deprecation timeframe.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org