[PATCH 0/4] vhost: drop backend_features

[PATCH 0/4] vhost: drop backend_features

[Vladimir Sementsov-Ogievskiy]

This field is mostly unused and sometimes confusing (we even have
a TODO-like comment to drop it). Let's finally do.

Vladimir Sementsov-Ogievskiy (4):
  vhost: introduce vhost_ops->vhost_set_vring_enable_supported method
  vhost-user: stop use backend_features
  vhost_net: stop use backend_features
  hw/vhost: finally drop vhost_dev.backend_features field

 hw/block/vhost-user-blk.c         |  1 -
 hw/net/vhost_net.c                | 14 ++++++--------
 hw/scsi/vhost-scsi.c              |  1 -
 hw/scsi/vhost-user-scsi.c         |  1 -
 hw/virtio/vdpa-dev.c              |  1 -
 hw/virtio/vhost-user.c            | 25 ++++++++++++++++---------
 hw/virtio/vhost.c                 | 15 ++++++---------
 hw/virtio/virtio-qmp.c            |  2 --
 include/hw/virtio/vhost-backend.h |  2 ++
 include/hw/virtio/vhost.h         |  7 -------
 10 files changed, 30 insertions(+), 39 deletions(-)

-- 
2.48.1

[PATCH 1/4] vhost: introduce vhost_ops->vhost_set_vring_enable_supported method

[Vladimir Sementsov-Ogievskiy]

Remove vhost-user specific hack from generic code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/virtio/vhost-user.c            |  8 ++++++++
 hw/virtio/vhost.c                 | 15 ++++++---------
 include/hw/virtio/vhost-backend.h |  2 ++
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1e1d6b0d6e0..1b2879a90cc 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1230,6 +1230,12 @@ static int vhost_user_set_vring_base(struct vhost_dev *dev,
     return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring, false);
 }
 
+static bool vhost_user_set_vring_enable_supported(struct vhost_dev *dev)
+{
+    return virtio_has_feature(dev->backend_features,
+                              VHOST_USER_F_PROTOCOL_FEATURES);
+}
+
 static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
 {
     int i;
@@ -3032,6 +3038,8 @@ const VhostOps user_ops = {
         .vhost_reset_device = vhost_user_reset_device,
         .vhost_get_vq_index = vhost_user_get_vq_index,
         .vhost_set_vring_enable = vhost_user_set_vring_enable,
+        .vhost_set_vring_enable_supported =
+            vhost_user_set_vring_enable_supported,
         .vhost_requires_shm_log = vhost_user_requires_shm_log,
         .vhost_migration_done = vhost_user_migration_done,
         .vhost_net_set_mtu = vhost_user_net_set_mtu,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index fc438537048..429fad07ded 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1988,15 +1988,12 @@ static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
         return 0;
     }
 
-    /*
-     * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
-     * been negotiated, the rings start directly in the enabled state, and
-     * .vhost_set_vring_enable callback will fail since
-     * VHOST_USER_SET_VRING_ENABLE is not supported.
-     */
-    if (hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER &&
-        !virtio_has_feature(hdev->backend_features,
-                            VHOST_USER_F_PROTOCOL_FEATURES)) {
+    if (hdev->vhost_ops->vhost_set_vring_enable_supported &&
+        !hdev->vhost_ops->vhost_set_vring_enable_supported(hdev)) {
+        /*
+         * This means, that rings are always enabled, and disable/enable
+         * API is not supported.
+         */
         return 0;
     }
 
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index d6df209a2f0..f65fa26298e 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -105,6 +105,7 @@ typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
 typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
 typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
                                          int enable);
+typedef bool (*vhost_set_vring_enable_supported_op)(struct vhost_dev *dev);
 typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
 typedef int (*vhost_migration_done_op)(struct vhost_dev *dev,
                                        char *mac_addr);
@@ -193,6 +194,7 @@ typedef struct VhostOps {
     vhost_reset_device_op vhost_reset_device;
     vhost_get_vq_index_op vhost_get_vq_index;
     vhost_set_vring_enable_op vhost_set_vring_enable;
+    vhost_set_vring_enable_supported_op vhost_set_vring_enable_supported;
     vhost_requires_shm_log_op vhost_requires_shm_log;
     vhost_migration_done_op vhost_migration_done;
     vhost_vsock_set_guest_cid_op vhost_vsock_set_guest_cid;
-- 
2.48.1

[PATCH 2/4] vhost-user: stop use backend_features

[Vladimir Sementsov-Ogievskiy]

Simply use features instead, we have same flag here.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/virtio/vhost-user.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1b2879a90cc..cf6f53801db 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1232,7 +1232,7 @@ static int vhost_user_set_vring_base(struct vhost_dev *dev,
 
 static bool vhost_user_set_vring_enable_supported(struct vhost_dev *dev)
 {
-    return virtio_has_feature(dev->backend_features,
+    return virtio_has_feature(dev->features,
                               VHOST_USER_F_PROTOCOL_FEATURES);
 }
 
@@ -1449,14 +1449,15 @@ static int vhost_user_set_features(struct vhost_dev *dev,
     int ret;
 
     /*
-     * We need to include any extra backend only feature bits that
-     * might be needed by our device. Currently this includes the
-     * VHOST_USER_F_PROTOCOL_FEATURES bit for enabling protocol
-     * features.
+     * Don't lose VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user
+     * specific.
      */
-    ret = vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
-                              features | dev->backend_features,
-                              log_enabled);
+    if (virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
+        features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+    }
+
+    ret = vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+                             log_enabled);
 
     if (virtio_has_feature(dev->protocol_features,
                            VHOST_USER_PROTOCOL_F_STATUS)) {
@@ -2187,8 +2188,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
             (dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
         uint64_t protocol_features;
 
-        dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
-
         err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
                                  &protocol_features);
         if (err < 0) {
-- 
2.48.1

[PATCH 3/4] vhost_net: stop use backend_features

[Vladimir Sementsov-Ogievskiy]

We are going to drop backend_features variable as an extra one.
Call to qemu_has_vnet_hdr() is cheap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/net/vhost_net.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 891f235a0a6..38cbc6caa14 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -139,7 +139,8 @@ int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
 
 void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
 {
-    net->dev.acked_features = net->dev.backend_features;
+    net->dev.acked_features = qemu_has_vnet_hdr(net->nc)
+        ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
     vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
 }
 
@@ -338,12 +339,9 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
         if (r < 0) {
             goto fail;
         }
-        net->dev.backend_features = qemu_has_vnet_hdr(options->net_backend)
-            ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
         net->backend = r;
         net->dev.protocol_features = 0;
     } else {
-        net->dev.backend_features = 0;
         net->dev.protocol_features = 0;
         net->backend = -1;
 
@@ -363,10 +361,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
                                sizeof(struct virtio_net_hdr_mrg_rxbuf))) {
             net->dev.features &= ~(1ULL << VIRTIO_NET_F_MRG_RXBUF);
         }
-        if (~net->dev.features & net->dev.backend_features) {
-            fprintf(stderr, "vhost lacks feature mask 0x%" PRIx64
-                   " for backend\n",
-                   (uint64_t)(~net->dev.features & net->dev.backend_features));
+        if (!qemu_has_vnet_hdr(options->net_backend) &&
+            (~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR))) {
+            fprintf(stderr, "vhost lacks feature mask 0x%llx for backend\n",
+                    ~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR));
             goto fail;
         }
     }
-- 
2.48.1

[PATCH 4/4] hw/vhost: finally drop vhost_dev.backend_features field

[Vladimir Sementsov-Ogievskiy]

It's unused now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/block/vhost-user-blk.c | 1 -
 hw/scsi/vhost-scsi.c      | 1 -
 hw/scsi/vhost-user-scsi.c | 1 -
 hw/virtio/vdpa-dev.c      | 1 -
 hw/virtio/virtio-qmp.c    | 2 --
 include/hw/virtio/vhost.h | 7 -------
 6 files changed, 13 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 0eebbcd80d8..079a6492355 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -343,7 +343,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
     s->dev.nvqs = s->num_queues;
     s->dev.vqs = s->vhost_vqs;
     s->dev.vq_index = 0;
-    s->dev.backend_features = 0;
 
     vhost_dev_set_config_notifier(&s->dev, &blk_ops);
 
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index cdf405b0f86..d694a25fe2d 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -276,7 +276,6 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs);
     vsc->dev.vqs = vqs;
     vsc->dev.vq_index = 0;
-    vsc->dev.backend_features = 0;
 
     ret = vhost_dev_init(&vsc->dev, (void *)(uintptr_t)vhostfd,
                          VHOST_BACKEND_TYPE_KERNEL, 0, errp);
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 25f2d894e7c..0c80a271d8b 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -159,7 +159,6 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
     vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
     vsc->dev.vqs = s->vhost_vqs;
     vsc->dev.vq_index = 0;
-    vsc->dev.backend_features = 0;
 
     ret = vhost_dev_init(&vsc->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
                          errp);
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index d1da40afc80..3c0eed3e8e1 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -104,7 +104,6 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
     v->dev.vqs = vqs;
     v->dev.vq_index = 0;
     v->dev.vq_index_end = v->dev.nvqs;
-    v->dev.backend_features = 0;
     v->started = false;
 
     ret = vhost_vdpa_get_iova_range(v->vhostfd, &iova_range);
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 3b6377cf0d2..e514a4797e3 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -788,8 +788,6 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
             qmp_decode_features(vdev->device_id, hdev->features);
         status->vhost_dev->acked_features =
             qmp_decode_features(vdev->device_id, hdev->acked_features);
-        status->vhost_dev->backend_features =
-            qmp_decode_features(vdev->device_id, hdev->backend_features);
         status->vhost_dev->protocol_features =
             qmp_decode_protocols(hdev->protocol_features);
         status->vhost_dev->max_queues = hdev->max_queues;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 38800a7156b..d6afde8b5dd 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -99,16 +99,9 @@ struct vhost_dev {
      *
      * @features: available features provided by the backend
      * @acked_features: final negotiated features with front-end driver
-     *
-     * @backend_features: this is used in a couple of places to either
-     * store VHOST_USER_F_PROTOCOL_FEATURES to apply to
-     * VHOST_USER_SET_FEATURES or VHOST_NET_F_VIRTIO_NET_HDR. Its
-     * future use should be discouraged and the variable retired as
-     * its easy to confuse with the VirtIO backend_features.
      */
     uint64_t features;
     uint64_t acked_features;
-    uint64_t backend_features;
 
     /**
      * @protocol_features: is the vhost-user only feature set by
-- 
2.48.1

Re: [PATCH 0/4] vhost: drop backend_features

[Lei Yang]

I tested this series of patches with virtio-net regression tests which
were triggered because the virtio-net code was changed. Everything
works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Thu, Jul 3, 2025 at 8:55 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> This field is mostly unused and sometimes confusing (we even have
> a TODO-like comment to drop it). Let's finally do.
>
> Vladimir Sementsov-Ogievskiy (4):
>   vhost: introduce vhost_ops->vhost_set_vring_enable_supported method
>   vhost-user: stop use backend_features
>   vhost_net: stop use backend_features
>   hw/vhost: finally drop vhost_dev.backend_features field
>
>  hw/block/vhost-user-blk.c         |  1 -
>  hw/net/vhost_net.c                | 14 ++++++--------
>  hw/scsi/vhost-scsi.c              |  1 -
>  hw/scsi/vhost-user-scsi.c         |  1 -
>  hw/virtio/vdpa-dev.c              |  1 -
>  hw/virtio/vhost-user.c            | 25 ++++++++++++++++---------
>  hw/virtio/vhost.c                 | 15 ++++++---------
>  hw/virtio/virtio-qmp.c            |  2 --
>  include/hw/virtio/vhost-backend.h |  2 ++
>  include/hw/virtio/vhost.h         |  7 -------
>  10 files changed, 30 insertions(+), 39 deletions(-)
>
> --
> 2.48.1
>
>

Re: [PATCH 0/4] vhost: drop backend_features

[Vladimir Sementsov-Ogievskiy]

ping:)

On 03.07.25 15:47, Vladimir Sementsov-Ogievskiy wrote:
> This field is mostly unused and sometimes confusing (we even have
> a TODO-like comment to drop it). Let's finally do.
> 
> Vladimir Sementsov-Ogievskiy (4):
>    vhost: introduce vhost_ops->vhost_set_vring_enable_supported method
>    vhost-user: stop use backend_features
>    vhost_net: stop use backend_features
>    hw/vhost: finally drop vhost_dev.backend_features field
> 
>   hw/block/vhost-user-blk.c         |  1 -
>   hw/net/vhost_net.c                | 14 ++++++--------
>   hw/scsi/vhost-scsi.c              |  1 -
>   hw/scsi/vhost-user-scsi.c         |  1 -
>   hw/virtio/vdpa-dev.c              |  1 -
>   hw/virtio/vhost-user.c            | 25 ++++++++++++++++---------
>   hw/virtio/vhost.c                 | 15 ++++++---------
>   hw/virtio/virtio-qmp.c            |  2 --
>   include/hw/virtio/vhost-backend.h |  2 ++
>   include/hw/virtio/vhost.h         |  7 -------
>   10 files changed, 30 insertions(+), 39 deletions(-)
> 

-- 
Best regards,
Vladimir

Re: [PATCH 0/4] vhost: drop backend_features

[Michael S. Tsirkin]

On Thu, Jul 03, 2025 at 03:47:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> This field is mostly unused and sometimes confusing (we even have
> a TODO-like comment to drop it). Let's finally do.

Breaks make check with UBSAN enabled:
32/109 /arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/vhost-user/flags-mismatch - ERROR:../tests/qtest/qos-test.c:189:subprocess_run_one_test: child process (/arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/vhost-user/flags-mismatch/subprocess [9701]) failed unexpectedly FAIL       



https://gitlab.com/mstredhat/qemu/-/jobs/10668177755


To trigger, configure with:

./configure '--cc=clang' '--cxx=clang++' '--enable-ubsan' '--extra-cflags=-fno-sanitize-recover=undefined -fno-sanitize=pointer-overflow' '--target-list=arm-softmmu' 

make
make check


> Vladimir Sementsov-Ogievskiy (4):
>   vhost: introduce vhost_ops->vhost_set_vring_enable_supported method
>   vhost-user: stop use backend_features
>   vhost_net: stop use backend_features
>   hw/vhost: finally drop vhost_dev.backend_features field
> 
>  hw/block/vhost-user-blk.c         |  1 -
>  hw/net/vhost_net.c                | 14 ++++++--------
>  hw/scsi/vhost-scsi.c              |  1 -
>  hw/scsi/vhost-user-scsi.c         |  1 -
>  hw/virtio/vdpa-dev.c              |  1 -
>  hw/virtio/vhost-user.c            | 25 ++++++++++++++++---------
>  hw/virtio/vhost.c                 | 15 ++++++---------
>  hw/virtio/virtio-qmp.c            |  2 --
>  include/hw/virtio/vhost-backend.h |  2 ++
>  include/hw/virtio/vhost.h         |  7 -------
>  10 files changed, 30 insertions(+), 39 deletions(-)
> 
> -- 
> 2.48.1

Re: [PATCH 0/4] vhost: drop backend_features

[Vladimir Sementsov-Ogievskiy]

On 14.07.25 13:41, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2025 at 03:47:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> This field is mostly unused and sometimes confusing (we even have
>> a TODO-like comment to drop it). Let's finally do.
> 
> Breaks make check with UBSAN enabled:
> 32/109 /arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/vhost-user/flags-mismatch - ERROR:../tests/qtest/qos-test.c:189:subprocess_run_one_test: child process (/arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/vhost-user/flags-mismatch/subprocess [9701]) failed unexpectedly FAIL
> 
> 
> 
> https://gitlab.com/mstredhat/qemu/-/jobs/10668177755
> 
> 
> To trigger, configure with:
> 
> ./configure '--cc=clang' '--cxx=clang++' '--enable-ubsan' '--extra-cflags=-fno-sanitize-recover=undefined -fno-sanitize=pointer-overflow' '--target-list=arm-softmmu'
> 
> make
> make check


Thanks for reproducer, it works for me.


Hm. Seems, that because I miss that I've changed the behavior for vhost_net_ack_features():
with may patches it doesn't include VHOST_USER_F_PROTOCOL_FEATURES (taken from backend_features) into acked_features..

And with such change:

  void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
  {
-    net->dev.acked_features = qemu_has_vnet_hdr(net->nc)
-        ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
+    net->dev.acked_features =
+        (qemu_has_vnet_hdr(net->nc) ? 0 :
+                                      (1ULL << VHOST_NET_F_VIRTIO_NET_HDR)) |
+        (net->dev.features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
      vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
  }


test starts to work again.

But I can't understand, what is the relation with VHOST_USER_F_PROTOCOL_FEATURES and acked_features.
I though, VHOST_USER_F_PROTOCOL_FEATURES make sense only for vhost <-> QEMU communication, and should
not present in guest-negotiated acked_features..


> 
> 
>> Vladimir Sementsov-Ogievskiy (4):
>>    vhost: introduce vhost_ops->vhost_set_vring_enable_supported method
>>    vhost-user: stop use backend_features
>>    vhost_net: stop use backend_features
>>    hw/vhost: finally drop vhost_dev.backend_features field
>>
>>   hw/block/vhost-user-blk.c         |  1 -
>>   hw/net/vhost_net.c                | 14 ++++++--------
>>   hw/scsi/vhost-scsi.c              |  1 -
>>   hw/scsi/vhost-user-scsi.c         |  1 -
>>   hw/virtio/vdpa-dev.c              |  1 -
>>   hw/virtio/vhost-user.c            | 25 ++++++++++++++++---------
>>   hw/virtio/vhost.c                 | 15 ++++++---------
>>   hw/virtio/virtio-qmp.c            |  2 --
>>   include/hw/virtio/vhost-backend.h |  2 ++
>>   include/hw/virtio/vhost.h         |  7 -------
>>   10 files changed, 30 insertions(+), 39 deletions(-)
>>
>> -- 
>> 2.48.1
> 

-- 
Best regards,
Vladimir

Re: [PATCH 0/4] vhost: drop backend_features

[Vladimir Sementsov-Ogievskiy]

On 14.07.25 22:23, Vladimir Sementsov-Ogievskiy wrote:
> On 14.07.25 13:41, Michael S. Tsirkin wrote:
>> On Thu, Jul 03, 2025 at 03:47:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> This field is mostly unused and sometimes confusing (we even have
>>> a TODO-like comment to drop it). Let's finally do.
>>
>> Breaks make check with UBSAN enabled:
>> 32/109 /arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/vhost-user/flags-mismatch - ERROR:../tests/qtest/qos-test.c:189:subprocess_run_one_test: child process (/arm/virt/virtio-mmio/virtio-bus/virtio-net-device/virtio-net/virtio-net-tests/vhost-user/flags-mismatch/subprocess [9701]) failed unexpectedly FAIL
>>
>>
>>
>> https://gitlab.com/mstredhat/qemu/-/jobs/10668177755
>>
>>
>> To trigger, configure with:
>>
>> ./configure '--cc=clang' '--cxx=clang++' '--enable-ubsan' '--extra-cflags=-fno-sanitize-recover=undefined -fno-sanitize=pointer-overflow' '--target-list=arm-softmmu'
>>
>> make
>> make check
> 
> 
> Thanks for reproducer, it works for me.
> 
> 
> Hm. Seems, that because I miss that I've changed the behavior for vhost_net_ack_features():
> with may patches it doesn't include VHOST_USER_F_PROTOCOL_FEATURES (taken from backend_features) into acked_features..
> 
> And with such change:
> 
>   void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
>   {
> -    net->dev.acked_features = qemu_has_vnet_hdr(net->nc)
> -        ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
> +    net->dev.acked_features =
> +        (qemu_has_vnet_hdr(net->nc) ? 0 :
> +                                      (1ULL << VHOST_NET_F_VIRTIO_NET_HDR)) |
> +        (net->dev.features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
>       vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
>   }
> 
> 
> test starts to work again.
> 
> But I can't understand, what is the relation with VHOST_USER_F_PROTOCOL_FEATURES and acked_features.
> I though, VHOST_USER_F_PROTOCOL_FEATURES make sense only for vhost <-> QEMU communication, and should
> not present in guest-negotiated acked_features..
> 
> 

UPD: undestand.

the test wants to check features mismatch, and on this code in vhost_net_init():

#ifdef CONFIG_VHOST_NET_USER
     if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
         features = vhost_user_get_acked_features(net->nc);
         if (~net->dev.features & features) {
             fprintf(stderr, "vhost lacks feature mask 0x%" PRIx64
                     " for backend\n",
                     (uint64_t)(~net->dev.features & features));
             goto fail;
         }
     }
#endif

still the only mismatching feature is VHOST_USER_F_PROTOCOL_FEATURES, which vhost_net code (before my changes) passes to acked features.

Instead of exploiting acked_features this way, we may improve this check to also check VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user-only.

I'll resend

>>
>>
>>> Vladimir Sementsov-Ogievskiy (4):
>>>    vhost: introduce vhost_ops->vhost_set_vring_enable_supported method
>>>    vhost-user: stop use backend_features
>>>    vhost_net: stop use backend_features
>>>    hw/vhost: finally drop vhost_dev.backend_features field
>>>
>>>   hw/block/vhost-user-blk.c         |  1 -
>>>   hw/net/vhost_net.c                | 14 ++++++--------
>>>   hw/scsi/vhost-scsi.c              |  1 -
>>>   hw/scsi/vhost-user-scsi.c         |  1 -
>>>   hw/virtio/vdpa-dev.c              |  1 -
>>>   hw/virtio/vhost-user.c            | 25 ++++++++++++++++---------
>>>   hw/virtio/vhost.c                 | 15 ++++++---------
>>>   hw/virtio/virtio-qmp.c            |  2 --
>>>   include/hw/virtio/vhost-backend.h |  2 ++
>>>   include/hw/virtio/vhost.h         |  7 -------
>>>   10 files changed, 30 insertions(+), 39 deletions(-)
>>>
>>> -- 
>>> 2.48.1
>>
> 

-- 
Best regards,
Vladimir