Re: [RFC v3 3/6] qmp: decode feature bits in virtio-status

[Eric Blake]

On 5/7/20 6:49 AM, Laurent Vivier wrote:
> Display feature names instead of a features bitmap for host, guest
> and backend.
>
> Decode features according device type, transport features are
> on the first line. Undecoded bits (if any) are stored in a separate
> field.
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---

I didn't closely review the code, but for the QAPI parts:

> +++ b/qapi/virtio.json

> +##
> +# @VirtioBlkFeature:
> +#
> +# An enumeration of Virtio block features
> +#
> +# Since: 5.1
> +##
> +
> +{ 'enum': 'VirtioBlkFeature',
> +  'data': [ 'size-max', 'seg-max', 'geometry', 'ro', 'blk-size', 'topology',   
>            'mq', 'discard', 'write-zeroes', 'barrier', 'scsi', 'flush',

Missing newline.

> +##
> +# @VirtioDeviceFeatures:
> +#
> +# An union to define the list of features for a virtio device

s/An/A/ (in English, 'an' goes with soft u, 'a' goes with pronounced u. 
You can remember with "a unicorn gets an umbrella")

> +#
> +# Since: 5.1
> +##
> +
> +{ 'union': 'VirtioDeviceFeatures',
> +  'data': {
> +    'virtio-serial': [ 'VirtioSerialFeature' ],
> +    'virtio-blk': [ 'VirtioBlkFeature' ],
> +    'virtio-gpu': [ 'VirtioGpuFeature' ],
> +    'virtio-net': [ 'VirtioNetFeature' ],
> +    'virtio-scsi': [ 'VirtioScsiFeature' ],
> +    'virtio-balloon': [ 'VirtioBalloonFeature' ],
> +    'virtio-iommu': [ 'VirtioIommuFeature' ]
> +  }
> +}

This is a legacy union rather than a flat union, which results in more 
{} in the QMP wire format.  Is it worth trying to make this a flat 
union, by labeling an appropriate member as 'discriminator'?

> +
> +##
> +# @VirtioStatusFeatures:
> +#
> +# @transport: the list of transport features of the virtio device
> +#
> +# @device: the list of the virtio device specific features
> +#
> +# @unknown: virtio bitmap of the undecoded features
> +#
> +# Since: 5.1
> +##
> +
> +{ 'struct': 'VirtioStatusFeatures',
> +  'data': { 'transport': [ 'VirtioTransportFeature' ],
> +            'device': 'VirtioDeviceFeatures',
> +            'unknown': 'uint64' }
> +}
> +
>   ##
>   # @VirtioStatus:
>   #
> @@ -101,9 +245,9 @@
>     'data': {
>       'device-id': 'int',
>       'device-endian': 'VirtioStatusEndianness',
> -    'guest-features': 'uint64',
> -    'host-features': 'uint64',
> -    'backend-features': 'uint64',
> +    'guest-features': 'VirtioStatusFeatures',
> +    'host-features': 'VirtioStatusFeatures',
> +    'backend-features': 'VirtioStatusFeatures',

This is intra-series churn.  Should we be trying to get the right types 
in place from the get-go?  Or at least clarify in the commit message of 
the earlier patch that the format will be incrementally improved later?

>       'num-vqs': 'uint16'
>     }
>   }
> @@ -123,18 +267,40 @@
>   #
>   # -> { "execute": "x-debug-virtio-status",
>   #      "arguments": {
> -#          "path": "/machine/peripheral-anon/device[3]/virtio-backend"
> +#          "path": "/machine/peripheral-anon/device[1]/virtio-backend"
>   #      }
>   #   }
>   # <- { "return": {
> -#          "backend-features": 0,
> -#          "guest-features": 5111807911,
> -#          "num-vqs": 3,
> -#          "host-features": 6337593319,
>   #          "device-endian": "little",
> -#          "device-id": 1
> +#          "device-id": 3,
> +#          "backend-features": {
> +#              "device": {
> +#                  "type": "virtio-serial",
> +#                  "data": []
> +#              },
> +#              "unknown": 0,
> +#              "transport": []
> +#          },

If we use a flat union, this could look like:

"backend-feature": {
  "type": "virtio-serial",
  "features": [],
  "unknown": 0,
  "transport": []
},

Should 'unknown' be optional so it can be omitted when zero?  Should it 
be named 'unknown-features'?

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