[RFC,1/2] qapi/virtio: introduce the "show-bits" argument for x-query-virtio-status

Commit Message

Hyman Huang Nov. 12, 2023, 1:03 p.m. UTC

This patch allows to display feature and status bits in virtio-status.

An optional argument is introduced: show-bits. For example:
{"execute": "x-query-virtio-status",
 "arguments": {"path": "/machine/peripheral-anon/device[1]/virtio-backend",
               "show-bits": true}

Features and status bits could be helpful for applications to compare
directly. For instance, when an upper application aims to ensure the
virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
the "ovs-vsctl list interface" command to retrieve interface features
(in number format) and the QMP command x-query-virtio-status to retrieve
vhost-user net device features. If "show-bits" is added, the application
can compare the two features directly; No need to encoding the features
returned by the QMP command.

This patch also serves as a preparation for the next one, which implements
a vhost-user test case about acked features of vhost-user protocol.

Note that since the matching HMP command is typically used for human,
leave it unchanged.

Signed-off-by: Hyman Huang <[e-mail]>
---
 hw/virtio/virtio-hmp-cmds.c |  2 +-
 hw/virtio/virtio-qmp.c      | 21 +++++++++++++++-
 qapi/virtio.json            | 49 ++++++++++++++++++++++++++++++++++---
 3 files changed, 67 insertions(+), 5 deletions(-)

Comments

Markus Armbruster Nov. 16, 2023, 2:44 p.m. UTC | #1

Hyman Huang <[e-mail]> writes:

> This patch allows to display feature and status bits in virtio-status.
>
> An optional argument is introduced: show-bits. For example:
> {"execute": "x-query-virtio-status",
>  "arguments": {"path": "/machine/peripheral-anon/device[1]/virtio-backend",
>                "show-bits": true}
>
> Features and status bits could be helpful for applications to compare
> directly. For instance, when an upper application aims to ensure the
> virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
> the "ovs-vsctl list interface" command to retrieve interface features
> (in number format) and the QMP command x-query-virtio-status to retrieve
> vhost-user net device features. If "show-bits" is added, the application
> can compare the two features directly; No need to encoding the features
> returned by the QMP command.
>
> This patch also serves as a preparation for the next one, which implements
> a vhost-user test case about acked features of vhost-user protocol.
>
> Note that since the matching HMP command is typically used for human,
> leave it unchanged.
>
> Signed-off-by: Hyman Huang <[e-mail]>
> ---
>  hw/virtio/virtio-hmp-cmds.c |  2 +-
>  hw/virtio/virtio-qmp.c      | 21 +++++++++++++++-
>  qapi/virtio.json            | 49 ++++++++++++++++++++++++++++++++++---
>  3 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
> index 477c97dea2..3774f3d4bf 100644
> --- a/hw/virtio/virtio-hmp-cmds.c
> +++ b/hw/virtio/virtio-hmp-cmds.c
> @@ -108,7 +108,7 @@ void hmp_virtio_status(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>      const char *path = qdict_get_try_str(qdict, "path");
> -    VirtioStatus *s = qmp_x_query_virtio_status(path, &err);
> +    VirtioStatus *s = qmp_x_query_virtio_status(path, false, false, &err);
>  
>      if (err != NULL) {
>          hmp_handle_error(mon, err);
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index 1dd96ed20f..2e92bf28ac 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -718,10 +718,15 @@ VirtIODevice *qmp_find_virtio_device(const char *path)
>      return VIRTIO_DEVICE(dev);
>  }
>  
> -VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> +VirtioStatus *qmp_x_query_virtio_status(const char *path,
> +                                        bool has_show_bits,
> +                                        bool show_bits,
> +                                        Error **errp)
>  {
>      VirtIODevice *vdev;
>      VirtioStatus *status;
> +    bool display_bits =
> +        has_show_bits ? show_bits : false;

Since !has_show_bits implies !show_bits, you can simply use
if (show_bits).

>  
>      vdev = qmp_find_virtio_device(path);
>      if (vdev == NULL) {
> @@ -733,6 +738,11 @@ 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;
> +    if (display_bits) {
> +        status->guest_features_bits = vdev->guest_features;
> +        status->host_features_bits = vdev->host_features;
> +        status->backend_features_bits = vdev->backend_features;
> +    }
>      status->guest_features = qmp_decode_features(vdev->device_id,
>                                                   vdev->guest_features);
>      status->host_features = qmp_decode_features(vdev->device_id,
> @@ -753,6 +763,9 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>      }
>  
>      status->num_vqs = virtio_get_num_queues(vdev);
> +    if (display_bits) {
> +        status->status_bits = vdev->status;
> +    }
>      status->status = qmp_decode_status(vdev->status);
>      status->isr = vdev->isr;
>      status->queue_sel = vdev->queue_sel;
> @@ -775,6 +788,12 @@ 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;
> +        if (display_bits) {
> +            status->vhost_dev->features_bits = hdev->features;
> +            status->vhost_dev->acked_features_bits = hdev->acked_features;
> +            status->vhost_dev->backend_features_bits = hdev->backend_features;
> +            status->vhost_dev->protocol_features_bits = hdev->protocol_features;
> +        }
>          status->vhost_dev->features =
>              qmp_decode_features(vdev->device_id, hdev->features);
>          status->vhost_dev->acked_features =
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index e6dcee7b83..608b841a89 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -79,12 +79,20 @@
>  #
>  # @vq-index: vhost_dev vq_index
>  #
> +# @features-bits: vhost_dev features in decimal format

Say "encoded as a number".  The number is decimal just because the
transport is JSON.  We could have another transport some day.  Or
language bindings wrapping around the JSON transport.

> +#
>  # @features: vhost_dev features

Double-checking...  @feature-bits provides the exact same information as
@features, only in another encoding.  Correct?

Same for all the other new -bits.  Correct?

>  #
> +# @acked-features-bits: vhost_dev acked_features in decimal format
> +#
>  # @acked-features: vhost_dev acked_features
>  #
> +# @backend-features-bits: vhost_dev backend_features in decimal format
> +#
>  # @backend-features: vhost_dev backend_features
>  #
> +# @protocol-features-bits: vhost_dev protocol_features in decimal format
> +#
>  # @protocol-features: vhost_dev protocol_features
>  #
>  # @max-queues: vhost_dev max_queues
> @@ -102,9 +110,13 @@
>              'n-tmp-sections': 'int',
>              'nvqs': 'uint32',
>              'vq-index': 'int',
> +            'features-bits': 'uint64',
>              'features': 'VirtioDeviceFeatures',
> +            'acked-features-bits': 'uint64',
>              'acked-features': 'VirtioDeviceFeatures',
> +            'backend-features-bits': 'uint64',
>              'backend-features': 'VirtioDeviceFeatures',
> +            'protocol-features-bits': 'uint64',
>              'protocol-features': 'VhostDeviceProtocols',
>              'max-queues': 'uint64',
>              'backend-cap': 'uint64',
> @@ -124,10 +136,16 @@
>  #
>  # @vhost-started: VirtIODevice vhost_started flag
>  #
> +# @guest-features-bits: VirtIODevice guest_features in decimal format
> +#
>  # @guest-features: VirtIODevice guest_features
>  #
> +# @host-features-bits: VirtIODevice host_features in decimal format
> +#
>  # @host-features: VirtIODevice host_features
>  #
> +# @backend-features-bits: VirtIODevice backend_features in decimal format
> +#
>  # @backend-features: VirtIODevice backend_features
>  #
>  # @device-endian: VirtIODevice device_endian
> @@ -135,6 +153,9 @@
>  # @num-vqs: VirtIODevice virtqueue count.  This is the number of
>  #     active virtqueues being used by the VirtIODevice.
>  #
> +# @status-bits: VirtIODevice configuration status in decimal format
> +#     (VirtioDeviceStatus)
> +#
>  # @status: VirtIODevice configuration status (VirtioDeviceStatus)
>  #
>  # @isr: VirtIODevice ISR
> @@ -170,10 +191,14 @@
>              'device-id': 'uint16',
>              'vhost-started': 'bool',
>              'device-endian': 'str',
> +            'guest-features-bits': 'uint64',
>              'guest-features': 'VirtioDeviceFeatures',
> +            'host-features-bits': 'uint64',
>              'host-features': 'VirtioDeviceFeatures',
> +            'backend-features-bits': 'uint64',
>              'backend-features': 'VirtioDeviceFeatures',
>              'num-vqs': 'int',
> +            'status-bits': 'uint8',
>              'status': 'VirtioDeviceStatus',
>              'isr': 'uint8',
>              'queue-sel': 'uint16',
> @@ -195,6 +220,9 @@
>  #
>  # @path: Canonical QOM path of the VirtIODevice
>  #
> +# @show-bits: Whether to display the feature & status bits.
> +#     Default is disabled. (Since 8.2)

This makes the new members opt-in.  Why not include them always?

> +#
>  # Features:
>  #
>  # @unstable: This command is meant for debugging.
> @@ -208,7 +236,8 @@
>  # 1. Poll for the status of virtio-crypto (no vhost-crypto active)
>  #
>  # -> { "execute": "x-query-virtio-status",
> -#      "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend" }
> +#      "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend",
> +#                     "show-bits": true }
>  #    }
>  # <- { "return": {
>  #          "device-endian": "little",
> @@ -216,6 +245,7 @@
>  #          "disable-legacy-check": false,
>  #          "name": "virtio-crypto",
>  #          "started": true,
> +#          "guest-features-bits": 5100273664,
>  #          "device-id": 20,
>  #          "backend-features": {
>  #              "transports": [],
> @@ -241,6 +271,7 @@
>  #                  "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
>  #              ]
>  #          },
> +#          "host-features-bits": 6325010432,
>  #          "host-features": {
>  #              "unknown-dev-features": 1073741824,
>  #              "dev-features": [],
> @@ -252,9 +283,11 @@
>  #                  "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
>  #              ]
>  #          },
> +#          "backend-features-bits": 0,
>  #          "use-guest-notifier-mask": true,
>  #          "vm-running": true,
>  #          "queue-sel": 1,
> +#          "status-bits": 15,
>  #          "disabled": false,
>  #          "vhost-started": false,
>  #          "use-started": true
> @@ -264,7 +297,8 @@
>  # 2. Poll for the status of virtio-net (vhost-net is active)
>  #
>  # -> { "execute": "x-query-virtio-status",
> -#      "arguments": { "path": "/machine/peripheral-anon/device[1]/virtio-backend" }
> +#      "arguments": { "path": "/machine/peripheral-anon/device[1]/virtio-backend",
> +#                     "show-bits": true }
>  #    }
>  # <- { "return": {
>  #          "device-endian": "little",
> @@ -272,11 +306,13 @@
>  #          "disabled-legacy-check": false,
>  #          "name": "virtio-net",
>  #          "started": true,
> +#          "guest-features-bits": 5111807911,
>  #          "device-id": 1,
>  #          "vhost-dev": {
>  #              "n-tmp-sections": 4,
>  #              "n-mem-sections": 4,
>  #              "max-queues": 1,
> +#              "features-bits": 13908344832
>  #              "backend-cap": 2,
>  #              "log-size": 0,
>  #              "backend-features": {
> @@ -284,6 +320,8 @@
>  #                  "transports": []
>  #              },
>  #              "nvqs": 2,
> +#              "acked-features-bits": 5100306432,
> +#              "backend-features-bits": 0,
>  #              "protocol-features": {
>  #                  "protocols": []
>  #              },
> @@ -299,6 +337,7 @@
>  #                      "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
>  #                  ]
>  #              },
> +#              "protocol-features-bits": 0,
>  #              "features": {
>  #                  "dev-features": [
>  #                      "VHOST_F_LOG_ALL: Logging write descriptors supported",
> @@ -387,6 +426,7 @@
>  #                  "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
>  #             ]
>  #          },
> +#          "host-features-bits": 6337593319,
>  #          "host-features": {
>  #              "dev-features": [
>  #                  "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features negotiation supported",
> @@ -420,9 +460,11 @@
>  #                  "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
>  #             ]
>  #          },
> +#          "backend-features-bits": 6337593319,
>  #          "use-guest-notifier-mask": true,
>  #          "vm-running": true,
>  #          "queue-sel": 2,
> +#          "status-bits": 15,
>  #          "disabled": false,
>  #          "vhost-started": true,
>  #          "use-started": true
> @@ -430,7 +472,8 @@
>  #    }
>  ##
>  { 'command': 'x-query-virtio-status',
> -  'data': { 'path': 'str' },
> +  'data': { 'path': 'str',
> +            '*show-bits': 'bool'},
>    'returns': 'VirtioStatus',
>    'features': [ 'unstable' ] }

Hyman Huang Nov. 17, 2023, 4:12 p.m. UTC | #2

On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]>
wrote:

> Hyman Huang <[e-mail]> writes:
>
> > This patch allows to display feature and status bits in virtio-status.
> >
> > An optional argument is introduced: show-bits. For example:
> > {"execute": "x-query-virtio-status",
> >  "arguments": {"path":
> "/machine/peripheral-anon/device[1]/virtio-backend",
> >                "show-bits": true}
> >
> > Features and status bits could be helpful for applications to compare
> > directly. For instance, when an upper application aims to ensure the
> > virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
> > the "ovs-vsctl list interface" command to retrieve interface features
> > (in number format) and the QMP command x-query-virtio-status to retrieve
> > vhost-user net device features. If "show-bits" is added, the application
> > can compare the two features directly; No need to encoding the features
> > returned by the QMP command.
> >
> > This patch also serves as a preparation for the next one, which
> implements
> > a vhost-user test case about acked features of vhost-user protocol.
> >
> > Note that since the matching HMP command is typically used for human,
> > leave it unchanged.
> >
> > Signed-off-by: Hyman Huang <[e-mail]>
> > ---
> >  hw/virtio/virtio-hmp-cmds.c |  2 +-
> >  hw/virtio/virtio-qmp.c      | 21 +++++++++++++++-
> >  qapi/virtio.json            | 49 ++++++++++++++++++++++++++++++++++---
> >  3 files changed, 67 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
> > index 477c97dea2..3774f3d4bf 100644
> > --- a/hw/virtio/virtio-hmp-cmds.c
> > +++ b/hw/virtio/virtio-hmp-cmds.c
> > @@ -108,7 +108,7 @@ void hmp_virtio_status(Monitor *mon, const QDict
> *qdict)
> >  {
> >      Error *err = NULL;
> >      const char *path = qdict_get_try_str(qdict, "path");
> > -    VirtioStatus *s = qmp_x_query_virtio_status(path, &err);
> > +    VirtioStatus *s = qmp_x_query_virtio_status(path, false, false,
> &err);
> >
> >      if (err != NULL) {
> >          hmp_handle_error(mon, err);
> > diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> > index 1dd96ed20f..2e92bf28ac 100644
> > --- a/hw/virtio/virtio-qmp.c
> > +++ b/hw/virtio/virtio-qmp.c
> > @@ -718,10 +718,15 @@ VirtIODevice *qmp_find_virtio_device(const char
> *path)
> >      return VIRTIO_DEVICE(dev);
> >  }
> >
> > -VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> > +VirtioStatus *qmp_x_query_virtio_status(const char *path,
> > +                                        bool has_show_bits,
> > +                                        bool show_bits,
> > +                                        Error **errp)
> >  {
> >      VirtIODevice *vdev;
> >      VirtioStatus *status;
> > +    bool display_bits =
> > +        has_show_bits ? show_bits : false;
>
> Since !has_show_bits implies !show_bits, you can simply use
> if (show_bits).
>
Ok

>
> >
> >      vdev = qmp_find_virtio_device(path);
> >      if (vdev == NULL) {
> > @@ -733,6 +738,11 @@ 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;
> > +    if (display_bits) {
> > +        status->guest_features_bits = vdev->guest_features;
> > +        status->host_features_bits = vdev->host_features;
> > +        status->backend_features_bits = vdev->backend_features;
> > +    }
> >      status->guest_features = qmp_decode_features(vdev->device_id,
> >                                                   vdev->guest_features);
> >      status->host_features = qmp_decode_features(vdev->device_id,
> > @@ -753,6 +763,9 @@ VirtioStatus *qmp_x_query_virtio_status(const char
> *path, Error **errp)
> >      }
> >
> >      status->num_vqs = virtio_get_num_queues(vdev);
> > +    if (display_bits) {
> > +        status->status_bits = vdev->status;
> > +    }
> >      status->status = qmp_decode_status(vdev->status);
> >      status->isr = vdev->isr;
> >      status->queue_sel = vdev->queue_sel;
> > @@ -775,6 +788,12 @@ 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;
> > +        if (display_bits) {
> > +            status->vhost_dev->features_bits = hdev->features;
> > +            status->vhost_dev->acked_features_bits =
> hdev->acked_features;
> > +            status->vhost_dev->backend_features_bits =
> hdev->backend_features;
> > +            status->vhost_dev->protocol_features_bits =
> hdev->protocol_features;
> > +        }
> >          status->vhost_dev->features =
> >              qmp_decode_features(vdev->device_id, hdev->features);
> >          status->vhost_dev->acked_features =
> > diff --git a/qapi/virtio.json b/qapi/virtio.json
> > index e6dcee7b83..608b841a89 100644
> > --- a/qapi/virtio.json
> > +++ b/qapi/virtio.json
> > @@ -79,12 +79,20 @@
> >  #
> >  # @vq-index: vhost_dev vq_index
> >  #
> > +# @features-bits: vhost_dev features in decimal format
>
> Say "encoded as a number".  The number is decimal just because the
> transport is JSON.  We could have another transport some day.  Or
> language bindings wrapping around the JSON transport.
>
> > +#
> >  # @features: vhost_dev features
>
> Double-checking...  @feature-bits provides the exact same information as
> @features, only in another encoding.  Correct?


> Same for all the other new -bits.  Correct?

Yes, all the new fields are only about providing another encoding.

>
> >  #
> > +# @acked-features-bits: vhost_dev acked_features in decimal format
> > +#
> >  # @acked-features: vhost_dev acked_features
> >  #
> > +# @backend-features-bits: vhost_dev backend_features in decimal format
> > +#
> >  # @backend-features: vhost_dev backend_features
> >  #
> > +# @protocol-features-bits: vhost_dev protocol_features in decimal format
> > +#
> >  # @protocol-features: vhost_dev protocol_features
> >  #
> >  # @max-queues: vhost_dev max_queues
> > @@ -102,9 +110,13 @@
> >              'n-tmp-sections': 'int',
> >              'nvqs': 'uint32',
> >              'vq-index': 'int',
> > +            'features-bits': 'uint64',
> >              'features': 'VirtioDeviceFeatures',
> > +            'acked-features-bits': 'uint64',
> >              'acked-features': 'VirtioDeviceFeatures',
> > +            'backend-features-bits': 'uint64',
> >              'backend-features': 'VirtioDeviceFeatures',
> > +            'protocol-features-bits': 'uint64',
> >              'protocol-features': 'VhostDeviceProtocols',
> >              'max-queues': 'uint64',
> >              'backend-cap': 'uint64',
> > @@ -124,10 +136,16 @@
> >  #
> >  # @vhost-started: VirtIODevice vhost_started flag
> >  #
> > +# @guest-features-bits: VirtIODevice guest_features in decimal format
> > +#
> >  # @guest-features: VirtIODevice guest_features
> >  #
> > +# @host-features-bits: VirtIODevice host_features in decimal format
> > +#
> >  # @host-features: VirtIODevice host_features
> >  #
> > +# @backend-features-bits: VirtIODevice backend_features in decimal
> format
> > +#
> >  # @backend-features: VirtIODevice backend_features
> >  #
> >  # @device-endian: VirtIODevice device_endian
> > @@ -135,6 +153,9 @@
> >  # @num-vqs: VirtIODevice virtqueue count.  This is the number of
> >  #     active virtqueues being used by the VirtIODevice.
> >  #
> > +# @status-bits: VirtIODevice configuration status in decimal format
> > +#     (VirtioDeviceStatus)
> > +#
> >  # @status: VirtIODevice configuration status (VirtioDeviceStatus)
> >  #
> >  # @isr: VirtIODevice ISR
> > @@ -170,10 +191,14 @@
> >              'device-id': 'uint16',
> >              'vhost-started': 'bool',
> >              'device-endian': 'str',
> > +            'guest-features-bits': 'uint64',
> >              'guest-features': 'VirtioDeviceFeatures',
> > +            'host-features-bits': 'uint64',
> >              'host-features': 'VirtioDeviceFeatures',
> > +            'backend-features-bits': 'uint64',
> >              'backend-features': 'VirtioDeviceFeatures',
> >              'num-vqs': 'int',
> > +            'status-bits': 'uint8',
> >              'status': 'VirtioDeviceStatus',
> >              'isr': 'uint8',
> >              'queue-sel': 'uint16',
> > @@ -195,6 +220,9 @@
> >  #
> >  # @path: Canonical QOM path of the VirtIODevice
> >  #
> > +# @show-bits: Whether to display the feature & status bits.
> > +#     Default is disabled. (Since 8.2)
>
> This makes the new members opt-in.  Why not include them always?
>

Agree, this makes things simple.

>
> > +#
> >  # Features:
> >  #
> >  # @unstable: This command is meant for debugging.
> > @@ -208,7 +236,8 @@
> >  # 1. Poll for the status of virtio-crypto (no vhost-crypto active)
> >  #
> >  # -> { "execute": "x-query-virtio-status",
> > -#      "arguments": { "path":
> "/machine/peripheral/crypto0/virtio-backend" }
> > +#      "arguments": { "path":
> "/machine/peripheral/crypto0/virtio-backend",
> > +#                     "show-bits": true }
> >  #    }
> >  # <- { "return": {
> >  #          "device-endian": "little",
> > @@ -216,6 +245,7 @@
> >  #          "disable-legacy-check": false,
> >  #          "name": "virtio-crypto",
> >  #          "started": true,
> > +#          "guest-features-bits": 5100273664,
> >  #          "device-id": 20,
> >  #          "backend-features": {
> >  #              "transports": [],
> > @@ -241,6 +271,7 @@
> >  #                  "VIRTIO_F_VERSION_1: Device compliant for v1 spec
> (legacy)"
> >  #              ]
> >  #          },
> > +#          "host-features-bits": 6325010432,
> >  #          "host-features": {
> >  #              "unknown-dev-features": 1073741824,
> >  #              "dev-features": [],
> > @@ -252,9 +283,11 @@
> >  #                  "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs
> out of avail. descs. on VQ"
> >  #              ]
> >  #          },
> > +#          "backend-features-bits": 0,
> >  #          "use-guest-notifier-mask": true,
> >  #          "vm-running": true,
> >  #          "queue-sel": 1,
> > +#          "status-bits": 15,
> >  #          "disabled": false,
> >  #          "vhost-started": false,
> >  #          "use-started": true
> > @@ -264,7 +297,8 @@
> >  # 2. Poll for the status of virtio-net (vhost-net is active)
> >  #
> >  # -> { "execute": "x-query-virtio-status",
> > -#      "arguments": { "path":
> "/machine/peripheral-anon/device[1]/virtio-backend" }
> > +#      "arguments": { "path":
> "/machine/peripheral-anon/device[1]/virtio-backend",
> > +#                     "show-bits": true }
> >  #    }
> >  # <- { "return": {
> >  #          "device-endian": "little",
> > @@ -272,11 +306,13 @@
> >  #          "disabled-legacy-check": false,
> >  #          "name": "virtio-net",
> >  #          "started": true,
> > +#          "guest-features-bits": 5111807911,
> >  #          "device-id": 1,
> >  #          "vhost-dev": {
> >  #              "n-tmp-sections": 4,
> >  #              "n-mem-sections": 4,
> >  #              "max-queues": 1,
> > +#              "features-bits": 13908344832
> >  #              "backend-cap": 2,
> >  #              "log-size": 0,
> >  #              "backend-features": {
> > @@ -284,6 +320,8 @@
> >  #                  "transports": []
> >  #              },
> >  #              "nvqs": 2,
> > +#              "acked-features-bits": 5100306432,
> > +#              "backend-features-bits": 0,
> >  #              "protocol-features": {
> >  #                  "protocols": []
> >  #              },
> > @@ -299,6 +337,7 @@
> >  #                      "VIRTIO_F_VERSION_1: Device compliant for v1
> spec (legacy)"
> >  #                  ]
> >  #              },
> > +#              "protocol-features-bits": 0,
> >  #              "features": {
> >  #                  "dev-features": [
> >  #                      "VHOST_F_LOG_ALL: Logging write descriptors
> supported",
> > @@ -387,6 +426,7 @@
> >  #                  "VIRTIO_F_VERSION_1: Device compliant for v1 spec
> (legacy)"
> >  #             ]
> >  #          },
> > +#          "host-features-bits": 6337593319,
> >  #          "host-features": {
> >  #              "dev-features": [
> >  #                  "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol
> features negotiation supported",
> > @@ -420,9 +460,11 @@
> >  #                  "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs
> out of avail. descs. on VQ"
> >  #             ]
> >  #          },
> > +#          "backend-features-bits": 6337593319,
> >  #          "use-guest-notifier-mask": true,
> >  #          "vm-running": true,
> >  #          "queue-sel": 2,
> > +#          "status-bits": 15,
> >  #          "disabled": false,
> >  #          "vhost-started": true,
> >  #          "use-started": true
> > @@ -430,7 +472,8 @@
> >  #    }
> >  ##
> >  { 'command': 'x-query-virtio-status',
> > -  'data': { 'path': 'str' },
> > +  'data': { 'path': 'str',
> > +            '*show-bits': 'bool'},
> >    'returns': 'VirtioStatus',
> >    'features': [ 'unstable' ] }
>
>
Yong

Markus Armbruster Nov. 21, 2023, 7:58 a.m. UTC | #3

Laurent, there's a question for you at the end.

Yong Huang <[e-mail]> writes:

> On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]>
> wrote:
>
>> Hyman Huang <[e-mail]> writes:
>>
>> > This patch allows to display feature and status bits in virtio-status.
>> >
>> > An optional argument is introduced: show-bits. For example:
>> > {"execute": "x-query-virtio-status",
>> >  "arguments": {"path": "/machine/peripheral-anon/device[1]/virtio-backend",
>> >                "show-bits": true}
>> >
>> > Features and status bits could be helpful for applications to compare
>> > directly. For instance, when an upper application aims to ensure the
>> > virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>> > the "ovs-vsctl list interface" command to retrieve interface features
>> > (in number format) and the QMP command x-query-virtio-status to retrieve
>> > vhost-user net device features. If "show-bits" is added, the application
>> > can compare the two features directly; No need to encoding the features
>> > returned by the QMP command.
>> >
>> > This patch also serves as a preparation for the next one, which implements
>> > a vhost-user test case about acked features of vhost-user protocol.
>> >
>> > Note that since the matching HMP command is typically used for human,
>> > leave it unchanged.
>> >
>> > Signed-off-by: Hyman Huang <[e-mail]>
>> > ---
>> >  hw/virtio/virtio-hmp-cmds.c |  2 +-
>> >  hw/virtio/virtio-qmp.c      | 21 +++++++++++++++-
>> >  qapi/virtio.json            | 49 ++++++++++++++++++++++++++++++++++---
>> >  3 files changed, 67 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
>> > index 477c97dea2..3774f3d4bf 100644
>> > --- a/hw/virtio/virtio-hmp-cmds.c
>> > +++ b/hw/virtio/virtio-hmp-cmds.c
>> > @@ -108,7 +108,7 @@ void hmp_virtio_status(Monitor *mon, const QDict *qdict)
>> >  {
>> >      Error *err = NULL;
>> >      const char *path = qdict_get_try_str(qdict, "path");
>> > -    VirtioStatus *s = qmp_x_query_virtio_status(path, &err);
>> > +    VirtioStatus *s = qmp_x_query_virtio_status(path, false, false, &err);
>> >
>> >      if (err != NULL) {
>> >          hmp_handle_error(mon, err);
>> > diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>> > index 1dd96ed20f..2e92bf28ac 100644
>> > --- a/hw/virtio/virtio-qmp.c
>> > +++ b/hw/virtio/virtio-qmp.c
>> > @@ -718,10 +718,15 @@ VirtIODevice *qmp_find_virtio_device(const char *path)
>> >      return VIRTIO_DEVICE(dev);
>> >  }
>> >
>> > -VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>> > +VirtioStatus *qmp_x_query_virtio_status(const char *path,
>> > +                                        bool has_show_bits,
>> > +                                        bool show_bits,
>> > +                                        Error **errp)
>> >  {
>> >      VirtIODevice *vdev;
>> >      VirtioStatus *status;
>> > +    bool display_bits =
>> > +        has_show_bits ? show_bits : false;
>>
>> Since !has_show_bits implies !show_bits, you can simply use
>> if (show_bits).
>>
> Ok
>
>>
>> >
>> >      vdev = qmp_find_virtio_device(path);
>> >      if (vdev == NULL) {
>> > @@ -733,6 +738,11 @@ 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;
>> > +    if (display_bits) {
>> > +        status->guest_features_bits = vdev->guest_features;
>> > +        status->host_features_bits = vdev->host_features;
>> > +        status->backend_features_bits = vdev->backend_features;
>> > +    }
>> >      status->guest_features = qmp_decode_features(vdev->device_id,
>> >                                                   vdev->guest_features);
>> >      status->host_features = qmp_decode_features(vdev->device_id,
>> > @@ -753,6 +763,9 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>> >      }
>> >
>> >      status->num_vqs = virtio_get_num_queues(vdev);
>> > +    if (display_bits) {
>> > +        status->status_bits = vdev->status;
>> > +    }
>> >      status->status = qmp_decode_status(vdev->status);
>> >      status->isr = vdev->isr;
>> >      status->queue_sel = vdev->queue_sel;
>> > @@ -775,6 +788,12 @@ 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;
>> > +        if (display_bits) {
>> > +            status->vhost_dev->features_bits = hdev->features;
>> > +            status->vhost_dev->acked_features_bits = hdev->acked_features;
>> > +            status->vhost_dev->backend_features_bits = hdev->backend_features;
>> > +            status->vhost_dev->protocol_features_bits = hdev->protocol_features;
>> > +        }
>> >          status->vhost_dev->features =
>> >              qmp_decode_features(vdev->device_id, hdev->features);
>> >          status->vhost_dev->acked_features =
>> > diff --git a/qapi/virtio.json b/qapi/virtio.json
>> > index e6dcee7b83..608b841a89 100644
>> > --- a/qapi/virtio.json
>> > +++ b/qapi/virtio.json
>> > @@ -79,12 +79,20 @@
>> >  #
>> >  # @vq-index: vhost_dev vq_index
>> >  #
>> > +# @features-bits: vhost_dev features in decimal format
>>
>> Say "encoded as a number".  The number is decimal just because the
>> transport is JSON.  We could have another transport some day.  Or
>> language bindings wrapping around the JSON transport.
>>
>> > +#
>> >  # @features: vhost_dev features
>>
>> Double-checking...  @feature-bits provides the exact same information as
>> @features, only in another encoding.  Correct?
>
>
>> Same for all the other new -bits.  Correct?
>
> Yes, all the new fields are only about providing another encoding.

Why do we want to return the same information in two different
encodings?  I figure the commit message tries to answer this question:

     Features and status bits could be helpful for applications to compare
     directly. For instance, when an upper application aims to ensure the
     virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
     the "ovs-vsctl list interface" command to retrieve interface features
     (in number format) and the QMP command x-query-virtio-status to retrieve
     vhost-user net device features. If "show-bits" is added, the application
     can compare the two features directly; No need to encoding the features
     returned by the QMP command.

     This patch also serves as a preparation for the next one, which implements
     a vhost-user test case about acked features of vhost-user protocol.

I guess you're trying to simplify use cases where the QMP client wants
to compare entire feature sets without caring for individual features.

The comparison is easy if both sets are represented the same way,
e.g. both are numbers, or both are lists of symbols.

With different representations, we first have to map to a common
representation.  Unfortunately, the design of x-query-virtio-status
makes this harder than it should be.

We use QAPI types VirtioDeviceStatus, VhostDeviceProtocols,
VirtioDeviceFeatures to represent feature sets.  They all work the same
way: array of strings plus a number.  For each bit QEMU knows, there's a
string in the array.  Any remaining bits go into the number.

The format of the string is undocumented.  They look like

    "WELL_KNOWN_SYMBOL: human readable explanation"

Mapping from bit to this string in a client would require duplicating
QEMU's code exactly.

Mapping both bit and string to just "WELL_KNOWN_SYMBOL" could perhaps be
done.

The mapping between symbols and bits is not visible in QMP.  Mapping
from string to bit requires exploiting the undocumented format: extract
the well-known symbol and decode it.

This encoding of feature sets goes back to commit f3034ad71fc (qmp:
decode feature & status bits in virtio-status) v7.2.  Before that, the
command returned the bits as a number.

For example, return value "member "status":

    Before f3034ad71fc:

        "status": 15,

    Since f3034ad71fc:

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

    With your patch:

        "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-bits": 15,

Looks like commit f3034ad71fc improved one use case at the expense of
another, and your patch tries to revert the damage.  Which one exactly
it improved is unclear; the commit message doesn't tell.  Laurent?

[...]

Hyman Huang Nov. 21, 2023, 10:02 a.m. UTC | #4

On Tue, Nov 21, 2023 at 3:58 PM Markus Armbruster <[e-mail]> wrote:

> Laurent, there's a question for you at the end.
>
> Yong Huang <[e-mail]> writes:
>
> > On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]>
> > wrote:
> >
> >> Hyman Huang <[e-mail]> writes:
> >>
> >> > This patch allows to display feature and status bits in virtio-status.
> >> >
> >> > An optional argument is introduced: show-bits. For example:
> >> > {"execute": "x-query-virtio-status",
> >> >  "arguments": {"path":
> "/machine/peripheral-anon/device[1]/virtio-backend",
> >> >                "show-bits": true}
> >> >
> >> > Features and status bits could be helpful for applications to compare
> >> > directly. For instance, when an upper application aims to ensure the
> >> > virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it
> use
> >> > the "ovs-vsctl list interface" command to retrieve interface features
> >> > (in number format) and the QMP command x-query-virtio-status to
> retrieve
> >> > vhost-user net device features. If "show-bits" is added, the
> application
> >> > can compare the two features directly; No need to encoding the
> features
> >> > returned by the QMP command.
> >> >
> >> > This patch also serves as a preparation for the next one, which
> implements
> >> > a vhost-user test case about acked features of vhost-user protocol.
> >> >
> >> > Note that since the matching HMP command is typically used for human,
> >> > leave it unchanged.
> >> >
> >> > Signed-off-by: Hyman Huang <[e-mail]>
> >> > ---
> >> >  hw/virtio/virtio-hmp-cmds.c |  2 +-
> >> >  hw/virtio/virtio-qmp.c      | 21 +++++++++++++++-
> >> >  qapi/virtio.json            | 49
> ++++++++++++++++++++++++++++++++++---
> >> >  3 files changed, 67 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
> >> > index 477c97dea2..3774f3d4bf 100644
> >> > --- a/hw/virtio/virtio-hmp-cmds.c
> >> > +++ b/hw/virtio/virtio-hmp-cmds.c
> >> > @@ -108,7 +108,7 @@ void hmp_virtio_status(Monitor *mon, const QDict
> *qdict)
> >> >  {
> >> >      Error *err = NULL;
> >> >      const char *path = qdict_get_try_str(qdict, "path");
> >> > -    VirtioStatus *s = qmp_x_query_virtio_status(path, &err);
> >> > +    VirtioStatus *s = qmp_x_query_virtio_status(path, false, false,
> &err);
> >> >
> >> >      if (err != NULL) {
> >> >          hmp_handle_error(mon, err);
> >> > diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> >> > index 1dd96ed20f..2e92bf28ac 100644
> >> > --- a/hw/virtio/virtio-qmp.c
> >> > +++ b/hw/virtio/virtio-qmp.c
> >> > @@ -718,10 +718,15 @@ VirtIODevice *qmp_find_virtio_device(const char
> *path)
> >> >      return VIRTIO_DEVICE(dev);
> >> >  }
> >> >
> >> > -VirtioStatus *qmp_x_query_virtio_status(const char *path, Error
> **errp)
> >> > +VirtioStatus *qmp_x_query_virtio_status(const char *path,
> >> > +                                        bool has_show_bits,
> >> > +                                        bool show_bits,
> >> > +                                        Error **errp)
> >> >  {
> >> >      VirtIODevice *vdev;
> >> >      VirtioStatus *status;
> >> > +    bool display_bits =
> >> > +        has_show_bits ? show_bits : false;
> >>
> >> Since !has_show_bits implies !show_bits, you can simply use
> >> if (show_bits).
> >>
> > Ok
> >
> >>
> >> >
> >> >      vdev = qmp_find_virtio_device(path);
> >> >      if (vdev == NULL) {
> >> > @@ -733,6 +738,11 @@ 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;
> >> > +    if (display_bits) {
> >> > +        status->guest_features_bits = vdev->guest_features;
> >> > +        status->host_features_bits = vdev->host_features;
> >> > +        status->backend_features_bits = vdev->backend_features;
> >> > +    }
> >> >      status->guest_features = qmp_decode_features(vdev->device_id,
> >> >
>  vdev->guest_features);
> >> >      status->host_features = qmp_decode_features(vdev->device_id,
> >> > @@ -753,6 +763,9 @@ VirtioStatus *qmp_x_query_virtio_status(const
> char *path, Error **errp)
> >> >      }
> >> >
> >> >      status->num_vqs = virtio_get_num_queues(vdev);
> >> > +    if (display_bits) {
> >> > +        status->status_bits = vdev->status;
> >> > +    }
> >> >      status->status = qmp_decode_status(vdev->status);
> >> >      status->isr = vdev->isr;
> >> >      status->queue_sel = vdev->queue_sel;
> >> > @@ -775,6 +788,12 @@ 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;
> >> > +        if (display_bits) {
> >> > +            status->vhost_dev->features_bits = hdev->features;
> >> > +            status->vhost_dev->acked_features_bits =
> hdev->acked_features;
> >> > +            status->vhost_dev->backend_features_bits =
> hdev->backend_features;
> >> > +            status->vhost_dev->protocol_features_bits =
> hdev->protocol_features;
> >> > +        }
> >> >          status->vhost_dev->features =
> >> >              qmp_decode_features(vdev->device_id, hdev->features);
> >> >          status->vhost_dev->acked_features =
> >> > diff --git a/qapi/virtio.json b/qapi/virtio.json
> >> > index e6dcee7b83..608b841a89 100644
> >> > --- a/qapi/virtio.json
> >> > +++ b/qapi/virtio.json
> >> > @@ -79,12 +79,20 @@
> >> >  #
> >> >  # @vq-index: vhost_dev vq_index
> >> >  #
> >> > +# @features-bits: vhost_dev features in decimal format
> >>
> >> Say "encoded as a number".  The number is decimal just because the
> >> transport is JSON.  We could have another transport some day.  Or
> >> language bindings wrapping around the JSON transport.
> >>
> >> > +#
> >> >  # @features: vhost_dev features
> >>
> >> Double-checking...  @feature-bits provides the exact same information as
> >> @features, only in another encoding.  Correct?
> >
> >
> >> Same for all the other new -bits.  Correct?
> >
> > Yes, all the new fields are only about providing another encoding.
>
> Why do we want to return the same information in two different
> encodings?  I figure the commit message tries to answer this question:
>
>      Features and status bits could be helpful for applications to compare
>      directly. For instance, when an upper application aims to ensure the
>      virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it
> use
>      the "ovs-vsctl list interface" command to retrieve interface features
>      (in number format) and the QMP command x-query-virtio-status to
> retrieve
>      vhost-user net device features. If "show-bits" is added, the
> application
>      can compare the two features directly; No need to encoding the
> features
>      returned by the QMP command.
>
>      This patch also serves as a preparation for the next one, which
> implements
>      a vhost-user test case about acked features of vhost-user protocol.
>
> I guess you're trying to simplify use cases where the QMP client wants
> to compare entire feature sets without caring for individual features.
>
> The comparison is easy if both sets are represented the same way,
> e.g. both are numbers, or both are lists of symbols.


Yes,  quite correct, this patch just aims to keep the unencoded bits for
easy comparison.

>


> With different representations, we first have to map to a common
> representation.  Unfortunately, the design of x-query-virtio-status
> makes this harder than it should be.
>
> We use QAPI types VirtioDeviceStatus, VhostDeviceProtocols,
> VirtioDeviceFeatures to represent feature sets.  They all work the same
> way: array of strings plus a number.  For each bit QEMU knows, there's a
> string in the array.  Any remaining bits go into the number.
>
> The format of the string is undocumented.  They look like
>
>     "WELL_KNOWN_SYMBOL: human readable explanation"
>
> Mapping from bit to this string in a client would require duplicating
> QEMU's code exactly.
>
> Mapping both bit and string to just "WELL_KNOWN_SYMBOL" could perhaps be
> done.
>
> The mapping between symbols and bits is not visible in QMP.  Mapping
> from string to bit requires exploiting the undocumented format: extract
> the well-known symbol and decode it.
>
> This encoding of feature sets goes back to commit f3034ad71fc (qmp:
> decode feature & status bits in virtio-status) v7.2.  Before that, the
> command returned the bits as a number.
>
> For example, return value "member "status":
>
>     Before f3034ad71fc:
>
>         "status": 15,
>
>     Since f3034ad71fc:
>
>         "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"
>             ]},
>
>     With your patch:
>
>         "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-bits": 15,
>
> Looks like commit f3034ad71fc improved one use case at the expense of
> another, and your patch tries to revert the damage.  Which one exactly
> it improved is unclear; the commit message doesn't tell.  Laurent?
>
> [...]
>
>

Laurent Vivier Dec. 1, 2023, 10:41 a.m. UTC | #5

On 11/21/23 08:58, Markus Armbruster wrote:
> Laurent, there's a question for you at the end.
> 
> Yong Huang <[e-mail]> writes:
> 
>> On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]>
>> wrote:
>>
>>> Hyman Huang <[e-mail]> writes:
>>>
>>>> This patch allows to display feature and status bits in virtio-status.
>>>>
>>>> An optional argument is introduced: show-bits. For example:
>>>> {"execute": "x-query-virtio-status",
>>>>   "arguments": {"path": "/machine/peripheral-anon/device[1]/virtio-backend",
>>>>                 "show-bits": true}
>>>>
>>>> Features and status bits could be helpful for applications to compare
>>>> directly. For instance, when an upper application aims to ensure the
>>>> virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>>>> the "ovs-vsctl list interface" command to retrieve interface features
>>>> (in number format) and the QMP command x-query-virtio-status to retrieve
>>>> vhost-user net device features. If "show-bits" is added, the application
>>>> can compare the two features directly; No need to encoding the features
>>>> returned by the QMP command.
>>>>
>>>> This patch also serves as a preparation for the next one, which implements
>>>> a vhost-user test case about acked features of vhost-user protocol.
>>>>
>>>> Note that since the matching HMP command is typically used for human,
>>>> leave it unchanged.
>>>>
>>>> Signed-off-by: Hyman Huang <[e-mail]>
>>>> ---
>>>>   hw/virtio/virtio-hmp-cmds.c |  2 +-
>>>>   hw/virtio/virtio-qmp.c      | 21 +++++++++++++++-
>>>>   qapi/virtio.json            | 49 ++++++++++++++++++++++++++++++++++---
>>>>   3 files changed, 67 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
>>>> index 477c97dea2..3774f3d4bf 100644
>>>> --- a/hw/virtio/virtio-hmp-cmds.c
>>>> +++ b/hw/virtio/virtio-hmp-cmds.c
>>>> @@ -108,7 +108,7 @@ void hmp_virtio_status(Monitor *mon, const QDict *qdict)
>>>>   {
>>>>       Error *err = NULL;
>>>>       const char *path = qdict_get_try_str(qdict, "path");
>>>> -    VirtioStatus *s = qmp_x_query_virtio_status(path, &err);
>>>> +    VirtioStatus *s = qmp_x_query_virtio_status(path, false, false, &err);
>>>>
>>>>       if (err != NULL) {
>>>>           hmp_handle_error(mon, err);
>>>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>>>> index 1dd96ed20f..2e92bf28ac 100644
>>>> --- a/hw/virtio/virtio-qmp.c
>>>> +++ b/hw/virtio/virtio-qmp.c
>>>> @@ -718,10 +718,15 @@ VirtIODevice *qmp_find_virtio_device(const char *path)
>>>>       return VIRTIO_DEVICE(dev);
>>>>   }
>>>>
>>>> -VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>>>> +VirtioStatus *qmp_x_query_virtio_status(const char *path,
>>>> +                                        bool has_show_bits,
>>>> +                                        bool show_bits,
>>>> +                                        Error **errp)
>>>>   {
>>>>       VirtIODevice *vdev;
>>>>       VirtioStatus *status;
>>>> +    bool display_bits =
>>>> +        has_show_bits ? show_bits : false;
>>>
>>> Since !has_show_bits implies !show_bits, you can simply use
>>> if (show_bits).
>>>
>> Ok
>>
>>>
>>>>
>>>>       vdev = qmp_find_virtio_device(path);
>>>>       if (vdev == NULL) {
>>>> @@ -733,6 +738,11 @@ 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;
>>>> +    if (display_bits) {
>>>> +        status->guest_features_bits = vdev->guest_features;
>>>> +        status->host_features_bits = vdev->host_features;
>>>> +        status->backend_features_bits = vdev->backend_features;
>>>> +    }
>>>>       status->guest_features = qmp_decode_features(vdev->device_id,
>>>>                                                    vdev->guest_features);
>>>>       status->host_features = qmp_decode_features(vdev->device_id,
>>>> @@ -753,6 +763,9 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>>>>       }
>>>>
>>>>       status->num_vqs = virtio_get_num_queues(vdev);
>>>> +    if (display_bits) {
>>>> +        status->status_bits = vdev->status;
>>>> +    }
>>>>       status->status = qmp_decode_status(vdev->status);
>>>>       status->isr = vdev->isr;
>>>>       status->queue_sel = vdev->queue_sel;
>>>> @@ -775,6 +788,12 @@ 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;
>>>> +        if (display_bits) {
>>>> +            status->vhost_dev->features_bits = hdev->features;
>>>> +            status->vhost_dev->acked_features_bits = hdev->acked_features;
>>>> +            status->vhost_dev->backend_features_bits = hdev->backend_features;
>>>> +            status->vhost_dev->protocol_features_bits = hdev->protocol_features;
>>>> +        }
>>>>           status->vhost_dev->features =
>>>>               qmp_decode_features(vdev->device_id, hdev->features);
>>>>           status->vhost_dev->acked_features =
>>>> diff --git a/qapi/virtio.json b/qapi/virtio.json
>>>> index e6dcee7b83..608b841a89 100644
>>>> --- a/qapi/virtio.json
>>>> +++ b/qapi/virtio.json
>>>> @@ -79,12 +79,20 @@
>>>>   #
>>>>   # @vq-index: vhost_dev vq_index
>>>>   #
>>>> +# @features-bits: vhost_dev features in decimal format
>>>
>>> Say "encoded as a number".  The number is decimal just because the
>>> transport is JSON.  We could have another transport some day.  Or
>>> language bindings wrapping around the JSON transport.
>>>
>>>> +#
>>>>   # @features: vhost_dev features
>>>
>>> Double-checking...  @feature-bits provides the exact same information as
>>> @features, only in another encoding.  Correct?
>>
>>
>>> Same for all the other new -bits.  Correct?
>>
>> Yes, all the new fields are only about providing another encoding.
> 
> Why do we want to return the same information in two different
> encodings?  I figure the commit message tries to answer this question:
> 
>       Features and status bits could be helpful for applications to compare
>       directly. For instance, when an upper application aims to ensure the
>       virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>       the "ovs-vsctl list interface" command to retrieve interface features
>       (in number format) and the QMP command x-query-virtio-status to retrieve
>       vhost-user net device features. If "show-bits" is added, the application
>       can compare the two features directly; No need to encoding the features
>       returned by the QMP command.
> 
>       This patch also serves as a preparation for the next one, which implements
>       a vhost-user test case about acked features of vhost-user protocol.
> 
> I guess you're trying to simplify use cases where the QMP client wants
> to compare entire feature sets without caring for individual features.
> 
> The comparison is easy if both sets are represented the same way,
> e.g. both are numbers, or both are lists of symbols.
> 
> With different representations, we first have to map to a common
> representation.  Unfortunately, the design of x-query-virtio-status
> makes this harder than it should be.
> 
> We use QAPI types VirtioDeviceStatus, VhostDeviceProtocols,
> VirtioDeviceFeatures to represent feature sets.  They all work the same
> way: array of strings plus a number.  For each bit QEMU knows, there's a
> string in the array.  Any remaining bits go into the number.
> 
> The format of the string is undocumented.  They look like
> 
>      "WELL_KNOWN_SYMBOL: human readable explanation"
> 
> Mapping from bit to this string in a client would require duplicating
> QEMU's code exactly.
> 
> Mapping both bit and string to just "WELL_KNOWN_SYMBOL" could perhaps be
> done.
> 
> The mapping between symbols and bits is not visible in QMP.  Mapping
> from string to bit requires exploiting the undocumented format: extract
> the well-known symbol and decode it.
> 
> This encoding of feature sets goes back to commit f3034ad71fc (qmp:
> decode feature & status bits in virtio-status) v7.2.  Before that, the
> command returned the bits as a number.
> 
> For example, return value "member "status":
> 
>      Before f3034ad71fc:
> 
>          "status": 15,
> 
>      Since f3034ad71fc:
> 
>          "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"
>              ]},
> 
>      With your patch:
> 
>          "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-bits": 15,
> 
> Looks like commit f3034ad71fc improved one use case at the expense of
> another, and your patch tries to revert the damage.  Which one exactly
> it improved is unclear; the commit message doesn't tell.  Laurent?
> 
> [...]
> 

The first idea of the series "hmp,qmp: Add commands to introspect virtio devices" 
including commit f3034ad71fc was to help developer to debug virtio devices, so for this 
purpose it was interesting to display the status in a human readable manner.

Of course, if you want to run automatic tests and be able to compare the result to have 
the status bits result seems to be better.

As these are two different use cases, it's understandable to have two different 
representations of the same information.

Thanks,
Laurent

Markus Armbruster Dec. 1, 2023, 3:21 p.m. UTC | #6

Laurent Vivier <[e-mail]> writes:

> On 11/21/23 08:58, Markus Armbruster wrote:
>> Laurent, there's a question for you at the end.
>> 
>> Yong Huang <[e-mail]> writes:
>> 
>>> On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]>
>>> wrote:
>>>
>>>> Hyman Huang <[e-mail]> writes:
>>>>
>>>>> This patch allows to display feature and status bits in virtio-status.
>>>>>
>>>>> An optional argument is introduced: show-bits. For example:
>>>>> {"execute": "x-query-virtio-status",
>>>>>   "arguments": {"path": "/machine/peripheral-anon/device[1]/virtio-backend",
>>>>>                 "show-bits": true}
>>>>>
>>>>> Features and status bits could be helpful for applications to compare
>>>>> directly. For instance, when an upper application aims to ensure the
>>>>> virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>>>>> the "ovs-vsctl list interface" command to retrieve interface features
>>>>> (in number format) and the QMP command x-query-virtio-status to retrieve
>>>>> vhost-user net device features. If "show-bits" is added, the application
>>>>> can compare the two features directly; No need to encoding the features
>>>>> returned by the QMP command.
>>>>>
>>>>> This patch also serves as a preparation for the next one, which implements
>>>>> a vhost-user test case about acked features of vhost-user protocol.
>>>>>
>>>>> Note that since the matching HMP command is typically used for human,
>>>>> leave it unchanged.
>>>>>
>>>>> Signed-off-by: Hyman Huang <[e-mail]>

[...]

>>>> Double-checking...  @feature-bits provides the exact same information as
>>>> @features, only in another encoding.  Correct?
>>>
>>>
>>>> Same for all the other new -bits.  Correct?
>>>
>>> Yes, all the new fields are only about providing another encoding.
>> 
>> Why do we want to return the same information in two different
>> encodings?  I figure the commit message tries to answer this question:
>> 
>>       Features and status bits could be helpful for applications to compare
>>       directly. For instance, when an upper application aims to ensure the
>>       virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>>       the "ovs-vsctl list interface" command to retrieve interface features
>>       (in number format) and the QMP command x-query-virtio-status to retrieve
>>       vhost-user net device features. If "show-bits" is added, the application
>>       can compare the two features directly; No need to encoding the features
>>       returned by the QMP command.
>> 
>>       This patch also serves as a preparation for the next one, which implements
>>       a vhost-user test case about acked features of vhost-user protocol.
>> 
>> I guess you're trying to simplify use cases where the QMP client wants
>> to compare entire feature sets without caring for individual features.
>> 
>> The comparison is easy if both sets are represented the same way,
>> e.g. both are numbers, or both are lists of symbols.
>> 
>> With different representations, we first have to map to a common
>> representation.  Unfortunately, the design of x-query-virtio-status
>> makes this harder than it should be.
>> 
>> We use QAPI types VirtioDeviceStatus, VhostDeviceProtocols,
>> VirtioDeviceFeatures to represent feature sets.  They all work the same
>> way: array of strings plus a number.  For each bit QEMU knows, there's a
>> string in the array.  Any remaining bits go into the number.
>> 
>> The format of the string is undocumented.  They look like
>> 
>>      "WELL_KNOWN_SYMBOL: human readable explanation"
>> 
>> Mapping from bit to this string in a client would require duplicating
>> QEMU's code exactly.
>> 
>> Mapping both bit and string to just "WELL_KNOWN_SYMBOL" could perhaps be
>> done.
>> 
>> The mapping between symbols and bits is not visible in QMP.  Mapping
>> from string to bit requires exploiting the undocumented format: extract
>> the well-known symbol and decode it.
>> 
>> This encoding of feature sets goes back to commit f3034ad71fc (qmp:
>> decode feature & status bits in virtio-status) v7.2.  Before that, the
>> command returned the bits as a number.
>> 
>> For example, return value "member "status":
>> 
>>      Before f3034ad71fc:
>> 
>>          "status": 15,
>> 
>>      Since f3034ad71fc:
>> 
>>          "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"
>>              ]},
>> 
>>      With your patch:
>> 
>>          "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-bits": 15,
>> 
>> Looks like commit f3034ad71fc improved one use case at the expense of
>> another, and your patch tries to revert the damage.  Which one exactly
>> it improved is unclear; the commit message doesn't tell.  Laurent?
>> 
>> [...]
>> 
>
> The first idea of the series "hmp,qmp: Add commands to introspect virtio devices" 
> including commit f3034ad71fc was to help developer to debug virtio devices, so for this 
> purpose it was interesting to display the status in a human readable manner.
>
> Of course, if you want to run automatic tests and be able to compare the result to have 
> the status bits result seems to be better.
>
> As these are two different use cases, it's understandable to have two different 
> representations of the same information.

Thanks!

Both use cases are valid, but I dislike both the existing and the
proposed interface.

We can change it: x-query-virtio-status isn't stable (it's for debugging
and testing).  But even unstable interfaces should only be changed for
good, clear reasons.

I feel the change from "bits encoded as a number" to "bits as list of
descriptive strings plus number for the unknown ones" fell short.  Let
me explain.

The initial version of the command had "bits encoded as number".  Unless
we understand why that was done, we should assume it was done for a
reason.  We now know it was: Hyman Huang posted a patch to get it back.

Instead of "bits as list of descriptive strings plus number for the
unknown ones", we could have done "bits encoded as number, plus list of
descriptive strings", or plus some other human-readable encoding.

QMP output of the form "WELL_KNOWN_SYMBOL: human readable explanation"
smells of encoding structured information in strings, which is a no-no.

Perhaps we could have added human-readable output just in HMP.  That's
what we normally do.

Here are a few possible alternatives to Hyman Huang's patch:

1. Revert commit f3034ad71fc for QMP, keep it for HMP.

2. Replace @unknown-FOO (just the unknown bits) by @FOO-bits (all bits).

3. Add @FOO-bits next to @unknown-FOO, deprecate @unknown-FOO.

4. Create a QAPI enum for the known bits.  Clients can use introspection
   to learn the mapping between symbols and bits.  Requires dumbing down
   the descriptive strings to just the symbols.  This feels
   both overengineered and cumbersome to use.

For 2 and 3, I'd prefer to also dumb down the descriptive strings to
just the symbols.

Thoughts?

Laurent Vivier Dec. 1, 2023, 3:37 p.m. UTC | #7

On 12/1/23 16:21, Markus Armbruster wrote:
> Laurent Vivier <[e-mail]> writes:
> 
>> On 11/21/23 08:58, Markus Armbruster wrote:
>>> Laurent, there's a question for you at the end.
>>>
>>> Yong Huang <[e-mail]> writes:
>>>
>>>> On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]>
>>>> wrote:
>>>>
>>>>> Hyman Huang <[e-mail]> writes:
>>>>>
>>>>>> This patch allows to display feature and status bits in virtio-status.
>>>>>>
>>>>>> An optional argument is introduced: show-bits. For example:
>>>>>> {"execute": "x-query-virtio-status",
>>>>>>    "arguments": {"path": "/machine/peripheral-anon/device[1]/virtio-backend",
>>>>>>                  "show-bits": true}
>>>>>>
>>>>>> Features and status bits could be helpful for applications to compare
>>>>>> directly. For instance, when an upper application aims to ensure the
>>>>>> virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>>>>>> the "ovs-vsctl list interface" command to retrieve interface features
>>>>>> (in number format) and the QMP command x-query-virtio-status to retrieve
>>>>>> vhost-user net device features. If "show-bits" is added, the application
>>>>>> can compare the two features directly; No need to encoding the features
>>>>>> returned by the QMP command.
>>>>>>
>>>>>> This patch also serves as a preparation for the next one, which implements
>>>>>> a vhost-user test case about acked features of vhost-user protocol.
>>>>>>
>>>>>> Note that since the matching HMP command is typically used for human,
>>>>>> leave it unchanged.
>>>>>>
>>>>>> Signed-off-by: Hyman Huang <[e-mail]>
> 
> [...]
> 
>>>>> Double-checking...  @feature-bits provides the exact same information as
>>>>> @features, only in another encoding.  Correct?
>>>>
>>>>
>>>>> Same for all the other new -bits.  Correct?
>>>>
>>>> Yes, all the new fields are only about providing another encoding.
>>>
>>> Why do we want to return the same information in two different
>>> encodings?  I figure the commit message tries to answer this question:
>>>
>>>        Features and status bits could be helpful for applications to compare
>>>        directly. For instance, when an upper application aims to ensure the
>>>        virtio negotiation correctness between guest, QEMU, and OVS-DPDK, it use
>>>        the "ovs-vsctl list interface" command to retrieve interface features
>>>        (in number format) and the QMP command x-query-virtio-status to retrieve
>>>        vhost-user net device features. If "show-bits" is added, the application
>>>        can compare the two features directly; No need to encoding the features
>>>        returned by the QMP command.
>>>
>>>        This patch also serves as a preparation for the next one, which implements
>>>        a vhost-user test case about acked features of vhost-user protocol.
>>>
>>> I guess you're trying to simplify use cases where the QMP client wants
>>> to compare entire feature sets without caring for individual features.
>>>
>>> The comparison is easy if both sets are represented the same way,
>>> e.g. both are numbers, or both are lists of symbols.
>>>
>>> With different representations, we first have to map to a common
>>> representation.  Unfortunately, the design of x-query-virtio-status
>>> makes this harder than it should be.
>>>
>>> We use QAPI types VirtioDeviceStatus, VhostDeviceProtocols,
>>> VirtioDeviceFeatures to represent feature sets.  They all work the same
>>> way: array of strings plus a number.  For each bit QEMU knows, there's a
>>> string in the array.  Any remaining bits go into the number.
>>>
>>> The format of the string is undocumented.  They look like
>>>
>>>       "WELL_KNOWN_SYMBOL: human readable explanation"
>>>
>>> Mapping from bit to this string in a client would require duplicating
>>> QEMU's code exactly.
>>>
>>> Mapping both bit and string to just "WELL_KNOWN_SYMBOL" could perhaps be
>>> done.
>>>
>>> The mapping between symbols and bits is not visible in QMP.  Mapping
>>> from string to bit requires exploiting the undocumented format: extract
>>> the well-known symbol and decode it.
>>>
>>> This encoding of feature sets goes back to commit f3034ad71fc (qmp:
>>> decode feature & status bits in virtio-status) v7.2.  Before that, the
>>> command returned the bits as a number.
>>>
>>> For example, return value "member "status":
>>>
>>>       Before f3034ad71fc:
>>>
>>>           "status": 15,
>>>
>>>       Since f3034ad71fc:
>>>
>>>           "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"
>>>               ]},
>>>
>>>       With your patch:
>>>
>>>           "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-bits": 15,
>>>
>>> Looks like commit f3034ad71fc improved one use case at the expense of
>>> another, and your patch tries to revert the damage.  Which one exactly
>>> it improved is unclear; the commit message doesn't tell.  Laurent?
>>>
>>> [...]
>>>
>>
>> The first idea of the series "hmp,qmp: Add commands to introspect virtio devices"
>> including commit f3034ad71fc was to help developer to debug virtio devices, so for this
>> purpose it was interesting to display the status in a human readable manner.
>>
>> Of course, if you want to run automatic tests and be able to compare the result to have
>> the status bits result seems to be better.
>>
>> As these are two different use cases, it's understandable to have two different
>> representations of the same information.
> 
> Thanks!
> 
> Both use cases are valid, but I dislike both the existing and the
> proposed interface.
> 
> We can change it: x-query-virtio-status isn't stable (it's for debugging
> and testing).  But even unstable interfaces should only be changed for
> good, clear reasons.
> 
> I feel the change from "bits encoded as a number" to "bits as list of
> descriptive strings plus number for the unknown ones" fell short.  Let
> me explain.
> 
> The initial version of the command had "bits encoded as number".  Unless
> we understand why that was done, we should assume it was done for a
> reason.  We now know it was: Hyman Huang posted a patch to get it back.
> 
> Instead of "bits as list of descriptive strings plus number for the
> unknown ones", we could have done "bits encoded as number, plus list of
> descriptive strings", or plus some other human-readable encoding.
> 
> QMP output of the form "WELL_KNOWN_SYMBOL: human readable explanation"
> smells of encoding structured information in strings, which is a no-no.
> 
> Perhaps we could have added human-readable output just in HMP.  That's
> what we normally do.
> 
> Here are a few possible alternatives to Hyman Huang's patch:
> 
> 1. Revert commit f3034ad71fc for QMP, keep it for HMP.
> 
> 2. Replace @unknown-FOO (just the unknown bits) by @FOO-bits (all bits).
> 
> 3. Add @FOO-bits next to @unknown-FOO, deprecate @unknown-FOO.
> 
> 4. Create a QAPI enum for the known bits.  Clients can use introspection
>     to learn the mapping between symbols and bits.  Requires dumbing down
>     the descriptive strings to just the symbols.  This feels
>     both overengineered and cumbersome to use.
> 
> For 2 and 3, I'd prefer to also dumb down the descriptive strings to
> just the symbols.
> 
> Thoughts?
> 

I agree with you. As x-CMD are unstable, perhaps we can go directly to 2?
(and of course to remove the descriptive strings. Is it easily possible to keep them for 
the HMP version?)

Thanks,
Laurent

Markus Armbruster Dec. 6, 2023, 7:35 a.m. UTC | #8

Laurent Vivier <[e-mail]> writes:

> On 12/1/23 16:21, Markus Armbruster wrote:

[...]

>> Both use cases are valid, but I dislike both the existing and the
>> proposed interface.
>>
>> We can change it: x-query-virtio-status isn't stable (it's for debugging
>> and testing).  But even unstable interfaces should only be changed for
>> good, clear reasons.
>>
>> I feel the change from "bits encoded as a number" to "bits as list of
>> descriptive strings plus number for the unknown ones" fell short.  Let
>> me explain.
>>
>> The initial version of the command had "bits encoded as number".  Unless
>> we understand why that was done, we should assume it was done for a
>> reason.  We now know it was: Hyman Huang posted a patch to get it back.
>>
>> Instead of "bits as list of descriptive strings plus number for the
>> unknown ones", we could have done "bits encoded as number, plus list of
>> descriptive strings", or plus some other human-readable encoding.
>>
>> QMP output of the form "WELL_KNOWN_SYMBOL: human readable explanation"
>> smells of encoding structured information in strings, which is a no-no.
>>
>> Perhaps we could have added human-readable output just in HMP.  That's
>> what we normally do.
>>
>> Here are a few possible alternatives to Hyman Huang's patch:
>>
>> 1. Revert commit f3034ad71fc for QMP, keep it for HMP.
>>
>> 2. Replace @unknown-FOO (just the unknown bits) by @FOO-bits (all bits).
>>
>> 3. Add @FOO-bits next to @unknown-FOO, deprecate @unknown-FOO.
>>
>> 4. Create a QAPI enum for the known bits.  Clients can use introspection
>>    to learn the mapping between symbols and bits.  Requires dumbing down
>>    the descriptive strings to just the symbols.  This feels
>>    both overengineered and cumbersome to use.
>>
>> For 2 and 3, I'd prefer to also dumb down the descriptive strings to
>> just the symbols.
>>
>> Thoughts?
>> 
>
> I agree with you. As x-CMD are unstable, perhaps we can go directly to 2?

We can.  It might incovenience existing users of @unknown-FOO.

> (and of course to remove the descriptive strings. Is it easily possible to keep them for the HMP version?)

We could change qmp_virtio_feature_map_t to

    typedef struct {
        int virtio_bit;
        const char *feature_name;
        const char *feature_desc;
    } qmp_virtio_feature_map_t;

and FEATURE_ENTRY() to

    #define FEATURE_ENTRY(bit, name, desc) (qmp_virtio_feature_map_t) \
        { .virtio_bit = (bit), .feature_name = (name), .feature_desc = (desc) }

Aside: POSIX reserves names ending with _t.  An actual clash is of
course vanishingly unlikely.  But avoiding _t suffix is a good habit.

qmp_x_query_virtio_status() could then convert bits to a list of known names
using .feature_name.

To keep the descriptions in HMP, simply print the bits using
.feature_name and .feature_desc, ignoring list of known names returned
by qmp_x_query_virtio_status().

Alternatively, make the code to convert bits to list of strings flexible
enough to be usable in both places.

If qmp_virtio_feature_map_t is still only used in virtio-qmp.c, move its
there.

Hyman Huang Dec. 9, 2023, 5:03 a.m. UTC | #9

On Fri, Dec 1, 2023 at 11:37 PM Laurent Vivier <[e-mail]> wrote:

> On 12/1/23 16:21, Markus Armbruster wrote:
> > Laurent Vivier <[e-mail]> writes:
> >
> >> On 11/21/23 08:58, Markus Armbruster wrote:
> >>> Laurent, there's a question for you at the end.
> >>>
> >>> Yong Huang <[e-mail]> writes:
> >>>
> >>>> On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <[e-mail]
> >
> >>>> wrote:
> >>>>
> >>>>> Hyman Huang <[e-mail]> writes:
> >>>>>
> >>>>>> This patch allows to display feature and status bits in
> virtio-status.
> >>>>>>
> >>>>>> An optional argument is introduced: show-bits. For example:
> >>>>>> {"execute": "x-query-virtio-status",
> >>>>>>    "arguments": {"path":
> "/machine/peripheral-anon/device[1]/virtio-backend",
> >>>>>>                  "show-bits": true}
> >>>>>>
> >>>>>> Features and status bits could be helpful for applications to
> compare
> >>>>>> directly. For instance, when an upper application aims to ensure the
> >>>>>> virtio negotiation correctness between guest, QEMU, and OVS-DPDK,
> it use
> >>>>>> the "ovs-vsctl list interface" command to retrieve interface
> features
> >>>>>> (in number format) and the QMP command x-query-virtio-status to
> retrieve
> >>>>>> vhost-user net device features. If "show-bits" is added, the
> application
> >>>>>> can compare the two features directly; No need to encoding the
> features
> >>>>>> returned by the QMP command.
> >>>>>>
> >>>>>> This patch also serves as a preparation for the next one, which
> implements
> >>>>>> a vhost-user test case about acked features of vhost-user protocol.
> >>>>>>
> >>>>>> Note that since the matching HMP command is typically used for
> human,
> >>>>>> leave it unchanged.
> >>>>>>
> >>>>>> Signed-off-by: Hyman Huang <[e-mail]>
> >
> > [...]
> >
> >>>>> Double-checking...  @feature-bits provides the exact same
> information as
> >>>>> @features, only in another encoding.  Correct?
> >>>>
> >>>>
> >>>>> Same for all the other new -bits.  Correct?
> >>>>
> >>>> Yes, all the new fields are only about providing another encoding.
> >>>
> >>> Why do we want to return the same information in two different
> >>> encodings?  I figure the commit message tries to answer this question:
> >>>
> >>>        Features and status bits could be helpful for applications to
> compare
> >>>        directly. For instance, when an upper application aims to
> ensure the
> >>>        virtio negotiation correctness between guest, QEMU, and
> OVS-DPDK, it use
> >>>        the "ovs-vsctl list interface" command to retrieve interface
> features
> >>>        (in number format) and the QMP command x-query-virtio-status to
> retrieve
> >>>        vhost-user net device features. If "show-bits" is added, the
> application
> >>>        can compare the two features directly; No need to encoding the
> features
> >>>        returned by the QMP command.
> >>>
> >>>        This patch also serves as a preparation for the next one, which
> implements
> >>>        a vhost-user test case about acked features of vhost-user
> protocol.
> >>>
> >>> I guess you're trying to simplify use cases where the QMP client wants
> >>> to compare entire feature sets without caring for individual features.
> >>>
> >>> The comparison is easy if both sets are represented the same way,
> >>> e.g. both are numbers, or both are lists of symbols.
> >>>
> >>> With different representations, we first have to map to a common
> >>> representation.  Unfortunately, the design of x-query-virtio-status
> >>> makes this harder than it should be.
> >>>
> >>> We use QAPI types VirtioDeviceStatus, VhostDeviceProtocols,
> >>> VirtioDeviceFeatures to represent feature sets.  They all work the same
> >>> way: array of strings plus a number.  For each bit QEMU knows, there's
> a
> >>> string in the array.  Any remaining bits go into the number.
> >>>
> >>> The format of the string is undocumented.  They look like
> >>>
> >>>       "WELL_KNOWN_SYMBOL: human readable explanation"
> >>>
> >>> Mapping from bit to this string in a client would require duplicating
> >>> QEMU's code exactly.
> >>>
> >>> Mapping both bit and string to just "WELL_KNOWN_SYMBOL" could perhaps
> be
> >>> done.
> >>>
> >>> The mapping between symbols and bits is not visible in QMP.  Mapping
> >>> from string to bit requires exploiting the undocumented format: extract
> >>> the well-known symbol and decode it.
> >>>
> >>> This encoding of feature sets goes back to commit f3034ad71fc (qmp:
> >>> decode feature & status bits in virtio-status) v7.2.  Before that, the
> >>> command returned the bits as a number.
> >>>
> >>> For example, return value "member "status":
> >>>
> >>>       Before f3034ad71fc:
> >>>
> >>>           "status": 15,
> >>>
> >>>       Since f3034ad71fc:
> >>>
> >>>           "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"
> >>>               ]},
> >>>
> >>>       With your patch:
> >>>
> >>>           "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-bits": 15,
> >>>
> >>> Looks like commit f3034ad71fc improved one use case at the expense of
> >>> another, and your patch tries to revert the damage.  Which one exactly
> >>> it improved is unclear; the commit message doesn't tell.  Laurent?
> >>>
> >>> [...]
> >>>
> >>
> >> The first idea of the series "hmp,qmp: Add commands to introspect
> virtio devices"
> >> including commit f3034ad71fc was to help developer to debug virtio
> devices, so for this
> >> purpose it was interesting to display the status in a human readable
> manner.
> >>
> >> Of course, if you want to run automatic tests and be able to compare
> the result to have
> >> the status bits result seems to be better.
> >>
> >> As these are two different use cases, it's understandable to have two
> different
> >> representations of the same information.
> >
> > Thanks!
> >
> > Both use cases are valid, but I dislike both the existing and the
> > proposed interface.
> >
> > We can change it: x-query-virtio-status isn't stable (it's for debugging
> > and testing).  But even unstable interfaces should only be changed for
> > good, clear reasons.
> >
> > I feel the change from "bits encoded as a number" to "bits as list of
> > descriptive strings plus number for the unknown ones" fell short.  Let
> > me explain.
> >
> > The initial version of the command had "bits encoded as number".  Unless
> > we understand why that was done, we should assume it was done for a
> > reason.  We now know it was: Hyman Huang posted a patch to get it back.
> >
> > Instead of "bits as list of descriptive strings plus number for the
> > unknown ones", we could have done "bits encoded as number, plus list of
> > descriptive strings", or plus some other human-readable encoding.
> >
> > QMP output of the form "WELL_KNOWN_SYMBOL: human readable explanation"
> > smells of encoding structured information in strings, which is a no-no.
> >
> > Perhaps we could have added human-readable output just in HMP.  That's
> > what we normally do.
> >
> > Here are a few possible alternatives to Hyman Huang's patch:
> >
> > 1. Revert commit f3034ad71fc for QMP, keep it for HMP.
> >
> > 2. Replace @unknown-FOO (just the unknown bits) by @FOO-bits (all bits).
> >
> > 3. Add @FOO-bits next to @unknown-FOO, deprecate @unknown-FOO.
> >
> > 4. Create a QAPI enum for the known bits.  Clients can use introspection
> >     to learn the mapping between symbols and bits.  Requires dumbing down
> >     the descriptive strings to just the symbols.  This feels
> >     both overengineered and cumbersome to use.
> >
> > For 2 and 3, I'd prefer to also dumb down the descriptive strings to
> > just the symbols.
> >
> > Thoughts?
> >
>
> I agree with you. As x-CMD are unstable, perhaps we can go directly to 2?
> (and of course to remove the descriptive strings. Is it easily possible to
> keep them for
> the HMP version?)
>
> Thanks,
> Laurent
>
>
Sorry for the late reply. :(

Let me make a conclusion about our discussion, if i misunderstand
something,
point that out please:

1. we take the policy of adding human-readable output just in HMP.

2. For the HMP output, we display the human-readable information and drop
   the unknown bits in practice.

3. For the QMP output, we remove the descriptive strings and only display
    bits encoded as numbers.

I'll do that in the next version and make the PATCH 1/2 apart from the
PATCH 2/2
since it could be handled in an independent context.

Patch

diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
index 477c97dea2..3774f3d4bf 100644
--- a/hw/virtio/virtio-hmp-cmds.c
+++ b/hw/virtio/virtio-hmp-cmds.c
@@ -108,7 +108,7 @@  void hmp_virtio_status(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
     const char *path = qdict_get_try_str(qdict, "path");
-    VirtioStatus *s = qmp_x_query_virtio_status(path, &err);
+    VirtioStatus *s = qmp_x_query_virtio_status(path, false, false, &err);
 
     if (err != NULL) {
         hmp_handle_error(mon, err);
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 1dd96ed20f..2e92bf28ac 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -718,10 +718,15 @@  VirtIODevice *qmp_find_virtio_device(const char *path)
     return VIRTIO_DEVICE(dev);
 }
 
-VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
+VirtioStatus *qmp_x_query_virtio_status(const char *path,
+                                        bool has_show_bits,
+                                        bool show_bits,
+                                        Error **errp)
 {
     VirtIODevice *vdev;
     VirtioStatus *status;
+    bool display_bits =
+        has_show_bits ? show_bits : false;
 
     vdev = qmp_find_virtio_device(path);
     if (vdev == NULL) {
@@ -733,6 +738,11 @@  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;
+    if (display_bits) {
+        status->guest_features_bits = vdev->guest_features;
+        status->host_features_bits = vdev->host_features;
+        status->backend_features_bits = vdev->backend_features;
+    }
     status->guest_features = qmp_decode_features(vdev->device_id,
                                                  vdev->guest_features);
     status->host_features = qmp_decode_features(vdev->device_id,
@@ -753,6 +763,9 @@  VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
     }
 
     status->num_vqs = virtio_get_num_queues(vdev);
+    if (display_bits) {
+        status->status_bits = vdev->status;
+    }
     status->status = qmp_decode_status(vdev->status);
     status->isr = vdev->isr;
     status->queue_sel = vdev->queue_sel;
@@ -775,6 +788,12 @@  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;
+        if (display_bits) {
+            status->vhost_dev->features_bits = hdev->features;
+            status->vhost_dev->acked_features_bits = hdev->acked_features;
+            status->vhost_dev->backend_features_bits = hdev->backend_features;
+            status->vhost_dev->protocol_features_bits = hdev->protocol_features;
+        }
         status->vhost_dev->features =
             qmp_decode_features(vdev->device_id, hdev->features);
         status->vhost_dev->acked_features =
diff --git a/qapi/virtio.json b/qapi/virtio.json
index e6dcee7b83..608b841a89 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -79,12 +79,20 @@ 
 #
 # @vq-index: vhost_dev vq_index
 #
+# @features-bits: vhost_dev features in decimal format
+#
 # @features: vhost_dev features
 #
+# @acked-features-bits: vhost_dev acked_features in decimal format
+#
 # @acked-features: vhost_dev acked_features
 #
+# @backend-features-bits: vhost_dev backend_features in decimal format
+#
 # @backend-features: vhost_dev backend_features
 #
+# @protocol-features-bits: vhost_dev protocol_features in decimal format
+#
 # @protocol-features: vhost_dev protocol_features
 #
 # @max-queues: vhost_dev max_queues
@@ -102,9 +110,13 @@ 
             'n-tmp-sections': 'int',
             'nvqs': 'uint32',
             'vq-index': 'int',
+            'features-bits': 'uint64',
             'features': 'VirtioDeviceFeatures',
+            'acked-features-bits': 'uint64',
             'acked-features': 'VirtioDeviceFeatures',
+            'backend-features-bits': 'uint64',
             'backend-features': 'VirtioDeviceFeatures',
+            'protocol-features-bits': 'uint64',
             'protocol-features': 'VhostDeviceProtocols',
             'max-queues': 'uint64',
             'backend-cap': 'uint64',
@@ -124,10 +136,16 @@ 
 #
 # @vhost-started: VirtIODevice vhost_started flag
 #
+# @guest-features-bits: VirtIODevice guest_features in decimal format
+#
 # @guest-features: VirtIODevice guest_features
 #
+# @host-features-bits: VirtIODevice host_features in decimal format
+#
 # @host-features: VirtIODevice host_features
 #
+# @backend-features-bits: VirtIODevice backend_features in decimal format
+#
 # @backend-features: VirtIODevice backend_features
 #
 # @device-endian: VirtIODevice device_endian
@@ -135,6 +153,9 @@ 
 # @num-vqs: VirtIODevice virtqueue count.  This is the number of
 #     active virtqueues being used by the VirtIODevice.
 #
+# @status-bits: VirtIODevice configuration status in decimal format
+#     (VirtioDeviceStatus)
+#
 # @status: VirtIODevice configuration status (VirtioDeviceStatus)
 #
 # @isr: VirtIODevice ISR
@@ -170,10 +191,14 @@ 
             'device-id': 'uint16',
             'vhost-started': 'bool',
             'device-endian': 'str',
+            'guest-features-bits': 'uint64',
             'guest-features': 'VirtioDeviceFeatures',
+            'host-features-bits': 'uint64',
             'host-features': 'VirtioDeviceFeatures',
+            'backend-features-bits': 'uint64',
             'backend-features': 'VirtioDeviceFeatures',
             'num-vqs': 'int',
+            'status-bits': 'uint8',
             'status': 'VirtioDeviceStatus',
             'isr': 'uint8',
             'queue-sel': 'uint16',
@@ -195,6 +220,9 @@ 
 #
 # @path: Canonical QOM path of the VirtIODevice
 #
+# @show-bits: Whether to display the feature & status bits.
+#     Default is disabled. (Since 8.2)
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -208,7 +236,8 @@ 
 # 1. Poll for the status of virtio-crypto (no vhost-crypto active)
 #
 # -> { "execute": "x-query-virtio-status",
-#      "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend" }
+#      "arguments": { "path": "/machine/peripheral/crypto0/virtio-backend",
+#                     "show-bits": true }
 #    }
 # <- { "return": {
 #          "device-endian": "little",
@@ -216,6 +245,7 @@ 
 #          "disable-legacy-check": false,
 #          "name": "virtio-crypto",
 #          "started": true,
+#          "guest-features-bits": 5100273664,
 #          "device-id": 20,
 #          "backend-features": {
 #              "transports": [],
@@ -241,6 +271,7 @@ 
 #                  "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
 #              ]
 #          },
+#          "host-features-bits": 6325010432,
 #          "host-features": {
 #              "unknown-dev-features": 1073741824,
 #              "dev-features": [],
@@ -252,9 +283,11 @@ 
 #                  "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
 #              ]
 #          },
+#          "backend-features-bits": 0,
 #          "use-guest-notifier-mask": true,
 #          "vm-running": true,
 #          "queue-sel": 1,
+#          "status-bits": 15,
 #          "disabled": false,
 #          "vhost-started": false,
 #          "use-started": true
@@ -264,7 +297,8 @@ 
 # 2. Poll for the status of virtio-net (vhost-net is active)
 #
 # -> { "execute": "x-query-virtio-status",
-#      "arguments": { "path": "/machine/peripheral-anon/device[1]/virtio-backend" }
+#      "arguments": { "path": "/machine/peripheral-anon/device[1]/virtio-backend",
+#                     "show-bits": true }
 #    }
 # <- { "return": {
 #          "device-endian": "little",
@@ -272,11 +306,13 @@ 
 #          "disabled-legacy-check": false,
 #          "name": "virtio-net",
 #          "started": true,
+#          "guest-features-bits": 5111807911,
 #          "device-id": 1,
 #          "vhost-dev": {
 #              "n-tmp-sections": 4,
 #              "n-mem-sections": 4,
 #              "max-queues": 1,
+#              "features-bits": 13908344832
 #              "backend-cap": 2,
 #              "log-size": 0,
 #              "backend-features": {
@@ -284,6 +320,8 @@ 
 #                  "transports": []
 #              },
 #              "nvqs": 2,
+#              "acked-features-bits": 5100306432,
+#              "backend-features-bits": 0,
 #              "protocol-features": {
 #                  "protocols": []
 #              },
@@ -299,6 +337,7 @@ 
 #                      "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
 #                  ]
 #              },
+#              "protocol-features-bits": 0,
 #              "features": {
 #                  "dev-features": [
 #                      "VHOST_F_LOG_ALL: Logging write descriptors supported",
@@ -387,6 +426,7 @@ 
 #                  "VIRTIO_F_VERSION_1: Device compliant for v1 spec (legacy)"
 #             ]
 #          },
+#          "host-features-bits": 6337593319,
 #          "host-features": {
 #              "dev-features": [
 #                  "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features negotiation supported",
@@ -420,9 +460,11 @@ 
 #                  "VIRTIO_F_NOTIFY_ON_EMPTY: Notify when device runs out of avail. descs. on VQ"
 #             ]
 #          },
+#          "backend-features-bits": 6337593319,
 #          "use-guest-notifier-mask": true,
 #          "vm-running": true,
 #          "queue-sel": 2,
+#          "status-bits": 15,
 #          "disabled": false,
 #          "vhost-started": true,
 #          "use-started": true
@@ -430,7 +472,8 @@ 
 #    }
 ##
 { 'command': 'x-query-virtio-status',
-  'data': { 'path': 'str' },
+  'data': { 'path': 'str',
+            '*show-bits': 'bool'},
   'returns': 'VirtioStatus',
   'features': [ 'unstable' ] }