diff mbox series

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

Message ID 20190604073459.15651-6-xieyongji@baidu.com (mailing list archive)
State New, archived
Headers show
Series virtio: fix some issues of "started" and "start_on_kick" flag | expand

Commit Message

Yongji Xie June 4, 2019, 7:34 a.m. UTC 
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(-)

Comments

Greg Kurz June 5, 2019, 8:59 a.m. UTC | #1 
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
Yongji Xie June 5, 2019, 9:14 a.m. UTC | #2 
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
diff mbox series

Patch

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