1. Patchew
  2. QEMU
  3. View series

[PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices

Stefano Garzarella posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221123131630.52020-1-sgarzare@redhat.com
Maintainers: "Gonglei (Arei)" <arei.gonglei@huawei.com>, "Michael S. Tsirkin" <mst@redhat.com>, Raphael Norwitz <raphael.norwitz@nutanix.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Jason Wang <jasowang@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Viresh Kumar <viresh.kumar@linaro.org>, Mathieu Poirier <mathieu.poirier@linaro.org>
include/hw/virtio/vhost.h      |  6 +++--
backends/cryptodev-vhost.c     |  4 ++--
backends/vhost-user.c          |  4 ++--
hw/block/vhost-user-blk.c      |  4 ++--
hw/net/vhost_net.c             |  8 +++----
hw/scsi/vhost-scsi-common.c    |  4 ++--
hw/virtio/vhost-user-fs.c      |  4 ++--
hw/virtio/vhost-user-gpio.c    |  4 ++--
hw/virtio/vhost-user-i2c.c     |  4 ++--
hw/virtio/vhost-user-rng.c     |  4 ++--
hw/virtio/vhost-vsock-common.c |  4 ++--
hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
hw/virtio/trace-events         |  4 ++--
13 files changed, 67 insertions(+), 31 deletions(-)
[PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefano Garzarella 1 year, 5 months ago 
Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
backend, but we forgot to enable vrings as specified in
docs/interop/vhost-user.rst:

    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
    ring starts directly in the enabled state.

    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
    initialized in a disabled state and is enabled by
    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.

Some vhost-user front-ends already did this by calling
vhost_ops.vhost_set_vring_enable() directly:
- backends/cryptodev-vhost.c
- hw/net/virtio-net.c
- hw/virtio/vhost-user-gpio.c

But most didn't do that, so we would leave the vrings disabled and some
backends would not work. We observed this issue with the rust version of
virtiofsd [1], which uses the event loop [2] provided by the
vhost-user-backend crate where requests are not processed if vring is
not enabled.

Let's fix this issue by enabling the vrings in vhost_dev_start() for
vhost-user front-ends that don't already do this directly. Same thing
also in vhost_dev_stop() where we disable vrings.

[1] https://gitlab.com/virtio-fs/virtiofsd
[2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217

Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
Reported-by: German Maglione <gmaglione@redhat.com>
Tested-by: German Maglione <gmaglione@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/hw/virtio/vhost.h      |  6 +++--
 backends/cryptodev-vhost.c     |  4 ++--
 backends/vhost-user.c          |  4 ++--
 hw/block/vhost-user-blk.c      |  4 ++--
 hw/net/vhost_net.c             |  8 +++----
 hw/scsi/vhost-scsi-common.c    |  4 ++--
 hw/virtio/vhost-user-fs.c      |  4 ++--
 hw/virtio/vhost-user-gpio.c    |  4 ++--
 hw/virtio/vhost-user-i2c.c     |  4 ++--
 hw/virtio/vhost-user-rng.c     |  4 ++--
 hw/virtio/vhost-vsock-common.c |  4 ++--
 hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
 hw/virtio/trace-events         |  4 ++--
 13 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 353252ac3e..67a6807fac 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
  * vhost_dev_start() - start the vhost device
  * @hdev: common vhost_dev structure
  * @vdev: the VirtIODevice structure
+ * @vrings: true to have vrings enabled in this call
  *
  * Starts the vhost device. From this point VirtIO feature negotiation
  * can start and the device can start processing VirtIO transactions.
  *
  * Return: 0 on success, < 0 on error.
  */
-int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
+int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
 
 /**
  * vhost_dev_stop() - stop the vhost device
  * @hdev: common vhost_dev structure
  * @vdev: the VirtIODevice structure
+ * @vrings: true to have vrings disabled in this call
  *
  * Stop the vhost device. After the device is stopped the notifiers
  * can be disabled (@vhost_dev_disable_notifiers) and the device can
  * be torn down (@vhost_dev_cleanup).
  */
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
+void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
 
 /**
  * DOC: vhost device configuration handling
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index bc13e466b4..572f87b3be 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
         goto fail_notifiers;
     }
 
-    r = vhost_dev_start(&crypto->dev, dev);
+    r = vhost_dev_start(&crypto->dev, dev, false);
     if (r < 0) {
         goto fail_start;
     }
@@ -111,7 +111,7 @@ static void
 cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
                                  VirtIODevice *dev)
 {
-    vhost_dev_stop(&crypto->dev, dev);
+    vhost_dev_stop(&crypto->dev, dev, false);
     vhost_dev_disable_notifiers(&crypto->dev, dev);
 }
 
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 5dedb2d987..7bfcaef976 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
     }
 
     b->dev.acked_features = b->vdev->guest_features;
-    ret = vhost_dev_start(&b->dev, b->vdev);
+    ret = vhost_dev_start(&b->dev, b->vdev, true);
     if (ret < 0) {
         error_report("Error start vhost dev");
         goto err_guest_notifiers;
@@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
         return;
     }
 
-    vhost_dev_stop(&b->dev, b->vdev);
+    vhost_dev_stop(&b->dev, b->vdev, true);
 
     if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent,
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 0d5190accf..1177064631 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
     }
 
     s->dev.vq_index_end = s->dev.nvqs;
-    ret = vhost_dev_start(&s->dev, vdev);
+    ret = vhost_dev_start(&s->dev, vdev, true);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Error starting vhost");
         goto err_guest_notifiers;
@@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&s->dev, vdev);
+    vhost_dev_stop(&s->dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 26e4930676..043058ff43 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
         goto fail_notifiers;
     }
 
-    r = vhost_dev_start(&net->dev, dev);
+    r = vhost_dev_start(&net->dev, dev, false);
     if (r < 0) {
         goto fail_start;
     }
@@ -308,7 +308,7 @@ fail:
     if (net->nc->info->poll) {
         net->nc->info->poll(net->nc, true);
     }
-    vhost_dev_stop(&net->dev, dev);
+    vhost_dev_stop(&net->dev, dev, false);
 fail_start:
     vhost_dev_disable_notifiers(&net->dev, dev);
 fail_notifiers:
@@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
     if (net->nc->info->poll) {
         net->nc->info->poll(net->nc, true);
     }
-    vhost_dev_stop(&net->dev, dev);
+    vhost_dev_stop(&net->dev, dev, false);
     if (net->nc->info->stop) {
         net->nc->info->stop(net->nc);
     }
@@ -606,7 +606,7 @@ err_start:
         assert(r >= 0);
     }
 
-    vhost_dev_stop(&net->dev, vdev);
+    vhost_dev_stop(&net->dev, vdev, false);
 
     return r;
 }
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index 767f827e55..18ea5dcfa1 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
         goto err_guest_notifiers;
     }
 
-    ret = vhost_dev_start(&vsc->dev, vdev);
+    ret = vhost_dev_start(&vsc->dev, vdev, true);
     if (ret < 0) {
         error_report("Error start vhost dev");
         goto err_guest_notifiers;
@@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     int ret = 0;
 
-    vhost_dev_stop(&vsc->dev, vdev);
+    vhost_dev_stop(&vsc->dev, vdev, true);
 
     if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index dc4014cdef..d97b179e6f 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
     }
 
     fs->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&fs->vhost_dev, vdev);
+    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost: %d", -ret);
         goto err_guest_notifiers;
@@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&fs->vhost_dev, vdev);
+    vhost_dev_stop(&fs->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 5851cb3bc9..0b40ebd15a 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
      */
     vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
 
-    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
+    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
     if (ret < 0) {
         error_report("Error starting vhost-user-gpio: %d", ret);
         goto err_guest_notifiers;
@@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(vhost_dev, vdev);
+    vhost_dev_stop(vhost_dev, vdev, false);
 
     ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 1c9f3d20dc..dc5c828ba6 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
 
     i2c->vhost_dev.acked_features = vdev->guest_features;
 
-    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
+    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost-user-i2c: %d", -ret);
         goto err_guest_notifiers;
@@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&i2c->vhost_dev, vdev);
+    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index f9084cde58..201a39e220 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
     }
 
     rng->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&rng->vhost_dev, vdev);
+    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost-user-rng: %d", -ret);
         goto err_guest_notifiers;
@@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&rng->vhost_dev, vdev);
+    vhost_dev_stop(&rng->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index a67a275de2..d21c72b401 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
     }
 
     vvc->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
+    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
     if (ret < 0) {
         error_report("Error starting vhost: %d", -ret);
         goto err_guest_notifiers;
@@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
         return;
     }
 
-    vhost_dev_stop(&vvc->vhost_dev, vdev);
+    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
 
     ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
     if (ret < 0) {
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d1c4c20b8c..7fb008bc9e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
     return 0;
 }
 
+static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
+{
+    if (!hdev->vhost_ops->vhost_set_vring_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)) {
+        return 0;
+    }
+
+    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
+}
+
 /* Host notifiers must be enabled at this point. */
-int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
+int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
 {
     int i, r;
 
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    trace_vhost_dev_start(hdev, vdev->name);
+    trace_vhost_dev_start(hdev, vdev->name, vrings);
 
     vdev->vhost_started = true;
     hdev->started = true;
@@ -1830,10 +1851,16 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
             goto fail_log;
         }
     }
+    if (vrings) {
+        r = vhost_dev_set_vring_enable(hdev, true);
+        if (r) {
+            goto fail_log;
+        }
+    }
     if (hdev->vhost_ops->vhost_dev_start) {
         r = hdev->vhost_ops->vhost_dev_start(hdev, true);
         if (r) {
-            goto fail_log;
+            goto fail_start;
         }
     }
     if (vhost_dev_has_iommu(hdev) &&
@@ -1848,6 +1875,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
     }
     return 0;
+fail_start:
+    if (vrings) {
+        vhost_dev_set_vring_enable(hdev, false);
+    }
 fail_log:
     vhost_log_put(hdev, false);
 fail_vq:
@@ -1866,18 +1897,21 @@ fail_features:
 }
 
 /* Host notifiers must be enabled at this point. */
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
+void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
 {
     int i;
 
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    trace_vhost_dev_stop(hdev, vdev->name);
+    trace_vhost_dev_stop(hdev, vdev->name, vrings);
 
     if (hdev->vhost_ops->vhost_dev_start) {
         hdev->vhost_ops->vhost_dev_start(hdev, false);
     }
+    if (vrings) {
+        vhost_dev_set_vring_enable(hdev, false);
+    }
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_stop(hdev,
                              vdev,
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 820dadc26c..14fc5b9bb2 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -9,8 +9,8 @@ vhost_section(const char *name) "%s"
 vhost_reject_section(const char *name, int d) "%s:%d"
 vhost_iotlb_miss(void *dev, int step) "%p step %d"
 vhost_dev_cleanup(void *dev) "%p"
-vhost_dev_start(void *dev, const char *name) "%p:%s"
-vhost_dev_stop(void *dev, const char *name) "%p:%s"
+vhost_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
+vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
 
 
 # vhost-user.c
-- 
2.38.1
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefan Hajnoczi 1 year, 5 months ago 
On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote:
> Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
> backend, but we forgot to enable vrings as specified in
> docs/interop/vhost-user.rst:
> 
>     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>     ring starts directly in the enabled state.
> 
>     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>     initialized in a disabled state and is enabled by
>     ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
> 
> Some vhost-user front-ends already did this by calling
> vhost_ops.vhost_set_vring_enable() directly:
> - backends/cryptodev-vhost.c
> - hw/net/virtio-net.c
> - hw/virtio/vhost-user-gpio.c
> 
> But most didn't do that, so we would leave the vrings disabled and some
> backends would not work. We observed this issue with the rust version of
> virtiofsd [1], which uses the event loop [2] provided by the
> vhost-user-backend crate where requests are not processed if vring is
> not enabled.
> 
> Let's fix this issue by enabling the vrings in vhost_dev_start() for
> vhost-user front-ends that don't already do this directly. Same thing
> also in vhost_dev_stop() where we disable vrings.
> 
> [1] https://gitlab.com/virtio-fs/virtiofsd
> [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
> 
> Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> Reported-by: German Maglione <gmaglione@redhat.com>
> Tested-by: German Maglione <gmaglione@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  include/hw/virtio/vhost.h      |  6 +++--
>  backends/cryptodev-vhost.c     |  4 ++--
>  backends/vhost-user.c          |  4 ++--
>  hw/block/vhost-user-blk.c      |  4 ++--
>  hw/net/vhost_net.c             |  8 +++----
>  hw/scsi/vhost-scsi-common.c    |  4 ++--
>  hw/virtio/vhost-user-fs.c      |  4 ++--
>  hw/virtio/vhost-user-gpio.c    |  4 ++--
>  hw/virtio/vhost-user-i2c.c     |  4 ++--
>  hw/virtio/vhost-user-rng.c     |  4 ++--
>  hw/virtio/vhost-vsock-common.c |  4 ++--
>  hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
>  hw/virtio/trace-events         |  4 ++--
>  13 files changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 353252ac3e..67a6807fac 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
>   * vhost_dev_start() - start the vhost device
>   * @hdev: common vhost_dev structure
>   * @vdev: the VirtIODevice structure
> + * @vrings: true to have vrings enabled in this call
>   *
>   * Starts the vhost device. From this point VirtIO feature negotiation
>   * can start and the device can start processing VirtIO transactions.
>   *
>   * Return: 0 on success, < 0 on error.
>   */
> -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>  
>  /**
>   * vhost_dev_stop() - stop the vhost device
>   * @hdev: common vhost_dev structure
>   * @vdev: the VirtIODevice structure
> + * @vrings: true to have vrings disabled in this call
>   *
>   * Stop the vhost device. After the device is stopped the notifiers
>   * can be disabled (@vhost_dev_disable_notifiers) and the device can
>   * be torn down (@vhost_dev_cleanup).
>   */
> -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>  
>  /**
>   * DOC: vhost device configuration handling
> diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> index bc13e466b4..572f87b3be 100644
> --- a/backends/cryptodev-vhost.c
> +++ b/backends/cryptodev-vhost.c
> @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
>          goto fail_notifiers;
>      }
>  
> -    r = vhost_dev_start(&crypto->dev, dev);
> +    r = vhost_dev_start(&crypto->dev, dev, false);
>      if (r < 0) {
>          goto fail_start;
>      }
> @@ -111,7 +111,7 @@ static void
>  cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
>                                   VirtIODevice *dev)
>  {
> -    vhost_dev_stop(&crypto->dev, dev);
> +    vhost_dev_stop(&crypto->dev, dev, false);
>      vhost_dev_disable_notifiers(&crypto->dev, dev);
>  }
>  
> diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> index 5dedb2d987..7bfcaef976 100644
> --- a/backends/vhost-user.c
> +++ b/backends/vhost-user.c
> @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
>      }
>  
>      b->dev.acked_features = b->vdev->guest_features;
> -    ret = vhost_dev_start(&b->dev, b->vdev);
> +    ret = vhost_dev_start(&b->dev, b->vdev, true);
>      if (ret < 0) {
>          error_report("Error start vhost dev");
>          goto err_guest_notifiers;
> @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
>          return;
>      }
>  
> -    vhost_dev_stop(&b->dev, b->vdev);
> +    vhost_dev_stop(&b->dev, b->vdev, true);
>  
>      if (k->set_guest_notifiers) {
>          ret = k->set_guest_notifiers(qbus->parent,
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 0d5190accf..1177064631 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>      }
>  
>      s->dev.vq_index_end = s->dev.nvqs;
> -    ret = vhost_dev_start(&s->dev, vdev);
> +    ret = vhost_dev_start(&s->dev, vdev, true);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Error starting vhost");
>          goto err_guest_notifiers;
> @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>          return;
>      }
>  
> -    vhost_dev_stop(&s->dev, vdev);
> +    vhost_dev_stop(&s->dev, vdev, true);
>  
>      ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>      if (ret < 0) {
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 26e4930676..043058ff43 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>          goto fail_notifiers;
>      }
>  
> -    r = vhost_dev_start(&net->dev, dev);
> +    r = vhost_dev_start(&net->dev, dev, false);
>      if (r < 0) {
>          goto fail_start;
>      }
> @@ -308,7 +308,7 @@ fail:
>      if (net->nc->info->poll) {
>          net->nc->info->poll(net->nc, true);
>      }
> -    vhost_dev_stop(&net->dev, dev);
> +    vhost_dev_stop(&net->dev, dev, false);
>  fail_start:
>      vhost_dev_disable_notifiers(&net->dev, dev);
>  fail_notifiers:
> @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
>      if (net->nc->info->poll) {
>          net->nc->info->poll(net->nc, true);
>      }
> -    vhost_dev_stop(&net->dev, dev);
> +    vhost_dev_stop(&net->dev, dev, false);
>      if (net->nc->info->stop) {
>          net->nc->info->stop(net->nc);
>      }
> @@ -606,7 +606,7 @@ err_start:
>          assert(r >= 0);
>      }
>  
> -    vhost_dev_stop(&net->dev, vdev);
> +    vhost_dev_stop(&net->dev, vdev, false);
>  
>      return r;
>  }
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index 767f827e55..18ea5dcfa1 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>          goto err_guest_notifiers;
>      }
>  
> -    ret = vhost_dev_start(&vsc->dev, vdev);
> +    ret = vhost_dev_start(&vsc->dev, vdev, true);
>      if (ret < 0) {
>          error_report("Error start vhost dev");
>          goto err_guest_notifiers;
> @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      int ret = 0;
>  
> -    vhost_dev_stop(&vsc->dev, vdev);
> +    vhost_dev_stop(&vsc->dev, vdev, true);
>  
>      if (k->set_guest_notifiers) {
>          ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index dc4014cdef..d97b179e6f 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
>      }
>  
>      fs->vhost_dev.acked_features = vdev->guest_features;
> -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
> +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
>      if (ret < 0) {
>          error_report("Error starting vhost: %d", -ret);
>          goto err_guest_notifiers;
> @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
>          return;
>      }
>  
> -    vhost_dev_stop(&fs->vhost_dev, vdev);
> +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
>  
>      ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
>      if (ret < 0) {
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 5851cb3bc9..0b40ebd15a 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
>       */
>      vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
>  
> -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
> +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
>      if (ret < 0) {
>          error_report("Error starting vhost-user-gpio: %d", ret);
>          goto err_guest_notifiers;
> @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
>          return;
>      }
>  
> -    vhost_dev_stop(vhost_dev, vdev);
> +    vhost_dev_stop(vhost_dev, vdev, false);
>  
>      ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
>      if (ret < 0) {
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index 1c9f3d20dc..dc5c828ba6 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
>  
>      i2c->vhost_dev.acked_features = vdev->guest_features;
>  
> -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
> +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
>      if (ret < 0) {
>          error_report("Error starting vhost-user-i2c: %d", -ret);
>          goto err_guest_notifiers;
> @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>          return;
>      }
>  
> -    vhost_dev_stop(&i2c->vhost_dev, vdev);
> +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
>  
>      ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
>      if (ret < 0) {
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index f9084cde58..201a39e220 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
>      }
>  
>      rng->vhost_dev.acked_features = vdev->guest_features;
> -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
> +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
>      if (ret < 0) {
>          error_report("Error starting vhost-user-rng: %d", -ret);
>          goto err_guest_notifiers;
> @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>          return;
>      }
>  
> -    vhost_dev_stop(&rng->vhost_dev, vdev);
> +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
>  
>      ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
>      if (ret < 0) {
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index a67a275de2..d21c72b401 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
>      }
>  
>      vvc->vhost_dev.acked_features = vdev->guest_features;
> -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
> +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
>      if (ret < 0) {
>          error_report("Error starting vhost: %d", -ret);
>          goto err_guest_notifiers;
> @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
>          return;
>      }
>  
> -    vhost_dev_stop(&vvc->vhost_dev, vdev);
> +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
>  
>      ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
>      if (ret < 0) {
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index d1c4c20b8c..7fb008bc9e 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
>      return 0;
>  }
>  
> +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)

There is a similarly-named vhost_set_vring_enable(NetClientState *nc,
int enable) function which is actually part of vhost_net. Please rename
it to vhost_net_set_vring_enable(). It should probably call
vhost_dev_set_vring_enable().

> +{
> +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
> +        return 0;
> +    }

These semantics are the opposite of vhost_user_set_vring_enable():

  if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
      return -EINVAL;
  }

Please make vhost_user_set_vring_enable() and
vhost_dev_set_vring_enable() consistent. Code gets really confusing when
layers have different semantics for the same operation.

> +
> +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
> +}

The return value is hard to understand. An error return is only returned
by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are
other cases that seem like they should return an error but return
success instead. For example, when called with enable=false on a
non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy
vhost-user) we return success even though the vring wasn't disabled.

I know a quick fix is needed for 7.2, but I don't think this patch
should be merged in its current state. Given that this is a new
function, it should be possible to come up with straightforward return
value semantics.

> +
>  /* Host notifiers must be enabled at this point. */
> -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
>  {
>      int i, r;
>  
>      /* should only be called after backend is connected */
>      assert(hdev->vhost_ops);
>  
> -    trace_vhost_dev_start(hdev, vdev->name);
> +    trace_vhost_dev_start(hdev, vdev->name, vrings);
>  
>      vdev->vhost_started = true;
>      hdev->started = true;
> @@ -1830,10 +1851,16 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>              goto fail_log;
>          }
>      }
> +    if (vrings) {
> +        r = vhost_dev_set_vring_enable(hdev, true);
> +        if (r) {
> +            goto fail_log;
> +        }
> +    }
>      if (hdev->vhost_ops->vhost_dev_start) {
>          r = hdev->vhost_ops->vhost_dev_start(hdev, true);
>          if (r) {
> -            goto fail_log;
> +            goto fail_start;
>          }
>      }
>      if (vhost_dev_has_iommu(hdev) &&
> @@ -1848,6 +1875,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>          }
>      }
>      return 0;
> +fail_start:
> +    if (vrings) {
> +        vhost_dev_set_vring_enable(hdev, false);
> +    }
>  fail_log:
>      vhost_log_put(hdev, false);
>  fail_vq:
> @@ -1866,18 +1897,21 @@ fail_features:
>  }
>  
>  /* Host notifiers must be enabled at this point. */
> -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
>  {
>      int i;
>  
>      /* should only be called after backend is connected */
>      assert(hdev->vhost_ops);
>  
> -    trace_vhost_dev_stop(hdev, vdev->name);
> +    trace_vhost_dev_stop(hdev, vdev->name, vrings);
>  
>      if (hdev->vhost_ops->vhost_dev_start) {
>          hdev->vhost_ops->vhost_dev_start(hdev, false);
>      }
> +    if (vrings) {
> +        vhost_dev_set_vring_enable(hdev, false);
> +    }
>      for (i = 0; i < hdev->nvqs; ++i) {
>          vhost_virtqueue_stop(hdev,
>                               vdev,
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 820dadc26c..14fc5b9bb2 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -9,8 +9,8 @@ vhost_section(const char *name) "%s"
>  vhost_reject_section(const char *name, int d) "%s:%d"
>  vhost_iotlb_miss(void *dev, int step) "%p step %d"
>  vhost_dev_cleanup(void *dev) "%p"
> -vhost_dev_start(void *dev, const char *name) "%p:%s"
> -vhost_dev_stop(void *dev, const char *name) "%p:%s"
> +vhost_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> +vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
>  
>  
>  # vhost-user.c
> -- 
> 2.38.1
>
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefano Garzarella 1 year, 5 months ago 
On Thu, Nov 24, 2022 at 01:36:29PM -0500, Stefan Hajnoczi wrote:
>On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote:
>> Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
>> backend, but we forgot to enable vrings as specified in
>> docs/interop/vhost-user.rst:
>>
>>     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>>     ring starts directly in the enabled state.
>>
>>     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>>     initialized in a disabled state and is enabled by
>>     ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>>
>> Some vhost-user front-ends already did this by calling
>> vhost_ops.vhost_set_vring_enable() directly:
>> - backends/cryptodev-vhost.c
>> - hw/net/virtio-net.c
>> - hw/virtio/vhost-user-gpio.c
>>
>> But most didn't do that, so we would leave the vrings disabled and some
>> backends would not work. We observed this issue with the rust version of
>> virtiofsd [1], which uses the event loop [2] provided by the
>> vhost-user-backend crate where requests are not processed if vring is
>> not enabled.
>>
>> Let's fix this issue by enabling the vrings in vhost_dev_start() for
>> vhost-user front-ends that don't already do this directly. Same thing
>> also in vhost_dev_stop() where we disable vrings.
>>
>> [1] https://gitlab.com/virtio-fs/virtiofsd
>> [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
>>
>> Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> Reported-by: German Maglione <gmaglione@redhat.com>
>> Tested-by: German Maglione <gmaglione@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  include/hw/virtio/vhost.h      |  6 +++--
>>  backends/cryptodev-vhost.c     |  4 ++--
>>  backends/vhost-user.c          |  4 ++--
>>  hw/block/vhost-user-blk.c      |  4 ++--
>>  hw/net/vhost_net.c             |  8 +++----
>>  hw/scsi/vhost-scsi-common.c    |  4 ++--
>>  hw/virtio/vhost-user-fs.c      |  4 ++--
>>  hw/virtio/vhost-user-gpio.c    |  4 ++--
>>  hw/virtio/vhost-user-i2c.c     |  4 ++--
>>  hw/virtio/vhost-user-rng.c     |  4 ++--
>>  hw/virtio/vhost-vsock-common.c |  4 ++--
>>  hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
>>  hw/virtio/trace-events         |  4 ++--
>>  13 files changed, 67 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index 353252ac3e..67a6807fac 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
>>   * vhost_dev_start() - start the vhost device
>>   * @hdev: common vhost_dev structure
>>   * @vdev: the VirtIODevice structure
>> + * @vrings: true to have vrings enabled in this call
>>   *
>>   * Starts the vhost device. From this point VirtIO feature negotiation
>>   * can start and the device can start processing VirtIO transactions.
>>   *
>>   * Return: 0 on success, < 0 on error.
>>   */
>> -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>> +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>>
>>  /**
>>   * vhost_dev_stop() - stop the vhost device
>>   * @hdev: common vhost_dev structure
>>   * @vdev: the VirtIODevice structure
>> + * @vrings: true to have vrings disabled in this call
>>   *
>>   * Stop the vhost device. After the device is stopped the notifiers
>>   * can be disabled (@vhost_dev_disable_notifiers) and the device can
>>   * be torn down (@vhost_dev_cleanup).
>>   */
>> -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>> +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>>
>>  /**
>>   * DOC: vhost device configuration handling
>> diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
>> index bc13e466b4..572f87b3be 100644
>> --- a/backends/cryptodev-vhost.c
>> +++ b/backends/cryptodev-vhost.c
>> @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
>>          goto fail_notifiers;
>>      }
>>
>> -    r = vhost_dev_start(&crypto->dev, dev);
>> +    r = vhost_dev_start(&crypto->dev, dev, false);
>>      if (r < 0) {
>>          goto fail_start;
>>      }
>> @@ -111,7 +111,7 @@ static void
>>  cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
>>                                   VirtIODevice *dev)
>>  {
>> -    vhost_dev_stop(&crypto->dev, dev);
>> +    vhost_dev_stop(&crypto->dev, dev, false);
>>      vhost_dev_disable_notifiers(&crypto->dev, dev);
>>  }
>>
>> diff --git a/backends/vhost-user.c b/backends/vhost-user.c
>> index 5dedb2d987..7bfcaef976 100644
>> --- a/backends/vhost-user.c
>> +++ b/backends/vhost-user.c
>> @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
>>      }
>>
>>      b->dev.acked_features = b->vdev->guest_features;
>> -    ret = vhost_dev_start(&b->dev, b->vdev);
>> +    ret = vhost_dev_start(&b->dev, b->vdev, true);
>>      if (ret < 0) {
>>          error_report("Error start vhost dev");
>>          goto err_guest_notifiers;
>> @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(&b->dev, b->vdev);
>> +    vhost_dev_stop(&b->dev, b->vdev, true);
>>
>>      if (k->set_guest_notifiers) {
>>          ret = k->set_guest_notifiers(qbus->parent,
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 0d5190accf..1177064631 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>>      }
>>
>>      s->dev.vq_index_end = s->dev.nvqs;
>> -    ret = vhost_dev_start(&s->dev, vdev);
>> +    ret = vhost_dev_start(&s->dev, vdev, true);
>>      if (ret < 0) {
>>          error_setg_errno(errp, -ret, "Error starting vhost");
>>          goto err_guest_notifiers;
>> @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(&s->dev, vdev);
>> +    vhost_dev_stop(&s->dev, vdev, true);
>>
>>      ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>>      if (ret < 0) {
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 26e4930676..043058ff43 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>>          goto fail_notifiers;
>>      }
>>
>> -    r = vhost_dev_start(&net->dev, dev);
>> +    r = vhost_dev_start(&net->dev, dev, false);
>>      if (r < 0) {
>>          goto fail_start;
>>      }
>> @@ -308,7 +308,7 @@ fail:
>>      if (net->nc->info->poll) {
>>          net->nc->info->poll(net->nc, true);
>>      }
>> -    vhost_dev_stop(&net->dev, dev);
>> +    vhost_dev_stop(&net->dev, dev, false);
>>  fail_start:
>>      vhost_dev_disable_notifiers(&net->dev, dev);
>>  fail_notifiers:
>> @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
>>      if (net->nc->info->poll) {
>>          net->nc->info->poll(net->nc, true);
>>      }
>> -    vhost_dev_stop(&net->dev, dev);
>> +    vhost_dev_stop(&net->dev, dev, false);
>>      if (net->nc->info->stop) {
>>          net->nc->info->stop(net->nc);
>>      }
>> @@ -606,7 +606,7 @@ err_start:
>>          assert(r >= 0);
>>      }
>>
>> -    vhost_dev_stop(&net->dev, vdev);
>> +    vhost_dev_stop(&net->dev, vdev, false);
>>
>>      return r;
>>  }
>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>> index 767f827e55..18ea5dcfa1 100644
>> --- a/hw/scsi/vhost-scsi-common.c
>> +++ b/hw/scsi/vhost-scsi-common.c
>> @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>          goto err_guest_notifiers;
>>      }
>>
>> -    ret = vhost_dev_start(&vsc->dev, vdev);
>> +    ret = vhost_dev_start(&vsc->dev, vdev, true);
>>      if (ret < 0) {
>>          error_report("Error start vhost dev");
>>          goto err_guest_notifiers;
>> @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>>      int ret = 0;
>>
>> -    vhost_dev_stop(&vsc->dev, vdev);
>> +    vhost_dev_stop(&vsc->dev, vdev, true);
>>
>>      if (k->set_guest_notifiers) {
>>          ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index dc4014cdef..d97b179e6f 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
>>      }
>>
>>      fs->vhost_dev.acked_features = vdev->guest_features;
>> -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
>> +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
>>      if (ret < 0) {
>>          error_report("Error starting vhost: %d", -ret);
>>          goto err_guest_notifiers;
>> @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(&fs->vhost_dev, vdev);
>> +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
>>
>>      ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
>>      if (ret < 0) {
>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>> index 5851cb3bc9..0b40ebd15a 100644
>> --- a/hw/virtio/vhost-user-gpio.c
>> +++ b/hw/virtio/vhost-user-gpio.c
>> @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
>>       */
>>      vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
>>
>> -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
>> +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
>>      if (ret < 0) {
>>          error_report("Error starting vhost-user-gpio: %d", ret);
>>          goto err_guest_notifiers;
>> @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(vhost_dev, vdev);
>> +    vhost_dev_stop(vhost_dev, vdev, false);
>>
>>      ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
>>      if (ret < 0) {
>> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>> index 1c9f3d20dc..dc5c828ba6 100644
>> --- a/hw/virtio/vhost-user-i2c.c
>> +++ b/hw/virtio/vhost-user-i2c.c
>> @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
>>
>>      i2c->vhost_dev.acked_features = vdev->guest_features;
>>
>> -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
>> +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
>>      if (ret < 0) {
>>          error_report("Error starting vhost-user-i2c: %d", -ret);
>>          goto err_guest_notifiers;
>> @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(&i2c->vhost_dev, vdev);
>> +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
>>
>>      ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
>>      if (ret < 0) {
>> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>> index f9084cde58..201a39e220 100644
>> --- a/hw/virtio/vhost-user-rng.c
>> +++ b/hw/virtio/vhost-user-rng.c
>> @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
>>      }
>>
>>      rng->vhost_dev.acked_features = vdev->guest_features;
>> -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
>> +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
>>      if (ret < 0) {
>>          error_report("Error starting vhost-user-rng: %d", -ret);
>>          goto err_guest_notifiers;
>> @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(&rng->vhost_dev, vdev);
>> +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
>>
>>      ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
>>      if (ret < 0) {
>> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>> index a67a275de2..d21c72b401 100644
>> --- a/hw/virtio/vhost-vsock-common.c
>> +++ b/hw/virtio/vhost-vsock-common.c
>> @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
>>      }
>>
>>      vvc->vhost_dev.acked_features = vdev->guest_features;
>> -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
>> +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
>>      if (ret < 0) {
>>          error_report("Error starting vhost: %d", -ret);
>>          goto err_guest_notifiers;
>> @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
>>          return;
>>      }
>>
>> -    vhost_dev_stop(&vvc->vhost_dev, vdev);
>> +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
>>
>>      ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
>>      if (ret < 0) {
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index d1c4c20b8c..7fb008bc9e 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
>>      return 0;
>>  }
>>
>> +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
>
>There is a similarly-named vhost_set_vring_enable(NetClientState *nc,
>int enable) function which is actually part of vhost_net. Please rename
>it to vhost_net_set_vring_enable(). 

Should I rename it in this patch?

> It should probably call
>vhost_dev_set_vring_enable().

Ehm, the idea of this patch was to touch as little as possible to avoid 
new regressions.

Also, the semantics of vhost_dev_set_vring_enable() was meant to keep 
vhost_dev_start()/vhost_dev_stop() simple, not to be exposed to 
frontends. (maybe I should have written it, sorry about that).

However I agree that we should clean up vhost-net and also the other 
frontends as Raphael also suggested, but honestly I'm scared to do that 
now in this patch...

What I would have wanted to do, would be similar to what we do for 
vhost-vdpa: call SET_VRING_ENABLE in the vhost_ops->vhost_dev_start() 
callback of vhost-user.c.
Removing all the call to vhost_ops->vhost_set_vring_enable() in the 
frontends, but I think it's too risky to do that now.

>
>> +{
>> +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
>> +        return 0;
>> +    }
>
>These semantics are the opposite of vhost_user_set_vring_enable():
>
>  if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>      return -EINVAL;
>  }
>
>Please make vhost_user_set_vring_enable() and
>vhost_dev_set_vring_enable() consistent. Code gets really confusing when
>layers have different semantics for the same operation.

It's the opposite precisely because we shouldn't let 
vhost_dev_start()/vhost_dev_stop() fail if 
vhost_ops->vhost_set_vring_enable() can't be called because it would 
fail.

If I do it this way, then I have to put the check inside 
vhost_dev_start()/vhost_dev_stop(), and at this point I remove the 
function that would be useless (just a wrapper of 
hdev->vhost_ops->vhost_set_vring_enable).
Actually this was the first implementation I did, then I added the 
function just to have vhost_dev_start()/vhost_dev_stop() cleaner and to 
avoid duplicating the check.

>
>> +
>> +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
>> +}
>
>The return value is hard to understand. An error return is only returned
>by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are
>other cases that seem like they should return an error but return
>success instead. For example, when called with enable=false on a
>non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy
>vhost-user) we return success even though the vring wasn't disabled.

As I explained above, the idea was not to expose this function outside, 
but to use it only in vhost_dev_start()/vhost_dev_stop(). So the return 
value is 0 both when it has successes and when there is no need/way to 
enable/disable the vrings.

Perhaps since it is confusing, I will remove the function and put the 
code directly into vhost_dev_start()/vhost_dev_stop().

What do you think?

Thanks,
Stefano
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefan Hajnoczi 1 year, 4 months ago 
On Fri, Nov 25, 2022 at 09:12:43AM +0100, Stefano Garzarella wrote:
> On Thu, Nov 24, 2022 at 01:36:29PM -0500, Stefan Hajnoczi wrote:
> > On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote:
> > > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> > > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
> > > backend, but we forgot to enable vrings as specified in
> > > docs/interop/vhost-user.rst:
> > > 
> > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
> > >     ring starts directly in the enabled state.
> > > 
> > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
> > >     initialized in a disabled state and is enabled by
> > >     ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
> > > 
> > > Some vhost-user front-ends already did this by calling
> > > vhost_ops.vhost_set_vring_enable() directly:
> > > - backends/cryptodev-vhost.c
> > > - hw/net/virtio-net.c
> > > - hw/virtio/vhost-user-gpio.c
> > > 
> > > But most didn't do that, so we would leave the vrings disabled and some
> > > backends would not work. We observed this issue with the rust version of
> > > virtiofsd [1], which uses the event loop [2] provided by the
> > > vhost-user-backend crate where requests are not processed if vring is
> > > not enabled.
> > > 
> > > Let's fix this issue by enabling the vrings in vhost_dev_start() for
> > > vhost-user front-ends that don't already do this directly. Same thing
> > > also in vhost_dev_stop() where we disable vrings.
> > > 
> > > [1] https://gitlab.com/virtio-fs/virtiofsd
> > > [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
> > > 
> > > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> > > Reported-by: German Maglione <gmaglione@redhat.com>
> > > Tested-by: German Maglione <gmaglione@redhat.com>
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > >  include/hw/virtio/vhost.h      |  6 +++--
> > >  backends/cryptodev-vhost.c     |  4 ++--
> > >  backends/vhost-user.c          |  4 ++--
> > >  hw/block/vhost-user-blk.c      |  4 ++--
> > >  hw/net/vhost_net.c             |  8 +++----
> > >  hw/scsi/vhost-scsi-common.c    |  4 ++--
> > >  hw/virtio/vhost-user-fs.c      |  4 ++--
> > >  hw/virtio/vhost-user-gpio.c    |  4 ++--
> > >  hw/virtio/vhost-user-i2c.c     |  4 ++--
> > >  hw/virtio/vhost-user-rng.c     |  4 ++--
> > >  hw/virtio/vhost-vsock-common.c |  4 ++--
> > >  hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
> > >  hw/virtio/trace-events         |  4 ++--
> > >  13 files changed, 67 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index 353252ac3e..67a6807fac 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> > >   * vhost_dev_start() - start the vhost device
> > >   * @hdev: common vhost_dev structure
> > >   * @vdev: the VirtIODevice structure
> > > + * @vrings: true to have vrings enabled in this call
> > >   *
> > >   * Starts the vhost device. From this point VirtIO feature negotiation
> > >   * can start and the device can start processing VirtIO transactions.
> > >   *
> > >   * Return: 0 on success, < 0 on error.
> > >   */
> > > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> > > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> > > 
> > >  /**
> > >   * vhost_dev_stop() - stop the vhost device
> > >   * @hdev: common vhost_dev structure
> > >   * @vdev: the VirtIODevice structure
> > > + * @vrings: true to have vrings disabled in this call
> > >   *
> > >   * Stop the vhost device. After the device is stopped the notifiers
> > >   * can be disabled (@vhost_dev_disable_notifiers) and the device can
> > >   * be torn down (@vhost_dev_cleanup).
> > >   */
> > > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> > > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> > > 
> > >  /**
> > >   * DOC: vhost device configuration handling
> > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> > > index bc13e466b4..572f87b3be 100644
> > > --- a/backends/cryptodev-vhost.c
> > > +++ b/backends/cryptodev-vhost.c
> > > @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
> > >          goto fail_notifiers;
> > >      }
> > > 
> > > -    r = vhost_dev_start(&crypto->dev, dev);
> > > +    r = vhost_dev_start(&crypto->dev, dev, false);
> > >      if (r < 0) {
> > >          goto fail_start;
> > >      }
> > > @@ -111,7 +111,7 @@ static void
> > >  cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
> > >                                   VirtIODevice *dev)
> > >  {
> > > -    vhost_dev_stop(&crypto->dev, dev);
> > > +    vhost_dev_stop(&crypto->dev, dev, false);
> > >      vhost_dev_disable_notifiers(&crypto->dev, dev);
> > >  }
> > > 
> > > diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> > > index 5dedb2d987..7bfcaef976 100644
> > > --- a/backends/vhost-user.c
> > > +++ b/backends/vhost-user.c
> > > @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
> > >      }
> > > 
> > >      b->dev.acked_features = b->vdev->guest_features;
> > > -    ret = vhost_dev_start(&b->dev, b->vdev);
> > > +    ret = vhost_dev_start(&b->dev, b->vdev, true);
> > >      if (ret < 0) {
> > >          error_report("Error start vhost dev");
> > >          goto err_guest_notifiers;
> > > @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(&b->dev, b->vdev);
> > > +    vhost_dev_stop(&b->dev, b->vdev, true);
> > > 
> > >      if (k->set_guest_notifiers) {
> > >          ret = k->set_guest_notifiers(qbus->parent,
> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > index 0d5190accf..1177064631 100644
> > > --- a/hw/block/vhost-user-blk.c
> > > +++ b/hw/block/vhost-user-blk.c
> > > @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
> > >      }
> > > 
> > >      s->dev.vq_index_end = s->dev.nvqs;
> > > -    ret = vhost_dev_start(&s->dev, vdev);
> > > +    ret = vhost_dev_start(&s->dev, vdev, true);
> > >      if (ret < 0) {
> > >          error_setg_errno(errp, -ret, "Error starting vhost");
> > >          goto err_guest_notifiers;
> > > @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(&s->dev, vdev);
> > > +    vhost_dev_stop(&s->dev, vdev, true);
> > > 
> > >      ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> > >      if (ret < 0) {
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 26e4930676..043058ff43 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
> > >          goto fail_notifiers;
> > >      }
> > > 
> > > -    r = vhost_dev_start(&net->dev, dev);
> > > +    r = vhost_dev_start(&net->dev, dev, false);
> > >      if (r < 0) {
> > >          goto fail_start;
> > >      }
> > > @@ -308,7 +308,7 @@ fail:
> > >      if (net->nc->info->poll) {
> > >          net->nc->info->poll(net->nc, true);
> > >      }
> > > -    vhost_dev_stop(&net->dev, dev);
> > > +    vhost_dev_stop(&net->dev, dev, false);
> > >  fail_start:
> > >      vhost_dev_disable_notifiers(&net->dev, dev);
> > >  fail_notifiers:
> > > @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> > >      if (net->nc->info->poll) {
> > >          net->nc->info->poll(net->nc, true);
> > >      }
> > > -    vhost_dev_stop(&net->dev, dev);
> > > +    vhost_dev_stop(&net->dev, dev, false);
> > >      if (net->nc->info->stop) {
> > >          net->nc->info->stop(net->nc);
> > >      }
> > > @@ -606,7 +606,7 @@ err_start:
> > >          assert(r >= 0);
> > >      }
> > > 
> > > -    vhost_dev_stop(&net->dev, vdev);
> > > +    vhost_dev_stop(&net->dev, vdev, false);
> > > 
> > >      return r;
> > >  }
> > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > > index 767f827e55..18ea5dcfa1 100644
> > > --- a/hw/scsi/vhost-scsi-common.c
> > > +++ b/hw/scsi/vhost-scsi-common.c
> > > @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > >          goto err_guest_notifiers;
> > >      }
> > > 
> > > -    ret = vhost_dev_start(&vsc->dev, vdev);
> > > +    ret = vhost_dev_start(&vsc->dev, vdev, true);
> > >      if (ret < 0) {
> > >          error_report("Error start vhost dev");
> > >          goto err_guest_notifiers;
> > > @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > >      int ret = 0;
> > > 
> > > -    vhost_dev_stop(&vsc->dev, vdev);
> > > +    vhost_dev_stop(&vsc->dev, vdev, true);
> > > 
> > >      if (k->set_guest_notifiers) {
> > >          ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > index dc4014cdef..d97b179e6f 100644
> > > --- a/hw/virtio/vhost-user-fs.c
> > > +++ b/hw/virtio/vhost-user-fs.c
> > > @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
> > >      }
> > > 
> > >      fs->vhost_dev.acked_features = vdev->guest_features;
> > > -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
> > > +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
> > >      if (ret < 0) {
> > >          error_report("Error starting vhost: %d", -ret);
> > >          goto err_guest_notifiers;
> > > @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(&fs->vhost_dev, vdev);
> > > +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
> > > 
> > >      ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
> > >      if (ret < 0) {
> > > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> > > index 5851cb3bc9..0b40ebd15a 100644
> > > --- a/hw/virtio/vhost-user-gpio.c
> > > +++ b/hw/virtio/vhost-user-gpio.c
> > > @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
> > >       */
> > >      vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
> > > 
> > > -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
> > > +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
> > >      if (ret < 0) {
> > >          error_report("Error starting vhost-user-gpio: %d", ret);
> > >          goto err_guest_notifiers;
> > > @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(vhost_dev, vdev);
> > > +    vhost_dev_stop(vhost_dev, vdev, false);
> > > 
> > >      ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
> > >      if (ret < 0) {
> > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > > index 1c9f3d20dc..dc5c828ba6 100644
> > > --- a/hw/virtio/vhost-user-i2c.c
> > > +++ b/hw/virtio/vhost-user-i2c.c
> > > @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
> > > 
> > >      i2c->vhost_dev.acked_features = vdev->guest_features;
> > > 
> > > -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
> > > +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
> > >      if (ret < 0) {
> > >          error_report("Error starting vhost-user-i2c: %d", -ret);
> > >          goto err_guest_notifiers;
> > > @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(&i2c->vhost_dev, vdev);
> > > +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
> > > 
> > >      ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
> > >      if (ret < 0) {
> > > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > > index f9084cde58..201a39e220 100644
> > > --- a/hw/virtio/vhost-user-rng.c
> > > +++ b/hw/virtio/vhost-user-rng.c
> > > @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
> > >      }
> > > 
> > >      rng->vhost_dev.acked_features = vdev->guest_features;
> > > -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
> > > +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
> > >      if (ret < 0) {
> > >          error_report("Error starting vhost-user-rng: %d", -ret);
> > >          goto err_guest_notifiers;
> > > @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(&rng->vhost_dev, vdev);
> > > +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
> > > 
> > >      ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
> > >      if (ret < 0) {
> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > > index a67a275de2..d21c72b401 100644
> > > --- a/hw/virtio/vhost-vsock-common.c
> > > +++ b/hw/virtio/vhost-vsock-common.c
> > > @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
> > >      }
> > > 
> > >      vvc->vhost_dev.acked_features = vdev->guest_features;
> > > -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
> > > +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
> > >      if (ret < 0) {
> > >          error_report("Error starting vhost: %d", -ret);
> > >          goto err_guest_notifiers;
> > > @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
> > >          return;
> > >      }
> > > 
> > > -    vhost_dev_stop(&vvc->vhost_dev, vdev);
> > > +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
> > > 
> > >      ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
> > >      if (ret < 0) {
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index d1c4c20b8c..7fb008bc9e 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > >      return 0;
> > >  }
> > > 
> > > +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
> > 
> > There is a similarly-named vhost_set_vring_enable(NetClientState *nc,
> > int enable) function which is actually part of vhost_net. Please rename
> > it to vhost_net_set_vring_enable().
> 
> Should I rename it in this patch?
> 
> > It should probably call
> > vhost_dev_set_vring_enable().
> 
> Ehm, the idea of this patch was to touch as little as possible to avoid new
> regressions.
> 
> Also, the semantics of vhost_dev_set_vring_enable() was meant to keep
> vhost_dev_start()/vhost_dev_stop() simple, not to be exposed to frontends.
> (maybe I should have written it, sorry about that).
> 
> However I agree that we should clean up vhost-net and also the other
> frontends as Raphael also suggested, but honestly I'm scared to do that now
> in this patch...
> 
> What I would have wanted to do, would be similar to what we do for
> vhost-vdpa: call SET_VRING_ENABLE in the vhost_ops->vhost_dev_start()
> callback of vhost-user.c.
> Removing all the call to vhost_ops->vhost_set_vring_enable() in the
> frontends, but I think it's too risky to do that now.
> 
> > 
> > > +{
> > > +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
> > > +        return 0;
> > > +    }
> > 
> > These semantics are the opposite of vhost_user_set_vring_enable():
> > 
> >  if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >      return -EINVAL;
> >  }
> > 
> > Please make vhost_user_set_vring_enable() and
> > vhost_dev_set_vring_enable() consistent. Code gets really confusing when
> > layers have different semantics for the same operation.
> 
> It's the opposite precisely because we shouldn't let
> vhost_dev_start()/vhost_dev_stop() fail if
> vhost_ops->vhost_set_vring_enable() can't be called because it would fail.
> 
> If I do it this way, then I have to put the check inside
> vhost_dev_start()/vhost_dev_stop(), and at this point I remove the function
> that would be useless (just a wrapper of
> hdev->vhost_ops->vhost_set_vring_enable).
> Actually this was the first implementation I did, then I added the function
> just to have vhost_dev_start()/vhost_dev_stop() cleaner and to avoid
> duplicating the check.
> 
> > 
> > > +
> > > +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
> > > +}
> > 
> > The return value is hard to understand. An error return is only returned
> > by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are
> > other cases that seem like they should return an error but return
> > success instead. For example, when called with enable=false on a
> > non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy
> > vhost-user) we return success even though the vring wasn't disabled.
> 
> As I explained above, the idea was not to expose this function outside, but
> to use it only in vhost_dev_start()/vhost_dev_stop(). So the return value is
> 0 both when it has successes and when there is no need/way to enable/disable
> the vrings.
> 
> Perhaps since it is confusing, I will remove the function and put the code
> directly into vhost_dev_start()/vhost_dev_stop().
> 
> What do you think?

It's late now. We can merge it as-is.

I think this patch makes the vhost code even harder to understand and
it's important to do the clean ups that have been discussed for 8.0.
Will you work on the changes we discussed for 8.0?

Thanks,
Stefan
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefano Garzarella 1 year, 4 months ago 
On Wed, Nov 30, 2022 at 04:03:28PM -0500, Stefan Hajnoczi wrote:
>On Fri, Nov 25, 2022 at 09:12:43AM +0100, Stefano Garzarella wrote:
>> On Thu, Nov 24, 2022 at 01:36:29PM -0500, Stefan Hajnoczi wrote:
>> > On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote:
>> > > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> > > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
>> > > backend, but we forgot to enable vrings as specified in
>> > > docs/interop/vhost-user.rst:
>> > >
>> > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>> > >     ring starts directly in the enabled state.
>> > >
>> > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>> > >     initialized in a disabled state and is enabled by
>> > >     ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>> > >
>> > > Some vhost-user front-ends already did this by calling
>> > > vhost_ops.vhost_set_vring_enable() directly:
>> > > - backends/cryptodev-vhost.c
>> > > - hw/net/virtio-net.c
>> > > - hw/virtio/vhost-user-gpio.c
>> > >
>> > > But most didn't do that, so we would leave the vrings disabled and some
>> > > backends would not work. We observed this issue with the rust version of
>> > > virtiofsd [1], which uses the event loop [2] provided by the
>> > > vhost-user-backend crate where requests are not processed if vring is
>> > > not enabled.
>> > >
>> > > Let's fix this issue by enabling the vrings in vhost_dev_start() for
>> > > vhost-user front-ends that don't already do this directly. Same thing
>> > > also in vhost_dev_stop() where we disable vrings.
>> > >
>> > > [1] https://gitlab.com/virtio-fs/virtiofsd
>> > > [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
>> > >
>> > > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> > > Reported-by: German Maglione <gmaglione@redhat.com>
>> > > Tested-by: German Maglione <gmaglione@redhat.com>
>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > ---
>> > >  include/hw/virtio/vhost.h      |  6 +++--
>> > >  backends/cryptodev-vhost.c     |  4 ++--
>> > >  backends/vhost-user.c          |  4 ++--
>> > >  hw/block/vhost-user-blk.c      |  4 ++--
>> > >  hw/net/vhost_net.c             |  8 +++----
>> > >  hw/scsi/vhost-scsi-common.c    |  4 ++--
>> > >  hw/virtio/vhost-user-fs.c      |  4 ++--
>> > >  hw/virtio/vhost-user-gpio.c    |  4 ++--
>> > >  hw/virtio/vhost-user-i2c.c     |  4 ++--
>> > >  hw/virtio/vhost-user-rng.c     |  4 ++--
>> > >  hw/virtio/vhost-vsock-common.c |  4 ++--
>> > >  hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
>> > >  hw/virtio/trace-events         |  4 ++--
>> > >  13 files changed, 67 insertions(+), 31 deletions(-)
>> > >
>> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> > > index 353252ac3e..67a6807fac 100644
>> > > --- a/include/hw/virtio/vhost.h
>> > > +++ b/include/hw/virtio/vhost.h
>> > > @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
>> > >   * vhost_dev_start() - start the vhost device
>> > >   * @hdev: common vhost_dev structure
>> > >   * @vdev: the VirtIODevice structure
>> > > + * @vrings: true to have vrings enabled in this call
>> > >   *
>> > >   * Starts the vhost device. From this point VirtIO feature negotiation
>> > >   * can start and the device can start processing VirtIO transactions.
>> > >   *
>> > >   * Return: 0 on success, < 0 on error.
>> > >   */
>> > > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>> > > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>> > >
>> > >  /**
>> > >   * vhost_dev_stop() - stop the vhost device
>> > >   * @hdev: common vhost_dev structure
>> > >   * @vdev: the VirtIODevice structure
>> > > + * @vrings: true to have vrings disabled in this call
>> > >   *
>> > >   * Stop the vhost device. After the device is stopped the notifiers
>> > >   * can be disabled (@vhost_dev_disable_notifiers) and the device can
>> > >   * be torn down (@vhost_dev_cleanup).
>> > >   */
>> > > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>> > > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>> > >
>> > >  /**
>> > >   * DOC: vhost device configuration handling
>> > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
>> > > index bc13e466b4..572f87b3be 100644
>> > > --- a/backends/cryptodev-vhost.c
>> > > +++ b/backends/cryptodev-vhost.c
>> > > @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
>> > >          goto fail_notifiers;
>> > >      }
>> > >
>> > > -    r = vhost_dev_start(&crypto->dev, dev);
>> > > +    r = vhost_dev_start(&crypto->dev, dev, false);
>> > >      if (r < 0) {
>> > >          goto fail_start;
>> > >      }
>> > > @@ -111,7 +111,7 @@ static void
>> > >  cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
>> > >                                   VirtIODevice *dev)
>> > >  {
>> > > -    vhost_dev_stop(&crypto->dev, dev);
>> > > +    vhost_dev_stop(&crypto->dev, dev, false);
>> > >      vhost_dev_disable_notifiers(&crypto->dev, dev);
>> > >  }
>> > >
>> > > diff --git a/backends/vhost-user.c b/backends/vhost-user.c
>> > > index 5dedb2d987..7bfcaef976 100644
>> > > --- a/backends/vhost-user.c
>> > > +++ b/backends/vhost-user.c
>> > > @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
>> > >      }
>> > >
>> > >      b->dev.acked_features = b->vdev->guest_features;
>> > > -    ret = vhost_dev_start(&b->dev, b->vdev);
>> > > +    ret = vhost_dev_start(&b->dev, b->vdev, true);
>> > >      if (ret < 0) {
>> > >          error_report("Error start vhost dev");
>> > >          goto err_guest_notifiers;
>> > > @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&b->dev, b->vdev);
>> > > +    vhost_dev_stop(&b->dev, b->vdev, true);
>> > >
>> > >      if (k->set_guest_notifiers) {
>> > >          ret = k->set_guest_notifiers(qbus->parent,
>> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> > > index 0d5190accf..1177064631 100644
>> > > --- a/hw/block/vhost-user-blk.c
>> > > +++ b/hw/block/vhost-user-blk.c
>> > > @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>> > >      }
>> > >
>> > >      s->dev.vq_index_end = s->dev.nvqs;
>> > > -    ret = vhost_dev_start(&s->dev, vdev);
>> > > +    ret = vhost_dev_start(&s->dev, vdev, true);
>> > >      if (ret < 0) {
>> > >          error_setg_errno(errp, -ret, "Error starting vhost");
>> > >          goto err_guest_notifiers;
>> > > @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&s->dev, vdev);
>> > > +    vhost_dev_stop(&s->dev, vdev, true);
>> > >
>> > >      ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>> > >      if (ret < 0) {
>> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> > > index 26e4930676..043058ff43 100644
>> > > --- a/hw/net/vhost_net.c
>> > > +++ b/hw/net/vhost_net.c
>> > > @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>> > >          goto fail_notifiers;
>> > >      }
>> > >
>> > > -    r = vhost_dev_start(&net->dev, dev);
>> > > +    r = vhost_dev_start(&net->dev, dev, false);
>> > >      if (r < 0) {
>> > >          goto fail_start;
>> > >      }
>> > > @@ -308,7 +308,7 @@ fail:
>> > >      if (net->nc->info->poll) {
>> > >          net->nc->info->poll(net->nc, true);
>> > >      }
>> > > -    vhost_dev_stop(&net->dev, dev);
>> > > +    vhost_dev_stop(&net->dev, dev, false);
>> > >  fail_start:
>> > >      vhost_dev_disable_notifiers(&net->dev, dev);
>> > >  fail_notifiers:
>> > > @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
>> > >      if (net->nc->info->poll) {
>> > >          net->nc->info->poll(net->nc, true);
>> > >      }
>> > > -    vhost_dev_stop(&net->dev, dev);
>> > > +    vhost_dev_stop(&net->dev, dev, false);
>> > >      if (net->nc->info->stop) {
>> > >          net->nc->info->stop(net->nc);
>> > >      }
>> > > @@ -606,7 +606,7 @@ err_start:
>> > >          assert(r >= 0);
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&net->dev, vdev);
>> > > +    vhost_dev_stop(&net->dev, vdev, false);
>> > >
>> > >      return r;
>> > >  }
>> > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>> > > index 767f827e55..18ea5dcfa1 100644
>> > > --- a/hw/scsi/vhost-scsi-common.c
>> > > +++ b/hw/scsi/vhost-scsi-common.c
>> > > @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>> > >          goto err_guest_notifiers;
>> > >      }
>> > >
>> > > -    ret = vhost_dev_start(&vsc->dev, vdev);
>> > > +    ret = vhost_dev_start(&vsc->dev, vdev, true);
>> > >      if (ret < 0) {
>> > >          error_report("Error start vhost dev");
>> > >          goto err_guest_notifiers;
>> > > @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>> > >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> > >      int ret = 0;
>> > >
>> > > -    vhost_dev_stop(&vsc->dev, vdev);
>> > > +    vhost_dev_stop(&vsc->dev, vdev, true);
>> > >
>> > >      if (k->set_guest_notifiers) {
>> > >          ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>> > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> > > index dc4014cdef..d97b179e6f 100644
>> > > --- a/hw/virtio/vhost-user-fs.c
>> > > +++ b/hw/virtio/vhost-user-fs.c
>> > > @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
>> > >      }
>> > >
>> > >      fs->vhost_dev.acked_features = vdev->guest_features;
>> > > -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
>> > > +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
>> > >      if (ret < 0) {
>> > >          error_report("Error starting vhost: %d", -ret);
>> > >          goto err_guest_notifiers;
>> > > @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&fs->vhost_dev, vdev);
>> > > +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
>> > >
>> > >      ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
>> > >      if (ret < 0) {
>> > > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>> > > index 5851cb3bc9..0b40ebd15a 100644
>> > > --- a/hw/virtio/vhost-user-gpio.c
>> > > +++ b/hw/virtio/vhost-user-gpio.c
>> > > @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
>> > >       */
>> > >      vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
>> > >
>> > > -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
>> > > +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
>> > >      if (ret < 0) {
>> > >          error_report("Error starting vhost-user-gpio: %d", ret);
>> > >          goto err_guest_notifiers;
>> > > @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(vhost_dev, vdev);
>> > > +    vhost_dev_stop(vhost_dev, vdev, false);
>> > >
>> > >      ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
>> > >      if (ret < 0) {
>> > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>> > > index 1c9f3d20dc..dc5c828ba6 100644
>> > > --- a/hw/virtio/vhost-user-i2c.c
>> > > +++ b/hw/virtio/vhost-user-i2c.c
>> > > @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
>> > >
>> > >      i2c->vhost_dev.acked_features = vdev->guest_features;
>> > >
>> > > -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
>> > > +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
>> > >      if (ret < 0) {
>> > >          error_report("Error starting vhost-user-i2c: %d", -ret);
>> > >          goto err_guest_notifiers;
>> > > @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&i2c->vhost_dev, vdev);
>> > > +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
>> > >
>> > >      ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
>> > >      if (ret < 0) {
>> > > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>> > > index f9084cde58..201a39e220 100644
>> > > --- a/hw/virtio/vhost-user-rng.c
>> > > +++ b/hw/virtio/vhost-user-rng.c
>> > > @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
>> > >      }
>> > >
>> > >      rng->vhost_dev.acked_features = vdev->guest_features;
>> > > -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
>> > > +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
>> > >      if (ret < 0) {
>> > >          error_report("Error starting vhost-user-rng: %d", -ret);
>> > >          goto err_guest_notifiers;
>> > > @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&rng->vhost_dev, vdev);
>> > > +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
>> > >
>> > >      ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
>> > >      if (ret < 0) {
>> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>> > > index a67a275de2..d21c72b401 100644
>> > > --- a/hw/virtio/vhost-vsock-common.c
>> > > +++ b/hw/virtio/vhost-vsock-common.c
>> > > @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
>> > >      }
>> > >
>> > >      vvc->vhost_dev.acked_features = vdev->guest_features;
>> > > -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
>> > > +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
>> > >      if (ret < 0) {
>> > >          error_report("Error starting vhost: %d", -ret);
>> > >          goto err_guest_notifiers;
>> > > @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
>> > >          return;
>> > >      }
>> > >
>> > > -    vhost_dev_stop(&vvc->vhost_dev, vdev);
>> > > +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
>> > >
>> > >      ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
>> > >      if (ret < 0) {
>> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> > > index d1c4c20b8c..7fb008bc9e 100644
>> > > --- a/hw/virtio/vhost.c
>> > > +++ b/hw/virtio/vhost.c
>> > > @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
>> > >      return 0;
>> > >  }
>> > >
>> > > +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
>> >
>> > There is a similarly-named vhost_set_vring_enable(NetClientState *nc,
>> > int enable) function which is actually part of vhost_net. Please rename
>> > it to vhost_net_set_vring_enable().
>>
>> Should I rename it in this patch?
>>
>> > It should probably call
>> > vhost_dev_set_vring_enable().
>>
>> Ehm, the idea of this patch was to touch as little as possible to avoid new
>> regressions.
>>
>> Also, the semantics of vhost_dev_set_vring_enable() was meant to keep
>> vhost_dev_start()/vhost_dev_stop() simple, not to be exposed to frontends.
>> (maybe I should have written it, sorry about that).
>>
>> However I agree that we should clean up vhost-net and also the other
>> frontends as Raphael also suggested, but honestly I'm scared to do that now
>> in this patch...
>>
>> What I would have wanted to do, would be similar to what we do for
>> vhost-vdpa: call SET_VRING_ENABLE in the vhost_ops->vhost_dev_start()
>> callback of vhost-user.c.
>> Removing all the call to vhost_ops->vhost_set_vring_enable() in the
>> frontends, but I think it's too risky to do that now.
>>
>> >
>> > > +{
>> > > +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
>> > > +        return 0;
>> > > +    }
>> >
>> > These semantics are the opposite of vhost_user_set_vring_enable():
>> >
>> >  if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>> >      return -EINVAL;
>> >  }
>> >
>> > Please make vhost_user_set_vring_enable() and
>> > vhost_dev_set_vring_enable() consistent. Code gets really confusing when
>> > layers have different semantics for the same operation.
>>
>> It's the opposite precisely because we shouldn't let
>> vhost_dev_start()/vhost_dev_stop() fail if
>> vhost_ops->vhost_set_vring_enable() can't be called because it would fail.
>>
>> If I do it this way, then I have to put the check inside
>> vhost_dev_start()/vhost_dev_stop(), and at this point I remove the function
>> that would be useless (just a wrapper of
>> hdev->vhost_ops->vhost_set_vring_enable).
>> Actually this was the first implementation I did, then I added the function
>> just to have vhost_dev_start()/vhost_dev_stop() cleaner and to avoid
>> duplicating the check.
>>
>> >
>> > > +
>> > > +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
>> > > +}
>> >
>> > The return value is hard to understand. An error return is only returned
>> > by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are
>> > other cases that seem like they should return an error but return
>> > success instead. For example, when called with enable=false on a
>> > non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy
>> > vhost-user) we return success even though the vring wasn't disabled.
>>
>> As I explained above, the idea was not to expose this function outside, but
>> to use it only in vhost_dev_start()/vhost_dev_stop(). So the return value is
>> 0 both when it has successes and when there is no need/way to enable/disable
>> the vrings.
>>
>> Perhaps since it is confusing, I will remove the function and put the code
>> directly into vhost_dev_start()/vhost_dev_stop().
>>
>> What do you think?
>
>It's late now. We can merge it as-is.
>
>I think this patch makes the vhost code even harder to understand and
>it's important to do the clean ups that have been discussed for 8.0.

Agree.

>Will you work on the changes we discussed for 8.0?

Yep, sure.
I will try to unify all vhost/vhost-user devices.
Now I think it's also a mess because the devices do different things, we 
should have everything in the core.

Thanks,
Stefano
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Alex Bennée 1 year, 4 months ago 
Stefano Garzarella <sgarzare@redhat.com> writes:

> On Wed, Nov 30, 2022 at 04:03:28PM -0500, Stefan Hajnoczi wrote:
>>On Fri, Nov 25, 2022 at 09:12:43AM +0100, Stefano Garzarella wrote:
>>> On Thu, Nov 24, 2022 at 01:36:29PM -0500, Stefan Hajnoczi wrote:
>>> > On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote:
>>> > > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>>> > > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
>>> > > backend, but we forgot to enable vrings as specified in
>>> > > docs/interop/vhost-user.rst:
>>> > >
>>> > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>>> > >     ring starts directly in the enabled state.
>>> > >
>>> > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>>> > >     initialized in a disabled state and is enabled by
>>> > >     ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>>> > >
>>> > > Some vhost-user front-ends already did this by calling
>>> > > vhost_ops.vhost_set_vring_enable() directly:
>>> > > - backends/cryptodev-vhost.c
>>> > > - hw/net/virtio-net.c
>>> > > - hw/virtio/vhost-user-gpio.c
>>> > >
>>> > > But most didn't do that, so we would leave the vrings disabled and some
>>> > > backends would not work. We observed this issue with the rust version of
>>> > > virtiofsd [1], which uses the event loop [2] provided by the
>>> > > vhost-user-backend crate where requests are not processed if vring is
>>> > > not enabled.
>>> > >
>>> > > Let's fix this issue by enabling the vrings in vhost_dev_start() for
>>> > > vhost-user front-ends that don't already do this directly. Same thing
>>> > > also in vhost_dev_stop() where we disable vrings.
>>> > >
>>> > > [1] https://gitlab.com/virtio-fs/virtiofsd
>>> > > [2]
> https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
>>> > >
>>> > > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>>> > > Reported-by: German Maglione <gmaglione@redhat.com>
>>> > > Tested-by: German Maglione <gmaglione@redhat.com>
>>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> > > ---
>>> > >  include/hw/virtio/vhost.h      |  6 +++--
>>> > >  backends/cryptodev-vhost.c     |  4 ++--
>>> > >  backends/vhost-user.c          |  4 ++--
>>> > >  hw/block/vhost-user-blk.c      |  4 ++--
>>> > >  hw/net/vhost_net.c             |  8 +++----
>>> > >  hw/scsi/vhost-scsi-common.c    |  4 ++--
>>> > >  hw/virtio/vhost-user-fs.c      |  4 ++--
>>> > >  hw/virtio/vhost-user-gpio.c    |  4 ++--
>>> > >  hw/virtio/vhost-user-i2c.c     |  4 ++--
>>> > >  hw/virtio/vhost-user-rng.c     |  4 ++--
>>> > >  hw/virtio/vhost-vsock-common.c |  4 ++--
>>> > >  hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
>>> > >  hw/virtio/trace-events         |  4 ++--
>>> > >  13 files changed, 67 insertions(+), 31 deletions(-)
>>> > >
>>> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>>> > > index 353252ac3e..67a6807fac 100644
>>> > > --- a/include/hw/virtio/vhost.h
>>> > > +++ b/include/hw/virtio/vhost.h
>>> > > @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
>>> > >   * vhost_dev_start() - start the vhost device
>>> > >   * @hdev: common vhost_dev structure
>>> > >   * @vdev: the VirtIODevice structure
>>> > > + * @vrings: true to have vrings enabled in this call
>>> > >   *
>>> > >   * Starts the vhost device. From this point VirtIO feature negotiation
>>> > >   * can start and the device can start processing VirtIO transactions.
>>> > >   *
>>> > >   * Return: 0 on success, < 0 on error.
>>> > >   */
>>> > > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>>> > > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>>> > >
>>> > >  /**
>>> > >   * vhost_dev_stop() - stop the vhost device
>>> > >   * @hdev: common vhost_dev structure
>>> > >   * @vdev: the VirtIODevice structure
>>> > > + * @vrings: true to have vrings disabled in this call
>>> > >   *
>>> > >   * Stop the vhost device. After the device is stopped the notifiers
>>> > >   * can be disabled (@vhost_dev_disable_notifiers) and the device can
>>> > >   * be torn down (@vhost_dev_cleanup).
>>> > >   */
>>> > > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>>> > > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
>>> > >
>>> > >  /**
>>> > >   * DOC: vhost device configuration handling
>>> > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
>>> > > index bc13e466b4..572f87b3be 100644
>>> > > --- a/backends/cryptodev-vhost.c
>>> > > +++ b/backends/cryptodev-vhost.c
>>> > > @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
>>> > >          goto fail_notifiers;
>>> > >      }
>>> > >
>>> > > -    r = vhost_dev_start(&crypto->dev, dev);
>>> > > +    r = vhost_dev_start(&crypto->dev, dev, false);
>>> > >      if (r < 0) {
>>> > >          goto fail_start;
>>> > >      }
>>> > > @@ -111,7 +111,7 @@ static void
>>> > >  cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
>>> > >                                   VirtIODevice *dev)
>>> > >  {
>>> > > -    vhost_dev_stop(&crypto->dev, dev);
>>> > > +    vhost_dev_stop(&crypto->dev, dev, false);
>>> > >      vhost_dev_disable_notifiers(&crypto->dev, dev);
>>> > >  }
>>> > >
>>> > > diff --git a/backends/vhost-user.c b/backends/vhost-user.c
>>> > > index 5dedb2d987..7bfcaef976 100644
>>> > > --- a/backends/vhost-user.c
>>> > > +++ b/backends/vhost-user.c
>>> > > @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
>>> > >      }
>>> > >
>>> > >      b->dev.acked_features = b->vdev->guest_features;
>>> > > -    ret = vhost_dev_start(&b->dev, b->vdev);
>>> > > +    ret = vhost_dev_start(&b->dev, b->vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error start vhost dev");
>>> > >          goto err_guest_notifiers;
>>> > > @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&b->dev, b->vdev);
>>> > > +    vhost_dev_stop(&b->dev, b->vdev, true);
>>> > >
>>> > >      if (k->set_guest_notifiers) {
>>> > >          ret = k->set_guest_notifiers(qbus->parent,
>>> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> > > index 0d5190accf..1177064631 100644
>>> > > --- a/hw/block/vhost-user-blk.c
>>> > > +++ b/hw/block/vhost-user-blk.c
>>> > > @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>>> > >      }
>>> > >
>>> > >      s->dev.vq_index_end = s->dev.nvqs;
>>> > > -    ret = vhost_dev_start(&s->dev, vdev);
>>> > > +    ret = vhost_dev_start(&s->dev, vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_setg_errno(errp, -ret, "Error starting vhost");
>>> > >          goto err_guest_notifiers;
>>> > > @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&s->dev, vdev);
>>> > > +    vhost_dev_stop(&s->dev, vdev, true);
>>> > >
>>> > >      ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>>> > >      if (ret < 0) {
>>> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>> > > index 26e4930676..043058ff43 100644
>>> > > --- a/hw/net/vhost_net.c
>>> > > +++ b/hw/net/vhost_net.c
>>> > > @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>>> > >          goto fail_notifiers;
>>> > >      }
>>> > >
>>> > > -    r = vhost_dev_start(&net->dev, dev);
>>> > > +    r = vhost_dev_start(&net->dev, dev, false);
>>> > >      if (r < 0) {
>>> > >          goto fail_start;
>>> > >      }
>>> > > @@ -308,7 +308,7 @@ fail:
>>> > >      if (net->nc->info->poll) {
>>> > >          net->nc->info->poll(net->nc, true);
>>> > >      }
>>> > > -    vhost_dev_stop(&net->dev, dev);
>>> > > +    vhost_dev_stop(&net->dev, dev, false);
>>> > >  fail_start:
>>> > >      vhost_dev_disable_notifiers(&net->dev, dev);
>>> > >  fail_notifiers:
>>> > > @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
>>> > >      if (net->nc->info->poll) {
>>> > >          net->nc->info->poll(net->nc, true);
>>> > >      }
>>> > > -    vhost_dev_stop(&net->dev, dev);
>>> > > +    vhost_dev_stop(&net->dev, dev, false);
>>> > >      if (net->nc->info->stop) {
>>> > >          net->nc->info->stop(net->nc);
>>> > >      }
>>> > > @@ -606,7 +606,7 @@ err_start:
>>> > >          assert(r >= 0);
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&net->dev, vdev);
>>> > > +    vhost_dev_stop(&net->dev, vdev, false);
>>> > >
>>> > >      return r;
>>> > >  }
>>> > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>>> > > index 767f827e55..18ea5dcfa1 100644
>>> > > --- a/hw/scsi/vhost-scsi-common.c
>>> > > +++ b/hw/scsi/vhost-scsi-common.c
>>> > > @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>> > >          goto err_guest_notifiers;
>>> > >      }
>>> > >
>>> > > -    ret = vhost_dev_start(&vsc->dev, vdev);
>>> > > +    ret = vhost_dev_start(&vsc->dev, vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error start vhost dev");
>>> > >          goto err_guest_notifiers;
>>> > > @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>>> > >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>>> > >      int ret = 0;
>>> > >
>>> > > -    vhost_dev_stop(&vsc->dev, vdev);
>>> > > +    vhost_dev_stop(&vsc->dev, vdev, true);
>>> > >
>>> > >      if (k->set_guest_notifiers) {
>>> > >          ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>>> > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>>> > > index dc4014cdef..d97b179e6f 100644
>>> > > --- a/hw/virtio/vhost-user-fs.c
>>> > > +++ b/hw/virtio/vhost-user-fs.c
>>> > > @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
>>> > >      }
>>> > >
>>> > >      fs->vhost_dev.acked_features = vdev->guest_features;
>>> > > -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
>>> > > +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error starting vhost: %d", -ret);
>>> > >          goto err_guest_notifiers;
>>> > > @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&fs->vhost_dev, vdev);
>>> > > +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
>>> > >
>>> > >      ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
>>> > >      if (ret < 0) {
>>> > > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>>> > > index 5851cb3bc9..0b40ebd15a 100644
>>> > > --- a/hw/virtio/vhost-user-gpio.c
>>> > > +++ b/hw/virtio/vhost-user-gpio.c
>>> > > @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
>>> > >       */
>>> > >      vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
>>> > >
>>> > > -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
>>> > > +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error starting vhost-user-gpio: %d", ret);
>>> > >          goto err_guest_notifiers;
>>> > > @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(vhost_dev, vdev);
>>> > > +    vhost_dev_stop(vhost_dev, vdev, false);
>>> > >
>>> > >      ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
>>> > >      if (ret < 0) {
>>> > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>>> > > index 1c9f3d20dc..dc5c828ba6 100644
>>> > > --- a/hw/virtio/vhost-user-i2c.c
>>> > > +++ b/hw/virtio/vhost-user-i2c.c
>>> > > @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
>>> > >
>>> > >      i2c->vhost_dev.acked_features = vdev->guest_features;
>>> > >
>>> > > -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
>>> > > +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error starting vhost-user-i2c: %d", -ret);
>>> > >          goto err_guest_notifiers;
>>> > > @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&i2c->vhost_dev, vdev);
>>> > > +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
>>> > >
>>> > >      ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
>>> > >      if (ret < 0) {
>>> > > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>>> > > index f9084cde58..201a39e220 100644
>>> > > --- a/hw/virtio/vhost-user-rng.c
>>> > > +++ b/hw/virtio/vhost-user-rng.c
>>> > > @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
>>> > >      }
>>> > >
>>> > >      rng->vhost_dev.acked_features = vdev->guest_features;
>>> > > -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
>>> > > +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error starting vhost-user-rng: %d", -ret);
>>> > >          goto err_guest_notifiers;
>>> > > @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&rng->vhost_dev, vdev);
>>> > > +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
>>> > >
>>> > >      ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
>>> > >      if (ret < 0) {
>>> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>>> > > index a67a275de2..d21c72b401 100644
>>> > > --- a/hw/virtio/vhost-vsock-common.c
>>> > > +++ b/hw/virtio/vhost-vsock-common.c
>>> > > @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
>>> > >      }
>>> > >
>>> > >      vvc->vhost_dev.acked_features = vdev->guest_features;
>>> > > -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
>>> > > +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
>>> > >      if (ret < 0) {
>>> > >          error_report("Error starting vhost: %d", -ret);
>>> > >          goto err_guest_notifiers;
>>> > > @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
>>> > >          return;
>>> > >      }
>>> > >
>>> > > -    vhost_dev_stop(&vvc->vhost_dev, vdev);
>>> > > +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
>>> > >
>>> > >      ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
>>> > >      if (ret < 0) {
>>> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> > > index d1c4c20b8c..7fb008bc9e 100644
>>> > > --- a/hw/virtio/vhost.c
>>> > > +++ b/hw/virtio/vhost.c
>>> > > @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
>>> > >      return 0;
>>> > >  }
>>> > >
>>> > > +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
>>> >
>>> > There is a similarly-named vhost_set_vring_enable(NetClientState *nc,
>>> > int enable) function which is actually part of vhost_net. Please rename
>>> > it to vhost_net_set_vring_enable().
>>>
>>> Should I rename it in this patch?
>>>
>>> > It should probably call
>>> > vhost_dev_set_vring_enable().
>>>
>>> Ehm, the idea of this patch was to touch as little as possible to avoid new
>>> regressions.
>>>
>>> Also, the semantics of vhost_dev_set_vring_enable() was meant to keep
>>> vhost_dev_start()/vhost_dev_stop() simple, not to be exposed to frontends.
>>> (maybe I should have written it, sorry about that).
>>>
>>> However I agree that we should clean up vhost-net and also the other
>>> frontends as Raphael also suggested, but honestly I'm scared to do that now
>>> in this patch...
>>>
>>> What I would have wanted to do, would be similar to what we do for
>>> vhost-vdpa: call SET_VRING_ENABLE in the vhost_ops->vhost_dev_start()
>>> callback of vhost-user.c.
>>> Removing all the call to vhost_ops->vhost_set_vring_enable() in the
>>> frontends, but I think it's too risky to do that now.
>>>
>>> >
>>> > > +{
>>> > > +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
>>> > > +        return 0;
>>> > > +    }
>>> >
>>> > These semantics are the opposite of vhost_user_set_vring_enable():
>>> >
>>> >  if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>>> >      return -EINVAL;
>>> >  }
>>> >
>>> > Please make vhost_user_set_vring_enable() and
>>> > vhost_dev_set_vring_enable() consistent. Code gets really confusing when
>>> > layers have different semantics for the same operation.
>>>
>>> It's the opposite precisely because we shouldn't let
>>> vhost_dev_start()/vhost_dev_stop() fail if
>>> vhost_ops->vhost_set_vring_enable() can't be called because it would fail.
>>>
>>> If I do it this way, then I have to put the check inside
>>> vhost_dev_start()/vhost_dev_stop(), and at this point I remove the function
>>> that would be useless (just a wrapper of
>>> hdev->vhost_ops->vhost_set_vring_enable).
>>> Actually this was the first implementation I did, then I added the function
>>> just to have vhost_dev_start()/vhost_dev_stop() cleaner and to avoid
>>> duplicating the check.
>>>
>>> >
>>> > > +
>>> > > +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
>>> > > +}
>>> >
>>> > The return value is hard to understand. An error return is only returned
>>> > by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are
>>> > other cases that seem like they should return an error but return
>>> > success instead. For example, when called with enable=false on a
>>> > non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy
>>> > vhost-user) we return success even though the vring wasn't disabled.
>>>
>>> As I explained above, the idea was not to expose this function outside, but
>>> to use it only in vhost_dev_start()/vhost_dev_stop(). So the return value is
>>> 0 both when it has successes and when there is no need/way to enable/disable
>>> the vrings.
>>>
>>> Perhaps since it is confusing, I will remove the function and put the code
>>> directly into vhost_dev_start()/vhost_dev_stop().
>>>
>>> What do you think?
>>
>>It's late now. We can merge it as-is.
>>
>>I think this patch makes the vhost code even harder to understand and
>>it's important to do the clean ups that have been discussed for 8.0.
>
> Agree.
>
>>Will you work on the changes we discussed for 8.0?
>
> Yep, sure.
> I will try to unify all vhost/vhost-user devices.
> Now I think it's also a mess because the devices do different things,
> we should have everything in the core.

Do you think rust-vmm's vhost crates have enough of the state
management to manage vhost and vhost-user backends? Maybe it would be a
good experiment in replacing a (small well defined) piece of
functionality with rust?

That said there is a lot of deep magic in the vhost-net stuff which I
think is down to the interaction with things like vdpk and other network
optimisations that might be missing. For the rest of the devices most of
the code is basically boiler plate which has grown variations due to
code motion and change. This is the sort of thing that generics solves
well.

>
> Thanks,
> Stefano


-- 
Alex Bennée
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Michael S. Tsirkin 1 year, 4 months ago 
On Thu, Dec 01, 2022 at 10:14:39AM +0000, Alex Bennée wrote:
> Do you think rust-vmm's vhost crates have enough of the state
> management to manage vhost and vhost-user backends? Maybe it would be a
> good experiment in replacing a (small well defined) piece of
> functionality with rust?
> 
> That said there is a lot of deep magic in the vhost-net stuff which I
> think is down to the interaction with things like vdpk and other network
> optimisations that might be missing. For the rest of the devices most of
> the code is basically boiler plate which has grown variations due to
> code motion and change. This is the sort of thing that generics solves
> well.

Not sure what you want to replace with what though, libvhost-user or
vhost-user bits in qemu?

-- 
MST
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Alex Bennée 1 year, 4 months ago 
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Dec 01, 2022 at 10:14:39AM +0000, Alex Bennée wrote:
>> Do you think rust-vmm's vhost crates have enough of the state
>> management to manage vhost and vhost-user backends? Maybe it would be a
>> good experiment in replacing a (small well defined) piece of
>> functionality with rust?
>> 
>> That said there is a lot of deep magic in the vhost-net stuff which I
>> think is down to the interaction with things like vdpk and other network
>> optimisations that might be missing. For the rest of the devices most of
>> the code is basically boiler plate which has grown variations due to
>> code motion and change. This is the sort of thing that generics solves
>> well.
>
> Not sure what you want to replace with what though, libvhost-user or
> vhost-user bits in qemu?

The vhost-user bits in the main QEMU binary. We already don't use
libvhost-user for most of our backends anyway ;-)

-- 
Alex Bennée
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Michael S. Tsirkin 1 year, 4 months ago 
On Thu, Dec 01, 2022 at 12:21:21PM +0000, Alex Bennée wrote:
> 
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Dec 01, 2022 at 10:14:39AM +0000, Alex Bennée wrote:
> >> Do you think rust-vmm's vhost crates have enough of the state
> >> management to manage vhost and vhost-user backends? Maybe it would be a
> >> good experiment in replacing a (small well defined) piece of
> >> functionality with rust?
> >> 
> >> That said there is a lot of deep magic in the vhost-net stuff which I
> >> think is down to the interaction with things like vdpk and other network
> >> optimisations that might be missing. For the rest of the devices most of
> >> the code is basically boiler plate which has grown variations due to
> >> code motion and change. This is the sort of thing that generics solves
> >> well.
> >
> > Not sure what you want to replace with what though, libvhost-user or
> > vhost-user bits in qemu?
> 
> The vhost-user bits in the main QEMU binary. We already don't use
> libvhost-user for most of our backends anyway ;-)

Mixing C and Rust like this is far from trivial. I'd start with
something much less ambitious that virtio.

-- 
MST
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefano Garzarella 1 year, 4 months ago 
On Thu, Dec 01, 2022 at 04:49:37PM -0500, Michael S. Tsirkin wrote:
>On Thu, Dec 01, 2022 at 12:21:21PM +0000, Alex Bennée wrote:
>>
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>> > On Thu, Dec 01, 2022 at 10:14:39AM +0000, Alex Bennée wrote:
>> >> Do you think rust-vmm's vhost crates have enough of the state
>> >> management to manage vhost and vhost-user backends? Maybe it would be a
>> >> good experiment in replacing a (small well defined) piece of
>> >> functionality with rust?

I honestly don't know, but I think they are not 100% complete.
I do agree that an experiment would be nice though, maybe a GSoC?
Or maybe not, since it's not very clear whether we get to the end or not....

>> >>
>> >> That said there is a lot of deep magic in the vhost-net stuff which I
>> >> think is down to the interaction with things like vdpk and other network
>> >> optimisations that might be missing. For the rest of the devices most of
>> >> the code is basically boiler plate which has grown variations due to
>> >> code motion and change. This is the sort of thing that generics solves
>> >> well.
>> >
>> > Not sure what you want to replace with what though, libvhost-user or
>> > vhost-user bits in qemu?
>>
>> The vhost-user bits in the main QEMU binary. We already don't use
>> libvhost-user for most of our backends anyway ;-)
>
>Mixing C and Rust like this is far from trivial. I'd start with
>something much less ambitious that virtio.

We recently merged libblkio [1] support in QEMU [2], where we did
something similar.
libblkio provides a C API, but the library is totally written in Rust.
Writing the C interface was not too complicated.

The advantage here is that the Rust code is already part of the rust-vmm
crates.
I think it needs adjustments, but it's not a bad idea in my opinion.

Anyway, I don't think it's an easy job, though, so maybe GSoC could just
be about writing the C interface and adapting Rust crates. Touching QEMU
might require too much knowledge of the current code.

Thanks,
Stefano

[1] https://libblkio.gitlab.io/libblkio/
[2] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg02215.html
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Michael S. Tsirkin 1 year, 4 months ago 
On Wed, Nov 30, 2022 at 04:03:28PM -0500, Stefan Hajnoczi wrote:
> On Fri, Nov 25, 2022 at 09:12:43AM +0100, Stefano Garzarella wrote:
> > On Thu, Nov 24, 2022 at 01:36:29PM -0500, Stefan Hajnoczi wrote:
> > > On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote:
> > > > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> > > > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
> > > > backend, but we forgot to enable vrings as specified in
> > > > docs/interop/vhost-user.rst:
> > > > 
> > > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
> > > >     ring starts directly in the enabled state.
> > > > 
> > > >     If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
> > > >     initialized in a disabled state and is enabled by
> > > >     ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
> > > > 
> > > > Some vhost-user front-ends already did this by calling
> > > > vhost_ops.vhost_set_vring_enable() directly:
> > > > - backends/cryptodev-vhost.c
> > > > - hw/net/virtio-net.c
> > > > - hw/virtio/vhost-user-gpio.c
> > > > 
> > > > But most didn't do that, so we would leave the vrings disabled and some
> > > > backends would not work. We observed this issue with the rust version of
> > > > virtiofsd [1], which uses the event loop [2] provided by the
> > > > vhost-user-backend crate where requests are not processed if vring is
> > > > not enabled.
> > > > 
> > > > Let's fix this issue by enabling the vrings in vhost_dev_start() for
> > > > vhost-user front-ends that don't already do this directly. Same thing
> > > > also in vhost_dev_stop() where we disable vrings.
> > > > 
> > > > [1] https://gitlab.com/virtio-fs/virtiofsd
> > > > [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
> > > > 
> > > > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> > > > Reported-by: German Maglione <gmaglione@redhat.com>
> > > > Tested-by: German Maglione <gmaglione@redhat.com>
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > >  include/hw/virtio/vhost.h      |  6 +++--
> > > >  backends/cryptodev-vhost.c     |  4 ++--
> > > >  backends/vhost-user.c          |  4 ++--
> > > >  hw/block/vhost-user-blk.c      |  4 ++--
> > > >  hw/net/vhost_net.c             |  8 +++----
> > > >  hw/scsi/vhost-scsi-common.c    |  4 ++--
> > > >  hw/virtio/vhost-user-fs.c      |  4 ++--
> > > >  hw/virtio/vhost-user-gpio.c    |  4 ++--
> > > >  hw/virtio/vhost-user-i2c.c     |  4 ++--
> > > >  hw/virtio/vhost-user-rng.c     |  4 ++--
> > > >  hw/virtio/vhost-vsock-common.c |  4 ++--
> > > >  hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
> > > >  hw/virtio/trace-events         |  4 ++--
> > > >  13 files changed, 67 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > > index 353252ac3e..67a6807fac 100644
> > > > --- a/include/hw/virtio/vhost.h
> > > > +++ b/include/hw/virtio/vhost.h
> > > > @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> > > >   * vhost_dev_start() - start the vhost device
> > > >   * @hdev: common vhost_dev structure
> > > >   * @vdev: the VirtIODevice structure
> > > > + * @vrings: true to have vrings enabled in this call
> > > >   *
> > > >   * Starts the vhost device. From this point VirtIO feature negotiation
> > > >   * can start and the device can start processing VirtIO transactions.
> > > >   *
> > > >   * Return: 0 on success, < 0 on error.
> > > >   */
> > > > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> > > > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> > > > 
> > > >  /**
> > > >   * vhost_dev_stop() - stop the vhost device
> > > >   * @hdev: common vhost_dev structure
> > > >   * @vdev: the VirtIODevice structure
> > > > + * @vrings: true to have vrings disabled in this call
> > > >   *
> > > >   * Stop the vhost device. After the device is stopped the notifiers
> > > >   * can be disabled (@vhost_dev_disable_notifiers) and the device can
> > > >   * be torn down (@vhost_dev_cleanup).
> > > >   */
> > > > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> > > > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> > > > 
> > > >  /**
> > > >   * DOC: vhost device configuration handling
> > > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> > > > index bc13e466b4..572f87b3be 100644
> > > > --- a/backends/cryptodev-vhost.c
> > > > +++ b/backends/cryptodev-vhost.c
> > > > @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
> > > >          goto fail_notifiers;
> > > >      }
> > > > 
> > > > -    r = vhost_dev_start(&crypto->dev, dev);
> > > > +    r = vhost_dev_start(&crypto->dev, dev, false);
> > > >      if (r < 0) {
> > > >          goto fail_start;
> > > >      }
> > > > @@ -111,7 +111,7 @@ static void
> > > >  cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
> > > >                                   VirtIODevice *dev)
> > > >  {
> > > > -    vhost_dev_stop(&crypto->dev, dev);
> > > > +    vhost_dev_stop(&crypto->dev, dev, false);
> > > >      vhost_dev_disable_notifiers(&crypto->dev, dev);
> > > >  }
> > > > 
> > > > diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> > > > index 5dedb2d987..7bfcaef976 100644
> > > > --- a/backends/vhost-user.c
> > > > +++ b/backends/vhost-user.c
> > > > @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
> > > >      }
> > > > 
> > > >      b->dev.acked_features = b->vdev->guest_features;
> > > > -    ret = vhost_dev_start(&b->dev, b->vdev);
> > > > +    ret = vhost_dev_start(&b->dev, b->vdev, true);
> > > >      if (ret < 0) {
> > > >          error_report("Error start vhost dev");
> > > >          goto err_guest_notifiers;
> > > > @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&b->dev, b->vdev);
> > > > +    vhost_dev_stop(&b->dev, b->vdev, true);
> > > > 
> > > >      if (k->set_guest_notifiers) {
> > > >          ret = k->set_guest_notifiers(qbus->parent,
> > > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > > > index 0d5190accf..1177064631 100644
> > > > --- a/hw/block/vhost-user-blk.c
> > > > +++ b/hw/block/vhost-user-blk.c
> > > > @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
> > > >      }
> > > > 
> > > >      s->dev.vq_index_end = s->dev.nvqs;
> > > > -    ret = vhost_dev_start(&s->dev, vdev);
> > > > +    ret = vhost_dev_start(&s->dev, vdev, true);
> > > >      if (ret < 0) {
> > > >          error_setg_errno(errp, -ret, "Error starting vhost");
> > > >          goto err_guest_notifiers;
> > > > @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&s->dev, vdev);
> > > > +    vhost_dev_stop(&s->dev, vdev, true);
> > > > 
> > > >      ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> > > >      if (ret < 0) {
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index 26e4930676..043058ff43 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
> > > >          goto fail_notifiers;
> > > >      }
> > > > 
> > > > -    r = vhost_dev_start(&net->dev, dev);
> > > > +    r = vhost_dev_start(&net->dev, dev, false);
> > > >      if (r < 0) {
> > > >          goto fail_start;
> > > >      }
> > > > @@ -308,7 +308,7 @@ fail:
> > > >      if (net->nc->info->poll) {
> > > >          net->nc->info->poll(net->nc, true);
> > > >      }
> > > > -    vhost_dev_stop(&net->dev, dev);
> > > > +    vhost_dev_stop(&net->dev, dev, false);
> > > >  fail_start:
> > > >      vhost_dev_disable_notifiers(&net->dev, dev);
> > > >  fail_notifiers:
> > > > @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> > > >      if (net->nc->info->poll) {
> > > >          net->nc->info->poll(net->nc, true);
> > > >      }
> > > > -    vhost_dev_stop(&net->dev, dev);
> > > > +    vhost_dev_stop(&net->dev, dev, false);
> > > >      if (net->nc->info->stop) {
> > > >          net->nc->info->stop(net->nc);
> > > >      }
> > > > @@ -606,7 +606,7 @@ err_start:
> > > >          assert(r >= 0);
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&net->dev, vdev);
> > > > +    vhost_dev_stop(&net->dev, vdev, false);
> > > > 
> > > >      return r;
> > > >  }
> > > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > > > index 767f827e55..18ea5dcfa1 100644
> > > > --- a/hw/scsi/vhost-scsi-common.c
> > > > +++ b/hw/scsi/vhost-scsi-common.c
> > > > @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > >          goto err_guest_notifiers;
> > > >      }
> > > > 
> > > > -    ret = vhost_dev_start(&vsc->dev, vdev);
> > > > +    ret = vhost_dev_start(&vsc->dev, vdev, true);
> > > >      if (ret < 0) {
> > > >          error_report("Error start vhost dev");
> > > >          goto err_guest_notifiers;
> > > > @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > > >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > > >      int ret = 0;
> > > > 
> > > > -    vhost_dev_stop(&vsc->dev, vdev);
> > > > +    vhost_dev_stop(&vsc->dev, vdev, true);
> > > > 
> > > >      if (k->set_guest_notifiers) {
> > > >          ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > > index dc4014cdef..d97b179e6f 100644
> > > > --- a/hw/virtio/vhost-user-fs.c
> > > > +++ b/hw/virtio/vhost-user-fs.c
> > > > @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
> > > >      }
> > > > 
> > > >      fs->vhost_dev.acked_features = vdev->guest_features;
> > > > -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
> > > > +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
> > > >      if (ret < 0) {
> > > >          error_report("Error starting vhost: %d", -ret);
> > > >          goto err_guest_notifiers;
> > > > @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&fs->vhost_dev, vdev);
> > > > +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
> > > > 
> > > >      ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
> > > >      if (ret < 0) {
> > > > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> > > > index 5851cb3bc9..0b40ebd15a 100644
> > > > --- a/hw/virtio/vhost-user-gpio.c
> > > > +++ b/hw/virtio/vhost-user-gpio.c
> > > > @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
> > > >       */
> > > >      vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
> > > > 
> > > > -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
> > > > +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
> > > >      if (ret < 0) {
> > > >          error_report("Error starting vhost-user-gpio: %d", ret);
> > > >          goto err_guest_notifiers;
> > > > @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(vhost_dev, vdev);
> > > > +    vhost_dev_stop(vhost_dev, vdev, false);
> > > > 
> > > >      ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
> > > >      if (ret < 0) {
> > > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > > > index 1c9f3d20dc..dc5c828ba6 100644
> > > > --- a/hw/virtio/vhost-user-i2c.c
> > > > +++ b/hw/virtio/vhost-user-i2c.c
> > > > @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
> > > > 
> > > >      i2c->vhost_dev.acked_features = vdev->guest_features;
> > > > 
> > > > -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
> > > > +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
> > > >      if (ret < 0) {
> > > >          error_report("Error starting vhost-user-i2c: %d", -ret);
> > > >          goto err_guest_notifiers;
> > > > @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&i2c->vhost_dev, vdev);
> > > > +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
> > > > 
> > > >      ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
> > > >      if (ret < 0) {
> > > > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > > > index f9084cde58..201a39e220 100644
> > > > --- a/hw/virtio/vhost-user-rng.c
> > > > +++ b/hw/virtio/vhost-user-rng.c
> > > > @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
> > > >      }
> > > > 
> > > >      rng->vhost_dev.acked_features = vdev->guest_features;
> > > > -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
> > > > +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
> > > >      if (ret < 0) {
> > > >          error_report("Error starting vhost-user-rng: %d", -ret);
> > > >          goto err_guest_notifiers;
> > > > @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&rng->vhost_dev, vdev);
> > > > +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
> > > > 
> > > >      ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
> > > >      if (ret < 0) {
> > > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > > > index a67a275de2..d21c72b401 100644
> > > > --- a/hw/virtio/vhost-vsock-common.c
> > > > +++ b/hw/virtio/vhost-vsock-common.c
> > > > @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
> > > >      }
> > > > 
> > > >      vvc->vhost_dev.acked_features = vdev->guest_features;
> > > > -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
> > > > +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
> > > >      if (ret < 0) {
> > > >          error_report("Error starting vhost: %d", -ret);
> > > >          goto err_guest_notifiers;
> > > > @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
> > > >          return;
> > > >      }
> > > > 
> > > > -    vhost_dev_stop(&vvc->vhost_dev, vdev);
> > > > +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
> > > > 
> > > >      ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
> > > >      if (ret < 0) {
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > index d1c4c20b8c..7fb008bc9e 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > > >      return 0;
> > > >  }
> > > > 
> > > > +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
> > > 
> > > There is a similarly-named vhost_set_vring_enable(NetClientState *nc,
> > > int enable) function which is actually part of vhost_net. Please rename
> > > it to vhost_net_set_vring_enable().
> > 
> > Should I rename it in this patch?
> > 
> > > It should probably call
> > > vhost_dev_set_vring_enable().
> > 
> > Ehm, the idea of this patch was to touch as little as possible to avoid new
> > regressions.
> > 
> > Also, the semantics of vhost_dev_set_vring_enable() was meant to keep
> > vhost_dev_start()/vhost_dev_stop() simple, not to be exposed to frontends.
> > (maybe I should have written it, sorry about that).
> > 
> > However I agree that we should clean up vhost-net and also the other
> > frontends as Raphael also suggested, but honestly I'm scared to do that now
> > in this patch...
> > 
> > What I would have wanted to do, would be similar to what we do for
> > vhost-vdpa: call SET_VRING_ENABLE in the vhost_ops->vhost_dev_start()
> > callback of vhost-user.c.
> > Removing all the call to vhost_ops->vhost_set_vring_enable() in the
> > frontends, but I think it's too risky to do that now.
> > 
> > > 
> > > > +{
> > > > +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
> > > > +        return 0;
> > > > +    }
> > > 
> > > These semantics are the opposite of vhost_user_set_vring_enable():
> > > 
> > >  if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> > >      return -EINVAL;
> > >  }
> > > 
> > > Please make vhost_user_set_vring_enable() and
> > > vhost_dev_set_vring_enable() consistent. Code gets really confusing when
> > > layers have different semantics for the same operation.
> > 
> > It's the opposite precisely because we shouldn't let
> > vhost_dev_start()/vhost_dev_stop() fail if
> > vhost_ops->vhost_set_vring_enable() can't be called because it would fail.
> > 
> > If I do it this way, then I have to put the check inside
> > vhost_dev_start()/vhost_dev_stop(), and at this point I remove the function
> > that would be useless (just a wrapper of
> > hdev->vhost_ops->vhost_set_vring_enable).
> > Actually this was the first implementation I did, then I added the function
> > just to have vhost_dev_start()/vhost_dev_stop() cleaner and to avoid
> > duplicating the check.
> > 
> > > 
> > > > +
> > > > +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
> > > > +}
> > > 
> > > The return value is hard to understand. An error return is only returned
> > > by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are
> > > other cases that seem like they should return an error but return
> > > success instead. For example, when called with enable=false on a
> > > non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy
> > > vhost-user) we return success even though the vring wasn't disabled.
> > 
> > As I explained above, the idea was not to expose this function outside, but
> > to use it only in vhost_dev_start()/vhost_dev_stop(). So the return value is
> > 0 both when it has successes and when there is no need/way to enable/disable
> > the vrings.
> > 
> > Perhaps since it is confusing, I will remove the function and put the code
> > directly into vhost_dev_start()/vhost_dev_stop().
> > 
> > What do you think?
> 
> It's late now. We can merge it as-is.
> 
> I think this patch makes the vhost code even harder to understand and
> it's important to do the clean ups that have been discussed for 8.0.

Yes I agree. Only acked because we don't have better ideas on how to
fix vmstate issues without breaking CI.

> Will you work on the changes we discussed for 8.0?
> 
> Thanks,
> Stefan

-- 
MST
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Raphael Norwitz 1 year, 5 months ago 
> On Nov 23, 2022, at 8:16 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
> backend, but we forgot to enable vrings as specified in
> docs/interop/vhost-user.rst:
> 
>    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>    ring starts directly in the enabled state.
> 
>    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>    initialized in a disabled state and is enabled by
>    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
> 
> Some vhost-user front-ends already did this by calling
> vhost_ops.vhost_set_vring_enable() directly:
> - backends/cryptodev-vhost.c
> - hw/net/virtio-net.c
> - hw/virtio/vhost-user-gpio.c

To simplify why not rather change these devices to use the new semantics?

> 
> But most didn't do that, so we would leave the vrings disabled and some
> backends would not work. We observed this issue with the rust version of
> virtiofsd [1], which uses the event loop [2] provided by the
> vhost-user-backend crate where requests are not processed if vring is
> not enabled.
> 
> Let's fix this issue by enabling the vrings in vhost_dev_start() for
> vhost-user front-ends that don't already do this directly. Same thing
> also in vhost_dev_stop() where we disable vrings.
> 
> [1] https://gitlab.com/virtio-fs/virtiofsd
> [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
> Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> Reported-by: German Maglione <gmaglione@redhat.com>
> Tested-by: German Maglione <gmaglione@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Looks good for vhost-user-blk/vhost-user-scsi.

Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
> include/hw/virtio/vhost.h      |  6 +++--
> backends/cryptodev-vhost.c     |  4 ++--
> backends/vhost-user.c          |  4 ++--
> hw/block/vhost-user-blk.c      |  4 ++--
> hw/net/vhost_net.c             |  8 +++----
> hw/scsi/vhost-scsi-common.c    |  4 ++--
> hw/virtio/vhost-user-fs.c      |  4 ++--
> hw/virtio/vhost-user-gpio.c    |  4 ++--
> hw/virtio/vhost-user-i2c.c     |  4 ++--
> hw/virtio/vhost-user-rng.c     |  4 ++--
> hw/virtio/vhost-vsock-common.c |  4 ++--
> hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
> hw/virtio/trace-events         |  4 ++--
> 13 files changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 353252ac3e..67a6807fac 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
>  * vhost_dev_start() - start the vhost device
>  * @hdev: common vhost_dev structure
>  * @vdev: the VirtIODevice structure
> + * @vrings: true to have vrings enabled in this call
>  *
>  * Starts the vhost device. From this point VirtIO feature negotiation
>  * can start and the device can start processing VirtIO transactions.
>  *
>  * Return: 0 on success, < 0 on error.
>  */
> -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> 
> /**
>  * vhost_dev_stop() - stop the vhost device
>  * @hdev: common vhost_dev structure
>  * @vdev: the VirtIODevice structure
> + * @vrings: true to have vrings disabled in this call
>  *
>  * Stop the vhost device. After the device is stopped the notifiers
>  * can be disabled (@vhost_dev_disable_notifiers) and the device can
>  * be torn down (@vhost_dev_cleanup).
>  */
> -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> 
> /**
>  * DOC: vhost device configuration handling
> diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> index bc13e466b4..572f87b3be 100644
> --- a/backends/cryptodev-vhost.c
> +++ b/backends/cryptodev-vhost.c
> @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
>         goto fail_notifiers;
>     }
> 
> -    r = vhost_dev_start(&crypto->dev, dev);
> +    r = vhost_dev_start(&crypto->dev, dev, false);
>     if (r < 0) {
>         goto fail_start;
>     }
> @@ -111,7 +111,7 @@ static void
> cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
>                                  VirtIODevice *dev)
> {
> -    vhost_dev_stop(&crypto->dev, dev);
> +    vhost_dev_stop(&crypto->dev, dev, false);
>     vhost_dev_disable_notifiers(&crypto->dev, dev);
> }
> 
> diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> index 5dedb2d987..7bfcaef976 100644
> --- a/backends/vhost-user.c
> +++ b/backends/vhost-user.c
> @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
>     }
> 
>     b->dev.acked_features = b->vdev->guest_features;
> -    ret = vhost_dev_start(&b->dev, b->vdev);
> +    ret = vhost_dev_start(&b->dev, b->vdev, true);
>     if (ret < 0) {
>         error_report("Error start vhost dev");
>         goto err_guest_notifiers;
> @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
>         return;
>     }
> 
> -    vhost_dev_stop(&b->dev, b->vdev);
> +    vhost_dev_stop(&b->dev, b->vdev, true);
> 
>     if (k->set_guest_notifiers) {
>         ret = k->set_guest_notifiers(qbus->parent,
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 0d5190accf..1177064631 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>     }
> 
>     s->dev.vq_index_end = s->dev.nvqs;
> -    ret = vhost_dev_start(&s->dev, vdev);
> +    ret = vhost_dev_start(&s->dev, vdev, true);
>     if (ret < 0) {
>         error_setg_errno(errp, -ret, "Error starting vhost");
>         goto err_guest_notifiers;
> @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>         return;
>     }
> 
> -    vhost_dev_stop(&s->dev, vdev);
> +    vhost_dev_stop(&s->dev, vdev, true);
> 
>     ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>     if (ret < 0) {
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 26e4930676..043058ff43 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
>         goto fail_notifiers;
>     }
> 
> -    r = vhost_dev_start(&net->dev, dev);
> +    r = vhost_dev_start(&net->dev, dev, false);
>     if (r < 0) {
>         goto fail_start;
>     }
> @@ -308,7 +308,7 @@ fail:
>     if (net->nc->info->poll) {
>         net->nc->info->poll(net->nc, true);
>     }
> -    vhost_dev_stop(&net->dev, dev);
> +    vhost_dev_stop(&net->dev, dev, false);
> fail_start:
>     vhost_dev_disable_notifiers(&net->dev, dev);
> fail_notifiers:
> @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
>     if (net->nc->info->poll) {
>         net->nc->info->poll(net->nc, true);
>     }
> -    vhost_dev_stop(&net->dev, dev);
> +    vhost_dev_stop(&net->dev, dev, false);
>     if (net->nc->info->stop) {
>         net->nc->info->stop(net->nc);
>     }
> @@ -606,7 +606,7 @@ err_start:
>         assert(r >= 0);
>     }
> 
> -    vhost_dev_stop(&net->dev, vdev);
> +    vhost_dev_stop(&net->dev, vdev, false);
> 
>     return r;
> }
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index 767f827e55..18ea5dcfa1 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>         goto err_guest_notifiers;
>     }
> 
> -    ret = vhost_dev_start(&vsc->dev, vdev);
> +    ret = vhost_dev_start(&vsc->dev, vdev, true);
>     if (ret < 0) {
>         error_report("Error start vhost dev");
>         goto err_guest_notifiers;
> @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>     int ret = 0;
> 
> -    vhost_dev_stop(&vsc->dev, vdev);
> +    vhost_dev_stop(&vsc->dev, vdev, true);
> 
>     if (k->set_guest_notifiers) {
>         ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index dc4014cdef..d97b179e6f 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
>     }
> 
>     fs->vhost_dev.acked_features = vdev->guest_features;
> -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
> +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
>     if (ret < 0) {
>         error_report("Error starting vhost: %d", -ret);
>         goto err_guest_notifiers;
> @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
>         return;
>     }
> 
> -    vhost_dev_stop(&fs->vhost_dev, vdev);
> +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
> 
>     ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
>     if (ret < 0) {
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 5851cb3bc9..0b40ebd15a 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
>      */
>     vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
> 
> -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
> +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
>     if (ret < 0) {
>         error_report("Error starting vhost-user-gpio: %d", ret);
>         goto err_guest_notifiers;
> @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
>         return;
>     }
> 
> -    vhost_dev_stop(vhost_dev, vdev);
> +    vhost_dev_stop(vhost_dev, vdev, false);
> 
>     ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
>     if (ret < 0) {
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index 1c9f3d20dc..dc5c828ba6 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
> 
>     i2c->vhost_dev.acked_features = vdev->guest_features;
> 
> -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
> +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
>     if (ret < 0) {
>         error_report("Error starting vhost-user-i2c: %d", -ret);
>         goto err_guest_notifiers;
> @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>         return;
>     }
> 
> -    vhost_dev_stop(&i2c->vhost_dev, vdev);
> +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
> 
>     ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
>     if (ret < 0) {
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index f9084cde58..201a39e220 100644
> --- a/hw/virtio/vhost-user-rng.c
> +++ b/hw/virtio/vhost-user-rng.c
> @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
>     }
> 
>     rng->vhost_dev.acked_features = vdev->guest_features;
> -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
> +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
>     if (ret < 0) {
>         error_report("Error starting vhost-user-rng: %d", -ret);
>         goto err_guest_notifiers;
> @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>         return;
>     }
> 
> -    vhost_dev_stop(&rng->vhost_dev, vdev);
> +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
> 
>     ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
>     if (ret < 0) {
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index a67a275de2..d21c72b401 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
>     }
> 
>     vvc->vhost_dev.acked_features = vdev->guest_features;
> -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
> +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
>     if (ret < 0) {
>         error_report("Error starting vhost: %d", -ret);
>         goto err_guest_notifiers;
> @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
>         return;
>     }
> 
> -    vhost_dev_stop(&vvc->vhost_dev, vdev);
> +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
> 
>     ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
>     if (ret < 0) {
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index d1c4c20b8c..7fb008bc9e 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
>     return 0;
> }
> 
> +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
> +{
> +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
> +        return 0;
> +    }
> +
> +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
> +}
> +
> /* Host notifiers must be enabled at this point. */
> -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
> {
>     int i, r;
> 
>     /* should only be called after backend is connected */
>     assert(hdev->vhost_ops);
> 
> -    trace_vhost_dev_start(hdev, vdev->name);
> +    trace_vhost_dev_start(hdev, vdev->name, vrings);
> 
>     vdev->vhost_started = true;
>     hdev->started = true;
> @@ -1830,10 +1851,16 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>             goto fail_log;
>         }
>     }
> +    if (vrings) {
> +        r = vhost_dev_set_vring_enable(hdev, true);
> +        if (r) {
> +            goto fail_log;
> +        }
> +    }
>     if (hdev->vhost_ops->vhost_dev_start) {
>         r = hdev->vhost_ops->vhost_dev_start(hdev, true);
>         if (r) {
> -            goto fail_log;
> +            goto fail_start;
>         }
>     }
>     if (vhost_dev_has_iommu(hdev) &&
> @@ -1848,6 +1875,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>         }
>     }
>     return 0;
> +fail_start:
> +    if (vrings) {
> +        vhost_dev_set_vring_enable(hdev, false);
> +    }
> fail_log:
>     vhost_log_put(hdev, false);
> fail_vq:
> @@ -1866,18 +1897,21 @@ fail_features:
> }
> 
> /* Host notifiers must be enabled at this point. */
> -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
> {
>     int i;
> 
>     /* should only be called after backend is connected */
>     assert(hdev->vhost_ops);
> 
> -    trace_vhost_dev_stop(hdev, vdev->name);
> +    trace_vhost_dev_stop(hdev, vdev->name, vrings);
> 
>     if (hdev->vhost_ops->vhost_dev_start) {
>         hdev->vhost_ops->vhost_dev_start(hdev, false);
>     }
> +    if (vrings) {
> +        vhost_dev_set_vring_enable(hdev, false);
> +    }
>     for (i = 0; i < hdev->nvqs; ++i) {
>         vhost_virtqueue_stop(hdev,
>                              vdev,
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 820dadc26c..14fc5b9bb2 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -9,8 +9,8 @@ vhost_section(const char *name) "%s"
> vhost_reject_section(const char *name, int d) "%s:%d"
> vhost_iotlb_miss(void *dev, int step) "%p step %d"
> vhost_dev_cleanup(void *dev) "%p"
> -vhost_dev_start(void *dev, const char *name) "%p:%s"
> -vhost_dev_stop(void *dev, const char *name) "%p:%s"
> +vhost_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> +vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> 
> 
> # vhost-user.c
> -- 
> 2.38.1
>
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Michael S. Tsirkin 1 year, 5 months ago 
On Thu, Nov 24, 2022 at 12:19:25AM +0000, Raphael Norwitz wrote:
> 
> > On Nov 23, 2022, at 8:16 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
> > 
> > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
> > backend, but we forgot to enable vrings as specified in
> > docs/interop/vhost-user.rst:
> > 
> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
> >    ring starts directly in the enabled state.
> > 
> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
> >    initialized in a disabled state and is enabled by
> >    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
> > 
> > Some vhost-user front-ends already did this by calling
> > vhost_ops.vhost_set_vring_enable() directly:
> > - backends/cryptodev-vhost.c
> > - hw/net/virtio-net.c
> > - hw/virtio/vhost-user-gpio.c
> 
> To simplify why not rather change these devices to use the new semantics?

Granted this is already scary enough for this release.

> > 
> > But most didn't do that, so we would leave the vrings disabled and some
> > backends would not work. We observed this issue with the rust version of
> > virtiofsd [1], which uses the event loop [2] provided by the
> > vhost-user-backend crate where requests are not processed if vring is
> > not enabled.
> > 
> > Let's fix this issue by enabling the vrings in vhost_dev_start() for
> > vhost-user front-ends that don't already do this directly. Same thing
> > also in vhost_dev_stop() where we disable vrings.
> > 
> > [1] https://gitlab.com/virtio-fs/virtiofsd
> > [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
> > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
> > Reported-by: German Maglione <gmaglione@redhat.com>
> > Tested-by: German Maglione <gmaglione@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Looks good for vhost-user-blk/vhost-user-scsi.
> 
> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> 
> > ---
> > include/hw/virtio/vhost.h      |  6 +++--
> > backends/cryptodev-vhost.c     |  4 ++--
> > backends/vhost-user.c          |  4 ++--
> > hw/block/vhost-user-blk.c      |  4 ++--
> > hw/net/vhost_net.c             |  8 +++----
> > hw/scsi/vhost-scsi-common.c    |  4 ++--
> > hw/virtio/vhost-user-fs.c      |  4 ++--
> > hw/virtio/vhost-user-gpio.c    |  4 ++--
> > hw/virtio/vhost-user-i2c.c     |  4 ++--
> > hw/virtio/vhost-user-rng.c     |  4 ++--
> > hw/virtio/vhost-vsock-common.c |  4 ++--
> > hw/virtio/vhost.c              | 44 ++++++++++++++++++++++++++++++----
> > hw/virtio/trace-events         |  4 ++--
> > 13 files changed, 67 insertions(+), 31 deletions(-)
> > 
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 353252ac3e..67a6807fac 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
> >  * vhost_dev_start() - start the vhost device
> >  * @hdev: common vhost_dev structure
> >  * @vdev: the VirtIODevice structure
> > + * @vrings: true to have vrings enabled in this call
> >  *
> >  * Starts the vhost device. From this point VirtIO feature negotiation
> >  * can start and the device can start processing VirtIO transactions.
> >  *
> >  * Return: 0 on success, < 0 on error.
> >  */
> > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> > 
> > /**
> >  * vhost_dev_stop() - stop the vhost device
> >  * @hdev: common vhost_dev structure
> >  * @vdev: the VirtIODevice structure
> > + * @vrings: true to have vrings disabled in this call
> >  *
> >  * Stop the vhost device. After the device is stopped the notifiers
> >  * can be disabled (@vhost_dev_disable_notifiers) and the device can
> >  * be torn down (@vhost_dev_cleanup).
> >  */
> > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
> > 
> > /**
> >  * DOC: vhost device configuration handling
> > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> > index bc13e466b4..572f87b3be 100644
> > --- a/backends/cryptodev-vhost.c
> > +++ b/backends/cryptodev-vhost.c
> > @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *crypto,
> >         goto fail_notifiers;
> >     }
> > 
> > -    r = vhost_dev_start(&crypto->dev, dev);
> > +    r = vhost_dev_start(&crypto->dev, dev, false);
> >     if (r < 0) {
> >         goto fail_start;
> >     }
> > @@ -111,7 +111,7 @@ static void
> > cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto,
> >                                  VirtIODevice *dev)
> > {
> > -    vhost_dev_stop(&crypto->dev, dev);
> > +    vhost_dev_stop(&crypto->dev, dev, false);
> >     vhost_dev_disable_notifiers(&crypto->dev, dev);
> > }
> > 
> > diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> > index 5dedb2d987..7bfcaef976 100644
> > --- a/backends/vhost-user.c
> > +++ b/backends/vhost-user.c
> > @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b)
> >     }
> > 
> >     b->dev.acked_features = b->vdev->guest_features;
> > -    ret = vhost_dev_start(&b->dev, b->vdev);
> > +    ret = vhost_dev_start(&b->dev, b->vdev, true);
> >     if (ret < 0) {
> >         error_report("Error start vhost dev");
> >         goto err_guest_notifiers;
> > @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(&b->dev, b->vdev);
> > +    vhost_dev_stop(&b->dev, b->vdev, true);
> > 
> >     if (k->set_guest_notifiers) {
> >         ret = k->set_guest_notifiers(qbus->parent,
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 0d5190accf..1177064631 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
> >     }
> > 
> >     s->dev.vq_index_end = s->dev.nvqs;
> > -    ret = vhost_dev_start(&s->dev, vdev);
> > +    ret = vhost_dev_start(&s->dev, vdev, true);
> >     if (ret < 0) {
> >         error_setg_errno(errp, -ret, "Error starting vhost");
> >         goto err_guest_notifiers;
> > @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(&s->dev, vdev);
> > +    vhost_dev_stop(&s->dev, vdev, true);
> > 
> >     ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> >     if (ret < 0) {
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 26e4930676..043058ff43 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net *net,
> >         goto fail_notifiers;
> >     }
> > 
> > -    r = vhost_dev_start(&net->dev, dev);
> > +    r = vhost_dev_start(&net->dev, dev, false);
> >     if (r < 0) {
> >         goto fail_start;
> >     }
> > @@ -308,7 +308,7 @@ fail:
> >     if (net->nc->info->poll) {
> >         net->nc->info->poll(net->nc, true);
> >     }
> > -    vhost_dev_stop(&net->dev, dev);
> > +    vhost_dev_stop(&net->dev, dev, false);
> > fail_start:
> >     vhost_dev_disable_notifiers(&net->dev, dev);
> > fail_notifiers:
> > @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> >     if (net->nc->info->poll) {
> >         net->nc->info->poll(net->nc, true);
> >     }
> > -    vhost_dev_stop(&net->dev, dev);
> > +    vhost_dev_stop(&net->dev, dev, false);
> >     if (net->nc->info->stop) {
> >         net->nc->info->stop(net->nc);
> >     }
> > @@ -606,7 +606,7 @@ err_start:
> >         assert(r >= 0);
> >     }
> > 
> > -    vhost_dev_stop(&net->dev, vdev);
> > +    vhost_dev_stop(&net->dev, vdev, false);
> > 
> >     return r;
> > }
> > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > index 767f827e55..18ea5dcfa1 100644
> > --- a/hw/scsi/vhost-scsi-common.c
> > +++ b/hw/scsi/vhost-scsi-common.c
> > @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> >         goto err_guest_notifiers;
> >     }
> > 
> > -    ret = vhost_dev_start(&vsc->dev, vdev);
> > +    ret = vhost_dev_start(&vsc->dev, vdev, true);
> >     if (ret < 0) {
> >         error_report("Error start vhost dev");
> >         goto err_guest_notifiers;
> > @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> >     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> >     int ret = 0;
> > 
> > -    vhost_dev_stop(&vsc->dev, vdev);
> > +    vhost_dev_stop(&vsc->dev, vdev, true);
> > 
> >     if (k->set_guest_notifiers) {
> >         ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index dc4014cdef..d97b179e6f 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev)
> >     }
> > 
> >     fs->vhost_dev.acked_features = vdev->guest_features;
> > -    ret = vhost_dev_start(&fs->vhost_dev, vdev);
> > +    ret = vhost_dev_start(&fs->vhost_dev, vdev, true);
> >     if (ret < 0) {
> >         error_report("Error starting vhost: %d", -ret);
> >         goto err_guest_notifiers;
> > @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(&fs->vhost_dev, vdev);
> > +    vhost_dev_stop(&fs->vhost_dev, vdev, true);
> > 
> >     ret = k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvqs, false);
> >     if (ret < 0) {
> > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> > index 5851cb3bc9..0b40ebd15a 100644
> > --- a/hw/virtio/vhost-user-gpio.c
> > +++ b/hw/virtio/vhost-user-gpio.c
> > @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
> >      */
> >     vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
> > 
> > -    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
> > +    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
> >     if (ret < 0) {
> >         error_report("Error starting vhost-user-gpio: %d", ret);
> >         goto err_guest_notifiers;
> > @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(vhost_dev, vdev);
> > +    vhost_dev_stop(vhost_dev, vdev, false);
> > 
> >     ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
> >     if (ret < 0) {
> > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> > index 1c9f3d20dc..dc5c828ba6 100644
> > --- a/hw/virtio/vhost-user-i2c.c
> > +++ b/hw/virtio/vhost-user-i2c.c
> > @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev)
> > 
> >     i2c->vhost_dev.acked_features = vdev->guest_features;
> > 
> > -    ret = vhost_dev_start(&i2c->vhost_dev, vdev);
> > +    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
> >     if (ret < 0) {
> >         error_report("Error starting vhost-user-i2c: %d", -ret);
> >         goto err_guest_notifiers;
> > @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(&i2c->vhost_dev, vdev);
> > +    vhost_dev_stop(&i2c->vhost_dev, vdev, true);
> > 
> >     ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
> >     if (ret < 0) {
> > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> > index f9084cde58..201a39e220 100644
> > --- a/hw/virtio/vhost-user-rng.c
> > +++ b/hw/virtio/vhost-user-rng.c
> > @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev)
> >     }
> > 
> >     rng->vhost_dev.acked_features = vdev->guest_features;
> > -    ret = vhost_dev_start(&rng->vhost_dev, vdev);
> > +    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
> >     if (ret < 0) {
> >         error_report("Error starting vhost-user-rng: %d", -ret);
> >         goto err_guest_notifiers;
> > @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(&rng->vhost_dev, vdev);
> > +    vhost_dev_stop(&rng->vhost_dev, vdev, true);
> > 
> >     ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
> >     if (ret < 0) {
> > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > index a67a275de2..d21c72b401 100644
> > --- a/hw/virtio/vhost-vsock-common.c
> > +++ b/hw/virtio/vhost-vsock-common.c
> > @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
> >     }
> > 
> >     vvc->vhost_dev.acked_features = vdev->guest_features;
> > -    ret = vhost_dev_start(&vvc->vhost_dev, vdev);
> > +    ret = vhost_dev_start(&vvc->vhost_dev, vdev, true);
> >     if (ret < 0) {
> >         error_report("Error starting vhost: %d", -ret);
> >         goto err_guest_notifiers;
> > @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev)
> >         return;
> >     }
> > 
> > -    vhost_dev_stop(&vvc->vhost_dev, vdev);
> > +    vhost_dev_stop(&vvc->vhost_dev, vdev, true);
> > 
> >     ret = k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nvqs, false);
> >     if (ret < 0) {
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index d1c4c20b8c..7fb008bc9e 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> >     return 0;
> > }
> > 
> > +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
> > +{
> > +    if (!hdev->vhost_ops->vhost_set_vring_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)) {
> > +        return 0;
> > +    }
> > +
> > +    return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
> > +}
> > +
> > /* Host notifiers must be enabled at this point. */
> > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
> > {
> >     int i, r;
> > 
> >     /* should only be called after backend is connected */
> >     assert(hdev->vhost_ops);
> > 
> > -    trace_vhost_dev_start(hdev, vdev->name);
> > +    trace_vhost_dev_start(hdev, vdev->name, vrings);
> > 
> >     vdev->vhost_started = true;
> >     hdev->started = true;
> > @@ -1830,10 +1851,16 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >             goto fail_log;
> >         }
> >     }
> > +    if (vrings) {
> > +        r = vhost_dev_set_vring_enable(hdev, true);
> > +        if (r) {
> > +            goto fail_log;
> > +        }
> > +    }
> >     if (hdev->vhost_ops->vhost_dev_start) {
> >         r = hdev->vhost_ops->vhost_dev_start(hdev, true);
> >         if (r) {
> > -            goto fail_log;
> > +            goto fail_start;
> >         }
> >     }
> >     if (vhost_dev_has_iommu(hdev) &&
> > @@ -1848,6 +1875,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >         }
> >     }
> >     return 0;
> > +fail_start:
> > +    if (vrings) {
> > +        vhost_dev_set_vring_enable(hdev, false);
> > +    }
> > fail_log:
> >     vhost_log_put(hdev, false);
> > fail_vq:
> > @@ -1866,18 +1897,21 @@ fail_features:
> > }
> > 
> > /* Host notifiers must be enabled at this point. */
> > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
> > {
> >     int i;
> > 
> >     /* should only be called after backend is connected */
> >     assert(hdev->vhost_ops);
> > 
> > -    trace_vhost_dev_stop(hdev, vdev->name);
> > +    trace_vhost_dev_stop(hdev, vdev->name, vrings);
> > 
> >     if (hdev->vhost_ops->vhost_dev_start) {
> >         hdev->vhost_ops->vhost_dev_start(hdev, false);
> >     }
> > +    if (vrings) {
> > +        vhost_dev_set_vring_enable(hdev, false);
> > +    }
> >     for (i = 0; i < hdev->nvqs; ++i) {
> >         vhost_virtqueue_stop(hdev,
> >                              vdev,
> > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > index 820dadc26c..14fc5b9bb2 100644
> > --- a/hw/virtio/trace-events
> > +++ b/hw/virtio/trace-events
> > @@ -9,8 +9,8 @@ vhost_section(const char *name) "%s"
> > vhost_reject_section(const char *name, int d) "%s:%d"
> > vhost_iotlb_miss(void *dev, int step) "%p step %d"
> > vhost_dev_cleanup(void *dev) "%p"
> > -vhost_dev_start(void *dev, const char *name) "%p:%s"
> > -vhost_dev_stop(void *dev, const char *name) "%p:%s"
> > +vhost_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> > +vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> > 
> > 
> > # vhost-user.c
> > -- 
> > 2.38.1
> >
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Stefano Garzarella 1 year, 5 months ago 
On Thu, Nov 24, 2022 at 01:50:19AM -0500, Michael S. Tsirkin wrote:
>On Thu, Nov 24, 2022 at 12:19:25AM +0000, Raphael Norwitz wrote:
>>
>> > On Nov 23, 2022, at 8:16 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >
>> > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
>> > backend, but we forgot to enable vrings as specified in
>> > docs/interop/vhost-user.rst:
>> >
>> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>> >    ring starts directly in the enabled state.
>> >
>> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>> >    initialized in a disabled state and is enabled by
>> >    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>> >
>> > Some vhost-user front-ends already did this by calling
>> > vhost_ops.vhost_set_vring_enable() directly:
>> > - backends/cryptodev-vhost.c
>> > - hw/net/virtio-net.c
>> > - hw/virtio/vhost-user-gpio.c
>>
>> To simplify why not rather change these devices to use the new 
>> semantics?

Maybe the only one I wouldn't be scared of is vhost-user-gpio, but for 
example vhost-net would require a lot of changes, maybe better after the 
release.

For example, we could do like vhost-vdpa and call SET_VRING_ENABLE in 
the VhostOps.vhost_dev_start callback of vhost-user.c, but I think it's 
too risky to do that now.

>
>Granted this is already scary enough for this release.

Yeah, I tried to touch as little as possible but I'm scared too, I just 
haven't found a better solution for now :-(

>
>> >
>> > But most didn't do that, so we would leave the vrings disabled and some
>> > backends would not work. We observed this issue with the rust version of
>> > virtiofsd [1], which uses the event loop [2] provided by the
>> > vhost-user-backend crate where requests are not processed if vring is
>> > not enabled.
>> >
>> > Let's fix this issue by enabling the vrings in vhost_dev_start() for
>> > vhost-user front-ends that don't already do this directly. Same thing
>> > also in vhost_dev_stop() where we disable vrings.
>> >
>> > [1] https://gitlab.com/virtio-fs/virtiofsd
>> > [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217
>> > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>> > Reported-by: German Maglione <gmaglione@redhat.com>
>> > Tested-by: German Maglione <gmaglione@redhat.com>
>> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>
>> Looks good for vhost-user-blk/vhost-user-scsi.
>>
>> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

Thanks for the review!
Stefano
Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices
Posted by Raphael Norwitz 1 year, 5 months ago 

> On Nov 24, 2022, at 2:54 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> On Thu, Nov 24, 2022 at 01:50:19AM -0500, Michael S. Tsirkin wrote:
>> On Thu, Nov 24, 2022 at 12:19:25AM +0000, Raphael Norwitz wrote:
>>> 
>>> > On Nov 23, 2022, at 8:16 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
>>> >
>>> > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>>> > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user
>>> > backend, but we forgot to enable vrings as specified in
>>> > docs/interop/vhost-user.rst:
>>> >
>>> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>>> >    ring starts directly in the enabled state.
>>> >
>>> >    If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>>> >    initialized in a disabled state and is enabled by
>>> >    ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>>> >
>>> > Some vhost-user front-ends already did this by calling
>>> > vhost_ops.vhost_set_vring_enable() directly:
>>> > - backends/cryptodev-vhost.c
>>> > - hw/net/virtio-net.c
>>> > - hw/virtio/vhost-user-gpio.c
>>> 
>>> To simplify why not rather change these devices to use the new semantics?
> 
> Maybe the only one I wouldn't be scared of is vhost-user-gpio, but for example vhost-net would require a lot of changes, maybe better after the release.
> 
> For example, we could do like vhost-vdpa and call SET_VRING_ENABLE in the VhostOps.vhost_dev_start callback of vhost-user.c, but I think it's too risky to do that now.
> 
>> 
>> Granted this is already scary enough for this release.
> 
> Yeah, I tried to touch as little as possible but I'm scared too, I just haven't found a better solution for now :-(
> 

Sure - no need to force a more disruptive change in right before the release. If anything can be simplified later.

>> 
>>> >
>>> > But most didn't do that, so we would leave the vrings disabled and some
>>> > backends would not work. We observed this issue with the rust version of
>>> > virtiofsd [1], which uses the event loop [2] provided by the
>>> > vhost-user-backend crate where requests are not processed if vring is
>>> > not enabled.
>>> >
>>> > Let's fix this issue by enabling the vrings in vhost_dev_start() for
>>> > vhost-user front-ends that don't already do this directly. Same thing
>>> > also in vhost_dev_stop() where we disable vrings.
>>> >
>>> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_virtio-2Dfs_virtiofsd&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=PcC4TEq5C80Knek-ScCNI26rQ13h0n3QEMNNhc-ENd7Txd8wHYqwC1TYfXW_hYor&s=5pdxt8m4-ks8VB2tRSXQV05kdfdP50iy-aAxuGe-Ffc&e= > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_rust-2Dvmm_vhost_blob_240fc2966_crates_vhost-2Duser-2Dbackend_src_event-5Floop.rs-23L217&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=PcC4TEq5C80Knek-ScCNI26rQ13h0n3QEMNNhc-ENd7Txd8wHYqwC1TYfXW_hYor&s=-3NUG1pPKN-FwUeDkuu52roXoPoeLR1y4gjrddHUz2U&e= > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features")
>>> > Reported-by: German Maglione <gmaglione@redhat.com>
>>> > Tested-by: German Maglione <gmaglione@redhat.com>
>>> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> 
>>> Looks good for vhost-user-blk/vhost-user-scsi.
>>> 
>>> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> 
> Thanks for the review!
> Stefano

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.