diff mbox

[2/2] migration/virtio: Remove simple .get/.put use

Message ID 20160115120143.GB2432@work-vm
State New
Headers show

Commit Message

Dr. David Alan Gilbert Jan. 15, 2016, 12:01 p.m. UTC 
Hi Sascha,
  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.


Dave

From f300fa0dd902b28417744d364986c71dd8357a88 Mon Sep 17 00:00:00 2001
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Date: Fri, 15 Jan 2016 11:02:02 +0000
Subject: [PATCH] Fix virito migration

I misunderstood the vmstate macro definition when
I reworked the virtio .get/.put - but I can't
get it to break for me, which suggests I'm perhaps
not managing to get that structure into being
sent in my tests.

Fixes as per Sascha's suggestions/debug.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reported-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Fixes: 50e5ae4dc3e4f21e874512f9e87b93b5472d26e0
Fixes: 2cf0148674430b6693c60d42b7eef721bfa9509f
---
 hw/virtio/virtio.c          |  8 ++++----
 include/migration/vmstate.h | 26 +++++++++++++-------------
 2 files changed, 17 insertions(+), 17 deletions(-)

Comments

Cornelia Huck Jan. 18, 2016, 7:52 a.m. UTC | #1 
On Fri, 15 Jan 2016 12:01:44 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> I misunderstood the vmstate macro definition when
> I reworked the virtio .get/.put - but I can't
> get it to break for me, which suggests I'm perhaps
> not managing to get that structure into being
> sent in my tests.

The first of the structures should be sent whenever virtio-1 is enabled
on the host. I think virtio-pci (unlike virtio-ccw) still defaults to
off?
Sascha Silbe Jan. 18, 2016, 4:40 p.m. UTC | #2 
Dear David,

"Dr. David Alan Gilbert" <dgilbert@redhat.com> 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 <silbe@linux.vnet.ibm.com>

(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 <silbe@linux.vnet.ibm.com>

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
Sascha Silbe Jan. 18, 2016, 7:41 p.m. UTC | #3 
Dear David,

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> +/* a variable length array (i.e. _type *_field) but we know the
> + * length
> + */
> +#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \
[...]

Thinking about it some more, wouldn't VMSTATE_STRUCT_ARRAY_POINTER be a
better name? Like with VMSTATE_ARRAY, the size of the array is known at
compile-time. It's just that you need to dereference it first, hence
..._POINTER. There's nothing variable about it at all.

But keep in mind I don't understand the current naming scheme in the
first place, e.g. VMSTATE_ARRAY_INT32_UNSAFE vs. VMSTATE_VARRAY_INT32,
with both of them specifying VMS_VARRAY_INT32...

Sascha
Dr. David Alan Gilbert Jan. 19, 2016, 10:36 a.m. UTC | #4 
* Sascha Silbe (silbe@linux.vnet.ibm.com) wrote:
> Dear David,
> 
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > +/* a variable length array (i.e. _type *_field) but we know the
> > + * length
> > + */
> > +#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \
> [...]
> 
> Thinking about it some more, wouldn't VMSTATE_STRUCT_ARRAY_POINTER be a
> better name? Like with VMSTATE_ARRAY, the size of the array is known at
> compile-time. It's just that you need to dereference it first, hence
> ..._POINTER. There's nothing variable about it at all.

t's all a bit confusing; but the only pattern I'd figured out was that the
things after the 'VARRAY_' part tended to be talking about the length of
the array rather than the contents.

> But keep in mind I don't understand the current naming scheme in the
> first place, e.g. VMSTATE_ARRAY_INT32_UNSAFE vs. VMSTATE_VARRAY_INT32,
> with both of them specifying VMS_VARRAY_INT32...

No, I don't really either; one for Juan or Amit to suggest if they
prefer one or the other.

Dave

> 
> Sascha
> -- 
> Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
> https://se-silbe.de/
> USt-IdNr. DE281696641
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Jan. 19, 2016, 12:08 p.m. UTC | #5 
* Sascha Silbe (silbe@linux.vnet.ibm.com) wrote:
> Dear David,
> 
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> 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 <silbe@linux.vnet.ibm.com>

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 <silbe@linux.vnet.ibm.com>
> 
> 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 / dgilbert@redhat.com / Manchester, UK
Sascha Silbe Jan. 21, 2016, 8:56 p.m. UTC | #6 
Dear David,

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

>     <qemu:arg value='--global'/>
>     <qemu:arg value='virtio-pci.disable-modern=off'/>

Interesting, didn't know about --global yet. Thanks for posting the
example. That will come in handy for my tests.


> Thanks I've reused a chunk of that;  I'll post the fix soon.
> Thanks for your help on this.

Thanks, looking forward to the next version of your patch. Will use the
the previous one in the meantime as a local work-around.

Sascha
Cornelia Huck Jan. 29, 2016, 12:53 p.m. UTC | #7 
On Thu, 21 Jan 2016 21:56:22 +0100
Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> > Thanks I've reused a chunk of that;  I'll post the fix soon.
> > Thanks for your help on this.
> 
> Thanks, looking forward to the next version of your patch. Will use the
> the previous one in the meantime as a local work-around.

Any updates around this? I always need to remember to put this patch on
top when I test migration...
Dr. David Alan Gilbert Jan. 29, 2016, 1:14 p.m. UTC | #8 
* Cornelia Huck (cornelia.huck@de.ibm.com) wrote:
> On Thu, 21 Jan 2016 21:56:22 +0100
> Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> 
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > > Thanks I've reused a chunk of that;  I'll post the fix soon.
> > > Thanks for your help on this.
> > 
> > Thanks, looking forward to the next version of your patch. Will use the
> > the previous one in the meantime as a local work-around.
> 
> Any updates around this? I always need to remember to put this patch on
> top when I test migration...

Hmm - I thought I posted this properly but I can't see it on patchwork; I'll
repost it.

Dave

> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bd6b4df..41a8a8a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1143,8 +1143,8 @@  static const VMStateDescription vmstate_virtio_virtqueues = {
     .minimum_version_id = 1,
     .needed = &virtio_virtqueue_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX,
-                      0, vmstate_virtqueue, VirtQueue),
+        VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice,
+                      VIRTIO_QUEUE_MAX, 0, vmstate_virtqueue, VirtQueue),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -1165,8 +1165,8 @@  static const VMStateDescription vmstate_virtio_ringsize = {
     .minimum_version_id = 1,
     .needed = &virtio_ringsize_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX,
-                      0, vmstate_ringsize, VirtQueue),
+        VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice,
+                      VIRTIO_QUEUE_MAX, 0, vmstate_ringsize, VirtQueue),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 97d44d3..4a5d1dd 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -374,19 +374,6 @@  extern const VMStateInfo vmstate_info_bitmap;
     .offset       = vmstate_offset_array(_state, _field, _type, _num),\
 }
 
-/* a variable length array (i.e. _type *_field) but we know the
- * length
- */
-#define VMSTATE_STRUCT_VARRAY_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \
-    .name       = (stringify(_field)),                               \
-    .num          = (_num),                                          \
-    .version_id = (_version),                                        \
-    .vmsd       = &(_vmsd),                                          \
-    .size       = sizeof(_type),                                     \
-    .flags      = VMS_STRUCT|VMS_ARRAY,                              \
-    .offset     = offsetof(_state, _field),                          \
-}
-
 #define VMSTATE_STRUCT_VARRAY_UINT8(_field, _state, _field_num, _version, _vmsd, _type) { \
     .name       = (stringify(_field)),                               \
     .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
@@ -397,6 +384,19 @@  extern const VMStateInfo vmstate_info_bitmap;
     .offset     = offsetof(_state, _field),                          \
 }
 
+/* a variable length array (i.e. _type *_field) but we know the
+ * length
+ */
+#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \
+    .name       = (stringify(_field)),                               \
+    .num          = (_num),                                          \
+    .version_id = (_version),                                        \
+    .vmsd       = &(_vmsd),                                          \
+    .size       = sizeof(_type),                                     \
+    .flags      = VMS_STRUCT|VMS_ARRAY|VMS_POINTER,                  \
+    .offset     = offsetof(_state, _field),                          \
+}
+
 #define VMSTATE_STRUCT_VARRAY_POINTER_INT32(_field, _state, _field_num, _vmsd, _type) { \
     .name       = (stringify(_field)),                               \
     .version_id = 0,                                                 \