[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] virtio: add "use-started" property
From: |
Yongji Xie |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] virtio: add "use-started" property |
Date: |
Tue, 4 Jun 2019 10:37:15 +0800 |
On Tue, 4 Jun 2019 at 04:49, Greg Kurz <address@hidden> wrote:
>
> On Wed, 29 May 2019 15:09:55 +0800
> address@hidden wrote:
>
> > From: Xie Yongji <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Xie Yongji <address@hidden>
> > ---
> > 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