Re: [PATCH v2 2/5] include/hw: VM state takes precedence in virtio_device_should_start

[Bernhard Beschow]

Am 25. November 2022 17:30:40 UTC schrieb "Alex Bennée" 
<alex.bennee@linaro.org>:
>The VM status should always preempt the device status for these
>checks. This ensures the device is in the correct state when we
>suspend the VM prior to migrations. This restores the checks to the
>order they where in before the refactoring moved things around.
>
>While we are at it lets improve our documentation of the various
>fields involved and document the two functions.
>
>Fixes: 9f6bcfd99f (hw/virtio: move vm_running check to virtio_device_started)
>Fixes: 259d69c00b (hw/virtio: introduce virtio_device_should_start)
>Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>---
> include/hw/virtio/virtio.h | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
>diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>index 0f612067f7..48f539d0fe 100644
>--- a/include/hw/virtio/virtio.h
>+++ b/include/hw/virtio/virtio.h
>@@ -133,6 +133,13 @@ struct VirtIODevice
>     bool broken; /* device in invalid state, needs reset */
>     bool use_disabled_flag; /* allow use of 'disable' flag when needed */
>     bool disabled; /* device in temporarily disabled state */
>+    /**
>+     * @use_started: true if the @started flag should be used to check the
>+     * current state of the VirtIO device. Otherwise status bits
>+     * should be checked for a current status of the device.
>+     * @use_started is only set via QMP and defaults to true for all
>+     * modern machines (since 4.1).
>+     */
>     bool use_started;
>     bool started;
>     bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
>@@ -408,6 +415,17 @@ static inline bool virtio_is_big_endian(VirtIODevice 
>*vdev)
>     return false;
> }
> 
>+

This adds an extra empty line.

>+/**
>+ * virtio_device_should_start() - check if device started

s/virtio_device_should_start/virtio_device_started/

Best regards,
Bernhard

>+ * @vdev - the VirtIO device
>+ * @status - the devices status bits
>+ *
>+ * Check if the device is started. For most modern machines this is
>+ * tracked via the @vdev->started field (to support migration),
>+ * otherwise we check for the final negotiated status bit that
>+ * indicates everything is ready.
>+ */
> static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
> {
>     if (vdev->use_started) {
>@@ -428,15 +446,11 @@ static inline bool virtio_device_started(VirtIODevice 
>*vdev, uint8_t status)
>  */
> static inline bool virtio_device_should_start(VirtIODevice *vdev, uint8_t 
> status)
> {
>-    if (vdev->use_started) {
>-        return vdev->started;
>-    }
>-
>     if (!vdev->vm_running) {
>         return false;
>     }
> 
>-    return status & VIRTIO_CONFIG_S_DRIVER_OK;
>+    return virtio_device_started(vdev, status);
> }
> 
> static inline void virtio_set_started(VirtIODevice *vdev, bool started)