Re: [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-virtio' QMP command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4283 bytes --]

On 10/03/2017 07:47 AM, Jan Dakinevich wrote:
> The command is intended for gathering virtio information such as status,
> feature bits, negotiation status. It is convenient and useful for debug
> purpose.
> 
> The commands returns generic virtio information for virtio such as
> common feature names and status bits names and information for all
> attached to current machine devices.
> 
> To retrieve names of device-specific features `get_feature_name'
> callback in VirtioDeviceClass also was introduced.
> 
> Cc: Denis V. Lunev <den@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  hw/block/virtio-blk.c       |  21 +++++++++
>  hw/char/virtio-serial-bus.c |  15 +++++++
>  hw/display/virtio-gpu.c     |  13 ++++++
>  hw/net/virtio-net.c         |  35 +++++++++++++++
>  hw/scsi/virtio-scsi.c       |  16 +++++++
>  hw/virtio/Makefile.objs     |   2 +
>  hw/virtio/virtio-balloon.c  |  15 +++++++
>  hw/virtio/virtio-stub.c     |   9 ++++
>  hw/virtio/virtio.c          | 101 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h  |   2 +
>  qapi-schema.json            |   1 +
>  qapi/virtio.json            |  94 +++++++++++++++++++++++++++++++++++++++++
>  12 files changed, 324 insertions(+)
>  create mode 100644 hw/virtio/virtio-stub.c
>  create mode 100644 qapi/virtio.json

This creates a new .json file, but does not touch MAINTAINERS.  Our idea
in splitting the .json files was to make it easier for each sub-file
that needs a specific maintainer in addition to the overall *.json line
for QAPI maintainers, so this may deserve a MAINTAINERS entry.


> +++ b/qapi/virtio.json
> @@ -0,0 +1,94 @@
> +# -*- Mode: Python -*-
> +#
> +
> +##
> +# = Virtio devices
> +##
> +
> +{ 'include': 'common.json' }
> +
> +##
> +# @VirtioInfoBit:
> +#
> +# Named virtio bit
> +#
> +# @bit: bit number
> +#
> +# @name: bit name
> +#
> +# Since: 2.11.0
> +#
> +##
> +{
> +    'struct': 'VirtioInfoBit',
> +    'data': {
> +        'bit': 'uint64',

Why is this a 64-bit value? Are the values 0-63, or are they 1, 2, 4, 8,
...?  The documentation on 'bit number' is rather sparse.

> +        'name': 'str'

Wouldn't an enum type be better than an open-ended string?

> +    }
> +}
> +
> +##
> +# @VirtioInfoDevice:
> +#
> +# Information about specific virtio device
> +#
> +# @qom_path: QOM path of the device

Please make this 'qom-path' - new interfaces should prefer '-' over '_'.

> +#
> +# @feature-names: names of device-specific features
> +#
> +# @host-features: bitmask of features, provided by devices
> +#
> +# @guest-features: bitmask of features, acknowledged by guest
> +#
> +# @status: virtio device status bitmask
> +#
> +# Since: 2.11.0
> +#
> +##
> +{
> +    'struct': 'VirtioInfoDevice',
> +    'data': {
> +        'qom_path': 'str',
> +        'feature-names': ['VirtioInfoBit'],
> +        'host-features': 'uint64',
> +        'guest-features': 'uint64',
> +        'status': 'uint64'

I'm wondering if this is the best representation (where the caller has
to parse the integer and then lookup in feature-names what each bit of
the integer represents).  But I'm not sure I have anything better off
the top of my head.

> +    }
> +}
> +
> +##
> +# @VirtioInfo:
> +#
> +# Information about virtio devices
> +#
> +# @feature-names: names of common virtio features
> +#
> +# @status-names: names of bits which represents virtio device status
> +#
> +# @devices: list of per-device virtio information
> +#
> +# Since: 2.11.0
> +#
> +##
> +{
> +    'struct': 'VirtioInfo',
> +    'data': {
> +        'feature-names': ['VirtioInfoBit'],

Why is feature-names listed at two different nestings of the return value?

> +        'status-names': ['VirtioInfoBit'],
> +        'devices': ['VirtioInfoDevice']
> +    }
> +}
> +
> +
> +##
> +# @query-virtio:
> +#
> +# Returns generic and per-device virtio information
> +#
> +# Since: 2.11.0
> +#
> +##
> +{
> +    'command': 'query-virtio',
> +    'returns': 'VirtioInfo'
> +}
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]