mbox series

[v8,0/8] hmp,qmp: Add commands to introspect virtio devices

Message ID 1635334909-31614-1-git-send-email-jonah.palmer@oracle.com (mailing list archive)
Headers show
Series hmp,qmp: Add commands to introspect virtio devices | expand

Message

Jonah Palmer Oct. 27, 2021, 11:41 a.m. UTC
This series introduces new QMP/HMP commands to dump the status of a
virtio device at different levels.

[Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches
 are from Laurent Vivier from May 2020.

 Rebase from v7 to v8 includes an additional assert to make sure
 we're not returning NULL in virtio_id_to_name(). Rebase also
 includes minor additions/edits to qapi/virtio.json.]

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 vhost-queue-status path queue -- Display status of a given vhost queue
        virtio queue-element path queue [index] -- Display element of a given virtio queue

2. List available virtio devices in the machine

HMP Form:

    virtio query

    Example:

        (qemu) virtio query
        /machine/peripheral/vsock0/virtio-backend [vhost-vsock]
        /machine/peripheral/crypto0/virtio-backend [virtio-crypto]
        /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/vsock0/virtio-backend",
                    "type": "vhost-vsock"
                },
                {
                    "path": "/machine/peripheral/crypto0/virtio-backend",
                    "type": "virtio-crypto"
                },
                {
                    "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/vsock0/virtio-backend
        /machine/peripheral/vsock0/virtio-backend:
            device_name:             vhost-vsock (vhost)
            device_id:               19
            vhost_started:           true
            bus_name:                (null)
            broken:                  false
            disabled:                false
            disable_legacy_check:    false
            started:                 true
            use_started:             true
            start_on_kick:           false
            use_guest_notifier_mask: true
            vm_running:              true
            num_vqs:                 3
            queue_sel:               2
            isr:                     0
            endianness:              little
            status: acknowledge, driver, features-ok, driver-ok
            Guest features:   event-idx, indirect-desc, version-1
            Host features:    protocol-features, event-idx, indirect-desc, version-1, any-layout,
                              notify-on-empty
            Backend features: 
            VHost:
                nvqs:           2
                vq_index:       0
                max_queues:     0
                n_mem_sections: 4
                n_tmp_sections: 4
                backend_cap:    0
                log_enabled:    false
                log_size:       0
                Features:          event-idx, indirect-desc, version-1, any-layout, notify-on-empty,
                                   log-all
                Acked features:    event-idx, indirect-desc, version-1
                Backend features:  
                Protocol features:

QMP Form:

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

    Example:

        -> { "execute": "x-debug-virtio-status",
             "arguments": {
                "path": "/machine/peripheral/vsock0/virtio-backend"
             }
           }
        <- { "return": {
                "device-endian": "little",
                "bus-name": "",
                "disable-legacy-check": false,
                "name": "vhost-vsock",
                "started": true,
                "device-id": 19,
                "vhost-dev": {
                    "n-tmp-sections": 4,
                    "n-mem-sections": 4,
                    "max-queues": 0,
                    "backend-cap": 0,
                    "log-size": 0,
                    "backend-features": {
                        "transport": [],
                        "type": "vhost-vsock",
                        "features": []
                    },
                    "nvqs": 2,
                    "protocol-features": {
                        "features": []
                    },
                    "vq-index": 0,
                    "log-enabled": false,
                    "acked-features": {
                        "transport": ["event-idx", "indirect-desc", "version-1"],
                        "type": "vhost-vsock",
                        "features": []
                    },
                    "features": {
                        "transport": ["event-idx", "indirect-desc", "version-1", "any-layout",
                                      "notify-on-empty"],
                        "type": "vhost-vsock",
                        "features": ["log-all"]
                    }
                },
                "backend-features": {
                    "transport": [],
                    "type": "vhost-vsock",
                    "features": []
                },
                "start-on-kick": false,
                "isr": 0,
                "broken": false,
                "status": {
                    "dev-status": ["acknowledge", "driver", "features-ok", "driver-ok"]
                },
                "num-vqs": 3,
                "guest-features": {
                    "transport": ["event-idx", "indirect-desc", "version-1"],
                    "type": "vhost-vsock",
                    "features": []
                },
                "host-features": {
                    "transport": ["protocol-features", "event-idx", "indirect-desc", "version-1",
                                  "any-layout", "notify-on-empty"],
                    "type": "vhost-vsock",
                    "features": []
                },
                "use-guest-notifier-mask": true,
                "vm-running": true,
                "queue-sel": 2,
                "disabled": false,
                "vhost-started": true,
                "use-started": true
             }
           }

4. Display status of a given virtio queue

HMP Form:

    virtio queue-status <path> <queue>

    Example:

        (qemu) virtio queue-status /machine/peripheral-anon/device[1]/virtio-backend 2
        /machine/peripheral-anon/device[1]/virtio-backend:
            device_name:          virtio-net
            queue_index:          2
            inuse:                0
            used_idx:             27
            signalled_used:       27
            signalled_used_valid: true
            VRing:
                num:          64
                num_default:  64
                align:        4096
                desc:         0x00000001342b5000
                avail:        0x00000001342b5400
                used:         0x00000001342b54c0

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/device[1]/virtio-backend",
                "queue": 2
             }
           }
        <- { "return": {
                "signalled-used": 27,
                "inuse": 0,
                "vring-align": 4096,
                "vring-desc": 5170221056,
                "signalled-used-valid": true,
                "vring-num-default": 64,
                "vring-avail": 5170222080,
                "queue-index": 2,
                "vring-used": 5170222272,
                "used-idx": 27,
                "device-name": "virtio-net",
                "vring-num": 64
             }
           }

5. Display status of a given vhost queue

HMP Form:

    virtio vhost-queue-status <path> <queue>

    Example:

        (qemu) virtio vhost-queue-status /machine/peripheral-anon/device[1]/virtio-backend 1
        /machine/peripheral-anon/device[1]/virtio-backend:
            device_name:          virtio-net (vhost)
            kick:                 0
            call:                 0
            VRing:
                num:         256
                desc:        0x00007f31c032c000
                desc_phys:   0x00000001340c6000
                desc_size:   4096
                avail:       0x00007f31c032d000
                avail_phys:  0x00000001340c7000
                avail_size:  518
                used:        0x00007f31c032d240
                used_phys:   0x00000001340c7240
                used_size:   2054

QMP Form:

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

    Example:

        -> { "execute": "x-debug-virtio-vhost-queue-status",
             "arguments": {
                "path": "/machine/peripheral-anon/device[1]/virtio-backend",
                "queue": 1
             }
           }
        <- { "return": {
                "avail-phys": 5168197632,
                "used-phys": 5168198208,
                "avail-size": 518,
                "desc-size": 4096,
                "used-size": 2054,
                "desc": 139851654676480,
                "num": 256,
                "device-name": "virtio-net",
                "call": 0,
                "avail": 139851654680576,
                "desc-phys": 5168193536,
                "used": 139851654681152,
                "kick": 0
             }
           }

6. Display an 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[2]/virtio-backend 2
        /machine/peripheral-anon/device[2]/virtio-backend:
            device_name: virtio-scsi
            index:       125
            desc:
                ndescs:  1
                descs:   addr 0xa4f90f1d0653b5fc len 1862028160 (used, avail, next)
            avail:
                flags: 0
                idx:   2936
                ring:  125
            used:
                flags: 0
                idx:   2936

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": 2
             }
           }
        <- { "return": {
                "index": 125,
                "ndescs": 1,
                "device-name": "virtio-scsi",
                "descs": [
                    {
                        "flags": ["used", "avail", "next"],
                        "len": 1862028160,
                        "addr": 11887549308755752444
                    }
                ],
                "avail": {
                    "idx": 2936,
                    "flags": 0,
                    "ring": 125
                },
                "used": {
                    "idx": 2936,
                    "flags": 0
                }
             }
           }

[Jonah - Comments:
 Note: for patch 8/8, checkpatch.pl gives the following error:

    ERROR: spaces required around that '*' (ctx:WxV)
    #374: FILE: hw/virtio/virtio.c:4107:
             type##FeatureList *list = features->u.field.features;
                               ^
 However, adding a space between the asterisk gives a similar error
 telling me to change it back to the former representation... so I
 just left it as this.]

v8: add assert in virtio_id_to_name() to make sure we're
    not returning NULL
    minor documentation additions to qapi/virtio.json
    add virtio introspection support for vhost-user-rng

v7: rebased for upstream (Qemu 6.2)
    add ability to map between numberic device ID and
    string device ID (name) for virtio devices
    add get_vhost() callback function for VirtIODevices
    expose more fields of VirtIODevice
    expose fields of vhost devices
    decode vhost user protocol features
    decode VirtIODevice configuration statuses
    vhost support for displaying virtio queue statuses
    vhost support for displaying vhost queue statuses
    expose driver and device areas when introspecting a
    virtio queue element
    changed patch attribution

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

Jonah Palmer (2):
  virtio: drop name parameter for virtio_init()
  virtio: add vhost support for virtio devices

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

 docs/system/monitor.rst                     |    2 +
 hmp-commands-virtio.hx                      |  250 +++++
 hmp-commands.hx                             |   10 +
 hw/9pfs/virtio-9p-device.c                  |    2 +-
 hw/block/vhost-user-blk.c                   |    9 +-
 hw/block/virtio-blk.c                       |   30 +-
 hw/char/virtio-serial-bus.c                 |   15 +-
 hw/display/vhost-user-gpu.c                 |    7 +
 hw/display/virtio-gpu-base.c                |   20 +-
 hw/input/vhost-user-input.c                 |    7 +
 hw/input/virtio-input.c                     |   14 +-
 hw/net/virtio-net.c                         |   58 +-
 hw/scsi/vhost-scsi.c                        |    8 +
 hw/scsi/virtio-scsi.c                       |   20 +-
 hw/virtio/meson.build                       |    2 +
 hw/virtio/vhost-user-fs.c                   |   20 +-
 hw/virtio/vhost-user-i2c.c                  |    6 +-
 hw/virtio/vhost-user-rng.c                  |    9 +-
 hw/virtio/vhost-user-vsock.c                |    2 +-
 hw/virtio/vhost-vsock-common.c              |   21 +-
 hw/virtio/vhost-vsock.c                     |    2 +-
 hw/virtio/vhost.c                           |    3 +
 hw/virtio/virtio-balloon.c                  |   17 +-
 hw/virtio/virtio-crypto.c                   |   22 +-
 hw/virtio/virtio-iommu.c                    |   17 +-
 hw/virtio/virtio-mem.c                      |    3 +-
 hw/virtio/virtio-pmem.c                     |    3 +-
 hw/virtio/virtio-rng.c                      |    2 +-
 hw/virtio/virtio-stub.c                     |   42 +
 hw/virtio/virtio.c                          | 1036 ++++++++++++++++++++-
 include/hw/virtio/vhost-vsock-common.h      |    2 +-
 include/hw/virtio/vhost.h                   |    3 +
 include/hw/virtio/virtio-gpu.h              |    3 +-
 include/hw/virtio/virtio.h                  |   24 +-
 include/monitor/hmp.h                       |    5 +
 include/standard-headers/linux/virtio_ids.h |    1 +
 meson.build                                 |    1 +
 monitor/misc.c                              |   17 +
 qapi/meson.build                            |    1 +
 qapi/qapi-schema.json                       |    1 +
 qapi/virtio.json                            | 1303 +++++++++++++++++++++++++++
 tests/qtest/qmp-cmd-test.c                  |    1 +
 42 files changed, 2978 insertions(+), 43 deletions(-)
 create mode 100644 hmp-commands-virtio.hx
 create mode 100644 hw/virtio/virtio-stub.c
 create mode 100644 qapi/virtio.json

Comments

Daniel P. Berrangé Oct. 27, 2021, 11:55 a.m. UTC | #1
On Wed, Oct 27, 2021 at 07:41:41AM -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 Oct. 5 (v7). Original patches
>  are from Laurent Vivier from May 2020.
> 
>  Rebase from v7 to v8 includes an additional assert to make sure
>  we're not returning NULL in virtio_id_to_name(). Rebase also
>  includes minor additions/edits to qapi/virtio.json.]
> 
> 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 vhost-queue-status path queue -- Display status of a given vhost queue
>         virtio queue-element path queue [index] -- Display element of a given virtio queue

I don't see a compelling reason why these are setup as sub-commands
under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query'
naming just feels needlessly different from the current QEMU practices.

IMHO they should just be "info" subcommands for HMP. ie

         info virtio  -- List all available virtio devices
         info virtio-status path -- Display status of a given virtio device
         info virtio-queue-status path queue -- Display status of a given virtio queue
         info virtio-vhost-queue-status path queue -- Display status of a given vhost queue
         info virtio-queue-element path queue [index] -- Display element of a given virtio queue

While the corresponding QMP commands ought to be

         x-query-virtio
         x-query-virtio-status
         x-query-virtio-queue-status
         x-query-virtio-vhost-queue-status
         x-query-virtio-queue-element


Regards,
Daniel
David Hildenbrand Oct. 27, 2021, 11:59 a.m. UTC | #2
On 27.10.21 13:41, Jonah Palmer wrote:
> From: Laurent Vivier <lvivier@redhat.com>
> 
> Display feature names instead of bitmaps for host, guest, and
> backend for VirtIODevice.
> 
> Display status names instead of bitmaps for VirtIODevice.
> 
> Display feature names instead of bitmaps for backend, protocol,
> acked, and features (hdev->features) for vhost devices.
> 
> Decode features according to device type. Decode status
> according to configuration status bitmap (config_status_map).
> Decode vhost user protocol features according to vhost user
> protocol bitmap (vhost_user_protocol_map).
> 
> Transport features are on the first line. Undecoded bits
> (if any) are stored in a separate field. Vhost device field
> wont show if there's no vhost active for a given VirtIODevice.
> 
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/block/virtio-blk.c          |  28 ++
>  hw/char/virtio-serial-bus.c    |  11 +
>  hw/display/virtio-gpu-base.c   |  18 +-
>  hw/input/virtio-input.c        |  11 +-
>  hw/net/virtio-net.c            |  47 ++++
>  hw/scsi/virtio-scsi.c          |  17 ++
>  hw/virtio/vhost-user-fs.c      |  10 +
>  hw/virtio/vhost-vsock-common.c |  10 +
>  hw/virtio/virtio-balloon.c     |  14 +
>  hw/virtio/virtio-crypto.c      |  10 +
>  hw/virtio/virtio-iommu.c       |  14 +
>  hw/virtio/virtio.c             | 273 ++++++++++++++++++-
>  include/hw/virtio/vhost.h      |   3 +
>  include/hw/virtio/virtio.h     |  17 ++
>  qapi/virtio.json               | 577 ++++++++++++++++++++++++++++++++++++++---

Any particular reason we're not handling virtio-mem?

(there is only a single feature bit so far, a second one to be
introduced soon)
Laurent Vivier Oct. 27, 2021, 12:18 p.m. UTC | #3
On 27/10/2021 13:59, David Hildenbrand wrote:
> On 27.10.21 13:41, Jonah Palmer wrote:
>> From: Laurent Vivier <lvivier@redhat.com>
>>
>> Display feature names instead of bitmaps for host, guest, and
>> backend for VirtIODevice.
>>
>> Display status names instead of bitmaps for VirtIODevice.
>>
>> Display feature names instead of bitmaps for backend, protocol,
>> acked, and features (hdev->features) for vhost devices.
>>
>> Decode features according to device type. Decode status
>> according to configuration status bitmap (config_status_map).
>> Decode vhost user protocol features according to vhost user
>> protocol bitmap (vhost_user_protocol_map).
>>
>> Transport features are on the first line. Undecoded bits
>> (if any) are stored in a separate field. Vhost device field
>> wont show if there's no vhost active for a given VirtIODevice.
>>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/block/virtio-blk.c          |  28 ++
>>   hw/char/virtio-serial-bus.c    |  11 +
>>   hw/display/virtio-gpu-base.c   |  18 +-
>>   hw/input/virtio-input.c        |  11 +-
>>   hw/net/virtio-net.c            |  47 ++++
>>   hw/scsi/virtio-scsi.c          |  17 ++
>>   hw/virtio/vhost-user-fs.c      |  10 +
>>   hw/virtio/vhost-vsock-common.c |  10 +
>>   hw/virtio/virtio-balloon.c     |  14 +
>>   hw/virtio/virtio-crypto.c      |  10 +
>>   hw/virtio/virtio-iommu.c       |  14 +
>>   hw/virtio/virtio.c             | 273 ++++++++++++++++++-
>>   include/hw/virtio/vhost.h      |   3 +
>>   include/hw/virtio/virtio.h     |  17 ++
>>   qapi/virtio.json               | 577 ++++++++++++++++++++++++++++++++++++++---
> 
> Any particular reason we're not handling virtio-mem?
> 
> (there is only a single feature bit so far, a second one to be
> introduced soon)
> 

I think this is because the v1 of the series has been written in March 2020 and it has not 
been update when virtio-mem has been added (June 2020).

Thanks,
Laurent
Jonah Palmer Oct. 28, 2021, 7:54 a.m. UTC | #4
On 10/27/21 07:55, Daniel P. Berrangé wrote:
> On Wed, Oct 27, 2021 at 07:41:41AM -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 Oct. 5 (v7). Original patches
>>   are from Laurent Vivier from May 2020.
>>
>>   Rebase from v7 to v8 includes an additional assert to make sure
>>   we're not returning NULL in virtio_id_to_name(). Rebase also
>>   includes minor additions/edits to qapi/virtio.json.]
>>
>> 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 vhost-queue-status path queue -- Display status of a given vhost queue
>>          virtio queue-element path queue [index] -- Display element of a given virtio queue
> I don't see a compelling reason why these are setup as sub-commands
> under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query'
> naming just feels needlessly different from the current QEMU practices.
>
> IMHO they should just be "info" subcommands for HMP. ie
>
>           info virtio  -- List all available virtio devices
>           info virtio-status path -- Display status of a given virtio device
>           info virtio-queue-status path queue -- Display status of a given virtio queue
>           info virtio-vhost-queue-status path queue -- Display status of a given vhost queue
>           info virtio-queue-element path queue [index] -- Display element of a given virtio queue
>
> While the corresponding QMP commands ought to be
>
>           x-query-virtio
>           x-query-virtio-status
>           x-query-virtio-queue-status
>           x-query-virtio-vhost-queue-status
>           x-query-virtio-queue-element
>
>
> Regards,
> Daniel

Sure, I don't mind changing it to this if this is what others would prefer.
If there aren't any objections, I'll switch it to this in the next revision.

Jonah
Jonah Palmer Oct. 28, 2021, 7:56 a.m. UTC | #5
On 10/27/21 08:18, Laurent Vivier wrote:
> On 27/10/2021 13:59, David Hildenbrand wrote:
>> On 27.10.21 13:41, Jonah Palmer wrote:
>>> From: Laurent Vivier <lvivier@redhat.com>
>>>
>>> Display feature names instead of bitmaps for host, guest, and
>>> backend for VirtIODevice.
>>>
>>> Display status names instead of bitmaps for VirtIODevice.
>>>
>>> Display feature names instead of bitmaps for backend, protocol,
>>> acked, and features (hdev->features) for vhost devices.
>>>
>>> Decode features according to device type. Decode status
>>> according to configuration status bitmap (config_status_map).
>>> Decode vhost user protocol features according to vhost user
>>> protocol bitmap (vhost_user_protocol_map).
>>>
>>> Transport features are on the first line. Undecoded bits
>>> (if any) are stored in a separate field. Vhost device field
>>> wont show if there's no vhost active for a given VirtIODevice.
>>>
>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>> ---
>>>   hw/block/virtio-blk.c          |  28 ++
>>>   hw/char/virtio-serial-bus.c    |  11 +
>>>   hw/display/virtio-gpu-base.c   |  18 +-
>>>   hw/input/virtio-input.c        |  11 +-
>>>   hw/net/virtio-net.c            |  47 ++++
>>>   hw/scsi/virtio-scsi.c          |  17 ++
>>>   hw/virtio/vhost-user-fs.c      |  10 +
>>>   hw/virtio/vhost-vsock-common.c |  10 +
>>>   hw/virtio/virtio-balloon.c     |  14 +
>>>   hw/virtio/virtio-crypto.c      |  10 +
>>>   hw/virtio/virtio-iommu.c       |  14 +
>>>   hw/virtio/virtio.c             | 273 ++++++++++++++++++-
>>>   include/hw/virtio/vhost.h      |   3 +
>>>   include/hw/virtio/virtio.h     |  17 ++
>>>   qapi/virtio.json               | 577 
>>> ++++++++++++++++++++++++++++++++++++++---
>>
>> Any particular reason we're not handling virtio-mem?
>>
>> (there is only a single feature bit so far, a second one to be
>> introduced soon)
>>
>
> I think this is because the v1 of the series has been written in March 
> 2020 and it has not been update when virtio-mem has been added (June 
> 2020).
>
> Thanks,
> Laurent

Oops, I think I just might've missed this device. I can add in support for virtio-mem
in the next revision!

Jonah

>
>
David Hildenbrand Oct. 28, 2021, 7:57 a.m. UTC | #6
On 28.10.21 09:56, Jonah Palmer wrote:
> On 10/27/21 08:18, Laurent Vivier wrote:
>> On 27/10/2021 13:59, David Hildenbrand wrote:
>>> On 27.10.21 13:41, Jonah Palmer wrote:
>>>> From: Laurent Vivier <lvivier@redhat.com>
>>>>
>>>> Display feature names instead of bitmaps for host, guest, and
>>>> backend for VirtIODevice.
>>>>
>>>> Display status names instead of bitmaps for VirtIODevice.
>>>>
>>>> Display feature names instead of bitmaps for backend, protocol,
>>>> acked, and features (hdev->features) for vhost devices.
>>>>
>>>> Decode features according to device type. Decode status
>>>> according to configuration status bitmap (config_status_map).
>>>> Decode vhost user protocol features according to vhost user
>>>> protocol bitmap (vhost_user_protocol_map).
>>>>
>>>> Transport features are on the first line. Undecoded bits
>>>> (if any) are stored in a separate field. Vhost device field
>>>> wont show if there's no vhost active for a given VirtIODevice.
>>>>
>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>> ---
>>>>   hw/block/virtio-blk.c          |  28 ++
>>>>   hw/char/virtio-serial-bus.c    |  11 +
>>>>   hw/display/virtio-gpu-base.c   |  18 +-
>>>>   hw/input/virtio-input.c        |  11 +-
>>>>   hw/net/virtio-net.c            |  47 ++++
>>>>   hw/scsi/virtio-scsi.c          |  17 ++
>>>>   hw/virtio/vhost-user-fs.c      |  10 +
>>>>   hw/virtio/vhost-vsock-common.c |  10 +
>>>>   hw/virtio/virtio-balloon.c     |  14 +
>>>>   hw/virtio/virtio-crypto.c      |  10 +
>>>>   hw/virtio/virtio-iommu.c       |  14 +
>>>>   hw/virtio/virtio.c             | 273 ++++++++++++++++++-
>>>>   include/hw/virtio/vhost.h      |   3 +
>>>>   include/hw/virtio/virtio.h     |  17 ++
>>>>   qapi/virtio.json               | 577
>>>> ++++++++++++++++++++++++++++++++++++++---
>>>
>>> Any particular reason we're not handling virtio-mem?
>>>
>>> (there is only a single feature bit so far, a second one to be
>>> introduced soon)
>>>
>>
>> I think this is because the v1 of the series has been written in March
>> 2020 and it has not been update when virtio-mem has been added (June
>> 2020).
>>
>> Thanks,
>> Laurent
> 
> Oops, I think I just might've missed this device. I can add in support for virtio-mem
> in the next revision!

Cool, thanks! I consider the introspection interface very valuable!
Dr. David Alan Gilbert Oct. 28, 2021, 9:04 a.m. UTC | #7
* Jonah Palmer (jonah.palmer@oracle.com) wrote:
> 
> On 10/27/21 07:55, Daniel P. Berrangé wrote:
> > On Wed, Oct 27, 2021 at 07:41:41AM -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 Oct. 5 (v7). Original patches
> > >   are from Laurent Vivier from May 2020.
> > > 
> > >   Rebase from v7 to v8 includes an additional assert to make sure
> > >   we're not returning NULL in virtio_id_to_name(). Rebase also
> > >   includes minor additions/edits to qapi/virtio.json.]
> > > 
> > > 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 vhost-queue-status path queue -- Display status of a given vhost queue
> > >          virtio queue-element path queue [index] -- Display element of a given virtio queue
> > I don't see a compelling reason why these are setup as sub-commands
> > under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query'
> > naming just feels needlessly different from the current QEMU practices.
> > 
> > IMHO they should just be "info" subcommands for HMP. ie
> > 
> >           info virtio  -- List all available virtio devices
> >           info virtio-status path -- Display status of a given virtio device
> >           info virtio-queue-status path queue -- Display status of a given virtio queue
> >           info virtio-vhost-queue-status path queue -- Display status of a given vhost queue
> >           info virtio-queue-element path queue [index] -- Display element of a given virtio queue
> > 
> > While the corresponding QMP commands ought to be
> > 
> >           x-query-virtio
> >           x-query-virtio-status
> >           x-query-virtio-queue-status
> >           x-query-virtio-vhost-queue-status
> >           x-query-virtio-queue-element
> > 
> > 
> > Regards,
> > Daniel
> 
> Sure, I don't mind changing it to this if this is what others would prefer.
> If there aren't any objections, I'll switch it to this in the next revision.

I agree with Dan that there's no need for the extra level; however
you could do it all in one HMP command I think:

  info virtio -- list all available virtio devices
  info virtio path  -- Display status of a given virtio device
  info virtio path queue -- Display status of a given virtio queue
  info virtio -h path queue -- Display status of a given vhost queue
  info virtio -e path queue [index] -- Display element of a given virtio
queue

It wouldn't need to change the qmp commands underneath unless it made
sense.

Dave

> Jonah
Markus Armbruster Nov. 5, 2021, 7:26 a.m. UTC | #8
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 27, 2021 at 07:41:41AM -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 Oct. 5 (v7). Original patches
>>  are from Laurent Vivier from May 2020.
>> 
>>  Rebase from v7 to v8 includes an additional assert to make sure
>>  we're not returning NULL in virtio_id_to_name(). Rebase also
>>  includes minor additions/edits to qapi/virtio.json.]
>> 
>> 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 vhost-queue-status path queue -- Display status of a given vhost queue
>>         virtio queue-element path queue [index] -- Display element of a given virtio queue
>
> I don't see a compelling reason why these are setup as sub-commands
> under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query'
> naming just feels needlessly different from the current QEMU practices.
>
> IMHO they should just be "info" subcommands for HMP. ie
>
>          info virtio  -- List all available virtio devices
>          info virtio-status path -- Display status of a given virtio device
>          info virtio-queue-status path queue -- Display status of a given virtio queue
>          info virtio-vhost-queue-status path queue -- Display status of a given vhost queue
>          info virtio-queue-element path queue [index] -- Display element of a given virtio queue

I agree with Dan (but I'm not the maintainer).

> While the corresponding QMP commands ought to be
>
>          x-query-virtio
>          x-query-virtio-status
>          x-query-virtio-queue-status
>          x-query-virtio-vhost-queue-status
>          x-query-virtio-queue-element

I agree with Dan (and I am the maintainer).

The x- is not strictly required anymore (see commit a3c45b3e62 'qapi:
New special feature flag "unstable"').  I lean towards keeping it here,
because we don't plan to stabilize these commands.
Markus Armbruster Nov. 5, 2021, 8:50 a.m. UTC | #9
This series increases total size (text + data + bss) by 134KiB for me.
QAPI clearly was not designed for space efficiency.  Still, it's a drop
in the bucket.

If debugging commands ever become a burden for certain use cases, we can
think about making them compile-time optional.
Jonah Palmer Nov. 5, 2021, 8:58 a.m. UTC | #10
On 11/5/21 03:26, Markus Armbruster wrote:
> Daniel P. Berrangé<berrange@redhat.com>  writes:
>
>> On Wed, Oct 27, 2021 at 07:41:41AM -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 Oct. 5 (v7). Original patches
>>>   are from Laurent Vivier from May 2020.
>>>
>>>   Rebase from v7 to v8 includes an additional assert to make sure
>>>   we're not returning NULL in virtio_id_to_name(). Rebase also
>>>   includes minor additions/edits to qapi/virtio.json.]
>>>
>>> 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 vhost-queue-status path queue -- Display status of a given vhost queue
>>>          virtio queue-element path queue [index] -- Display element of a given virtio queue
>> I don't see a compelling reason why these are setup as sub-commands
>> under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query'
>> naming just feels needlessly different from the current QEMU practices.
>>
>> IMHO they should just be "info" subcommands for HMP. ie
>>
>>           info virtio  -- List all available virtio devices
>>           info virtio-status path -- Display status of a given virtio device
>>           info virtio-queue-status path queue -- Display status of a given virtio queue
>>           info virtio-vhost-queue-status path queue -- Display status of a given vhost queue
>>           info virtio-queue-element path queue [index] -- Display element of a given virtio queue
> I agree with Dan (but I'm not the maintainer).

I do like this format a bit better than Dave's recommendation. Feels a bit
more intuitive to understand what the commands should be doing, but I'm
not sure if this is just because I'm new to these things.

I'd like to format it like above if that's okay.

>
>> While the corresponding QMP commands ought to be
>>
>>           x-query-virtio
>>           x-query-virtio-status
>>           x-query-virtio-queue-status
>>           x-query-virtio-vhost-queue-status
>>           x-query-virtio-queue-element
> I agree with Dan (and I am the maintainer).
>
> The x- is not strictly required anymore (see commit a3c45b3e62 'qapi:
> New special feature flag "unstable"').  I lean towards keeping it here,
> because we don't plan to stabilize these commands.

Ok! I'll keep the 'x-' in and change them to the above.

Thank you for the comments!!

Jonah