Re: [PATCH v10 7/8] qmp: add QMP command x-query-virtio-queue-element

[Markus Armbruster]

Jonah Palmer <jonah.palmer@oracle.com> writes:

> From: Laurent Vivier <lvivier@redhat.com>
>
> This new command shows the information of a VirtQueue element.
>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-stub.c |   9 +++
>  hw/virtio/virtio.c      | 154 ++++++++++++++++++++++++++++++++++++++++
>  qapi/virtio.json        | 183 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 346 insertions(+)
>
> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
> index 13e5f93..7ddb22c 100644
> --- a/hw/virtio/virtio-stub.c
> +++ b/hw/virtio/virtio-stub.c
> @@ -31,3 +31,12 @@ VirtQueueStatus *qmp_x_query_virtio_queue_status(const 
> char *path,
>  {
>      return qmp_virtio_unsupported(errp);
>  }
> +
> +VirtioQueueElement *qmp_x_query_virtio_queue_element(const char *path,
> +                                                     uint16_t queue,
> +                                                     bool has_index,
> +                                                     uint16_t index,
> +                                                     Error **errp)
> +{
> +    return qmp_virtio_unsupported(errp);
> +}
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 459bfb2..8c6cc27 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -475,6 +475,19 @@ static inline void vring_used_write(VirtQueue *vq, 
> VRingUsedElem *uelem,
>      address_space_cache_invalidate(&caches->used, pa, sizeof(VRingUsedElem));
>  }
>  
> +/* Called within rcu_read_lock(). */
> +static inline uint16_t vring_used_flags(VirtQueue *vq)
> +{
> +    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
> +    hwaddr pa = offsetof(VRingUsed, flags);
> +
> +    if (!caches) {
> +        return 0;
> +    }
> +
> +    return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
> +}
> +
>  /* Called within rcu_read_lock().  */
>  static uint16_t vring_used_idx(VirtQueue *vq)
>  {
> @@ -4381,6 +4394,147 @@ VirtQueueStatus 
> *qmp_x_query_virtio_queue_status(const char *path,
>      return status;
>  }
>  
> +static strList *qmp_decode_vring_desc_flags(uint16_t flags)
> +{
> +    strList *list = NULL;
> +    strList *node;
> +    int i;
> +
> +    struct {
> +        uint16_t flag;
> +        const char *value;
> +    } map[] = {
> +        { VRING_DESC_F_NEXT, "next" },
> +        { VRING_DESC_F_WRITE, "write" },
> +        { VRING_DESC_F_INDIRECT, "indirect" },
> +        { 1 << VRING_PACKED_DESC_F_AVAIL, "avail" },
> +        { 1 << VRING_PACKED_DESC_F_USED, "used" },
> +        { 0, "" }
> +    };
> +
> +    for (i = 0; map[i].flag; i++) {
> +        if ((map[i].flag & flags) == 0) {
> +            continue;
> +        }
> +        node = g_malloc0(sizeof(strList));
> +        node->value = g_strdup(map[i].value);
> +        node->next = list;
> +        list = node;
> +    }
> +
> +    return list;
> +}
> +
> +VirtioQueueElement *qmp_x_query_virtio_queue_element(const char *path,
> +                                                     uint16_t queue,
> +                                                     bool has_index,
> +                                                     uint16_t index,
> +                                                     Error **errp)
> +{
> +    VirtIODevice *vdev;
> +    VirtQueue *vq;
> +    VirtioQueueElement *element = NULL;
> +
> +    vdev = virtio_device_find(path);
> +    if (vdev == NULL) {
> +        error_setg(errp, "Path %s is not a VirtIO device", path);
> +        return NULL;
> +    }
> +
> +    if (queue >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, queue)) {
> +        error_setg(errp, "Invalid virtqueue number %d", queue);
> +        return NULL;
> +    }
> +    vq = &vdev->vq[queue];
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        error_setg(errp, "Packed ring not supported");
> +        return NULL;
> +    } else {
> +        unsigned int head, i, max;
> +        VRingMemoryRegionCaches *caches;
> +        MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> +        MemoryRegionCache *desc_cache;
> +        VRingDesc desc;
> +        VirtioRingDescList *list = NULL;
> +        VirtioRingDescList *node;
> +        int rc; int ndescs;
> +
> +        RCU_READ_LOCK_GUARD();
> +
> +        max = vq->vring.num;
> +
> +        if (!has_index) {
> +            head = vring_avail_ring(vq, vq->last_avail_idx % vq->vring.num);
> +        } else {
> +            head = vring_avail_ring(vq, index % vq->vring.num);
> +        }
> +        i = head;
> +
> +        caches = vring_get_region_caches(vq);
> +        if (!caches) {
> +            error_setg(errp, "Region caches not initialized");
> +            return NULL;
> +        }
> +        if (caches->desc.len < max * sizeof(VRingDesc)) {
> +            error_setg(errp, "Cannot map descriptor ring");
> +            return NULL;
> +        }
> +
> +        desc_cache = &caches->desc;
> +        vring_split_desc_read(vdev, &desc, desc_cache, i);
> +        if (desc.flags & VRING_DESC_F_INDIRECT) {
> +            int64_t len;
> +            len = address_space_cache_init(&indirect_desc_cache, 
> vdev->dma_as,
> +                                           desc.addr, desc.len, false);
> +            desc_cache = &indirect_desc_cache;
> +            if (len < desc.len) {
> +                error_setg(errp, "Cannot map indirect buffer");
> +                goto done;
> +            }
> +
> +            max = desc.len / sizeof(VRingDesc);
> +            i = 0;
> +            vring_split_desc_read(vdev, &desc, desc_cache, i);
> +        }
> +
> +        element = g_new0(VirtioQueueElement, 1);
> +        element->avail = g_new0(VirtioRingAvail, 1);
> +        element->used = g_new0(VirtioRingUsed, 1);
> +        element->name = g_strdup(vdev->name);
> +        element->index = head;
> +        element->avail->flags = vring_avail_flags(vq);
> +        element->avail->idx = vring_avail_idx(vq);
> +        element->avail->ring = head;
> +        element->used->flags = vring_used_flags(vq);
> +        element->used->idx = vring_used_idx(vq);
> +        ndescs = 0;
> +
> +        do {
> +            /* A buggy driver may produce an infinite loop */
> +            if (ndescs >= max) {
> +                break;
> +            }
> +            node = g_new0(VirtioRingDescList, 1);
> +            node->value = g_new0(VirtioRingDesc, 1);
> +            node->value->addr = desc.addr;
> +            node->value->len = desc.len;
> +            node->value->flags = qmp_decode_vring_desc_flags(desc.flags);
> +            node->next = list;
> +            list = node;
> +
> +            ndescs++;
> +            rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache,
> +                                                max, &i);
> +        } while (rc == VIRTQUEUE_READ_DESC_MORE);
> +        element->descs = list;
> +done:
> +        address_space_cache_destroy(&indirect_desc_cache);
> +    }
> +
> +    return element;
> +}
> +
>  static const TypeInfo virtio_device_info = {
>      .name = TYPE_VIRTIO_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 56e56d2..2984e48 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -654,3 +654,186 @@
>    'data': { 'path': 'str', 'queue': 'uint16' },
>    'returns': 'VirtVhostQueueStatus',
>    'features': [ 'unstable' ] }
> +
> +##
> +# @VirtioRingDesc:
> +#
> +# Information regarding the vring descriptor area
> +#
> +# @addr: Guest physical address of the descriptor area
> +#
> +# @len: Length of the descriptor area
> +#
> +# @flags: List of descriptor flags
> +#
> +# Since: 7.0
> +#
> +##
> +
> +{ 'struct': 'VirtioRingDesc',
> +  'data': { 'addr': 'uint64',
> +            'len': 'uint32',
> +            'flags': [ 'str' ] } }
> +
> +##
> +# @VirtioRingAvail:
> +#
> +# Information regarding the avail vring (a.k.a. driver area)
> +#
> +# @flags: VRingAvail flags
> +#
> +# @idx: VRingAvail index
> +#
> +# @ring: VRingAvail ring[] entry at provided index
> +#
> +# Since: 7.0
> +#
> +##
> +
> +{ 'struct': 'VirtioRingAvail',
> +  'data': { 'flags': 'uint16',
> +            'idx': 'uint16',
> +            'ring': 'uint16' } }
> +
> +##
> +# @VirtioRingUsed:
> +#
> +# Information regarding the used vring (a.k.a. device area)
> +#
> +# @flags: VRingUsed flags
> +#
> +# @idx: VRingUsed index
> +#
> +# Since: 7.0
> +#
> +##
> +
> +{ 'struct': 'VirtioRingUsed',
> +  'data': { 'flags': 'uint16',
> +            'idx': 'uint16' } }
> +
> +##
> +# @VirtioQueueElement:
> +#
> +# Information regarding a VirtQueue's VirtQueueElement including
> +# descriptor, driver, and device areas
> +#
> +# @name: Name of the VirtIODevice that uses this VirtQueue
> +#
> +# @index: Index of the element in the queue
> +#
> +# @descs: List of descriptors (VirtioRingDesc)
> +#
> +# @avail: VRingAvail info
> +#
> +# @used: VRingUsed info
> +#
> +# Since: 7.0
> +#
> +##
> +
> +{ 'struct': 'VirtioQueueElement',
> +  'data': { 'name': 'str',
> +            'index': 'uint32',
> +            'descs': [ 'VirtioRingDesc' ],
> +            'avail': 'VirtioRingAvail',
> +            'used': 'VirtioRingUsed' } }
> +
> +##
> +# @x-query-virtio-queue-element:
> +#
> +# Return the information about a VirtQueue's VirtQueueElement. By
> +# default it looks at the head of the queue (if no index is given)
> +#
> +# @path: VirtIODevice canonical QOM path
> +#
> +# @queue: VirtQueue index to examine
> +#
> +# @index: Index of the element in the queue

Suggest "(default: head of the queue)", and drop "By default it
looks..." above.

> +#
> +# Features:
> +# @unstable: This command is meant for debugging

End with a period for consistency with existing docs, like you did in
v9.

> +#
> +# Returns: VirtioQueueElement information
> +#
> +# Since: 7.0
> +#
> +# Examples:
> +#
> +# 1. Introspect on virtio-net's VirtQueue 0 at index 5
> +#
> +# -> { "execute": "x-query-virtio-queue-element",
> +#      "arguments": { "path": 
> "/machine/peripheral-anon/device[1]/virtio-backend",
> +#                     "queue": 0,
> +#                     "index": 5 }
> +#    }
> +# <- { "return": {
> +#            "index": 5,
> +#            "name": "virtio-net",
> +#            "descs": [
> +#               { "flags": ["write"], "len": 1536, "addr": 5257305600 }
> +#            ],
> +#            "avail": {
> +#               "idx": 256,
> +#               "flags": 0,
> +#               "ring": 5
> +#            },
> +#            "used": {
> +#               "idx": 13,
> +#               "flags": 0
> +#            },
> +#    }
> +#
> +# 2. Introspect on virtio-crypto's VirtQueue 1 at head
> +#
> +# -> { "execute": "x-query-virtio-queue-element",
> +#      "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend",
> +#                     "queue": 1 }
> +#    }
> +# <- { "return": {
> +#            "index": 0,
> +#            "name": "virtio-crypto",
> +#            "descs": [
> +#               { "flags": [], "len": 0, "addr": 8080268923184214134 }
> +#            ],
> +#            "avail": {
> +#               "idx": 280,
> +#               "flags": 0,
> +#               "ring": 0
> +#            },
> +#            "used": {
> +#               "idx": 280,
> +#               "flags": 0
> +#            }
> +#    }
> +#
> +# 3. Introspect on virtio-scsi's VirtQueue 2 at head
> +#
> +# -> { "execute": "x-query-virtio-queue-element",
> +#      "arguments": { "path": 
> "/machine/peripheral-anon/device[2]/virtio-backend",
> +#                     "queue": 2 }
> +#    }
> +# <- { "return": {
> +#            "index": 19,
> +#            "name": "virtio-scsi",
> +#            "descs": [
> +#               { "flags": ["used", "indirect", "write"], "len": 4099327944,
> +#                 "addr": 12055409292258155293 }
> +#            ],
> +#            "avail": {
> +#               "idx": 1147,
> +#               "flags": 0,
> +#               "ring": 19
> +#            },
> +#            "used": {
> +#               "idx": 280,
> +#               "flags": 0
> +#            }
> +#    }
> +#
> +##
> +
> +{ 'command': 'x-query-virtio-queue-element',
> +  'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' },
> +  'returns': 'VirtioQueueElement',
> +  'features': [ 'unstable' ] }

With my doc remarks addressed, QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>

One more thing: a brief note in the commit messages on why I made you
use 'str' instead of enums would be nice.