All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices
@ 2021-07-12 10:35 Jonah Palmer
  2021-07-12 10:35 ` [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio Jonah Palmer
                   ` (7 more replies)
  0 siblings, 8 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

This series introduces new QMP/HMP commands to dump the status of a
virtio device at different levels.

[Jonah: Rebasing previous patchset from March for Qemu 6.1
(here: https://lore.kernel.org/qemu-devel/1616084984-11263-1-git-send-email-jonah.palmer@oracle.com/)
from Laurent's original qmp/hmp virtio commands from last May
(here: https://lore.kernel.org/qemu-devel/20200507134800.10837-1-lvivier@redhat.com/)
which included updating Makefile to meson.build, adding all virtio/vhost types, and
other minor patching (e.g. qmp_x_debug_query_virtio uses QAPI_LIST_PREPEND rather
than open coding to iterate through list of virtio devices, see patch 1/6).

Also, added new features (since Qemu 6.1) to virtio-gpu, virtio-net, and
virtio-balloon. Lastly, added display for the virtio device name in a
few of the qmp/hmp commands as well as relative indicies for vrings 
(see patches 4/6, 6/6).]

1. Main command

HMP Only:

    virtio [subcommand]

    Example:

        List all sub-commands:

        (qemu) virtio
        virtio query -- List all available virtio devices
        virtio status path -- Display status of a given virtio device
        virtio queue-status path queue -- Display status of a given virtio queue
        virtio queue-element path queue [index] -- Display element of a given virtio queue

2. List available virtio deices in the machine

HMP Form:

    virtio query

    Example:

        (qemu) virtio query
        /machine/peripheral-anon/device[2]/virtio-backend [virtio-scsi]
        /machine/peripheral-anon/device[1]/virtio-backend [virtio-net]
        /machine/peripheral-anon/device[0]/virtio-backend [virtio-serial]

QMP Form:

    { 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }

    Example:

        -> { "execute": "x-debug-query-virtio" }
        <- { "return": [
                {
                    "path": "/machine/peripheral-anon/device[2]/virtio-backend",
                    "type": "virtio-scsi"
                },
                {
                    "path": "/machine/peripheral-anon/device[1]/virtio-backend",
                    "type": "virtio-net"
                },
                {
                    "path": "/machine/peripheral-anon/device[0]/virtio-backend",
                    "type": "virtio-serial"
                }
            ]
        }

3. Display status of a given virtio device

HMP Form:

    virtio status <path>

    Example:

        (qemu) virtio status /machine/peripheral-anon/device[2]/virtio-backend
        /machine/peripheral-anon/device[2]/virtio-backend:
            Device Id:          8
            Guest features:     event-idx, indirect-desc, version-1, change,
                                hotplug
            Host features:      event-idx, indirect-desc, bad-feature, version-1,
                                any-layout, notify-on-empty, change, hotplug
            Backend features:
            Endianness:         little
            VirtQueues:         4

QMP Form:

    { 'command': 'x-debug-virtio-status',
      'data': { 'path': 'str' },
      'returns': 'VirtioStatus'
    }

    Example:

        -> { "execute": "x-debug-virtio-status"
             "arguments": {
                "path": "/machine/peripheral-anon/device[2]/virtio-backend"
             }
           }
        <- { "return": {
                "device-endian": "little",
                "device-id": 8,
                "backend-features": {
                    "transport": [
                    ],
                    "type": "virtio-scsi",
                    "features": [
                    ]
                },
                "num-vqs": 4,
                "guest-features": {
                    "transport": [
                        "event-idx",
                        "indirect-desc",
                        "version-1"
                    ],
                    "type": "virtio-scsi",
                    "features": [
                        "change",
                        "hotplug"
                    ]
                },
                "host-features": {
                    "transport": [
                        "event-idx",
                        "indirect-desc",
                        "bad-feature",
                        "version-1",
                        "any-layout",
                        "notify-on-empty"
                    ],
                    "type": "virtio-scsi",
                    "features": [
                        "change",
                        "hotplug"
                    ]
                }
            }
        }

4. Display status of a given virtio queue

HMP Form:

    virtio queue-status <path> <queue>

    Example:

        (qemu) virtio queue-status /machine/peripheral-anon/device[2]/virtio-backend 3
        /machine/peripheral-anon/device[2]/virtio-backend:
            device_type:            virtio-scsi
            index:                  3
            inuse:                  0
            last_avail_idx:         4174 (78 % 256)
            shadow_avail_idx:       4174 (78 % 256)
            signalled_used:         4174 (78 % 256)
            signalled_used_valid:   1
            VRing:
                num:            256
                num_default:    256
                align:          4096
                desc:           0x000000003cf9e000
                avail:          0x000000003cf9f000
                used:           0x000000003cf9f240

QMP Form:

    { 'command': 'x-debug-virtio-queue-status',
      'data': { 'path': 'str', 'queue': 'uint16' },
      'returns': 'VirtQueueStatus'
    }

    Example:

        -> { "execute": "x-debug-virtio-queue-status",
            "arguments": {
                "path": "/machine/peripheral-anon/decice[2]/virtio-backend",
                "queue": 3
            }
           }
        <- { "return": {
                "signalled-used": 4188,
                "inuse": 0,
                "vring-align": 4096,
                "vring-desc": 1023008768,
                "signalled-used-valid": 1,
                "vring-num-default": 256,
                "vring-avail": 1023012864,
                "queue-index": 3,
                "last-avail-idx": 4188,
                "vring-used": 1023013440,
                "used-idx": 4188,
                "device-type": "virtio-scsi",
                "shadow-avail-idx": 4188
                "vring-num": 256
           }
          }

5. Display element of a given virtio queue

HMP Form:

    virtio queue-element <path> <queue> [index]

    Example:

        Dump the information of the head element of the third queue of virtio-scsi:

        (qemu) virtio queue-element /machine/peripheral-anon/device[3]/virtio-backend 3
        index: 122
        ndescs: 3
        descs: addr 0x7302d000 len 4096 (write), addr 0x3c951763 len 108 (write, next),
               addr 0x3c951728 len 51 (next)

        (qemu) xp/128x 0x7302d000
        000000007302d000: 0xce 0x14 0x56 0x77 0x78 0x7f 0x00 0x00
        000000007302d008: 0x05 0x00 0x00 0x00 0x00 0x00 0x00 0x00
        000000007302d010: 0xf9 0x00 0x00 0x00 0x00 0x00 0x00 0x00
        000000007302d018: 0x4f 0xf9 0x55 0x77 0x78 0x7f 0x00 0x00
        ...

QMP Form:

    { 'command': 'x-debug-virtio-queue-element',
      'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' },
      'returns': 'VirtioQueueElement'
    }

    Example:

        -> { "execute": "x-debug-virtio-queue-element",
             "arguments": {
                "path": "/machine/peripheral-anon/device[2]/virtio-backend",
                "queue": 0
             }
           }
        <- { "return": {
                "index": 122,
                "ndescs": 3,
                "descs": [
                    {
                        "flags": [
                            "write"
                        ],
                        "len": 4096,
                        "addr": 1929564160
                    },
                    {
                        "flags": [
                            "write",
                            "next"
                        ],
                        "len": 108,
                        "addr": 1016403811
                    },
                    {
                        "flags": [
                            "next"
                        ],
                        "len": 51,
                        "addr": 1016403752
                    }
                ]
            }
        }

[Jonah:
Comments:

One thing worth mentioning that this patch series adds is some maintenance
burden. More specifically, the following would need to be done if a new
virtio device type (with features) were to be added:

 - In the new */virtio-<device>.c: add #include "qapi/qapi-visit-virtio.h"
   and define a qmp_virtio_feature_map_t map structure with device feature
   entries (e.g. FEATURE_ENTRY(_FEATURE_))

 - In qapi/virtio.json: define a new enumeration of Virtio<Device>Feature
   (including its enumerated features) and define a new
   VirtioDeviceFeaturesOptions<Device> and add it to VirtioDeviceFeatures

 - In hw/virtio/virtio.c: add a case to switch statements in qmp_decode_features
   and hmp_virtio_dump_features

If you're just adding a new feature for an already existing virtio device:

 - In */virtio-<device>.c: add the new FEATURE_ENTRY(_FEATURE_) in the
   device's qmp_virtio_feature_map_t structure

 - In qapi/virtio.json: add the enumeration of the new virtio device
   feature in the corresponding Virtio<Device>Feature JSON structure

In the previous patch series (v4) there was a comment regarding the
implementation of the switch case in hmp_virtio_dump_features. It would
be nice to not need to explicitly define a case for each virtio device
type (with features) here, but it's not very clear (to me) on how this
could be achieved (and optimally we'd probably want something similar for
qmp_decode_features as well).

The suggestion to this problem (from last May) was to do something like:

    if (features->type < something_MAX) {
        features_str = anarray[features->type];
        ...
        monitor_printf(mon, "%s", features_str(list->value));
        ...
    }

But, (1): the device type enumerations were changed to "flat" enums in v4
and, as I understand it, flat enums have no value associated with them so
we wouldn't be able to use it to index anarray. And (2): not every virtio
device has features (e.g. virtio-9p, virtio-input, vhost-user-fs, etc.)
so we'd still need to take that into account and filter-out those devices
as well.

I've looked at it for awhile but, at least to me, it wasn't obvious how
this could be done.

Note: for patch 5/6, checkpatch.pl gives the following error:

   ERROR: "foo * bar" should be "foo *bar"
   #329: FILE: hw/virtio/virtio.c:4164
            type##FeatureList * list = features->u.field.features;

But doing both 'type##FeatureList * list = ...' and
'type##FeatureList *list = ...' give errors, so I just left it as the former
representation. ]

v6: rebased for upstream (Qemu 6.1)
    add all virtio/vhost types in same order as 
    include/standard-headers/linux/virtio_ids.h
    use QAPI_LIST_PREPEND in qmp_x_debug_query_virtio rather than open
    coding

v5: rebased for upstream
    add device name, used index, and relative indicies to virtio queue-status
    HMP command
    add device name to virtio queue-status QMP command
    add new virtio device features

v4: re-send series as v3 didn't reach qemu-devel

v3: use qapi_free_VirtioInfoList() on the head of the list, not on the tail.
    Prefix the QMP commands with 'x-debug-'

v2: introduce VirtioType enum
    use an enum for the endianness
    change field names to stick to naming convertions (s/_/-/)
    add a patch to decode feature bits
    don't check if the queue is empty to allow display of old elements
    use enum for desc flags
    manage indirect desc
    decode device features in the HMP command

Laurent Vivier (6):
  qmp: add QMP command x-debug-query-virtio
  qmp: add QMP command x-debug-virtio-status
  qmp: decode feature bits in virtio-status
  qmp: add QMP command x-debug-virtio-queue-status
  qmp: add QMP command x-debug-virtio-queue-element
  hmp: add virtio commands

 docs/system/monitor.rst      |   2 +
 hmp-commands-virtio.hx       | 162 ++++++++++++
 hmp-commands.hx              |  10 +
 hw/block/virtio-blk.c        |  23 ++
 hw/char/virtio-serial-bus.c  |  11 +
 hw/display/virtio-gpu-base.c |  12 +
 hw/net/virtio-net.c          |  38 +++
 hw/scsi/virtio-scsi.c        |  12 +
 hw/virtio/meson.build        |   2 +
 hw/virtio/virtio-balloon.c   |  14 +
 hw/virtio/virtio-iommu.c     |  14 +
 hw/virtio/virtio-stub.c      |  34 +++
 hw/virtio/virtio.c           | 574 ++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h   |  14 +
 include/monitor/hmp.h        |   4 +
 meson.build                  |   1 +
 monitor/misc.c               |  17 ++
 qapi/meson.build             |   1 +
 qapi/qapi-schema.json        |   1 +
 qapi/virtio.json             | 604 +++++++++++++++++++++++++++++++++++++++++++
 tests/qtest/qmp-cmd-test.c   |   1 +
 21 files changed, 1551 insertions(+)
 create mode 100644 hmp-commands-virtio.hx
 create mode 100644 hw/virtio/virtio-stub.c
 create mode 100644 qapi/virtio.json

-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
@ 2021-07-12 10:35 ` Jonah Palmer
  2021-08-07 12:35   ` Markus Armbruster
  2021-07-12 10:35 ` [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status Jonah Palmer
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

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>
---
 hw/virtio/meson.build      |  2 ++
 hw/virtio/virtio-stub.c    | 14 +++++++++
 hw/virtio/virtio.c         | 27 +++++++++++++++++
 include/hw/virtio/virtio.h |  1 +
 qapi/meson.build           |  1 +
 qapi/qapi-schema.json      |  1 +
 qapi/virtio.json           | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qtest/qmp-cmd-test.c |  1 +
 8 files changed, 119 insertions(+)
 create mode 100644 hw/virtio/virtio-stub.c
 create mode 100644 qapi/virtio.json

 [Jonah: Added all virtio/vhost types in same order from
 include/standard-headers/linux/virtio_ids.h to qapi/virtio.json
 so that the QAPI generated enum values for 'VirtioType' match
 the virtio_ids constants (to avoid conversion errors).

 Iterating through realized virtio devices in qmp_x_debug_query_virtio
 now uses QAPI_LIST_PREPEND rather than open coding.]

diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index fbff9bc..81dc4c9 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -6,8 +6,10 @@ softmmu_virtio_ss.add(when: 'CONFIG_VHOST', if_false: files('vhost-stub.c'))
 
 softmmu_ss.add_all(when: 'CONFIG_VIRTIO', if_true: softmmu_virtio_ss)
 softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('vhost-stub.c'))
+softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('virtio-stub.c'))
 
 softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c'))
+softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c'))
 
 virtio_ss = ss.source_set()
 virtio_ss.add(files('virtio.c'))
diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
new file mode 100644
index 0000000..d4a88f5
--- /dev/null
+++ b/hw/virtio/virtio-stub.c
@@ -0,0 +1,14 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-virtio.h"
+
+static void *qmp_virtio_unsupported(Error **errp)
+{
+    error_setg(errp, "Virtio is disabled");
+    return NULL;
+}
+
+VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
+{
+    return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 874377f..f3fc1bb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -13,6 +13,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-virtio.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "cpu.h"
 #include "trace.h"
 #include "qemu/error-report.h"
@@ -29,6 +31,8 @@
 #include "sysemu/runstate.h"
 #include "standard-headers/linux/virtio_ids.h"
 
+static QTAILQ_HEAD(, VirtIODevice) virtio_list;
+
 /*
  * The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. This is the default, used by transports like PCI
@@ -3672,6 +3676,7 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
 
     vdev->listener.commit = virtio_memory_listener_commit;
     memory_listener_register(&vdev->listener, vdev->dma_as);
+    QTAILQ_INSERT_TAIL(&virtio_list, vdev, next);
 }
 
 static void virtio_device_unrealize(DeviceState *dev)
@@ -3686,6 +3691,7 @@ static void virtio_device_unrealize(DeviceState *dev)
         vdc->unrealize(dev);
     }
 
+    QTAILQ_REMOVE(&virtio_list, vdev, next);
     g_free(vdev->bus_name);
     vdev->bus_name = NULL;
 }
@@ -3859,6 +3865,8 @@ static void virtio_device_class_init(ObjectClass *klass, void *data)
     vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
 
     vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
+
+    QTAILQ_INIT(&virtio_list);
 }
 
 bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
@@ -3869,6 +3877,25 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
     return virtio_bus_ioeventfd_enabled(vbus);
 }
 
+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;
+}
+
 static const TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 8bab9cf..f7da326 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -108,6 +108,7 @@ struct VirtIODevice
     bool use_guest_notifier_mask;
     AddressSpace *dma_as;
     QLIST_HEAD(, VirtQueue) *vector_queues;
+    QTAILQ_ENTRY(VirtIODevice) next;
 };
 
 struct VirtioDeviceClass {
diff --git a/qapi/meson.build b/qapi/meson.build
index 376f4ce..4e1da08 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -44,6 +44,7 @@ qapi_all_modules = [
   'sockets',
   'trace',
   'transaction',
+  'virtio',
   'yank',
 ]
 if have_system
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 @@
+##
+# = Virtio devices
+##
+
+##
+# @VirtioType:
+#
+# An enumeration of Virtio device types.
+#
+# Since: 6.1
+##
+{ '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' ]
+}
+
+##
+# @VirtioInfo:
+#
+# Information about a given VirtIODevice
+#
+# @path: VirtIO device canonical path.
+#
+# @type: VirtIO device type.
+#
+# Since: 6.1
+#
+##
+{ 'struct': 'VirtioInfo',
+  'data': {
+    'path': 'str',
+    'type': 'VirtioType'
+  }
+}
+
+##
+# @x-debug-query-virtio:
+#
+# Return the list of all VirtIO devices
+#
+# Returns: list of @VirtioInfo
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "x-debug-query-virtio" }
+# <- { "return": [
+#        {
+#            "path": "/machine/peripheral-anon/device[3]/virtio-backend",
+#            "type": "virtio-net"
+#        },
+#        {
+#            "path": "/machine/peripheral-anon/device[1]/virtio-backend",
+#            "type": "virtio-serial"
+#        },
+#        {
+#            "path": "/machine/peripheral-anon/device[0]/virtio-backend",
+#            "type": "virtio-blk"
+#        }
+#      ]
+#    }
+#
+##
+
+{ 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d..b2cf0628 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -95,6 +95,7 @@ static bool query_is_ignored(const char *cmd)
         "query-gic-capabilities", /* arm */
         /* Success depends on target-specific build configuration: */
         "query-pci",              /* CONFIG_PCI */
+        "x-debug-query-virtio",   /* CONFIG_VIRTIO */
         /* Success depends on launching SEV guest */
         "query-sev-launch-measure",
         /* Success depends on Host or Hypervisor SEV support */
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
  2021-07-12 10:35 ` [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio Jonah Palmer
@ 2021-07-12 10:35 ` Jonah Palmer
  2021-08-07 12:42   ` Markus Armbruster
  2021-09-03  8:26   ` Michael S. Tsirkin
  2021-07-12 10:35 ` [PATCH v6 3/6] qmp: decode feature bits in virtio-status Jonah Palmer
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

From: Laurent Vivier <lvivier@redhat.com>

This new command shows the status of a VirtIODevice
(features, endianness and number of virtqueues)

Next patch will improve output by decoding feature bits.

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

diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
index d4a88f5..ddb592f 100644
--- a/hw/virtio/virtio-stub.c
+++ b/hw/virtio/virtio-stub.c
@@ -12,3 +12,8 @@ VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
 {
     return qmp_virtio_unsupported(errp);
 }
+
+VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
+{
+    return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f3fc1bb..222330a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3896,6 +3896,56 @@ VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
     return list;
 }
 
+static VirtIODevice *virtio_device_find(const char *path)
+{
+    VirtIODevice *vdev;
+
+    QTAILQ_FOREACH(vdev, &virtio_list, next) {
+        DeviceState *dev = DEVICE(vdev);
+
+        if (strcmp(dev->canonical_path, path) != 0) {
+            continue;
+        }
+        return vdev;
+    }
+
+    return NULL;
+}
+
+VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
+{
+    VirtIODevice *vdev;
+    VirtioStatus *status;
+
+    vdev = virtio_device_find(path);
+    if (vdev == NULL) {
+        error_setg(errp, "Path %s is not a VirtIO device", path);
+        return NULL;
+    }
+
+    status = g_new0(VirtioStatus, 1);
+    status->guest_features = vdev->guest_features;
+    status->host_features = vdev->host_features;
+    status->backend_features = vdev->backend_features;
+    status->device_id = vdev->device_id;
+
+    switch (vdev->device_endian) {
+    case VIRTIO_DEVICE_ENDIAN_LITTLE:
+        status->device_endian = VIRTIO_STATUS_ENDIANNESS_LITTLE;
+        break;
+    case VIRTIO_DEVICE_ENDIAN_BIG:
+        status->device_endian = VIRTIO_STATUS_ENDIANNESS_BIG;
+        break;
+    default:
+        status->device_endian = VIRTIO_STATUS_ENDIANNESS_UNKNOWN;
+        break;
+    }
+
+    status->num_vqs = virtio_get_num_queues(vdev);
+
+    return status;
+}
+
 static const TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 804adbe..4bd09c9 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -70,3 +70,79 @@
 ##
 
 { 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
+
+##
+# @VirtioStatusEndianness:
+#
+# Enumeration of endianness for VirtioDevice
+#
+# Since: 6.1
+##
+{ 'enum': 'VirtioStatusEndianness',
+  'data': [ 'unknown', 'little', 'big' ]
+}
+
+##
+# @VirtioStatus:
+#
+# @device-id: VirtIODevice status
+#
+# @device-endian: VirtIODevice device_endian
+#
+# @guest-features: VirtIODevice guest_features
+#
+# @host-features: VirtIODevice host_features
+#
+# @backend-features: VirtIODevice backend_features
+#
+# @num-vqs: number of VirtIODevice queues
+#
+# Since: 6.1
+#
+##
+
+{ 'struct': 'VirtioStatus',
+  'data': {
+    'device-id': 'int',
+    'device-endian': 'VirtioStatusEndianness',
+    'guest-features': 'uint64',
+    'host-features': 'uint64',
+    'backend-features': 'uint64',
+    'num-vqs': 'uint16'
+  }
+}
+
+##
+# @x-debug-virtio-status:
+#
+# Return the status of virtio device
+#
+# @path: QOBject path of the VirtIODevice
+#
+# Returns: status of the VirtIODevice
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "x-debug-virtio-status",
+#      "arguments": {
+#          "path": "/machine/peripheral-anon/device[3]/virtio-backend"
+#      }
+#   }
+# <- { "return": {
+#          "backend-features": 0,
+#          "guest-features": 5111807911,
+#          "num-vqs": 3,
+#          "host-features": 6337593319,
+#          "device-endian": "little",
+#          "device-id": 1
+#      }
+#    }
+#
+##
+
+{ 'command': 'x-debug-virtio-status',
+  'data': { 'path': 'str' },
+  'returns': 'VirtioStatus'
+}
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v6 3/6] qmp: decode feature bits in virtio-status
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
  2021-07-12 10:35 ` [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio Jonah Palmer
  2021-07-12 10:35 ` [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status Jonah Palmer
@ 2021-07-12 10:35 ` Jonah Palmer
  2021-07-12 10:35 ` [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status Jonah Palmer
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

From: Laurent Vivier <lvivier@redhat.com>

Display feature names instead of a features bitmap for host, guest,
and backend.

Decode features according to 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>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/block/virtio-blk.c        |  23 ++++
 hw/char/virtio-serial-bus.c  |  11 ++
 hw/display/virtio-gpu-base.c |  12 ++
 hw/net/virtio-net.c          |  38 ++++++
 hw/scsi/virtio-scsi.c        |  12 ++
 hw/virtio/virtio-balloon.c   |  14 +++
 hw/virtio/virtio-iommu.c     |  14 +++
 hw/virtio/virtio.c           | 129 +++++++++++++++++++-
 include/hw/virtio/virtio.h   |  13 ++
 qapi/virtio.json             | 280 +++++++++++++++++++++++++++++++++++++++++--
 10 files changed, 533 insertions(+), 13 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f139cd7..e66c5eb 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "qemu/iov.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
@@ -48,6 +49,28 @@ static const VirtIOFeature feature_sizes[] = {
     {}
 };
 
+qmp_virtio_feature_map_t blk_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_BLK_F_##name, VIRTIO_BLK_FEATURE_##name }
+    FEATURE_ENTRY(SIZE_MAX),
+    FEATURE_ENTRY(SEG_MAX),
+    FEATURE_ENTRY(GEOMETRY),
+    FEATURE_ENTRY(RO),
+    FEATURE_ENTRY(BLK_SIZE),
+    FEATURE_ENTRY(TOPOLOGY),
+    FEATURE_ENTRY(MQ),
+    FEATURE_ENTRY(DISCARD),
+    FEATURE_ENTRY(WRITE_ZEROES),
+#ifndef VIRTIO_BLK_NO_LEGACY
+    FEATURE_ENTRY(BARRIER),
+    FEATURE_ENTRY(SCSI),
+    FEATURE_ENTRY(FLUSH),
+    FEATURE_ENTRY(CONFIG_WCE),
+#endif /* !VIRTIO_BLK_NO_LEGACY */
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
 {
     s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index dd6bc27..4a626e6 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "qemu/iov.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
@@ -32,6 +33,16 @@
 #include "hw/virtio/virtio-serial.h"
 #include "hw/virtio/virtio-access.h"
 
+qmp_virtio_feature_map_t serial_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_CONSOLE_F_##name, VIRTIO_SERIAL_FEATURE_##name }
+    FEATURE_ENTRY(SIZE),
+    FEATURE_ENTRY(MULTIPORT),
+    FEATURE_ENTRY(EMERG_WRITE),
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 static struct VirtIOSerialDevices {
     QLIST_HEAD(, VirtIOSerial) devices;
 } vserdevices;
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index dd29427..34c1ead 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -16,9 +16,21 @@
 #include "hw/virtio/virtio-gpu.h"
 #include "migration/blocker.h"
 #include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 
+qmp_virtio_feature_map_t gpu_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_GPU_F_##name, VIRTIO_GPU_FEATURE_##name }
+    FEATURE_ENTRY(VIRGL),
+    FEATURE_ENTRY(EDID),
+    FEATURE_ENTRY(RESOURCE_UUID),
+    FEATURE_ENTRY(RESOURCE_BLOB),
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 void
 virtio_gpu_base_reset(VirtIOGPUBase *g)
 {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cd..0a05fe6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -35,6 +35,7 @@
 #include "hw/qdev-properties.h"
 #include "qapi/qapi-types-migration.h"
 #include "qapi/qapi-events-migration.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "hw/virtio/virtio-access.h"
 #include "migration/misc.h"
 #include "standard-headers/linux/ethtool.h"
@@ -89,6 +90,43 @@
                                          VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \
                                          VIRTIO_NET_RSS_HASH_TYPE_UDP_EX)
 
+qmp_virtio_feature_map_t net_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_NET_F_##name, VIRTIO_NET_FEATURE_##name }
+    FEATURE_ENTRY(CSUM),
+    FEATURE_ENTRY(GUEST_CSUM),
+    FEATURE_ENTRY(CTRL_GUEST_OFFLOADS),
+    FEATURE_ENTRY(MTU),
+    FEATURE_ENTRY(MAC),
+    FEATURE_ENTRY(GUEST_TSO4),
+    FEATURE_ENTRY(GUEST_TSO6),
+    FEATURE_ENTRY(GUEST_ECN),
+    FEATURE_ENTRY(GUEST_UFO),
+    FEATURE_ENTRY(HOST_TSO4),
+    FEATURE_ENTRY(HOST_TSO6),
+    FEATURE_ENTRY(HOST_ECN),
+    FEATURE_ENTRY(HOST_UFO),
+    FEATURE_ENTRY(MRG_RXBUF),
+    FEATURE_ENTRY(STATUS),
+    FEATURE_ENTRY(CTRL_VQ),
+    FEATURE_ENTRY(CTRL_RX),
+    FEATURE_ENTRY(CTRL_VLAN),
+    FEATURE_ENTRY(CTRL_RX_EXTRA),
+    FEATURE_ENTRY(GUEST_ANNOUNCE),
+    FEATURE_ENTRY(MQ),
+    FEATURE_ENTRY(CTRL_MAC_ADDR),
+    FEATURE_ENTRY(HASH_REPORT),
+    FEATURE_ENTRY(RSS),
+    FEATURE_ENTRY(RSC_EXT),
+    FEATURE_ENTRY(STANDBY),
+    FEATURE_ENTRY(SPEED_DUPLEX),
+#ifndef VIRTIO_NET_NO_LEGACY
+    FEATURE_ENTRY(GSO),
+#endif /* VIRTIO_NET_NO_LEGACY */
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 static const VirtIOFeature feature_sizes[] = {
     {.flags = 1ULL << VIRTIO_NET_F_MAC,
      .end = endof(struct virtio_net_config, mac)},
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 6d80730..8187dc3 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -15,6 +15,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "hw/virtio/virtio-scsi.h"
 #include "migration/qemu-file-types.h"
@@ -29,6 +30,17 @@
 #include "hw/virtio/virtio-access.h"
 #include "trace.h"
 
+qmp_virtio_feature_map_t scsi_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_SCSI_F_##name, VIRTIO_SCSI_FEATURE_##name }
+    FEATURE_ENTRY(INOUT),
+    FEATURE_ENTRY(HOTPLUG),
+    FEATURE_ENTRY(CHANGE),
+    FEATURE_ENTRY(T10_PI),
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 static inline int virtio_scsi_get_lun(uint8_t *lun)
 {
     return ((lun[2] << 8) | lun[3]) & 0x3FFF;
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 4b5d9e5..4bc05de 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -27,6 +27,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-events-machine.h"
 #include "qapi/visitor.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "migration/misc.h"
@@ -34,6 +35,19 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 
+qmp_virtio_feature_map_t balloon_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_BALLOON_F_##name, VIRTIO_BALLOON_FEATURE_##name }
+    FEATURE_ENTRY(MUST_TELL_HOST),
+    FEATURE_ENTRY(STATS_VQ),
+    FEATURE_ENTRY(DEFLATE_ON_OOM),
+    FEATURE_ENTRY(FREE_PAGE_HINT),
+    FEATURE_ENTRY(PAGE_POISON),
+    FEATURE_ENTRY(REPORTING),
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
 
 typedef struct PartiallyBalloonedPage {
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 1b23e8e..6deddcb 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -25,6 +25,7 @@
 #include "hw/virtio/virtio.h"
 #include "sysemu/kvm.h"
 #include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 
@@ -36,6 +37,19 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci.h"
 
+qmp_virtio_feature_map_t iommu_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_IOMMU_F_##name, VIRTIO_IOMMU_FEATURE_##name }
+    FEATURE_ENTRY(INPUT_RANGE),
+    FEATURE_ENTRY(DOMAIN_RANGE),
+    FEATURE_ENTRY(MAP_UNMAP),
+    FEATURE_ENTRY(BYPASS),
+    FEATURE_ENTRY(PROBE),
+    FEATURE_ENTRY(MMIO),
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 /* Max size */
 #define VIOMMU_DEFAULT_QUEUE_SIZE 256
 #define VIOMMU_PROBE_SIZE 512
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 222330a..81a0ee8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -30,9 +30,32 @@
 #include "sysemu/dma.h"
 #include "sysemu/runstate.h"
 #include "standard-headers/linux/virtio_ids.h"
+#include CONFIG_DEVICES
 
 static QTAILQ_HEAD(, VirtIODevice) virtio_list;
 
+static qmp_virtio_feature_map_t transport_map[] = {
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_F_##name, VIRTIO_TRANSPORT_FEATURE_##name }
+#ifndef VIRTIO_CONFIG_NO_LEGACY
+    FEATURE_ENTRY(NOTIFY_ON_EMPTY),
+    FEATURE_ENTRY(ANY_LAYOUT),
+#endif /* VIRTIO_CONFIG_NO_LEGACY */
+    FEATURE_ENTRY(VERSION_1),
+    FEATURE_ENTRY(IOMMU_PLATFORM),
+    FEATURE_ENTRY(RING_PACKED),
+    FEATURE_ENTRY(ORDER_PLATFORM),
+    FEATURE_ENTRY(SR_IOV),
+    FEATURE_ENTRY(BAD_FEATURE),
+#undef FEATURE_ENTRY
+#define FEATURE_ENTRY(name) \
+    { VIRTIO_RING_F_##name, VIRTIO_TRANSPORT_FEATURE_##name }
+    FEATURE_ENTRY(INDIRECT_DESC),
+    FEATURE_ENTRY(EVENT_IDX),
+#undef FEATURE_ENTRY
+    { -1, -1 }
+};
+
 /*
  * The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. This is the default, used by transports like PCI
@@ -3912,6 +3935,103 @@ static VirtIODevice *virtio_device_find(const char *path)
     return NULL;
 }
 
+#define CONVERT_FEATURES(type, map)                \
+    ({                                           \
+        type *list = NULL;                         \
+        type *node;                                \
+        for (i = 0; map[i].virtio_bit != -1; i++) {\
+            bit = 1ULL << map[i].virtio_bit;       \
+            if ((bitmap & bit) == 0) {             \
+                continue;                          \
+            }                                      \
+            node = g_new0(type, 1);                \
+            node->value = map[i].qapi_virtio_enum; \
+            node->next = list;                     \
+            list = node;                           \
+            bitmap ^= bit;                         \
+        }                                          \
+        list;                                      \
+    })
+
+static VirtioDeviceFeatures *qmp_decode_features(const char *name,
+                                                 uint64_t bitmap)
+{
+    VirtioDeviceFeatures *features;
+    uint64_t bit;
+    int i;
+
+    features = g_new0(VirtioDeviceFeatures, 1);
+
+    /* transport features */
+    features->transport = CONVERT_FEATURES(VirtioTransportFeatureList, \
+                                           transport_map);
+
+    /* device features */
+    features->type = qapi_enum_parse(&VirtioType_lookup,
+                                     name, -1, NULL);
+    switch (features->type) {
+#ifdef CONFIG_VIRTIO_SERIAL
+    case VIRTIO_TYPE_VIRTIO_SERIAL:
+        features->u.virtio_serial.features =
+                          CONVERT_FEATURES(VirtioSerialFeatureList, serial_map);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_BLK
+    case VIRTIO_TYPE_VIRTIO_BLK:
+        features->u.virtio_blk.features =
+                                CONVERT_FEATURES(VirtioBlkFeatureList, blk_map);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_GPU
+    case VIRTIO_TYPE_VIRTIO_GPU:
+        features->u.virtio_gpu.features =
+                                CONVERT_FEATURES(VirtioGpuFeatureList, gpu_map);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_NET
+    case VIRTIO_TYPE_VIRTIO_NET:
+        features->u.virtio_net.features =
+                                CONVERT_FEATURES(VirtioNetFeatureList, net_map);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_SCSI
+    case VIRTIO_TYPE_VIRTIO_SCSI:
+        features->u.virtio_scsi.features =
+                              CONVERT_FEATURES(VirtioScsiFeatureList, scsi_map);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_BALLOON
+    case VIRTIO_TYPE_VIRTIO_BALLOON:
+        features->u.virtio_balloon.features =
+                        CONVERT_FEATURES(VirtioBalloonFeatureList, balloon_map);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_IOMMU
+    case VIRTIO_TYPE_VIRTIO_IOMMU:
+        features->u.virtio_iommu.features =
+                            CONVERT_FEATURES(VirtioIommuFeatureList, iommu_map);
+        break;
+#endif
+    /* No features */
+    case VIRTIO_TYPE_VIRTIO_9P:
+    case VIRTIO_TYPE_VIRTIO_INPUT:
+    case VIRTIO_TYPE_VHOST_USER_FS:
+    case VIRTIO_TYPE_VHOST_VSOCK:
+    case VIRTIO_TYPE_VIRTIO_CRYPTO:
+    case VIRTIO_TYPE_VIRTIO_PMEM:
+    case VIRTIO_TYPE_VIRTIO_RNG:
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    features->has_unknown_features = bitmap != 0;
+    if (features->has_unknown_features) {
+        features->unknown_features = bitmap;
+    }
+
+    return features;
+}
+
 VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
 {
     VirtIODevice *vdev;
@@ -3924,9 +4044,12 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
     }
 
     status = g_new0(VirtioStatus, 1);
-    status->guest_features = vdev->guest_features;
-    status->host_features = vdev->host_features;
-    status->backend_features = vdev->backend_features;
+    status->guest_features = qmp_decode_features(vdev->name,
+                                                 vdev->guest_features);
+    status->host_features = qmp_decode_features(vdev->name,
+                                                vdev->host_features);
+    status->backend_features = qmp_decode_features(vdev->name,
+                                                   vdev->backend_features);
     status->device_id = vdev->device_id;
 
     switch (vdev->device_endian) {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f7da326..b872dcf 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -163,6 +163,19 @@ struct VirtioDeviceClass {
     bool (*primary_unplug_pending)(void *opaque);
 };
 
+typedef struct {
+    int virtio_bit;
+    int qapi_virtio_enum;
+} qmp_virtio_feature_map_t;
+
+extern qmp_virtio_feature_map_t serial_map[];
+extern qmp_virtio_feature_map_t blk_map[];
+extern qmp_virtio_feature_map_t gpu_map[];
+extern qmp_virtio_feature_map_t net_map[];
+extern qmp_virtio_feature_map_t scsi_map[];
+extern qmp_virtio_feature_map_t balloon_map[];
+extern qmp_virtio_feature_map_t iommu_map[];
+
 void virtio_instance_init_common(Object *proxy_obj, void *data,
                                  size_t vdev_size, const char *vdev_name);
 
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 4bd09c9..78873cd 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -83,6 +83,253 @@
 }
 
 ##
+# @VirtioTransportFeature:
+#
+# An enumeration of Virtio device transport features, including virtio-ring
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioTransportFeature',
+  'data': [ 'notify-on-empty', 'any-layout', 'version-1', 'iommu-platform',
+            'ring-packed', 'order-platform', 'sr-iov', 'indirect-desc',
+            'event-idx', 'bad-feature' ]
+}
+
+##
+# @VirtioSerialFeature:
+#
+# An enumeration of Virtio serial features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioSerialFeature',
+  'data': [ 'size', 'multiport', 'emerg-write' ]
+}
+
+##
+# @VirtioBlkFeature:
+#
+# An enumeration of Virtio block features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioBlkFeature',
+  'data': [ 'size-max', 'seg-max', 'geometry', 'ro', 'blk-size', 'topology',
+            'mq', 'discard', 'write-zeroes', 'barrier', 'scsi', 'flush',
+            'config-wce' ]
+}
+
+##
+# @VirtioGpuFeature:
+#
+# An enumeration of Virtio gpu features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioGpuFeature',
+  'data': [ 'virgl', 'edid', 'resource-uuid', 'resource-blob' ]
+}
+
+##
+# @VirtioNetFeature:
+#
+# An enumeration of Virtio net features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioNetFeature',
+  'data': [ 'csum', 'guest-csum', 'ctrl-guest-offloads', 'mtu', 'mac',
+            'guest-tso4', 'guest-tso6', 'guest-ecn', 'guest-ufo',
+            'host-tso4', 'host-tso6', 'host-ecn', 'host-ufo', 'mrg-rxbuf',
+            'status', 'ctrl-vq', 'ctrl-rx', 'ctrl-vlan', 'ctrl-rx-extra',
+            'guest-announce', 'mq', 'ctrl-mac-addr', 'standby',
+            'speed-duplex', 'gso', 'hash-report', 'rss', 'rsc-ext' ]
+}
+
+##
+# @VirtioScsiFeature:
+#
+# An enumeration of Virtio scsi features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioScsiFeature',
+  'data': [ 'inout', 'hotplug', 'change', 't10-pi' ]
+}
+
+##
+# @VirtioBalloonFeature:
+#
+# An enumeration of Virtio balloon features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioBalloonFeature',
+  'data': [ 'must-tell-host', 'stats-vq', 'deflate-on-oom', 'free-page-hint',
+            'page-poison', 'reporting' ]
+}
+
+##
+# @VirtioIommuFeature:
+#
+# An enumeration of Virtio iommu features
+#
+# Since: 6.1
+##
+
+{ 'enum': 'VirtioIommuFeature',
+  'data': [ 'input-range', 'domain-range', 'map-unmap', 'bypass', 'probe',
+            'mmio' ]
+}
+
+##
+# @VirtioDeviceFeaturesBase:
+#
+# The common fields that apply to all Virtio devices
+#
+# @type: virtio device type
+# @transport: the list of transport features of the virtio device
+# @unknown-features: virtio device features bitmap that have not been decoded
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesBase',
+  'data': {
+    'type': 'VirtioType',
+    'transport': [ 'VirtioTransportFeature' ],
+    '*unknown-features': 'uint64'
+  }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsSerial:
+#
+# The options that apply to Virtio serial device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsSerial',
+  'data': { 'features': [ 'VirtioSerialFeature' ] }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsBlk:
+#
+# The options that apply to Virtio block device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsBlk',
+  'data': { 'features': [ 'VirtioBlkFeature' ] }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsGpu:
+#
+# The options that apply to Virtio GPU device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsGpu',
+  'data': { 'features': [ 'VirtioGpuFeature' ] }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsNet:
+#
+# The options that apply to Virtio net device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsNet',
+  'data': { 'features': [ 'VirtioNetFeature' ] }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsScsi:
+#
+# The options that apply to Virtio SCSI device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsScsi',
+  'data': { 'features': [ 'VirtioScsiFeature' ] }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsBalloon:
+#
+# The options that apply to Virtio balloon device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsBalloon',
+  'data': { 'features': [ 'VirtioBalloonFeature' ] }
+}
+
+##
+# @VirtioDeviceFeaturesOptionsIommu:
+#
+# The options that apply to Virtio IOMMU device
+#
+# @features: List of the device features
+#
+# Since: 6.1
+##
+
+{ 'struct': 'VirtioDeviceFeaturesOptionsIommu',
+  'data': { 'features': [ 'VirtioIommuFeature' ] }
+}
+
+##
+# @VirtioDeviceFeatures:
+#
+# A union to define the list of features for a virtio device
+#
+# Since: 6.1
+##
+
+{ 'union': 'VirtioDeviceFeatures',
+  'base': 'VirtioDeviceFeaturesBase',
+  'discriminator': 'type',
+  'data': {
+    'virtio-serial': 'VirtioDeviceFeaturesOptionsSerial',
+    'virtio-blk': 'VirtioDeviceFeaturesOptionsBlk',
+    'virtio-gpu': 'VirtioDeviceFeaturesOptionsGpu',
+    'virtio-net': 'VirtioDeviceFeaturesOptionsNet',
+    'virtio-scsi': 'VirtioDeviceFeaturesOptionsScsi',
+    'virtio-balloon': 'VirtioDeviceFeaturesOptionsBalloon',
+    'virtio-iommu': 'VirtioDeviceFeaturesOptionsIommu'
+  }
+}
+
+##
 # @VirtioStatus:
 #
 # @device-id: VirtIODevice status
@@ -105,9 +352,9 @@
   'data': {
     'device-id': 'int',
     'device-endian': 'VirtioStatusEndianness',
-    'guest-features': 'uint64',
-    'host-features': 'uint64',
-    'backend-features': 'uint64',
+    'guest-features': 'VirtioDeviceFeatures',
+    'host-features': 'VirtioDeviceFeatures',
+    'backend-features': 'VirtioDeviceFeatures',
     'num-vqs': 'uint16'
   }
 }
@@ -127,18 +374,31 @@
 #
 # -> { "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": {
+#              "transport": [],
+#              "type": "virtio-serial",
+#              "features": []
+#          },
+#          "num-vqs": 64,
+#          "guest-features": {
+#              "transport": [ "event-idx", "indirect-desc", "version-1" ],
+#              "type": "virtio-serial",
+#              "features": [ "multiport" ]
+#          },
+#          "host-features": {
+#              "transport": [ "event-idx", "indirect-desc", "bad-feature",
+#                             "version-1", "any-layout", "notify-on-empty" ],
+#              "type": "virtio-serial",
+#              "features": [ "emerg-write", "multiport" ]
+#          }
 #      }
-#    }
+#  }
 #
 ##
 
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
                   ` (2 preceding siblings ...)
  2021-07-12 10:35 ` [PATCH v6 3/6] qmp: decode feature bits in virtio-status Jonah Palmer
@ 2021-07-12 10:35 ` Jonah Palmer
  2021-07-14  2:37   ` Jason Wang
  2021-08-07 12:45   ` Markus Armbruster
  2021-07-12 10:35 ` [PATCH v6 5/6] qmp: add QMP command x-debug-virtio-queue-element Jonah Palmer
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

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;
+    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 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'
+}
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v6 5/6] qmp: add QMP command x-debug-virtio-queue-element
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
                   ` (3 preceding siblings ...)
  2021-07-12 10:35 ` [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status Jonah Palmer
@ 2021-07-12 10:35 ` Jonah Palmer
  2021-07-12 10:35 ` [PATCH v6 6/6] hmp: add virtio commands Jonah Palmer
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

From: Laurent Vivier <lvivier@redhat.com>

This new command shows the information of a VirtQueue element.

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

diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
index 3c1bf17..8275e31 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 ccd4371..bd4d13d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4106,6 +4106,141 @@ 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;
+            }
+            max = desc.len / sizeof(VRingDesc);
+            i = 0;
+            vring_split_desc_read(vdev, &desc, desc_cache, i);
+        }
+
+        element = g_new0(VirtioQueueElement, 1);
+        element->index = head;
+        element->ndescs = 0;
+
+        do {
+            /* A buggy driver may produce an infinite loop */
+            if (element->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;
+
+            element->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 7007e0c..6bd9524 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -508,3 +508,97 @@
   'data': { 'path': 'str', 'queue': 'uint16' },
   'returns': 'VirtQueueStatus'
 }
+
+##
+# @VirtioRingDescFlags:
+#
+# An enumeration of the virtio ring descriptor flags
+#
+# Since: 6.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: 6.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: 6.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: 6.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'
+}
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v6 6/6] hmp: add virtio commands
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
                   ` (4 preceding siblings ...)
  2021-07-12 10:35 ` [PATCH v6 5/6] qmp: add QMP command x-debug-virtio-queue-element Jonah Palmer
@ 2021-07-12 10:35 ` Jonah Palmer
  2021-07-14  2:40   ` Jason Wang
  2021-07-13 15:25 ` [PATCH v6 0/6] hmp,qmp: Add some commands to introspect virtio devices Michael S. Tsirkin
  2021-07-14  2:42 ` [PATCH v6 0/6] hmp, qmp: " Jason Wang
  7 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-07-12 10:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, mst, jasowang, kraxel, si-wei.liu, joao.m.martins,
	qemu-block, david, armbru, marcandre.lureau, jonah.palmer, thuth,
	amit, michael.roth, dgilbert, eric.auger, dmitrii.stepanov,
	stefanha, kwolf, laurent, mreitz, pbonzini

From: Laurent Vivier <lvivier@redhat.com>

This patch implements HMP version of the virtio QMP commands

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 docs/system/monitor.rst |   2 +
 hmp-commands-virtio.hx  | 162 ++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx         |  10 +++
 hw/virtio/virtio.c      | 204 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/monitor/hmp.h   |   4 +
 meson.build             |   1 +
 monitor/misc.c          |  17 ++++
 7 files changed, 399 insertions(+), 1 deletion(-)
 create mode 100644 hmp-commands-virtio.hx

 [Jonah: Added relative indicies, device type, and used index
 in HMP virtio queue-status command.]

diff --git a/docs/system/monitor.rst b/docs/system/monitor.rst
index ff5c434..10418fc 100644
--- a/docs/system/monitor.rst
+++ b/docs/system/monitor.rst
@@ -21,6 +21,8 @@ The following commands are available:
 
 .. hxtool-doc:: hmp-commands.hx
 
+.. hxtool-doc:: hmp-commands-virtio.hx
+
 .. hxtool-doc:: hmp-commands-info.hx
 
 Integer expressions
diff --git a/hmp-commands-virtio.hx b/hmp-commands-virtio.hx
new file mode 100644
index 0000000..0ef01b9
--- /dev/null
+++ b/hmp-commands-virtio.hx
@@ -0,0 +1,162 @@
+HXCOMM Use DEFHEADING() to define headings in both help text and rST.
+HXCOMM Text between SRST and ERST is copied to the rST version and
+HXCOMM discarded from C version.
+HXCOMM DEF(command, args, callback, arg_string, help) is used to construct
+HXCOMM monitor info commands
+HXCOMM HXCOMM can be used for comments, discarded from both rST and C.
+HXCOMM
+HXCOMM In this file, generally SRST fragments should have two extra
+HXCOMM spaces of indent, so that the documentation list item for "virtio cmd"
+HXCOMM appears inside the documentation list item for the top level
+HXCOMM "virtio" documentation entry. The exception is the first SRST
+HXCOMM fragment that defines that top level entry.
+
+SRST
+``virtio`` *subcommand*
+  Show various information about virtio.
+
+  Example:
+
+  List all sub-commands::
+
+    (qemu) virtio
+    virtio query  -- List all available virtio devices
+    virtio status path -- Display status of a given virtio device
+    virtio queue-status path queue -- Display status of a given virtio queue
+    virtio queue-element path queue [index] -- Display element of a given virtio queue
+
+ERST
+
+    {
+        .name       = "query",
+        .args_type  = "",
+        .params     = "",
+        .help       = "List all available virtio devices",
+        .cmd        = hmp_virtio_query,
+        .flags      = "p",
+    },
+
+SRST
+  ``virtio query``
+    List all available virtio devices
+
+    Example:
+
+    List all available virtio devices in the machine::
+
+      (qemu) virtio query
+      /machine/peripheral-anon/device[3]/virtio-backend [virtio-net]
+      /machine/peripheral-anon/device[1]/virtio-backend [virtio-serial]
+      /machine/peripheral-anon/device[0]/virtio-backend [virtio-blk]
+
+ERST
+
+    {
+        .name       = "status",
+        .args_type  = "path:s",
+        .params     = "path",
+        .help       = "Display status of a given virtio device",
+        .cmd        = hmp_virtio_status,
+        .flags      = "p",
+    },
+
+SRST
+  ``virtio status`` *path*
+    Display status of a given virtio device
+
+    Example:
+
+    Dump the status of the first virtio device::
+
+      (qemu) virtio status /machine/peripheral-anon/device[3]/virtio-backend
+      /machine/peripheral-anon/device[3]/virtio-backend:
+        Device Id:        1
+        Guest features:   event-idx, indirect-desc, version-1
+                          ctrl-mac-addr, guest-announce, ctrl-vlan, ctrl-rx, ctrl-vq, status, mrg-rxbuf, host-ufo, host-ecn, host-tso6, host-tso4, guest-ufo, guest-ecn, guest-tso6, guest-tso4, mac, ctrl-guest-offloads, guest-csum, csum
+        Host features:    event-idx, indirect-desc, bad-feature, version-1, any-layout, notify-on-empty
+                          gso, ctrl-mac-addr, guest-announce, ctrl-rx-extra, ctrl-vlan, ctrl-rx, ctrl-vq, status, mrg-rxbuf, host-ufo, host-ecn, host-tso6, host-tso4, guest-ufo, guest-ecn, guest-tso6, guest-tso4, mac, ctrl-guest-offloads, guest-csum, csum
+        Backend features:
+        Endianness:       little
+        VirtQueues:       3
+
+ERST
+
+    {
+        .name       = "queue-status",
+        .args_type  = "path:s,queue:i",
+        .params     = "path queue",
+        .help       = "Display status of a given virtio queue",
+        .cmd        = hmp_virtio_queue_status,
+        .flags      = "p",
+    },
+
+SRST
+  ``virtio queue-status`` *path* *queue*
+    Display status of a given virtio queue
+
+    Example:
+
+    Dump the status of the first queue of the first virtio device::
+
+      (qemu) virtio queue-status /machine/peripheral-anon/device[3]/virtio-backend 0
+      /machine/peripheral-anon/device[3]/virtio-backend:
+        device_type:          virtio-net
+        index:                0
+        inuse:                0
+        last_avail_idx:       61 (61 % 256)
+        shadow_avail_idx:     292 (36 % 256)
+        used_idx:             61 (61 % 256)
+        signalled_used:       61 (61 % 256)
+        signalled_used_valid: 1
+        VRing:
+          num:         256
+          num_default: 256
+          align:       4096
+          desc:        0x000000006c352000
+          avail:       0x000000006c353000
+          used:        0x000000006c353240
+
+ERST
+
+    {
+        .name       = "queue-element",
+        .args_type  = "path:s,queue:i,index:i?",
+        .params     = "path queue [index]",
+        .help       = "Display element of a given virtio queue",
+        .cmd        = hmp_virtio_queue_element,
+        .flags      = "p",
+    },
+
+SRST
+  ``virtio queue-element`` *path* *queue* [*index*]
+    Display element of a given virtio queue
+
+    Example:
+
+    Dump the information of the head element of the first queue of
+    the first virtio device::
+
+      (qemu) virtio queue-element/machine/peripheral-anon/device[3]/virtio-backend 0
+      index:  67
+      ndescs: 1
+      descs:  addr 0x6fe69800 len 1536 (write)
+
+      (qemu) xp/128bx 0x6fe69800
+      000000006fe69800: 0x02 0x00 0x00 0x00 0x00 0x00 0x00 0x00
+      000000006fe69808: 0x00 0x00 0x01 0x00 0x52 0x54 0x00 0x12
+      000000006fe69810: 0x34 0x56 0x52 0x54 0x00 0x09 0x51 0xde
+      000000006fe69818: 0x08 0x00 0x45 0x00 0x00 0x4c 0x8f 0x32
+
+    device[3] is a virtio-net device and we can see in the element buffer the
+    MAC address of the card::
+
+      [root@localhost ~]# ip link show ens4
+      2: ens4: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP m0
+          link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
+
+    and the MAC address of the gateway::
+
+      [root@localhost ~]# arp -a
+      _gateway (192.168.122.1) at 52:54:00:09:51:de [ether] on ens4
+
+ERST
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8e45bce..4ef0630 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1720,6 +1720,16 @@ SRST
 ERST
 
     {
+        .name       = "virtio",
+        .args_type  = "name:S?",
+        .params     = "[cmd]",
+        .help       = "show various information about virtio",
+        .cmd        = hmp_virtio_help,
+        .sub_table  = hmp_virtio_cmds,
+        .flags      = "p",
+    },
+
+    {
         .name       = "info",
         .args_type  = "item:s?",
         .params     = "[subcommand]",
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bd4d13d..db0c5b8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -31,6 +31,9 @@
 #include "sysemu/runstate.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include CONFIG_DEVICES
+#include "monitor/hmp.h"
+#include "monitor/monitor.h"
+#include "qapi/qmp/qdict.h"
 
 static QTAILQ_HEAD(, VirtIODevice) virtio_list;
 
@@ -3919,6 +3922,32 @@ VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
     return list;
 }
 
+void hmp_virtio_query(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    VirtioInfoList *list = qmp_x_debug_query_virtio(&err);
+    VirtioInfoList *node;
+
+    if (err != NULL) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+
+    if (list == NULL) {
+        monitor_printf(mon, "No VirtIO devices\n");
+        return;
+    }
+
+    node = list;
+    while (node) {
+        monitor_printf(mon, "%s [%s]\n", node->value->path,
+                       VirtioType_str(node->value->type));
+        node = node->next;
+    }
+
+    qapi_free_VirtioInfoList(list);
+}
+
 static VirtIODevice *virtio_device_find(const char *path)
 {
     VirtIODevice *vdev;
@@ -3972,8 +4001,48 @@ VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
     return status;
 }
 
+void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *path = qdict_get_try_str(qdict, "path");
+    int queue = qdict_get_int(qdict, "queue");
+    VirtQueueStatus *s = qmp_x_debug_virtio_queue_status(path, queue, &err);
+
+    if (err != NULL) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+
+    monitor_printf(mon, "%s:\n", path);
+    monitor_printf(mon, "  device_type:          %s\n",
+                   VirtioType_str(s->device_type));
+    monitor_printf(mon, "  index:                %d\n", s->queue_index);
+    monitor_printf(mon, "  inuse:                %d\n", s->inuse);
+    monitor_printf(mon, "  last_avail_idx:       %d (%"PRId64" %% %"PRId64")\n",
+                   s->last_avail_idx, s->last_avail_idx % s->vring_num,
+                   s->vring_num);
+    monitor_printf(mon, "  shadow_avail_idx:     %d (%"PRId64" %% %"PRId64")\n",
+                   s->shadow_avail_idx, s->shadow_avail_idx % s->vring_num,
+                   s->vring_num);
+    monitor_printf(mon, "  used_idx:             %d (%"PRId64" %% %"PRId64")\n",
+                   s->used_idx, s->used_idx % s->vring_num, s->vring_num);
+    monitor_printf(mon, "  signalled_used:       %d (%"PRId64" %% %"PRId64")\n",
+                   s->signalled_used, s->signalled_used % s->vring_num,
+                   s->vring_num);
+    monitor_printf(mon, "  signalled_used_valid: %d\n", s->signalled_used_valid);
+    monitor_printf(mon, "  VRing:\n");
+    monitor_printf(mon, "    num:         %"PRId64"\n", s->vring_num);
+    monitor_printf(mon, "    num_default: %"PRId64"\n", s->vring_num_default);
+    monitor_printf(mon, "    align:       %"PRId64"\n", s->vring_align);
+    monitor_printf(mon, "    desc:        0x%016"PRIx64"\n", s->vring_desc);
+    monitor_printf(mon, "    avail:       0x%016"PRIx64"\n", s->vring_avail);
+    monitor_printf(mon, "    used:        0x%016"PRIx64"\n", s->vring_used);
+
+    qapi_free_VirtQueueStatus(s);
+}
+
 #define CONVERT_FEATURES(type, map)                \
-    ({                                           \
+    ({                                             \
         type *list = NULL;                         \
         type *node;                                \
         for (i = 0; map[i].virtio_bit != -1; i++) {\
@@ -4106,6 +4175,93 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
     return status;
 }
 
+#define DUMP_FEATURES(type, field)                                         \
+    do {                                                                   \
+        type##FeatureList * list = features->u.field.features;              \
+        if (list) {                                                        \
+            monitor_printf(mon, "                    ");                   \
+            while (list) {                                                 \
+                monitor_printf(mon, "%s", type##Feature_str(list->value)); \
+                list = list->next;                                         \
+                if (list != NULL) {                                        \
+                    monitor_printf(mon, ", ");                             \
+                }                                                          \
+            }                                                              \
+            monitor_printf(mon, "\n");                                     \
+        }                                                                  \
+    } while (0)
+
+static void hmp_virtio_dump_features(Monitor *mon,
+                                     VirtioDeviceFeatures *features)
+{
+    VirtioTransportFeatureList *transport_list = features->transport;
+    while (transport_list) {
+        monitor_printf(mon, "%s",
+                       VirtioTransportFeature_str(transport_list->value));
+        transport_list = transport_list->next;
+        if (transport_list != NULL) {
+            monitor_printf(mon, ", ");
+        }
+    }
+    monitor_printf(mon, "\n");
+    switch (features->type) {
+    case VIRTIO_TYPE_VIRTIO_SERIAL:
+        DUMP_FEATURES(VirtioSerial, virtio_serial);
+        break;
+    case VIRTIO_TYPE_VIRTIO_BLK:
+        DUMP_FEATURES(VirtioBlk, virtio_blk);
+        break;
+    case VIRTIO_TYPE_VIRTIO_GPU:
+        DUMP_FEATURES(VirtioGpu, virtio_gpu);
+        break;
+    case VIRTIO_TYPE_VIRTIO_NET:
+        DUMP_FEATURES(VirtioNet, virtio_net);
+        break;
+    case VIRTIO_TYPE_VIRTIO_SCSI:
+        DUMP_FEATURES(VirtioScsi, virtio_scsi);
+        break;
+    case VIRTIO_TYPE_VIRTIO_BALLOON:
+        DUMP_FEATURES(VirtioBalloon, virtio_balloon);
+        break;
+    case VIRTIO_TYPE_VIRTIO_IOMMU:
+        DUMP_FEATURES(VirtioIommu, virtio_iommu);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    if (features->has_unknown_features) {
+        monitor_printf(mon, "                    "
+                            "unknown-features(0x%016"PRIx64")\n",
+                       features->unknown_features);
+    }
+}
+
+void hmp_virtio_status(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *path = qdict_get_try_str(qdict, "path");
+    VirtioStatus *s = qmp_x_debug_virtio_status(path, &err);
+
+    if (err != NULL) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+
+    monitor_printf(mon, "%s:\n", path);
+    monitor_printf(mon, "  Device Id:        %"PRId64"\n", s->device_id);
+    monitor_printf(mon, "  Guest features:   ");
+    hmp_virtio_dump_features(mon, s->guest_features);
+    monitor_printf(mon, "  Host features:    ");
+    hmp_virtio_dump_features(mon, s->host_features);
+    monitor_printf(mon, "  Backend features: ");
+    hmp_virtio_dump_features(mon, s->backend_features);
+    monitor_printf(mon, "  Endianness:       %s\n",
+                   VirtioStatusEndianness_str(s->device_endian));
+    monitor_printf(mon, "  VirtQueues:       %d\n", s->num_vqs);
+
+    qapi_free_VirtioStatus(s);
+}
+
 static VirtioRingDescFlagsList *qmp_decode_vring_desc_flags(uint16_t flags)
 {
     VirtioRingDescFlagsList *list = NULL;
@@ -4241,6 +4397,52 @@ done:
     return element;
 }
 
+void hmp_virtio_queue_element(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *path = qdict_get_try_str(qdict, "path");
+    int queue = qdict_get_int(qdict, "queue");
+    int index = qdict_get_try_int(qdict, "index", -1);
+    VirtioQueueElement *element;
+    VirtioRingDescList *list;
+
+    element = qmp_x_debug_virtio_queue_element(path, queue, index != -1,
+                                               index, &err);
+    if (err != NULL) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+
+    monitor_printf(mon, "index:  %d\n", element->index);
+    monitor_printf(mon, "ndescs: %d\n", element->ndescs);
+    monitor_printf(mon, "descs:  ");
+
+    list = element->descs;
+    while (list) {
+        monitor_printf(mon, "addr 0x%"PRIx64" len %d", list->value->addr,
+                       list->value->len);
+        if (list->value->flags) {
+            VirtioRingDescFlagsList *flag = list->value->flags;
+            monitor_printf(mon, " (");
+            while (flag) {
+                monitor_printf(mon, "%s", VirtioRingDescFlags_str(flag->value));
+                flag = flag->next;
+                if (flag) {
+                    monitor_printf(mon, ", ");
+                }
+            }
+            monitor_printf(mon, ")");
+        }
+        list = list->next;
+        if (list) {
+            monitor_printf(mon, ", ");
+        }
+    }
+    monitor_printf(mon, "\n");
+
+    qapi_free_VirtioQueueElement(element);
+}
+
 static const TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 3baa105..e777b32 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -94,6 +94,10 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict);
 void hmp_qom_get(Monitor *mon, const QDict *qdict);
 void hmp_qom_set(Monitor *mon, const QDict *qdict);
 void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
+void hmp_virtio_query(Monitor *mon, const QDict *qdict);
+void hmp_virtio_status(Monitor *mon, const QDict *qdict);
+void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict);
+void hmp_virtio_queue_element(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
 void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
diff --git a/meson.build b/meson.build
index eb362ee..d690de2 100644
--- a/meson.build
+++ b/meson.build
@@ -1964,6 +1964,7 @@ if have_system
   hx_headers += [
     ['hmp-commands.hx', 'hmp-commands.h'],
     ['hmp-commands-info.hx', 'hmp-commands-info.h'],
+    ['hmp-commands-virtio.hx', 'hmp-commands-virtio.h'],
   ]
 endif
 foreach d : hx_headers
diff --git a/monitor/misc.c b/monitor/misc.c
index 1539e18..ef8f87c 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include CONFIG_DEVICES
 #include "monitor-internal.h"
 #include "monitor/qdev.h"
 #include "hw/usb.h"
@@ -219,6 +220,15 @@ static void hmp_info_help(Monitor *mon, const QDict *qdict)
     help_cmd(mon, "info");
 }
 
+static void hmp_virtio_help(Monitor *mon, const QDict *qdict)
+{
+#if defined(CONFIG_VIRTIO)
+    help_cmd(mon, "virtio");
+#else
+    monitor_printf(mon, "Virtio is disabled\n");
+#endif
+}
+
 static void monitor_init_qmp_commands(void)
 {
     /*
@@ -1451,6 +1461,13 @@ static HMPCommand hmp_info_cmds[] = {
     { NULL, NULL, },
 };
 
+static HMPCommand hmp_virtio_cmds[] = {
+#if defined(CONFIG_VIRTIO)
+#include "hmp-commands-virtio.h"
+#endif
+    { NULL, NULL, },
+};
+
 /* hmp_cmds and hmp_info_cmds would be sorted at runtime */
 HMPCommand hmp_cmds[] = {
 #include "hmp-commands.h"
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 0/6] hmp,qmp: Add some commands to introspect virtio devices
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
                   ` (5 preceding siblings ...)
  2021-07-12 10:35 ` [PATCH v6 6/6] hmp: add virtio commands Jonah Palmer
@ 2021-07-13 15:25 ` Michael S. Tsirkin
  2021-07-14  2:42 ` [PATCH v6 0/6] hmp, qmp: " Jason Wang
  7 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-07-13 15:25 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, kwolf, thuth, qemu-block, amit, michael.roth, jasowang,
	david, dgilbert, qemu-devel, eric.auger, dmitrii.stepanov,
	kraxel, stefanha, pbonzini, si-wei.liu, marcandre.lureau,
	joao.m.martins, mreitz, armbru, laurent

On Mon, Jul 12, 2021 at 06:35:31AM -0400, Jonah Palmer wrote:
> This series introduces new QMP/HMP commands to dump the status of a
> virtio device at different levels.
> 
> [Jonah: Rebasing previous patchset from March for Qemu 6.1
> (here: https://lore.kernel.org/qemu-devel/1616084984-11263-1-git-send-email-jonah.palmer@oracle.com/)
> from Laurent's original qmp/hmp virtio commands from last May
> (here: https://lore.kernel.org/qemu-devel/20200507134800.10837-1-lvivier@redhat.com/)
> which included updating Makefile to meson.build, adding all virtio/vhost types, and
> other minor patching (e.g. qmp_x_debug_query_virtio uses QAPI_LIST_PREPEND rather
> than open coding to iterate through list of virtio devices, see patch 1/6).
> 
> Also, added new features (since Qemu 6.1) to virtio-gpu, virtio-net, and
> virtio-balloon. Lastly, added display for the virtio device name in a
> few of the qmp/hmp commands as well as relative indicies for vrings 
> (see patches 4/6, 6/6).]


Acked-by: Michael S. Tsirkin <mst@redhat.com>

needs to be either merged or acked by HMP maintainer.


> 1. Main command
> 
> HMP Only:
> 
>     virtio [subcommand]
> 
>     Example:
> 
>         List all sub-commands:
> 
>         (qemu) virtio
>         virtio query -- List all available virtio devices
>         virtio status path -- Display status of a given virtio device
>         virtio queue-status path queue -- Display status of a given virtio queue
>         virtio queue-element path queue [index] -- Display element of a given virtio queue
> 
> 2. List available virtio deices in the machine
> 
> HMP Form:
> 
>     virtio query
> 
>     Example:
> 
>         (qemu) virtio query
>         /machine/peripheral-anon/device[2]/virtio-backend [virtio-scsi]
>         /machine/peripheral-anon/device[1]/virtio-backend [virtio-net]
>         /machine/peripheral-anon/device[0]/virtio-backend [virtio-serial]
> 
> QMP Form:
> 
>     { 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
> 
>     Example:
> 
>         -> { "execute": "x-debug-query-virtio" }
>         <- { "return": [
>                 {
>                     "path": "/machine/peripheral-anon/device[2]/virtio-backend",
>                     "type": "virtio-scsi"
>                 },
>                 {
>                     "path": "/machine/peripheral-anon/device[1]/virtio-backend",
>                     "type": "virtio-net"
>                 },
>                 {
>                     "path": "/machine/peripheral-anon/device[0]/virtio-backend",
>                     "type": "virtio-serial"
>                 }
>             ]
>         }
> 
> 3. Display status of a given virtio device
> 
> HMP Form:
> 
>     virtio status <path>
> 
>     Example:
> 
>         (qemu) virtio status /machine/peripheral-anon/device[2]/virtio-backend
>         /machine/peripheral-anon/device[2]/virtio-backend:
>             Device Id:          8
>             Guest features:     event-idx, indirect-desc, version-1, change,
>                                 hotplug
>             Host features:      event-idx, indirect-desc, bad-feature, version-1,
>                                 any-layout, notify-on-empty, change, hotplug
>             Backend features:
>             Endianness:         little
>             VirtQueues:         4
> 
> QMP Form:
> 
>     { 'command': 'x-debug-virtio-status',
>       'data': { 'path': 'str' },
>       'returns': 'VirtioStatus'
>     }
> 
>     Example:
> 
>         -> { "execute": "x-debug-virtio-status"
>              "arguments": {
>                 "path": "/machine/peripheral-anon/device[2]/virtio-backend"
>              }
>            }
>         <- { "return": {
>                 "device-endian": "little",
>                 "device-id": 8,
>                 "backend-features": {
>                     "transport": [
>                     ],
>                     "type": "virtio-scsi",
>                     "features": [
>                     ]
>                 },
>                 "num-vqs": 4,
>                 "guest-features": {
>                     "transport": [
>                         "event-idx",
>                         "indirect-desc",
>                         "version-1"
>                     ],
>                     "type": "virtio-scsi",
>                     "features": [
>                         "change",
>                         "hotplug"
>                     ]
>                 },
>                 "host-features": {
>                     "transport": [
>                         "event-idx",
>                         "indirect-desc",
>                         "bad-feature",
>                         "version-1",
>                         "any-layout",
>                         "notify-on-empty"
>                     ],
>                     "type": "virtio-scsi",
>                     "features": [
>                         "change",
>                         "hotplug"
>                     ]
>                 }
>             }
>         }
> 
> 4. Display status of a given virtio queue
> 
> HMP Form:
> 
>     virtio queue-status <path> <queue>
> 
>     Example:
> 
>         (qemu) virtio queue-status /machine/peripheral-anon/device[2]/virtio-backend 3
>         /machine/peripheral-anon/device[2]/virtio-backend:
>             device_type:            virtio-scsi
>             index:                  3
>             inuse:                  0
>             last_avail_idx:         4174 (78 % 256)
>             shadow_avail_idx:       4174 (78 % 256)
>             signalled_used:         4174 (78 % 256)
>             signalled_used_valid:   1
>             VRing:
>                 num:            256
>                 num_default:    256
>                 align:          4096
>                 desc:           0x000000003cf9e000
>                 avail:          0x000000003cf9f000
>                 used:           0x000000003cf9f240
> 
> QMP Form:
> 
>     { 'command': 'x-debug-virtio-queue-status',
>       'data': { 'path': 'str', 'queue': 'uint16' },
>       'returns': 'VirtQueueStatus'
>     }
> 
>     Example:
> 
>         -> { "execute": "x-debug-virtio-queue-status",
>             "arguments": {
>                 "path": "/machine/peripheral-anon/decice[2]/virtio-backend",
>                 "queue": 3
>             }
>            }
>         <- { "return": {
>                 "signalled-used": 4188,
>                 "inuse": 0,
>                 "vring-align": 4096,
>                 "vring-desc": 1023008768,
>                 "signalled-used-valid": 1,
>                 "vring-num-default": 256,
>                 "vring-avail": 1023012864,
>                 "queue-index": 3,
>                 "last-avail-idx": 4188,
>                 "vring-used": 1023013440,
>                 "used-idx": 4188,
>                 "device-type": "virtio-scsi",
>                 "shadow-avail-idx": 4188
>                 "vring-num": 256
>            }
>           }
> 
> 5. Display element of a given virtio queue
> 
> HMP Form:
> 
>     virtio queue-element <path> <queue> [index]
> 
>     Example:
> 
>         Dump the information of the head element of the third queue of virtio-scsi:
> 
>         (qemu) virtio queue-element /machine/peripheral-anon/device[3]/virtio-backend 3
>         index: 122
>         ndescs: 3
>         descs: addr 0x7302d000 len 4096 (write), addr 0x3c951763 len 108 (write, next),
>                addr 0x3c951728 len 51 (next)
> 
>         (qemu) xp/128x 0x7302d000
>         000000007302d000: 0xce 0x14 0x56 0x77 0x78 0x7f 0x00 0x00
>         000000007302d008: 0x05 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>         000000007302d010: 0xf9 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>         000000007302d018: 0x4f 0xf9 0x55 0x77 0x78 0x7f 0x00 0x00
>         ...
> 
> QMP Form:
> 
>     { 'command': 'x-debug-virtio-queue-element',
>       'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' },
>       'returns': 'VirtioQueueElement'
>     }
> 
>     Example:
> 
>         -> { "execute": "x-debug-virtio-queue-element",
>              "arguments": {
>                 "path": "/machine/peripheral-anon/device[2]/virtio-backend",
>                 "queue": 0
>              }
>            }
>         <- { "return": {
>                 "index": 122,
>                 "ndescs": 3,
>                 "descs": [
>                     {
>                         "flags": [
>                             "write"
>                         ],
>                         "len": 4096,
>                         "addr": 1929564160
>                     },
>                     {
>                         "flags": [
>                             "write",
>                             "next"
>                         ],
>                         "len": 108,
>                         "addr": 1016403811
>                     },
>                     {
>                         "flags": [
>                             "next"
>                         ],
>                         "len": 51,
>                         "addr": 1016403752
>                     }
>                 ]
>             }
>         }
> 
> [Jonah:
> Comments:
> 
> One thing worth mentioning that this patch series adds is some maintenance
> burden. More specifically, the following would need to be done if a new
> virtio device type (with features) were to be added:
> 
>  - In the new */virtio-<device>.c: add #include "qapi/qapi-visit-virtio.h"
>    and define a qmp_virtio_feature_map_t map structure with device feature
>    entries (e.g. FEATURE_ENTRY(_FEATURE_))
> 
>  - In qapi/virtio.json: define a new enumeration of Virtio<Device>Feature
>    (including its enumerated features) and define a new
>    VirtioDeviceFeaturesOptions<Device> and add it to VirtioDeviceFeatures
> 
>  - In hw/virtio/virtio.c: add a case to switch statements in qmp_decode_features
>    and hmp_virtio_dump_features
> 
> If you're just adding a new feature for an already existing virtio device:
> 
>  - In */virtio-<device>.c: add the new FEATURE_ENTRY(_FEATURE_) in the
>    device's qmp_virtio_feature_map_t structure
> 
>  - In qapi/virtio.json: add the enumeration of the new virtio device
>    feature in the corresponding Virtio<Device>Feature JSON structure
> 
> In the previous patch series (v4) there was a comment regarding the
> implementation of the switch case in hmp_virtio_dump_features. It would
> be nice to not need to explicitly define a case for each virtio device
> type (with features) here, but it's not very clear (to me) on how this
> could be achieved (and optimally we'd probably want something similar for
> qmp_decode_features as well).
> 
> The suggestion to this problem (from last May) was to do something like:
> 
>     if (features->type < something_MAX) {
>         features_str = anarray[features->type];
>         ...
>         monitor_printf(mon, "%s", features_str(list->value));
>         ...
>     }
> 
> But, (1): the device type enumerations were changed to "flat" enums in v4
> and, as I understand it, flat enums have no value associated with them so
> we wouldn't be able to use it to index anarray. And (2): not every virtio
> device has features (e.g. virtio-9p, virtio-input, vhost-user-fs, etc.)
> so we'd still need to take that into account and filter-out those devices
> as well.
> 
> I've looked at it for awhile but, at least to me, it wasn't obvious how
> this could be done.
> 
> Note: for patch 5/6, checkpatch.pl gives the following error:
> 
>    ERROR: "foo * bar" should be "foo *bar"
>    #329: FILE: hw/virtio/virtio.c:4164
>             type##FeatureList * list = features->u.field.features;
> 
> But doing both 'type##FeatureList * list = ...' and
> 'type##FeatureList *list = ...' give errors, so I just left it as the former
> representation. ]
> 
> v6: rebased for upstream (Qemu 6.1)
>     add all virtio/vhost types in same order as 
>     include/standard-headers/linux/virtio_ids.h
>     use QAPI_LIST_PREPEND in qmp_x_debug_query_virtio rather than open
>     coding
> 
> v5: rebased for upstream
>     add device name, used index, and relative indicies to virtio queue-status
>     HMP command
>     add device name to virtio queue-status QMP command
>     add new virtio device features
> 
> v4: re-send series as v3 didn't reach qemu-devel
> 
> v3: use qapi_free_VirtioInfoList() on the head of the list, not on the tail.
>     Prefix the QMP commands with 'x-debug-'
> 
> v2: introduce VirtioType enum
>     use an enum for the endianness
>     change field names to stick to naming convertions (s/_/-/)
>     add a patch to decode feature bits
>     don't check if the queue is empty to allow display of old elements
>     use enum for desc flags
>     manage indirect desc
>     decode device features in the HMP command
> 
> Laurent Vivier (6):
>   qmp: add QMP command x-debug-query-virtio
>   qmp: add QMP command x-debug-virtio-status
>   qmp: decode feature bits in virtio-status
>   qmp: add QMP command x-debug-virtio-queue-status
>   qmp: add QMP command x-debug-virtio-queue-element
>   hmp: add virtio commands
> 
>  docs/system/monitor.rst      |   2 +
>  hmp-commands-virtio.hx       | 162 ++++++++++++
>  hmp-commands.hx              |  10 +
>  hw/block/virtio-blk.c        |  23 ++
>  hw/char/virtio-serial-bus.c  |  11 +
>  hw/display/virtio-gpu-base.c |  12 +
>  hw/net/virtio-net.c          |  38 +++
>  hw/scsi/virtio-scsi.c        |  12 +
>  hw/virtio/meson.build        |   2 +
>  hw/virtio/virtio-balloon.c   |  14 +
>  hw/virtio/virtio-iommu.c     |  14 +
>  hw/virtio/virtio-stub.c      |  34 +++
>  hw/virtio/virtio.c           | 574 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h   |  14 +
>  include/monitor/hmp.h        |   4 +
>  meson.build                  |   1 +
>  monitor/misc.c               |  17 ++
>  qapi/meson.build             |   1 +
>  qapi/qapi-schema.json        |   1 +
>  qapi/virtio.json             | 604 +++++++++++++++++++++++++++++++++++++++++++
>  tests/qtest/qmp-cmd-test.c   |   1 +
>  21 files changed, 1551 insertions(+)
>  create mode 100644 hmp-commands-virtio.hx
>  create mode 100644 hw/virtio/virtio-stub.c
>  create mode 100644 qapi/virtio.json
> 
> -- 
> 1.8.3.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-12 10:35 ` [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status Jonah Palmer
@ 2021-07-14  2:37   ` Jason Wang
  2021-07-21  8:59     ` Jonah Palmer
  2021-08-07 12:45   ` Markus Armbruster
  1 sibling, 1 reply; 32+ messages in thread
From: Jason Wang @ 2021-07-14  2:37 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	laurent


在 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.


> +    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'
> +}



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 6/6] hmp: add virtio commands
  2021-07-12 10:35 ` [PATCH v6 6/6] hmp: add virtio commands Jonah Palmer
@ 2021-07-14  2:40   ` Jason Wang
  2021-07-21  9:11     ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Jason Wang @ 2021-07-14  2:40 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	laurent


在 2021/7/12 下午6:35, Jonah Palmer 写道:
> +void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    const char *path = qdict_get_try_str(qdict, "path");
> +    int queue = qdict_get_int(qdict, "queue");
> +    VirtQueueStatus *s = qmp_x_debug_virtio_queue_status(path, queue, &err);
> +
> +    if (err != NULL) {
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "%s:\n", path);
> +    monitor_printf(mon, "  device_type:          %s\n",
> +                   VirtioType_str(s->device_type));
> +    monitor_printf(mon, "  index:                %d\n", s->queue_index);
> +    monitor_printf(mon, "  inuse:                %d\n", s->inuse);
> +    monitor_printf(mon, "  last_avail_idx:       %d (%"PRId64" %% %"PRId64")\n",
> +                   s->last_avail_idx, s->last_avail_idx % s->vring_num,
> +                   s->vring_num);
> +    monitor_printf(mon, "  shadow_avail_idx:     %d (%"PRId64" %% %"PRId64")\n",
> +                   s->shadow_avail_idx, s->shadow_avail_idx % s->vring_num,
> +                   s->vring_num);
> +    monitor_printf(mon, "  used_idx:             %d (%"PRId64" %% %"PRId64")\n",
> +                   s->used_idx, s->used_idx % s->vring_num, s->vring_num);


The modular information is not the case of packed ring where the queue 
size does not have to be a power of 2.

Thanks


> +    monitor_printf(mon, "  signalled_used:       %d (%"PRId64" %% %"PRId64")\n",
> +                   s->signalled_used, s->signalled_used % s->vring_num,
> +                   s->vring_num);
> +    monitor_printf(mon, "  signalled_used_valid: %d\n", s->signalled_used_valid);
> +    monitor_printf(mon, "  VRing:\n");
> +    monitor_printf(mon, "    num:         %"PRId64"\n", s->vring_num);
> +    monitor_printf(mon, "    num_default: %"PRId64"\n", s->vring_num_default);
> +    monitor_printf(mon, "    align:       %"PRId64"\n", s->vring_align);
> +    monitor_printf(mon, "    desc:        0x%016"PRIx64"\n", s->vring_desc);
> +    monitor_printf(mon, "    avail:       0x%016"PRIx64"\n", s->vring_avail);
> +    monitor_printf(mon, "    used:        0x%016"PRIx64"\n", s->vring_used);
> +
> +    qapi_free_VirtQueueStatus(s);



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices
  2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
                   ` (6 preceding siblings ...)
  2021-07-13 15:25 ` [PATCH v6 0/6] hmp,qmp: Add some commands to introspect virtio devices Michael S. Tsirkin
@ 2021-07-14  2:42 ` Jason Wang
  2021-07-21  8:53   ` Jonah Palmer
  7 siblings, 1 reply; 32+ messages in thread
From: Jason Wang @ 2021-07-14  2:42 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	laurent


在 2021/7/12 下午6:35, Jonah Palmer 写道:
>          Dump the information of the head element of the third queue of virtio-scsi:
>
>          (qemu) virtio queue-element /machine/peripheral-anon/device[3]/virtio-backend 3
>          index: 122
>          ndescs: 3
>          descs: addr 0x7302d000 len 4096 (write), addr 0x3c951763 len 108 (write, next),
>                 addr 0x3c951728 len 51 (next)


I think it would be nice if we can show driver area and device area as 
well here.

Thanks



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices
  2021-07-14  2:42 ` [PATCH v6 0/6] hmp, qmp: " Jason Wang
@ 2021-07-21  8:53   ` Jonah Palmer
  2021-07-22  9:16     ` Jason Wang
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-07-21  8:53 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent

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

Hi Jason. My apologies for the delayed response, several work-related 
things came up recently, but they're slowing down now so I'm turning my 
attention these patches to get taken care of.

A few questions and comments below (and in other following patches):


On 7/13/21 10:42 PM, Jason Wang wrote:
>
> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>>          Dump the information of the head element of the third queue 
>> of virtio-scsi:
>>
>>          (qemu) virtio queue-element 
>> /machine/peripheral-anon/device[3]/virtio-backend 3
>>          index: 122
>>          ndescs: 3
>>          descs: addr 0x7302d000 len 4096 (write), addr 0x3c951763 len 
>> 108 (write, next),
>>                 addr 0x3c951728 len 51 (next)
>
>
> I think it would be nice if we can show driver area and device area as 
> well here.

Sure thing. And I apologize if it's obvious (I'm relatively new to virtio), but how can I expose the driver area?

I understand that virtio devices are part of the Qemu process, but I also thought that virtio drivers are in the
guest's kernel, which I don't believe I can see into from Qemu (or, at least, it's not obvious to me).

Jonah

>
> Thanks
>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-14  2:37   ` Jason Wang
@ 2021-07-21  8:59     ` Jonah Palmer
  2021-07-22  9:22       ` Jason Wang
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-07-21  8:59 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent

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


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)? Same question for shadow_avail_idx below as well.

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'
>> +}
>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 6/6] hmp: add virtio commands
  2021-07-14  2:40   ` Jason Wang
@ 2021-07-21  9:11     ` Jonah Palmer
  2021-07-22  9:18       ` Jason Wang
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-07-21  9:11 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent

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


On 7/13/21 10:40 PM, Jason Wang wrote:
>
> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>> +void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict)
>> +{
>> +    Error *err = NULL;
>> +    const char *path = qdict_get_try_str(qdict, "path");
>> +    int queue = qdict_get_int(qdict, "queue");
>> +    VirtQueueStatus *s = qmp_x_debug_virtio_queue_status(path, 
>> queue, &err);
>> +
>> +    if (err != NULL) {
>> +        hmp_handle_error(mon, err);
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "%s:\n", path);
>> +    monitor_printf(mon, "  device_type:          %s\n",
>> +                   VirtioType_str(s->device_type));
>> +    monitor_printf(mon, "  index:                %d\n", 
>> s->queue_index);
>> +    monitor_printf(mon, "  inuse:                %d\n", s->inuse);
>> +    monitor_printf(mon, "  last_avail_idx:       %d (%"PRId64" %% 
>> %"PRId64")\n",
>> +                   s->last_avail_idx, s->last_avail_idx % s->vring_num,
>> +                   s->vring_num);
>> +    monitor_printf(mon, "  shadow_avail_idx:     %d (%"PRId64" %% 
>> %"PRId64")\n",
>> +                   s->shadow_avail_idx, s->shadow_avail_idx % 
>> s->vring_num,
>> +                   s->vring_num);
>> +    monitor_printf(mon, "  used_idx:             %d (%"PRId64" %% 
>> %"PRId64")\n",
>> +                   s->used_idx, s->used_idx % s->vring_num, 
>> s->vring_num);
>
>
> The modular information is not the case of packed ring where the queue 
> size does not have to be a power of 2.

Doesn't modulo work for any integer, regardless if it's a power of 2 or not? Could you clarify this for me?

Thank you,

Jonah

>
> Thanks
>
>
>> +    monitor_printf(mon, " signalled_used:       %d (%"PRId64" %% 
>> %"PRId64")\n",
>> +                   s->signalled_used, s->signalled_used % s->vring_num,
>> +                   s->vring_num);
>> +    monitor_printf(mon, "  signalled_used_valid: %d\n", 
>> s->signalled_used_valid);
>> +    monitor_printf(mon, "  VRing:\n");
>> +    monitor_printf(mon, "    num:         %"PRId64"\n", s->vring_num);
>> +    monitor_printf(mon, "    num_default: %"PRId64"\n", 
>> s->vring_num_default);
>> +    monitor_printf(mon, "    align:       %"PRId64"\n", 
>> s->vring_align);
>> +    monitor_printf(mon, "    desc:        0x%016"PRIx64"\n", 
>> s->vring_desc);
>> +    monitor_printf(mon, "    avail:       0x%016"PRIx64"\n", 
>> s->vring_avail);
>> +    monitor_printf(mon, "    used:        0x%016"PRIx64"\n", 
>> s->vring_used);
>> +
>> +    qapi_free_VirtQueueStatus(s);
>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices
  2021-07-21  8:53   ` Jonah Palmer
@ 2021-07-22  9:16     ` Jason Wang
  2021-07-26  9:11       ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Jason Wang @ 2021-07-22  9:16 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent


在 2021/7/21 下午4:53, Jonah Palmer 写道:
>
> Hi Jason. My apologies for the delayed response, several work-related 
> things came up recently, but they're slowing down now so I'm turning 
> my attention these patches to get taken care of.
>
> A few questions and comments below (and in other following patches):
>
>
> On 7/13/21 10:42 PM, Jason Wang wrote:
>>
>> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>>>          Dump the information of the head element of the third queue 
>>> of virtio-scsi:
>>>
>>>          (qemu) virtio queue-element 
>>> /machine/peripheral-anon/device[3]/virtio-backend 3
>>>          index: 122
>>>          ndescs: 3
>>>          descs: addr 0x7302d000 len 4096 (write), addr 0x3c951763 
>>> len 108 (write, next),
>>>                 addr 0x3c951728 len 51 (next)
>>
>>
>> I think it would be nice if we can show driver area and device area 
>> as well here.
> Sure thing. And I apologize if it's obvious (I'm relatively new to virtio), but how can I expose the driver area?


So the spec defines three parts: the device area, the driver area, and 
the descriptor area. And they are all located in the guest memory.


> I understand that virtio devices are part of the Qemu process, but I also thought that virtio drivers are in the
> guest's kernel, which I don't believe I can see into from Qemu (or, at least, it's not obvious to me).


It works like how you access the descriptor ring (descriptor area).

Thanks


>
> Jonah
>>
>> Thanks
>>



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 6/6] hmp: add virtio commands
  2021-07-21  9:11     ` Jonah Palmer
@ 2021-07-22  9:18       ` Jason Wang
  2021-07-26  9:38         ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Jason Wang @ 2021-07-22  9:18 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent


在 2021/7/21 下午5:11, Jonah Palmer 写道:
>
>
> On 7/13/21 10:40 PM, Jason Wang wrote:
>>
>> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>>> +void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict)
>>> +{
>>> +    Error *err = NULL;
>>> +    const char *path = qdict_get_try_str(qdict, "path");
>>> +    int queue = qdict_get_int(qdict, "queue");
>>> +    VirtQueueStatus *s = qmp_x_debug_virtio_queue_status(path, 
>>> queue, &err);
>>> +
>>> +    if (err != NULL) {
>>> +        hmp_handle_error(mon, err);
>>> +        return;
>>> +    }
>>> +
>>> +    monitor_printf(mon, "%s:\n", path);
>>> +    monitor_printf(mon, "  device_type:          %s\n",
>>> +                   VirtioType_str(s->device_type));
>>> +    monitor_printf(mon, "  index:                %d\n", 
>>> s->queue_index);
>>> +    monitor_printf(mon, "  inuse:                %d\n", s->inuse);
>>> +    monitor_printf(mon, "  last_avail_idx:       %d (%"PRId64" %% 
>>> %"PRId64")\n",
>>> +                   s->last_avail_idx, s->last_avail_idx % 
>>> s->vring_num,
>>> +                   s->vring_num);
>>> +    monitor_printf(mon, "  shadow_avail_idx:     %d (%"PRId64" %% 
>>> %"PRId64")\n",
>>> +                   s->shadow_avail_idx, s->shadow_avail_idx % 
>>> s->vring_num,
>>> +                   s->vring_num);
>>> +    monitor_printf(mon, "  used_idx:             %d (%"PRId64" %% 
>>> %"PRId64")\n",
>>> +                   s->used_idx, s->used_idx % s->vring_num, 
>>> s->vring_num);
>>
>>
>> The modular information is not the case of packed ring where the 
>> queue size does not have to be a power of 2.
> Doesn't modulo work for any integer, regardless if it's a power of 2 or not? Could you clarify this for me?


For packed ring, the index doesn't increase freely, it's always small 
than the virtqueue size.

So showing the modulo arithmetic seems useless since the device or 
driver doesn't use modulo for calculating the real offset.

Thanks


>
> Thank you,



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-21  8:59     ` Jonah Palmer
@ 2021-07-22  9:22       ` Jason Wang
  2021-07-26  9:33         ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Jason Wang @ 2021-07-22  9:22 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent


在 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


>
> 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'
>>> +}
>>



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices
  2021-07-22  9:16     ` Jason Wang
@ 2021-07-26  9:11       ` Jonah Palmer
  0 siblings, 0 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-26  9:11 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent


On 7/22/21 5:16 AM, Jason Wang wrote:
>
> 在 2021/7/21 下午4:53, Jonah Palmer 写道:
>>
>> Hi Jason. My apologies for the delayed response, several work-related 
>> things came up recently, but they're slowing down now so I'm turning 
>> my attention these patches to get taken care of.
>>
>> A few questions and comments below (and in other following patches):
>>
>>
>> On 7/13/21 10:42 PM, Jason Wang wrote:
>>>
>>> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>>>>          Dump the information of the head element of the third 
>>>> queue of virtio-scsi:
>>>>
>>>>          (qemu) virtio queue-element 
>>>> /machine/peripheral-anon/device[3]/virtio-backend 3
>>>>          index: 122
>>>>          ndescs: 3
>>>>          descs: addr 0x7302d000 len 4096 (write), addr 0x3c951763 
>>>> len 108 (write, next),
>>>>                 addr 0x3c951728 len 51 (next)
>>>
>>>
>>> I think it would be nice if we can show driver area and device area 
>>> as well here.
>> Sure thing. And I apologize if it's obvious (I'm relatively new to 
>> virtio), but how can I expose the driver area?
>
>
> So the spec defines three parts: the device area, the driver area, and 
> the descriptor area. And they are all located in the guest memory.
>
>
>> I understand that virtio devices are part of the Qemu process, but I 
>> also thought that virtio drivers are in the
>> guest's kernel, which I don't believe I can see into from Qemu (or, 
>> at least, it's not obvious to me).
>
>
> It works like how you access the descriptor ring (descriptor area).
>
> Thanks

Oh, I see now! I didn't realize the device area is essentially the used 
ring and the driver area is the avail ring (at least for the split 
virtqueue model). I see this in the virtio spec now.


Thank you!

>
>
>>
>> Jonah
>>>
>>> Thanks
>>>
>


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-22  9:22       ` Jason Wang
@ 2021-07-26  9:33         ` Jonah Palmer
  2021-08-26  6:25           ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-07-26  9:33 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent

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


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'
>>>> +}
>>>
>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 6/6] hmp: add virtio commands
  2021-07-22  9:18       ` Jason Wang
@ 2021-07-26  9:38         ` Jonah Palmer
  0 siblings, 0 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-07-26  9:38 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent


On 7/22/21 5:18 AM, Jason Wang wrote:
>
> 在 2021/7/21 下午5:11, Jonah Palmer 写道:
>>
>>
>> On 7/13/21 10:40 PM, Jason Wang wrote:
>>>
>>> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>>>> +void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict)
>>>> +{
>>>> +    Error *err = NULL;
>>>> +    const char *path = qdict_get_try_str(qdict, "path");
>>>> +    int queue = qdict_get_int(qdict, "queue");
>>>> +    VirtQueueStatus *s = qmp_x_debug_virtio_queue_status(path, 
>>>> queue, &err);
>>>> +
>>>> +    if (err != NULL) {
>>>> +        hmp_handle_error(mon, err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    monitor_printf(mon, "%s:\n", path);
>>>> +    monitor_printf(mon, "  device_type:          %s\n",
>>>> +                   VirtioType_str(s->device_type));
>>>> +    monitor_printf(mon, "  index:                %d\n", 
>>>> s->queue_index);
>>>> +    monitor_printf(mon, "  inuse:                %d\n", s->inuse);
>>>> +    monitor_printf(mon, "  last_avail_idx:       %d (%"PRId64" %% 
>>>> %"PRId64")\n",
>>>> +                   s->last_avail_idx, s->last_avail_idx % 
>>>> s->vring_num,
>>>> +                   s->vring_num);
>>>> +    monitor_printf(mon, "  shadow_avail_idx:     %d (%"PRId64" %% 
>>>> %"PRId64")\n",
>>>> +                   s->shadow_avail_idx, s->shadow_avail_idx % 
>>>> s->vring_num,
>>>> +                   s->vring_num);
>>>> +    monitor_printf(mon, "  used_idx:             %d (%"PRId64" %% 
>>>> %"PRId64")\n",
>>>> +                   s->used_idx, s->used_idx % s->vring_num, 
>>>> s->vring_num);
>>>
>>>
>>> The modular information is not the case of packed ring where the 
>>> queue size does not have to be a power of 2.
>> Doesn't modulo work for any integer, regardless if it's a power of 2 
>> or not? Could you clarify this for me?
>
>
> For packed ring, the index doesn't increase freely, it's always small 
> than the virtqueue size.
>
> So showing the modulo arithmetic seems useless since the device or 
> driver doesn't use modulo for calculating the real offset.
>
> Thanks

I see, got it. Thank you for the explanation.

I should be able to easily determine a packed or split ring via. 
virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED).


Jonah

>
>
>>
>> Thank you,
>


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
  2021-07-12 10:35 ` [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio Jonah Palmer
@ 2021-08-07 12:35   ` Markus Armbruster
  2021-08-10  6:36     ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-08-07 12:35 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, jasowang,
	david, qemu-devel, amit, dgilbert, eric.auger, dmitrii.stepanov,
	kraxel, stefanha, pbonzini, si-wei.liu, marcandre.lureau,
	joao.m.martins, mreitz, laurent

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
   #

> +##
> +# = Virtio devices
> +##
> +
> +##
> +# @VirtioType:
> +#
> +# An enumeration of Virtio device types.
> +#
> +# Since: 6.1

6.2 now, here and below.

> +##
> +{ '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.

> +}
> +
> +##
> +# @VirtioInfo:
> +#
> +# Information about a given VirtIODevice
> +#
> +# @path: VirtIO device canonical path.

Peeking ahead at the example, I conclude this is a QOM path.  Please
spell that out, e.g. "@path: the device's canonical QOM path".

> +#
> +# @type: VirtIO device type.
> +#
> +# Since: 6.1
> +#
> +##
> +{ 'struct': 'VirtioInfo',
> +  'data': {
> +    'path': 'str',
> +    'type': 'VirtioType'
> +  }
> +}
> +
> +##
> +# @x-debug-query-virtio:
> +#
> +# Return the list of all VirtIO devices
> +#
> +# Returns: list of @VirtioInfo
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "x-debug-query-virtio" }
> +# <- { "return": [
> +#        {
> +#            "path": "/machine/peripheral-anon/device[3]/virtio-backend",
> +#            "type": "virtio-net"
> +#        },
> +#        {
> +#            "path": "/machine/peripheral-anon/device[1]/virtio-backend",
> +#            "type": "virtio-serial"
> +#        },
> +#        {
> +#            "path": "/machine/peripheral-anon/device[0]/virtio-backend",
> +#            "type": "virtio-blk"
> +#        }
> +#      ]
> +#    }
> +#
> +##
> +
> +{ 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }

[...]



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status
  2021-07-12 10:35 ` [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status Jonah Palmer
@ 2021-08-07 12:42   ` Markus Armbruster
  2021-08-10  6:50     ` Jonah Palmer
  2021-09-03  8:26   ` Michael S. Tsirkin
  1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-08-07 12:42 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, jasowang,
	david, qemu-devel, amit, dgilbert, eric.auger, dmitrii.stepanov,
	kraxel, stefanha, pbonzini, si-wei.liu, marcandre.lureau,
	joao.m.martins, mreitz, laurent

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

> From: Laurent Vivier <lvivier@redhat.com>
>
> This new command shows the status of a VirtIODevice
> (features, endianness and number of virtqueues)
>
> Next patch will improve output by decoding feature bits.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>

[...]

> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 804adbe..4bd09c9 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -70,3 +70,79 @@
>  ##
>  
>  { 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
> +
> +##
> +# @VirtioStatusEndianness:
> +#
> +# Enumeration of endianness for VirtioDevice
> +#
> +# Since: 6.1

6.2 now, here, below, and in the remainder of this series.

> +##
> +{ 'enum': 'VirtioStatusEndianness',
> +  'data': [ 'unknown', 'little', 'big' ]
> +}
> +
> +##
> +# @VirtioStatus:
> +#
> +# @device-id: VirtIODevice status

"status"?  Really?

> +#
> +# @device-endian: VirtIODevice device_endian
> +#
> +# @guest-features: VirtIODevice guest_features
> +#
> +# @host-features: VirtIODevice host_features
> +#
> +# @backend-features: VirtIODevice backend_features
> +#
> +# @num-vqs: number of VirtIODevice queues
> +#
> +# Since: 6.1
> +#
> +##
> +
> +{ 'struct': 'VirtioStatus',
> +  'data': {
> +    'device-id': 'int',

VirtIODevice member @device_id is uint64_t.  Should this be 'uint16'?

> +    'device-endian': 'VirtioStatusEndianness',
> +    'guest-features': 'uint64',
> +    'host-features': 'uint64',
> +    'backend-features': 'uint64',
> +    'num-vqs': 'uint16'

virtio_get_num_queues() returns int.  Sure 'uint16' is the right type?

> +  }
> +}
> +
> +##
> +# @x-debug-virtio-status:
> +#
> +# Return the status of virtio device

"of a virtio device"

> +#
> +# @path: QOBject path of the VirtIODevice

"QOM path", please.

> +#
> +# Returns: status of the VirtIODevice
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "x-debug-virtio-status",
> +#      "arguments": {
> +#          "path": "/machine/peripheral-anon/device[3]/virtio-backend"
> +#      }
> +#   }
> +# <- { "return": {
> +#          "backend-features": 0,
> +#          "guest-features": 5111807911,
> +#          "num-vqs": 3,
> +#          "host-features": 6337593319,
> +#          "device-endian": "little",
> +#          "device-id": 1
> +#      }
> +#    }
> +#
> +##
> +
> +{ 'command': 'x-debug-virtio-status',
> +  'data': { 'path': 'str' },
> +  'returns': 'VirtioStatus'
> +}



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-12 10:35 ` [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status Jonah Palmer
  2021-07-14  2:37   ` Jason Wang
@ 2021-08-07 12:45   ` Markus Armbruster
  2021-08-10  7:25     ` Jonah Palmer
  1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-08-07 12:45 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, mst, jasowang, qemu-devel, kraxel, si-wei.liu,
	joao.m.martins, qemu-block, david, armbru, marcandre.lureau,
	thuth, amit, michael.roth, dgilbert, eric.auger,
	dmitrii.stepanov, stefanha, kwolf, laurent, mreitz, pbonzini

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

> 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>

[...]

> 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'
> +  }
> +}

I can't check the member types like I did for VirtioStatus in PATCH 2
right now.  Please double-check them.

> +
> +##
> +# @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'
> +}



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
  2021-08-07 12:35   ` Markus Armbruster
@ 2021-08-10  6:36     ` Jonah Palmer
  2021-08-23 13:27       ` Markus Armbruster
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-08-10  6:36 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: fam, mst, jasowang, qemu-devel, kraxel, si-wei.liu,
	joao.m.martins, qemu-block, david, marcandre.lureau, thuth, amit,
	michael.roth, dgilbert, eric.auger, dmitrii.stepanov, stefanha,
	Boris Ostrovsky, kwolf, laurent, mreitz, pbonzini

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


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' ]

>
>> +}
>> +
>> +##
>> +# @VirtioInfo:
>> +#
>> +# Information about a given VirtIODevice
>> +#
>> +# @path: VirtIO device canonical path.
> Peeking ahead at the example, I conclude this is a QOM path.  Please
> spell that out, e.g. "@path: the device's canonical QOM path".

Got it - will do for rest of occurrences in the series.

>
>> +#
>> +# @type: VirtIO device type.
>> +#
>> +# Since: 6.1
>> +#
>> +##
>> +{ 'struct': 'VirtioInfo',
>> +  'data': {
>> +    'path': 'str',
>> +    'type': 'VirtioType'
>> +  }
>> +}
>> +
>> +##
>> +# @x-debug-query-virtio:
>> +#
>> +# Return the list of all VirtIO devices
>> +#
>> +# Returns: list of @VirtioInfo
>> +#
>> +# Since: 6.1
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "x-debug-query-virtio" }
>> +# <- { "return": [
>> +#        {
>> +#            "path": "/machine/peripheral-anon/device[3]/virtio-backend",
>> +#            "type": "virtio-net"
>> +#        },
>> +#        {
>> +#            "path": "/machine/peripheral-anon/device[1]/virtio-backend",
>> +#            "type": "virtio-serial"
>> +#        },
>> +#        {
>> +#            "path": "/machine/peripheral-anon/device[0]/virtio-backend",
>> +#            "type": "virtio-blk"
>> +#        }
>> +#      ]
>> +#    }
>> +#
>> +##
>> +
>> +{ 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
> [...]
>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status
  2021-08-07 12:42   ` Markus Armbruster
@ 2021-08-10  6:50     ` Jonah Palmer
  0 siblings, 0 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-08-10  6:50 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: fam, mst, jasowang, qemu-devel, kraxel, si-wei.liu,
	joao.m.martins, qemu-block, david, marcandre.lureau, thuth, amit,
	michael.roth, dgilbert, eric.auger, dmitrii.stepanov, stefanha,
	Boris Ostrovsky, kwolf, laurent, mreitz, pbonzini

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


On 8/7/21 8:42 AM, Markus Armbruster wrote:
> Jonah Palmer <jonah.palmer@oracle.com> writes:
>
>> From: Laurent Vivier <lvivier@redhat.com>
>>
>> This new command shows the status of a VirtIODevice
>> (features, endianness and number of virtqueues)
>>
>> Next patch will improve output by decoding feature bits.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> [...]
>
>> diff --git a/qapi/virtio.json b/qapi/virtio.json
>> index 804adbe..4bd09c9 100644
>> --- a/qapi/virtio.json
>> +++ b/qapi/virtio.json
>> @@ -70,3 +70,79 @@
>>   ##
>>   
>>   { 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
>> +
>> +##
>> +# @VirtioStatusEndianness:
>> +#
>> +# Enumeration of endianness for VirtioDevice
>> +#
>> +# Since: 6.1
> 6.2 now, here, below, and in the remainder of this series.
>
>> +##
>> +{ 'enum': 'VirtioStatusEndianness',
>> +  'data': [ 'unknown', 'little', 'big' ]
>> +}
>> +
>> +##
>> +# @VirtioStatus:
>> +#
>> +# @device-id: VirtIODevice status
> "status"?  Really?

I'll change it to 'VirtIODevice ID' instead

>
>> +#
>> +# @device-endian: VirtIODevice device_endian
>> +#
>> +# @guest-features: VirtIODevice guest_features
>> +#
>> +# @host-features: VirtIODevice host_features
>> +#
>> +# @backend-features: VirtIODevice backend_features
>> +#
>> +# @num-vqs: number of VirtIODevice queues
>> +#
>> +# Since: 6.1
>> +#
>> +##
>> +
>> +{ 'struct': 'VirtioStatus',
>> +  'data': {
>> +    'device-id': 'int',
> VirtIODevice member @device_id is uint64_t.  Should this be 'uint16'?

Ah, yes this should be uint16. I'll update this.

>
>> +    'device-endian': 'VirtioStatusEndianness',
>> +    'guest-features': 'uint64',
>> +    'host-features': 'uint64',
>> +    'backend-features': 'uint64',
>> +    'num-vqs': 'uint16'
> virtio_get_num_queues() returns int.  Sure 'uint16' is the right type?

Yes, you're right, it should be an 'int' instead. I'll also update this.

>
>> +  }
>> +}
>> +
>> +##
>> +# @x-debug-virtio-status:
>> +#
>> +# Return the status of virtio device
> "of a virtio device"

Oops! Forgot the article, thank you.

>
>> +#
>> +# @path: QOBject path of the VirtIODevice
> "QOM path", please.
>
>> +#
>> +# Returns: status of the VirtIODevice
>> +#
>> +# Since: 6.1
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "x-debug-virtio-status",
>> +#      "arguments": {
>> +#          "path": "/machine/peripheral-anon/device[3]/virtio-backend"
>> +#      }
>> +#   }
>> +# <- { "return": {
>> +#          "backend-features": 0,
>> +#          "guest-features": 5111807911,
>> +#          "num-vqs": 3,
>> +#          "host-features": 6337593319,
>> +#          "device-endian": "little",
>> +#          "device-id": 1
>> +#      }
>> +#    }
>> +#
>> +##
>> +
>> +{ 'command': 'x-debug-virtio-status',
>> +  'data': { 'path': 'str' },
>> +  'returns': 'VirtioStatus'
>> +}

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-08-07 12:45   ` Markus Armbruster
@ 2021-08-10  7:25     ` Jonah Palmer
  0 siblings, 0 replies; 32+ messages in thread
From: Jonah Palmer @ 2021-08-10  7:25 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, jasowang,
	david, qemu-devel, amit, dgilbert, eric.auger, dmitrii.stepanov,
	kraxel, stefanha, pbonzini, si-wei.liu, marcandre.lureau,
	joao.m.martins, mreitz, laurent

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


On 8/7/21 8:45 AM, Markus Armbruster wrote:
> Jonah Palmer <jonah.palmer@oracle.com> writes:
>
>> 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>
> [...]
>
>> 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'
>> +  }
>> +}
> I can't check the member types like I did for VirtioStatus in PATCH 2
> right now.  Please double-check them.

Double-checked these; 'vring.num', 'vring.num_default', and 'vring.align'
are all of type 'unsigned int'. I think these should be of type 'uint32' here.

Also, 'signalled_used_valid' is of type bool, so I'll change it to 'bool'
here as well.

>
>> +
>> +##
>> +# @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'
>> +}

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
  2021-08-10  6:36     ` Jonah Palmer
@ 2021-08-23 13:27       ` Markus Armbruster
  2021-08-26  6:18         ` Jonah Palmer
  0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-08-23 13:27 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, mst, jasowang, qemu-devel, kraxel, si-wei.liu,
	joao.m.martins, qemu-block, david, marcandre.lureau, thuth, amit,
	michael.roth, dgilbert, eric.auger, dmitrii.stepanov, stefanha,
	Boris Ostrovsky, kwolf, laurent, mreitz, pbonzini

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.

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",
        ...
    }

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.

[...]



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
  2021-08-23 13:27       ` Markus Armbruster
@ 2021-08-26  6:18         ` Jonah Palmer
  2021-09-01 11:15           ` Markus Armbruster
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-08-26  6:18 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: fam, mst, jasowang, qemu-devel, kraxel, si-wei.liu,
	joao.m.martins, qemu-block, david, marcandre.lureau, thuth, amit,
	michael.roth, dgilbert, eric.auger, dmitrii.stepanov, stefanha,
	Boris Ostrovsky, kwolf, laurent, mreitz, pbonzini

[-- 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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-07-26  9:33         ` Jonah Palmer
@ 2021-08-26  6:25           ` Jonah Palmer
  2021-08-27  3:11             ` Jason Wang
  0 siblings, 1 reply; 32+ messages in thread
From: Jonah Palmer @ 2021-08-26  6:25 UTC (permalink / raw)
  To: Jason Wang, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	pbonzini, si-wei.liu, marcandre.lureau, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent

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

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'
>>>>> +}
>>>>
>>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status
  2021-08-26  6:25           ` Jonah Palmer
@ 2021-08-27  3:11             ` Jason Wang
  0 siblings, 0 replies; 32+ messages in thread
From: Jason Wang @ 2021-08-27  3:11 UTC (permalink / raw)
  To: Jonah Palmer, qemu-devel
  Cc: fam, kwolf, thuth, qemu-block, mst, michael.roth, david, armbru,
	amit, dgilbert, eric.auger, dmitrii.stepanov, kraxel, stefanha,
	marcandre.lureau, si-wei.liu, pbonzini, joao.m.martins, mreitz,
	Boris Ostrovsky, laurent


在 2021/8/26 下午2:25, Jonah Palmer 写道:
>
> 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;
>> }


Something like this, yes.


>> 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()).
>>

So I think instead of has_vhost, we probably need get_vhost() to have a 
pointer to vhost_dev. Then we can do anything we want other than a 
dedicated interface just for avail index.

Thanks


>> 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'
>>>>>> +}
>>>>>
>>>



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
  2021-08-26  6:18         ` Jonah Palmer
@ 2021-09-01 11:15           ` Markus Armbruster
  0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-09-01 11:15 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, mst, jasowang, qemu-devel, kraxel, si-wei.liu,
	joao.m.martins, qemu-block, david, marcandre.lureau, thuth, amit,
	michael.roth, dgilbert, eric.auger, dmitrii.stepanov, stefanha,
	Boris Ostrovsky, kwolf, laurent, mreitz, pbonzini

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

> No problem! Comments below:
>
> On 8/23/21 9:27 AM, Markus Armbruster wrote:

[...]

>> 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.

Yes, but it comes with the disadvantages I listed above.

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

I think it's simpler overall, especially if you can pick option
"2. Without QAPI enum VirtioType" below.

I could be wrong.  Suggest to try it out to see what you like better.

>>
>> 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?

Variable definitions normally live in .c.

> 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.
>>
>> [...]
>>



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status
  2021-07-12 10:35 ` [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status Jonah Palmer
  2021-08-07 12:42   ` Markus Armbruster
@ 2021-09-03  8:26   ` Michael S. Tsirkin
  1 sibling, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2021-09-03  8:26 UTC (permalink / raw)
  To: Jonah Palmer
  Cc: fam, kwolf, thuth, qemu-block, amit, michael.roth, jasowang,
	david, dgilbert, qemu-devel, eric.auger, dmitrii.stepanov,
	kraxel, stefanha, pbonzini, si-wei.liu, marcandre.lureau,
	joao.m.martins, mreitz, armbru, laurent

On Mon, Jul 12, 2021 at 06:35:33AM -0400, Jonah Palmer wrote:
> From: Laurent Vivier <lvivier@redhat.com>
> 
> This new command shows the status of a VirtIODevice
> (features, endianness and number of virtqueues)
> 
> Next patch will improve output by decoding feature bits.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>

Do we want to also decode the device status?
E.g. does it have driver, need reset, etc etc?

Also, the internal broken flag?


> ---
>  hw/virtio/virtio-stub.c |  5 ++++
>  hw/virtio/virtio.c      | 50 ++++++++++++++++++++++++++++++++
>  qapi/virtio.json        | 76 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
> 
> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
> index d4a88f5..ddb592f 100644
> --- a/hw/virtio/virtio-stub.c
> +++ b/hw/virtio/virtio-stub.c
> @@ -12,3 +12,8 @@ VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
>  {
>      return qmp_virtio_unsupported(errp);
>  }
> +
> +VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
> +{
> +    return qmp_virtio_unsupported(errp);
> +}
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f3fc1bb..222330a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3896,6 +3896,56 @@ VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
>      return list;
>  }
>  
> +static VirtIODevice *virtio_device_find(const char *path)
> +{
> +    VirtIODevice *vdev;
> +
> +    QTAILQ_FOREACH(vdev, &virtio_list, next) {
> +        DeviceState *dev = DEVICE(vdev);
> +
> +        if (strcmp(dev->canonical_path, path) != 0) {
> +            continue;
> +        }
> +        return vdev;
> +    }
> +
> +    return NULL;
> +}
> +
> +VirtioStatus *qmp_x_debug_virtio_status(const char* path, Error **errp)
> +{
> +    VirtIODevice *vdev;
> +    VirtioStatus *status;
> +
> +    vdev = virtio_device_find(path);
> +    if (vdev == NULL) {
> +        error_setg(errp, "Path %s is not a VirtIO device", path);
> +        return NULL;
> +    }
> +
> +    status = g_new0(VirtioStatus, 1);
> +    status->guest_features = vdev->guest_features;
> +    status->host_features = vdev->host_features;
> +    status->backend_features = vdev->backend_features;
> +    status->device_id = vdev->device_id;
> +
> +    switch (vdev->device_endian) {
> +    case VIRTIO_DEVICE_ENDIAN_LITTLE:
> +        status->device_endian = VIRTIO_STATUS_ENDIANNESS_LITTLE;
> +        break;
> +    case VIRTIO_DEVICE_ENDIAN_BIG:
> +        status->device_endian = VIRTIO_STATUS_ENDIANNESS_BIG;
> +        break;
> +    default:
> +        status->device_endian = VIRTIO_STATUS_ENDIANNESS_UNKNOWN;
> +        break;
> +    }
> +
> +    status->num_vqs = virtio_get_num_queues(vdev);
> +
> +    return status;
> +}
> +
>  static const TypeInfo virtio_device_info = {
>      .name = TYPE_VIRTIO_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 804adbe..4bd09c9 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -70,3 +70,79 @@
>  ##
>  
>  { 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
> +
> +##
> +# @VirtioStatusEndianness:
> +#
> +# Enumeration of endianness for VirtioDevice
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'VirtioStatusEndianness',
> +  'data': [ 'unknown', 'little', 'big' ]
> +}
> +
> +##
> +# @VirtioStatus:
> +#
> +# @device-id: VirtIODevice status
> +#
> +# @device-endian: VirtIODevice device_endian
> +#
> +# @guest-features: VirtIODevice guest_features
> +#
> +# @host-features: VirtIODevice host_features
> +#
> +# @backend-features: VirtIODevice backend_features
> +#
> +# @num-vqs: number of VirtIODevice queues
> +#
> +# Since: 6.1
> +#
> +##
> +
> +{ 'struct': 'VirtioStatus',
> +  'data': {
> +    'device-id': 'int',
> +    'device-endian': 'VirtioStatusEndianness',
> +    'guest-features': 'uint64',
> +    'host-features': 'uint64',
> +    'backend-features': 'uint64',
> +    'num-vqs': 'uint16'
> +  }
> +}
> +
> +##
> +# @x-debug-virtio-status:
> +#
> +# Return the status of virtio device
> +#
> +# @path: QOBject path of the VirtIODevice
> +#
> +# Returns: status of the VirtIODevice
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "x-debug-virtio-status",
> +#      "arguments": {
> +#          "path": "/machine/peripheral-anon/device[3]/virtio-backend"
> +#      }
> +#   }
> +# <- { "return": {
> +#          "backend-features": 0,
> +#          "guest-features": 5111807911,
> +#          "num-vqs": 3,
> +#          "host-features": 6337593319,
> +#          "device-endian": "little",
> +#          "device-id": 1
> +#      }
> +#    }
> +#
> +##
> +
> +{ 'command': 'x-debug-virtio-status',
> +  'data': { 'path': 'str' },
> +  'returns': 'VirtioStatus'
> +}
> -- 
> 1.8.3.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2021-09-03 13:23 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 10:35 [PATCH v6 0/6] hmp, qmp: Add some commands to introspect virtio devices Jonah Palmer
2021-07-12 10:35 ` [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio Jonah Palmer
2021-08-07 12:35   ` Markus Armbruster
2021-08-10  6:36     ` Jonah Palmer
2021-08-23 13:27       ` Markus Armbruster
2021-08-26  6:18         ` Jonah Palmer
2021-09-01 11:15           ` Markus Armbruster
2021-07-12 10:35 ` [PATCH v6 2/6] qmp: add QMP command x-debug-virtio-status Jonah Palmer
2021-08-07 12:42   ` Markus Armbruster
2021-08-10  6:50     ` Jonah Palmer
2021-09-03  8:26   ` Michael S. Tsirkin
2021-07-12 10:35 ` [PATCH v6 3/6] qmp: decode feature bits in virtio-status Jonah Palmer
2021-07-12 10:35 ` [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status Jonah Palmer
2021-07-14  2:37   ` Jason Wang
2021-07-21  8:59     ` Jonah Palmer
2021-07-22  9:22       ` Jason Wang
2021-07-26  9:33         ` Jonah Palmer
2021-08-26  6:25           ` Jonah Palmer
2021-08-27  3:11             ` Jason Wang
2021-08-07 12:45   ` Markus Armbruster
2021-08-10  7:25     ` Jonah Palmer
2021-07-12 10:35 ` [PATCH v6 5/6] qmp: add QMP command x-debug-virtio-queue-element Jonah Palmer
2021-07-12 10:35 ` [PATCH v6 6/6] hmp: add virtio commands Jonah Palmer
2021-07-14  2:40   ` Jason Wang
2021-07-21  9:11     ` Jonah Palmer
2021-07-22  9:18       ` Jason Wang
2021-07-26  9:38         ` Jonah Palmer
2021-07-13 15:25 ` [PATCH v6 0/6] hmp,qmp: Add some commands to introspect virtio devices Michael S. Tsirkin
2021-07-14  2:42 ` [PATCH v6 0/6] hmp, qmp: " Jason Wang
2021-07-21  8:53   ` Jonah Palmer
2021-07-22  9:16     ` Jason Wang
2021-07-26  9:11       ` Jonah Palmer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.