[5/5] virtio: add "use-started" property

From: Xie Yongji <xieyongji@baidu.com>

In order to avoid migration issues, we introduce a "use-started"
property to the base virtio device to indicate whether "started"
and "start_on_kick" flag could be used. This property will be
true by default and set to false when machine type <= 4.0.

Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/block/vhost-user-blk.c  |  8 ++++++--
 hw/core/machine.c          |  4 +++-
 hw/virtio/virtio.c         |  7 ++++---
 include/hw/virtio/virtio.h | 10 ++++++++++
 4 files changed, 23 insertions(+), 6 deletions(-)

On Wed, 29 May 2019 15:09:55 +0800
elohimes@gmail.com wrote:

> From: Xie Yongji <xieyongji@baidu.com>
> 
> In order to avoid migration issues, we introduce a "use-started"
> property to the base virtio device to indicate whether "started"
> and "start_on_kick" flag could be used. This property will be
> true by default and set to false when machine type <= 4.0.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> ---
>  hw/block/vhost-user-blk.c  |  8 ++++++--
>  hw/core/machine.c          |  4 +++-
>  hw/virtio/virtio.c         |  7 ++++---
>  include/hw/virtio/virtio.h | 10 ++++++++++
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9cb61336a6..520c5d3d4b 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -191,9 +191,13 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>  static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>  {
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -    bool should_start = vdev->started;
> +    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;

I guess you need the above because the set_status callback is called
before setting vdev->status, and virtio_device_started() thus doesn't
return the expected result for older machine types ?

What about adding a status argument to virtio_device_started() ?

    bool should_start = virtio_device_started(vdev, status);

>      int ret;
>  
> +    if (virtio_device_started(vdev)) {
> +        should_start = true;
> +    }
> +
>      if (!vdev->vm_running) {
>          should_start = false;
>      }
> @@ -317,7 +321,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
>      }
>  
>      /* restore vhost state */
> -    if (vdev->started) {
> +    if (virtio_device_started(vdev)) {

    if (virtio_device_started(vdev, vdev->status)) {

and so on...

>          ret = vhost_user_blk_start(vdev);
>          if (ret < 0) {
>              error_report("vhost-user-blk: vhost start failed: %s",
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 934c1bcceb..1730d28c1b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -24,7 +24,9 @@
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> -GlobalProperty hw_compat_4_0[] = {};
> +GlobalProperty hw_compat_4_0[] = {
> +    { "virtio-device", "use-started", "false" },
> +};
>  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
>  
>  GlobalProperty hw_compat_3_1[] = {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9e17293d46..4c05a9efe3 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1803,14 +1803,14 @@ static bool virtio_started_needed(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
>  
> -    return vdev->started;
> +    return vdev->started && vdev->use_started;

Hmm... the idea is that vdev->started should never be set when
"use-started" is "off". Instead of checking vdev->use_started
here, you should rather assign it to vdev->started everywhere
you currently assign true.

>  }
>  
>  static bool virtio_start_on_kick_needed(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
>  
> -    return vdev->start_on_kick;
> +    return vdev->start_on_kick && vdev->use_started;
>  }
>  
>  static const VMStateDescription vmstate_virtqueue = {
> @@ -2320,7 +2320,7 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
>      VirtIODevice *vdev = opaque;
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> -    bool backend_run = running && vdev->started;
> +    bool backend_run = running && virtio_device_started(vdev);
>      vdev->vm_running = running;
>  
>      if (backend_run) {
> @@ -2698,6 +2698,7 @@ static void virtio_device_instance_finalize(Object *obj)
>  
>  static Property virtio_properties[] = {
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
> +    DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 27c0efc3d0..ba4dbd7480 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -105,6 +105,7 @@ struct VirtIODevice
>      uint16_t device_id;
>      bool vm_running;
>      bool broken; /* device in invalid state, needs reset */
> +    bool use_started;
>      bool started;
>      bool start_on_kick; /* virtio 1.0 transitional devices support that */
>      VMChangeStateEntry *vmstate;
> @@ -351,4 +352,13 @@ static inline bool virtio_is_big_endian(VirtIODevice *vdev)
>      /* Devices conforming to VIRTIO 1.0 or later are always LE. */
>      return false;
>  }
> +
> +static inline bool virtio_device_started(VirtIODevice *vdev)
> +{
> +    if (vdev->use_started) {
> +        return vdev->started;
> +    }
> +
> +    return vdev->status & VIRTIO_CONFIG_S_DRIVER_OK;
> +}
>  #endif

On Tue, 4 Jun 2019 at 04:49, Greg Kurz <groug@kaod.org> wrote:
>
> On Wed, 29 May 2019 15:09:55 +0800
> elohimes@gmail.com wrote:
>
> > From: Xie Yongji <xieyongji@baidu.com>
> >
> > In order to avoid migration issues, we introduce a "use-started"
> > property to the base virtio device to indicate whether "started"
> > and "start_on_kick" flag could be used. This property will be
> > true by default and set to false when machine type <= 4.0.
> >
> > Suggested-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > ---
> >  hw/block/vhost-user-blk.c  |  8 ++++++--
> >  hw/core/machine.c          |  4 +++-
> >  hw/virtio/virtio.c         |  7 ++++---
> >  include/hw/virtio/virtio.h | 10 ++++++++++
> >  4 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 9cb61336a6..520c5d3d4b 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -191,9 +191,13 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> >  static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> >  {
> >      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > -    bool should_start = vdev->started;
> > +    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>
> I guess you need the above because the set_status callback is called
> before setting vdev->status, and virtio_device_started() thus doesn't
> return the expected result for older machine types ?
>

Yes, that's case.

> What about adding a status argument to virtio_device_started() ?
>
>     bool should_start = virtio_device_started(vdev, status);
>

Looks fine to me.

> >      int ret;
> >
> > +    if (virtio_device_started(vdev)) {
> > +        should_start = true;
> > +    }
> > +
> >      if (!vdev->vm_running) {
> >          should_start = false;
> >      }
> > @@ -317,7 +321,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
> >      }
> >
> >      /* restore vhost state */
> > -    if (vdev->started) {
> > +    if (virtio_device_started(vdev)) {
>
>     if (virtio_device_started(vdev, vdev->status)) {
>
> and so on...
>
> >          ret = vhost_user_blk_start(vdev);
> >          if (ret < 0) {
> >              error_report("vhost-user-blk: vhost start failed: %s",
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 934c1bcceb..1730d28c1b 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -24,7 +24,9 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/mem/nvdimm.h"
> >
> > -GlobalProperty hw_compat_4_0[] = {};
> > +GlobalProperty hw_compat_4_0[] = {
> > +    { "virtio-device", "use-started", "false" },
> > +};
> >  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
> >
> >  GlobalProperty hw_compat_3_1[] = {
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 9e17293d46..4c05a9efe3 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1803,14 +1803,14 @@ static bool virtio_started_needed(void *opaque)
> >  {
> >      VirtIODevice *vdev = opaque;
> >
> > -    return vdev->started;
> > +    return vdev->started && vdev->use_started;
>
> Hmm... the idea is that vdev->started should never be set when
> "use-started" is "off". Instead of checking vdev->use_started
> here, you should rather assign it to vdev->started everywhere
> you currently assign true.
>

Will do it in v2.

Thanks,
Yongji