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

[Sascha Silbe]

Dear David,

"Dr. David Alan Gilbert (git)" <address@hidden> 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
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641