[v2 0/2] vhost-user-test: Add negotiated features check

[v2 0/2] vhost-user-test: Add negotiated features check

[Hyman Huang]

Markus made suggestions for the changes to version 2, and thanks
for that as well.

v2:
- rebase on master.
- drop the "show-bits" option. 
- refine the comment.


v1:
The patchset "Fix the virtio features negotiation flaw" fix a
vhost-user negotiation flaw:
c9bdc449f9 vhost-user: Fix the virtio features negotiation flaw
bebcac052a vhost-user: Refactor the chr_closed_bh
937b7d96e4 vhost-user: Refactor vhost acked features saving

While the test case remain unmerged, the detail reference:
https://lore.kernel.org/qemu-devel/cover.1667232396.git.huangy81@chinatelecom.cn/

Since Michael pointed out that the info virtio makes sense to query
the negotiation feature, this patchset uses the x-query-virtio-status
to retrieve the features instead of exporting netdev capabilities and
information as we did in the previous patchset to aid in confirming
the negotiation's validity.

To do that, we first introduce an "show-bits" argument for
x-query-virtio-status such that the feature bits can be used
directly, and then implement the test case for negotiated features
check. As we post, the code is divided into two patches.

Please review, thanks,
Yong

Hyman Huang (2):
  qapi/virtio: Add feature and status bits for x-query-virtio-status
  vhost-user-test: Add negotiated features check

 hw/virtio/virtio-qmp.c        |   8 +++
 qapi/virtio.json              |  37 +++++++++++++
 tests/qtest/vhost-user-test.c | 100 ++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+)

-- 
2.39.1

[v2 1/2] qapi/virtio: Add feature and status bits for x-query-virtio-status

[Hyman Huang]

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

Applications could find it helpful to compare status and features
that are numeric encoded. For example, an upper application could
use the features (encoded as a number) in the output of "ovs-vsctl
list interface" and the feature bits fields in the output of QMP
command "x-query-virtio-status" to compare directly when attempting
to ensure the correctness of the virtio negotiation between guest,
QEMU, and OVS-DPDK. Not applying any more encoding.

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.huang@smartx.com>
---
 hw/virtio/virtio-qmp.c |  8 ++++++++
 qapi/virtio.json       | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 1dd96ed20f..13ba1e926e 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -733,6 +733,9 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
     status->name = g_strdup(vdev->name);
     status->device_id = vdev->device_id;
     status->vhost_started = vdev->vhost_started;
+    status->guest_features_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 +756,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
     }
 
     status->num_vqs = virtio_get_num_queues(vdev);
+    status->status_bits = vdev->status;
     status->status = qmp_decode_status(vdev->status);
     status->isr = vdev->isr;
     status->queue_sel = vdev->queue_sel;
@@ -775,6 +779,10 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
         status->vhost_dev->n_tmp_sections = hdev->n_tmp_sections;
         status->vhost_dev->nvqs = hdev->nvqs;
         status->vhost_dev->vq_index = hdev->vq_index;
+        status->vhost_dev->features_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..6f1b5e3710 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -79,12 +79,20 @@
 #
 # @vq-index: vhost_dev vq_index
 #
+# @features-bits: vhost_dev features encoded as a number
+#
 # @features: vhost_dev features
 #
+# @acked-features-bits: vhost_dev acked_features encoded as a number
+#
 # @acked-features: vhost_dev acked_features
 #
+# @backend-features-bits: vhost_dev backend_features encoded as a number
+#
 # @backend-features: vhost_dev backend_features
 #
+# @protocol-features-bits: vhost_dev protocol_features encoded as a number
+#
 # @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 encoded as a number
+#
 # @guest-features: VirtIODevice guest_features
 #
+# @host-features-bits: VirtIODevice host_features encoded as a number
+#
 # @host-features: VirtIODevice host_features
 #
+# @backend-features-bits: VirtIODevice backend_features encoded as a number
+#
 # @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 encoded as a number
+#     (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',
@@ -216,6 +241,7 @@
 #          "disable-legacy-check": false,
 #          "name": "virtio-crypto",
 #          "started": true,
+#          "guest-features-bits": 5100273664,
 #          "device-id": 20,
 #          "backend-features": {
 #              "transports": [],
@@ -241,6 +267,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 +279,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
@@ -272,11 +301,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 +315,8 @@
 #                  "transports": []
 #              },
 #              "nvqs": 2,
+#              "acked-features-bits": 5100306432,
+#              "backend-features-bits": 0,
 #              "protocol-features": {
 #                  "protocols": []
 #              },
@@ -299,6 +332,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 +421,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 +455,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
-- 
2.39.1

[v2 2/2] vhost-user-test: Add negotiated features check

[Hyman Huang]

When a vhost-user network device is restored from an unexpected
failure, the acked_features could be used as input for the
VHOST_USER_SET_FEATURES command because QEMU internally backups
the final features as acked_features after the guest acknowledges
features during virtio-net driver initialization.

The negotiated features check verifies whether the features in the
Vhost slave device and the acked_features in QEMU are identical.

Through the usage of the vhost-user protocol, the test case seeks to
verify that the vhost-user network device is correctly negotiating.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 tests/qtest/vhost-user-test.c | 100 ++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index d4e437265f..4f98ee2560 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -13,6 +13,7 @@
 #include "libqtest-single.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/range.h"
@@ -169,6 +170,7 @@ typedef struct TestServer {
     int test_flags;
     int queues;
     struct vhost_user_ops *vu_ops;
+    uint64_t features;
 } TestServer;
 
 struct vhost_user_ops {
@@ -1020,6 +1022,100 @@ static void test_multiqueue(void *obj, void *arg, QGuestAllocator *alloc)
 }
 
 
+static QDict *query_virtio(QTestState *who)
+{
+    QDict *rsp;
+
+    rsp = qtest_qmp(who, "{ 'execute': 'x-query-virtio'}");
+    g_assert(!qdict_haskey(rsp, "error"));
+    g_assert(qdict_haskey(rsp, "return"));
+
+    return rsp;
+}
+
+static QDict *query_virtio_status(QTestState *who, const char *path)
+{
+    QDict *rsp;
+
+    rsp = qtest_qmp(who, "{ 'execute': 'x-query-virtio-status', "
+        "'arguments': { 'path': %s} }", path);
+
+    g_assert(!qdict_haskey(rsp, "error"));
+    g_assert(qdict_haskey(rsp, "return"));
+
+    return rsp;
+}
+
+static uint64_t get_acked_features(QTestState *who)
+{
+    QDict *rsp_return, *status, *vhost_info, *dev;
+    QList *dev_list;
+    const QListEntry *entry;
+    const char *name;
+    char *path;
+    uint64_t acked_features;
+
+    /* query the virtio devices */
+    rsp_return = query_virtio(who);
+    g_assert(rsp_return);
+
+    dev_list = qdict_get_qlist(rsp_return, "return");
+    g_assert(dev_list && !qlist_empty(dev_list));
+
+    /* fetch the first and the sole device */
+    entry = qlist_first(dev_list);
+    g_assert(entry);
+
+    dev = qobject_to(QDict, qlist_entry_obj(entry));
+    g_assert(dev);
+
+    name = qdict_get_try_str(dev, "name");
+    g_assert_cmpstr(name, ==, "virtio-net");
+
+    path = g_strdup(qdict_get_try_str(dev, "path"));
+    g_assert(path);
+    qobject_unref(rsp_return);
+    rsp_return = NULL;
+
+    /* fetch the status of the virtio-net device by QOM path */
+    rsp_return = query_virtio_status(who, path);
+    g_assert(rsp_return);
+
+    status = qdict_get_qdict(rsp_return, "return");
+    g_assert(status);
+
+    vhost_info = qdict_get_qdict(status, "vhost-dev");
+    g_assert(vhost_info);
+
+    acked_features = qdict_get_try_int(vhost_info, "acked-features-bits", 0);
+
+    qobject_unref(rsp_return);
+    g_free(path);
+
+    return acked_features;
+}
+
+static void acked_features_check(QTestState *qts, TestServer *s)
+{
+    uint64_t acked_features;
+
+    acked_features = get_acked_features(qts);
+    g_assert_cmpint(acked_features, ==, s->features);
+}
+
+static void test_acked_features(void *obj,
+                                     void *arg,
+                                     QGuestAllocator *alloc)
+{
+    TestServer *server = arg;
+
+    if (!wait_for_fds(server)) {
+        return;
+    }
+
+    acked_features_check(global_qtest, server);
+}
+
 static uint64_t vu_net_get_features(TestServer *s)
 {
     uint64_t features = 0x1ULL << VHOST_F_LOG_ALL |
@@ -1040,6 +1136,7 @@ static void vu_net_set_features(TestServer *s, CharBackend *chr,
         qemu_chr_fe_disconnect(chr);
         s->test_flags = TEST_FLAGS_BAD;
     }
+    s->features = msg->payload.u64;
 }
 
 static void vu_net_get_protocol_features(TestServer *s, CharBackend *chr,
@@ -1109,6 +1206,9 @@ static void register_vhost_user_test(void)
     qos_add_test("vhost-user/multiqueue",
                  "virtio-net",
                  test_multiqueue, &opts);
+    qos_add_test("vhost-user/read_acked_features",
+                 "virtio-net",
+                 test_acked_features, &opts);
 }
 libqos_init(register_vhost_user_test);
 
-- 
2.39.1

Re: [v2 1/2] qapi/virtio: Add feature and status bits for x-query-virtio-status

[Markus Armbruster]

Hyman Huang <yong.huang@smartx.com> writes:

> This patch allows to display feature and status bits in
> virtio-status.
>
> Applications could find it helpful to compare status and features
> that are numeric encoded. For example, an upper application could
> use the features (encoded as a number) in the output of "ovs-vsctl
> list interface" and the feature bits fields in the output of QMP
> command "x-query-virtio-status" to compare directly when attempting
> to ensure the correctness of the virtio negotiation between guest,
> QEMU, and OVS-DPDK. Not applying any more encoding.
>
> 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.huang@smartx.com>

We discussed v1 some more since you posted this.  Do you intend to do a
v3?

Re: [v2 1/2] qapi/virtio: Add feature and status bits for x-query-virtio-status

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

Sure. of course. I'll do that next week. While v3 would be a single patch
that only contains the first commit.

Thanks,
Yong

On Fri, Dec 22, 2023 at 5:54 PM Markus Armbruster <armbru@redhat.com> wrote:

> Hyman Huang <yong.huang@smartx.com> writes:
>
> > This patch allows to display feature and status bits in
> > virtio-status.
> >
> > Applications could find it helpful to compare status and features
> > that are numeric encoded. For example, an upper application could
> > use the features (encoded as a number) in the output of "ovs-vsctl
> > list interface" and the feature bits fields in the output of QMP
> > command "x-query-virtio-status" to compare directly when attempting
> > to ensure the correctness of the virtio negotiation between guest,
> > QEMU, and OVS-DPDK. Not applying any more encoding.
> >
> > 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.huang@smartx.com>
>
> We discussed v1 some more since you posted this.  Do you intend to do a
> v3?
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 2407 bytes --]