| Message ID | 20190529070955.25565-3-xieyongji@baidu.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: fix some issues of "started" and "start_on_kick" flag | expand |
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 > } > };
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 > > } > > };
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 > > > } > > > };
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 > > > > } > > > > };
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
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 } };