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

[Markus Armbruster]

Jan Dakinevich <address@hidden> writes:

> On Tue, 19 Dec 2017 15:57:18 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> QAPI/QMP interface review only.
>> 
>> You neglected to cc: the maintainers of qapi-schema.json, so I'm doing
>> that for you.
>> 
>
> Thank you so much. I screwed up with this patchset at least twice: with
> list of maintainers and with my own e-mail.
>
>> Jan Dakinevich <address@hidden> writes:
>> 
>> > 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 `tell_feature_name'
>> > callback in VirtioDeviceClass also was introduced.
>> >
>> > Cc: Denis V. Lunev <address@hidden>
>> > Signed-off-by: Jan Dakinevich <address@hidden>
>> [...]
>> > diff --git a/qapi-schema.json b/qapi-schema.json
>> > index 1845795..51cd0f3 100644
>> > --- a/qapi-schema.json
>> > +++ b/qapi-schema.json
>> > @@ -3200,3 +3200,73 @@
>> >  # Since: 2.11
>> >  ##
>> >  { 'command': 'watchdog-set-action', 'data' : {'action':
>> > 'WatchdogAction'} } +
>> > +##
>> > +# @VirtioFeature:
>> > +#
>> > +# Virtio feature bit with negotiation status
>> > +#
>> > +# @name: name of feature bit
>> > +#
>> > +# @acked: negotiation status, `true' if guest acknowledged the
>> > feature +#
>> > +# Since: 2.12
>> > +##
>> > +{
>> > +    'struct': 'VirtioFeature',
>> > +    'data': {
>> > +        'name': 'str',
>> > +        'acked': 'bool'
>> > +    }
>> > +}
>> > +
>> > +##
>> > +# @VirtioInfo:
>> > +#
>> > +# Information about virtio device
>> > +#
>> > +# @qom-path: QOM path of the device
>> > +#
>> > +# @status: status bitmask
>> > +#
>> > +# @host-features: bitmask of features, exposed by device
>> > +#
>> > +# @guest-features: bitmask of features, acknowledged by guest
>> 
>> Quoting my review of v4: "Unsigned integer interpreted as combination
>> of well-known bit-valued symbols" is a fine C interface, but a pretty
>> horrid QMP interface.  What's wrong with doing a set the
>> straightforward way as "array of enum"?  As far as I can see, I
>> didn't get a reply back then.  You'll have to give one to get the
>> patch accepted.
>> 
>
> Most probably, I didn't understand you last time. Although, I'm still
> far from correct understanding.
>
> About 'array of enum', I could make the following description:
>
> {
>   'enum': 'VirtioFeatureScsiBit',
>   'data': ['inout', 'hotplug', 'change', 'ta10_pi']
> }
>
> {
>   'struct': 'VirtioFeatureScsi',
>   'data' : {
>       'bit': 'VirtioFeatureScsiBit',
>       'acked': 'bool'
>   }
> }
>
> {
>   'enum': 'VirtioFeatureSerialBit',
>   'data': ['size', 'multiport', 'emerg-write']
> }
>
> {
>   'struct': 'VirtioFeatureSerial',
>   'data' : {
>       'bit': 'VirtioFeatureSerialBit',
>       'acked': 'bool'
>   }
> }
>
> { 
>   'enum': 'VirtioFeatureCategory',
>   'data': ['scsi', 'serial'] 
> }
>
> {
>   'union': 'VirtioFeature',
>   'base': {
>     'category': 'VirtioFeatureCategory'
>   },
>   'discriminator': 'categoty',
>   'data': {
>     'scsi': 'VirtioFeatureScsi',
>     'serial': 'VirtioFeatureSerial'
>   }
> }
>
> {
>   'union': 'VirtioInfo',
>   'data': {
>     'qom-path': 'str',
>     'device-features': ['VirtioFeature']
>     ...
>   }
> }
>
> But IMO, it only increases complexity without any benefit. To use this
> description, at first it should be listed all features names in enums.
> Then, these enums should be mapped into bit numbers and differently for
> each virtio device. I expect something like this:
>
> VirtioFeatureScsiBit bit = ...
>
> switch (bit) {
> case VIRTIO_FEATURE_SCSI_BIT_INOUT:
>     return VIRTIO_SCSI_F_INOUT;
> case VIRTIO_FEATURE_SERIAL_BIT_HOTPLUG:
>     return VIRTIO_SCSI_F_HOTPLUG;
> case VIRTIO_FEATURE_SERIAL_BIT_CHANGE:
>     return VIRTIO_SCSI_F_CHANGE;
> case VIRTIO_FEATURE_SERIAL_BIT_T10_PI:
>     return VIRTIO_SCSI_F_T10_PI;
> }
>
> So, boilerplate for mapping between bit numbers and and theirs names
> will not go away.

No argument.  It's not about simplifying the implementation, it's about
finding the appropriate QMP interface.

> Furthermore, HMP command should produce readable output from QMP data,
> features's enum values should be converted to strings from json
> description. For that, for each possible VirtioFeatureCategary should
> be called respective VirtioFeature*_str macro to resolve enum value.
>
> So, all my attempts to declare bit names within QAPI brings me to
> familiar solutions.

QMP prefers symbols (strings on the wire, preferably enums in the
schema) over magic numbers.

That said, numbers may be okay in certain cases, say when they're
sufficiently well-known and QEMU only passes them along.  Drawback:
defeats introspection; you can't see what bits this QEMU supports.  Why
is that not interesting?

Taking a step back: what's the intended purpose of query-virtio?  The
cover letter doesn't really say:

    After some discussion, I am going to suggest reworked QMP/HMP for
    gathering virtio info. It would provide the following monitor output.

    (qemu) info virtio
    virtio-blk-device at 0000:00:02.0
      QOM path: /machine/peripheral-anon/device[0]/virtio-backend
      status: 0x0f
        VIRTIO_CONFIG_S_ACKNOWLEDGE
        VIRTIO_CONFIG_S_DRIVER
        VIRTIO_CONFIG_S_DRIVER_OK
        VIRTIO_CONFIG_S_FEATURES_OK
      host features:  0x0000000179000e54
      guest features: 0x0000000130000e54
      common features:
        VIRTIO_F_NOTIFY_ON_EMPTY
        VIRTIO_F_ANY_LAYOUT
        VIRTIO_RING_F_INDIRECT_DESC              acked
        VIRTIO_RING_F_EVENT_IDX                  acked
        VIRTIO_F_BAD_FEATURE
        VIRTIO_F_VERSION_1                       acked
      device features:
        VIRTIO_BLK_F_SEG_MAX                     acked
        VIRTIO_BLK_F_BLK_SIZE                    acked
        VIRTIO_BLK_F_FLUSH                       acked
        VIRTIO_BLK_F_TOPOLOGY                    acked

The first sentence carries no useful information; suggest to scratch it.
The rest is an HMP example.  Examples are great, but we really need to
understand *why* you need this command.

>> In case symbolic names may not be known at QEMU compile time (me
>> having to wonder at v6 is a sign of rather ineffective patch review,
>> sorry about that): you have to explain this in the doc comment, with a
>> reference to where the bits are specified.
>> 
>
> ok

Can you think of a scenario where status or feature bits occur that are
not known to QEMU at compile time?

>> > +#
>> > +# @status-names: names of checked bits in status bitmask
>> 
>> How are the strings in @status-names connected to the bits in @status?
>> Spell it out in this doc string, please.
>> 
>
> ok
>
> ...but, strictly saying, I was not going to show in QAPI how these names
> are mapped into bits, QMP answer would not contain how these names are
> connected to bits. Otherwise, It would be reinvention of v4.

To avoid going in circles some more, let's take a step back and examine
what exactly you're trying to accomplish.  Give us use cases: this is my
need, and this is how you use the proposed command to satisfy my need.
Also give us design limitations: what the command is *not* trying to do.

>> > +#
>> > +# @common-features-names: names exposed features and negotiation
>> > status, +# that are common to all devices
>> > +#
>> > +# @device-features-names: names exposed features and negotiation
>> > status, +# that are specific to this device
>> 
>> Likewise.
>> 
>
> ok
>
>> > +#
>> > +# Since: 2.12
>> > +##
>> > +{
>> > +    'struct': 'VirtioInfo',
>> > +    'data': {
>> > +        'qom-path': 'str',
>> > +
>> > +        'status': 'uint8',
>> > +        'host-features': 'uint64',
>> > +        'guest-features': 'uint64',
>> > +
>> > +        'status-names': ['str'],
>> > +        'common-features-names': ['VirtioFeature'],
>> > +        'device-features-names': ['VirtioFeature']
>> > +    }
>> > +}
>> > +
>> > +##
>> > +# @query-virtio:
>> > +#
>> > +# Returns virtio information such as device status and features
>> > +#
>> > +# Since: 2.12
>> > +##
>> > +{
>> > +    'command': 'query-virtio',
>> > +    'data': {'*path': 'str'},
>> > +    'returns': ['VirtioInfo']
>> > +}