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

[Qemu-devel] [PATCH v2 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 set "start_on_kick" flag for legacy devices.

The patch 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

v2:
- Recalculate "start_on_kick" flag after migration instead of migrating
  it
- Set "start_on_kick" flag for legacy devices
- Avoid setting "started" flag when "use_started" property is true
- Set "use_started" to false by hw_compat_4_0_1 instead of hw_compat_4_0

Xie Yongji (5):
  virtio: Set "start_on_kick" on virtio_set_features()
  virtio: Set "start_on_kick" for legacy devices
  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  |  4 +--
 hw/core/machine.c          |  4 ++-
 hw/virtio/virtio.c         | 53 ++++++++++++++++++++++----------------
 include/hw/virtio/virtio.h | 23 ++++++++++++++++-
 4 files changed, 58 insertions(+), 26 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 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.

Fixes: badaf79cfdbd3 ("virtio: Introduce started flag to VirtioDevice")
Signed-off-by: Xie Yongji <xieyongji@baidu.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/virtio/virtio.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 07f4a64b48..6508b2faad 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,22 @@ 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 (!vdev->started &&
+            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;
 }
@@ -2228,6 +2235,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         }
     }
 
+    if (!vdev->started &&
+        virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
+        !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        vdev->start_on_kick = true;
+    }
+
     rcu_read_lock();
     for (i = 0; i < num; i++) {
         if (vdev->vq[i].vring.desc) {
@@ -2330,8 +2343,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

[Qemu-devel] [PATCH v2 2/5] virtio: Set "start_on_kick" for legacy devices

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

Besides virtio 1.0 transitional devices, we should also
set "start_on_kick" flag for legacy devices (virtio 0.9).

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/virtio/virtio.c         | 2 --
 include/hw/virtio/virtio.h | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6508b2faad..6ec45d8f0a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2080,7 +2080,6 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
         }
 
         if (!vdev->started &&
-            virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
             !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
             vdev->start_on_kick = true;
         }
@@ -2236,7 +2235,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     }
 
     if (!vdev->started &&
-        virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
         !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         vdev->start_on_kick = true;
     }
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 27c0efc3d0..303242b3c2 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -106,7 +106,7 @@ struct VirtIODevice
     bool vm_running;
     bool broken; /* device in invalid state, needs reset */
     bool started;
-    bool start_on_kick; /* virtio 1.0 transitional devices support that */
+    bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/5] virtio: Set "start_on_kick" for legacy devices

[Greg Kurz]

On Tue,  4 Jun 2019 15:34:56 +0800
elohimes@gmail.com wrote:

> From: Xie Yongji <xieyongji@baidu.com>
> 
> Besides virtio 1.0 transitional devices, we should also
> set "start_on_kick" flag for legacy devices (virtio 0.9).
> 
> Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> ---

Patch looks good but it would be even better if applied
earlier so that it doesn't revert lines added by the
previous patch...

>  hw/virtio/virtio.c         | 2 --
>  include/hw/virtio/virtio.h | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 6508b2faad..6ec45d8f0a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2080,7 +2080,6 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
>          }
>  
>          if (!vdev->started &&
> -            virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&

... here and...
>              !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>              vdev->start_on_kick = true;
>          }
> @@ -2236,7 +2235,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>      }
>  
>      if (!vdev->started &&
> -        virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&

... here.

>          !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>          vdev->start_on_kick = true;
>      }
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 27c0efc3d0..303242b3c2 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -106,7 +106,7 @@ struct VirtIODevice
>      bool vm_running;
>      bool broken; /* device in invalid state, needs reset */
>      bool started;
> -    bool start_on_kick; /* virtio 1.0 transitional devices support that */
> +    bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
>      uint8_t device_endian;

Re: [Qemu-devel] [PATCH v2 2/5] virtio: Set "start_on_kick" for legacy devices

[Yongji Xie]

On Wed, 5 Jun 2019 at 14:42, Greg Kurz <groug@kaod.org> wrote:
>
> On Tue,  4 Jun 2019 15:34:56 +0800
> elohimes@gmail.com wrote:
>
> > From: Xie Yongji <xieyongji@baidu.com>
> >
> > Besides virtio 1.0 transitional devices, we should also
> > set "start_on_kick" flag for legacy devices (virtio 0.9).
> >
> > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > ---
>
> Patch looks good but it would be even better if applied
> earlier so that it doesn't revert lines added by the
> previous patch...
>

Fine with me. Will do it in v3.

Thanks,
Yongji

Re: [Qemu-devel] [PATCH v2 2/5] virtio: Set "start_on_kick" for legacy devices

[Greg Kurz]

On Wed, 5 Jun 2019 14:49:34 +0800
Yongji Xie <elohimes@gmail.com> wrote:

> On Wed, 5 Jun 2019 at 14:42, Greg Kurz <groug@kaod.org> wrote:
> >
> > On Tue,  4 Jun 2019 15:34:56 +0800
> > elohimes@gmail.com wrote:
> >  
> > > From: Xie Yongji <xieyongji@baidu.com>
> > >
> > > Besides virtio 1.0 transitional devices, we should also
> > > set "start_on_kick" flag for legacy devices (virtio 0.9).
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > > ---  
> >
> > Patch looks good but it would be even better if applied
> > earlier so that it doesn't revert lines added by the
> > previous patch...
> >  
> 
> Fine with me. Will do it in v3.
> 

Hold on before posting, I've just learned about hw_compat_4_0_1 while
reviewing patch 5... need so more time to understand the impact.

Cheers,

--
Greg

> Thanks,
> Yongji

Re: [Qemu-devel] [PATCH v2 2/5] virtio: Set "start_on_kick" for legacy devices

[Yongji Xie]

On Wed, 5 Jun 2019 at 15:14, Greg Kurz <groug@kaod.org> wrote:
>
> On Wed, 5 Jun 2019 14:49:34 +0800
> Yongji Xie <elohimes@gmail.com> wrote:
>
> > On Wed, 5 Jun 2019 at 14:42, Greg Kurz <groug@kaod.org> wrote:
> > >
> > > On Tue,  4 Jun 2019 15:34:56 +0800
> > > elohimes@gmail.com wrote:
> > >
> > > > From: Xie Yongji <xieyongji@baidu.com>
> > > >
> > > > Besides virtio 1.0 transitional devices, we should also
> > > > set "start_on_kick" flag for legacy devices (virtio 0.9).
> > > >
> > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > > > ---
> > >
> > > Patch looks good but it would be even better if applied
> > > earlier so that it doesn't revert lines added by the
> > > previous patch...
> > >
> >
> > Fine with me. Will do it in v3.
> >
>
> Hold on before posting, I've just learned about hw_compat_4_0_1 while
> reviewing patch 5... need so more time to understand the impact.
>

Sure.

Thanks,
Yongji

[Qemu-devel] [PATCH v2 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 6ec45d8f0a..3dc2f8c223 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 v2 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 3dc2f8c223..3960619bd4 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 v2 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 use
"started" flag or not. This property will be true by default and
set to false when machine type <= 4.0.1.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/block/vhost-user-blk.c  |  4 ++--
 hw/core/machine.c          |  4 +++-
 hw/virtio/virtio.c         | 21 ++++++++-------------
 include/hw/virtio/virtio.h | 21 +++++++++++++++++++++
 4 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9cb61336a6..85bc4017e7 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -191,7 +191,7 @@ 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 = virtio_device_started(vdev, status);
     int ret;
 
     if (!vdev->vm_running) {
@@ -317,7 +317,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
     }
 
     /* restore vhost state */
-    if (vdev->started) {
+    if (virtio_device_started(vdev, vdev->status)) {
         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 f1a0f45f9c..133c113ebf 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_1[] = {};
+GlobalProperty hw_compat_4_0_1[] = {
+    { "virtio-device", "use-started", "false" },
+};
 const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
 
 GlobalProperty hw_compat_4_0[] = {};
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3960619bd4..9af2e339af 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1165,10 +1165,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
 
     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;
-        }
+        virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
     }
 
     if (k->set_status) {
@@ -1539,8 +1536,7 @@ static bool virtio_queue_notify_aio_vq(VirtQueue *vq)
         ret = vq->handle_aio_output(vdev, vq);
 
         if (unlikely(vdev->start_on_kick)) {
-            vdev->started = true;
-            vdev->start_on_kick = false;
+            virtio_set_started(vdev, true);
         }
     }
 
@@ -1560,8 +1556,7 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
         vq->handle_output(vdev, vq);
 
         if (unlikely(vdev->start_on_kick)) {
-            vdev->started = true;
-            vdev->start_on_kick = false;
+            virtio_set_started(vdev, true);
         }
     }
 }
@@ -1581,8 +1576,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
         vq->handle_output(vdev, vq);
 
         if (unlikely(vdev->start_on_kick)) {
-            vdev->started = true;
-            vdev->start_on_kick = false;
+            virtio_set_started(vdev, true);
         }
     }
 }
@@ -2083,7 +2077,7 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
             }
         }
 
-        if (!vdev->started &&
+        if (!virtio_device_started(vdev, vdev->status) &&
             !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
             vdev->start_on_kick = true;
         }
@@ -2238,7 +2232,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         }
     }
 
-    if (!vdev->started &&
+    if (!virtio_device_started(vdev, vdev->status) &&
         !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         vdev->start_on_kick = true;
     }
@@ -2306,7 +2300,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->status);
     vdev->vm_running = running;
 
     if (backend_run) {
@@ -2683,6 +2677,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 303242b3c2..b189788cb2 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; /* when virtio 1.0 feature has not been negotiated */
     VMChangeStateEntry *vmstate;
@@ -351,4 +352,24 @@ 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, uint8_t status)
+{
+    if (vdev->use_started) {
+        return vdev->started;
+    }
+
+    return status & VIRTIO_CONFIG_S_DRIVER_OK;
+}
+
+static inline void virtio_set_started(VirtIODevice *vdev, bool started)
+{
+    if (started) {
+        vdev->start_on_kick = false;
+    }
+
+    if (vdev->use_started) {
+        vdev->started = started;
+    }
+}
 #endif
-- 
2.17.1

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

[Greg Kurz]

On Tue,  4 Jun 2019 15:34:59 +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 use
> "started" flag or not. This property will be true by default and
> set to false when machine type <= 4.0.1.
> 
> Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> ---
>  hw/block/vhost-user-blk.c  |  4 ++--
>  hw/core/machine.c          |  4 +++-
>  hw/virtio/virtio.c         | 21 ++++++++-------------
>  include/hw/virtio/virtio.h | 21 +++++++++++++++++++++
>  4 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9cb61336a6..85bc4017e7 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -191,7 +191,7 @@ 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 = virtio_device_started(vdev, status);
>      int ret;
>  
>      if (!vdev->vm_running) {
> @@ -317,7 +317,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
>      }
>  
>      /* restore vhost state */
> -    if (vdev->started) {
> +    if (virtio_device_started(vdev, vdev->status)) {
>          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 f1a0f45f9c..133c113ebf 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_1[] = {};
> +GlobalProperty hw_compat_4_0_1[] = {
> +    { "virtio-device", "use-started", "false" },
> +};
>  const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);

I'm discovering hw_compat_4_0_1, which seems to be only used by the
pc-q35-4.0.1 machine type...

>  
>  GlobalProperty hw_compat_4_0[] = {};

Not sure if it's the way to go but the same line should at least be added
here for all other machine types that use hw_compat_4_0[] eg. pseries-4.0
and older, which are the ones I need this fix for.

Cc'ing core machine code maintainers for advice.

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3960619bd4..9af2e339af 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1165,10 +1165,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>  
>      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;
> -        }
> +        virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);

virtio_set_started() takes a bool as second argument, so this should
rather be !!(val & VIRTIO_CONFIG_S_DRIVER_OK) to avoid potential
warnings from picky compilers.

The rest looks good, but I'm wondering if this patch should be the first
one in the series to narrow the range of commits where backward migration
is broken.

>      }
>  
>      if (k->set_status) {
> @@ -1539,8 +1536,7 @@ static bool virtio_queue_notify_aio_vq(VirtQueue *vq)
>          ret = vq->handle_aio_output(vdev, vq);
>  
>          if (unlikely(vdev->start_on_kick)) {
> -            vdev->started = true;
> -            vdev->start_on_kick = false;
> +            virtio_set_started(vdev, true);
>          }
>      }
>  
> @@ -1560,8 +1556,7 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
>          vq->handle_output(vdev, vq);
>  
>          if (unlikely(vdev->start_on_kick)) {
> -            vdev->started = true;
> -            vdev->start_on_kick = false;
> +            virtio_set_started(vdev, true);
>          }
>      }
>  }
> @@ -1581,8 +1576,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
>          vq->handle_output(vdev, vq);
>  
>          if (unlikely(vdev->start_on_kick)) {
> -            vdev->started = true;
> -            vdev->start_on_kick = false;
> +            virtio_set_started(vdev, true);
>          }
>      }
>  }
> @@ -2083,7 +2077,7 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
>              }
>          }
>  
> -        if (!vdev->started &&
> +        if (!virtio_device_started(vdev, vdev->status) &&
>              !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>              vdev->start_on_kick = true;
>          }
> @@ -2238,7 +2232,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          }
>      }
>  
> -    if (!vdev->started &&
> +    if (!virtio_device_started(vdev, vdev->status) &&
>          !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>          vdev->start_on_kick = true;
>      }
> @@ -2306,7 +2300,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->status);
>      vdev->vm_running = running;
>  
>      if (backend_run) {
> @@ -2683,6 +2677,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 303242b3c2..b189788cb2 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; /* when virtio 1.0 feature has not been negotiated */
>      VMChangeStateEntry *vmstate;
> @@ -351,4 +352,24 @@ 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, uint8_t status)
> +{
> +    if (vdev->use_started) {
> +        return vdev->started;
> +    }
> +
> +    return status & VIRTIO_CONFIG_S_DRIVER_OK;
> +}
> +
> +static inline void virtio_set_started(VirtIODevice *vdev, bool started)
> +{
> +    if (started) {
> +        vdev->start_on_kick = false;
> +    }
> +
> +    if (vdev->use_started) {
> +        vdev->started = started;
> +    }
> +}
>  #endif

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

[Yongji Xie]

On Wed, 5 Jun 2019 at 17:00, Greg Kurz <groug@kaod.org> wrote:
>
> On Tue,  4 Jun 2019 15:34:59 +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 use
> > "started" flag or not. This property will be true by default and
> > set to false when machine type <= 4.0.1.
> >
> > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > ---
> >  hw/block/vhost-user-blk.c  |  4 ++--
> >  hw/core/machine.c          |  4 +++-
> >  hw/virtio/virtio.c         | 21 ++++++++-------------
> >  include/hw/virtio/virtio.h | 21 +++++++++++++++++++++
> >  4 files changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 9cb61336a6..85bc4017e7 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -191,7 +191,7 @@ 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 = virtio_device_started(vdev, status);
> >      int ret;
> >
> >      if (!vdev->vm_running) {
> > @@ -317,7 +317,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
> >      }
> >
> >      /* restore vhost state */
> > -    if (vdev->started) {
> > +    if (virtio_device_started(vdev, vdev->status)) {
> >          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 f1a0f45f9c..133c113ebf 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_1[] = {};
> > +GlobalProperty hw_compat_4_0_1[] = {
> > +    { "virtio-device", "use-started", "false" },
> > +};
> >  const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
>
> I'm discovering hw_compat_4_0_1, which seems to be only used by the
> pc-q35-4.0.1 machine type...
>

Oops, my mistake.

> >
> >  GlobalProperty hw_compat_4_0[] = {};
>
> Not sure if it's the way to go but the same line should at least be added
> here for all other machine types that use hw_compat_4_0[] eg. pseries-4.0
> and older, which are the ones I need this fix for.
>

I agree.

> Cc'ing core machine code maintainers for advice.
>
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 3960619bd4..9af2e339af 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1165,10 +1165,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
> >
> >      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;
> > -        }
> > +        virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
>
> virtio_set_started() takes a bool as second argument, so this should
> rather be !!(val & VIRTIO_CONFIG_S_DRIVER_OK) to avoid potential
> warnings from picky compilers.
>

Will fix it in v3.

> The rest looks good, but I'm wondering if this patch should be the first
> one in the series to narrow the range of commits where backward migration
> is broken.
>

It's OK to me.

Thanks,
Yongji