Skip to contentSkip to conversation

virtio-net: Fix save/load

PATCH Applied Alex Williamson 4 participants 9 messages Jan 15, 2009 – May 7, 2009 KVM

lore
Share & tools
lore

Alex Williamson

author
#0 lore raw

PATCH 1/1 virtio-net: Fix save/load

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 <[email protected]>

---

 qemu/hw/virtio-net.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)
qemu/hw/virtio-net.c +17 -2
index 358c382..6ce2ff5 100644
@@ -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;
#1 lore raw

Hi Alex,

Quoted 2 lines · Alex Williamson

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

Uggh, good point.

Quoted 1 line

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.

Quoted 26 lines

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 <[email protected]>

---

 qemu/hw/virtio-net.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)
qemu/hw/virtio-net.c +2 -0
index 358c382..6ce2ff5 100644
@@ -21,6 +21,8 @@
 
 #define TAP_VNET_HDR
 
+#define VIRTIO_NET_VM_VERSION    3
+
 typedef struct VirtIONet
 {
     VirtIODevice vdev;

qemu/hw/virtio-net.c static void virtio_net_tx_timer(void *opaque) · line 377

Quoted 8 lines
qemu/hw/virtio-net.c +5 -0
@@ -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.

Quoted 10 lines
qemu/hw/virtio-net.c +2 -0
 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/hw/virtio-net.c static void virtio_net_save(QEMUFile *f, void *opaque) · line 401

Quoted 10 lines
qemu/hw/virtio-net.c +3 -1
+
+    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.

author
#2 lore raw

Hi Mark,

Quoted 7 lines · Mark McLoughlin

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.

Quoted 8 lines
Quoted 5 lines

+/*
+ * 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 ;)

Quoted 9 lines
Quoted 1 line

static int virtio_net_load(QEMUFile *f, void *opaque, int

version_id)

qemu/hw/virtio-net.c static void virtio_net_save(QEMUFile *f, void *opaque) · line 401

Quoted 5 lines
 {
     VirtIONet *n = opaque;
qemu/hw/virtio-net.c +1 -1
-    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

Signature · 1 line
Alex Williamson                             HP Open Source & Linux Org.
#3 lore raw
Quoted 7 lines · Alex Williamson
Quoted 7 lines

version_id)

Quoted 5 lines
 {
     VirtIONet *n = opaque;
qemu/hw/virtio-net.c +1 -1
-    if (version_id != 2)
+    if (version_id < 3 || version_id > VIRTIO_NET_VM_VERSION)
Quoted 7 lines
Quoted 1 line

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.

#4 lore raw
In reply to Alex Williamson
Quoted 8 lines · 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.

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

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
#5 lore raw
Quoted 11 lines · Avi Kivity
Quoted 8 lines · 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.

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

#6 lore raw
Quoted 11 lines · Anthony Liguori
Quoted 10 lines · Avi Kivity
Quoted 8 lines · 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.

Quoted 4 lines
Quoted 1 line

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.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
author
#7 lore raw
In reply to Avi Kivity
Quoted 10 lines · Avi Kivity
Quoted 8 lines · 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.

Quoted 2 lines

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 <[email protected]>

---

 hw/virtio-net.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
hw/virtio-net.c +5 -4
index 6dfe758..4beb16d 100644
@@ -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,
Applied
#8 lore raw
Quoted 11 lines · Alex Williamson

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.

Signature · 1 line
error compiling committee.c: too many arguments to function