Message ID | 20160115120143.GB2432@work-vm |
---|---|
State | New |
Headers | show |
On Fri, 15 Jan 2016 12:01:44 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > I misunderstood the vmstate macro definition when > I reworked the virtio .get/.put - but I can't > get it to break for me, which suggests I'm perhaps > not managing to get that structure into being > sent in my tests. The first of the structures should be sent whenever virtio-1 is enabled on the host. I think virtio-pci (unlike virtio-ccw) still defaults to off?
Dear David, "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > Can you try this and let me know if it fixes it for you; I've > still not managed to persuade x86-64 to fail. With Conny's hint re. virtio-1 (thanks!) I managed to make it fail on x86_64, too. I'm using libvirt for testing (virDomainSave() / virDomainRestore() use the qemu migration API internally, allowing for easy testing of migration code). Since current libvirt doesn't offer any knobs to set disable-modern/disable, I had to configure the devices manually: <qemu:commandline> <qemu:arg value='-device'/> <qemu:arg value='virtio-serial-pci,id=virtio-serial0,bus=pci.0,disable-modern=off,disable-legacy=on'/> <qemu:arg value='-chardev'/> <qemu:arg value='file,id=charconsole0,path=/tmp/test0.log'/> <qemu:arg value='-device'/> <qemu:arg value='virtconsole,chardev=charconsole0'/> </qemu:commandline> With the above, migration fails on x86_64, too. Your patch fixes the basic save/resume test on both x86_64 and s390x, so: Tested-By: Sascha Silbe <silbe@linux.vnet.ibm.com> (I currently don't have a more extensive test for migration; in particular nothing that puts the guest in a pre-defined state and compares on-the-wire data across qemu versions.) I'm also confident by now that I'm having a reasonable grasp of this particular aspect of the code, so for the actual code changes: Reviewed-By: Sascha Silbe <silbe@linux.vnet.ibm.com> A commit message explaining what's going on would be nice, though. Maybe something along these lines: migration/virtio: fix migration of VirtQueues Commit 50e5ae4d [migration/virtio: Remove simple .get/.put use] refactored the virtio migration code to use the VMStateDescription API instead of the previous custom VMStateInfo API. It relied on VMSTATE_STRUCT_VARRAY_KNOWN, introduced by commit 2cf01486 [Add VMSTATE_STRUCT_VARRAY_KNOWN]. This was described as being for "a variable length array (i.e. _type *_field) but we know the length". However it actually specified operation for arrays embedded in the struct (i.e. _type _field[]) since it lacked the VMS_POINTER flag. This caused offset calculation to be completely off, examining and potentially sending random data instead of the VirtQueue content. Replace the otherwise unused VMSTATE_STRUCT_VARRAY_KNOWN with a VMSTATE_STRUCT_VARRAY_POINTER_KNOWN that includes the VMS_POINTER flag (so now actually doing what it advertises) and use it in the virtio migration code. (Feel free to reuse any or all of this). Sascha
Dear David, "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > +/* a variable length array (i.e. _type *_field) but we know the > + * length > + */ > +#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \ [...] Thinking about it some more, wouldn't VMSTATE_STRUCT_ARRAY_POINTER be a better name? Like with VMSTATE_ARRAY, the size of the array is known at compile-time. It's just that you need to dereference it first, hence ..._POINTER. There's nothing variable about it at all. But keep in mind I don't understand the current naming scheme in the first place, e.g. VMSTATE_ARRAY_INT32_UNSAFE vs. VMSTATE_VARRAY_INT32, with both of them specifying VMS_VARRAY_INT32... Sascha
* Sascha Silbe (silbe@linux.vnet.ibm.com) wrote: > Dear David, > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > +/* a variable length array (i.e. _type *_field) but we know the > > + * length > > + */ > > +#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \ > [...] > > Thinking about it some more, wouldn't VMSTATE_STRUCT_ARRAY_POINTER be a > better name? Like with VMSTATE_ARRAY, the size of the array is known at > compile-time. It's just that you need to dereference it first, hence > ..._POINTER. There's nothing variable about it at all. t's all a bit confusing; but the only pattern I'd figured out was that the things after the 'VARRAY_' part tended to be talking about the length of the array rather than the contents. > But keep in mind I don't understand the current naming scheme in the > first place, e.g. VMSTATE_ARRAY_INT32_UNSAFE vs. VMSTATE_VARRAY_INT32, > with both of them specifying VMS_VARRAY_INT32... No, I don't really either; one for Juan or Amit to suggest if they prefer one or the other. 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
* Sascha Silbe (silbe@linux.vnet.ibm.com) wrote: > Dear David, > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > Can you try this and let me know if it fixes it for you; I've > > still not managed to persuade x86-64 to fail. > > With Conny's hint re. virtio-1 (thanks!) I managed to make it fail on > x86_64, too. I'm using libvirt for testing (virDomainSave() / > virDomainRestore() use the qemu migration API internally, allowing for > easy testing of migration code). Since current libvirt doesn't offer any > knobs to set disable-modern/disable, I had to configure the devices > manually: > > <qemu:commandline> > <qemu:arg value='-device'/> > <qemu:arg value='virtio-serial-pci,id=virtio-serial0,bus=pci.0,disable-modern=off,disable-legacy=on'/> > <qemu:arg value='-chardev'/> > <qemu:arg value='file,id=charconsole0,path=/tmp/test0.log'/> > <qemu:arg value='-device'/> > <qemu:arg value='virtconsole,chardev=charconsole0'/> > </qemu:commandline> > > With the above, migration fails on x86_64, too. Thank you! With that example I used: <qemu:commandline> <qemu:arg value='--global'/> <qemu:arg value='virtio-pci.disable-modern=off'/> <qemu:arg value='--global'/> <qemu:arg value='virtio-pci.disable-legacy=on'/> <qemu:arg value='--global'/> <qemu:arg value='virtio-pci.migrate-extra=on'/> </qemu:commandline> (I had to use ide disk, my guest didn't like virtio-disk with that; but still had virtio-net and virtio-serial). > basic save/resume test on both x86_64 and s390x, so: > > Tested-By: Sascha Silbe <silbe@linux.vnet.ibm.com> Thanks. > (I currently don't have a more extensive test for migration; in > particular nothing that puts the guest in a pre-defined state and > compares on-the-wire data across qemu versions.) No, I don't think anyone does; too many fields change depending on timing etc - and the structure of the migration stream is too arbitrary to pull apart [One thing I'm trying to fix by avoiding .get/.put !]. > I'm also confident by now that I'm having a reasonable grasp of this > particular aspect of the code, so for the actual code changes: > > Reviewed-By: Sascha Silbe <silbe@linux.vnet.ibm.com> > > A commit message explaining what's going on would be nice, though. Maybe > something along these lines: > > migration/virtio: fix migration of VirtQueues > > Commit 50e5ae4d [migration/virtio: Remove simple .get/.put use] > refactored the virtio migration code to use the VMStateDescription API > instead of the previous custom VMStateInfo API. It relied on > VMSTATE_STRUCT_VARRAY_KNOWN, introduced by commit 2cf01486 [Add > VMSTATE_STRUCT_VARRAY_KNOWN]. This was described as being for "a > variable length array (i.e. _type *_field) but we know the > length". However it actually specified operation for arrays embedded in > the struct (i.e. _type _field[]) since it lacked the VMS_POINTER > flag. This caused offset calculation to be completely off, examining and > potentially sending random data instead of the VirtQueue content. > > Replace the otherwise unused VMSTATE_STRUCT_VARRAY_KNOWN with a > VMSTATE_STRUCT_VARRAY_POINTER_KNOWN that includes the VMS_POINTER flag > (so now actually doing what it advertises) and use it in the virtio > migration code. > > (Feel free to reuse any or all of this). Thanks I've reused a chunk of that; I'll post the fix soon. Thanks for your help on this. 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
Dear David, "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > <qemu:arg value='--global'/> > <qemu:arg value='virtio-pci.disable-modern=off'/> Interesting, didn't know about --global yet. Thanks for posting the example. That will come in handy for my tests. > Thanks I've reused a chunk of that; I'll post the fix soon. > Thanks for your help on this. Thanks, looking forward to the next version of your patch. Will use the the previous one in the meantime as a local work-around. Sascha
On Thu, 21 Jan 2016 21:56:22 +0100 Sascha Silbe <silbe@linux.vnet.ibm.com> wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > Thanks I've reused a chunk of that; I'll post the fix soon. > > Thanks for your help on this. > > Thanks, looking forward to the next version of your patch. Will use the > the previous one in the meantime as a local work-around. Any updates around this? I always need to remember to put this patch on top when I test migration...
* Cornelia Huck (cornelia.huck@de.ibm.com) wrote: > On Thu, 21 Jan 2016 21:56:22 +0100 > Sascha Silbe <silbe@linux.vnet.ibm.com> wrote: > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > > Thanks I've reused a chunk of that; I'll post the fix soon. > > > Thanks for your help on this. > > > > Thanks, looking forward to the next version of your patch. Will use the > > the previous one in the meantime as a local work-around. > > Any updates around this? I always need to remember to put this patch on > top when I test migration... Hmm - I thought I posted this properly but I can't see it on patchwork; I'll repost it. Dave > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index bd6b4df..41a8a8a 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1143,8 +1143,8 @@ static const VMStateDescription vmstate_virtio_virtqueues = { .minimum_version_id = 1, .needed = &virtio_virtqueue_needed, .fields = (VMStateField[]) { - VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, - 0, vmstate_virtqueue, VirtQueue), + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, + VIRTIO_QUEUE_MAX, 0, vmstate_virtqueue, VirtQueue), VMSTATE_END_OF_LIST() } }; @@ -1165,8 +1165,8 @@ static const VMStateDescription vmstate_virtio_ringsize = { .minimum_version_id = 1, .needed = &virtio_ringsize_needed, .fields = (VMStateField[]) { - VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, - 0, vmstate_ringsize, VirtQueue), + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, + VIRTIO_QUEUE_MAX, 0, vmstate_ringsize, VirtQueue), VMSTATE_END_OF_LIST() } }; diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 97d44d3..4a5d1dd 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -374,19 +374,6 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = vmstate_offset_array(_state, _field, _type, _num),\ } -/* a variable length array (i.e. _type *_field) but we know the - * length - */ -#define VMSTATE_STRUCT_VARRAY_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \ - .name = (stringify(_field)), \ - .num = (_num), \ - .version_id = (_version), \ - .vmsd = &(_vmsd), \ - .size = sizeof(_type), \ - .flags = VMS_STRUCT|VMS_ARRAY, \ - .offset = offsetof(_state, _field), \ -} - #define VMSTATE_STRUCT_VARRAY_UINT8(_field, _state, _field_num, _version, _vmsd, _type) { \ .name = (stringify(_field)), \ .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \ @@ -397,6 +384,19 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = offsetof(_state, _field), \ } +/* a variable length array (i.e. _type *_field) but we know the + * length + */ +#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \ + .name = (stringify(_field)), \ + .num = (_num), \ + .version_id = (_version), \ + .vmsd = &(_vmsd), \ + .size = sizeof(_type), \ + .flags = VMS_STRUCT|VMS_ARRAY|VMS_POINTER, \ + .offset = offsetof(_state, _field), \ +} + #define VMSTATE_STRUCT_VARRAY_POINTER_INT32(_field, _state, _field_num, _vmsd, _type) { \ .name = (stringify(_field)), \ .version_id = 0, \