| Message ID | 1452083019-15141-2-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 06, 2016 at 12:23:39PM +0000, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE > macros rather than hand coded .get/.put > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> I queued this, thanks! > --- > hw/virtio/virtio.c | 87 ++++++++++++------------------------------------------ > 1 file changed, 19 insertions(+), 68 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 1edef59..28300cd 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1126,33 +1126,15 @@ static bool virtio_extra_state_needed(void *opaque) > k->has_extra_state(qbus->parent); > } > > -static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size) > -{ > - VirtIODevice *vdev = pv; > - int i; > - > - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - qemu_put_be64(f, vdev->vq[i].vring.avail); > - qemu_put_be64(f, vdev->vq[i].vring.used); > - } > -} > - > -static int get_virtqueue_state(QEMUFile *f, void *pv, size_t size) > -{ > - VirtIODevice *vdev = pv; > - int i; > - > - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - vdev->vq[i].vring.avail = qemu_get_be64(f); > - vdev->vq[i].vring.used = qemu_get_be64(f); > - } > - return 0; > -} > - > -static VMStateInfo vmstate_info_virtqueue = { > +static const VMStateDescription vmstate_virtqueue = { > .name = "virtqueue_state", > - .get = get_virtqueue_state, > - .put = put_virtqueue_state, > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(vring.avail, struct VirtQueue), > + VMSTATE_UINT64(vring.used, struct VirtQueue), > + VMSTATE_END_OF_LIST() > + } > }; > > static const VMStateDescription vmstate_virtio_virtqueues = { > @@ -1161,44 +1143,20 @@ static const VMStateDescription vmstate_virtio_virtqueues = { > .minimum_version_id = 1, > .needed = &virtio_virtqueue_needed, > .fields = (VMStateField[]) { > - { > - .name = "virtqueues", > - .version_id = 0, > - .field_exists = NULL, > - .size = 0, > - .info = &vmstate_info_virtqueue, > - .flags = VMS_SINGLE, > - .offset = 0, > - }, > + VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, > + 0, vmstate_virtqueue, VirtQueue), > VMSTATE_END_OF_LIST() > } > }; > > -static void put_ringsize_state(QEMUFile *f, void *pv, size_t size) > -{ > - VirtIODevice *vdev = pv; > - int i; > - > - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - qemu_put_be32(f, vdev->vq[i].vring.num_default); > - } > -} > - > -static int get_ringsize_state(QEMUFile *f, void *pv, size_t size) > -{ > - VirtIODevice *vdev = pv; > - int i; > - > - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - vdev->vq[i].vring.num_default = qemu_get_be32(f); > - } > - return 0; > -} > - > -static VMStateInfo vmstate_info_ringsize = { > +static const VMStateDescription vmstate_ringsize = { > .name = "ringsize_state", > - .get = get_ringsize_state, > - .put = put_ringsize_state, > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(vring.num_default, struct VirtQueue), > + VMSTATE_END_OF_LIST() > + } > }; > > static const VMStateDescription vmstate_virtio_ringsize = { > @@ -1207,15 +1165,8 @@ static const VMStateDescription vmstate_virtio_ringsize = { > .minimum_version_id = 1, > .needed = &virtio_ringsize_needed, > .fields = (VMStateField[]) { > - { > - .name = "ringsize", > - .version_id = 0, > - .field_exists = NULL, > - .size = 0, > - .info = &vmstate_info_ringsize, > - .flags = VMS_SINGLE, > - .offset = 0, > - }, > + VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, > + 0, vmstate_ringsize, VirtQueue), > VMSTATE_END_OF_LIST() > } > }; > -- > 2.5.0
On (Wed) 06 Jan 2016 [12:23:39], Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE > macros rather than hand coded .get/.put > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Amit Shah <amit.shah@redhat.com> Amit
Dear David, "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE > macros rather than hand coded .get/.put [...] > @@ -1161,44 +1143,20 @@ static const VMStateDescription vmstate_virtio_virtqueues = { > .minimum_version_id = 1, > .needed = &virtio_virtqueue_needed, > .fields = (VMStateField[]) { > - { > - .name = "virtqueues", > - .version_id = 0, > - .field_exists = NULL, > - .size = 0, > - .info = &vmstate_info_virtqueue, > - .flags = VMS_SINGLE, > - .offset = 0, > - }, > + VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, > + 0, vmstate_virtqueue, VirtQueue), > VMSTATE_END_OF_LIST() > } > }; This completely breaks migration (including virsh save / restore) on s390x. It appears to work on x86_64 with a trivial test case, but that's probably pure luck and I'd expect a more extensive test case to show guest state corruption on migration. After staring at the code for several hours (this whole VMSTATE_* stuff is severely underdocumented), I think I've finally understood what's going on. Given the VMS_STRUCT|VMS_ARRAY field flags set by VMSTATE_STRUCT_VARRAY_KNOWN, vmstate_save_state() expects an array of fixed size embedded in the struct. However, VirtIODevice.vq is a pointer to an allocated array. Adding the VMS_POINTER flag looks like it could do the trick and indeed allows my trivial test case to complete on s390x again. (No further testing was done). I won't be sending a fix for this for now as I don't understand what the naming conventions for VMSTATE_* are (both VMSTATE_ARRAY* and VMSTATE_VARRAY* can use VMS_VARRAY, something with and sometimes without VMS_POINTER). Should VMSTATE_STRUCT_VARRAY_KNOWN be fixed to include VMS_POINTER or do you need to introduce another macro VMSTATE_STRUCT_VARRAY_POINTER_KNOWN? PS: Won't your patch break migration between different qemu versions? I don't see any compat code and you're changing at least some field names (e.g. "virtqueues" vs. "vq"). Sascha
* Sascha Silbe (silbe@linux.vnet.ibm.com) wrote: > Dear David, Hi Sascha, Thanks for the mail. > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > > > The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE > > macros rather than hand coded .get/.put > [...] > > @@ -1161,44 +1143,20 @@ static const VMStateDescription vmstate_virtio_virtqueues = { > > .minimum_version_id = 1, > > .needed = &virtio_virtqueue_needed, > > .fields = (VMStateField[]) { > > - { > > - .name = "virtqueues", > > - .version_id = 0, > > - .field_exists = NULL, > > - .size = 0, > > - .info = &vmstate_info_virtqueue, > > - .flags = VMS_SINGLE, > > - .offset = 0, > > - }, > > + VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, > > + 0, vmstate_virtqueue, VirtQueue), > > VMSTATE_END_OF_LIST() > > } > > }; > > This completely breaks migration (including virsh save / restore) on > s390x. It appears to work on x86_64 with a trivial test case, but that's > probably pure luck and I'd expect a more extensive test case to show > guest state corruption on migration. Interesting; I'd given it what I thought was a reasonable test of migrating a VM using virtio back and forward between the old and new versions on x86_64 a few times. > After staring at the code for several hours (this whole VMSTATE_* stuff > is severely underdocumented), I think I've finally understood what's > going on. Yes, I agree - 130+ macros with similar names and ~5 cryptic parameters each and barely a comment. > Given the VMS_STRUCT|VMS_ARRAY field flags set by > VMSTATE_STRUCT_VARRAY_KNOWN, vmstate_save_state() expects an array of > fixed size embedded in the struct. However, VirtIODevice.vq is a pointer > to an allocated array. Adding the VMS_POINTER flag looks like it could > do the trick and indeed allows my trivial test case to complete on s390x > again. (No further testing was done). Hmm, yes adding VMS_POINTER makes sense to me. > I won't be sending a fix for this for now as I don't understand what the > naming conventions for VMSTATE_* are (both VMSTATE_ARRAY* and > VMSTATE_VARRAY* can use VMS_VARRAY, something with and sometimes without > VMS_POINTER). Should VMSTATE_STRUCT_VARRAY_KNOWN be fixed to include > VMS_POINTER or do you need to introduce another macro > VMSTATE_STRUCT_VARRAY_POINTER_KNOWN? I think it needs renaming to VMSTATE_STRUCT_VARRAY_POINTER_KNOWN with the VMS_POINTER. > PS: Won't your patch break migration between different qemu versions? I > don't see any compat code and you're changing at least some field names > (e.g. "virtqueues" vs. "vq"). The field names generally dont hit the wire and so renaming is normally safe; the exception is subsection names. Thanks for spotting this, I'll write a patch and try and figure out how to test it better. Dave > > Sascha > -- > Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg > https://se-silbe.de/ > USt-IdNr. DE281696641 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 1edef59..28300cd 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1126,33 +1126,15 @@ static bool virtio_extra_state_needed(void *opaque) k->has_extra_state(qbus->parent); } -static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size) -{ - VirtIODevice *vdev = pv; - int i; - - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { - qemu_put_be64(f, vdev->vq[i].vring.avail); - qemu_put_be64(f, vdev->vq[i].vring.used); - } -} - -static int get_virtqueue_state(QEMUFile *f, void *pv, size_t size) -{ - VirtIODevice *vdev = pv; - int i; - - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { - vdev->vq[i].vring.avail = qemu_get_be64(f); - vdev->vq[i].vring.used = qemu_get_be64(f); - } - return 0; -} - -static VMStateInfo vmstate_info_virtqueue = { +static const VMStateDescription vmstate_virtqueue = { .name = "virtqueue_state", - .get = get_virtqueue_state, - .put = put_virtqueue_state, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT64(vring.avail, struct VirtQueue), + VMSTATE_UINT64(vring.used, struct VirtQueue), + VMSTATE_END_OF_LIST() + } }; static const VMStateDescription vmstate_virtio_virtqueues = { @@ -1161,44 +1143,20 @@ static const VMStateDescription vmstate_virtio_virtqueues = { .minimum_version_id = 1, .needed = &virtio_virtqueue_needed, .fields = (VMStateField[]) { - { - .name = "virtqueues", - .version_id = 0, - .field_exists = NULL, - .size = 0, - .info = &vmstate_info_virtqueue, - .flags = VMS_SINGLE, - .offset = 0, - }, + VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, + 0, vmstate_virtqueue, VirtQueue), VMSTATE_END_OF_LIST() } }; -static void put_ringsize_state(QEMUFile *f, void *pv, size_t size) -{ - VirtIODevice *vdev = pv; - int i; - - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { - qemu_put_be32(f, vdev->vq[i].vring.num_default); - } -} - -static int get_ringsize_state(QEMUFile *f, void *pv, size_t size) -{ - VirtIODevice *vdev = pv; - int i; - - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { - vdev->vq[i].vring.num_default = qemu_get_be32(f); - } - return 0; -} - -static VMStateInfo vmstate_info_ringsize = { +static const VMStateDescription vmstate_ringsize = { .name = "ringsize_state", - .get = get_ringsize_state, - .put = put_ringsize_state, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(vring.num_default, struct VirtQueue), + VMSTATE_END_OF_LIST() + } }; static const VMStateDescription vmstate_virtio_ringsize = { @@ -1207,15 +1165,8 @@ static const VMStateDescription vmstate_virtio_ringsize = { .minimum_version_id = 1, .needed = &virtio_ringsize_needed, .fields = (VMStateField[]) { - { - .name = "ringsize", - .version_id = 0, - .field_exists = NULL, - .size = 0, - .info = &vmstate_info_ringsize, - .flags = VMS_SINGLE, - .offset = 0, - }, + VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, + 0, vmstate_ringsize, VirtQueue), VMSTATE_END_OF_LIST() } };