| Message ID | cde5e47e38e5e26390a6b0379092c197aaff9521.1706883915.git.yong.huang@smartx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Adjust the output of x-query-virtio-status | expand |
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
On Tue, Feb 13, 2024 at 6:26 PM Michael S. Tsirkin <mst@redhat.com> wrote: > 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? > What if the next patch went back to this implementation in the following patch? All you need to do is add a matching xxx_bits entry for each feature and status. https://patchew.org/QEMU/cover.1700319559.git.yong.huang@smartx.com/3c7161a47b141af04b1f8272e8e24c5faa46ddb2.1700319559.git.yong.huang@smartx.com/ E.g. I mostly use QMP even when I read it myself. > Thus, it is recommended that both numerical and decoded types be kept for QMP; this approach can be at odds with what was previously discussed. What do you think about this, Markus? > > 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 > >
Hyman Huang <yong.huang@smartx.com> writes: > 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 added 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. > > 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 Scratch "does". > comes without a stability promise. > > Signed-off-by: Hyman Huang <yong.huang@smartx.com> Acked-by: Markus Armbruster <armbru@redhat.com>
Yong Huang <yong.huang@smartx.com> writes: > On Tue, Feb 13, 2024 at 6:26 PM Michael S. Tsirkin <mst@redhat.com> wrote: > >> 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? > > What if the next patch went back to this implementation in the following > patch? All you need to do is add a matching xxx_bits entry for each feature > and status. > > https://patchew.org/QEMU/cover.1700319559.git.yong.huang@smartx.com/3c7161a47b141af04b1f8272e8e24c5faa46ddb2.1700319559.git.yong.huang@smartx.com/ > > > E.g. I mostly use QMP even when I read it myself. >> > > Thus, it is recommended that both numerical and decoded types be kept for > QMP; this approach can be at odds with what was previously discussed. > What do you think about this, Markus? QMP is for machines. That said, I won't object to adding development & debugging aids meant for humans as long as 1. they're marked unstable, and 2. they either add only a modest amount of output, or the additional output is off by default. What exactly is a "modest amount" depends on how machines use the command. If they use it all the time, even going from 8KiB to 64KiB could be too much. If they use it just once per guest, we can afford more waste. In this particular case, we could add an unstable member of type ['str'] next to the int-valued bits. For each set bit in the int, add a string to the list. If QEMU knows the bit, use the well-known name. I'd omit the common prefix, though: use "GUEST_ANNOUNCE" or "guest-announce" instead of "VIRTIO_NET_F_GUEST_ANNOUNCE". If QEMU doesn't know the bit, you could use the bit's numeric value. Recommend a separate patch, to avoid delaying this series. Makes sense?
On Mon, Feb 19, 2024 at 11:32 PM Markus Armbruster <armbru@redhat.com> wrote: > Yong Huang <yong.huang@smartx.com> writes: > > > On Tue, Feb 13, 2024 at 6:26 PM Michael S. Tsirkin <mst@redhat.com> > wrote: > > > >> 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? > > > > What if the next patch went back to this implementation in the following > > patch? All you need to do is add a matching xxx_bits entry for each > feature > > and status. > > > > > https://patchew.org/QEMU/cover.1700319559.git.yong.huang@smartx.com/3c7161a47b141af04b1f8272e8e24c5faa46ddb2.1700319559.git.yong.huang@smartx.com/ > > > > > > E.g. I mostly use QMP even when I read it myself. > >> > > > > Thus, it is recommended that both numerical and decoded types be kept for > > QMP; this approach can be at odds with what was previously discussed. > > What do you think about this, Markus? > > QMP is for machines. > > That said, I won't object to adding development & debugging aids meant > for humans as long as > > 1. they're marked unstable, and > > 2. they either add only a modest amount of output, or the additional > output is off by default. > > What exactly is a "modest amount" depends on how machines use the > command. If they use it all the time, even going from 8KiB to 64KiB > could be too much. If they use it just once per guest, we can afford > more waste. > > In this particular case, we could add an unstable member of type ['str'] > next to the int-valued bits. For each set bit in the int, add a string > to the list. If QEMU knows the bit, use the well-known name. I'd omit > the common prefix, though: use "GUEST_ANNOUNCE" or "guest-announce" > instead of "VIRTIO_NET_F_GUEST_ANNOUNCE". If QEMU doesn't know the bit, > you could use the bit's numeric value. > > Recommend a separate patch, to avoid delaying this series. > > Makes sense? > > I agree, and thanks for the comments. Yong
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,
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. 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(-)