[Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN

[Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN

[Dr. David Alan Gilbert (git)]

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

At the moment we have VMSTATE_STRUCT_ARRAY that requires
the field is declared as an array of fixed size.
We also have VMSTATE_STRUCT_VARRAY_UINT* that allows
a field declared as a pointer, but requires that the length
is a field member in the structure being loaded/saved.

VMSTATE_STRUCT_VARRAY_KNOWN is for arrays defined as pointers
yet we somehow know the length of.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/vmstate.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7267e38..97d44d3 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -374,6 +374,19 @@ 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), \
-- 
2.5.0

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

[Dr. David Alan Gilbert (git)]

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

The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE
macros rather than hand coded .get/.put

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/virtio.c | 87 ++++++++++++------------------------------------------
 1 file changed, 19 insertions(+), 68 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1edef59..28300cd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1126,33 +1126,15 @@ static bool virtio_extra_state_needed(void *opaque)
         k->has_extra_state(qbus->parent);
 }
 
-static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
-{
-    VirtIODevice *vdev = pv;
-    int i;
-
-    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        qemu_put_be64(f, vdev->vq[i].vring.avail);
-        qemu_put_be64(f, vdev->vq[i].vring.used);
-    }
-}
-
-static int get_virtqueue_state(QEMUFile *f, void *pv, size_t size)
-{
-    VirtIODevice *vdev = pv;
-    int i;
-
-    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        vdev->vq[i].vring.avail = qemu_get_be64(f);
-        vdev->vq[i].vring.used = qemu_get_be64(f);
-    }
-    return 0;
-}
-
-static VMStateInfo vmstate_info_virtqueue = {
+static const VMStateDescription vmstate_virtqueue = {
     .name = "virtqueue_state",
-    .get = get_virtqueue_state,
-    .put = put_virtqueue_state,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(vring.avail, struct VirtQueue),
+        VMSTATE_UINT64(vring.used, struct VirtQueue),
+        VMSTATE_END_OF_LIST()
+    }
 };
 
 static const VMStateDescription vmstate_virtio_virtqueues = {
@@ -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()
     }
 };
 
-static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
-{
-    VirtIODevice *vdev = pv;
-    int i;
-
-    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        qemu_put_be32(f, vdev->vq[i].vring.num_default);
-    }
-}
-
-static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
-{
-    VirtIODevice *vdev = pv;
-    int i;
-
-    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        vdev->vq[i].vring.num_default = qemu_get_be32(f);
-    }
-    return 0;
-}
-
-static VMStateInfo vmstate_info_ringsize = {
+static const VMStateDescription vmstate_ringsize = {
     .name = "ringsize_state",
-    .get = get_ringsize_state,
-    .put = put_ringsize_state,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(vring.num_default, struct VirtQueue),
+        VMSTATE_END_OF_LIST()
+    }
 };
 
 static const VMStateDescription vmstate_virtio_ringsize = {
@@ -1207,15 +1165,8 @@ static const VMStateDescription vmstate_virtio_ringsize = {
     .minimum_version_id = 1,
     .needed = &virtio_ringsize_needed,
     .fields = (VMStateField[]) {
-        {
-            .name         = "ringsize",
-            .version_id   = 0,
-            .field_exists = NULL,
-            .size         = 0,
-            .info         = &vmstate_info_ringsize,
-            .flags        = VMS_SINGLE,
-            .offset       = 0,
-        },
+        VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX,
+                      0, vmstate_ringsize, VirtQueue),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.5.0

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

[Michael S. Tsirkin]

On Wed, Jan 06, 2016 at 12:23:39PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE
> macros rather than hand coded .get/.put
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


I queued this, thanks!

> ---
>  hw/virtio/virtio.c | 87 ++++++++++++------------------------------------------
>  1 file changed, 19 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1edef59..28300cd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1126,33 +1126,15 @@ static bool virtio_extra_state_needed(void *opaque)
>          k->has_extra_state(qbus->parent);
>  }
>  
> -static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
> -{
> -    VirtIODevice *vdev = pv;
> -    int i;
> -
> -    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        qemu_put_be64(f, vdev->vq[i].vring.avail);
> -        qemu_put_be64(f, vdev->vq[i].vring.used);
> -    }
> -}
> -
> -static int get_virtqueue_state(QEMUFile *f, void *pv, size_t size)
> -{
> -    VirtIODevice *vdev = pv;
> -    int i;
> -
> -    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        vdev->vq[i].vring.avail = qemu_get_be64(f);
> -        vdev->vq[i].vring.used = qemu_get_be64(f);
> -    }
> -    return 0;
> -}
> -
> -static VMStateInfo vmstate_info_virtqueue = {
> +static const VMStateDescription vmstate_virtqueue = {
>      .name = "virtqueue_state",
> -    .get = get_virtqueue_state,
> -    .put = put_virtqueue_state,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(vring.avail, struct VirtQueue),
> +        VMSTATE_UINT64(vring.used, struct VirtQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
>  };
>  
>  static const VMStateDescription vmstate_virtio_virtqueues = {
> @@ -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()
>      }
>  };
>  
> -static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
> -{
> -    VirtIODevice *vdev = pv;
> -    int i;
> -
> -    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        qemu_put_be32(f, vdev->vq[i].vring.num_default);
> -    }
> -}
> -
> -static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
> -{
> -    VirtIODevice *vdev = pv;
> -    int i;
> -
> -    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        vdev->vq[i].vring.num_default = qemu_get_be32(f);
> -    }
> -    return 0;
> -}
> -
> -static VMStateInfo vmstate_info_ringsize = {
> +static const VMStateDescription vmstate_ringsize = {
>      .name = "ringsize_state",
> -    .get = get_ringsize_state,
> -    .put = put_ringsize_state,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(vring.num_default, struct VirtQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
>  };
>  
>  static const VMStateDescription vmstate_virtio_ringsize = {
> @@ -1207,15 +1165,8 @@ static const VMStateDescription vmstate_virtio_ringsize = {
>      .minimum_version_id = 1,
>      .needed = &virtio_ringsize_needed,
>      .fields = (VMStateField[]) {
> -        {
> -            .name         = "ringsize",
> -            .version_id   = 0,
> -            .field_exists = NULL,
> -            .size         = 0,
> -            .info         = &vmstate_info_ringsize,
> -            .flags        = VMS_SINGLE,
> -            .offset       = 0,
> -        },
> +        VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX,
> +                      0, vmstate_ringsize, VirtQueue),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> -- 
> 2.5.0

Re: [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN

[Amit Shah]

On (Wed) 06 Jan 2016 [12:23:38], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> At the moment we have VMSTATE_STRUCT_ARRAY that requires
> the field is declared as an array of fixed size.
> We also have VMSTATE_STRUCT_VARRAY_UINT* that allows
> a field declared as a pointer, but requires that the length
> is a field member in the structure being loaded/saved.
> 
> VMSTATE_STRUCT_VARRAY_KNOWN is for arrays defined as pointers
> yet we somehow know the length of.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit

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

[Amit Shah]

On (Wed) 06 Jan 2016 [12:23:39], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The 'virtqueue_state' and 'ringsize' can be saved using VMSTATE
> macros rather than hand coded .get/.put
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Amit Shah <amit.shah@redhat.com>


		Amit

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

[Sascha Silbe]

Dear David,

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> 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

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

[Dr. David Alan Gilbert]

* Sascha Silbe (silbe@linux.vnet.ibm.com) wrote:
> Dear David,

Hi Sascha,
  Thanks for the mail.

> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> 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 / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

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

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,                                                 \
-- 
2.5.0

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

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

[Cornelia Huck]

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?

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

[Sascha Silbe]

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

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

[Sascha Silbe]

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

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

[Dr. David Alan Gilbert]

* 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

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

[Dr. David Alan Gilbert]

* 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

[Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Sascha Silbe]

The VMState API is rather sparsely documented. Start by describing the
meaning of all VMStateFlags.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
Since I had to dive into the code for debugging the migration breakage
anyway, I figured I could just as well write down the result in the
form of comments so the next one trying to make some sense out of it
will have a better start.

This is based on my understanding of the code, not necessarily what
the original author(s) intended.

 include/migration/vmstate.h | 93 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 9 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 97d44d3..b1bbf68 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -88,20 +88,95 @@ struct VMStateInfo {
 };
 
 enum VMStateFlags {
+    /* Ignored */
     VMS_SINGLE           = 0x001,
+
+    /* The struct member at opaque + VMStateField.offset is a pointer
+     * to the actual field (e.g. struct a { uint8_t *b;
+     * }). Dereference the pointer before using it as basis for
+     * further pointer arithmetic (see e.g. VMS_ARRAY). Does not
+     * affect the meaning of VMStateField.num_offset or
+     * VMStateField.size_offset; see VMS_VARRAY* and VMS_VBUFFER for
+     * those. */
     VMS_POINTER          = 0x002,
+
+    /* The field is an array of fixed size. VMStateField.num contains
+     * the number of entries in the array. The size of each entry is
+     * given by VMStateField.size and / or opaque +
+     * VMStateField.size_offset; see VMS_VBUFFER and
+     * VMS_MULTIPLY. Each array entry will be processed individually
+     * (VMStateField.info.get()/put() if VMS_STRUCT is not set,
+     * recursion into VMStateField.vmsd if VMS_STRUCT is set). May not
+     * be combined with VMS_VARRAY*. */
     VMS_ARRAY            = 0x004,
+
+    /* The field is itself a struct, containing one or more
+     * fields. Recurse into VMStateField.vmsd. Most useful in
+     * combination with VMS_ARRAY / VMS_VARRAY*, recursing into each
+     * array entry. */
     VMS_STRUCT           = 0x008,
-    VMS_VARRAY_INT32     = 0x010,  /* Array with size in int32_t field*/
-    VMS_BUFFER           = 0x020,  /* static sized buffer */
+
+    /* The field is an array of variable size. The int32_t at opaque +
+     * VMStateField.num_offset contains the number of entries in the
+     * array. See the VMS_ARRAY description regarding array handling
+     * in general. May not be combined with VMS_ARRAY or any other
+     * VMS_VARRAY*. */
+    VMS_VARRAY_INT32     = 0x010,
+
+    /* Ignored */
+    VMS_BUFFER           = 0x020,
+
+    /* The field is a (fixed-size or variable-size) array of pointers
+     * (e.g. struct a { uint8_t *b[]; }). Dereference each array entry
+     * before using it. Note: Does not imply any one of VMS_ARRAY /
+     * VMS_VARRAY*; these need to be set explicitly. */
     VMS_ARRAY_OF_POINTER = 0x040,
-    VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
-    VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
-    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
-    VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
-    VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
-    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
-    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
+
+    /* The field is an array of variable size. The uint16_t at opaque
+     * + VMStateField.num_offset contains the number of entries in the
+     * array. See the VMS_ARRAY description regarding array handling
+     * in general. May not be combined with VMS_ARRAY or any other
+     * VMS_VARRAY*. */
+    VMS_VARRAY_UINT16    = 0x080,
+
+    /* The size of the individual entries (a single array entry if
+     * VMS_ARRAY or VMS_VARRAY are set, or the field itself if neither
+     * is set) is variable (i.e. not known at compile-time), but the
+     * same for all entries. Use the int32_t at opaque +
+     * VMStateField.size_offset (subject to VMS_MULTIPLY) to determine
+     * the size of each (and every) entry. */
+    VMS_VBUFFER          = 0x100,
+
+    /* Multiply the entry size given by the int32_t at opaque +
+     * VMStateField.size_offset (see VMS_VBUFFER description) with
+     * VMStateField.size to determine the number of bytes to be
+     * allocated. Only valid in combination with VMS_VBUFFER. */
+    VMS_MULTIPLY         = 0x200,
+
+    /* The field is an array of variable size. The uint8_t at opaque +
+     * VMStateField.num_offset contains the number of entries in the
+     * array. See the VMS_ARRAY description regarding array handling
+     * in general. May not be combined with VMS_ARRAY or any other
+     * VMS_VARRAY*. */
+    VMS_VARRAY_UINT8     = 0x400,
+
+    /* The field is an array of variable size. The uint32_t at opaque
+     * + VMStateField.num_offset contains the number of entries in the
+     * array. See the VMS_ARRAY description regarding array handling
+     * in general. May not be combined with VMS_ARRAY or any other
+     * VMS_VARRAY*. */
+    VMS_VARRAY_UINT32    = 0x800,
+
+    /* Fail loading the serialised VM state if this field is missing
+     * from the input. */
+    VMS_MUST_EXIST       = 0x1000,
+
+    /* When loading serialised VM state, allocate memory for the
+     * (entire) field. Only valid in combination with
+     * VMS_POINTER. Note: Not all combinations with other flags are
+     * currently supported, e.g. VMS_ALLOC|VMS_ARRAY_OF_POINTER won't
+     * cause the individual entries to be allocated. */
+    VMS_ALLOC            = 0x2000,
 };
 
 typedef struct {
-- 
2.1.4

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

[Sascha Silbe]

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

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

[Cornelia Huck]

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

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

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Amit Shah]

On (Thu) 21 Jan 2016 [21:39:27], Sascha Silbe wrote:
> The VMState API is rather sparsely documented. Start by describing the
> meaning of all VMStateFlags.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>

Thanks, this is much needed.  I'm just returning from a long trip,
will go through this in a bit.

		Amit

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Amit Shah]

On (Thu) 21 Jan 2016 [21:39:27], Sascha Silbe wrote:
> The VMState API is rather sparsely documented. Start by describing the
> meaning of all VMStateFlags.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>

Reviewed-by: Amit Shah <amit.shah@redhat.com>

> ---
> Since I had to dive into the code for debugging the migration breakage
> anyway, I figured I could just as well write down the result in the
> form of comments so the next one trying to make some sense out of it
> will have a better start.

Yes, nice.  Thanks!

		Amit

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Juan Quintela]

Sascha Silbe <silbe@linux.vnet.ibm.com> wrote:
> The VMState API is rather sparsely documented. Start by describing the
> meaning of all VMStateFlags.
>
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> ---
> Since I had to dive into the code for debugging the migration breakage
> anyway, I figured I could just as well write down the result in the
> form of comments so the next one trying to make some sense out of it
> will have a better start.
>
> This is based on my understanding of the code, not necessarily what
> the original author(s) intended.
>
>  include/migration/vmstate.h | 93 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 84 insertions(+), 9 deletions(-)
>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks very much


> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 97d44d3..b1bbf68 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -88,20 +88,95 @@ struct VMStateInfo {
>  };
>  
>  enum VMStateFlags {
> +    /* Ignored */
>      VMS_SINGLE           = 0x001,
> +
> +    /* The struct member at opaque + VMStateField.offset is a pointer
> +     * to the actual field (e.g. struct a { uint8_t *b;
> +     * }). Dereference the pointer before using it as basis for
> +     * further pointer arithmetic (see e.g. VMS_ARRAY). Does not
> +     * affect the meaning of VMStateField.num_offset or
> +     * VMStateField.size_offset; see VMS_VARRAY* and VMS_VBUFFER for
> +     * those. */
>      VMS_POINTER          = 0x002,
> +
> +    /* The field is an array of fixed size. VMStateField.num contains
> +     * the number of entries in the array. The size of each entry is
> +     * given by VMStateField.size and / or opaque +
> +     * VMStateField.size_offset; see VMS_VBUFFER and
> +     * VMS_MULTIPLY. Each array entry will be processed individually
> +     * (VMStateField.info.get()/put() if VMS_STRUCT is not set,
> +     * recursion into VMStateField.vmsd if VMS_STRUCT is set). May not
> +     * be combined with VMS_VARRAY*. */
>      VMS_ARRAY            = 0x004,
> +
> +    /* The field is itself a struct, containing one or more
> +     * fields. Recurse into VMStateField.vmsd. Most useful in
> +     * combination with VMS_ARRAY / VMS_VARRAY*, recursing into each
> +     * array entry. */
>      VMS_STRUCT           = 0x008,
> -    VMS_VARRAY_INT32     = 0x010,  /* Array with size in int32_t field*/
> -    VMS_BUFFER           = 0x020,  /* static sized buffer */
> +
> +    /* The field is an array of variable size. The int32_t at opaque +
> +     * VMStateField.num_offset contains the number of entries in the
> +     * array. See the VMS_ARRAY description regarding array handling
> +     * in general. May not be combined with VMS_ARRAY or any other
> +     * VMS_VARRAY*. */
> +    VMS_VARRAY_INT32     = 0x010,
> +
> +    /* Ignored */
> +    VMS_BUFFER           = 0x020,
> +
> +    /* The field is a (fixed-size or variable-size) array of pointers
> +     * (e.g. struct a { uint8_t *b[]; }). Dereference each array entry
> +     * before using it. Note: Does not imply any one of VMS_ARRAY /
> +     * VMS_VARRAY*; these need to be set explicitly. */
>      VMS_ARRAY_OF_POINTER = 0x040,
> -    VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
> -    VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
> -    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
> -    VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
> -    VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
> -    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
> -    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
> +
> +    /* The field is an array of variable size. The uint16_t at opaque
> +     * + VMStateField.num_offset contains the number of entries in the
> +     * array. See the VMS_ARRAY description regarding array handling
> +     * in general. May not be combined with VMS_ARRAY or any other
> +     * VMS_VARRAY*. */
> +    VMS_VARRAY_UINT16    = 0x080,
> +
> +    /* The size of the individual entries (a single array entry if
> +     * VMS_ARRAY or VMS_VARRAY are set, or the field itself if neither
> +     * is set) is variable (i.e. not known at compile-time), but the
> +     * same for all entries. Use the int32_t at opaque +
> +     * VMStateField.size_offset (subject to VMS_MULTIPLY) to determine
> +     * the size of each (and every) entry. */
> +    VMS_VBUFFER          = 0x100,
> +
> +    /* Multiply the entry size given by the int32_t at opaque +
> +     * VMStateField.size_offset (see VMS_VBUFFER description) with
> +     * VMStateField.size to determine the number of bytes to be
> +     * allocated. Only valid in combination with VMS_VBUFFER. */
> +    VMS_MULTIPLY         = 0x200,
> +
> +    /* The field is an array of variable size. The uint8_t at opaque +
> +     * VMStateField.num_offset contains the number of entries in the
> +     * array. See the VMS_ARRAY description regarding array handling
> +     * in general. May not be combined with VMS_ARRAY or any other
> +     * VMS_VARRAY*. */
> +    VMS_VARRAY_UINT8     = 0x400,
> +
> +    /* The field is an array of variable size. The uint32_t at opaque
> +     * + VMStateField.num_offset contains the number of entries in the
> +     * array. See the VMS_ARRAY description regarding array handling
> +     * in general. May not be combined with VMS_ARRAY or any other
> +     * VMS_VARRAY*. */
> +    VMS_VARRAY_UINT32    = 0x800,
> +
> +    /* Fail loading the serialised VM state if this field is missing
> +     * from the input. */
> +    VMS_MUST_EXIST       = 0x1000,
> +
> +    /* When loading serialised VM state, allocate memory for the
> +     * (entire) field. Only valid in combination with
> +     * VMS_POINTER. Note: Not all combinations with other flags are
> +     * currently supported, e.g. VMS_ALLOC|VMS_ARRAY_OF_POINTER won't
> +     * cause the individual entries to be allocated. */
> +    VMS_ALLOC            = 0x2000,
>  };
>  
>  typedef struct {

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Amit Shah]

On (Thu) 21 Jan 2016 [21:39:27], Sascha Silbe wrote:
> The VMState API is rather sparsely documented. Start by describing the
> meaning of all VMStateFlags.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> ---
> Since I had to dive into the code for debugging the migration breakage
> anyway, I figured I could just as well write down the result in the
> form of comments so the next one trying to make some sense out of it
> will have a better start.
> 
> This is based on my understanding of the code, not necessarily what
> the original author(s) intended.

Thanks, I've pulled this in my queue.

There's one new flag added: VMS_MULTIPLY_ELEMENTS.  Do you want to
send a follow-up patch to add a description to that one as well?

		Amit

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Sascha Silbe]

Dear Amit,

Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) 21 Jan 2016 [21:39:27], Sascha Silbe wrote:
>> The VMState API is rather sparsely documented. Start by describing the
>> meaning of all VMStateFlags.

> Thanks, I've pulled this in my queue.
>
> There's one new flag added: VMS_MULTIPLY_ELEMENTS.  Do you want to
> send a follow-up patch to add a description to that one as well?

I can either send a new version with the conflicts resolved and
VMS_MULTIPLY_ELEMENTS documented (i.e. rebased on current master), or a
follow-up patch with just the differences to your version if you point
me at your git repo. Whatever works best for you.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Sascha Silbe]

Dear Amit,

> Amit Shah <amit.shah@redhat.com> writes:
>
>> On (Thu) 21 Jan 2016 [21:39:27], Sascha Silbe wrote:
>>> The VMState API is rather sparsely documented. Start by describing the
>>> meaning of all VMStateFlags.
>
>> Thanks, I've pulled this in my queue.
>>
>> There's one new flag added: VMS_MULTIPLY_ELEMENTS.  Do you want to
>> send a follow-up patch to add a description to that one as well?
>
> I can either send a new version with the conflicts resolved and
> VMS_MULTIPLY_ELEMENTS documented (i.e. rebased on current master), or a
> follow-up patch with just the differences to your version if you point
> me at your git repo. Whatever works best for you.

FWIW, here's the version based on current master. As mentioned before;
I'm happy to rebase on your version if you prefer.

Sascha

--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8--->8---

Subject: [PATCH v2] migration/vmstate: document VMStateFlags

The VMState API is rather sparsely documented. Start by describing the
meaning of all VMStateFlags.

Reviewed-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
v1->v2:
  - rebased on current devel
  - documented VMS_MULTIPLY_ELEMENTS (including references to
    VMS_MULTIPLY_ELEMENTS in the VMS_VARRAY* description)
  - fixed up VMS_VARRAY* reference in VMS_VBUFFER
  - included Amit's and Juan's Reviewed-By (based on v1)
---
 include/migration/vmstate.h | 100 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 10 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7246f29..84ee355 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -88,21 +88,101 @@ struct VMStateInfo {
 };
 
 enum VMStateFlags {
+    /* Ignored */
     VMS_SINGLE           = 0x001,
+
+    /* The struct member at opaque + VMStateField.offset is a pointer
+     * to the actual field (e.g. struct a { uint8_t *b;
+     * }). Dereference the pointer before using it as basis for
+     * further pointer arithmetic (see e.g. VMS_ARRAY). Does not
+     * affect the meaning of VMStateField.num_offset or
+     * VMStateField.size_offset; see VMS_VARRAY* and VMS_VBUFFER for
+     * those. */
     VMS_POINTER          = 0x002,
+
+    /* The field is an array of fixed size. VMStateField.num contains
+     * the number of entries in the array. The size of each entry is
+     * given by VMStateField.size and / or opaque +
+     * VMStateField.size_offset; see VMS_VBUFFER and
+     * VMS_MULTIPLY. Each array entry will be processed individually
+     * (VMStateField.info.get()/put() if VMS_STRUCT is not set,
+     * recursion into VMStateField.vmsd if VMS_STRUCT is set). May not
+     * be combined with VMS_VARRAY*. */
     VMS_ARRAY            = 0x004,
+
+    /* The field is itself a struct, containing one or more
+     * fields. Recurse into VMStateField.vmsd. Most useful in
+     * combination with VMS_ARRAY / VMS_VARRAY*, recursing into each
+     * array entry. */
     VMS_STRUCT           = 0x008,
-    VMS_VARRAY_INT32     = 0x010,  /* Array with size in int32_t field*/
-    VMS_BUFFER           = 0x020,  /* static sized buffer */
+
+    /* The field is an array of variable size. The int32_t at opaque +
+     * VMStateField.num_offset contains the number of entries in the
+     * array. See the VMS_ARRAY description regarding array handling
+     * in general. May not be combined with VMS_ARRAY or any other
+     * VMS_VARRAY*. */
+    VMS_VARRAY_INT32     = 0x010,
+
+    /* Ignored */
+    VMS_BUFFER           = 0x020,
+
+    /* The field is a (fixed-size or variable-size) array of pointers
+     * (e.g. struct a { uint8_t *b[]; }). Dereference each array entry
+     * before using it. Note: Does not imply any one of VMS_ARRAY /
+     * VMS_VARRAY*; these need to be set explicitly. */
     VMS_ARRAY_OF_POINTER = 0x040,
-    VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
-    VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
-    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
-    VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
-    VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
-    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
-    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
-    VMS_MULTIPLY_ELEMENTS = 0x4000, /* multiply varray size by field->num */
+
+    /* The field is an array of variable size. The uint16_t at opaque
+     * + VMStateField.num_offset (subject to VMS_MULTIPLY_ELEMENTS)
+     * contains the number of entries in the array. See the VMS_ARRAY
+     * description regarding array handling in general. May not be
+     * combined with VMS_ARRAY or any other VMS_VARRAY*. */
+    VMS_VARRAY_UINT16    = 0x080,
+
+    /* The size of the individual entries (a single array entry if
+     * VMS_ARRAY or any of VMS_VARRAY* are set, or the field itself if
+     * neither is set) is variable (i.e. not known at compile-time),
+     * but the same for all entries. Use the int32_t at opaque +
+     * VMStateField.size_offset (subject to VMS_MULTIPLY) to determine
+     * the size of each (and every) entry. */
+    VMS_VBUFFER          = 0x100,
+
+    /* Multiply the entry size given by the int32_t at opaque +
+     * VMStateField.size_offset (see VMS_VBUFFER description) with
+     * VMStateField.size to determine the number of bytes to be
+     * allocated. Only valid in combination with VMS_VBUFFER. */
+    VMS_MULTIPLY         = 0x200,
+
+    /* The field is an array of variable size. The uint8_t at opaque +
+     * VMStateField.num_offset (subject to VMS_MULTIPLY_ELEMENTS)
+     * contains the number of entries in the array. See the VMS_ARRAY
+     * description regarding array handling in general. May not be
+     * combined with VMS_ARRAY or any other VMS_VARRAY*. */
+    VMS_VARRAY_UINT8     = 0x400,
+
+    /* The field is an array of variable size. The uint32_t at opaque
+     * + VMStateField.num_offset (subject to VMS_MULTIPLY_ELEMENTS)
+     * contains the number of entries in the array. See the VMS_ARRAY
+     * description regarding array handling in general. May not be
+     * combined with VMS_ARRAY or any other VMS_VARRAY*. */
+    VMS_VARRAY_UINT32    = 0x800,
+
+    /* Fail loading the serialised VM state if this field is missing
+     * from the input. */
+    VMS_MUST_EXIST       = 0x1000,
+
+    /* When loading serialised VM state, allocate memory for the
+     * (entire) field. Only valid in combination with
+     * VMS_POINTER. Note: Not all combinations with other flags are
+     * currently supported, e.g. VMS_ALLOC|VMS_ARRAY_OF_POINTER won't
+     * cause the individual entries to be allocated. */
+    VMS_ALLOC            = 0x2000,
+
+    /* Multiply the number of entries given by the integer at opaque +
+     * VMStateField.num_offset (see VMS_VARRAY*) with VMStateField.num
+     * to determine the number of entries in the array. Only valid in
+     * combination with one of VMS_VARRAY*. */
+    VMS_MULTIPLY_ELEMENTS = 0x4000,
 };
 
 typedef struct {
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Amit Shah]

On (Thu) 25 Feb 2016 [21:45:23], Sascha Silbe wrote:
> Dear Amit,
> 
> > Amit Shah <amit.shah@redhat.com> writes:
> >
> >> On (Thu) 21 Jan 2016 [21:39:27], Sascha Silbe wrote:
> >>> The VMState API is rather sparsely documented. Start by describing the
> >>> meaning of all VMStateFlags.
> >
> >> Thanks, I've pulled this in my queue.
> >>
> >> There's one new flag added: VMS_MULTIPLY_ELEMENTS.  Do you want to
> >> send a follow-up patch to add a description to that one as well?
> >
> > I can either send a new version with the conflicts resolved and
> > VMS_MULTIPLY_ELEMENTS documented (i.e. rebased on current master), or a
> > follow-up patch with just the differences to your version if you point
> > me at your git repo. Whatever works best for you.
> 
> FWIW, here's the version based on current master. As mentioned before;
> I'm happy to rebase on your version if you prefer.

Thanks, can you please post this as a standalone email?  Easier for me
to apply.

Thanks,

		Amit

[Qemu-devel] [PATCH v2] migration/vmstate: document VMStateFlags

[Sascha Silbe]

The VMState API is rather sparsely documented. Start by describing the
meaning of all VMStateFlags.

Reviewed-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
v1->v2:
  - rebased on current devel
  - documented VMS_MULTIPLY_ELEMENTS (including references to
    VMS_MULTIPLY_ELEMENTS in the VMS_VARRAY* description)
  - fixed up VMS_VARRAY* reference in VMS_VBUFFER
  - included Amit's and Juan's Reviewed-By (based on v1)
---
 include/migration/vmstate.h | 100 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 10 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7246f29..84ee355 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -88,21 +88,101 @@ struct VMStateInfo {
 };
 
 enum VMStateFlags {
+    /* Ignored */
     VMS_SINGLE           = 0x001,
+
+    /* The struct member at opaque + VMStateField.offset is a pointer
+     * to the actual field (e.g. struct a { uint8_t *b;
+     * }). Dereference the pointer before using it as basis for
+     * further pointer arithmetic (see e.g. VMS_ARRAY). Does not
+     * affect the meaning of VMStateField.num_offset or
+     * VMStateField.size_offset; see VMS_VARRAY* and VMS_VBUFFER for
+     * those. */
     VMS_POINTER          = 0x002,
+
+    /* The field is an array of fixed size. VMStateField.num contains
+     * the number of entries in the array. The size of each entry is
+     * given by VMStateField.size and / or opaque +
+     * VMStateField.size_offset; see VMS_VBUFFER and
+     * VMS_MULTIPLY. Each array entry will be processed individually
+     * (VMStateField.info.get()/put() if VMS_STRUCT is not set,
+     * recursion into VMStateField.vmsd if VMS_STRUCT is set). May not
+     * be combined with VMS_VARRAY*. */
     VMS_ARRAY            = 0x004,
+
+    /* The field is itself a struct, containing one or more
+     * fields. Recurse into VMStateField.vmsd. Most useful in
+     * combination with VMS_ARRAY / VMS_VARRAY*, recursing into each
+     * array entry. */
     VMS_STRUCT           = 0x008,
-    VMS_VARRAY_INT32     = 0x010,  /* Array with size in int32_t field*/
-    VMS_BUFFER           = 0x020,  /* static sized buffer */
+
+    /* The field is an array of variable size. The int32_t at opaque +
+     * VMStateField.num_offset contains the number of entries in the
+     * array. See the VMS_ARRAY description regarding array handling
+     * in general. May not be combined with VMS_ARRAY or any other
+     * VMS_VARRAY*. */
+    VMS_VARRAY_INT32     = 0x010,
+
+    /* Ignored */
+    VMS_BUFFER           = 0x020,
+
+    /* The field is a (fixed-size or variable-size) array of pointers
+     * (e.g. struct a { uint8_t *b[]; }). Dereference each array entry
+     * before using it. Note: Does not imply any one of VMS_ARRAY /
+     * VMS_VARRAY*; these need to be set explicitly. */
     VMS_ARRAY_OF_POINTER = 0x040,
-    VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
-    VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
-    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
-    VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
-    VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
-    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
-    VMS_ALLOC            = 0x2000, /* Alloc a buffer on the destination */
-    VMS_MULTIPLY_ELEMENTS = 0x4000, /* multiply varray size by field->num */
+
+    /* The field is an array of variable size. The uint16_t at opaque
+     * + VMStateField.num_offset (subject to VMS_MULTIPLY_ELEMENTS)
+     * contains the number of entries in the array. See the VMS_ARRAY
+     * description regarding array handling in general. May not be
+     * combined with VMS_ARRAY or any other VMS_VARRAY*. */
+    VMS_VARRAY_UINT16    = 0x080,
+
+    /* The size of the individual entries (a single array entry if
+     * VMS_ARRAY or any of VMS_VARRAY* are set, or the field itself if
+     * neither is set) is variable (i.e. not known at compile-time),
+     * but the same for all entries. Use the int32_t at opaque +
+     * VMStateField.size_offset (subject to VMS_MULTIPLY) to determine
+     * the size of each (and every) entry. */
+    VMS_VBUFFER          = 0x100,
+
+    /* Multiply the entry size given by the int32_t at opaque +
+     * VMStateField.size_offset (see VMS_VBUFFER description) with
+     * VMStateField.size to determine the number of bytes to be
+     * allocated. Only valid in combination with VMS_VBUFFER. */
+    VMS_MULTIPLY         = 0x200,
+
+    /* The field is an array of variable size. The uint8_t at opaque +
+     * VMStateField.num_offset (subject to VMS_MULTIPLY_ELEMENTS)
+     * contains the number of entries in the array. See the VMS_ARRAY
+     * description regarding array handling in general. May not be
+     * combined with VMS_ARRAY or any other VMS_VARRAY*. */
+    VMS_VARRAY_UINT8     = 0x400,
+
+    /* The field is an array of variable size. The uint32_t at opaque
+     * + VMStateField.num_offset (subject to VMS_MULTIPLY_ELEMENTS)
+     * contains the number of entries in the array. See the VMS_ARRAY
+     * description regarding array handling in general. May not be
+     * combined with VMS_ARRAY or any other VMS_VARRAY*. */
+    VMS_VARRAY_UINT32    = 0x800,
+
+    /* Fail loading the serialised VM state if this field is missing
+     * from the input. */
+    VMS_MUST_EXIST       = 0x1000,
+
+    /* When loading serialised VM state, allocate memory for the
+     * (entire) field. Only valid in combination with
+     * VMS_POINTER. Note: Not all combinations with other flags are
+     * currently supported, e.g. VMS_ALLOC|VMS_ARRAY_OF_POINTER won't
+     * cause the individual entries to be allocated. */
+    VMS_ALLOC            = 0x2000,
+
+    /* Multiply the number of entries given by the integer at opaque +
+     * VMStateField.num_offset (see VMS_VARRAY*) with VMStateField.num
+     * to determine the number of entries in the array. Only valid in
+     * combination with one of VMS_VARRAY*. */
+    VMS_MULTIPLY_ELEMENTS = 0x4000,
 };
 
 typedef struct {
-- 
1.9.1

Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags

[Sascha Silbe]

Dear Amit,

Amit Shah <amit.shah@redhat.com> writes:

>> FWIW, here's the version based on current master. As mentioned before;
>> I'm happy to rebase on your version if you prefer.
>
> Thanks, can you please post this as a standalone email?  Easier for me
> to apply.

Sure.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641