diff mbox series

[v3,2/7] include/hw: VM state takes precedence in virtio_device_should_start

Message ID 20221128164105.1191058-3-alex.bennee@linaro.org
State Superseded
Headers show
Series fix vhost-user issues with CI | expand

Commit Message

Alex Bennée Nov. 28, 2022, 4:41 p.m. UTC
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>

---
v3
  - rm extra line
  - fix fn name in comment for virtio_device_started()
---
 include/hw/virtio/virtio.h | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Michael S. Tsirkin Nov. 28, 2022, 5:09 p.m. UTC | #1
On Mon, Nov 28, 2022 at 04:41:00PM +0000, Alex Bennée wrote:
> 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>


Doesn't this need to be like the last patch in the series?
Otherwise bisect will break on CI, right?

> ---
> v3
>   - rm extra line
>   - fix fn name in comment for virtio_device_started()
> ---
>  include/hw/virtio/virtio.h | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 0f612067f7..24561e933a 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,16 @@ static inline bool virtio_is_big_endian(VirtIODevice *vdev)
>      return false;
>  }
>  
> +/**
> + * virtio_device_started() - check if device started
> + * @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 +445,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)
> -- 
> 2.34.1
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 0f612067f7..24561e933a 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,16 @@  static inline bool virtio_is_big_endian(VirtIODevice *vdev)
     return false;
 }
 
+/**
+ * virtio_device_started() - check if device started
+ * @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 +445,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)