Re: [PATCH] block/vhost-user-blk: Fix hang on boot for some odd guests

[Andrey Ryabinin]

On 4/10/23 10:35, Andrey Ryabinin wrote:
> Some guests hang on boot when using the vhost-user-blk-pci device,
> but boot normally when using the virtio-blk device. The problem occurs
> because the guest advertises VIRTIO_F_VERSION_1 but kicks the virtqueue
> before setting VIRTIO_CONFIG_S_DRIVER_OK, causing vdev->start_on_kick to
> be false in vhost_user_blk_handle_output() and preventing the device from
> starting.
> 
> Fix this by removing the check for vdev->start_on_kick to ensure
> that the device starts after the kick. This aligns the behavior of
> 'vhost-user-blk-pci' device with 'virtio-blk' as it does the similar
> thing in its virtio_blk_handle_output() function.
> 
> Fixes: 110b9463d5c8 ("vhost-user-blk: start vhost when guest kicks")
> Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com>
> ---
>  hw/block/vhost-user-blk.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index aff4d2b8cbd..448ead448f3 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -279,10 +279,6 @@ static void vhost_user_blk_handle_output(VirtIODevice 
> *vdev, VirtQueue *vq)
>      Error *local_err = NULL;
>      int i, ret;
>  
> -    if (!vdev->start_on_kick) {
> -        return;
> -    }
> -
>      if (!s->connected) {
>          return;
>      }


After looking a bit closer to this ->start_on_kick thing ( commit badaf79cfdbd 
("virtio: Introduce started flag to VirtioDevice")
and follow ups) I'm starting to think that removing it entirely would be the 
right thing to do here.
The whole reason for it was to add special case for !VIRTIO_F_VERSION_1 guests.
If we making start on kick thing for misbehaving VIRTIO_F_VERSION_1 guests too, 
than the flag is no longer required,
so we can do following:

---
 hw/block/vhost-user-blk.c  |  4 ----
 hw/virtio/virtio-qmp.c     |  2 +-
 hw/virtio/virtio.c         | 21 ++-------------------
 include/hw/virtio/virtio.h |  5 -----
 4 files changed, 3 insertions(+), 29 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index aff4d2b8cbd..448ead448f3 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -279,10 +279,6 @@ static void vhost_user_blk_handle_output(VirtIODevice 
*vdev, VirtQueue *vq)
     Error *local_err = NULL;
     int i, ret;
 
-    if (!vdev->start_on_kick) {
-        return;
-    }
-
     if (!s->connected) {
         return;
     }
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index e4d4bece2d7..4865819cd2f 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -773,7 +773,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, 
Error **errp)
     status->disabled = vdev->disabled;
     status->use_started = vdev->use_started;
     status->started = vdev->started;
-    status->start_on_kick = vdev->start_on_kick;
+    status->start_on_kick = true;
     status->disable_legacy_check = vdev->disable_legacy_check;
     status->bus_name = g_strdup(vdev->bus_name);
     status->use_guest_notifier_mask = vdev->use_guest_notifier_mask;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fcd..218584eae85 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2126,7 +2126,6 @@ void virtio_reset(void *opaque)
         k->reset(vdev);
     }
 
-    vdev->start_on_kick = false;
     vdev->started = false;
     vdev->broken = false;
     vdev->guest_features = 0;
@@ -2248,9 +2247,7 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
         trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
         vq->handle_output(vdev, vq);
 
-        if (unlikely(vdev->start_on_kick)) {
-            virtio_set_started(vdev, true);
-        }
+        virtio_set_started(vdev, true);
     }
 }
 
@@ -2268,9 +2265,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
     } else if (vq->handle_output) {
         vq->handle_output(vdev, vq);
 
-        if (unlikely(vdev->start_on_kick)) {
-            virtio_set_started(vdev, true);
-        }
+        virtio_set_started(vdev, true);
     }
 }
 
@@ -2881,12 +2876,6 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
             }
         }
     }
-    if (!ret) {
-        if (!virtio_device_started(vdev, vdev->status) &&
-            !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
-            vdev->start_on_kick = true;
-        }
-    }
     return ret;
 }
 
@@ -3039,11 +3028,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
         }
     }
 
-    if (!virtio_device_started(vdev, vdev->status) &&
-        !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
-        vdev->start_on_kick = true;
-    }
-
     RCU_READ_LOCK_GUARD();
     for (i = 0; i < num; i++) {
         if (vdev->vq[i].vring.desc) {
@@ -3162,7 +3146,6 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, 
size_t config_size)
             g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
     }
 
-    vdev->start_on_kick = false;
     vdev->started = false;
     vdev->vhost_started = false;
     vdev->device_id = device_id;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 77c6c55929f..5742876b4fa 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -144,7 +144,6 @@ struct VirtIODevice
      */
     bool use_started;
     bool started;
-    bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
     bool disable_legacy_check;
     bool vhost_started;
     VMChangeStateEntry *vmstate;
@@ -460,10 +459,6 @@ static inline bool virtio_device_should_start(VirtIODevice 
*vdev, uint8_t status
 
 static inline void virtio_set_started(VirtIODevice *vdev, bool started)
 {
-    if (started) {
-        vdev->start_on_kick = false;
-    }
-
     if (vdev->use_started) {
         vdev->started = started;
     }
-- 
2.39.2