[PATCH] virtio-net: Fix save/load

[PATCH] virtio-net: Fix save/load

[Alex Williamson]

We can't rely on build switches to tell us if a save image
includes a given field.  We also need to save status since
it's visible to the guest.  Draw another line in the sand
for broken save versions.  The version number should always
be updated when new values are saved and load should make
an attempt to set reasonable defaults for lower version save
images.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 qemu/hw/virtio-net.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
index 358c382..6ce2ff5 100644
--- a/qemu/hw/virtio-net.c
+++ b/qemu/hw/virtio-net.c
@@ -21,6 +21,8 @@
 
 #define TAP_VNET_HDR
 
+#define VIRTIO_NET_VM_VERSION    3
+
 typedef struct VirtIONet
 {
     VirtIODevice vdev;
@@ -368,6 +370,11 @@ static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }
 
+/*
+ * Anything added here should cause a bump in VIRTIO_NET_VM_VERSION
+ * and appropriate conditionalized load with sane defaults for older
+ * images should be added to virtio_net_load().
+ */
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
     VirtIONet *n = opaque;
@@ -380,14 +387,18 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
 
 #ifdef TAP_VNET_HDR
     qemu_put_be32(f, tap_has_vnet_hdr(n->vc->vlan->first_client));
+#else
+    qemu_put_be32(f, 0);
 #endif
+
+    qemu_put_be16(f, n->status);
 }
 
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
 
-    if (version_id != 2)
+    if (version_id < 3 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
     virtio_load(&n->vdev, f);
@@ -399,8 +410,12 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 #ifdef TAP_VNET_HDR
     if (qemu_get_be32(f))
         tap_using_vnet_hdr(n->vc->vlan->first_client, 1);
+#else
+    qemu_get_be32(f);
 #endif
 
+    n->status = qemu_get_be16(f);
+
     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
                        qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
@@ -439,7 +454,7 @@ PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
     n->tx_timer_active = 0;
     n->mergeable_rx_bufs = 0;
 
-    register_savevm("virtio-net", virtio_net_id++, 2,
+    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
                     virtio_net_save, virtio_net_load, n);
 
     return (PCIDevice *)n;

Re: [PATCH] virtio-net: Fix save/load

[Mark McLoughlin]

Hi Alex,

On Thu, 2009-01-15 at 11:05 -0700, Alex Williamson wrote:

> We can't rely on build switches to tell us if a save image
> includes a given field.

Uggh, good point.

> We also need to save status since it's visible to the guest.

Actually, we really need to handle VLANClientState:link_down for all
vlan clients, not just virtio-net and then update status on load
according to link_down.

It's not critically important, though - if we neglect to save/load this
the only side effect is that "set link down" would have to be called
again. Does need to be fixed, though.

>   Draw another line in the sand
> for broken save versions.  The version number should always
> be updated when new values are saved and load should make
> an attempt to set reasonable defaults for lower version save
> images.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  qemu/hw/virtio-net.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
> index 358c382..6ce2ff5 100644
> --- a/qemu/hw/virtio-net.c
> +++ b/qemu/hw/virtio-net.c
> @@ -21,6 +21,8 @@
>  
>  #define TAP_VNET_HDR
>  
> +#define VIRTIO_NET_VM_VERSION    3
> +
>  typedef struct VirtIONet
>  {
>      VirtIODevice vdev;
> @@ -368,6 +370,11 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
>  
> +/*
> + * Anything added here should cause a bump in VIRTIO_NET_VM_VERSION
> + * and appropriate conditionalized load with sane defaults for older
> + * images should be added to virtio_net_load().
> + */

This is true for all savevm() code, so I don't think we need the comment
here.

>  static void virtio_net_save(QEMUFile *f, void *opaque)
>  {
>      VirtIONet *n = opaque;
> @@ -380,14 +387,18 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>  
>  #ifdef TAP_VNET_HDR
>      qemu_put_be32(f, tap_has_vnet_hdr(n->vc->vlan->first_client));
> +#else
> +    qemu_put_be32(f, 0);
>  #endif
> +
> +    qemu_put_be16(f, n->status);
>  }
>  
>  static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
>  
> -    if (version_id != 2)
> +    if (version_id < 3 || version_id > VIRTIO_NET_VM_VERSION)

This bit isn't right - how can this code load e.g. version 4?
Cheers,
Mark.

Re: [PATCH] virtio-net: Fix save/load

[Alex Williamson]

Hi Mark,

On Thu, 2009-01-15 at 18:21 +0000, Mark McLoughlin wrote:
> Actually, we really need to handle VLANClientState:link_down for all
> vlan clients, not just virtio-net and then update status on load
> according to link_down.
> 
> It's not critically important, though - if we neglect to save/load this
> the only side effect is that "set link down" would have to be called
> again. Does need to be fixed, though.

The link status may be able to get away w/o a save/load, but what else
is going to get added to there later.  Seems like we might as well add
it now.

> > +/*
> > + * Anything added here should cause a bump in VIRTIO_NET_VM_VERSION
> > + * and appropriate conditionalized load with sane defaults for older
> > + * images should be added to virtio_net_load().
> > + */
> 
> This is true for all savevm() code, so I don't think we need the comment
> here.

Doesn't hurt and it's obviously easy to forget ;)

> >  static int virtio_net_load(QEMUFile *f, void *opaque, int
> version_id)
> >  {
> >      VirtIONet *n = opaque;
> >  
> > -    if (version_id != 2)
> > +    if (version_id < 3 || version_id > VIRTIO_NET_VM_VERSION)
> 
> This bit isn't right - how can this code load e.g. version 4?

It can't.  There's no way to load a save image for a version_id greater
than VIRTIO_NET_VM_VERSION, because we don't know how much data to pop
off or what state we'd be missing.  We can only load older save images
on equal or newer versions of qemu.  As far as know...  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH] virtio-net: Fix save/load

[Mark McLoughlin]

On Thu, 2009-01-15 at 11:39 -0700, Alex Williamson wrote:
> > version_id)
> > >  {
> > >      VirtIONet *n = opaque;
> > >  
> > > -    if (version_id != 2)
> > > +    if (version_id < 3 || version_id > VIRTIO_NET_VM_VERSION)
> > 
> > This bit isn't right - how can this code load e.g. version 4?
> 
> It can't.  There's no way to load a save image for a version_id
> greater
> than VIRTIO_NET_VM_VERSION, because we don't know how much data to pop
> off or what state we'd be missing.  We can only load older save images
> on equal or newer versions of qemu.  As far as know...  Thanks,

Yes, I read the code wrong :)

Cheers,
Mark.

Re: [PATCH] virtio-net: Fix save/load

[Avi Kivity]

Alex Williamson wrote:
> We can't rely on build switches to tell us if a save image
> includes a given field.  We also need to save status since
> it's visible to the guest.  Draw another line in the sand
> for broken save versions.  The version number should always
> be updated when new values are saved and load should make
> an attempt to set reasonable defaults for lower version save
> images.
>   

I applied the the first part of this ancient patch.  Can't tell from the 
thread what the consensus is for the rest.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] virtio-net: Fix save/load

[Anthony Liguori]

Avi Kivity wrote:
> Alex Williamson wrote:
>> We can't rely on build switches to tell us if a save image
>> includes a given field.  We also need to save status since
>> it's visible to the guest.  Draw another line in the sand
>> for broken save versions.  The version number should always
>> be updated when new values are saved and load should make
>> an attempt to set reasonable defaults for lower version save
>> images.
>>   
>
> I applied the the first part of this ancient patch.  Can't tell from 
> the thread what the consensus is for the rest.

Please don't bump the version number.  If you do, you'll be incompatible 
with upstream QEMU.  We have to coordinate version number changes or 
else we're going to guarantee that we break backwards compatibility down 
the road.

Regards,

Anthony Liguori

Re: [PATCH] virtio-net: Fix save/load

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Alex Williamson wrote:
>>> We can't rely on build switches to tell us if a save image
>>> includes a given field.  We also need to save status since
>>> it's visible to the guest.  Draw another line in the sand
>>> for broken save versions.  The version number should always
>>> be updated when new values are saved and load should make
>>> an attempt to set reasonable defaults for lower version save
>>> images.
>>>   
>>
>> I applied the the first part of this ancient patch.  Can't tell from 
>> the thread what the consensus is for the rest.
>
> Please don't bump the version number.  If you do, you'll be 
> incompatible with upstream QEMU.  We have to coordinate version number 
> changes or else we're going to guarantee that we break backwards 
> compatibility down the road.
>

I didn't.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] virtio-net: Fix save/load

[Alex Williamson]

On Wed, 2009-05-06 at 10:21 +0300, Avi Kivity wrote:
> Alex Williamson wrote:
> > We can't rely on build switches to tell us if a save image
> > includes a given field.  We also need to save status since
> > it's visible to the guest.  Draw another line in the sand
> > for broken save versions.  The version number should always
> > be updated when new values are saved and load should make
> > an attempt to set reasonable defaults for lower version save
> > images.
> >   
> 
> I applied the the first part of this ancient patch.  Can't tell from the 
> thread what the consensus is for the rest.

Thanks Avi, I think you got the important bits.  I might recommend the
following change to keep this contained to a version_id 7 load and bail
if vnet header support is required, but not available.  Thanks,

Alex


kvm: virtio-net: Cleanup load from vnet header saved image

Bail if the saved image requires vnet header support.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 hw/virtio-net.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)


diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 6dfe758..4beb16d 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -663,14 +663,15 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id >= 6)
         qemu_get_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
 
-#ifdef TAP_VNET_HDR
     if (version_id == 7 && qemu_get_be32(f)) {
+#ifdef TAP_VNET_HDR
         tap_using_vnet_hdr(n->vc->vlan->first_client, 1);
-    }
 #else
-    /* FIXME: error out if nonzero? */
-    qemu_get_be32(f);
+        fprintf(stderr,
+                "virtio-net: saved image requires vnet header support\n");
+        exit(1);
 #endif
+    }
 
     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,

Re: [PATCH] virtio-net: Fix save/load

[Avi Kivity]

Alex Williamson wrote:
> Thanks Avi, I think you got the important bits.  I might recommend the
> following change to keep this contained to a version_id 7 load and bail
> if vnet header support is required, but not available.  Thanks,
>
> Alex
>
>
> kvm: virtio-net: Cleanup load from vnet header saved image
>
> Bail if the saved image requires vnet header support.
>   

Thanks, applied.  I even thought of this while applying the original 
patch but I guess my attention span is in a downward spiral so I forgot 
about it.

-- 
error compiling committee.c: too many arguments to function