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

[Raphael Norwitz]

Hey Andrey - apologies for the late reply here.

It sounds like you are dealing with a buggy guest, rather than a QEMU issue.

> On Apr 10, 2023, at 11:39 AM, Andrey Ryabinin <arbn@yandex-team.com> wrote:
> 
> 
> 
> 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

Virtio 1.1 Section 3.1.1, says during setup “[t]he driver MUST NOT notify the 
device before setting DRIVER_OK.”

Therefore what you are describing is buggy guest behavior. Sounds like the 
driver should be made to either
- not advertise VIRTIO_F_VERSION_1
- not kick before setting VIRTIO_CONFIG_S_DRIVER_OK

If anything, the virtio-blk virtio_blk_handle_output() function should probably 
check start_on_kick?

>> 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.

The virtio 1.0 spec section 2.1.2 explicitly says: "The device MUST NOT consume 
buffers or notify the driver before DRIVER_OK.”

Your change here would make QEMU violate this condition. I don’t know what the 
danger is but I assume that wording is there for a reason.

Unless MST or Cornellia (CCed) say otherwise I don’t think this is the correct 
approach.

> 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