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

[Eric Blake <eblake@redhat.com>]

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

On 10/24/2017 12:50 PM, 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.
> 
> Cc: Denis V. Lunev <den@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---

> +++ b/qapi-schema.json
> @@ -3200,3 +3200,469 @@
>  # Since: 2.11
>  ##
>  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> +
> +##
> +# @VirtioStatus:
> +#
> +# Virtio device status bits
> +#
> +# Since: 2.11

We've entered soft freeze, and this is a new feature, so it may be
better to propose this for 2.12 rather than trying to rush it in now.

> +#
> +##
> +{
> +    'enum': 'VirtioStatus',
> +    'data': [
> +        'acknowledge',
> +        'driver',
> +        'driver-ok',
> +        'features-ok',
> +        'status-reserved-4',
> +        'status-reserved-5',

Are we ever going to report a '*-reserved-NNN' enum value to the end
user?  I hope not, in which case, we don't need to expose it in the QAPI
enums.  Yes, that means you have to do some mapping between named bits
and enum strings (you would no longer have the property that
'status-reserved-4' is value 4, but would instead have to map that bit 6
translates to 'needs-reset' with enum value 4), but a clean
public-facing interface may be worth the hassle.  What's more, as newer
qemu learns new bit names down the road, then expanding the enums to
cover those new names is introspectible; whereas you can't rename an
existing enum value while still retaining back-compat.


> +##
> +# @VirtioFeatureName:
> +#
> +# Since: 2.11
> +#
> +##
> +{
> +    'union': 'VirtioFeatureName',
> +    'data': {
> +        'common': 'VirtioFeatureCommon',
> +        'net': 'VirtioFeatureNet',
> +        'blk': 'VirtioFeatureBlk',

Do we really need to abbreviate this one?

> +        'serial': 'VirtioFeatureSerial',
> +        'balloon': 'VirtioFeatureBalloon',
> +        'scsi': 'VirtioFeatureScsi',
> +        'virtfs': 'VirtioFeatureVirtfs',
> +        'gpu': 'VirtioFeatureGpu',
> +        'other': 'VirtioFeatureOther'
> +    }
> +}

This is a so-called "simple union" - but we do not want any more of
these in our code.  In fact, Markus and I are toying with the idea of
renaming it to "legacy union", and/or making the syntax more painful,
because we WANT to use "flat unions" in all new code (and make their
syntax easier to type).

So, a comparable flat union would be something like:

{ 'enum': 'VirtioFeatureCategory',
  'data': [ 'common', 'net', 'block', 'serial', 'balloon', 'scsi',
'virtfs', 'gpu', 'other' ] }
{ 'union': 'VirtioFeatureName',
  'base': { 'category': 'VirtioFeatureCategory' },
  'discriminator': 'category',
  'data': { 'common': 'VirtioFeatureCommon', ... } }

except that flat unions don't permit enum types (they require a struct
type).  Hmm.

> +
> +##
> +# @VirtioFeature:
> +#
> +# Feature bit with negotiation status
> +#
> +# @host: true if host exposes the feature
> +#
> +# @guest: true if guest acknowledges the feature
> +#
> +# Since: 2.11
> +#
> +##
> +{
> +    'struct': 'VirtioFeature',
> +    'data': {
> +        'host': 'bool',
> +        'guest': 'bool',
> +        'name': 'VirtioFeatureName'
> +    }
> +}

With your proposal, an example on the wire might be:

"feature": { "host": true, "guest": false, "name": { "type": "common",
"data": "notify-on-empty" } }

> +
> +##
> +# @VirtioInfo:
> +#
> +# Information about virtio device
> +#
> +# @qom-path: QOM path of the device
> +#
> +# @status: status bitmask
> +#
> +# @status-names: names of checked bits in status bitmask
> +#
> +# @host-features: bitmask of features, exposed by device
> +#
> +# @guest-features: bitmask of features, acknowledged by guest
> +#
> +# @features: negotiation status and names of device features
> +#
> +##
> +{
> +    'struct': 'VirtioInfo',
> +    'data': {
> +        'qom-path': 'str',
> +        'status': 'uint8',
> +        'status-names': ['VirtioStatus'],

Umm, isn't it better to just have 'status': 'VirtioStatus' (as we can
only ever have a single status value, so we don't really need an array
to tell us what all the other status values are named).

> +        'host-features': 'uint64',
> +        'guest-features': 'uint64',
> +        'features': ['VirtioFeature']

Do we really want to expose both a 64-bit number and an array of feature
names, where the array repeats the information on whether host and guest
support the bit?

I guess we first want to figure out WHAT we want to send over the wire.
Maybe:

"info": { "qom-path": "...", "status": "driver-ok",
          "host": [ { "common": "notify-on-empty" },
                    { "common": "any-layout" },
                    { "net": "csum" } ],
          "guest": [ { "common": "notify-on-empty" },
                     { "net": "csum" } ] }

or

"info": { "qom-path": "...", "status": "driver-ok",
          "host": [ { "type": "common", "value": "notify-on-empty" },
                    { "type": "common", "value": "any-layout" },
                    { "type": "net", "value": "csum" } ],
          "guest": [ { "type": "common", "value": "notify-on-empty" },
                     { "type": "net", "value": "csum" } ] }

or

"info": { "qom-path": "...", "status": "driver-ok",
          "host": { "common": [ "notify-on-empty", "any-layout" ],
                    "net": [ "csum" ] },
          "guest": { "common": [ "notify-on-empty" ] },
                     "net": [ "csum" ] } }

(some of those forms may be easier than others to express in our current
QAPI language; if we need to modify the qapi language to get our ideal
form supported, then that may be worth the effort).

> +
> +##
> +# @query-virtio:
> +#
> +# Returns virtio information such as device status and features
> +#
> +# Since: 2.11
> +#
> +##
> +{
> +    'command': 'query-virtio',
> +    'data': { '*path': 'str' },

Do we need filterable queries, or is it better to just have the command
return info on all virtio devices at once and let the client filter the
results as desired?

> +    '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 --]