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

[Dr. David Alan Gilbert]

* Sascha Silbe (address@hidden) wrote:
> 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.

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

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 <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).

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 / address@hidden / Manchester, UK