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

[Michael S. Tsirkin]

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