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

[Dr. David Alan Gilbert]

* Sascha Silbe (address@hidden) wrote:
> Dear David,

Hi Sascha,
  Thanks for the mail.

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

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