Re: [Qemu-devel] [PULL v2 04/36] virtio: Introduce started flag to VirtioDevice

[Greg Kurz]

On Mon, 20 May 2019 19:10:35 -0400
"Michael S. Tsirkin" <address@hidden> wrote:

> From: Xie Yongji <address@hidden>
> 
> The virtio 1.0 transitional devices support driver uses the device
> before setting the DRIVER_OK status bit. So we introduce a started
> flag to indicate whether driver has started the device or not.
> 
> Signed-off-by: Xie Yongji <address@hidden>
> Signed-off-by: Zhang Yu <address@hidden>
> Message-Id: <address@hidden>
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  include/hw/virtio/virtio.h |  2 ++
>  hw/virtio/virtio.c         | 52 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 7140381e3a..27c0efc3d0 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -105,6 +105,8 @@ struct VirtIODevice
>      uint16_t device_id;
>      bool vm_running;
>      bool broken; /* device in invalid state, needs reset */
> +    bool started;
> +    bool start_on_kick; /* virtio 1.0 transitional devices support that */
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
>      uint8_t device_endian;
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 28056a7ef7..5d533ac74e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1162,10 +1162,16 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>              }
>          }
>      }
> +    vdev->started = val & VIRTIO_CONFIG_S_DRIVER_OK;
> +    if (unlikely(vdev->start_on_kick && vdev->started)) {
> +        vdev->start_on_kick = false;
> +    }
> +
>      if (k->set_status) {
>          k->set_status(vdev, val);
>      }
>      vdev->status = val;
> +
>      return 0;
>  }
>  
> @@ -1208,6 +1214,9 @@ void virtio_reset(void *opaque)
>          k->reset(vdev);
>      }
>  
> +    vdev->start_on_kick = (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) 
> &&
> +                          !virtio_vdev_has_feature(vdev, 
> VIRTIO_F_VERSION_1));
> +    vdev->started = false;
>      vdev->broken = false;
>      vdev->guest_features = 0;
>      vdev->queue_sel = 0;
> @@ -1518,14 +1527,21 @@ void virtio_queue_set_align(VirtIODevice *vdev, int 
> n, int align)
>  
>  static bool virtio_queue_notify_aio_vq(VirtQueue *vq)
>  {
> +    bool ret = false;
> +
>      if (vq->vring.desc && vq->handle_aio_output) {
>          VirtIODevice *vdev = vq->vdev;
>  
>          trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
> -        return vq->handle_aio_output(vdev, vq);
> +        ret = vq->handle_aio_output(vdev, vq);
> +
> +        if (unlikely(vdev->start_on_kick)) {
> +            vdev->started = true;
> +            vdev->start_on_kick = false;
> +        }
>      }
>  
> -    return false;
> +    return ret;
>  }
>  
>  static void virtio_queue_notify_vq(VirtQueue *vq)
> @@ -1539,6 +1555,11 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
>  
>          trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
>          vq->handle_output(vdev, vq);
> +
> +        if (unlikely(vdev->start_on_kick)) {
> +            vdev->started = true;
> +            vdev->start_on_kick = false;
> +        }
>      }
>  }
>  
> @@ -1556,6 +1577,11 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
>      } else if (vq->handle_output) {
>          vq->handle_output(vdev, vq);
>      }
> +
> +    if (unlikely(vdev->start_on_kick)) {
> +        vdev->started = true;
> +        vdev->start_on_kick = false;
> +    }
>  }
>  
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> @@ -1770,6 +1796,13 @@ static bool virtio_broken_needed(void *opaque)
>      return vdev->broken;
>  }
>  
> +static bool virtio_started_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return vdev->started;

Existing machine types don't know about the "virtio/started" subsection. This
breaks migration to older QEMUs if the driver has started the device, ie. most
probably always when it comes to live migration.

My understanding is that we do try to support backward migration though. It
is a regular practice in datacenters to migrate workloads without having to
take care of the QEMU version. FWIW I had to fix similar issues downstream
many times in the past because customers had filed bugs.

Cc'ing David for his opinion.

> +}
> +
>  static const VMStateDescription vmstate_virtqueue = {
>      .name = "virtqueue_state",
>      .version_id = 1,
> @@ -1898,6 +1931,17 @@ static const VMStateDescription vmstate_virtio_broken 
> = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_virtio_started = {
> +    .name = "virtio/started",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_started_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(started, VirtIODevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio = {
>      .name = "virtio",
>      .version_id = 1,
> @@ -1913,6 +1957,7 @@ static const VMStateDescription vmstate_virtio = {
>          &vmstate_virtio_ringsize,
>          &vmstate_virtio_broken,
>          &vmstate_virtio_extra_state,
> +        &vmstate_virtio_started,
>          NULL
>      }
>  };
> @@ -2286,6 +2331,9 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>              g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
>      }
>  
> +    vdev->start_on_kick = (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) 
> &&
> +                          !virtio_vdev_has_feature(vdev, 
> VIRTIO_F_VERSION_1));
> +    vdev->started = false;
>      vdev->device_id = device_id;
>      vdev->status = 0;
>      atomic_set(&vdev->isr, 0);