Re: [Qemu-devel] [PATCH] Fix virtio migration

[Cornelia Huck]

On Fri, 29 Jan 2016 13:18:56 +
"Dr. David Alan Gilbert (git)"  wrote:

> From: "Dr. David Alan Gilbert" 
> 
> I misunderstood the vmstate macro definition when I reworked the
> virtio .get/.put.
> The VMSTATE_STRUCT_VARRAY_KNOWN, 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.
> 
> Fixes and description as per Sascha's suggestions/debug.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> Reported-by: Sascha Silbe 
> Tested-By: Sascha Silbe 
> Reviewed-By: Sascha Silbe 
> 
> Fixes: 50e5ae4dc3e4f21e874512f9e87b93b5472d26e0
> Fixes: 2cf0148674430b6693c60d42b7eef721bfa9509f
> ---
>  hw/virtio/virtio.c  |  8 
>  include/migration/vmstate.h | 18 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)

Tested-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH] Fix virtio migration

[Peter Maydell]

On 29 January 2016 at 13:18, Dr. David Alan Gilbert (git)
 wrote:
> From: "Dr. David Alan Gilbert" 
>
> I misunderstood the vmstate macro definition when I reworked the
> virtio .get/.put.
> The VMSTATE_STRUCT_VARRAY_KNOWN, 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.

Yeah, these macro names are a bit of a mess. I had an idea ages
back about autogenerating them all as an orthogonal cross product
of the different kinds of thing (and filling in some of the random
gaps in coverage as a side effect), but I never really got anywhere
with it.

thanks
-- PMM

[Qemu-devel] [PATCH] Fix virtio migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

I misunderstood the vmstate macro definition when I reworked the
virtio .get/.put.
The VMSTATE_STRUCT_VARRAY_KNOWN, 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.

Fixes and description as per Sascha's suggestions/debug.

Signed-off-by: Dr. David Alan Gilbert 
Reported-by: Sascha Silbe 
Tested-By: Sascha Silbe 
Reviewed-By: Sascha Silbe 

Fixes: 50e5ae4dc3e4f21e874512f9e87b93b5472d26e0
Fixes: 2cf0148674430b6693c60d42b7eef721bfa9509f
---
 hw/virtio/virtio.c  |  8 
 include/migration/vmstate.h | 18 +-
 2 files changed, 13 insertions(+), 13 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 = _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 = _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 a4b81bb..7246f29 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -386,26 +386,26 @@ 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) { \
+#define VMSTATE_STRUCT_VARRAY_UINT8(_field, _state, _field_num, _version, 
_vmsd, _type) { \
 .name   = (stringify(_field)),   \
-.num  = (_num),  \
+.num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
 .version_id = (_version),\
 .vmsd   = &(_vmsd),  \
 .size   = sizeof(_type), \
-.flags  = VMS_STRUCT|VMS_ARRAY,  \
+.flags  = VMS_STRUCT|VMS_VARRAY_UINT8,   \
 .offset = offsetof(_state, _field),  \
 }
 
-#define VMSTATE_STRUCT_VARRAY_UINT8(_field, _state, _field_num, _version, 
_vmsd, _type) { \
+/* 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_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
+.num  = (_num),  \
 .version_id = (_version),\
 .vmsd   = &(_vmsd),  \
 .size   = sizeof(_type), \
-.flags  = VMS_STRUCT|VMS_VARRAY_UINT8,   \
+.flags  = VMS_STRUCT|VMS_ARRAY|VMS_POINTER,  \
 .offset = offsetof(_state, _field),  \
 }
 
-- 
2.5.0