| Message ID | 20190614093121.5580-2-xieyongji@baidu.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: fix some issues of "started" and "start_on_kick" flag | expand |
On Fri, 14 Jun 2019 17:31:17 +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. > > Suggested-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > --- > hw/block/vhost-user-blk.c | 4 ++-- > hw/core/machine.c | 8 ++++++-- This patch conflicts with latest upstream changes to hw_compat_4_0_1[]. It seems you need to rebase. Also, I'm still not sure how we're supposed to handle hw_compat_4_0_1[] versus hw_compat_4_0[]... nobody commented on: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00637.html https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00641.html Maybe worth to sort that out before re-posting. > hw/virtio/virtio.c | 18 +++++++----------- > include/hw/virtio/virtio.h | 21 +++++++++++++++++++++ > 4 files changed, 36 insertions(+), 15 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..60d1e27d2c 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -24,10 +24,14 @@ > #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[] = {}; > +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 07f4a64b48..19062fbb96 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1162,10 +1162,8 @@ 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; > - } > + > + virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK); > > if (k->set_status) { > k->set_status(vdev, val); > @@ -1536,8 +1534,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); > } > } > > @@ -1557,8 +1554,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); > } > } > } > @@ -1579,8 +1575,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n) > } > > if (unlikely(vdev->start_on_kick)) { > - vdev->started = true; > - vdev->start_on_kick = false; > + virtio_set_started(vdev, true); > } > } > > @@ -2291,7 +2286,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) { > @@ -2669,6 +2664,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..15d5366939 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,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
On Fri, 14 Jun 2019 at 19:45, Greg Kurz <groug@kaod.org> wrote: > > On Fri, 14 Jun 2019 17:31:17 +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. > > > > Suggested-by: Greg Kurz <groug@kaod.org> > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > > --- > > hw/block/vhost-user-blk.c | 4 ++-- > > hw/core/machine.c | 8 ++++++-- > > This patch conflicts with latest upstream changes to hw_compat_4_0_1[]. > > It seems you need to rebase. Also, I'm still not sure how we're supposed > to handle hw_compat_4_0_1[] versus hw_compat_4_0[]... nobody commented > on: > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00637.html > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00641.html > > Maybe worth to sort that out before re-posting. > If hw_compat_4_0_1[] is introduced only for q35, I think this patch should be OK. If not, maybe we should handle hw_compat_4_0_1[] in other machine types (i440fx, arm, ppc, s390)? Hi Alex and Paolo, Any comment for this? Thanks, Yongji
On Mon, 17 Jun 2019 10:14:30 +0800 Yongji Xie <elohimes@gmail.com> wrote: > On Fri, 14 Jun 2019 at 19:45, Greg Kurz <groug@kaod.org> wrote: > > > > On Fri, 14 Jun 2019 17:31:17 +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. > > > > > > Suggested-by: Greg Kurz <groug@kaod.org> > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > > > --- > > > hw/block/vhost-user-blk.c | 4 ++-- > > > hw/core/machine.c | 8 ++++++-- > > > > This patch conflicts with latest upstream changes to hw_compat_4_0_1[]. > > > > It seems you need to rebase. Also, I'm still not sure how we're supposed > > to handle hw_compat_4_0_1[] versus hw_compat_4_0[]... nobody commented > > on: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00637.html > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00641.html > > > > Maybe worth to sort that out before re-posting. > > > > If hw_compat_4_0_1[] is introduced only for q35, I think this patch > should be OK. If not, maybe we should handle hw_compat_4_0_1[] in > other machine types (i440fx, arm, ppc, s390)? > It turns out that hw_compat_4_0_1[] isn't needed at all. Please see: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg03054.html > Hi Alex and Paolo, > > Any comment for this? > > Thanks, > Yongji
On Mon, 17 Jun 2019 at 13:24, Greg Kurz <groug@kaod.org> wrote: > > On Mon, 17 Jun 2019 10:14:30 +0800 > Yongji Xie <elohimes@gmail.com> wrote: > > > On Fri, 14 Jun 2019 at 19:45, Greg Kurz <groug@kaod.org> wrote: > > > > > > On Fri, 14 Jun 2019 17:31:17 +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. > > > > > > > > Suggested-by: Greg Kurz <groug@kaod.org> > > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > > > > --- > > > > hw/block/vhost-user-blk.c | 4 ++-- > > > > hw/core/machine.c | 8 ++++++-- > > > > > > This patch conflicts with latest upstream changes to hw_compat_4_0_1[]. > > > > > > It seems you need to rebase. Also, I'm still not sure how we're supposed > > > to handle hw_compat_4_0_1[] versus hw_compat_4_0[]... nobody commented > > > on: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00637.html > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00641.html > > > > > > Maybe worth to sort that out before re-posting. > > > > > > > If hw_compat_4_0_1[] is introduced only for q35, I think this patch > > should be OK. If not, maybe we should handle hw_compat_4_0_1[] in > > other machine types (i440fx, arm, ppc, s390)? > > > > It turns out that hw_compat_4_0_1[] isn't needed at all. Please see: > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg03054.html > Oh, great! I will rebase my patch after this commit is merged. Thanks, Yongji
On Mon, 17 Jun 2019 14:04:10 +0800 Yongji Xie <elohimes@gmail.com> wrote: > On Mon, 17 Jun 2019 at 13:24, Greg Kurz <groug@kaod.org> wrote: > > > > On Mon, 17 Jun 2019 10:14:30 +0800 > > Yongji Xie <elohimes@gmail.com> wrote: > > > > > On Fri, 14 Jun 2019 at 19:45, Greg Kurz <groug@kaod.org> wrote: > > > > > > > > On Fri, 14 Jun 2019 17:31:17 +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. > > > > > > > > > > Suggested-by: Greg Kurz <groug@kaod.org> > > > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > > > > > --- > > > > > hw/block/vhost-user-blk.c | 4 ++-- > > > > > hw/core/machine.c | 8 ++++++-- > > > > > > > > This patch conflicts with latest upstream changes to hw_compat_4_0_1[]. > > > > > > > > It seems you need to rebase. Also, I'm still not sure how we're supposed > > > > to handle hw_compat_4_0_1[] versus hw_compat_4_0[]... nobody commented > > > > on: > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00637.html > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00641.html > > > > > > > > Maybe worth to sort that out before re-posting. > > > > > > > > > > If hw_compat_4_0_1[] is introduced only for q35, I think this patch > > > should be OK. If not, maybe we should handle hw_compat_4_0_1[] in > > > other machine types (i440fx, arm, ppc, s390)? > > > > > > > It turns out that hw_compat_4_0_1[] isn't needed at all. Please see: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg03054.html > > > > Oh, great! I will rebase my patch after this commit is merged. > > Thanks, > Yongji You can proceed. https://git.qemu.org/?p=qemu.git;a=commit;h=8e8cbed09ad9d577955691b4c061b61b602406d1 Cheers, -- Greg
On Sat, 22 Jun 2019 at 23:51, Greg Kurz <groug@kaod.org> wrote: > > On Mon, 17 Jun 2019 14:04:10 +0800 > Yongji Xie <elohimes@gmail.com> wrote: > > > On Mon, 17 Jun 2019 at 13:24, Greg Kurz <groug@kaod.org> wrote: > > > > > > On Mon, 17 Jun 2019 10:14:30 +0800 > > > Yongji Xie <elohimes@gmail.com> wrote: > > > > > > > On Fri, 14 Jun 2019 at 19:45, Greg Kurz <groug@kaod.org> wrote: > > > > > > > > > > On Fri, 14 Jun 2019 17:31:17 +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. > > > > > > > > > > > > Suggested-by: Greg Kurz <groug@kaod.org> > > > > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > > > > > > --- > > > > > > hw/block/vhost-user-blk.c | 4 ++-- > > > > > > hw/core/machine.c | 8 ++++++-- > > > > > > > > > > This patch conflicts with latest upstream changes to hw_compat_4_0_1[]. > > > > > > > > > > It seems you need to rebase. Also, I'm still not sure how we're supposed > > > > > to handle hw_compat_4_0_1[] versus hw_compat_4_0[]... nobody commented > > > > > on: > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00637.html > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00641.html > > > > > > > > > > Maybe worth to sort that out before re-posting. > > > > > > > > > > > > > If hw_compat_4_0_1[] is introduced only for q35, I think this patch > > > > should be OK. If not, maybe we should handle hw_compat_4_0_1[] in > > > > other machine types (i440fx, arm, ppc, s390)? > > > > > > > > > > It turns out that hw_compat_4_0_1[] isn't needed at all. Please see: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg03054.html > > > > > > > Oh, great! I will rebase my patch after this commit is merged. > > > > Thanks, > > Yongji > > You can proceed. > > https://git.qemu.org/?p=qemu.git;a=commit;h=8e8cbed09ad9d577955691b4c061b61b602406d1 > OK, Thank you. Thanks, Yongji
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..60d1e27d2c 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -24,10 +24,14 @@ #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[] = {}; +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 07f4a64b48..19062fbb96 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1162,10 +1162,8 @@ 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; - } + + virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK); if (k->set_status) { k->set_status(vdev, val); @@ -1536,8 +1534,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); } } @@ -1557,8 +1554,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); } } } @@ -1579,8 +1575,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n) } if (unlikely(vdev->start_on_kick)) { - vdev->started = true; - vdev->start_on_kick = false; + virtio_set_started(vdev, true); } } @@ -2291,7 +2286,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) { @@ -2669,6 +2664,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..15d5366939 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,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