Re: [PATCH] hw/virtio: Fix the de-initialization of vhost-user devices

[Manos Pitsidianakis]

On Tue, 18 Jun 2024 15:19, Thomas Huth <thuth@redhat.com> wrote:
> The unrealize functions of the various vhost-user devices are
> calling the corresponding vhost_*_set_status() functions with a
> status of 0 to shut down the device correctly.
>
> Now these vhost_*_set_status() functions all follow this scheme:
>
>    bool should_start = virtio_device_should_start(vdev, status);
>
>    if (vhost_dev_is_started(&vvc->vhost_dev) == should_start) {
>        return;
>    }
>
>    if (should_start) {
>        /* ... do the initialization stuff ... */
>    } else {
>        /* ... do the cleanup stuff ... */
>    }
>
> The problem here is virtio_device_should_start(vdev, 0) currently
> always returns "true" since it internally only looks at vdev->started
> instead of looking at the "status" parameter. Thus once the device
> got started once, virtio_device_should_start() always returns true


virtio_device_should_start() returning true if it's already started and 
running looks like a code smell to me... it intuitively feels like a 
ternary state instead of boolean: not startable, startable, already 
started.

> and thus the vhost_*_set_status() functions return early, without
> ever doing any clean-up when being called with status == 0. This
> causes e.g. problems when trying to hot-plug and hot-unplug a vhost
> user devices multiple times since the de-initialization step is
> completely skipped during the unplug operation.
>
> This bug has been introduced in commit 9f6bcfd99f ("hw/virtio: move
> vm_running check to virtio_device_started") which replaced
>
> should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>
> with
>
> should_start = virtio_device_started(vdev, status);
>
> which later got replaced by virtio_device_should_start(). This blocked
> the possibility to set should_start to false in case the status flag
> VIRTIO_CONFIG_S_DRIVER_OK was not set.
>
> Fix it by adjusting the virtio_device_should_start() function to
> only consider the status flag instead of vdev->started. Since this
> function is only used in the various vhost_*_set_status() functions
> for exactly the same purpose, it should be fine to fix it in this
> central place there without any risk to change the behavior of other
> code.
>
> Fixes: 9f6bcfd99f ("hw/virtio: move vm_running check to virtio_device_started")
> Buglink: https://issues.redhat.com/browse/RHEL-40708
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---


Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>