On Tue, Nov 21, 2023 at 3:58 PM Markus Armbruster <arm...@redhat.com> wrote:
> Laurent, there's a question for you at the end.
>
> Yong Huang <yong.hu...@smartx.com> writes:
>
> > On Thu, Nov 16, 2023 at 10:44 PM Markus Armbruster <arm...@redhat.com>
> > wrote:
> >
> >> Hyman Huang <yong.hu...@smartx.com> 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 <yong.hu...@smartx.com>
> >> > ---
> >> > 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?
>
> [...]
>
>
--
Best regards
