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

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

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>