[Qemu-devel] [PATCH 0/5] virtio: fix some issues of "started" and "start_on_kick" flag

[Qemu-devel] [PATCH 0/5] virtio: fix some issues of "started" and "start_on_kick" flag

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

We introduced two flags "started" and "start_on_kick" to indicate virtio
device's state before. But there still are some problems with them. So
we try to fixup them in this patchset.

The patch 1 fixes a regression bug that old guest is not able to boot with
vhost-user-blk device.

The patch 2,3,4 fix some problems with "started" and "start_on_kick" flag.

The patch 5 introduces a "use-started" property to avoid a migration
issue under Greg Kurz's suggestion [1].

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg06247.html

Xie Yongji (5):
  virtio: Set "start_on_kick" on virtio_set_features()
  virtio: Migrate the "start_on_kick" flag
  virtio: Make sure we get correct state of device on
    handle_aio_output()
  virtio: Don't change "started" flag on virtio_vmstate_change()
  virtio: add "use-started" property

 hw/block/vhost-user-blk.c  |  8 +++--
 hw/core/machine.c          |  4 ++-
 hw/virtio/virtio.c         | 67 +++++++++++++++++++++++++++-----------
 include/hw/virtio/virtio.h | 10 ++++++
 4 files changed, 67 insertions(+), 22 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/5] virtio: Set "start_on_kick" on virtio_set_features()

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

The guest feature is not set correctly on virtio_reset() and
virtio_init(). So we should not use it to set "start_on_kick" at that
point. This patch set "start_on_kick" on virtio_set_features() instead.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/virtio/virtio.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4805727b53..fc8fca81ad 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1214,8 +1214,7 @@ 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->start_on_kick = false;
     vdev->started = false;
     vdev->broken = false;
     vdev->guest_features = 0;
@@ -2069,14 +2068,21 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
         return -EINVAL;
     }
     ret = virtio_set_features_nocheck(vdev, val);
-    if (!ret && virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
-        int i;
-        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-            if (vdev->vq[i].vring.num != 0) {
-                virtio_init_region_cache(vdev, i);
+    if (!ret) {
+        if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
+            /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
+            int i;
+            for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+                if (vdev->vq[i].vring.num != 0) {
+                    virtio_init_region_cache(vdev, i);
+                }
             }
         }
+
+        if (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
+            !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+            vdev->start_on_kick = true;
+        }
     }
     return ret;
 }
@@ -2331,8 +2337,7 @@ 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->start_on_kick = false;
     vdev->started = false;
     vdev->device_id = device_id;
     vdev->status = 0;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/5] virtio: Set "start_on_kick" on virtio_set_features()

[Greg Kurz]

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

> From: Xie Yongji <xieyongji@baidu.com>
> 
> The guest feature is not set correctly on virtio_reset() and
> virtio_init(). So we should not use it to set "start_on_kick" at that
> point. This patch set "start_on_kick" on virtio_set_features() instead.
> 
> Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> ---

Maybe add a Fixes: badaf79cfdbd3 ?

>  hw/virtio/virtio.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 4805727b53..fc8fca81ad 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1214,8 +1214,7 @@ 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->start_on_kick = false;
>      vdev->started = false;
>      vdev->broken = false;
>      vdev->guest_features = 0;
> @@ -2069,14 +2068,21 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
>          return -EINVAL;
>      }
>      ret = virtio_set_features_nocheck(vdev, val);
> -    if (!ret && virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
> -        int i;
> -        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -            if (vdev->vq[i].vring.num != 0) {
> -                virtio_init_region_cache(vdev, i);
> +    if (!ret) {
> +        if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> +            /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
> +            int i;
> +            for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +                if (vdev->vq[i].vring.num != 0) {
> +                    virtio_init_region_cache(vdev, i);
> +                }
>              }
>          }
> +
> +        if (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +            vdev->start_on_kick = true;
> +        }
>      }
>      return ret;
>  }
> @@ -2331,8 +2337,7 @@ 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->start_on_kick = false;

virtio_init() is called at realize on an object that was just allocated with
g_malloc0(). You shouldn't need to set anything to 0 or false... or I'm
missing something ?

Anyway, I guess this doesn't hurt, so:

Reviewed-by: Greg Kurz <groug@kaod.org>

>      vdev->started = false;
>      vdev->device_id = device_id;
>      vdev->status = 0;

Re: [Qemu-devel] [PATCH 1/5] virtio: Set "start_on_kick" on virtio_set_features()

[Yongji Xie]

On Tue, 4 Jun 2019 at 00:53, Greg Kurz <groug@kaod.org> wrote:
>
> On Wed, 29 May 2019 15:09:51 +0800
> elohimes@gmail.com wrote:
>
> > From: Xie Yongji <xieyongji@baidu.com>
> >
> > The guest feature is not set correctly on virtio_reset() and
> > virtio_init(). So we should not use it to set "start_on_kick" at that
> > point. This patch set "start_on_kick" on virtio_set_features() instead.
> >
> > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > ---
>
> Maybe add a Fixes: badaf79cfdbd3 ?
>

Will add it in v2. Thanks for review.

Thanks,
Yongji

[Qemu-devel] [PATCH 2/5] virtio: Migrate the "start_on_kick" flag

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

We should migrate the "start_on_kick" flag so that we
would not miss starting device on kicking at startup
after migration.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/virtio/virtio.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index fc8fca81ad..4d4ff67791 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1802,6 +1802,13 @@ static bool virtio_started_needed(void *opaque)
     return vdev->started;
 }
 
+static bool virtio_start_on_kick_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    return vdev->start_on_kick;
+}
+
 static const VMStateDescription vmstate_virtqueue = {
     .name = "virtqueue_state",
     .version_id = 1,
@@ -1941,6 +1948,17 @@ static const VMStateDescription vmstate_virtio_started = {
     }
 };
 
+static const VMStateDescription vmstate_virtio_start_on_kick = {
+    .name = "virtio/start_on_kick",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_start_on_kick_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(start_on_kick, VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio = {
     .name = "virtio",
     .version_id = 1,
@@ -1957,6 +1975,7 @@ static const VMStateDescription vmstate_virtio = {
         &vmstate_virtio_broken,
         &vmstate_virtio_extra_state,
         &vmstate_virtio_started,
+        &vmstate_virtio_start_on_kick,
         NULL
     }
 };
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/5] virtio: Migrate the "start_on_kick" flag

[Greg Kurz]

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

> From: Xie Yongji <xieyongji@baidu.com>
> 
> We should migrate the "start_on_kick" flag so that we
> would not miss starting device on kicking at startup
> after migration.
> 

Hmm... IIUC "start_on_kick" means "virtio 1.0 transitional device that has
not been started yet", ie:

!vdev->started &&
(virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
 !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1))

If so, not sure why you need this extra field in the first place, but
you probably don't need to migrate it. Just recalculate in a post load
callback.

> Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> ---
>  hw/virtio/virtio.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index fc8fca81ad..4d4ff67791 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1802,6 +1802,13 @@ static bool virtio_started_needed(void *opaque)
>      return vdev->started;
>  }
>  
> +static bool virtio_start_on_kick_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return vdev->start_on_kick;
> +}
> +
>  static const VMStateDescription vmstate_virtqueue = {
>      .name = "virtqueue_state",
>      .version_id = 1,
> @@ -1941,6 +1948,17 @@ static const VMStateDescription vmstate_virtio_started = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_virtio_start_on_kick = {
> +    .name = "virtio/start_on_kick",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_start_on_kick_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(start_on_kick, VirtIODevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio = {
>      .name = "virtio",
>      .version_id = 1,
> @@ -1957,6 +1975,7 @@ static const VMStateDescription vmstate_virtio = {
>          &vmstate_virtio_broken,
>          &vmstate_virtio_extra_state,
>          &vmstate_virtio_started,
> +        &vmstate_virtio_start_on_kick,
>          NULL
>      }
>  };

Re: [Qemu-devel] [PATCH 2/5] virtio: Migrate the "start_on_kick" flag

[Michael S. Tsirkin]

On Mon, Jun 03, 2019 at 10:16:39PM +0200, Greg Kurz wrote:
> On Wed, 29 May 2019 15:09:52 +0800
> elohimes@gmail.com wrote:
> 
> > From: Xie Yongji <xieyongji@baidu.com>
> > 
> > We should migrate the "start_on_kick" flag so that we
> > would not miss starting device on kicking at startup
> > after migration.
> > 
> 
> Hmm... IIUC "start_on_kick" means "virtio 1.0 transitional device that has
> not been started yet", ie:
> 
> !vdev->started &&
> (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
>  !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1))

Or a legacy device I guess?

> If so, not sure why you need this extra field in the first place, but
> you probably don't need to migrate it. Just recalculate in a post load
> callback.
> 
> > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > ---
> >  hw/virtio/virtio.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index fc8fca81ad..4d4ff67791 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1802,6 +1802,13 @@ static bool virtio_started_needed(void *opaque)
> >      return vdev->started;
> >  }
> >  
> > +static bool virtio_start_on_kick_needed(void *opaque)
> > +{
> > +    VirtIODevice *vdev = opaque;
> > +
> > +    return vdev->start_on_kick;
> > +}
> > +
> >  static const VMStateDescription vmstate_virtqueue = {
> >      .name = "virtqueue_state",
> >      .version_id = 1,
> > @@ -1941,6 +1948,17 @@ static const VMStateDescription vmstate_virtio_started = {
> >      }
> >  };
> >  
> > +static const VMStateDescription vmstate_virtio_start_on_kick = {
> > +    .name = "virtio/start_on_kick",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = &virtio_start_on_kick_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_BOOL(start_on_kick, VirtIODevice),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static const VMStateDescription vmstate_virtio = {
> >      .name = "virtio",
> >      .version_id = 1,
> > @@ -1957,6 +1975,7 @@ static const VMStateDescription vmstate_virtio = {
> >          &vmstate_virtio_broken,
> >          &vmstate_virtio_extra_state,
> >          &vmstate_virtio_started,
> > +        &vmstate_virtio_start_on_kick,
> >          NULL
> >      }
> >  };

Re: [Qemu-devel] [PATCH 2/5] virtio: Migrate the "start_on_kick" flag

[Greg Kurz]

On Mon, 3 Jun 2019 17:03:06 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jun 03, 2019 at 10:16:39PM +0200, Greg Kurz wrote:
> > On Wed, 29 May 2019 15:09:52 +0800
> > elohimes@gmail.com wrote:
> >   
> > > From: Xie Yongji <xieyongji@baidu.com>
> > > 
> > > We should migrate the "start_on_kick" flag so that we
> > > would not miss starting device on kicking at startup
> > > after migration.
> > >   
> > 
> > Hmm... IIUC "start_on_kick" means "virtio 1.0 transitional device that has
> > not been started yet", ie:
> > 
> > !vdev->started &&
> > (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> >  !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1))  
> 
> Or a legacy device I guess?
> 

Do you mean "start_on_kick" should be set for both legacy
and virtio 1.0 transitional devices ?

> > If so, not sure why you need this extra field in the first place, but
> > you probably don't need to migrate it. Just recalculate in a post load
> > callback.
> >   
> > > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > > ---
> > >  hw/virtio/virtio.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index fc8fca81ad..4d4ff67791 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1802,6 +1802,13 @@ static bool virtio_started_needed(void *opaque)
> > >      return vdev->started;
> > >  }
> > >  
> > > +static bool virtio_start_on_kick_needed(void *opaque)
> > > +{
> > > +    VirtIODevice *vdev = opaque;
> > > +
> > > +    return vdev->start_on_kick;
> > > +}
> > > +
> > >  static const VMStateDescription vmstate_virtqueue = {
> > >      .name = "virtqueue_state",
> > >      .version_id = 1,
> > > @@ -1941,6 +1948,17 @@ static const VMStateDescription vmstate_virtio_started = {
> > >      }
> > >  };
> > >  
> > > +static const VMStateDescription vmstate_virtio_start_on_kick = {
> > > +    .name = "virtio/start_on_kick",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = &virtio_start_on_kick_needed,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_BOOL(start_on_kick, VirtIODevice),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >  static const VMStateDescription vmstate_virtio = {
> > >      .name = "virtio",
> > >      .version_id = 1,
> > > @@ -1957,6 +1975,7 @@ static const VMStateDescription vmstate_virtio = {
> > >          &vmstate_virtio_broken,
> > >          &vmstate_virtio_extra_state,
> > >          &vmstate_virtio_started,
> > > +        &vmstate_virtio_start_on_kick,
> > >          NULL
> > >      }
> > >  };  

Re: [Qemu-devel] [PATCH 2/5] virtio: Migrate the "start_on_kick" flag

[Michael S. Tsirkin]

On Mon, Jun 03, 2019 at 11:25:23PM +0200, Greg Kurz wrote:
> On Mon, 3 Jun 2019 17:03:06 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jun 03, 2019 at 10:16:39PM +0200, Greg Kurz wrote:
> > > On Wed, 29 May 2019 15:09:52 +0800
> > > elohimes@gmail.com wrote:
> > >   
> > > > From: Xie Yongji <xieyongji@baidu.com>
> > > > 
> > > > We should migrate the "start_on_kick" flag so that we
> > > > would not miss starting device on kicking at startup
> > > > after migration.
> > > >   
> > > 
> > > Hmm... IIUC "start_on_kick" means "virtio 1.0 transitional device that has
> > > not been started yet", ie:
> > > 
> > > !vdev->started &&
> > > (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> > >  !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1))  
> > 
> > Or a legacy device I guess?
> > 
> 
> Do you mean "start_on_kick" should be set for both legacy
> and virtio 1.0 transitional devices ?

Yes - generally when virtio 1 feature has not
been negotiated.

> > > If so, not sure why you need this extra field in the first place, but
> > > you probably don't need to migrate it. Just recalculate in a post load
> > > callback.
> > >   
> > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index fc8fca81ad..4d4ff67791 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1802,6 +1802,13 @@ static bool virtio_started_needed(void *opaque)
> > > >      return vdev->started;
> > > >  }
> > > >  
> > > > +static bool virtio_start_on_kick_needed(void *opaque)
> > > > +{
> > > > +    VirtIODevice *vdev = opaque;
> > > > +
> > > > +    return vdev->start_on_kick;
> > > > +}
> > > > +
> > > >  static const VMStateDescription vmstate_virtqueue = {
> > > >      .name = "virtqueue_state",
> > > >      .version_id = 1,
> > > > @@ -1941,6 +1948,17 @@ static const VMStateDescription vmstate_virtio_started = {
> > > >      }
> > > >  };
> > > >  
> > > > +static const VMStateDescription vmstate_virtio_start_on_kick = {
> > > > +    .name = "virtio/start_on_kick",
> > > > +    .version_id = 1,
> > > > +    .minimum_version_id = 1,
> > > > +    .needed = &virtio_start_on_kick_needed,
> > > > +    .fields = (VMStateField[]) {
> > > > +        VMSTATE_BOOL(start_on_kick, VirtIODevice),
> > > > +        VMSTATE_END_OF_LIST()
> > > > +    }
> > > > +};
> > > > +
> > > >  static const VMStateDescription vmstate_virtio = {
> > > >      .name = "virtio",
> > > >      .version_id = 1,
> > > > @@ -1957,6 +1975,7 @@ static const VMStateDescription vmstate_virtio = {
> > > >          &vmstate_virtio_broken,
> > > >          &vmstate_virtio_extra_state,
> > > >          &vmstate_virtio_started,
> > > > +        &vmstate_virtio_start_on_kick,
> > > >          NULL
> > > >      }
> > > >  };  

Re: [Qemu-devel] [PATCH 2/5] virtio: Migrate the "start_on_kick" flag

[Yongji Xie]

On Tue, 4 Jun 2019 at 04:16, Greg Kurz <groug@kaod.org> wrote:
>
> On Wed, 29 May 2019 15:09:52 +0800
> elohimes@gmail.com wrote:
>
> > From: Xie Yongji <xieyongji@baidu.com>
> >
> > We should migrate the "start_on_kick" flag so that we
> > would not miss starting device on kicking at startup
> > after migration.
> >
>
> Hmm... IIUC "start_on_kick" means "virtio 1.0 transitional device that has
> not been started yet", ie:
>
> !vdev->started &&
> (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
>  !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1))
>
> If so, not sure why you need this extra field in the first place, but
> you probably don't need to migrate it. Just recalculate in a post load
> callback.
>

Good idea! Will recalculate this in virtio_load() in v2. Thank you.

Thanks,
Yongji

[Qemu-devel] [PATCH 3/5] virtio: Make sure we get correct state of device on handle_aio_output()

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

We should set the flags: "start_on_kick" and "started" after we call
the kick functions (handle_aio_output() and handle_output()).

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/virtio/virtio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4d4ff67791..8a81374492 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1575,11 +1575,11 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
         event_notifier_set(&vq->host_notifier);
     } else if (vq->handle_output) {
         vq->handle_output(vdev, vq);
-    }
 
-    if (unlikely(vdev->start_on_kick)) {
-        vdev->started = true;
-        vdev->start_on_kick = false;
+        if (unlikely(vdev->start_on_kick)) {
+            vdev->started = true;
+            vdev->start_on_kick = false;
+        }
     }
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 4/5] virtio: Don't change "started" flag on virtio_vmstate_change()

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

We will call virtio_set_status() on virtio_vmstate_change().
The "started" flag should not be changed in this case. Otherwise,
we may get an incorrect value when we set "started" flag but
not set DRIVER_OK in source VM.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/virtio/virtio.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8a81374492..9e17293d46 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1162,9 +1162,13 @@ 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 ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) !=
+        (val & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        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) {
-- 
2.17.1

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

[elohimes]

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;
     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)) {
         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;
 }
 
 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
-- 
2.17.1

Re: [Qemu-devel] [PATCH 5/5] virtio: add "use-started" property

[Greg Kurz]

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

Re: [Qemu-devel] [PATCH 5/5] virtio: add "use-started" property

[Yongji Xie]

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