* Re: [RFC v3 1/6] qmp: add QMP command x-debug-query-virtio
[not found] ` <20200507114927.6733-2-lvivier@redhat.com>
@ 2020-05-07 15:38 ` Eric Blake
0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2020-05-07 15:38 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Jason Wang,
Michael S. Tsirkin, Michael Roth, Amit Shah, Max Reitz,
Eric Auger, Gerd Hoffmann, Stefan Hajnoczi,
Marc-André Lureau, Paolo Bonzini, Dr. David Alan Gilbert
On 5/7/20 6:49 AM, Laurent Vivier wrote:
> This new command lists all the instances of VirtIODevice with
> their path and virtio type
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> +++ b/qapi/virtio.json
> @@ -0,0 +1,68 @@
> +##
> +# = Virtio devices
> +##
> +
> +##
> +# @VirtioType:
> +#
> +# An enumeration of Virtio device types.
> +#
> +# Since: 5.1.0
Inconsistent on x.y.z vs.
> +##
> +{ 'enum': 'VirtioType',
> + 'data': [ 'unknown', 'virtio-9p', 'virtio-blk', 'virtio-serial',
> + 'virtio-gpu', 'virtio-input', 'virtio-net', 'virtio-scsi',
> + 'vhost-user-fs', 'vhost-vsock', 'virtio-balloon', 'virtio-crypto',
> + 'virtio-iommu', 'virtio-pmem', 'virtio-rng' ]
> +}
> +
> +##
> +# @VirtioInfo:
> +#
> +# Information about a given VirtIODevice
> +#
> +# @path: VirtIO device canonical path.
> +#
> +# @type: VirtIO device type.
> +#
> +# Since: 5.1
x.y. Most of our QAPI seems to have settled on just x.y these days.
The x-debug- prefix is nice; it gives us more flexibility to mess with
this later as needed.
Otherwise:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 3/6] qmp: decode feature bits in virtio-status
[not found] ` <20200507114927.6733-4-lvivier@redhat.com>
@ 2020-05-07 15:47 ` Eric Blake
2020-05-08 2:54 ` Jason Wang
1 sibling, 0 replies; 8+ messages in thread
From: Eric Blake @ 2020-05-07 15:47 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Jason Wang,
Michael S. Tsirkin, Michael Roth, Amit Shah, Max Reitz,
Eric Auger, Gerd Hoffmann, Stefan Hajnoczi,
Marc-André Lureau, Paolo Bonzini, Dr. David Alan Gilbert
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 <lvivier@redhat.com>
> ---
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 6/6] hmp: add x-debug-virtio commands
[not found] ` <20200507114927.6733-7-lvivier@redhat.com>
@ 2020-05-07 15:51 ` Eric Blake
0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2020-05-07 15:51 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Jason Wang,
Michael S. Tsirkin, Michael Roth, Amit Shah, Max Reitz,
Eric Auger, Gerd Hoffmann, Stefan Hajnoczi,
Marc-André Lureau, Paolo Bonzini, Dr. David Alan Gilbert
On 5/7/20 6:49 AM, Laurent Vivier wrote:
> This patch implements HMP version of the virtio QMP commands
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Most HMP commands do not use '-' in their name. Also, HMP doesn't
promise api compatibility; so we could just name this 'info virtio' or
'debug_virtio' without an x- prefix, with no real loss.
> ---
> Makefile | 2 +-
> Makefile.target | 7 +-
> docs/system/monitor.rst | 2 +
> hmp-commands-virtio.hx | 160 +++++++++++++++++++++++++++++++++
> hmp-commands.hx | 10 +++
> hw/virtio/virtio.c | 193 +++++++++++++++++++++++++++++++++++++++-
> include/monitor/hmp.h | 4 +
> monitor/misc.c | 17 ++++
> 8 files changed, 391 insertions(+), 4 deletions(-)
> create mode 100644 hmp-commands-virtio.hx
>
> +SRST
> +``x-debug-virtio`` *subcommand*
> + Show various information about virtio.
> +
> + Example:
> +
> + List all sub-commands::
> +
> + (qemu) x-debug-virtio
> + x-debug-virtio query -- List all available virtio devices
> + x-debug-virtio status path -- Display status of a given virtio device
> + x-debug-virtio queue-status path queue -- Display status of a given virtio queue
> + x-debug-virtio queue-element path queue [index] -- Display element of a given virtio queue
Oh, you're introducing it as a metacommand (like 'info' already is) with
several subcommands. Still, naming it 'virtio' instead of
'x-debug-virtio' would make sense to me, even though the underlying QMP
commands are (correctly) in the x-debug- namespace.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 3/6] qmp: decode feature bits in virtio-status
[not found] ` <20200507114927.6733-4-lvivier@redhat.com>
2020-05-07 15:47 ` [RFC v3 3/6] qmp: decode feature bits in virtio-status Eric Blake
@ 2020-05-08 2:54 ` Jason Wang
1 sibling, 0 replies; 8+ messages in thread
From: Jason Wang @ 2020-05-08 2:54 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Michael S. Tsirkin,
Michael Roth, Amit Shah, Max Reitz, Eric Auger, Gerd Hoffmann,
Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini,
Dr. David Alan Gilbert
On 2020/5/7 下午7:49, 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<lvivier@redhat.com>
This is really useful. I wonder maybe we need something similar in
driver side, current sysfs can only display magic binary numbers.
Thanks
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 4/6] qmp: add QMP command x-debug-virtio-queue-status
[not found] ` <20200507114927.6733-5-lvivier@redhat.com>
@ 2020-05-08 2:57 ` Jason Wang
2020-05-15 15:16 ` Laurent Vivier
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2020-05-08 2:57 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Michael S. Tsirkin,
Michael Roth, Amit Shah, Max Reitz, Eric Auger, Gerd Hoffmann,
Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini,
Dr. David Alan Gilbert
On 2020/5/7 下午7:49, Laurent Vivier wrote:
> This new command shows internal status of a VirtQueue.
> (vrings and indexes).
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
It looks to me that packed virtqueue is not supported. It's better to
add them in the future.
> ---
> hw/virtio/virtio-stub.c | 6 +++
> hw/virtio/virtio.c | 35 +++++++++++++++
> qapi/virtio.json | 98 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 139 insertions(+)
>
> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
> index ddb592f72eee..3c1bf172acf6 100644
> --- a/hw/virtio/virtio-stub.c
> +++ b/hw/virtio/virtio-stub.c
> @@ -17,3 +17,9 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
> {
> return qmp_virtio_unsupported(errp);
> }
> +
> +VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
> + uint16_t queue, Error **errp)
> +{
> + return qmp_virtio_unsupported(errp);
> +}
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 59bf6ef651a6..57552bf05014 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3877,6 +3877,41 @@ static VirtIODevice *virtio_device_find(const char *path)
> return NULL;
> }
>
> +VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
> + uint16_t queue, Error **errp)
> +{
> + VirtIODevice *vdev;
> + VirtQueueStatus *status;
> +
> + 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;
> + }
> +
> + status = g_new0(VirtQueueStatus, 1);
> + status->queue_index = vdev->vq[queue].queue_index;
> + status->inuse = vdev->vq[queue].inuse;
> + status->vring_num = vdev->vq[queue].vring.num;
> + status->vring_num_default = vdev->vq[queue].vring.num_default;
> + status->vring_align = vdev->vq[queue].vring.align;
> + status->vring_desc = vdev->vq[queue].vring.desc;
> + status->vring_avail = vdev->vq[queue].vring.avail;
> + status->vring_used = vdev->vq[queue].vring.used;
> + status->last_avail_idx = vdev->vq[queue].last_avail_idx;
This might not be correct when vhost is used.
We may consider to sync last_avail_idx from vhost backends here?
Thanks
> + status->shadow_avail_idx = vdev->vq[queue].shadow_avail_idx;
> + status->used_idx = vdev->vq[queue].used_idx;
> + status->signalled_used = vdev->vq[queue].signalled_used;
> + status->signalled_used_valid = vdev->vq[queue].signalled_used_valid;
> +
> + return status;
> +}
> +
> #define CONVERT_FEATURES(type, map) \
> ({ \
> type *list = NULL; \
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 69dd107d0c9b..43c234a9fc69 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -308,3 +308,101 @@
> 'data': { 'path': 'str' },
> 'returns': 'VirtioStatus'
> }
> +
> +##
> +# @VirtQueueStatus:
> +#
> +# Status of a VirtQueue
> +#
> +# @queue-index: VirtQueue queue_index
> +#
> +# @inuse: VirtQueue inuse
> +#
> +# @vring-num: VirtQueue vring.num
> +#
> +# @vring-num-default: VirtQueue vring.num_default
> +#
> +# @vring-align: VirtQueue vring.align
> +#
> +# @vring-desc: VirtQueue vring.desc
> +#
> +# @vring-avail: VirtQueue vring.avail
> +#
> +# @vring-used: VirtQueue vring.used
> +#
> +# @last-avail-idx: VirtQueue last_avail_idx
> +#
> +# @shadow-avail-idx: VirtQueue shadow_avail_idx
> +#
> +# @used-idx: VirtQueue used_idx
> +#
> +# @signalled-used: VirtQueue signalled_used
> +#
> +# @signalled-used-valid: VirtQueue signalled_used_valid
> +#
> +# Since: 5.1
> +#
> +##
> +
> +{ 'struct': 'VirtQueueStatus',
> + 'data': {
> + 'queue-index': 'uint16',
> + 'inuse': 'uint32',
> + 'vring-num': 'int',
> + 'vring-num-default': 'int',
> + 'vring-align': 'int',
> + 'vring-desc': 'uint64',
> + 'vring-avail': 'uint64',
> + 'vring-used': 'uint64',
> + 'last-avail-idx': 'uint16',
> + 'shadow-avail-idx': 'uint16',
> + 'used-idx': 'uint16',
> + 'signalled-used': 'uint16',
> + 'signalled-used-valid': 'uint16'
> + }
> +}
> +
> +##
> +# @x-debug-virtio-queue-status:
> +#
> +# Return the status of a given VirtQueue
> +#
> +# @path: QOBject path of the VirtIODevice
> +#
> +# @queue: queue number to examine
> +#
> +# Returns: Status of the VirtQueue
> +#
> +# Since: 5.1
> +#
> +# Example:
> +#
> +# -> { "execute": "x-debug-virtio-queue-status",
> +# "arguments": {
> +# "path": "/machine/peripheral-anon/device[3]/virtio-backend",
> +# "queue": 0
> +# }
> +# }
> +# <- { "return": {
> +# "signalled-used": 373,
> +# "inuse": 0,
> +# "vring-desc": 864411648,
> +# "vring-num-default": 256,
> +# "signalled-used-valid": 1,
> +# "vring-avail": 864415744,
> +# "last-avail-idx": 373,
> +# "queue-index": 0,
> +# "vring-used": 864416320,
> +# "shadow-avail-idx": 619,
> +# "used-idx": 373,
> +# "vring-num": 256,
> +# "vring-align": 4096
> +# }
> +# }
> +#
> +##
> +
> +{ 'command': 'x-debug-virtio-queue-status',
> + 'data': { 'path': 'str', 'queue': 'uint16' },
> + 'returns': 'VirtQueueStatus'
> +}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 5/6] qmp: add QMP command x-debug-virtio-queue-element
[not found] ` <20200507114927.6733-6-lvivier@redhat.com>
@ 2020-05-08 3:03 ` Jason Wang
0 siblings, 0 replies; 8+ messages in thread
From: Jason Wang @ 2020-05-08 3:03 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Michael S. Tsirkin,
Michael Roth, Amit Shah, Max Reitz, Eric Auger, Gerd Hoffmann,
Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini,
Dr. David Alan Gilbert
On 2020/5/7 下午7:49, Laurent Vivier wrote:
> This new command shows the information of a VirtQueue element.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hw/virtio/virtio-stub.c | 9 +++
> hw/virtio/virtio.c | 130 ++++++++++++++++++++++++++++++++++++++++
> qapi/virtio.json | 94 +++++++++++++++++++++++++++++
> 3 files changed, 233 insertions(+)
>
> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
> index 3c1bf172acf6..8275e31430e7 100644
> --- a/hw/virtio/virtio-stub.c
> +++ b/hw/virtio/virtio-stub.c
> @@ -23,3 +23,12 @@ VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
> {
> return qmp_virtio_unsupported(errp);
> }
> +
> +VirtioQueueElement *qmp_x_debug_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 57552bf05014..66dc2cef1b39 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -4033,6 +4033,136 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
> return status;
> }
>
> +static VirtioRingDescFlagsList *qmp_decode_vring_desc_flags(uint16_t flags)
> +{
> + VirtioRingDescFlagsList *list = NULL;
> + VirtioRingDescFlagsList *node;
> + int i;
> + struct {
> + uint16_t flag;
> + VirtioRingDescFlags value;
> + } map[] = {
> + { VRING_DESC_F_NEXT, VIRTIO_RING_DESC_FLAGS_NEXT },
> + { VRING_DESC_F_WRITE, VIRTIO_RING_DESC_FLAGS_WRITE },
> + { VRING_DESC_F_INDIRECT, VIRTIO_RING_DESC_FLAGS_INDIRECT },
> + { 1 << VRING_PACKED_DESC_F_AVAIL, VIRTIO_RING_DESC_FLAGS_AVAIL },
> + { 1 << VRING_PACKED_DESC_F_USED, VIRTIO_RING_DESC_FLAGS_USED },
> + { 0, -1 }
> + };
> +
> + for (i = 0; map[i].flag; i++) {
> + if ((map[i].flag & flags) == 0) {
> + continue;
> + }
> + node = g_malloc0(sizeof(VirtioRingDescFlagsList));
> + node->value = map[i].value;
> + node->next = list;
> + list = node;
> + }
> +
> + return list;
> +}
> +
> +VirtioQueueElement *qmp_x_debug_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;
> +
> + 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;
> + }
> + i = 0;
> + vring_split_desc_read(vdev, &desc, desc_cache, i);
> + }
> +
> + element = g_new0(VirtioQueueElement, 1);
> + element->index = head;
> + element->ndescs = 0;
> +
> + do {
> + 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;
> +
> + element->ndescs++;
> +
> + rc = virtqueue_split_read_next_desc(vdev, &desc, desc_cache,
> + max, &i);
> + } while (rc == VIRTQUEUE_READ_DESC_MORE);
It's better to limit the maximum number of iterations here to e.g
vring.num. A buggy driver may produce a infinite loop here.
Thanks
> +
> + 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 43c234a9fc69..a55103dca780 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -406,3 +406,97 @@
> 'data': { 'path': 'str', 'queue': 'uint16' },
> 'returns': 'VirtQueueStatus'
> }
> +
> +##
> +# @VirtioRingDescFlags:
> +#
> +# An enumeration of the virtio ring descriptor flags
> +#
> +# Since: 5.1
> +#
> +##
> +
> +{ 'enum': 'VirtioRingDescFlags',
> + 'data': [ 'next', 'write', 'indirect', 'avail', 'used' ]
> +}
> +
> +##
> +# @VirtioRingDesc:
> +#
> +# @addr: guest physical address of the descriptor data
> +#
> +# @len: length of the descriptor data
> +#
> +# @flags: descriptor flags
> +#
> +# Since: 5.1
> +#
> +##
> +
> +{ 'struct': 'VirtioRingDesc',
> + 'data': {
> + 'addr': 'uint64',
> + 'len': 'uint32',
> + 'flags': [ 'VirtioRingDescFlags' ]
> + }
> +}
> +
> +##
> +# @VirtioQueueElement:
> +#
> +# @index: index of the element in the queue
> +#
> +# @ndescs: number of descriptors
> +#
> +# @descs: list of the descriptors
> +#
> +# Since: 5.1
> +#
> +##
> +
> +{ 'struct': 'VirtioQueueElement',
> + 'data': {
> + 'index': 'uint32',
> + 'ndescs': 'uint32',
> + 'descs': ['VirtioRingDesc']
> + }
> +}
> +
> +##
> +# @x-debug-virtio-queue-element:
> +#
> +# Return the information about an element queue (by default head)
> +#
> +# @path: QOBject path of the VirtIODevice
> +#
> +# @queue: queue number to examine
> +#
> +# @index: the index in the queue, by default head
> +#
> +# Returns: the element information
> +#
> +# Since: 5.1
> +#
> +# Example:
> +#
> +# -> { "execute": "x-debug-virtio-queue-element",
> +# "arguments": {
> +# "path": "/machine/peripheral-anon/device[3]/virtio-backend",
> +# "queue": 0
> +# }
> +# }
> +# -> { "return": {
> +# "index": 24,
> +# "ndescs": 1,
> +# "descs": [
> +# { "flags": ["write"], "len": 1536, "addr": 2027557376 }
> +# ]
> +# }
> +# }
> +#
> +##
> +
> +{ 'command': 'x-debug-virtio-queue-element',
> + 'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' },
> + 'returns': 'VirtioQueueElement'
> +}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 4/6] qmp: add QMP command x-debug-virtio-queue-status
2020-05-08 2:57 ` [RFC v3 4/6] qmp: add QMP command x-debug-virtio-queue-status Jason Wang
@ 2020-05-15 15:16 ` Laurent Vivier
2020-05-18 3:31 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2020-05-15 15:16 UTC (permalink / raw)
To: Jason Wang, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Michael S. Tsirkin,
Michael Roth, Amit Shah, Max Reitz, Eric Auger, Gerd Hoffmann,
Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini,
Dr. David Alan Gilbert
On 08/05/2020 04:57, Jason Wang wrote:
>
> On 2020/5/7 下午7:49, Laurent Vivier wrote:
>> This new command shows internal status of a VirtQueue.
>> (vrings and indexes).
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>
>
> It looks to me that packed virtqueue is not supported. It's better to
> add them in the future.
I agree, it's why the series still remains an "RFC".
...
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 59bf6ef651a6..57552bf05014 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -3877,6 +3877,41 @@ static VirtIODevice *virtio_device_find(const
>> char *path)
>> return NULL;
>> }
>> +VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
>> + uint16_t queue,
>> Error **errp)
>> +{
>> + VirtIODevice *vdev;
>> + VirtQueueStatus *status;
>> +
>> + 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;
>> + }
>> +
>> + status = g_new0(VirtQueueStatus, 1);
>> + status->queue_index = vdev->vq[queue].queue_index;
>> + status->inuse = vdev->vq[queue].inuse;
>> + status->vring_num = vdev->vq[queue].vring.num;
>> + status->vring_num_default = vdev->vq[queue].vring.num_default;
>> + status->vring_align = vdev->vq[queue].vring.align;
>> + status->vring_desc = vdev->vq[queue].vring.desc;
>> + status->vring_avail = vdev->vq[queue].vring.avail;
>> + status->vring_used = vdev->vq[queue].vring.used;
>> + status->last_avail_idx = vdev->vq[queue].last_avail_idx;
>
>
> This might not be correct when vhost is used.
>
> We may consider to sync last_avail_idx from vhost backends here?
Yes, but I don't know how to do that. Where can I find the information?
Thanks,
Laurent
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC v3 4/6] qmp: add QMP command x-debug-virtio-queue-status
2020-05-15 15:16 ` Laurent Vivier
@ 2020-05-18 3:31 ` Jason Wang
0 siblings, 0 replies; 8+ messages in thread
From: Jason Wang @ 2020-05-18 3:31 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block,
David Hildenbrand, Markus Armbruster, Michael S. Tsirkin,
Michael Roth, Amit Shah, Max Reitz, Eric Auger, Gerd Hoffmann,
Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini,
Dr. David Alan Gilbert
On 2020/5/15 下午11:16, Laurent Vivier wrote:
> On 08/05/2020 04:57, Jason Wang wrote:
>> On 2020/5/7 下午7:49, Laurent Vivier wrote:
>>> This new command shows internal status of a VirtQueue.
>>> (vrings and indexes).
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>
>> It looks to me that packed virtqueue is not supported. It's better to
>> add them in the future.
> I agree, it's why the series still remains an "RFC".
>
> ...
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 59bf6ef651a6..57552bf05014 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -3877,6 +3877,41 @@ static VirtIODevice *virtio_device_find(const
>>> char *path)
>>> return NULL;
>>> }
>>> +VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
>>> + uint16_t queue,
>>> Error **errp)
>>> +{
>>> + VirtIODevice *vdev;
>>> + VirtQueueStatus *status;
>>> +
>>> + 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;
>>> + }
>>> +
>>> + status = g_new0(VirtQueueStatus, 1);
>>> + status->queue_index = vdev->vq[queue].queue_index;
>>> + status->inuse = vdev->vq[queue].inuse;
>>> + status->vring_num = vdev->vq[queue].vring.num;
>>> + status->vring_num_default = vdev->vq[queue].vring.num_default;
>>> + status->vring_align = vdev->vq[queue].vring.align;
>>> + status->vring_desc = vdev->vq[queue].vring.desc;
>>> + status->vring_avail = vdev->vq[queue].vring.avail;
>>> + status->vring_used = vdev->vq[queue].vring.used;
>>> + status->last_avail_idx = vdev->vq[queue].last_avail_idx;
>>
>> This might not be correct when vhost is used.
>>
>> We may consider to sync last_avail_idx from vhost backends here?
> Yes, but I don't know how to do that. Where can I find the information?
It could be synced through vhost ops vhost_get_vring_base(), see
vhost_virtqueue_stop().
Thanks
>
> Thanks,
> Laurent
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-18 3:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200507114927.6733-1-lvivier@redhat.com>
[not found] ` <20200507114927.6733-2-lvivier@redhat.com>
2020-05-07 15:38 ` [RFC v3 1/6] qmp: add QMP command x-debug-query-virtio Eric Blake
[not found] ` <20200507114927.6733-4-lvivier@redhat.com>
2020-05-07 15:47 ` [RFC v3 3/6] qmp: decode feature bits in virtio-status Eric Blake
2020-05-08 2:54 ` Jason Wang
[not found] ` <20200507114927.6733-7-lvivier@redhat.com>
2020-05-07 15:51 ` [RFC v3 6/6] hmp: add x-debug-virtio commands Eric Blake
[not found] ` <20200507114927.6733-5-lvivier@redhat.com>
2020-05-08 2:57 ` [RFC v3 4/6] qmp: add QMP command x-debug-virtio-queue-status Jason Wang
2020-05-15 15:16 ` Laurent Vivier
2020-05-18 3:31 ` Jason Wang
[not found] ` <20200507114927.6733-6-lvivier@redhat.com>
2020-05-08 3:03 ` [RFC v3 5/6] qmp: add QMP command x-debug-virtio-queue-element Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).