From: "Michael S. Tsirkin" <mst@redhat.com>
To: Hyman Huang <yong.huang@smartx.com>
Cc: qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
Eric Blake <eblake@redhat.com>
Subject: Re: [PATCH v3 1/3] qmp: Switch x-query-virtio-status back to numeric encoding
Date: Tue, 13 Feb 2024 05:26:41 -0500 [thread overview]
Message-ID: <20240213052542-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <cde5e47e38e5e26390a6b0379092c197aaff9521.1706883915.git.yong.huang@smartx.com>
On Fri, Feb 02, 2024 at 10:32:15PM +0800, Hyman Huang wrote:
> x-query-virtio-status returns several sets of virtio feature and
> status flags. It goes back to v7.2.0.
>
> In the initial commit 90c066cd682 (qmp: add QMP command
> x-query-virtio-status), we returned them as numbers, using virtio's
> well-known binary encoding.
>
> The next commit f3034ad71fc (qmp: decode feature & status bits in
> virtio-status) replaced the numbers by objects. The objects represent
> bits QEMU knows symbolically, and any unknown bits numerically just like
> before.
>
> Commit 8a8287981d1 (hmp: add virtio commands) the matching HMP command
> "info virtio" (and a few more, which aren't relevant here).
>
> The symbolic representation uses lists of strings. The string format is
> undocumented. The strings look like "WELL_KNOWN_SYMBOL: human readable
> explanation".
>
> This symbolic representation is nice for humans. Machines it can save
> the trouble of decoding virtio's well-known binary encoding.
>
> However, we sometimes want to compare features and status bits without
> caring for their exact meaning. Say we want to verify the correctness
> of the virtio negotiation between guest, QEMU, and OVS-DPDK. We can use
> QMP command x-query-virtio-status to retrieve vhost-user net device
> features, and the "ovs-vsctl list interface" command to retrieve
> interface features. Without commit f3034ad71fc, we could then simply
> compare the numbers. With this commit, we first have to map from the
> strings back to the numeric encoding.
>
> Revert the decoding for QMP, but keep it for HMP.
Is there a way to maybe have both decoded and numerical one?
E.g. I mostly use QMP even when I read it myself.
> This makes the QMP command easier to use for use cases where we
> don't need to decode, like the comparison above. For use cases
> where we need to decode, we replace parsing undocumented strings by
> decoding virtio's well-known binary encoding.
>
> Incompatible change; acceptable because x-query-virtio-status does
> comes without a stability promise.
>
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
> hw/virtio/virtio-hmp-cmds.c | 25 +++--
> hw/virtio/virtio-qmp.c | 23 ++---
> qapi/virtio.json | 192 ++++--------------------------------
> 3 files changed, 45 insertions(+), 195 deletions(-)
>
> diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
> index 477c97dea2..721c630ab0 100644
> --- a/hw/virtio/virtio-hmp-cmds.c
> +++ b/hw/virtio/virtio-hmp-cmds.c
> @@ -6,6 +6,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "virtio-qmp.h"
> #include "monitor/hmp.h"
> #include "monitor/monitor.h"
> #include "qapi/qapi-commands-virtio.h"
> @@ -145,13 +146,17 @@ void hmp_virtio_status(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, " endianness: %s\n",
> s->device_endian);
> monitor_printf(mon, " status:\n");
> - hmp_virtio_dump_status(mon, s->status);
> + hmp_virtio_dump_status(mon,
> + qmp_decode_status(s->status));
> monitor_printf(mon, " Guest features:\n");
> - hmp_virtio_dump_features(mon, s->guest_features);
> + hmp_virtio_dump_features(mon,
> + qmp_decode_features(s->device_id, s->guest_features));
> monitor_printf(mon, " Host features:\n");
> - hmp_virtio_dump_features(mon, s->host_features);
> + hmp_virtio_dump_features(mon,
> + qmp_decode_features(s->device_id, s->host_features));
> monitor_printf(mon, " Backend features:\n");
> - hmp_virtio_dump_features(mon, s->backend_features);
> + hmp_virtio_dump_features(mon,
> + qmp_decode_features(s->device_id, s->backend_features));
>
> if (s->vhost_dev) {
> monitor_printf(mon, " VHost:\n");
> @@ -172,13 +177,17 @@ void hmp_virtio_status(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, " log_size: %"PRId64"\n",
> s->vhost_dev->log_size);
> monitor_printf(mon, " Features:\n");
> - hmp_virtio_dump_features(mon, s->vhost_dev->features);
> + hmp_virtio_dump_features(mon,
> + qmp_decode_features(s->device_id, s->vhost_dev->features));
> monitor_printf(mon, " Acked features:\n");
> - hmp_virtio_dump_features(mon, s->vhost_dev->acked_features);
> + hmp_virtio_dump_features(mon,
> + qmp_decode_features(s->device_id, s->vhost_dev->acked_features));
> monitor_printf(mon, " Backend features:\n");
> - hmp_virtio_dump_features(mon, s->vhost_dev->backend_features);
> + hmp_virtio_dump_features(mon,
> + qmp_decode_features(s->device_id, s->vhost_dev->backend_features));
> monitor_printf(mon, " Protocol features:\n");
> - hmp_virtio_dump_protocols(mon, s->vhost_dev->protocol_features);
> + hmp_virtio_dump_protocols(mon,
> + qmp_decode_protocols(s->vhost_dev->protocol_features));
> }
>
> qapi_free_VirtioStatus(s);
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index 1dd96ed20f..1660c17653 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -733,12 +733,9 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> status->name = g_strdup(vdev->name);
> status->device_id = vdev->device_id;
> status->vhost_started = vdev->vhost_started;
> - status->guest_features = qmp_decode_features(vdev->device_id,
> - vdev->guest_features);
> - status->host_features = qmp_decode_features(vdev->device_id,
> - vdev->host_features);
> - status->backend_features = qmp_decode_features(vdev->device_id,
> - vdev->backend_features);
> + status->guest_features = vdev->guest_features;
> + status->host_features = vdev->host_features;
> + status->backend_features = vdev->backend_features;
>
> switch (vdev->device_endian) {
> case VIRTIO_DEVICE_ENDIAN_LITTLE:
> @@ -753,7 +750,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> }
>
> status->num_vqs = virtio_get_num_queues(vdev);
> - status->status = qmp_decode_status(vdev->status);
> + status->status = vdev->status;
> status->isr = vdev->isr;
> status->queue_sel = vdev->queue_sel;
> status->vm_running = vdev->vm_running;
> @@ -775,14 +772,10 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> status->vhost_dev->n_tmp_sections = hdev->n_tmp_sections;
> status->vhost_dev->nvqs = hdev->nvqs;
> status->vhost_dev->vq_index = hdev->vq_index;
> - status->vhost_dev->features =
> - qmp_decode_features(vdev->device_id, hdev->features);
> - status->vhost_dev->acked_features =
> - qmp_decode_features(vdev->device_id, hdev->acked_features);
> - status->vhost_dev->backend_features =
> - qmp_decode_features(vdev->device_id, hdev->backend_features);
> - status->vhost_dev->protocol_features =
> - qmp_decode_protocols(hdev->protocol_features);
> + status->vhost_dev->features = hdev->features;
> + status->vhost_dev->acked_features = hdev->acked_features;
> + status->vhost_dev->backend_features = hdev->backend_features;
> + status->vhost_dev->protocol_features = hdev->protocol_features;
> status->vhost_dev->max_queues = hdev->max_queues;
> status->vhost_dev->backend_cap = hdev->backend_cap;
> status->vhost_dev->log_enabled = hdev->log_enabled;
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 19c7c36e36..26516fb29c 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -102,10 +102,10 @@
> 'n-tmp-sections': 'int',
> 'nvqs': 'uint32',
> 'vq-index': 'int',
> - 'features': 'VirtioDeviceFeatures',
> - 'acked-features': 'VirtioDeviceFeatures',
> - 'backend-features': 'VirtioDeviceFeatures',
> - 'protocol-features': 'VhostDeviceProtocols',
> + 'features': 'uint64',
> + 'acked-features': 'uint64',
> + 'backend-features': 'uint64',
> + 'protocol-features': 'uint64',
> 'max-queues': 'uint64',
> 'backend-cap': 'uint64',
> 'log-enabled': 'bool',
> @@ -170,11 +170,11 @@
> 'device-id': 'uint16',
> 'vhost-started': 'bool',
> 'device-endian': 'str',
> - 'guest-features': 'VirtioDeviceFeatures',
> - 'host-features': 'VirtioDeviceFeatures',
> - 'backend-features': 'VirtioDeviceFeatures',
> + 'guest-features': 'uint64',
> + 'host-features': 'uint64',
> + 'backend-features': 'uint64',
> 'num-vqs': 'int',
> - 'status': 'VirtioDeviceStatus',
> + 'status': 'uint8',
> 'isr': 'uint8',
> 'queue-sel': 'uint16',
> 'vm-running': 'bool',
> @@ -217,41 +217,14 @@
> # "name": "virtio-crypto",
> # "started": true,
> # "device-id": 20,
> -# "backend-features": {
> -# "transports": [],
> -# "dev-features": []
> -# },
> +# "backend-features": 0,
> # "start-on-kick": false,
> # "isr": 1,
> # "broken": false,
> -# "status": {
> -# "statuses": [
> -# "VIRTIO_CONFIG_S_ACKNOWLEDGE: Valid virtio device found",
> -# "VIRTIO_CONFIG_S_DRIVER: Guest OS compatible with device",
> -# "VIRTIO_CONFIG_S_FEATURES_OK: Feature negotiation complete",
> -# "VIRTIO_CONFIG_S_DRIVER_OK: Driver setup and ready"
> -# ]
> -# },
> +# "status": 15,
> # "num-vqs": 2,
> -# "guest-features": {
> -# "dev-features": [],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
> -# ]
> -# },
> -# "host-features": {
> -# "unknown-dev-features": 1073741824,
> -# "dev-features": [],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)",
> -# "VIRTIO_F_ANY_LAYOUT: Device accepts arbitrary desc. layouts",
> -# "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
> -# ]
> -# },
> +# "guest-features": 5100273664,
> +# "host-features": 6325010432,
> # "use-guest-notifier-mask": true,
> # "vm-running": true,
> # "queue-sel": 1,
> @@ -279,147 +252,22 @@
> # "max-queues": 1,
> # "backend-cap": 2,
> # "log-size": 0,
> -# "backend-features": {
> -# "dev-features": [],
> -# "transports": []
> -# },
> +# "backend-features": 0,
> # "nvqs": 2,
> -# "protocol-features": {
> -# "protocols": []
> -# },
> +# "protocol-features": 0,
> # "vq-index": 0,
> # "log-enabled": false,
> -# "acked-features": {
> -# "dev-features": [
> -# "VIRTIO_NET_F_MRG_RXBUF: Driver can merge receive buffers"
> -# ],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
> -# ]
> -# },
> -# "features": {
> -# "dev-features": [
> -# "VHOST_F_LOG_ALL: Logging write descriptors supported",
> -# "VIRTIO_NET_F_MRG_RXBUF: Driver can merge receive buffers"
> -# ],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_IOMMU_PLATFORM: Device can be used on IOMMU platform",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)",
> -# "VIRTIO_F_ANY_LAYOUT: Device accepts arbitrary desc. layouts",
> -# "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
> -# ]
> -# }
> -# },
> -# "backend-features": {
> -# "dev-features": [
> -# "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features negotiation supported",
> -# "VIRTIO_NET_F_GSO: Handling GSO-type packets supported",
> -# "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control channel",
> -# "VIRTIO_NET_F_GUEST_ANNOUNCE: Driver sending gratuitous packets supported",
> -# "VIRTIO_NET_F_CTRL_RX_EXTRA: Extra RX mode control supported",
> -# "VIRTIO_NET_F_CTRL_VLAN: Control channel VLAN filtering supported",
> -# "VIRTIO_NET_F_CTRL_RX: Control channel RX mode supported",
> -# "VIRTIO_NET_F_CTRL_VQ: Control channel available",
> -# "VIRTIO_NET_F_STATUS: Configuration status field available",
> -# "VIRTIO_NET_F_MRG_RXBUF: Driver can merge receive buffers",
> -# "VIRTIO_NET_F_HOST_UFO: Device can receive UFO",
> -# "VIRTIO_NET_F_HOST_ECN: Device can receive TSO with ECN",
> -# "VIRTIO_NET_F_HOST_TSO6: Device can receive TSOv6",
> -# "VIRTIO_NET_F_HOST_TSO4: Device can receive TSOv4",
> -# "VIRTIO_NET_F_GUEST_UFO: Driver can receive UFO",
> -# "VIRTIO_NET_F_GUEST_ECN: Driver can receive TSO with ECN",
> -# "VIRTIO_NET_F_GUEST_TSO6: Driver can receive TSOv6",
> -# "VIRTIO_NET_F_GUEST_TSO4: Driver can receive TSOv4",
> -# "VIRTIO_NET_F_MAC: Device has given MAC address",
> -# "VIRTIO_NET_F_CTRL_GUEST_OFFLOADS: Control channel offloading reconfig. supported",
> -# "VIRTIO_NET_F_GUEST_CSUM: Driver handling packets with partial checksum supported",
> -# "VIRTIO_NET_F_CSUM: Device handling packets with partial checksum supported"
> -# ],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)",
> -# "VIRTIO_F_ANY_LAYOUT: Device accepts arbitrary desc. layouts",
> -# "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
> -# ]
> +# "acked-features": 5100306432,
> +# "features": 13908344832,
> # },
> +# "backend-features": 6337593319,
> # "start-on-kick": false,
> # "isr": 1,
> # "broken": false,
> -# "status": {
> -# "statuses": [
> -# "VIRTIO_CONFIG_S_ACKNOWLEDGE: Valid virtio device found",
> -# "VIRTIO_CONFIG_S_DRIVER: Guest OS compatible with device",
> -# "VIRTIO_CONFIG_S_FEATURES_OK: Feature negotiation complete",
> -# "VIRTIO_CONFIG_S_DRIVER_OK: Driver setup and ready"
> -# ]
> -# },
> +# "status": 15,
> # "num-vqs": 3,
> -# "guest-features": {
> -# "dev-features": [
> -# "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control channel",
> -# "VIRTIO_NET_F_GUEST_ANNOUNCE: Driver sending gratuitous packets supported",
> -# "VIRTIO_NET_F_CTRL_VLAN: Control channel VLAN filtering supported",
> -# "VIRTIO_NET_F_CTRL_RX: Control channel RX mode supported",
> -# "VIRTIO_NET_F_CTRL_VQ: Control channel available",
> -# "VIRTIO_NET_F_STATUS: Configuration status field available",
> -# "VIRTIO_NET_F_MRG_RXBUF: Driver can merge receive buffers",
> -# "VIRTIO_NET_F_HOST_UFO: Device can receive UFO",
> -# "VIRTIO_NET_F_HOST_ECN: Device can receive TSO with ECN",
> -# "VIRTIO_NET_F_HOST_TSO6: Device can receive TSOv6",
> -# "VIRTIO_NET_F_HOST_TSO4: Device can receive TSOv4",
> -# "VIRTIO_NET_F_GUEST_UFO: Driver can receive UFO",
> -# "VIRTIO_NET_F_GUEST_ECN: Driver can receive TSO with ECN",
> -# "VIRTIO_NET_F_GUEST_TSO6: Driver can receive TSOv6",
> -# "VIRTIO_NET_F_GUEST_TSO4: Driver can receive TSOv4",
> -# "VIRTIO_NET_F_MAC: Device has given MAC address",
> -# "VIRTIO_NET_F_CTRL_GUEST_OFFLOADS: Control channel offloading reconfig. supported",
> -# "VIRTIO_NET_F_GUEST_CSUM: Driver handling packets with partial checksum supported",
> -# "VIRTIO_NET_F_CSUM: Device handling packets with partial checksum supported"
> -# ],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
> -# ]
> -# },
> -# "host-features": {
> -# "dev-features": [
> -# "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features negotiation supported",
> -# "VIRTIO_NET_F_GSO: Handling GSO-type packets supported",
> -# "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control channel",
> -# "VIRTIO_NET_F_GUEST_ANNOUNCE: Driver sending gratuitous packets supported",
> -# "VIRTIO_NET_F_CTRL_RX_EXTRA: Extra RX mode control supported",
> -# "VIRTIO_NET_F_CTRL_VLAN: Control channel VLAN filtering supported",
> -# "VIRTIO_NET_F_CTRL_RX: Control channel RX mode supported",
> -# "VIRTIO_NET_F_CTRL_VQ: Control channel available",
> -# "VIRTIO_NET_F_STATUS: Configuration status field available",
> -# "VIRTIO_NET_F_MRG_RXBUF: Driver can merge receive buffers",
> -# "VIRTIO_NET_F_HOST_UFO: Device can receive UFO",
> -# "VIRTIO_NET_F_HOST_ECN: Device can receive TSO with ECN",
> -# "VIRTIO_NET_F_HOST_TSO6: Device can receive TSOv6",
> -# "VIRTIO_NET_F_HOST_TSO4: Device can receive TSOv4",
> -# "VIRTIO_NET_F_GUEST_UFO: Driver can receive UFO",
> -# "VIRTIO_NET_F_GUEST_ECN: Driver can receive TSO with ECN",
> -# "VIRTIO_NET_F_GUEST_TSO6: Driver can receive TSOv6",
> -# "VIRTIO_NET_F_GUEST_TSO4: Driver can receive TSOv4",
> -# "VIRTIO_NET_F_MAC: Device has given MAC address",
> -# "VIRTIO_NET_F_CTRL_GUEST_OFFLOADS: Control channel offloading reconfig. supported",
> -# "VIRTIO_NET_F_GUEST_CSUM: Driver handling packets with partial checksum supported",
> -# "VIRTIO_NET_F_CSUM: Device handling packets with partial checksum supported"
> -# ],
> -# "transports": [
> -# "VIRTIO_RING_F_EVENT_IDX: Used & avail. event fields enabled",
> -# "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported",
> -# "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)",
> -# "VIRTIO_F_ANY_LAYOUT: Device accepts arbitrary desc. layouts",
> -# "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
> -# ]
> -# },
> +# "guest-features": 5111807911,
> +# "host-features": 6337593319,
> # "use-guest-notifier-mask": true,
> # "vm-running": true,
> # "queue-sel": 2,
> --
> 2.31.1
next prev parent reply other threads:[~2024-02-13 10:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-02 14:32 [PATCH v3 0/3] Adjust the output of x-query-virtio-status Hyman Huang
2024-02-02 14:32 ` [PATCH v3 1/3] qmp: Switch x-query-virtio-status back to numeric encoding Hyman Huang
2024-02-13 10:26 ` Michael S. Tsirkin [this message]
2024-02-15 12:49 ` Yong Huang
2024-02-19 15:32 ` Markus Armbruster
2024-02-20 1:27 ` Yong Huang
2024-02-19 15:18 ` Markus Armbruster
2024-02-02 14:32 ` [PATCH v3 2/3] virtio: Declare the decoding functions to static Hyman Huang
2024-02-19 15:38 ` Markus Armbruster
2024-02-21 3:59 ` Yong Huang
2024-02-02 14:32 ` [PATCH v3 3/3] qapi: Define VhostDeviceProtocols and VirtioDeviceFeatures as plain C types Hyman Huang
2024-02-19 15:33 ` Markus Armbruster
2024-02-13 10:25 ` [PATCH v3 0/3] Adjust the output of x-query-virtio-status Michael S. Tsirkin
2024-02-13 13:30 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240213052542-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=armbru@redhat.com \
--cc=eblake@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=yong.huang@smartx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).