Re: [PATCH v6 0/6] hmp,qmp: Add some commands to introspect virtio devices

[Michael S. Tsirkin]

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