| Message ID | 1232042731.20605.9.camel@bling (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;
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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html