* Sascha Silbe ([email protected]) wrote: > Dear David, Hi Sascha, Thanks for the mail.
> "Dr. David Alan Gilbert (git)" <[email protected]> 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 / [email protected] / Manchester, UK
