qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status


From: Jonah Palmer
Subject: Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
Date: Thu, 26 Aug 2021 02:25:34 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0

Hi Jason, could I get your thoughts on this implementation question below?

I'm not too sure on how I should proceed determining if vhost is active or not.

Thank you!


Jonah

On 7/26/21 5:33 AM, Jonah Palmer wrote:


On 7/22/21 5:22 AM, Jason Wang wrote:

在 2021/7/21 下午4:59, Jonah Palmer 写道:


On 7/13/21 10:37 PM, Jason Wang wrote:

在 2021/7/12 下午6:35, Jonah Palmer 写道:
From: Laurent Vivier <lvivier@redhat.com>

This new command shows internal status of a VirtQueue.
(vrings and indexes).

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
  hw/virtio/virtio-stub.c |   6 +++
  hw/virtio/virtio.c      |  37 ++++++++++++++++++
  qapi/virtio.json        | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 145 insertions(+)

  [Jonah: Added 'device-type' field to VirtQueueStatus and
  qmp command x-debug-virtio-queue-status.]

diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
index ddb592f..3c1bf17 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 81a0ee8..ccd4371 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3935,6 +3935,43 @@ 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->device_type = qapi_enum_parse(&VirtioType_lookup, vdev->name,
+ VIRTIO_TYPE_UNKNOWN, NULL);
+    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;


As mentioned in previous versions. We need add vhost support otherwise the value here is wrong.
Got it. I'll add a case to determine if vhost is active for a given device.
So, in the case that vhost is active, should I just not print out the value or would I substitute it with
another value (whatever that might be)?


You can query the vhost for those index.

(vhost_get_vring_base())


  Same question for shadow_avail_idx below as well.


It's an implementation specific. I think we can simply not show it if vhost is enabled.

Thanks

Ah I see, thank you!

So, it appears to me that it's not very easy to get the struct vhost_dev pointer from struct VirtIODevice to indicate whether or not vhost is active, e.g. there's no virtio class-independent way to get struct vhost_dev.

I was thinking of adding an op/callback function to struct VirtioDeviceClass, e.g. bool has_vhost(VirtIODevice *vdev), and implement it for each virtio class (net, scsi, blk, etc.).

For example, for virtio-net, maybe it'd be something like:

bool has_vhost(VirtIODevice *vdev) {
  VirtIONet *n = VIRTIO_NET(vdev);
  NetClientState *nc = qemu_get_queue(n->nic);
  return nc->peer ? get_vhost_net(nc->peer) : false;
}

Also, for getting the last_avail_idx, I was also thinking of adding another op/callback to struct VirtioDeviceClass, e.g. unsigned int get_last_avail_idx(VirtIODevice *vdev, unsigned int vq_idx) that finds if vhost is active or not and either gets last_avail_idx from virtio directly or through vhost (e.g. vhost_dev->vhost_ops->vhost_get_vring_base()).

I wanted to run this by you and get your opinion on this before I started implementing it in code. Let me know what you think about this.


Jonah




Jonah


+    status->shadow_avail_idx = vdev->vq[queue].shadow_avail_idx;


The shadow index is something that is implementation specific e.g in the case of vhost it's kind of meaningless.

Thanks


+    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 78873cd..7007e0c 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -406,3 +406,105 @@
    'data': { 'path': 'str' },
    'returns': 'VirtioStatus'
  }
+
+##
+# @VirtQueueStatus:
+#
+# Status of a VirtQueue
+#
+# @device-type: VirtIO device type
+#
+# @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: 6.1
+#
+##
+
+{ 'struct': 'VirtQueueStatus',
+  'data': {
+    'device-type': 'VirtioType',
+    '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: 6.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-align": 4096,
+#      "vring-desc": 864411648,
+#      "signalled-used-valid": 0,
+#      "vring-num-default": 256,
+#      "vring-avail": 864415744,
+#      "queue-index": 0,
+#      "last-avail-idx": 373,
+#      "vring-used": 864416320,
+#      "used-idx": 373,
+#      "device-type": "virtio-net",
+#      "shadow-avail-idx": 619,
+#      "vring-num": 256
+#      }
+#    }
+#
+##
+
+{ 'command': 'x-debug-virtio-queue-status',
+  'data': { 'path': 'str', 'queue': 'uint16' },
+  'returns': 'VirtQueueStatus'
+}



reply via email to

[Prev in Thread] Current Thread [Next in Thread]