Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use

[Sascha Silbe]

Dear David,

"Dr. David Alan Gilbert" <address@hidden> 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 <address@hidden>

(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 <address@hidden>

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
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641