diff mbox series

[2/5] virtio: Migrate the "start_on_kick" flag

Message ID 20190529070955.25565-3-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 May 29, 2019, 7:09 a.m. UTC 
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.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/virtio/virtio.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Greg Kurz June 3, 2019, 8:16 p.m. UTC | #1 
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
>      }
>  };
Michael S. Tsirkin June 3, 2019, 9:03 p.m. UTC | #2 
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
> >      }
> >  };
Greg Kurz June 3, 2019, 9:25 p.m. UTC | #3 
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
> > >      }
> > >  };
Michael S. Tsirkin June 3, 2019, 9:51 p.m. UTC | #4 
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
> > > >      }
> > > >  };
Yongji Xie June 4, 2019, 2:15 a.m. UTC | #5 
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 mbox series

Patch

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
     }
 };