* [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN
@ 2016-01-06 12:23 Dr. David Alan Gilbert (git)
2016-01-06 12:23 ` [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use Dr. David Alan Gilbert (git)
2016-01-08 12:12 ` [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN Amit Shah
0 siblings, 2 replies; 26+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-01-06 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, mst, quintela
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-06 12:23 [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN Dr. David Alan Gilbert (git)
@ 2016-01-06 12:23 ` Dr. David Alan Gilbert (git)
2016-01-07 11:35 ` Michael S. Tsirkin
` (2 more replies)
2016-01-08 12:12 ` [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN Amit Shah
1 sibling, 3 replies; 26+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-01-06 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, mst, quintela
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-06 12:23 ` [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use Dr. David Alan Gilbert (git)
@ 2016-01-07 11:35 ` Michael S. Tsirkin
2016-01-08 12:12 ` Amit Shah
2016-01-14 21:16 ` Sascha Silbe
2 siblings, 0 replies; 26+ messages in thread
From: Michael S. Tsirkin @ 2016-01-07 11:35 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: amit.shah, qemu-devel, quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN
2016-01-06 12:23 [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN Dr. David Alan Gilbert (git)
2016-01-06 12:23 ` [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use Dr. David Alan Gilbert (git)
@ 2016-01-08 12:12 ` Amit Shah
1 sibling, 0 replies; 26+ messages in thread
From: Amit Shah @ 2016-01-08 12:12 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: mst, qemu-devel, quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-06 12:23 ` [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use Dr. David Alan Gilbert (git)
2016-01-07 11:35 ` Michael S. Tsirkin
@ 2016-01-08 12:12 ` Amit Shah
2016-01-14 21:16 ` Sascha Silbe
2 siblings, 0 replies; 26+ messages in thread
From: Amit Shah @ 2016-01-08 12:12 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: mst, qemu-devel, quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-06 12:23 ` [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use Dr. David Alan Gilbert (git)
2016-01-07 11:35 ` Michael S. Tsirkin
2016-01-08 12:12 ` Amit Shah
@ 2016-01-14 21:16 ` Sascha Silbe
2016-01-15 9:24 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 26+ messages in thread
From: Sascha Silbe @ 2016-01-14 21:16 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel
Cc: amit.shah, Cornelia Huck, quintela, mst
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-14 21:16 ` Sascha Silbe
@ 2016-01-15 9:24 ` Dr. David Alan Gilbert
2016-01-15 12:01 ` Dr. David Alan Gilbert
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
0 siblings, 2 replies; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2016-01-15 9:24 UTC (permalink / raw)
To: Sascha Silbe; +Cc: amit.shah, Cornelia Huck, quintela, qemu-devel, mst
* 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-15 9:24 ` Dr. David Alan Gilbert
@ 2016-01-15 12:01 ` Dr. David Alan Gilbert
2016-01-18 7:52 ` Cornelia Huck
` (2 more replies)
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
1 sibling, 3 replies; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2016-01-15 12:01 UTC (permalink / raw)
To: Sascha Silbe; +Cc: amit.shah, Cornelia Huck, mst, qemu-devel, quintela
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-15 12:01 ` Dr. David Alan Gilbert
@ 2016-01-18 7:52 ` Cornelia Huck
2016-01-18 16:40 ` Sascha Silbe
2016-01-18 19:41 ` Sascha Silbe
2 siblings, 0 replies; 26+ messages in thread
From: Cornelia Huck @ 2016-01-18 7:52 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: amit.shah, quintela, mst, qemu-devel, Sascha Silbe
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?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-15 12:01 ` Dr. David Alan Gilbert
2016-01-18 7:52 ` Cornelia Huck
@ 2016-01-18 16:40 ` Sascha Silbe
2016-01-19 12:08 ` Dr. David Alan Gilbert
2016-01-18 19:41 ` Sascha Silbe
2 siblings, 1 reply; 26+ messages in thread
From: Sascha Silbe @ 2016-01-18 16:40 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: amit.shah, Cornelia Huck, quintela, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-15 12:01 ` Dr. David Alan Gilbert
2016-01-18 7:52 ` Cornelia Huck
2016-01-18 16:40 ` Sascha Silbe
@ 2016-01-18 19:41 ` Sascha Silbe
2016-01-19 10:36 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 26+ messages in thread
From: Sascha Silbe @ 2016-01-18 19:41 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: amit.shah, Cornelia Huck, quintela, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-18 19:41 ` Sascha Silbe
@ 2016-01-19 10:36 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2016-01-19 10:36 UTC (permalink / raw)
To: Sascha Silbe; +Cc: amit.shah, Cornelia Huck, quintela, qemu-devel, mst
* 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-18 16:40 ` Sascha Silbe
@ 2016-01-19 12:08 ` Dr. David Alan Gilbert
2016-01-21 20:56 ` Sascha Silbe
0 siblings, 1 reply; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2016-01-19 12:08 UTC (permalink / raw)
To: Sascha Silbe; +Cc: amit.shah, Cornelia Huck, quintela, qemu-devel, mst
* 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-01-15 9:24 ` Dr. David Alan Gilbert
2016-01-15 12:01 ` Dr. David Alan Gilbert
@ 2016-01-21 20:39 ` Sascha Silbe
2016-02-03 12:38 ` Amit Shah
` (3 more replies)
1 sibling, 4 replies; 26+ messages in thread
From: Sascha Silbe @ 2016-01-21 20:39 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Michael S. Tsirkin, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-19 12:08 ` Dr. David Alan Gilbert
@ 2016-01-21 20:56 ` Sascha Silbe
2016-01-29 12:53 ` Cornelia Huck
0 siblings, 1 reply; 26+ messages in thread
From: Sascha Silbe @ 2016-01-21 20:56 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: amit.shah, Cornelia Huck, mst, qemu-devel, quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-21 20:56 ` Sascha Silbe
@ 2016-01-29 12:53 ` Cornelia Huck
2016-01-29 13:14 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 26+ messages in thread
From: Cornelia Huck @ 2016-01-29 12:53 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: amit.shah, quintela, mst, qemu-devel, Sascha Silbe
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...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use
2016-01-29 12:53 ` Cornelia Huck
@ 2016-01-29 13:14 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 26+ messages in thread
From: Dr. David Alan Gilbert @ 2016-01-29 13:14 UTC (permalink / raw)
To: Cornelia Huck; +Cc: amit.shah, quintela, mst, qemu-devel, Sascha Silbe
* 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
@ 2016-02-03 12:38 ` Amit Shah
2016-02-23 10:39 ` Amit Shah
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Amit Shah @ 2016-02-03 12:38 UTC (permalink / raw)
To: Sascha Silbe
Cc: Michael S. Tsirkin, qemu-devel, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
2016-02-03 12:38 ` Amit Shah
@ 2016-02-23 10:39 ` Amit Shah
2016-02-23 12:32 ` Juan Quintela
2016-02-25 5:05 ` Amit Shah
3 siblings, 0 replies; 26+ messages in thread
From: Amit Shah @ 2016-02-23 10:39 UTC (permalink / raw)
To: Sascha Silbe
Cc: Michael S. Tsirkin, qemu-devel, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
2016-02-03 12:38 ` Amit Shah
2016-02-23 10:39 ` Amit Shah
@ 2016-02-23 12:32 ` Juan Quintela
2016-02-25 5:05 ` Amit Shah
3 siblings, 0 replies; 26+ messages in thread
From: Juan Quintela @ 2016-02-23 12:32 UTC (permalink / raw)
To: Sascha Silbe
Cc: Amit Shah, qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin
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 {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
` (2 preceding siblings ...)
2016-02-23 12:32 ` Juan Quintela
@ 2016-02-25 5:05 ` Amit Shah
2016-02-25 20:25 ` Sascha Silbe
3 siblings, 1 reply; 26+ messages in thread
From: Amit Shah @ 2016-02-25 5:05 UTC (permalink / raw)
To: Sascha Silbe
Cc: Michael S. Tsirkin, qemu-devel, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-02-25 5:05 ` Amit Shah
@ 2016-02-25 20:25 ` Sascha Silbe
2016-02-25 20:45 ` Sascha Silbe
0 siblings, 1 reply; 26+ messages in thread
From: Sascha Silbe @ 2016-02-25 20:25 UTC (permalink / raw)
To: Amit Shah
Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-02-25 20:25 ` Sascha Silbe
@ 2016-02-25 20:45 ` Sascha Silbe
2016-02-26 5:27 ` Amit Shah
0 siblings, 1 reply; 26+ messages in thread
From: Sascha Silbe @ 2016-02-25 20:45 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, qemu-devel, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-02-25 20:45 ` Sascha Silbe
@ 2016-02-26 5:27 ` Amit Shah
2016-02-26 8:18 ` [Qemu-devel] [PATCH v2] " Sascha Silbe
2016-02-26 8:19 ` [Qemu-devel] [PATCH qemu] " Sascha Silbe
0 siblings, 2 replies; 26+ messages in thread
From: Amit Shah @ 2016-02-26 5:27 UTC (permalink / raw)
To: Sascha Silbe
Cc: Michael S. Tsirkin, qemu-devel, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v2] migration/vmstate: document VMStateFlags
2016-02-26 5:27 ` Amit Shah
@ 2016-02-26 8:18 ` Sascha Silbe
2016-02-26 8:19 ` [Qemu-devel] [PATCH qemu] " Sascha Silbe
1 sibling, 0 replies; 26+ messages in thread
From: Sascha Silbe @ 2016-02-26 8:18 UTC (permalink / raw)
To: qemu-devel, Amit Shah, Juan Quintela
Cc: Dr. David Alan Gilbert, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags
2016-02-26 5:27 ` Amit Shah
2016-02-26 8:18 ` [Qemu-devel] [PATCH v2] " Sascha Silbe
@ 2016-02-26 8:19 ` Sascha Silbe
1 sibling, 0 replies; 26+ messages in thread
From: Sascha Silbe @ 2016-02-26 8:19 UTC (permalink / raw)
To: Amit Shah
Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2016-02-26 8:19 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-06 12:23 [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN Dr. David Alan Gilbert (git)
2016-01-06 12:23 ` [Qemu-devel] [PATCH 2/2] migration/virtio: Remove simple .get/.put use Dr. David Alan Gilbert (git)
2016-01-07 11:35 ` Michael S. Tsirkin
2016-01-08 12:12 ` Amit Shah
2016-01-14 21:16 ` Sascha Silbe
2016-01-15 9:24 ` Dr. David Alan Gilbert
2016-01-15 12:01 ` Dr. David Alan Gilbert
2016-01-18 7:52 ` Cornelia Huck
2016-01-18 16:40 ` Sascha Silbe
2016-01-19 12:08 ` Dr. David Alan Gilbert
2016-01-21 20:56 ` Sascha Silbe
2016-01-29 12:53 ` Cornelia Huck
2016-01-29 13:14 ` Dr. David Alan Gilbert
2016-01-18 19:41 ` Sascha Silbe
2016-01-19 10:36 ` Dr. David Alan Gilbert
2016-01-21 20:39 ` [Qemu-devel] [PATCH qemu] migration/vmstate: document VMStateFlags Sascha Silbe
2016-02-03 12:38 ` Amit Shah
2016-02-23 10:39 ` Amit Shah
2016-02-23 12:32 ` Juan Quintela
2016-02-25 5:05 ` Amit Shah
2016-02-25 20:25 ` Sascha Silbe
2016-02-25 20:45 ` Sascha Silbe
2016-02-26 5:27 ` Amit Shah
2016-02-26 8:18 ` [Qemu-devel] [PATCH v2] " Sascha Silbe
2016-02-26 8:19 ` [Qemu-devel] [PATCH qemu] " Sascha Silbe
2016-01-08 12:12 ` [Qemu-devel] [PATCH 1/2] Add VMSTATE_STRUCT_VARRAY_KNOWN Amit Shah
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.