Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio

[Jonah Palmer <jonah.palmer@oracle.com>]

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

No problem! Comments below:

On 8/23/21 9:27 AM, Markus Armbruster wrote:
> Back from my summer break, please excuse the delay.
>
> Jonah Palmer <jonah.palmer@oracle.com> writes:
>
>> On 8/7/21 8:35 AM, Markus Armbruster wrote:
>>> QAPI schema review only.
>>>
>>> Jonah Palmer <jonah.palmer@oracle.com> writes:
>>>
>>>> From: Laurent Vivier <lvivier@redhat.com>
>>>>
>>>> This new command lists all the instances of VirtIODevice with
>>>> their path and virtio type.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>> [...]
>>>
>>>> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>>>> index 4912b97..0c89789 100644
>>>> --- a/qapi/qapi-schema.json
>>>> +++ b/qapi/qapi-schema.json
>>>> @@ -91,5 +91,6 @@
>>>>    { 'include': 'misc.json' }
>>>>    { 'include': 'misc-target.json' }
>>>>    { 'include': 'audio.json' }
>>>> +{ 'include': 'virtio.json' }
>>>>    { 'include': 'acpi.json' }
>>>>    { 'include': 'pci.json' }
>>>> diff --git a/qapi/virtio.json b/qapi/virtio.json
>>>> new file mode 100644
>>>> index 0000000..804adbe
>>>> --- /dev/null
>>>> +++ b/qapi/virtio.json
>>>> @@ -0,0 +1,72 @@
>>> Please insert at the beginning
>>>
>>>      # -*- Mode: Python -*-
>>>      # vim: filetype=python
>>>      #
>> Will do.
>>
>>>> +##
>>>> +# = Virtio devices
>>>> +##
>>>> +
>>>> +##
>>>> +# @VirtioType:
>>>> +#
>>>> +# An enumeration of Virtio device types.
>>>> +#
>>>> +# Since: 6.1
>>> 6.2 now, here and below.
>> Okay, will update for entire series.
>>
>>>> +##
>>>> +{ 'enum': 'VirtioType',
>>>> +  'data': [ 'unknown', 'virtio-net', 'virtio-blk', 'virtio-console',
>>>> +            'virtio-rng', 'virtio-balloon', 'virtio-iomem', 'virtio-rpmsg',
>>>> +            'virtio-scsi', 'virtio-9p', 'virtio-mac80211-wlan',
>>>> +            'virtio-serial', 'virtio-caif', 'virtio-memory-balloon',
>>>> +            'unknown-14', 'unknown-15', 'virtio-gpu', 'virtio-clock',
>>>> +            'virtio-input', 'vhost-vsock', 'virtio-crypto', 'virtio-signal-dist',
>>>> +            'virtio-pstore', 'virtio-iommu', 'virtio-mem', 'unknown-25',
>>>> +            'vhost-user-fs', 'virtio-pmem', 'unknown-28', 'virtio-mac80211-hwsim' ]
>>> Please limit line length to approximately 70 characters.
>> Fixed, sorry about that. Also, should I continue this up to 'virtio-bluetooth'? E.g:
>>
>> ...
>> 'virtio-mac80211-hwsim', 'unknown-30', 'unknown-31',
>> 'unknown-32', 'unknown-33', 'unknown-34', 'unknown-35',
>> 'unknown-36', 'unknown-37', 'unknown-38', 'unknown-39',
>> 'virtio-bluetooth' ]
> Hmm...  how is this enum used?  In this patch:
>
>      VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
>      {
>          VirtioInfoList *list = NULL;
>          VirtioInfoList *node;
>          VirtIODevice *vdev;
>
>          QTAILQ_FOREACH(vdev, &virtio_list, next) {
>              DeviceState *dev = DEVICE(vdev);
>              node = g_new0(VirtioInfoList, 1);
>              node->value = g_new(VirtioInfo, 1);
>              node->value->path = g_strdup(dev->canonical_path);
> --->        node->value->type = qapi_enum_parse(&VirtioType_lookup, vdev->name,
> --->                                            VIRTIO_TYPE_UNKNOWN, NULL);
>              QAPI_LIST_PREPEND(list, node->value);
>          }
>
>          return list;
>      }
>
> This maps VirtioDevice member @name (a string) to an enum value.
>
> As far as I can tell, this member is set in virtio_init() only, to its
> argument.  All callers pass string literals.  They also pass a numeric
> device_id, and the two seem to match, e.g. "virtio-blk" and
> VIRTIO_ID_BLOCK.
>
> Thus, the pairs (numeric device ID, string device ID) already exist in
> the code, but not in a way that lets you map between them.  To get that,
> you *duplicate* the pairs in QAPI.
>
> Having two copies means we get to keep them consistent.  Can we avoid
> that?
>
> The enum has a special member 'unknown' that gets used when @name does
> not parse as enum member, i.e. we failed at keeping the copies
> consistent.  I'm afraid this sweeps a programming error under the rug.
>
> The enum has a bunch of dummy members like 'unknown-14' to make QAPI
> generate numeric enum values match the device IDs.  Error prone.  Can't
> see offhand why we need them to match.

Sure, I don't mind doing this instead. Just as an FYI, from the previous
RFC series (RFC v5 1/6), David recommended here that I create a list of
all the types and in the same order as include/standard-headers/linux/virtio_ids.h.

The benefit from this was that we never have to convert between the QAPI number
and the header number.

Let me know if this is still something you'd like me to do!

>
> What about the following.  Define a map from numeric device ID to
> string, like so
>
>      const char *virtio_device_name[] = {
>          [VIRTIO_ID_NET] = "virtio-net",
>          [VIRTIO_ID_BLOCK] = "virtio-blk",
>          ...
>      }

Sorry if this is obvious, but where should I define this mapping?
virtio.c or virtio.h?


Jonah

> This lets you
>
> * map device ID to string by subscripting virtio_device_name[].
>    Guarding with assert(device_id < G_N_ELEMENTS(virtio_device_name)) may
>    be advisable.
>
> * map string to device ID by searching virtio_device_name[].  May want a
>    function for that,
>
> Now you can have virtio_init() map parameter @device_id to string, and
> drop parameter @name.
>
> Then you have two options:
>
> 1. With QAPI enum VirtioType
>
>     Define it without the special and the dummy members.
>
>     To map from string to QAPI enum, use qapi_enum_parse().
>
>     To map from QAPI enum to string, use VirtioType_str().
>
>     To map from QAPI enum to device ID, map through string.
>
> 2. Without QAPI enum VirtioType
>
>     Simply use uint16_t device_id, just like struct VirtioDevice.
>
> The choice between 1. and 2. depends on whether you actually need
> additional functionality provided by QAPI, such as types being visible
> in query-qmp-schema.
>
> [...]
>

[-- Attachment #2: Type: text/html, Size: 7996 bytes --]