* [PATCH RFC 01/10] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:36 ` Alexander Mikhalitsyn
2026-03-17 23:23 ` [PATCH RFC 02/10] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
Passing in a pointer almost never helps. Convert it to pass in struct for
further refactoring on VMS_ARRAY_OF_POINTER.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 6 +++---
tests/unit/test-vmstate.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index d4a39aa794..8992fba57d 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -547,9 +547,9 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num = (_num), \
.info = &(_info), \
- .size = sizeof(_type), \
+ .size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
- .offset = vmstate_offset_array(_state, _field, _type, _num), \
+ .offset = vmstate_offset_array(_state, _field, _type*, _num), \
}
#define VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(_f, _s, _n, _v, _vmsd, _type) { \
@@ -1093,7 +1093,7 @@ extern const VMStateInfo vmstate_info_qlist;
VMSTATE_TIMER_PTR_V(_f, _s, 0)
#define VMSTATE_TIMER_PTR_ARRAY(_f, _s, _n) \
- VMSTATE_ARRAY_OF_POINTER(_f, _s, _n, 0, vmstate_info_timer, QEMUTimer *)
+ VMSTATE_ARRAY_OF_POINTER(_f, _s, _n, 0, vmstate_info_timer, QEMUTimer)
#define VMSTATE_TIMER_TEST(_f, _s, _test) \
VMSTATE_SINGLE_TEST(_f, _s, _test, 0, vmstate_info_timer, QEMUTimer)
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index cadbab3c5e..6a42cc1a4e 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -668,7 +668,7 @@ const VMStateDescription vmsd_arpp = {
.minimum_version_id = 1,
.fields = (const VMStateField[]) {
VMSTATE_ARRAY_OF_POINTER(ar, TestArrayOfPtrToInt,
- AR_SIZE, 0, vmstate_info_int32, int32_t*),
+ AR_SIZE, 0, vmstate_info_int32, int32_t),
VMSTATE_END_OF_LIST()
}
};
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 01/10] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
2026-03-17 23:23 ` [PATCH RFC 01/10] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
@ 2026-03-18 9:36 ` Alexander Mikhalitsyn
0 siblings, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:36 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> Passing in a pointer almost never helps. Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 6 +++---
> tests/unit/test-vmstate.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index d4a39aa794..8992fba57d 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -547,9 +547,9 @@ extern const VMStateInfo vmstate_info_qlist;
> .version_id = (_version), \
> .num = (_num), \
> .info = &(_info), \
> - .size = sizeof(_type), \
> + .size = sizeof(_type *), \
> .flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
> - .offset = vmstate_offset_array(_state, _field, _type, _num), \
> + .offset = vmstate_offset_array(_state, _field, _type*, _num), \
> }
>
> #define VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(_f, _s, _n, _v, _vmsd, _type) { \
> @@ -1093,7 +1093,7 @@ extern const VMStateInfo vmstate_info_qlist;
> VMSTATE_TIMER_PTR_V(_f, _s, 0)
>
> #define VMSTATE_TIMER_PTR_ARRAY(_f, _s, _n) \
> - VMSTATE_ARRAY_OF_POINTER(_f, _s, _n, 0, vmstate_info_timer, QEMUTimer *)
> + VMSTATE_ARRAY_OF_POINTER(_f, _s, _n, 0, vmstate_info_timer, QEMUTimer)
>
> #define VMSTATE_TIMER_TEST(_f, _s, _test) \
> VMSTATE_SINGLE_TEST(_f, _s, _test, 0, vmstate_info_timer, QEMUTimer)
> diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> index cadbab3c5e..6a42cc1a4e 100644
> --- a/tests/unit/test-vmstate.c
> +++ b/tests/unit/test-vmstate.c
> @@ -668,7 +668,7 @@ const VMStateDescription vmsd_arpp = {
> .minimum_version_id = 1,
> .fields = (const VMStateField[]) {
> VMSTATE_ARRAY_OF_POINTER(ar, TestArrayOfPtrToInt,
> - AR_SIZE, 0, vmstate_info_int32, int32_t*),
> + AR_SIZE, 0, vmstate_info_int32, int32_t),
> VMSTATE_END_OF_LIST()
> }
> };
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 02/10] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-17 23:23 ` [PATCH RFC 01/10] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:37 ` Alexander Mikhalitsyn
2026-03-17 23:23 ` [PATCH RFC 03/10] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
` (7 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
Passing in a pointer almost never helps. Convert it to pass in struct for
further refactoring on VMS_ARRAY_OF_POINTER.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/hw/intc/riscv_aclint.h | 6 +++---
include/migration/vmstate.h | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/hw/intc/riscv_aclint.h b/include/hw/intc/riscv_aclint.h
index 5310615cbf..0e0b98acb0 100644
--- a/include/hw/intc/riscv_aclint.h
+++ b/include/hw/intc/riscv_aclint.h
@@ -80,8 +80,8 @@ enum {
RISCV_ACLINT_SWI_SIZE = 0x4000
};
-#define VMSTATE_TIMER_PTR_VARRAY(_f, _s, _f_n) \
-VMSTATE_VARRAY_OF_POINTER_UINT32(_f, _s, _f_n, 0, vmstate_info_timer, \
- QEMUTimer *)
+#define VMSTATE_TIMER_PTR_VARRAY(_f, _s, _f_n) \
+ VMSTATE_VARRAY_OF_POINTER_UINT32(_f, _s, _f_n, 0, vmstate_info_timer, \
+ QEMUTimer)
#endif
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 8992fba57d..68fd954411 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -567,9 +567,9 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
.info = &(_info), \
- .size = sizeof(_type), \
+ .size = sizeof(_type *), \
.flags = VMS_VARRAY_UINT32 | VMS_ARRAY_OF_POINTER | VMS_POINTER, \
- .offset = vmstate_offset_pointer(_state, _field, _type), \
+ .offset = vmstate_offset_pointer(_state, _field, _type *), \
}
#define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 02/10] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
2026-03-17 23:23 ` [PATCH RFC 02/10] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
@ 2026-03-18 9:37 ` Alexander Mikhalitsyn
0 siblings, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:37 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> Passing in a pointer almost never helps. Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/hw/intc/riscv_aclint.h | 6 +++---
> include/migration/vmstate.h | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/intc/riscv_aclint.h b/include/hw/intc/riscv_aclint.h
> index 5310615cbf..0e0b98acb0 100644
> --- a/include/hw/intc/riscv_aclint.h
> +++ b/include/hw/intc/riscv_aclint.h
> @@ -80,8 +80,8 @@ enum {
> RISCV_ACLINT_SWI_SIZE = 0x4000
> };
>
> -#define VMSTATE_TIMER_PTR_VARRAY(_f, _s, _f_n) \
> -VMSTATE_VARRAY_OF_POINTER_UINT32(_f, _s, _f_n, 0, vmstate_info_timer, \
> - QEMUTimer *)
> +#define VMSTATE_TIMER_PTR_VARRAY(_f, _s, _f_n) \
> + VMSTATE_VARRAY_OF_POINTER_UINT32(_f, _s, _f_n, 0, vmstate_info_timer, \
> + QEMUTimer)
>
> #endif
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 8992fba57d..68fd954411 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -567,9 +567,9 @@ extern const VMStateInfo vmstate_info_qlist;
> .version_id = (_version), \
> .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> .info = &(_info), \
> - .size = sizeof(_type), \
> + .size = sizeof(_type *), \
> .flags = VMS_VARRAY_UINT32 | VMS_ARRAY_OF_POINTER | VMS_POINTER, \
> - .offset = vmstate_offset_pointer(_state, _field, _type), \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> }
>
> #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 03/10] vmstate: Do not set size for VMS_ARRAY_OF_POINTER
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-17 23:23 ` [PATCH RFC 01/10] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
2026-03-17 23:23 ` [PATCH RFC 02/10] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:37 ` Alexander Mikhalitsyn
2026-03-17 23:23 ` [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field Peter Xu
` (6 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
When VMS_ARRAY_OF_POINTER is specified, it means the vmstate field is an
array of pointers.
The size of the element is not relevant to whatever it is stored inside: it
is always the host pointer size.
Let's reserve the "size" field in this case for future use, update
vmstate_size() so as to make it still work for array of pointers properly.
When at this, provide rich documentation on how size / size_offset works in
vmstate.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 20 ++++++++++++++++----
migration/savevm.c | 3 +++
migration/vmstate.c | 10 +++++++++-
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 68fd954411..b4bc69486d 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -183,11 +183,26 @@ typedef enum {
struct VMStateField {
const char *name;
size_t offset;
+
+ /*
+ * @size or @size_offset specifies the size of the element embeded in
+ * the field. Only one of them should be present never both. When
+ * @size_offset is used together with VMS_VBUFFER, it means the size is
+ * dynamic calculated instead of a constant.
+ *
+ * When the field is an array of any type, this stores the size of one
+ * element of the array.
+ *
+ * NOTE: even if VMS_POINTER or VMS_ARRAY_OF_POINTER may be specified,
+ * this parameter always reflects the real size of the objects that a
+ * pointer point to.
+ */
size_t size;
+ size_t size_offset;
+
size_t start;
int num;
size_t num_offset;
- size_t size_offset;
const VMStateInfo *info;
enum VMStateFlags flags;
const VMStateDescription *vmsd;
@@ -547,7 +562,6 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num = (_num), \
.info = &(_info), \
- .size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
.offset = vmstate_offset_array(_state, _field, _type*, _num), \
}
@@ -557,7 +571,6 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_v), \
.num = (_n), \
.vmsd = &(_vmsd), \
- .size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
.offset = vmstate_offset_array(_s, _f, _type*, _n), \
}
@@ -567,7 +580,6 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
.info = &(_info), \
- .size = sizeof(_type *), \
.flags = VMS_VARRAY_UINT32 | VMS_ARRAY_OF_POINTER | VMS_POINTER, \
.offset = vmstate_offset_pointer(_state, _field, _type *), \
}
diff --git a/migration/savevm.c b/migration/savevm.c
index 8115203b51..f5a6fd0c66 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -868,6 +868,9 @@ static void vmstate_check(const VMStateDescription *vmsd)
if (field) {
while (field->name) {
+ if (field->flags & VMS_ARRAY_OF_POINTER) {
+ assert(field->size == 0);
+ }
if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
/* Recurse to sub structures */
vmstate_check(field->vmsd);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index e98b5f5346..e29a8c3f49 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -110,13 +110,21 @@ static int vmstate_n_elems(void *opaque, const VMStateField *field)
static int vmstate_size(void *opaque, const VMStateField *field)
{
- int size = field->size;
+ int size;
if (field->flags & VMS_VBUFFER) {
size = *(int32_t *)(opaque + field->size_offset);
if (field->flags & VMS_MULTIPLY) {
size *= field->size;
}
+ } else if (field->flags & VMS_ARRAY_OF_POINTER) {
+ /*
+ * For an array of pointer, the each element is always size of a
+ * host pointer.
+ */
+ size = sizeof(void *);
+ } else {
+ size = field->size;
}
return size;
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 03/10] vmstate: Do not set size for VMS_ARRAY_OF_POINTER
2026-03-17 23:23 ` [PATCH RFC 03/10] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
@ 2026-03-18 9:37 ` Alexander Mikhalitsyn
0 siblings, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:37 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> When VMS_ARRAY_OF_POINTER is specified, it means the vmstate field is an
> array of pointers.
>
> The size of the element is not relevant to whatever it is stored inside: it
> is always the host pointer size.
>
> Let's reserve the "size" field in this case for future use, update
> vmstate_size() so as to make it still work for array of pointers properly.
>
> When at this, provide rich documentation on how size / size_offset works in
> vmstate.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 20 ++++++++++++++++----
> migration/savevm.c | 3 +++
> migration/vmstate.c | 10 +++++++++-
> 3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 68fd954411..b4bc69486d 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -183,11 +183,26 @@ typedef enum {
> struct VMStateField {
> const char *name;
> size_t offset;
> +
> + /*
> + * @size or @size_offset specifies the size of the element embeded in
> + * the field. Only one of them should be present never both. When
> + * @size_offset is used together with VMS_VBUFFER, it means the size is
> + * dynamic calculated instead of a constant.
> + *
> + * When the field is an array of any type, this stores the size of one
> + * element of the array.
> + *
> + * NOTE: even if VMS_POINTER or VMS_ARRAY_OF_POINTER may be specified,
> + * this parameter always reflects the real size of the objects that a
> + * pointer point to.
> + */
> size_t size;
> + size_t size_offset;
> +
> size_t start;
> int num;
> size_t num_offset;
> - size_t size_offset;
> const VMStateInfo *info;
> enum VMStateFlags flags;
> const VMStateDescription *vmsd;
> @@ -547,7 +562,6 @@ extern const VMStateInfo vmstate_info_qlist;
> .version_id = (_version), \
> .num = (_num), \
> .info = &(_info), \
> - .size = sizeof(_type *), \
> .flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
> .offset = vmstate_offset_array(_state, _field, _type*, _num), \
> }
> @@ -557,7 +571,6 @@ extern const VMStateInfo vmstate_info_qlist;
> .version_id = (_v), \
> .num = (_n), \
> .vmsd = &(_vmsd), \
> - .size = sizeof(_type *), \
> .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> }
> @@ -567,7 +580,6 @@ extern const VMStateInfo vmstate_info_qlist;
> .version_id = (_version), \
> .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> .info = &(_info), \
> - .size = sizeof(_type *), \
> .flags = VMS_VARRAY_UINT32 | VMS_ARRAY_OF_POINTER | VMS_POINTER, \
> .offset = vmstate_offset_pointer(_state, _field, _type *), \
> }
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 8115203b51..f5a6fd0c66 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -868,6 +868,9 @@ static void vmstate_check(const VMStateDescription *vmsd)
>
> if (field) {
> while (field->name) {
> + if (field->flags & VMS_ARRAY_OF_POINTER) {
> + assert(field->size == 0);
> + }
> if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> /* Recurse to sub structures */
> vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index e98b5f5346..e29a8c3f49 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -110,13 +110,21 @@ static int vmstate_n_elems(void *opaque, const VMStateField *field)
>
> static int vmstate_size(void *opaque, const VMStateField *field)
> {
> - int size = field->size;
> + int size;
>
> if (field->flags & VMS_VBUFFER) {
> size = *(int32_t *)(opaque + field->size_offset);
> if (field->flags & VMS_MULTIPLY) {
> size *= field->size;
> }
> + } else if (field->flags & VMS_ARRAY_OF_POINTER) {
> + /*
> + * For an array of pointer, the each element is always size of a
> + * host pointer.
> + */
> + size = sizeof(void *);
> + } else {
> + size = field->size;
> }
>
> return size;
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (2 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 03/10] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:43 ` Alexander Mikhalitsyn
2026-03-26 19:27 ` Peter Xu
2026-03-17 23:23 ` [PATCH RFC 05/10] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
` (5 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
We have a trick in vmstate_save_vmsd_v() where we will try to compress
multiple JSON vmstate field dumps into one line with a count to avoid
duplicated entries.
That only applies to the cases where vmsd_can_compress() should return
true. For example, vmsd_desc_field_start() later (who will take the
updated max_elems as the last parameter) will ignore the value passed in
when vmsd_can_compress() returns false.
Add that check to the trick too, it will be used later to bypass this logic
to some special new VMSD fields.
No functional change intended in this patch alone.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index e29a8c3f49..caa7b50bce 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -578,7 +578,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
/*
- * This logic only matters when dumping VM Desc.
+ * This logic only matters when dumping VM Desc, and only
+ * when the VMSD field can be compressed.
*
* Due to the fake nullptr handling above, if there's mixed
* null/non-null data, it doesn't make sense to emit a
@@ -587,7 +588,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
* vs. nullptr). Search ahead for the next null/non-null element
* and start a new compressed array if found.
*/
- if (vmdesc && (field->flags & VMS_ARRAY_OF_POINTER) &&
+ if (vmdesc && vmsd_can_compress(field) &&
+ (field->flags & VMS_ARRAY_OF_POINTER) &&
is_null != is_prev_null) {
is_prev_null = is_null;
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field
2026-03-17 23:23 ` [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field Peter Xu
@ 2026-03-18 9:43 ` Alexander Mikhalitsyn
2026-03-26 19:27 ` Peter Xu
1 sibling, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:43 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> We have a trick in vmstate_save_vmsd_v() where we will try to compress
> multiple JSON vmstate field dumps into one line with a count to avoid
> duplicated entries.
>
> That only applies to the cases where vmsd_can_compress() should return
> true. For example, vmsd_desc_field_start() later (who will take the
> updated max_elems as the last parameter) will ignore the value passed in
> when vmsd_can_compress() returns false.
>
> Add that check to the trick too, it will be used later to bypass this logic
> to some special new VMSD fields.
>
> No functional change intended in this patch alone.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> migration/vmstate.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index e29a8c3f49..caa7b50bce 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -578,7 +578,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> /*
> - * This logic only matters when dumping VM Desc.
> + * This logic only matters when dumping VM Desc, and only
> + * when the VMSD field can be compressed.
> *
> * Due to the fake nullptr handling above, if there's mixed
> * null/non-null data, it doesn't make sense to emit a
> @@ -587,7 +588,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> * vs. nullptr). Search ahead for the next null/non-null element
> * and start a new compressed array if found.
> */
> - if (vmdesc && (field->flags & VMS_ARRAY_OF_POINTER) &&
> + if (vmdesc && vmsd_can_compress(field) &&
> + (field->flags & VMS_ARRAY_OF_POINTER) &&
> is_null != is_prev_null) {
>
> is_prev_null = is_null;
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field
2026-03-17 23:23 ` [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field Peter Xu
2026-03-18 9:43 ` Alexander Mikhalitsyn
@ 2026-03-26 19:27 ` Peter Xu
1 sibling, 0 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-26 19:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, Fabiano Rosas
On Tue, Mar 17, 2026 at 07:23:26PM -0400, Peter Xu wrote:
> We have a trick in vmstate_save_vmsd_v() where we will try to compress
> multiple JSON vmstate field dumps into one line with a count to avoid
> duplicated entries.
>
> That only applies to the cases where vmsd_can_compress() should return
> true. For example, vmsd_desc_field_start() later (who will take the
> updated max_elems as the last parameter) will ignore the value passed in
> when vmsd_can_compress() returns false.
>
> Add that check to the trick too, it will be used later to bypass this logic
> to some special new VMSD fields.
>
> No functional change intended in this patch alone.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/vmstate.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index e29a8c3f49..caa7b50bce 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -578,7 +578,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> /*
> - * This logic only matters when dumping VM Desc.
> + * This logic only matters when dumping VM Desc, and only
> + * when the VMSD field can be compressed.
> *
> * Due to the fake nullptr handling above, if there's mixed
> * null/non-null data, it doesn't make sense to emit a
> @@ -587,7 +588,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> * vs. nullptr). Search ahead for the next null/non-null element
> * and start a new compressed array if found.
> */
> - if (vmdesc && (field->flags & VMS_ARRAY_OF_POINTER) &&
> + if (vmdesc && vmsd_can_compress(field) &&
> + (field->flags & VMS_ARRAY_OF_POINTER) &&
> is_null != is_prev_null) {
>
> is_prev_null = is_null;
> --
> 2.50.1
>
This patch unfortunately stops working when I start to test
analyze-migration.py. I plan to use another similar patch below to replace
this one:
From e72df6470e64715caaf3857a899fe63086bf4628 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Tue, 17 Mar 2026 19:23:26 -0400
Subject: [PATCH] vmstate: Update max_elems early and check field compressable
once
QEMU has a trick in vmstate_save_vmsd_v(), where it will try to compress
multiple JSON entries into one with a count to avoid duplicated entries.
That only applies to the cases where vmsd_can_compress() should return
true. For example, vmsd_desc_field_start() later (who will take the
updated max_elems as the last parameter) will ignore the value passed in
when vmsd_can_compress() returns false.
Do that check once at the start of loop, and use it to update max_elems, so
that max_elems keeps 1 for uncompressable VMSD fields, which is more
straightforward.
This also paves way to make this counter work for ptr marker VMSD fields
too.
No functional change intended in this patch alone.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index e29a8c3f49..05badef42f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -556,7 +556,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *curr_elem = first_elem + size * i;
const VMStateField *inner_field;
bool is_null;
- int max_elems = n_elems - i;
+ /* maximum number of elements to compress in the JSON blob */
+ int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
old_offset = qemu_file_transferred(f);
if (field->flags & VMS_ARRAY_OF_POINTER) {
@@ -587,7 +588,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
* vs. nullptr). Search ahead for the next null/non-null element
* and start a new compressed array if found.
*/
- if (vmdesc && (field->flags & VMS_ARRAY_OF_POINTER) &&
+ if (vmdesc && max_elems > 1 &&
+ (field->flags & VMS_ARRAY_OF_POINTER) &&
is_null != is_prev_null) {
is_prev_null = is_null;
@@ -626,7 +628,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
/* Compressed arrays only care about the first element */
- if (vmdesc_loop && vmsd_can_compress(field)) {
+ if (vmdesc_loop && max_elems > 1) {
vmdesc_loop = NULL;
}
}
--
2.50.1
--
Peter Xu
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH RFC 05/10] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (3 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 04/10] vmstate: Limit the vmdesc_loop dedup trick to compressable field Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:38 ` Alexander Mikhalitsyn
2026-03-17 23:23 ` [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc() Peter Xu
` (4 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
Prepare for a new MARKER for non-NULL pointer.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 2 +-
migration/vmstate-types.c | 4 ++--
tests/unit/test-vmstate.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index b4bc69486d..092e8f7e9a 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -283,7 +283,7 @@ extern const VMStateInfo vmstate_info_uint64;
extern const VMStateInfo vmstate_info_fd;
/** Put this in the stream when migrating a null pointer.*/
-#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
+#define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
extern const VMStateInfo vmstate_info_nullptr;
extern const VMStateInfo vmstate_info_cpudouble;
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 23f3433696..7622cf8f01 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -363,7 +363,7 @@ static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
const VMStateField *field, Error **errp)
{
- if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
+ if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
return true;
}
@@ -377,7 +377,7 @@ static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
{
if (pv == NULL) {
- qemu_put_byte(f, VMS_NULLPTR_MARKER);
+ qemu_put_byte(f, VMS_MARKER_PTR_NULL);
return true;
}
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index 6a42cc1a4e..dae15786aa 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -620,7 +620,7 @@ static void test_arr_ptr_str_no0_load(void)
static uint8_t wire_arr_ptr_0[] = {
0x00, 0x00, 0x00, 0x00,
- VMS_NULLPTR_MARKER,
+ VMS_MARKER_PTR_NULL,
0x00, 0x00, 0x00, 0x02,
0x00, 0x00, 0x00, 0x03,
QEMU_VM_EOF
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 05/10] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
2026-03-17 23:23 ` [PATCH RFC 05/10] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
@ 2026-03-18 9:38 ` Alexander Mikhalitsyn
0 siblings, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:38 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> Prepare for a new MARKER for non-NULL pointer.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 2 +-
> migration/vmstate-types.c | 4 ++--
> tests/unit/test-vmstate.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index b4bc69486d..092e8f7e9a 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -283,7 +283,7 @@ extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> /** Put this in the stream when migrating a null pointer.*/
> -#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
> +#define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> extern const VMStateInfo vmstate_info_nullptr;
>
> extern const VMStateInfo vmstate_info_cpudouble;
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 23f3433696..7622cf8f01 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,7 +363,7 @@ static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
> + if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> return true;
> }
>
> @@ -377,7 +377,7 @@ static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
>
> {
> if (pv == NULL) {
> - qemu_put_byte(f, VMS_NULLPTR_MARKER);
> + qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> return true;
> }
>
> diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> index 6a42cc1a4e..dae15786aa 100644
> --- a/tests/unit/test-vmstate.c
> +++ b/tests/unit/test-vmstate.c
> @@ -620,7 +620,7 @@ static void test_arr_ptr_str_no0_load(void)
>
> static uint8_t wire_arr_ptr_0[] = {
> 0x00, 0x00, 0x00, 0x00,
> - VMS_NULLPTR_MARKER,
> + VMS_MARKER_PTR_NULL,
> 0x00, 0x00, 0x00, 0x02,
> 0x00, 0x00, 0x00, 0x03,
> QEMU_VM_EOF
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc()
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (4 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 05/10] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:39 ` Alexander Mikhalitsyn
2026-03-19 20:36 ` Fabiano Rosas
2026-03-17 23:23 ` [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
` (3 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
Introduce a helper to do both the JSON blob dump and vmstate dump. This
further shrinks the function a bit. More importantly, we'll need to dump
two fields in one loop very soon in the future with the JSON blob.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate.c | 38 +++++++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index caa7b50bce..5a6b352764 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -514,6 +514,30 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
return true;
}
+/*
+ * Dump a whole VMSD field, including its JSON blob separately when @vmdesc
+ * is specified.
+ */
+static inline bool
+vmstate_save_field_with_vmdesc(QEMUFile *f, void *pv, size_t size,
+ const VMStateDescription *vmsd,
+ const VMStateField *field, JSONWriter *vmdesc,
+ int i, int max, Error **errp)
+{
+ uint64_t old_offset, written_bytes;
+ bool ok;
+
+ vmsd_desc_field_start(vmsd, vmdesc, field, i, max);
+
+ old_offset = qemu_file_transferred(f);
+ ok = vmstate_save_field(f, pv, size, field, vmdesc, errp);
+ written_bytes = qemu_file_transferred(f) - old_offset;
+
+ vmsd_desc_field_end(vmsd, vmdesc, field, written_bytes);
+
+ return ok;
+}
+
static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp)
@@ -542,7 +566,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *first_elem = opaque + field->offset;
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
- uint64_t old_offset, written_bytes;
JSONWriter *vmdesc_loop = vmdesc;
bool is_prev_null = false;
@@ -558,7 +581,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
bool is_null;
int max_elems = n_elems - i;
- old_offset = qemu_file_transferred(f);
if (field->flags & VMS_ARRAY_OF_POINTER) {
assert(curr_elem);
curr_elem = *(void **)curr_elem;
@@ -606,15 +628,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
}
- vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
- i, max_elems);
-
- ok = vmstate_save_field(f, curr_elem, size, inner_field,
- vmdesc_loop, errp);
-
- written_bytes = qemu_file_transferred(f) - old_offset;
- vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
- written_bytes);
+ ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
+ inner_field, vmdesc_loop,
+ i, max_elems, errp);
/* If we used a fake temp field.. free it now */
if (is_null) {
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc()
2026-03-17 23:23 ` [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc() Peter Xu
@ 2026-03-18 9:39 ` Alexander Mikhalitsyn
2026-03-19 20:36 ` Fabiano Rosas
1 sibling, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:39 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> Introduce a helper to do both the JSON blob dump and vmstate dump. This
> further shrinks the function a bit. More importantly, we'll need to dump
> two fields in one loop very soon in the future with the JSON blob.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> migration/vmstate.c | 38 +++++++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index caa7b50bce..5a6b352764 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -514,6 +514,30 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
> return true;
> }
>
> +/*
> + * Dump a whole VMSD field, including its JSON blob separately when @vmdesc
> + * is specified.
> + */
> +static inline bool
> +vmstate_save_field_with_vmdesc(QEMUFile *f, void *pv, size_t size,
> + const VMStateDescription *vmsd,
> + const VMStateField *field, JSONWriter *vmdesc,
> + int i, int max, Error **errp)
> +{
> + uint64_t old_offset, written_bytes;
> + bool ok;
> +
> + vmsd_desc_field_start(vmsd, vmdesc, field, i, max);
> +
> + old_offset = qemu_file_transferred(f);
> + ok = vmstate_save_field(f, pv, size, field, vmdesc, errp);
> + written_bytes = qemu_file_transferred(f) - old_offset;
> +
> + vmsd_desc_field_end(vmsd, vmdesc, field, written_bytes);
> +
> + return ok;
> +}
> +
> static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc,
> int version_id, Error **errp)
> @@ -542,7 +566,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> - uint64_t old_offset, written_bytes;
> JSONWriter *vmdesc_loop = vmdesc;
> bool is_prev_null = false;
>
> @@ -558,7 +581,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> bool is_null;
> int max_elems = n_elems - i;
>
> - old_offset = qemu_file_transferred(f);
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> assert(curr_elem);
> curr_elem = *(void **)curr_elem;
> @@ -606,15 +628,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
> }
>
> - vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
> - i, max_elems);
> -
> - ok = vmstate_save_field(f, curr_elem, size, inner_field,
> - vmdesc_loop, errp);
> -
> - written_bytes = qemu_file_transferred(f) - old_offset;
> - vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
> - written_bytes);
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
> + inner_field, vmdesc_loop,
> + i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> if (is_null) {
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc()
2026-03-17 23:23 ` [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc() Peter Xu
2026-03-18 9:39 ` Alexander Mikhalitsyn
@ 2026-03-19 20:36 ` Fabiano Rosas
1 sibling, 0 replies; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-19 20:36 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx
Peter Xu <peterx@redhat.com> writes:
> Introduce a helper to do both the JSON blob dump and vmstate dump. This
> further shrinks the function a bit. More importantly, we'll need to dump
> two fields in one loop very soon in the future with the JSON blob.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/vmstate.c | 38 +++++++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index caa7b50bce..5a6b352764 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -514,6 +514,30 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
> return true;
> }
>
> +/*
> + * Dump a whole VMSD field, including its JSON blob separately when @vmdesc
Could do without the "dump" terminology all over the series. There's
already use for that term due to -dump-vmstate.
> + * is specified.
> + */
> +static inline bool
> +vmstate_save_field_with_vmdesc(QEMUFile *f, void *pv, size_t size,
> + const VMStateDescription *vmsd,
> + const VMStateField *field, JSONWriter *vmdesc,
> + int i, int max, Error **errp)
> +{
> + uint64_t old_offset, written_bytes;
> + bool ok;
> +
> + vmsd_desc_field_start(vmsd, vmdesc, field, i, max);
> +
> + old_offset = qemu_file_transferred(f);
> + ok = vmstate_save_field(f, pv, size, field, vmdesc, errp);
> + written_bytes = qemu_file_transferred(f) - old_offset;
> +
> + vmsd_desc_field_end(vmsd, vmdesc, field, written_bytes);
> +
> + return ok;
> +}
> +
> static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc,
> int version_id, Error **errp)
> @@ -542,7 +566,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> - uint64_t old_offset, written_bytes;
> JSONWriter *vmdesc_loop = vmdesc;
> bool is_prev_null = false;
>
> @@ -558,7 +581,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> bool is_null;
> int max_elems = n_elems - i;
>
> - old_offset = qemu_file_transferred(f);
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> assert(curr_elem);
> curr_elem = *(void **)curr_elem;
> @@ -606,15 +628,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
> }
>
> - vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
> - i, max_elems);
> -
> - ok = vmstate_save_field(f, curr_elem, size, inner_field,
> - vmdesc_loop, errp);
> -
> - written_bytes = qemu_file_transferred(f) - old_offset;
> - vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
> - written_bytes);
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
> + inner_field, vmdesc_loop,
> + i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> if (is_null) {
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (5 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 06/10] vmstate: Introduce vmstate_save_field_with_vmdesc() Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:40 ` Alexander Mikhalitsyn
2026-03-19 20:46 ` Fabiano Rosas
2026-03-17 23:23 ` [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core Peter Xu
` (2 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
We used to have one vmstate called "nullptr" which is only used to generate
one-byte hint to say one pointer is NULL.
Let's extend its use so that it will generate another byte to say the
pointer is non-NULL.
With that, the name of the info struct (or functions) do not apply anymore.
Update correspondingly.
No functional change intended yet.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 9 +++++++--
migration/vmstate-types.c | 34 ++++++++++++++++------------------
migration/vmstate.c | 29 ++++++++++++++---------------
3 files changed, 37 insertions(+), 35 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 092e8f7e9a..2e51b5ea04 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
extern const VMStateInfo vmstate_info_uint64;
extern const VMStateInfo vmstate_info_fd;
-/** Put this in the stream when migrating a null pointer.*/
+/*
+ * Put this in the stream when migrating a pointer to reflect either a NULL
+ * or valid pointer.
+ */
#define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
-extern const VMStateInfo vmstate_info_nullptr;
+#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
+
+extern const VMStateInfo vmstate_info_ptr_marker;
extern const VMStateInfo vmstate_info_cpudouble;
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 7622cf8f01..b31689fc3c 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
.save = save_fd,
};
-static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, Error **errp)
+static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
- if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
+ int byte = qemu_get_byte(f);
+
+ if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
+ /* TODO: process PTR_VALID case */
return true;
}
- error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
+ error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
return false;
}
-static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc,
- Error **errp)
+static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
- if (pv == NULL) {
- qemu_put_byte(f, VMS_MARKER_PTR_NULL);
- return true;
- }
-
- error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
- return false;
+ qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
+ return true;
}
-const VMStateInfo vmstate_info_nullptr = {
- .name = "nullptr",
- .load = load_nullptr,
- .save = save_nullptr,
+const VMStateInfo vmstate_info_ptr_marker = {
+ .name = "ptr-marker",
+ .load = load_ptr_marker,
+ .save = save_ptr_marker,
};
/* 64 bit unsigned int. See that the received value is the same than the one
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 5a6b352764..a3a5f25946 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
}
/*
- * Create a fake nullptr field when there's a NULL pointer detected in the
+ * Create a ptr marker field when there's a NULL pointer detected in the
* array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
* can't dereference the NULL pointer.
*/
static const VMStateField *
-vmsd_create_fake_nullptr_field(const VMStateField *field)
+vmsd_create_ptr_marker_field(const VMStateField *field)
{
VMStateField *fake = g_new0(VMStateField, 1);
@@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
/* See vmstate_info_nullptr - use 1 byte to represent nullptr */
fake->size = 1;
- fake->info = &vmstate_info_nullptr;
+ fake->info = &vmstate_info_ptr_marker;
fake->flags = VMS_SINGLE;
/* All the rest fields shouldn't matter.. */
@@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
* an array of pointers), use null placeholder and do
* not follow.
*/
- inner_field = vmsd_create_fake_nullptr_field(field);
+ inner_field = vmsd_create_ptr_marker_field(field);
} else {
inner_field = field;
}
@@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
JSONWriter *vmdesc_loop = vmdesc;
- bool is_prev_null = false;
+ bool use_marker_field_prev = false;
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
@@ -578,7 +578,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
const VMStateField *inner_field;
- bool is_null;
+ bool use_marker_field;
int max_elems = n_elems - i;
if (field->flags & VMS_ARRAY_OF_POINTER) {
@@ -586,17 +586,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
curr_elem = *(void **)curr_elem;
}
- if (!curr_elem && size) {
+ use_marker_field = !curr_elem && size;
+ if (use_marker_field) {
/*
* If null pointer found (which should only happen in
* an array of pointers), use null placeholder and do
* not follow.
*/
- inner_field = vmsd_create_fake_nullptr_field(field);
- is_null = true;
+ inner_field = vmsd_create_ptr_marker_field(field);
} else {
inner_field = field;
- is_null = false;
}
/*
@@ -612,16 +611,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
*/
if (vmdesc && vmsd_can_compress(field) &&
(field->flags & VMS_ARRAY_OF_POINTER) &&
- is_null != is_prev_null) {
+ use_marker_field != use_marker_field_prev) {
- is_prev_null = is_null;
+ use_marker_field_prev = use_marker_field;
vmdesc_loop = vmdesc;
for (int j = i + 1; j < n_elems; j++) {
void *elem = *(void **)(first_elem + size * j);
- bool elem_is_null = !elem && size;
+ bool elem_use_marker_field = !elem && size;
- if (is_null != elem_is_null) {
+ if (use_marker_field != elem_use_marker_field) {
max_elems = j - i;
break;
}
@@ -633,7 +632,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
i, max_elems, errp);
/* If we used a fake temp field.. free it now */
- if (is_null) {
+ if (use_marker_field) {
g_clear_pointer((gpointer *)&inner_field, g_free);
}
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-17 23:23 ` [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
@ 2026-03-18 9:40 ` Alexander Mikhalitsyn
2026-03-19 20:46 ` Fabiano Rosas
1 sibling, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:40 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> We used to have one vmstate called "nullptr" which is only used to generate
> one-byte hint to say one pointer is NULL.
>
> Let's extend its use so that it will generate another byte to say the
> pointer is non-NULL.
>
> With that, the name of the info struct (or functions) do not apply anymore.
> Update correspondingly.
>
> No functional change intended yet.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 9 +++++++--
> migration/vmstate-types.c | 34 ++++++++++++++++------------------
> migration/vmstate.c | 29 ++++++++++++++---------------
> 3 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 092e8f7e9a..2e51b5ea04 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
> extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> -/** Put this in the stream when migrating a null pointer.*/
> +/*
> + * Put this in the stream when migrating a pointer to reflect either a NULL
> + * or valid pointer.
> + */
> #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> -extern const VMStateInfo vmstate_info_nullptr;
> +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
> +
> +extern const VMStateInfo vmstate_info_ptr_marker;
>
> extern const VMStateInfo vmstate_info_cpudouble;
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 7622cf8f01..b31689fc3c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
> .save = save_fd,
> };
>
> -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, Error **errp)
> +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> + /* TODO: process PTR_VALID case */
> return true;
> }
>
> - error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> return false;
> }
>
> -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc,
> - Error **errp)
> +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
>
> {
> - if (pv == NULL) {
> - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> - return true;
> - }
> -
> - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> - return false;
> + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> + return true;
> }
>
> -const VMStateInfo vmstate_info_nullptr = {
> - .name = "nullptr",
> - .load = load_nullptr,
> - .save = save_nullptr,
> +const VMStateInfo vmstate_info_ptr_marker = {
> + .name = "ptr-marker",
> + .load = load_ptr_marker,
> + .save = save_ptr_marker,
> };
>
> /* 64 bit unsigned int. See that the received value is the same than the one
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5a6b352764..a3a5f25946 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> }
>
> /*
> - * Create a fake nullptr field when there's a NULL pointer detected in the
> + * Create a ptr marker field when there's a NULL pointer detected in the
> * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
> * can't dereference the NULL pointer.
> */
> static const VMStateField *
> -vmsd_create_fake_nullptr_field(const VMStateField *field)
> +vmsd_create_ptr_marker_field(const VMStateField *field)
> {
> VMStateField *fake = g_new0(VMStateField, 1);
>
> @@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
>
> /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
> fake->size = 1;
> - fake->info = &vmstate_info_nullptr;
> + fake->info = &vmstate_info_ptr_marker;
> fake->flags = VMS_SINGLE;
>
> /* All the rest fields shouldn't matter.. */
> @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> }
> @@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> - bool is_prev_null = false;
> + bool use_marker_field_prev = false;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -578,7 +578,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> - bool is_null;
> + bool use_marker_field;
> int max_elems = n_elems - i;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> @@ -586,17 +586,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> curr_elem = *(void **)curr_elem;
> }
>
> - if (!curr_elem && size) {
> + use_marker_field = !curr_elem && size;
> + if (use_marker_field) {
> /*
> * If null pointer found (which should only happen in
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> - is_null = true;
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> - is_null = false;
> }
>
> /*
> @@ -612,16 +611,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> */
> if (vmdesc && vmsd_can_compress(field) &&
> (field->flags & VMS_ARRAY_OF_POINTER) &&
> - is_null != is_prev_null) {
> + use_marker_field != use_marker_field_prev) {
>
> - is_prev_null = is_null;
> + use_marker_field_prev = use_marker_field;
> vmdesc_loop = vmdesc;
>
> for (int j = i + 1; j < n_elems; j++) {
> void *elem = *(void **)(first_elem + size * j);
> - bool elem_is_null = !elem && size;
> + bool elem_use_marker_field = !elem && size;
>
> - if (is_null != elem_is_null) {
> + if (use_marker_field != elem_use_marker_field) {
> max_elems = j - i;
> break;
> }
> @@ -633,7 +632,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> - if (is_null) {
> + if (use_marker_field) {
> g_clear_pointer((gpointer *)&inner_field, g_free);
> }
>
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-17 23:23 ` [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
2026-03-18 9:40 ` Alexander Mikhalitsyn
@ 2026-03-19 20:46 ` Fabiano Rosas
2026-03-26 19:25 ` Peter Xu
1 sibling, 1 reply; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-19 20:46 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx
Peter Xu <peterx@redhat.com> writes:
> We used to have one vmstate called "nullptr" which is only used to generate
> one-byte hint to say one pointer is NULL.
>
> Let's extend its use so that it will generate another byte to say the
> pointer is non-NULL.
>
> With that, the name of the info struct (or functions) do not apply anymore.
> Update correspondingly.
>
> No functional change intended yet.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/migration/vmstate.h | 9 +++++++--
> migration/vmstate-types.c | 34 ++++++++++++++++------------------
> migration/vmstate.c | 29 ++++++++++++++---------------
> 3 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 092e8f7e9a..2e51b5ea04 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
> extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> -/** Put this in the stream when migrating a null pointer.*/
> +/*
> + * Put this in the stream when migrating a pointer to reflect either a NULL
> + * or valid pointer.
> + */
> #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> -extern const VMStateInfo vmstate_info_nullptr;
> +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
> +
> +extern const VMStateInfo vmstate_info_ptr_marker;
>
> extern const VMStateInfo vmstate_info_cpudouble;
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 7622cf8f01..b31689fc3c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
> .save = save_fd,
> };
>
> -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, Error **errp)
> +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> + /* TODO: process PTR_VALID case */
> return true;
> }
>
> - error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> return false;
> }
>
> -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc,
> - Error **errp)
> +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
>
> {
> - if (pv == NULL) {
> - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> - return true;
> - }
> -
> - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> - return false;
> + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> + return true;
> }
>
> -const VMStateInfo vmstate_info_nullptr = {
> - .name = "nullptr",
> - .load = load_nullptr,
> - .save = save_nullptr,
> +const VMStateInfo vmstate_info_ptr_marker = {
> + .name = "ptr-marker",
> + .load = load_ptr_marker,
> + .save = save_ptr_marker,
> };
>
> /* 64 bit unsigned int. See that the received value is the same than the one
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5a6b352764..a3a5f25946 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> }
>
> /*
> - * Create a fake nullptr field when there's a NULL pointer detected in the
> + * Create a ptr marker field when there's a NULL pointer detected in the
> * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
> * can't dereference the NULL pointer.
> */
> static const VMStateField *
> -vmsd_create_fake_nullptr_field(const VMStateField *field)
> +vmsd_create_ptr_marker_field(const VMStateField *field)
> {
> VMStateField *fake = g_new0(VMStateField, 1);
>
> @@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
>
> /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
This comment needs updating.
> fake->size = 1;
> - fake->info = &vmstate_info_nullptr;
> + fake->info = &vmstate_info_ptr_marker;
> fake->flags = VMS_SINGLE;
>
> /* All the rest fields shouldn't matter.. */
> @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> }
> @@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> - bool is_prev_null = false;
> + bool use_marker_field_prev = false;
The logic below won't handle valid pointer as well, will it? We could
leave the is_null/is_prev_null terminology because it's way more
descriptive. Right? We're adding the marker here because we're
compressing and know the field is null.
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -578,7 +578,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> - bool is_null;
> + bool use_marker_field;
> int max_elems = n_elems - i;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> @@ -586,17 +586,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> curr_elem = *(void **)curr_elem;
> }
>
> - if (!curr_elem && size) {
> + use_marker_field = !curr_elem && size;
> + if (use_marker_field) {
> /*
> * If null pointer found (which should only happen in
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> - is_null = true;
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> - is_null = false;
> }
>
> /*
> @@ -612,16 +611,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> */
> if (vmdesc && vmsd_can_compress(field) &&
> (field->flags & VMS_ARRAY_OF_POINTER) &&
> - is_null != is_prev_null) {
> + use_marker_field != use_marker_field_prev) {
>
> - is_prev_null = is_null;
> + use_marker_field_prev = use_marker_field;
> vmdesc_loop = vmdesc;
>
> for (int j = i + 1; j < n_elems; j++) {
> void *elem = *(void **)(first_elem + size * j);
> - bool elem_is_null = !elem && size;
> + bool elem_use_marker_field = !elem && size;
See?
>
> - if (is_null != elem_is_null) {
> + if (use_marker_field != elem_use_marker_field) {
> max_elems = j - i;
> break;
> }
> @@ -633,7 +632,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> - if (is_null) {
> + if (use_marker_field) {
> g_clear_pointer((gpointer *)&inner_field, g_free);
> }
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-19 20:46 ` Fabiano Rosas
@ 2026-03-26 19:25 ` Peter Xu
2026-03-26 21:19 ` Fabiano Rosas
0 siblings, 1 reply; 34+ messages in thread
From: Peter Xu @ 2026-03-26 19:25 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin
On Thu, Mar 19, 2026 at 05:46:36PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > We used to have one vmstate called "nullptr" which is only used to generate
> > one-byte hint to say one pointer is NULL.
> >
> > Let's extend its use so that it will generate another byte to say the
> > pointer is non-NULL.
> >
> > With that, the name of the info struct (or functions) do not apply anymore.
> > Update correspondingly.
> >
> > No functional change intended yet.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > include/migration/vmstate.h | 9 +++++++--
> > migration/vmstate-types.c | 34 ++++++++++++++++------------------
> > migration/vmstate.c | 29 ++++++++++++++---------------
> > 3 files changed, 37 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 092e8f7e9a..2e51b5ea04 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
> > extern const VMStateInfo vmstate_info_uint64;
> > extern const VMStateInfo vmstate_info_fd;
> >
> > -/** Put this in the stream when migrating a null pointer.*/
> > +/*
> > + * Put this in the stream when migrating a pointer to reflect either a NULL
> > + * or valid pointer.
> > + */
> > #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> > -extern const VMStateInfo vmstate_info_nullptr;
> > +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
> > +
> > +extern const VMStateInfo vmstate_info_ptr_marker;
> >
> > extern const VMStateInfo vmstate_info_cpudouble;
> >
> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> > index 7622cf8f01..b31689fc3c 100644
> > --- a/migration/vmstate-types.c
> > +++ b/migration/vmstate-types.c
> > @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
> > .save = save_fd,
> > };
> >
> > -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> > - const VMStateField *field, Error **errp)
> > +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> > + const VMStateField *field, Error **errp)
> >
> > {
> > - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> > + int byte = qemu_get_byte(f);
> > +
> > + if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> > + /* TODO: process PTR_VALID case */
> > return true;
> > }
> >
> > - error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> > + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> > return false;
> > }
> >
> > -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> > - const VMStateField *field, JSONWriter *vmdesc,
> > - Error **errp)
> > +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> > + const VMStateField *field, JSONWriter *vmdesc,
> > + Error **errp)
> >
> > {
> > - if (pv == NULL) {
> > - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> > - return true;
> > - }
> > -
> > - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> > - return false;
> > + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> > + return true;
> > }
> >
> > -const VMStateInfo vmstate_info_nullptr = {
> > - .name = "nullptr",
> > - .load = load_nullptr,
> > - .save = save_nullptr,
> > +const VMStateInfo vmstate_info_ptr_marker = {
> > + .name = "ptr-marker",
> > + .load = load_ptr_marker,
> > + .save = save_ptr_marker,
> > };
> >
> > /* 64 bit unsigned int. See that the received value is the same than the one
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 5a6b352764..a3a5f25946 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> > }
> >
> > /*
> > - * Create a fake nullptr field when there's a NULL pointer detected in the
> > + * Create a ptr marker field when there's a NULL pointer detected in the
> > * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
> > * can't dereference the NULL pointer.
> > */
> > static const VMStateField *
> > -vmsd_create_fake_nullptr_field(const VMStateField *field)
> > +vmsd_create_ptr_marker_field(const VMStateField *field)
> > {
> > VMStateField *fake = g_new0(VMStateField, 1);
> >
> > @@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
> >
> > /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
>
> This comment needs updating.
Fixed.
>
> > fake->size = 1;
> > - fake->info = &vmstate_info_nullptr;
> > + fake->info = &vmstate_info_ptr_marker;
> > fake->flags = VMS_SINGLE;
> >
> > /* All the rest fields shouldn't matter.. */
> > @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> > * an array of pointers), use null placeholder and do
> > * not follow.
> > */
> > - inner_field = vmsd_create_fake_nullptr_field(field);
> > + inner_field = vmsd_create_ptr_marker_field(field);
> > } else {
> > inner_field = field;
> > }
> > @@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> > int i, n_elems = vmstate_n_elems(opaque, field);
> > int size = vmstate_size(opaque, field);
> > JSONWriter *vmdesc_loop = vmdesc;
> > - bool is_prev_null = false;
> > + bool use_marker_field_prev = false;
>
> The logic below won't handle valid pointer as well, will it? We could
> leave the is_null/is_prev_null terminology because it's way more
> descriptive. Right? We're adding the marker here because we're
> compressing and know the field is null.
I recovered the old bool for better readability.
One other note is I'll squash below analyze-migration.py change into this
patch when repost, let me know if there's early comments:
===8<===
From 82ac38a85ecd990a1aaa7b45b172e71f0d5b84da Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Thu, 26 Mar 2026 14:57:56 -0400
Subject: [PATCH] scripts/analyze-migration.py: Support new ptr-marker field
Signed-off-by: Peter Xu <peterx@redhat.com>
---
scripts/analyze-migration.py | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index e81deab8f9..1771ff781b 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -469,26 +469,26 @@ def __init__(self, desc, file):
super(VMSDFieldIntLE, self).__init__(desc, file)
self.dtype = '<i%d' % self.size
-class VMSDFieldNull(VMSDFieldGeneric):
+class VMSDFieldPtrMarker(VMSDFieldGeneric):
NULL_PTR_MARKER = b'0'
+ VALID_PTR_MARKER = b'1'
def __init__(self, desc, file):
- super(VMSDFieldNull, self).__init__(desc, file)
+ super(VMSDFieldPtrMarker, self).__init__(desc, file)
def __repr__(self):
- # A NULL pointer is encoded in the stream as a '0' to
- # disambiguate from a mere 0x0 value and avoid consumers
- # trying to follow the NULL pointer. Displaying '0', 0x30 or
- # 0x0 when analyzing the JSON debug stream could become
+ # A NULL / non-NULL pointer may be encoded in the stream as a
+ # '0'/'1' to represent the status of the pointer. Displaying '0',
+ # 0x30 or 0x0 when analyzing the JSON debug stream could become
# confusing, so use an explicit term instead.
- return "nullptr"
+ return "null-ptr" if self.data == self.NULL_PTR_MARKER else "valid-ptr"
def __str__(self):
return self.__repr__()
def read(self):
- super(VMSDFieldNull, self).read()
- assert(self.data == self.NULL_PTR_MARKER)
+ super(VMSDFieldPtrMarker, self).read()
+ assert(self.data in [self.NULL_PTR_MARKER, self.VALID_PTR_MARKER])
return self.data
class VMSDFieldBool(VMSDFieldGeneric):
@@ -642,7 +642,9 @@ def getDict(self):
"bitmap" : VMSDFieldGeneric,
"struct" : VMSDFieldStruct,
"capability": VMSDFieldCap,
- "nullptr": VMSDFieldNull,
+ # Keep the old nullptr for old binaries
+ "nullptr": VMSDFieldPtrMarker,
+ "ptr-marker": VMSDFieldPtrMarker,
"unknown" : VMSDFieldGeneric,
}
--
2.50.1
--
Peter Xu
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-26 19:25 ` Peter Xu
@ 2026-03-26 21:19 ` Fabiano Rosas
0 siblings, 0 replies; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-26 21:19 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin
Peter Xu <peterx@redhat.com> writes:
> On Thu, Mar 19, 2026 at 05:46:36PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > We used to have one vmstate called "nullptr" which is only used to generate
>> > one-byte hint to say one pointer is NULL.
>> >
>> > Let's extend its use so that it will generate another byte to say the
>> > pointer is non-NULL.
>> >
>> > With that, the name of the info struct (or functions) do not apply anymore.
>> > Update correspondingly.
>> >
>> > No functional change intended yet.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> > include/migration/vmstate.h | 9 +++++++--
>> > migration/vmstate-types.c | 34 ++++++++++++++++------------------
>> > migration/vmstate.c | 29 ++++++++++++++---------------
>> > 3 files changed, 37 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> > index 092e8f7e9a..2e51b5ea04 100644
>> > --- a/include/migration/vmstate.h
>> > +++ b/include/migration/vmstate.h
>> > @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
>> > extern const VMStateInfo vmstate_info_uint64;
>> > extern const VMStateInfo vmstate_info_fd;
>> >
>> > -/** Put this in the stream when migrating a null pointer.*/
>> > +/*
>> > + * Put this in the stream when migrating a pointer to reflect either a NULL
>> > + * or valid pointer.
>> > + */
>> > #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
>> > -extern const VMStateInfo vmstate_info_nullptr;
>> > +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
>> > +
>> > +extern const VMStateInfo vmstate_info_ptr_marker;
>> >
>> > extern const VMStateInfo vmstate_info_cpudouble;
>> >
>> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
>> > index 7622cf8f01..b31689fc3c 100644
>> > --- a/migration/vmstate-types.c
>> > +++ b/migration/vmstate-types.c
>> > @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
>> > .save = save_fd,
>> > };
>> >
>> > -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
>> > - const VMStateField *field, Error **errp)
>> > +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
>> > + const VMStateField *field, Error **errp)
>> >
>> > {
>> > - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
>> > + int byte = qemu_get_byte(f);
>> > +
>> > + if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
>> > + /* TODO: process PTR_VALID case */
>> > return true;
>> > }
>> >
>> > - error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
>> > + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
>> > return false;
>> > }
>> >
>> > -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
>> > - const VMStateField *field, JSONWriter *vmdesc,
>> > - Error **errp)
>> > +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
>> > + const VMStateField *field, JSONWriter *vmdesc,
>> > + Error **errp)
>> >
>> > {
>> > - if (pv == NULL) {
>> > - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
>> > - return true;
>> > - }
>> > -
>> > - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
>> > - return false;
>> > + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
>> > + return true;
>> > }
>> >
>> > -const VMStateInfo vmstate_info_nullptr = {
>> > - .name = "nullptr",
>> > - .load = load_nullptr,
>> > - .save = save_nullptr,
>> > +const VMStateInfo vmstate_info_ptr_marker = {
>> > + .name = "ptr-marker",
>> > + .load = load_ptr_marker,
>> > + .save = save_ptr_marker,
>> > };
>> >
>> > /* 64 bit unsigned int. See that the received value is the same than the one
>> > diff --git a/migration/vmstate.c b/migration/vmstate.c
>> > index 5a6b352764..a3a5f25946 100644
>> > --- a/migration/vmstate.c
>> > +++ b/migration/vmstate.c
>> > @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
>> > }
>> >
>> > /*
>> > - * Create a fake nullptr field when there's a NULL pointer detected in the
>> > + * Create a ptr marker field when there's a NULL pointer detected in the
>> > * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
>> > * can't dereference the NULL pointer.
>> > */
>> > static const VMStateField *
>> > -vmsd_create_fake_nullptr_field(const VMStateField *field)
>> > +vmsd_create_ptr_marker_field(const VMStateField *field)
>> > {
>> > VMStateField *fake = g_new0(VMStateField, 1);
>> >
>> > @@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
>> >
>> > /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
>>
>> This comment needs updating.
>
> Fixed.
>
>>
>> > fake->size = 1;
>> > - fake->info = &vmstate_info_nullptr;
>> > + fake->info = &vmstate_info_ptr_marker;
>> > fake->flags = VMS_SINGLE;
>> >
>> > /* All the rest fields shouldn't matter.. */
>> > @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>> > * an array of pointers), use null placeholder and do
>> > * not follow.
>> > */
>> > - inner_field = vmsd_create_fake_nullptr_field(field);
>> > + inner_field = vmsd_create_ptr_marker_field(field);
>> > } else {
>> > inner_field = field;
>> > }
>> > @@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>> > int i, n_elems = vmstate_n_elems(opaque, field);
>> > int size = vmstate_size(opaque, field);
>> > JSONWriter *vmdesc_loop = vmdesc;
>> > - bool is_prev_null = false;
>> > + bool use_marker_field_prev = false;
>>
>> The logic below won't handle valid pointer as well, will it? We could
>> leave the is_null/is_prev_null terminology because it's way more
>> descriptive. Right? We're adding the marker here because we're
>> compressing and know the field is null.
>
> I recovered the old bool for better readability.
>
> One other note is I'll squash below analyze-migration.py change into this
> patch when repost, let me know if there's early comments:
>
> ===8<===
>
> From 82ac38a85ecd990a1aaa7b45b172e71f0d5b84da Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 26 Mar 2026 14:57:56 -0400
> Subject: [PATCH] scripts/analyze-migration.py: Support new ptr-marker field
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> scripts/analyze-migration.py | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index e81deab8f9..1771ff781b 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -469,26 +469,26 @@ def __init__(self, desc, file):
> super(VMSDFieldIntLE, self).__init__(desc, file)
> self.dtype = '<i%d' % self.size
>
> -class VMSDFieldNull(VMSDFieldGeneric):
> +class VMSDFieldPtrMarker(VMSDFieldGeneric):
> NULL_PTR_MARKER = b'0'
> + VALID_PTR_MARKER = b'1'
>
> def __init__(self, desc, file):
> - super(VMSDFieldNull, self).__init__(desc, file)
> + super(VMSDFieldPtrMarker, self).__init__(desc, file)
>
> def __repr__(self):
> - # A NULL pointer is encoded in the stream as a '0' to
> - # disambiguate from a mere 0x0 value and avoid consumers
> - # trying to follow the NULL pointer. Displaying '0', 0x30 or
> - # 0x0 when analyzing the JSON debug stream could become
> + # A NULL / non-NULL pointer may be encoded in the stream as a
> + # '0'/'1' to represent the status of the pointer. Displaying '0',
> + # 0x30 or 0x0 when analyzing the JSON debug stream could become
> # confusing, so use an explicit term instead.
> - return "nullptr"
> + return "null-ptr" if self.data == self.NULL_PTR_MARKER else "valid-ptr"
>
> def __str__(self):
> return self.__repr__()
>
> def read(self):
> - super(VMSDFieldNull, self).read()
> - assert(self.data == self.NULL_PTR_MARKER)
> + super(VMSDFieldPtrMarker, self).read()
> + assert(self.data in [self.NULL_PTR_MARKER, self.VALID_PTR_MARKER])
> return self.data
>
> class VMSDFieldBool(VMSDFieldGeneric):
> @@ -642,7 +642,9 @@ def getDict(self):
> "bitmap" : VMSDFieldGeneric,
> "struct" : VMSDFieldStruct,
> "capability": VMSDFieldCap,
> - "nullptr": VMSDFieldNull,
> + # Keep the old nullptr for old binaries
> + "nullptr": VMSDFieldPtrMarker,
> + "ptr-marker": VMSDFieldPtrMarker,
> "unknown" : VMSDFieldGeneric,
> }
>
LGTM
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (6 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 07/10] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 9:48 ` Alexander Mikhalitsyn
2026-03-19 20:56 ` Fabiano Rosas
2026-03-17 23:23 ` [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-17 23:23 ` [PATCH RFC 10/10] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_ALLOW_NULL Peter Xu
9 siblings, 2 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
The loader side of ptr marker is pretty straightforward, instead of playing
the inner_field trick, just do the load manually assuming the marker layout
is a stable ABI (which it is true already).
This will remove some logic while loading VMSD, and hopefully it makes it
slightly easier to read. Unfortunately, we still need to keep the sender
side because of the JSON blob we're maintaining..
This paves way for future processing of non-NULL markers as well.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate-types.c | 12 ++++--------
migration/vmstate.c | 40 ++++++++++++++++++++++++---------------
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index b31689fc3c..ae465c5c2c 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
const VMStateField *field, Error **errp)
{
- int byte = qemu_get_byte(f);
-
- if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
- /* TODO: process PTR_VALID case */
- return true;
- }
-
- error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
+ /*
+ * Load is done in vmstate core, see vmstate_ptr_marker_load().
+ */
+ g_assert_not_reached();
return false;
}
diff --git a/migration/vmstate.c b/migration/vmstate.c
index a3a5f25946..d65fc84dfa 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
}
}
+static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
+ Error **errp)
+{
+ int byte = qemu_get_byte(f);
+
+ if (byte == VMS_MARKER_PTR_NULL) {
+ /* When it's a null ptr marker, do not continue the load */
+ *load_field = false;
+ return true;
+ }
+
+ error_setg(errp, "Unexpected ptr marker: %d", byte);
+ return false;
+}
+
static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
Error **errp)
{
@@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
}
for (i = 0; i < n_elems; i++) {
- bool ok;
+ /* If we will process the load of field? */
+ bool load_field = true;
+ bool ok = true;
void *curr_elem = first_elem + size * i;
- const VMStateField *inner_field;
if (field->flags & VMS_ARRAY_OF_POINTER) {
curr_elem = *(void **)curr_elem;
}
if (!curr_elem && size) {
- /*
- * If null pointer found (which should only happen in
- * an array of pointers), use null placeholder and do
- * not follow.
- */
- inner_field = vmsd_create_ptr_marker_field(field);
- } else {
- inner_field = field;
+ /* Read the marker instead of VMSD itself */
+ if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
+ trace_vmstate_load_field_error(field->name, -EINVAL);
+ return false;
+ }
}
- ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
-
- /* If we used a fake temp field.. free it now */
- if (inner_field != field) {
- g_clear_pointer((gpointer *)&inner_field, g_free);
+ if (load_field) {
+ ok = vmstate_load_field(f, curr_elem, size, field, errp);
}
if (ok) {
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-17 23:23 ` [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core Peter Xu
@ 2026-03-18 9:48 ` Alexander Mikhalitsyn
2026-03-19 20:56 ` Fabiano Rosas
1 sibling, 0 replies; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 9:48 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> The loader side of ptr marker is pretty straightforward, instead of playing
> the inner_field trick, just do the load manually assuming the marker layout
> is a stable ABI (which it is true already).
>
> This will remove some logic while loading VMSD, and hopefully it makes it
> slightly easier to read. Unfortunately, we still need to keep the sender
> side because of the JSON blob we're maintaining..
>
> This paves way for future processing of non-NULL markers as well.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> migration/vmstate-types.c | 12 ++++--------
> migration/vmstate.c | 40 ++++++++++++++++++++++++---------------
> 2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index b31689fc3c..ae465c5c2c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> const VMStateField *field, Error **errp)
>
> {
> - int byte = qemu_get_byte(f);
> -
> - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> - /* TODO: process PTR_VALID case */
> - return true;
> - }
> -
> - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> + /*
> + * Load is done in vmstate core, see vmstate_ptr_marker_load().
> + */
> + g_assert_not_reached();
> return false;
> }
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index a3a5f25946..d65fc84dfa 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> }
> }
>
> +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> + Error **errp)
> +{
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL) {
> + /* When it's a null ptr marker, do not continue the load */
> + *load_field = false;
> + return true;
> + }
> +
> + error_setg(errp, "Unexpected ptr marker: %d", byte);
> + return false;
> +}
> +
> static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
> Error **errp)
> {
> @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> for (i = 0; i < n_elems; i++) {
> - bool ok;
> + /* If we will process the load of field? */
> + bool load_field = true;
> + bool ok = true;
> void *curr_elem = first_elem + size * i;
> - const VMStateField *inner_field;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> curr_elem = *(void **)curr_elem;
> }
>
> if (!curr_elem && size) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> - inner_field = vmsd_create_ptr_marker_field(field);
> - } else {
> - inner_field = field;
> + /* Read the marker instead of VMSD itself */
> + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> + trace_vmstate_load_field_error(field->name, -EINVAL);
> + return false;
> + }
> }
>
> - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> -
> - /* If we used a fake temp field.. free it now */
> - if (inner_field != field) {
> - g_clear_pointer((gpointer *)&inner_field, g_free);
> + if (load_field) {
> + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> }
>
> if (ok) {
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-17 23:23 ` [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core Peter Xu
2026-03-18 9:48 ` Alexander Mikhalitsyn
@ 2026-03-19 20:56 ` Fabiano Rosas
2026-03-19 21:57 ` Peter Xu
1 sibling, 1 reply; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-19 20:56 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx
Peter Xu <peterx@redhat.com> writes:
> The loader side of ptr marker is pretty straightforward, instead of playing
> the inner_field trick, just do the load manually assuming the marker layout
> is a stable ABI (which it is true already).
>
> This will remove some logic while loading VMSD, and hopefully it makes it
> slightly easier to read. Unfortunately, we still need to keep the sender
> side because of the JSON blob we're maintaining..
>
/ramble
Is the JSON blob ABI? Can we kill it? AFAIR, it only serves to add
complexity and to break analyze-script from time to time. We'd be better
off parsing the actual stream from a file. Could maybe even use the same
loadvm code, but mock the .get functions to print instead of actually
loading.
Having separate code that parses a "thing" that's not the stream, but
that should reflect the stream, but it's not the stream, but pretty
close it's a bit weird to me. I recently had to simply open the raw
stream on emacs and navigate through it because the file: stream was
somehow different from the stream on the qcow2, for the same command
line.
(that's another point, parsing from the qcow2 would be cool, which the
JSON blob doesn't provide today I think)
ramble/
> This paves way for future processing of non-NULL markers as well.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/vmstate-types.c | 12 ++++--------
> migration/vmstate.c | 40 ++++++++++++++++++++++++---------------
> 2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index b31689fc3c..ae465c5c2c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> const VMStateField *field, Error **errp)
>
> {
> - int byte = qemu_get_byte(f);
> -
> - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> - /* TODO: process PTR_VALID case */
> - return true;
> - }
> -
> - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> + /*
> + * Load is done in vmstate core, see vmstate_ptr_marker_load().
> + */
> + g_assert_not_reached();
> return false;
> }
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index a3a5f25946..d65fc84dfa 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> }
> }
>
> +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> + Error **errp)
> +{
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL) {
> + /* When it's a null ptr marker, do not continue the load */
> + *load_field = false;
> + return true;
> + }
> +
> + error_setg(errp, "Unexpected ptr marker: %d", byte);
> + return false;
> +}
> +
> static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
> Error **errp)
> {
> @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> for (i = 0; i < n_elems; i++) {
> - bool ok;
> + /* If we will process the load of field? */
> + bool load_field = true;
maybe valid_ptr would be more clear?
> + bool ok = true;
> void *curr_elem = first_elem + size * i;
> - const VMStateField *inner_field;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> curr_elem = *(void **)curr_elem;
> }
>
> if (!curr_elem && size) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> - inner_field = vmsd_create_ptr_marker_field(field);
> - } else {
> - inner_field = field;
> + /* Read the marker instead of VMSD itself */
> + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> + trace_vmstate_load_field_error(field->name, -EINVAL);
> + return false;
> + }
> }
>
> - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> -
> - /* If we used a fake temp field.. free it now */
> - if (inner_field != field) {
> - g_clear_pointer((gpointer *)&inner_field, g_free);
> + if (load_field) {
> + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> }
>
> if (ok) {
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-19 20:56 ` Fabiano Rosas
@ 2026-03-19 21:57 ` Peter Xu
2026-03-19 22:07 ` Alexander Graf
2026-03-20 13:03 ` Fabiano Rosas
0 siblings, 2 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-19 21:57 UTC (permalink / raw)
To: Fabiano Rosas
Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin, Alexander Graf,
Markus Armbruster, Dr. David Alan Gilbert, Juan Quintela,
Peter Maydell, Daniel P. Berrangé
On Thu, Mar 19, 2026 at 05:56:52PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > The loader side of ptr marker is pretty straightforward, instead of playing
> > the inner_field trick, just do the load manually assuming the marker layout
> > is a stable ABI (which it is true already).
> >
> > This will remove some logic while loading VMSD, and hopefully it makes it
> > slightly easier to read. Unfortunately, we still need to keep the sender
> > side because of the JSON blob we're maintaining..
> >
>
> /ramble
> Is the JSON blob ABI? Can we kill it?
Yeah, that's a fair ramble. I had sometimes the same feeling that I wished
it not be there, it'll save quite some work..
Per my limited understanding of reading the code in the past, the plan 10
years ago was having those blob to always be together with the binary
stream so that the stream (especially when causing loading failures..) will
be parsable and it's easier to know why the load fail.
However I also confess in the past few years since I worked on migration, I
never used it to debug any real VMSD issues..
I think one reason might be that when migration was very new and when
device emulation developers were easier to mess things up, crash happen
more, and at that time this tool helps debugging things. It'll also almost
make the VM dump self-explanatory.
Nowadays, we added more codes into migration to help, e.g., I recall we
added things like QEMU_VM_SECTION_FOOTER and likely more after the vmdesc,
which can also help to identify VMSD issues directly from dest QEMU errors.
Let me copy some more people to see if there's any thoughts or inputs on
the JSON blobs..
> AFAIR, it only serves to add
> complexity and to break analyze-script from time to time. We'd be better
> off parsing the actual stream from a file. Could maybe even use the same
> loadvm code, but mock the .get functions to print instead of actually
> loading.
The problem might be that direct parsing of the binary stream is almost
impossible when without the json blob, due to the fact we have a very
condensed VMSD fields in the stream (we have zero headers for fields..), so
they're very efficient but compat, I think we won't be able to parse an
image if we don't know which version of QEMU it was generated from
otherwise.
But again, I'm not sure we strictly need this in each VM image we send,
especially right now on dest QEMU we explicitly allocates that buffer, load
this vmdesc dump and then drop it.. There's also that postcopy won't dump
this (pretty much because we know it's an active QEMU instance so JSON blob
may not really help..), so it's just a bit weird to not be able to see how
it helps besides debugging a "migrate to file" case with unknown QEMU
versions, for example. When I know the version of QEMU on src side when
something wrong happened (normally we do..), it may not be very needed.
>
> Having separate code that parses a "thing" that's not the stream, but
> that should reflect the stream, but it's not the stream, but pretty
> close it's a bit weird to me. I recently had to simply open the raw
> stream on emacs and navigate through it because the file: stream was
> somehow different from the stream on the qcow2, for the same command
> line.
Do you mean snapshot-save v.s. live migrate to file URIs when all caps off?
>
> (that's another point, parsing from the qcow2 would be cool, which the
> JSON blob doesn't provide today I think)
> ramble/
>
> > This paves way for future processing of non-NULL markers as well.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > migration/vmstate-types.c | 12 ++++--------
> > migration/vmstate.c | 40 ++++++++++++++++++++++++---------------
> > 2 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> > index b31689fc3c..ae465c5c2c 100644
> > --- a/migration/vmstate-types.c
> > +++ b/migration/vmstate-types.c
> > @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> > const VMStateField *field, Error **errp)
> >
> > {
> > - int byte = qemu_get_byte(f);
> > -
> > - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> > - /* TODO: process PTR_VALID case */
> > - return true;
> > - }
> > -
> > - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> > + /*
> > + * Load is done in vmstate core, see vmstate_ptr_marker_load().
> > + */
> > + g_assert_not_reached();
> > return false;
> > }
> >
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index a3a5f25946..d65fc84dfa 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> > }
> > }
> >
> > +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> > + Error **errp)
> > +{
> > + int byte = qemu_get_byte(f);
> > +
> > + if (byte == VMS_MARKER_PTR_NULL) {
> > + /* When it's a null ptr marker, do not continue the load */
> > + *load_field = false;
> > + return true;
> > + }
> > +
> > + error_setg(errp, "Unexpected ptr marker: %d", byte);
> > + return false;
> > +}
> > +
> > static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
> > Error **errp)
> > {
> > @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> > }
> >
> > for (i = 0; i < n_elems; i++) {
> > - bool ok;
> > + /* If we will process the load of field? */
> > + bool load_field = true;
>
> maybe valid_ptr would be more clear?
I don't think we can? Note, that we will only update this field iff the
field used the new VMS_ARRAY_OF_POINTER_AUTO_ALLOC, otherwise it simply
should be true always and it says "let's load this VMSD". IOW, in most
cases there's no ptr, hence "valid_ptr=true" will be weird on its own I
think..
>
> > + bool ok = true;
> > void *curr_elem = first_elem + size * i;
> > - const VMStateField *inner_field;
> >
> > if (field->flags & VMS_ARRAY_OF_POINTER) {
> > curr_elem = *(void **)curr_elem;
> > }
> >
> > if (!curr_elem && size) {
> > - /*
> > - * If null pointer found (which should only happen in
> > - * an array of pointers), use null placeholder and do
> > - * not follow.
> > - */
> > - inner_field = vmsd_create_ptr_marker_field(field);
> > - } else {
> > - inner_field = field;
> > + /* Read the marker instead of VMSD itself */
> > + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> > + trace_vmstate_load_field_error(field->name, -EINVAL);
> > + return false;
> > + }
> > }
> >
> > - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> > -
> > - /* If we used a fake temp field.. free it now */
> > - if (inner_field != field) {
> > - g_clear_pointer((gpointer *)&inner_field, g_free);
> > + if (load_field) {
> > + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> > }
> >
> > if (ok) {
>
--
Peter Xu
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-19 21:57 ` Peter Xu
@ 2026-03-19 22:07 ` Alexander Graf
2026-03-20 13:03 ` Fabiano Rosas
1 sibling, 0 replies; 34+ messages in thread
From: Alexander Graf @ 2026-03-19 22:07 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas
Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin,
Markus Armbruster, Dr. David Alan Gilbert, Juan Quintela,
Peter Maydell, Daniel P. Berrangé
On 19.03.26 22:57, Peter Xu wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Thu, Mar 19, 2026 at 05:56:52PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>>> The loader side of ptr marker is pretty straightforward, instead of playing
>>> the inner_field trick, just do the load manually assuming the marker layout
>>> is a stable ABI (which it is true already).
>>>
>>> This will remove some logic while loading VMSD, and hopefully it makes it
>>> slightly easier to read. Unfortunately, we still need to keep the sender
>>> side because of the JSON blob we're maintaining..
>>>
>> /ramble
>> Is the JSON blob ABI? Can we kill it?
> Yeah, that's a fair ramble. I had sometimes the same feeling that I wished
> it not be there, it'll save quite some work..
>
> Per my limited understanding of reading the code in the past, the plan 10
> years ago was having those blob to always be together with the binary
> stream so that the stream (especially when causing loading failures..) will
> be parsable and it's easier to know why the load fail.
>
> However I also confess in the past few years since I worked on migration, I
> never used it to debug any real VMSD issues..
>
> I think one reason might be that when migration was very new and when
> device emulation developers were easier to mess things up, crash happen
> more, and at that time this tool helps debugging things. It'll also almost
> make the VM dump self-explanatory.
>
> Nowadays, we added more codes into migration to help, e.g., I recall we
> added things like QEMU_VM_SECTION_FOOTER and likely more after the vmdesc,
> which can also help to identify VMSD issues directly from dest QEMU errors.
>
> Let me copy some more people to see if there's any thoughts or inputs on
> the JSON blobs..
I added and used it for 2 purposes:
1. To make the actual content of the device state more clearly visible.
It can help to stare at a device state dump to find out what's going wrong.
2. To allow for state diffing. When I introduced it, I would do a
migration to file, run my test, do another migration and then diff the
vmstates. That way I could easily see what actually changed between the
runs. It helped me identify a few CPU register state issues.
I don't believe I ever ended up using it to debug live migration issues :).
Alex
Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christof Hellmis, Andreas Stieger
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-19 21:57 ` Peter Xu
2026-03-19 22:07 ` Alexander Graf
@ 2026-03-20 13:03 ` Fabiano Rosas
2026-03-20 14:51 ` Peter Xu
1 sibling, 1 reply; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-20 13:03 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin, Alexander Graf,
Markus Armbruster, Dr. David Alan Gilbert, Juan Quintela,
Peter Maydell, Daniel P. Berrangé
Peter Xu <peterx@redhat.com> writes:
> On Thu, Mar 19, 2026 at 05:56:52PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > The loader side of ptr marker is pretty straightforward, instead of playing
>> > the inner_field trick, just do the load manually assuming the marker layout
>> > is a stable ABI (which it is true already).
>> >
>> > This will remove some logic while loading VMSD, and hopefully it makes it
>> > slightly easier to read. Unfortunately, we still need to keep the sender
>> > side because of the JSON blob we're maintaining..
>> >
>>
>> /ramble
>> Is the JSON blob ABI? Can we kill it?
>
> Yeah, that's a fair ramble. I had sometimes the same feeling that I wished
> it not be there, it'll save quite some work..
>
> Per my limited understanding of reading the code in the past, the plan 10
> years ago was having those blob to always be together with the binary
> stream so that the stream (especially when causing loading failures..) will
> be parsable and it's easier to know why the load fail.
>
This would only work for a file migration or snapshots. The live
migration wouldn't have received the blob at the point it fails I
think. That's ok, and it's not a bad idea, but I think we're not making
good use of it currently.
> However I also confess in the past few years since I worked on migration, I
> never used it to debug any real VMSD issues..
>
I actually do use it a few times a year, pretty much to diff the stream
between two different QEMU versions, like Alexander mentioned. From the
recent mips bug:
(echo "migrate file:migfile-11"; echo "quit") | ./build/qemu-system-mips64 -monitor stdio
(echo "migrate file:migfile-10"; echo "quit") | ./build-10.2/qemu-system-mips64 -monitor stdio
diff -y <(./scripts/analyze-migration.py -f migfile-10)
<(./scripts/analyze-migration.py -f migfile-11) | less
As you can see, usability is weird. A QEMU command that dumps a JSON
already formatted would be a good improvement. -dump-vmstate does it,
but there it's the dump of all vmstates like they were registered, not
what's actually migrated and of course it doesn't contain the data.
> I think one reason might be that when migration was very new and when
> device emulation developers were easier to mess things up, crash happen
> more, and at that time this tool helps debugging things. It'll also almost
> make the VM dump self-explanatory.
>
> Nowadays, we added more codes into migration to help, e.g., I recall we
> added things like QEMU_VM_SECTION_FOOTER and likely more after the vmdesc,
> which can also help to identify VMSD issues directly from dest QEMU errors.
>
> Let me copy some more people to see if there's any thoughts or inputs on
> the JSON blobs..
>
>> AFAIR, it only serves to add
>> complexity and to break analyze-script from time to time. We'd be better
>> off parsing the actual stream from a file. Could maybe even use the same
>> loadvm code, but mock the .get functions to print instead of actually
>> loading.
>
> The problem might be that direct parsing of the binary stream is almost
> impossible when without the json blob, due to the fact we have a very
> condensed VMSD fields in the stream (we have zero headers for fields..), so
> they're very efficient but compat, I think we won't be able to parse an
> image if we don't know which version of QEMU it was generated from
> otherwise.
>
I was thinking of using the same QEMU version that produced the binary
stream indeed. But yeah, it's probably not very useful.
Now, if QEMU can load the binary stream, it could dry-load it
somehow. We currently have the vmstate-walking code tightly coupled with
the actual loading of the data, I can see a scenario where these things
are separated and we can decided what operation to perform on a walk.
Would walking the vmstates and generating the JSON beforehand be useful?
We could then move it to the start of the binary stream, could have a
QEMU command that dumps it, could even use that set of data structures
in JSON format to then guide a second walk which either calls the
devices VMStateInfo or prints the data to a file for debugging. Thinking
out loud here...
> But again, I'm not sure we strictly need this in each VM image we send,
> especially right now on dest QEMU we explicitly allocates that buffer, load
> this vmdesc dump and then drop it.. There's also that postcopy won't dump
> this (pretty much because we know it's an active QEMU instance so JSON blob
> may not really help..), so it's just a bit weird to not be able to see how
> it helps besides debugging a "migrate to file" case with unknown QEMU
> versions, for example. When I know the version of QEMU on src side when
> something wrong happened (normally we do..), it may not be very needed.
>
Well, putting it on the binary stream for TCP migration might be useless
because we never do anything with it. And it's usually migrating live,
so we don't even get to it if anything fails. Putting it on the .qcow2
file is also useless because there's no tooling to inspect the binary
stream, although we could adapt analyze-migration to take an offset and
read from the middle of the file. And hexdump'ing it is not super hard
as well, but it's less likely anyone will do it.
Even the migrate to file approach is frail because depending on the
changes between the QEMU that produced the stream and the one we took
analyze-migration from, the script might not be able to parse the JSON
at all. Such as with this series.
>>
>> Having separate code that parses a "thing" that's not the stream, but
>> that should reflect the stream, but it's not the stream, but pretty
>> close it's a bit weird to me. I recently had to simply open the raw
>> stream on emacs and navigate through it because the file: stream was
>> somehow different from the stream on the qcow2, for the same command
>> line.
>
> Do you mean snapshot-save v.s. live migrate to file URIs when all caps off?
>
I was trying to ensure migration to file was producing the same binary
stream as snapshot-save. For a same command line, the migration was
succeeding, but the loadm was failing. So I resorted to opening the
binaries and searching for the QEVM marker.
>>
>> (that's another point, parsing from the qcow2 would be cool, which the
>> JSON blob doesn't provide today I think)
>> ramble/
>>
>> > This paves way for future processing of non-NULL markers as well.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> > migration/vmstate-types.c | 12 ++++--------
>> > migration/vmstate.c | 40 ++++++++++++++++++++++++---------------
>> > 2 files changed, 29 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
>> > index b31689fc3c..ae465c5c2c 100644
>> > --- a/migration/vmstate-types.c
>> > +++ b/migration/vmstate-types.c
>> > @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
>> > const VMStateField *field, Error **errp)
>> >
>> > {
>> > - int byte = qemu_get_byte(f);
>> > -
>> > - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
>> > - /* TODO: process PTR_VALID case */
>> > - return true;
>> > - }
>> > -
>> > - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
>> > + /*
>> > + * Load is done in vmstate core, see vmstate_ptr_marker_load().
>> > + */
>> > + g_assert_not_reached();
>> > return false;
>> > }
>> >
>> > diff --git a/migration/vmstate.c b/migration/vmstate.c
>> > index a3a5f25946..d65fc84dfa 100644
>> > --- a/migration/vmstate.c
>> > +++ b/migration/vmstate.c
>> > @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
>> > }
>> > }
>> >
>> > +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
>> > + Error **errp)
>> > +{
>> > + int byte = qemu_get_byte(f);
>> > +
>> > + if (byte == VMS_MARKER_PTR_NULL) {
>> > + /* When it's a null ptr marker, do not continue the load */
>> > + *load_field = false;
>> > + return true;
>> > + }
>> > +
>> > + error_setg(errp, "Unexpected ptr marker: %d", byte);
>> > + return false;
>> > +}
>> > +
>> > static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
>> > Error **errp)
>> > {
>> > @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>> > }
>> >
>> > for (i = 0; i < n_elems; i++) {
>> > - bool ok;
>> > + /* If we will process the load of field? */
>> > + bool load_field = true;
>>
>> maybe valid_ptr would be more clear?
>
> I don't think we can? Note, that we will only update this field iff the
> field used the new VMS_ARRAY_OF_POINTER_AUTO_ALLOC, otherwise it simply
> should be true always and it says "let's load this VMSD". IOW, in most
> cases there's no ptr, hence "valid_ptr=true" will be weird on its own I
> think..
>
Ah I see. I was looking at the save side and thinking we can maybe
simplify the inner_field logic a bit, but it doesn't apply here. I'll
try to write a patch on top of your series with my ideas.
>>
>> > + bool ok = true;
>> > void *curr_elem = first_elem + size * i;
>> > - const VMStateField *inner_field;
>> >
>> > if (field->flags & VMS_ARRAY_OF_POINTER) {
>> > curr_elem = *(void **)curr_elem;
>> > }
>> >
>> > if (!curr_elem && size) {
>> > - /*
>> > - * If null pointer found (which should only happen in
>> > - * an array of pointers), use null placeholder and do
>> > - * not follow.
>> > - */
>> > - inner_field = vmsd_create_ptr_marker_field(field);
>> > - } else {
>> > - inner_field = field;
>> > + /* Read the marker instead of VMSD itself */
>> > + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
>> > + trace_vmstate_load_field_error(field->name, -EINVAL);
>> > + return false;
>> > + }
>> > }
>> >
>> > - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
>> > -
>> > - /* If we used a fake temp field.. free it now */
>> > - if (inner_field != field) {
>> > - g_clear_pointer((gpointer *)&inner_field, g_free);
>> > + if (load_field) {
>> > + ok = vmstate_load_field(f, curr_elem, size, field, errp);
>> > }
>> >
>> > if (ok) {
>>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core
2026-03-20 13:03 ` Fabiano Rosas
@ 2026-03-20 14:51 ` Peter Xu
0 siblings, 0 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-20 14:51 UTC (permalink / raw)
To: Fabiano Rosas
Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin, Alexander Graf,
Markus Armbruster, Dr. David Alan Gilbert, Juan Quintela,
Peter Maydell, Daniel P. Berrangé
On Fri, Mar 20, 2026 at 10:03:54AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Thu, Mar 19, 2026 at 05:56:52PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >>
> >> > The loader side of ptr marker is pretty straightforward, instead of playing
> >> > the inner_field trick, just do the load manually assuming the marker layout
> >> > is a stable ABI (which it is true already).
> >> >
> >> > This will remove some logic while loading VMSD, and hopefully it makes it
> >> > slightly easier to read. Unfortunately, we still need to keep the sender
> >> > side because of the JSON blob we're maintaining..
> >> >
> >>
> >> /ramble
> >> Is the JSON blob ABI? Can we kill it?
> >
> > Yeah, that's a fair ramble. I had sometimes the same feeling that I wished
> > it not be there, it'll save quite some work..
> >
> > Per my limited understanding of reading the code in the past, the plan 10
> > years ago was having those blob to always be together with the binary
> > stream so that the stream (especially when causing loading failures..) will
> > be parsable and it's easier to know why the load fail.
> >
>
> This would only work for a file migration or snapshots. The live
> migration wouldn't have received the blob at the point it fails I
> think. That's ok, and it's not a bad idea, but I think we're not making
> good use of it currently.
Now I do start to question more if it's a good idea to put it into a live
stream, after I read both your reply and Alex's.
I think I might have over-thought it to be "working for any version of
QEMU". Maybe nobody (or at least less than I was expecting yesterday) that
really needs this functionality. Likely in 99.9999% cases, we always know
the version of QEMU binary together with the stream.
And when put into the live stream... I want to discuss the possibility of
removing it at least there. Destination read it and drop it right now: it
not only makes save() slower, and load() slower too.. not too much, just
not necessary.
>
> > However I also confess in the past few years since I worked on migration, I
> > never used it to debug any real VMSD issues..
> >
>
> I actually do use it a few times a year, pretty much to diff the stream
> between two different QEMU versions, like Alexander mentioned. From the
> recent mips bug:
>
> (echo "migrate file:migfile-11"; echo "quit") | ./build/qemu-system-mips64 -monitor stdio
> (echo "migrate file:migfile-10"; echo "quit") | ./build-10.2/qemu-system-mips64 -monitor stdio
> diff -y <(./scripts/analyze-migration.py -f migfile-10)
> <(./scripts/analyze-migration.py -f migfile-11) | less
>
> As you can see, usability is weird. A QEMU command that dumps a JSON
> already formatted would be a good improvement. -dump-vmstate does it,
> but there it's the dump of all vmstates like they were registered, not
> what's actually migrated and of course it doesn't contain the data.
This is a point almost to say it would be nice we keep this (or some
similar function that do more than -dump-vmstate) to be around.
>
> > I think one reason might be that when migration was very new and when
> > device emulation developers were easier to mess things up, crash happen
> > more, and at that time this tool helps debugging things. It'll also almost
> > make the VM dump self-explanatory.
> >
> > Nowadays, we added more codes into migration to help, e.g., I recall we
> > added things like QEMU_VM_SECTION_FOOTER and likely more after the vmdesc,
> > which can also help to identify VMSD issues directly from dest QEMU errors.
> >
> > Let me copy some more people to see if there's any thoughts or inputs on
> > the JSON blobs..
> >
> >> AFAIR, it only serves to add
> >> complexity and to break analyze-script from time to time. We'd be better
> >> off parsing the actual stream from a file. Could maybe even use the same
> >> loadvm code, but mock the .get functions to print instead of actually
> >> loading.
> >
> > The problem might be that direct parsing of the binary stream is almost
> > impossible when without the json blob, due to the fact we have a very
> > condensed VMSD fields in the stream (we have zero headers for fields..), so
> > they're very efficient but compat, I think we won't be able to parse an
> > image if we don't know which version of QEMU it was generated from
> > otherwise.
> >
>
> I was thinking of using the same QEMU version that produced the binary
> stream indeed. But yeah, it's probably not very useful.
I revoke my demand on "it needs to work on unknown QEMU version" now.
>
> Now, if QEMU can load the binary stream, it could dry-load it
> somehow. We currently have the vmstate-walking code tightly coupled with
> the actual loading of the data, I can see a scenario where these things
> are separated and we can decided what operation to perform on a walk.
Am I the only one who thinks this might be a very good idea? :)
>
> Would walking the vmstates and generating the JSON beforehand be useful?
> We could then move it to the start of the binary stream, could have a
> QEMU command that dumps it, could even use that set of data structures
> in JSON format to then guide a second walk which either calls the
> devices VMStateInfo or prints the data to a file for debugging. Thinking
> out loud here...
I think it won't work: the migration stream has dependency on device states
(including ram). It may dump different things when done at the beginning
v.s. when done at switchover, because the VM is running and device states
can change.
Consider one .needed() of a VMSD field that report different things from
time to time (say, the guest driver enabled some device feature during
migration running).
>
> > But again, I'm not sure we strictly need this in each VM image we send,
> > especially right now on dest QEMU we explicitly allocates that buffer, load
> > this vmdesc dump and then drop it.. There's also that postcopy won't dump
> > this (pretty much because we know it's an active QEMU instance so JSON blob
> > may not really help..), so it's just a bit weird to not be able to see how
> > it helps besides debugging a "migrate to file" case with unknown QEMU
> > versions, for example. When I know the version of QEMU on src side when
> > something wrong happened (normally we do..), it may not be very needed.
> >
>
> Well, putting it on the binary stream for TCP migration might be useless
> because we never do anything with it. And it's usually migrating live,
> so we don't even get to it if anything fails. Putting it on the .qcow2
> file is also useless because there's no tooling to inspect the binary
> stream, although we could adapt analyze-migration to take an offset and
> read from the middle of the file. And hexdump'ing it is not super hard
> as well, but it's less likely anyone will do it.
>
> Even the migrate to file approach is frail because depending on the
> changes between the QEMU that produced the stream and the one we took
> analyze-migration from, the script might not be able to parse the JSON
> at all. Such as with this series.
I agree. So I think we can discuss two separate things:
(1) removal of JSON blob in TCP live streaming
(2) removal of JSON blob in file migration, and replace it with the vmsd
walker idea you proposed above
One thing funny about (1) is, since (please anyone correct me otherwise..)
all QEMU binaries in the past expect QEMU_VM_VMDESCRIPTION to be present,
but drop the data all the time, we may even consider breaking the ABI
immediately, by sending some fake strings inside without generating the
real JSON.. Old QEMUs migration shouldn't be broken because they'll just
read less and drop less.
I had a feeling (2) should be able to fully replace the current analyze
script, meanwhile we can remove the script completely, and meanwhile remove
all the tricks we have done with the json writer like commit 9867c3a7ced,
35049eb0d2fc, etc., which affects production code paths very much...
>
> >>
> >> Having separate code that parses a "thing" that's not the stream, but
> >> that should reflect the stream, but it's not the stream, but pretty
> >> close it's a bit weird to me. I recently had to simply open the raw
> >> stream on emacs and navigate through it because the file: stream was
> >> somehow different from the stream on the qcow2, for the same command
> >> line.
> >
> > Do you mean snapshot-save v.s. live migrate to file URIs when all caps off?
> >
>
> I was trying to ensure migration to file was producing the same binary
> stream as snapshot-save. For a same command line, the migration was
> succeeding, but the loadm was failing. So I resorted to opening the
> binaries and searching for the QEVM marker.
OK. I hope I'm right we're all moving towards the file: URIs and maybe some
day we can obsolete the old snapshots.
>
> >>
> >> (that's another point, parsing from the qcow2 would be cool, which the
> >> JSON blob doesn't provide today I think)
> >> ramble/
> >>
> >> > This paves way for future processing of non-NULL markers as well.
> >> >
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > ---
> >> > migration/vmstate-types.c | 12 ++++--------
> >> > migration/vmstate.c | 40 ++++++++++++++++++++++++---------------
> >> > 2 files changed, 29 insertions(+), 23 deletions(-)
> >> >
> >> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> >> > index b31689fc3c..ae465c5c2c 100644
> >> > --- a/migration/vmstate-types.c
> >> > +++ b/migration/vmstate-types.c
> >> > @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> >> > const VMStateField *field, Error **errp)
> >> >
> >> > {
> >> > - int byte = qemu_get_byte(f);
> >> > -
> >> > - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> >> > - /* TODO: process PTR_VALID case */
> >> > - return true;
> >> > - }
> >> > -
> >> > - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> >> > + /*
> >> > + * Load is done in vmstate core, see vmstate_ptr_marker_load().
> >> > + */
> >> > + g_assert_not_reached();
> >> > return false;
> >> > }
> >> >
> >> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> > index a3a5f25946..d65fc84dfa 100644
> >> > --- a/migration/vmstate.c
> >> > +++ b/migration/vmstate.c
> >> > @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> >> > }
> >> > }
> >> >
> >> > +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> >> > + Error **errp)
> >> > +{
> >> > + int byte = qemu_get_byte(f);
> >> > +
> >> > + if (byte == VMS_MARKER_PTR_NULL) {
> >> > + /* When it's a null ptr marker, do not continue the load */
> >> > + *load_field = false;
> >> > + return true;
> >> > + }
> >> > +
> >> > + error_setg(errp, "Unexpected ptr marker: %d", byte);
> >> > + return false;
> >> > +}
> >> > +
> >> > static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
> >> > Error **errp)
> >> > {
> >> > @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> >> > }
> >> >
> >> > for (i = 0; i < n_elems; i++) {
> >> > - bool ok;
> >> > + /* If we will process the load of field? */
> >> > + bool load_field = true;
> >>
> >> maybe valid_ptr would be more clear?
> >
> > I don't think we can? Note, that we will only update this field iff the
> > field used the new VMS_ARRAY_OF_POINTER_AUTO_ALLOC, otherwise it simply
> > should be true always and it says "let's load this VMSD". IOW, in most
> > cases there's no ptr, hence "valid_ptr=true" will be weird on its own I
> > think..
> >
>
> Ah I see. I was looking at the save side and thinking we can maybe
> simplify the inner_field logic a bit, but it doesn't apply here. I'll
> try to write a patch on top of your series with my ideas.
Sure, I'll read it when it comes. Or if you think I'm adding code will be
removed soon, just shoot and we discuss to see if we can avoid adding it in
the first place.
>
> >>
> >> > + bool ok = true;
> >> > void *curr_elem = first_elem + size * i;
> >> > - const VMStateField *inner_field;
> >> >
> >> > if (field->flags & VMS_ARRAY_OF_POINTER) {
> >> > curr_elem = *(void **)curr_elem;
> >> > }
> >> >
> >> > if (!curr_elem && size) {
> >> > - /*
> >> > - * If null pointer found (which should only happen in
> >> > - * an array of pointers), use null placeholder and do
> >> > - * not follow.
> >> > - */
> >> > - inner_field = vmsd_create_ptr_marker_field(field);
> >> > - } else {
> >> > - inner_field = field;
> >> > + /* Read the marker instead of VMSD itself */
> >> > + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> >> > + trace_vmstate_load_field_error(field->name, -EINVAL);
> >> > + return false;
> >> > + }
> >> > }
> >> >
> >> > - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> >> > -
> >> > - /* If we used a fake temp field.. free it now */
> >> > - if (inner_field != field) {
> >> > - g_clear_pointer((gpointer *)&inner_field, g_free);
> >> > + if (load_field) {
> >> > + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> >> > }
> >> >
> >> > if (ok) {
> >>
>
--
Peter Xu
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (7 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
2026-03-18 10:00 ` Alexander Mikhalitsyn
` (2 more replies)
2026-03-17 23:23 ` [PATCH RFC 10/10] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_ALLOW_NULL Peter Xu
9 siblings, 3 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas
Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
must be used together with VMS_ARRAY_OF_POINTER.
It can be used to allow migration of an array of pointers where the
pointers may point to NULLs.
Note that we used to allow migration of a NULL pointer within an array that
is being migrated. That corresponds to the code around vmstate_info_nullptr
where we may get/put one byte showing that the element of an array is NULL.
That usage is fine but very limited, it's because even if it will migrate a
NULL pointer with a marker, it still works in a way that both src and dest
QEMUs must know exactly which elements of the array are non-NULL, so
instead of dynamically loading an array (which can have NULL pointers), it
actually only verifies the known NULL pointers are still NULL pointers
after migration.
Also, in that case since dest QEMU knows exactly which element is NULL,
which is not NULL, dest QEMU's device code will manage all allocations for
the elements before invoking vmstate_load_vmsd().
That's not enough per evolving needs of new device states that may want to
provide real dynamic array of pointers, like what Alexander proposed here
with the NVMe device migration:
https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
This patch is an alternative approach to address the problem.
Along with the flag, introduce two new macros:
VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
Which will be used very soon in the NVMe series.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 49 ++++++++++++++++-
migration/savevm.c | 31 ++++++++++-
migration/vmstate.c | 101 ++++++++++++++++++++++++++++++++----
3 files changed, 169 insertions(+), 12 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 2e51b5ea04..70bebc60ed 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -161,8 +161,19 @@ enum VMStateFlags {
* structure we are referencing to use. */
VMS_VSTRUCT = 0x8000,
+ /*
+ * This is a sub-flag for VMS_ARRAY_OF_POINTER, so VMS_ARRAY_OF_POINTER
+ * must be set altogether. When set, it means array elements can
+ * contain either valid or NULL pointers, vmstate core will sync it
+ * between the two QEMU instances via the stream protocol. When it's a
+ * valid pointer, the vmstate core will be responsible to do the proper
+ * memory allocations. It also means user of this flag must prepare
+ * the array to be all NULLs otherwise memory may leak.
+ */
+ VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
+
/* Marker for end of list */
- VMS_END = 0x10000
+ VMS_END = 0x20000,
};
typedef enum {
@@ -580,6 +591,42 @@ extern const VMStateInfo vmstate_info_qlist;
.offset = vmstate_offset_array(_s, _f, _type*, _n), \
}
+/*
+ * For migrating a dynamically allocated uint{8,32}-indexed array of
+ * pointers to structures (with NULL entries and with auto memory
+ * allocation).
+ *
+ * _type: type of structure pointed to
+ * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
+ * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
+ * start: size of (_type) pointed to (for auto memory allocation)
+ */
+#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
+ _field, _state, _field_num, _version, _vmsd, _type) { \
+ .name = (stringify(_field)), \
+ .version_id = (_version), \
+ .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
+ .vmsd = &(_vmsd), \
+ .size = sizeof(_type), \
+ .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
+ VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
+ VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
+ .offset = vmstate_offset_pointer(_state, _field, _type *), \
+}
+
+#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
+ _field, _state, _field_num, _version, _vmsd, _type) { \
+ .name = (stringify(_field)), \
+ .version_id = (_version), \
+ .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
+ .vmsd = &(_vmsd), \
+ .size = sizeof(_type), \
+ .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
+ VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
+ VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
+ .offset = vmstate_offset_pointer(_state, _field, _type *), \
+}
+
#define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
.name = (stringify(_field)), \
.version_id = (_version), \
diff --git a/migration/savevm.c b/migration/savevm.c
index f5a6fd0c66..34223de818 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -869,8 +869,37 @@ static void vmstate_check(const VMStateDescription *vmsd)
if (field) {
while (field->name) {
if (field->flags & VMS_ARRAY_OF_POINTER) {
- assert(field->size == 0);
+ if (VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
+ /*
+ * Size must be provided because dest QEMU needs that
+ * info to know what to allocate
+ */
+ assert(field->size || field->size_offset);
+ } else {
+ /*
+ * Otherwise size info isn't useful (because it's
+ * always the size of host pointer), enforce accidental
+ * setup of sizes in this case.
+ */
+ assert(field->size == 0 && field->size_offset == 0);
+ }
+ /*
+ * VMS_ARRAY_OF_POINTER must be used only together with one
+ * of VMS_(V)ARRAY* flags.
+ */
+ assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
+ VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
+ VMS_VARRAY_UINT32));
}
+
+ /*
+ * When VMS_ARRAY_OF_POINTER_ALLOW_NULL is used, we must have
+ * VMS_ARRAY_OF_POINTER set too.
+ */
+ if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
+ assert(field->flags & VMS_ARRAY_OF_POINTER);
+ }
+
if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
/* Recurse to sub structures */
vmstate_check(field->vmsd);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index d65fc84dfa..7d7d9c7e18 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
return true;
}
+ if (byte == VMS_MARKER_PTR_VALID) {
+ /* We need to load this field right after the marker */
+ *load_field = true;
+ return true;
+ }
+
error_setg(errp, "Unexpected ptr marker: %d", byte);
return false;
}
@@ -234,6 +240,22 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
return true;
}
+
+/*
+ * If we will use a ptr marker in the stream for a field? Two use cases:
+ *
+ * (1) When used with VMS_ARRAY_OF_POINTER_ALLOW_NULL, it must always be
+ * present to imply the population status of the pointer.
+ *
+ * (2) When used with normal VMSD array fields, only emit a null ptr marker
+ * if the pointer is NULL.
+ */
+static inline bool
+vmstate_use_marker_field(void *ptr, int size, bool dynamic_array)
+{
+ return (!ptr && size) || dynamic_array;
+}
+
bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp)
{
@@ -271,6 +293,12 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
void *first_elem = opaque + field->offset;
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
+ /*
+ * When this is enabled, it means we will always push a ptr
+ * marker first for each element saying if it's populated.
+ */
+ bool use_dynamic_array =
+ field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
vmstate_handle_alloc(first_elem, field, opaque);
if (field->flags & VMS_POINTER) {
@@ -282,18 +310,37 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
/* If we will process the load of field? */
bool load_field = true;
bool ok = true;
- void *curr_elem = first_elem + size * i;
+ bool use_marker_field;
+ void *curr_elem_p = first_elem + size * i;
+ void *curr_elem = curr_elem_p;
if (field->flags & VMS_ARRAY_OF_POINTER) {
- curr_elem = *(void **)curr_elem;
+ curr_elem = *(void **)curr_elem_p;
}
- if (!curr_elem && size) {
- /* Read the marker instead of VMSD itself */
+ use_marker_field = vmstate_use_marker_field(curr_elem, size,
+ use_dynamic_array);
+ if (use_marker_field) {
+ /* Read the marker instead of VMSD first */
if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
trace_vmstate_load_field_error(field->name, -EINVAL);
return false;
}
+
+ if (load_field) {
+ /*
+ * When reaching here, it means we received a
+ * non-NULL ptr marker, so we need to populate the
+ * field before loading it.
+ *
+ * NOTE: do not use vmstate_size() here, because we
+ * need the object size, not entry size of the
+ * array.
+ */
+ curr_elem = g_malloc0(field->size);
+ /* Remember to update the root pointer! */
+ *(void **)curr_elem_p = curr_elem;
+ }
}
if (load_field) {
@@ -397,6 +444,16 @@ static bool vmsd_can_compress(const VMStateField *field)
return false;
}
+ if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
+ /*
+ * This may involve two VMSD fields to be dumped, one for the
+ * marker to show if the pointer is NULL, followed by the real
+ * vmstate object. To make it simple at least for now, skip
+ * compression for this one.
+ */
+ return false;
+ }
+
if (field->flags & VMS_STRUCT) {
const VMStateField *sfield = field->vmsd->fields;
while (sfield->name) {
@@ -578,6 +635,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
int size = vmstate_size(opaque, field);
JSONWriter *vmdesc_loop = vmdesc;
bool use_marker_field_prev = false;
+ /*
+ * When this is enabled, it means we will always push a ptr
+ * marker first for each element saying if it's populated.
+ */
+ bool use_dynamic_array =
+ field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
@@ -596,13 +659,10 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
curr_elem = *(void **)curr_elem;
}
- use_marker_field = !curr_elem && size;
+ use_marker_field = vmstate_use_marker_field(curr_elem, size,
+ use_dynamic_array);
+
if (use_marker_field) {
- /*
- * If null pointer found (which should only happen in
- * an array of pointers), use null placeholder and do
- * not follow.
- */
inner_field = vmsd_create_ptr_marker_field(field);
} else {
inner_field = field;
@@ -652,6 +712,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
goto out;
}
+ /*
+ * If we're using dynamic array and the element is
+ * populated, dump the real object right after the marker.
+ */
+ if (use_dynamic_array && curr_elem) {
+ /*
+ * NOTE: do not use vmstate_size() here because we want
+ * to dump the real VMSD object now.
+ */
+ ok = vmstate_save_field_with_vmdesc(f, curr_elem,
+ field->size, vmsd,
+ field, vmdesc_loop,
+ i, max_elems, errp);
+
+ if (!ok) {
+ error_prepend(errp, "Save of field %s/%s failed: ",
+ vmsd->name, field->name);
+ goto out;
+ }
+ }
+
/* Compressed arrays only care about the first element */
if (vmdesc_loop && vmsd_can_compress(field)) {
vmdesc_loop = NULL;
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-17 23:23 ` [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
@ 2026-03-18 10:00 ` Alexander Mikhalitsyn
2026-03-19 14:10 ` Peter Xu
2026-03-19 22:06 ` Fabiano Rosas
2026-03-20 14:42 ` Fabiano Rosas
2 siblings, 1 reply; 34+ messages in thread
From: Alexander Mikhalitsyn @ 2026-03-18 10:00 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> must be used together with VMS_ARRAY_OF_POINTER.
>
> It can be used to allow migration of an array of pointers where the
> pointers may point to NULLs.
>
> Note that we used to allow migration of a NULL pointer within an array that
> is being migrated. That corresponds to the code around vmstate_info_nullptr
> where we may get/put one byte showing that the element of an array is NULL.
>
> That usage is fine but very limited, it's because even if it will migrate a
> NULL pointer with a marker, it still works in a way that both src and dest
> QEMUs must know exactly which elements of the array are non-NULL, so
> instead of dynamically loading an array (which can have NULL pointers), it
> actually only verifies the known NULL pointers are still NULL pointers
> after migration.
>
> Also, in that case since dest QEMU knows exactly which element is NULL,
> which is not NULL, dest QEMU's device code will manage all allocations for
> the elements before invoking vmstate_load_vmsd().
>
> That's not enough per evolving needs of new device states that may want to
> provide real dynamic array of pointers, like what Alexander proposed here
> with the NVMe device migration:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This patch is an alternative approach to address the problem.
>
> Along with the flag, introduce two new macros:
>
> VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>
> Which will be used very soon in the NVMe series.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Amazing, thank you, Peter!
Except small nitpicks I've left as inline comment in this patch, it is LGTM.
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Tested-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Also, I've rebased my NVMe series on top and tested everything
locally. No regressions were found.
Kind regards,
Alex
> ---
> include/migration/vmstate.h | 49 ++++++++++++++++-
> migration/savevm.c | 31 ++++++++++-
> migration/vmstate.c | 101 ++++++++++++++++++++++++++++++++----
> 3 files changed, 169 insertions(+), 12 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2e51b5ea04..70bebc60ed 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -161,8 +161,19 @@ enum VMStateFlags {
> * structure we are referencing to use. */
> VMS_VSTRUCT = 0x8000,
>
> + /*
> + * This is a sub-flag for VMS_ARRAY_OF_POINTER, so VMS_ARRAY_OF_POINTER
> + * must be set altogether. When set, it means array elements can
> + * contain either valid or NULL pointers, vmstate core will sync it
> + * between the two QEMU instances via the stream protocol. When it's a
> + * valid pointer, the vmstate core will be responsible to do the proper
> + * memory allocations. It also means user of this flag must prepare
> + * the array to be all NULLs otherwise memory may leak.
> + */
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> +
> /* Marker for end of list */
> - VMS_END = 0x10000
> + VMS_END = 0x20000,
> };
>
> typedef enum {
> @@ -580,6 +591,42 @@ extern const VMStateInfo vmstate_info_qlist;
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> }
>
> +/*
> + * For migrating a dynamically allocated uint{8,32}-indexed array of
> + * pointers to structures (with NULL entries and with auto memory
> + * allocation).
> + *
> + * _type: type of structure pointed to
> + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
nit: probably these are outdated now
> + * start: size of (_type) pointed to (for auto memory allocation)
nit: I guess we need to drop this line about "start" field.
> + */
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5a6fd0c66..34223de818 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -869,8 +869,37 @@ static void vmstate_check(const VMStateDescription *vmsd)
> if (field) {
> while (field->name) {
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - assert(field->size == 0);
> + if (VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * Size must be provided because dest QEMU needs that
> + * info to know what to allocate
> + */
> + assert(field->size || field->size_offset);
> + } else {
> + /*
> + * Otherwise size info isn't useful (because it's
> + * always the size of host pointer), enforce accidental
> + * setup of sizes in this case.
> + */
> + assert(field->size == 0 && field->size_offset == 0);
> + }
> + /*
> + * VMS_ARRAY_OF_POINTER must be used only together with one
> + * of VMS_(V)ARRAY* flags.
> + */
> + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> + VMS_VARRAY_UINT32));
> }
> +
> + /*
> + * When VMS_ARRAY_OF_POINTER_ALLOW_NULL is used, we must have
> + * VMS_ARRAY_OF_POINTER set too.
> + */
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + }
> +
> if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> /* Recurse to sub structures */
> vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index d65fc84dfa..7d7d9c7e18 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> return true;
> }
>
> + if (byte == VMS_MARKER_PTR_VALID) {
> + /* We need to load this field right after the marker */
> + *load_field = true;
> + return true;
> + }
> +
> error_setg(errp, "Unexpected ptr marker: %d", byte);
> return false;
> }
> @@ -234,6 +240,22 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
> return true;
> }
>
> +
> +/*
> + * If we will use a ptr marker in the stream for a field? Two use cases:
> + *
> + * (1) When used with VMS_ARRAY_OF_POINTER_ALLOW_NULL, it must always be
> + * present to imply the population status of the pointer.
> + *
> + * (2) When used with normal VMSD array fields, only emit a null ptr marker
> + * if the pointer is NULL.
> + */
> +static inline bool
> +vmstate_use_marker_field(void *ptr, int size, bool dynamic_array)
> +{
> + return (!ptr && size) || dynamic_array;
> +}
> +
> bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, int version_id, Error **errp)
> {
> @@ -271,6 +293,12 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> vmstate_handle_alloc(first_elem, field, opaque);
> if (field->flags & VMS_POINTER) {
> @@ -282,18 +310,37 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> /* If we will process the load of field? */
> bool load_field = true;
> bool ok = true;
> - void *curr_elem = first_elem + size * i;
> + bool use_marker_field;
> + void *curr_elem_p = first_elem + size * i;
> + void *curr_elem = curr_elem_p;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - curr_elem = *(void **)curr_elem;
> + curr_elem = *(void **)curr_elem_p;
> }
>
> - if (!curr_elem && size) {
> - /* Read the marker instead of VMSD itself */
> + use_marker_field = vmstate_use_marker_field(curr_elem, size,
> + use_dynamic_array);
> + if (use_marker_field) {
> + /* Read the marker instead of VMSD first */
> if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> trace_vmstate_load_field_error(field->name, -EINVAL);
> return false;
> }
> +
> + if (load_field) {
> + /*
> + * When reaching here, it means we received a
> + * non-NULL ptr marker, so we need to populate the
> + * field before loading it.
> + *
> + * NOTE: do not use vmstate_size() here, because we
> + * need the object size, not entry size of the
> + * array.
> + */
> + curr_elem = g_malloc0(field->size);
> + /* Remember to update the root pointer! */
> + *(void **)curr_elem_p = curr_elem;
> + }
> }
>
> if (load_field) {
> @@ -397,6 +444,16 @@ static bool vmsd_can_compress(const VMStateField *field)
> return false;
> }
>
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * This may involve two VMSD fields to be dumped, one for the
> + * marker to show if the pointer is NULL, followed by the real
> + * vmstate object. To make it simple at least for now, skip
> + * compression for this one.
> + */
> + return false;
> + }
> +
> if (field->flags & VMS_STRUCT) {
> const VMStateField *sfield = field->vmsd->fields;
> while (sfield->name) {
> @@ -578,6 +635,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> bool use_marker_field_prev = false;
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -596,13 +659,10 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> curr_elem = *(void **)curr_elem;
> }
>
> - use_marker_field = !curr_elem && size;
> + use_marker_field = vmstate_use_marker_field(curr_elem, size,
> + use_dynamic_array);
> +
> if (use_marker_field) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> @@ -652,6 +712,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> goto out;
> }
>
> + /*
> + * If we're using dynamic array and the element is
> + * populated, dump the real object right after the marker.
> + */
> + if (use_dynamic_array && curr_elem) {
> + /*
> + * NOTE: do not use vmstate_size() here because we want
> + * to dump the real VMSD object now.
> + */
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> + field->size, vmsd,
> + field, vmdesc_loop,
> + i, max_elems, errp);
> +
> + if (!ok) {
> + error_prepend(errp, "Save of field %s/%s failed: ",
> + vmsd->name, field->name);
> + goto out;
> + }
> + }
> +
> /* Compressed arrays only care about the first element */
> if (vmdesc_loop && vmsd_can_compress(field)) {
> vmdesc_loop = NULL;
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-18 10:00 ` Alexander Mikhalitsyn
@ 2026-03-19 14:10 ` Peter Xu
0 siblings, 0 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-19 14:10 UTC (permalink / raw)
To: Alexander Mikhalitsyn; +Cc: qemu-devel, Juraj Marcin, Fabiano Rosas
On Wed, Mar 18, 2026 at 11:00:02AM +0100, Alexander Mikhalitsyn wrote:
> Am Mi., 18. März 2026 um 00:23 Uhr schrieb Peter Xu <peterx@redhat.com>:
> >
> > Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> > must be used together with VMS_ARRAY_OF_POINTER.
> >
> > It can be used to allow migration of an array of pointers where the
> > pointers may point to NULLs.
> >
> > Note that we used to allow migration of a NULL pointer within an array that
> > is being migrated. That corresponds to the code around vmstate_info_nullptr
> > where we may get/put one byte showing that the element of an array is NULL.
> >
> > That usage is fine but very limited, it's because even if it will migrate a
> > NULL pointer with a marker, it still works in a way that both src and dest
> > QEMUs must know exactly which elements of the array are non-NULL, so
> > instead of dynamically loading an array (which can have NULL pointers), it
> > actually only verifies the known NULL pointers are still NULL pointers
> > after migration.
> >
> > Also, in that case since dest QEMU knows exactly which element is NULL,
> > which is not NULL, dest QEMU's device code will manage all allocations for
> > the elements before invoking vmstate_load_vmsd().
> >
> > That's not enough per evolving needs of new device states that may want to
> > provide real dynamic array of pointers, like what Alexander proposed here
> > with the NVMe device migration:
> >
> > https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
> >
> > This patch is an alternative approach to address the problem.
> >
> > Along with the flag, introduce two new macros:
> >
> > VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
> >
> > Which will be used very soon in the NVMe series.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
>
> Amazing, thank you, Peter!
>
> Except small nitpicks I've left as inline comment in this patch, it is LGTM.
>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> Tested-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
>
> Also, I've rebased my NVMe series on top and tested everything
> locally. No regressions were found.
Thanks for the quick review and update. I'll wait for a few more days
before a non-RFC respin.
[...]
> > +/*
> > + * For migrating a dynamically allocated uint{8,32}-indexed array of
> > + * pointers to structures (with NULL entries and with auto memory
> > + * allocation).
> > + *
> > + * _type: type of structure pointed to
> > + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> > + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
>
> nit: probably these are outdated now
>
> > + * start: size of (_type) pointed to (for auto memory allocation)
>
> nit: I guess we need to drop this line about "start" field.
Yep I'll fix those, thanks.
--
Peter Xu
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-17 23:23 ` [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-18 10:00 ` Alexander Mikhalitsyn
@ 2026-03-19 22:06 ` Fabiano Rosas
2026-03-20 14:42 ` Fabiano Rosas
2 siblings, 0 replies; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-19 22:06 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx
Peter Xu <peterx@redhat.com> writes:
> Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> must be used together with VMS_ARRAY_OF_POINTER.
>
> It can be used to allow migration of an array of pointers where the
> pointers may point to NULLs.
>
> Note that we used to allow migration of a NULL pointer within an array that
> is being migrated. That corresponds to the code around vmstate_info_nullptr
> where we may get/put one byte showing that the element of an array is NULL.
>
> That usage is fine but very limited, it's because even if it will migrate a
> NULL pointer with a marker, it still works in a way that both src and dest
> QEMUs must know exactly which elements of the array are non-NULL, so
> instead of dynamically loading an array (which can have NULL pointers), it
> actually only verifies the known NULL pointers are still NULL pointers
> after migration.
>
> Also, in that case since dest QEMU knows exactly which element is NULL,
> which is not NULL, dest QEMU's device code will manage all allocations for
> the elements before invoking vmstate_load_vmsd().
>
> That's not enough per evolving needs of new device states that may want to
> provide real dynamic array of pointers, like what Alexander proposed here
> with the NVMe device migration:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This patch is an alternative approach to address the problem.
>
> Along with the flag, introduce two new macros:
>
> VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>
> Which will be used very soon in the NVMe series.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/migration/vmstate.h | 49 ++++++++++++++++-
> migration/savevm.c | 31 ++++++++++-
> migration/vmstate.c | 101 ++++++++++++++++++++++++++++++++----
> 3 files changed, 169 insertions(+), 12 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2e51b5ea04..70bebc60ed 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -161,8 +161,19 @@ enum VMStateFlags {
> * structure we are referencing to use. */
> VMS_VSTRUCT = 0x8000,
>
> + /*
> + * This is a sub-flag for VMS_ARRAY_OF_POINTER, so VMS_ARRAY_OF_POINTER
> + * must be set altogether. When set, it means array elements can
> + * contain either valid or NULL pointers, vmstate core will sync it
> + * between the two QEMU instances via the stream protocol. When it's a
Not sure I understand what "will sync it" means. Isn't that just what
migration does?
> + * valid pointer, the vmstate core will be responsible to do the proper
> + * memory allocations. It also means user of this flag must prepare
> + * the array to be all NULLs otherwise memory may leak.
I don't get this as well. What's prepare to be NULL?
> + */
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> +
> /* Marker for end of list */
> - VMS_END = 0x10000
> + VMS_END = 0x20000,
> };
>
> typedef enum {
> @@ -580,6 +591,42 @@ extern const VMStateInfo vmstate_info_qlist;
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> }
>
> +/*
> + * For migrating a dynamically allocated uint{8,32}-indexed array of
> + * pointers to structures (with NULL entries and with auto memory
> + * allocation).
> + *
> + * _type: type of structure pointed to
> + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
> + * start: size of (_type) pointed to (for auto memory allocation)
> + */
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5a6fd0c66..34223de818 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -869,8 +869,37 @@ static void vmstate_check(const VMStateDescription *vmsd)
> if (field) {
> while (field->name) {
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - assert(field->size == 0);
> + if (VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * Size must be provided because dest QEMU needs that
> + * info to know what to allocate
> + */
> + assert(field->size || field->size_offset);
> + } else {
> + /*
> + * Otherwise size info isn't useful (because it's
> + * always the size of host pointer), enforce accidental
s/enforce/detect/
> + * setup of sizes in this case.
> + */
> + assert(field->size == 0 && field->size_offset == 0);
> + }
> + /*
> + * VMS_ARRAY_OF_POINTER must be used only together with one
> + * of VMS_(V)ARRAY* flags.
> + */
> + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> + VMS_VARRAY_UINT32));
> }
> +
> + /*
> + * When VMS_ARRAY_OF_POINTER_ALLOW_NULL is used, we must have
The name has changed.
> + * VMS_ARRAY_OF_POINTER set too.
> + */
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + }
> +
> if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> /* Recurse to sub structures */
> vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index d65fc84dfa..7d7d9c7e18 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> return true;
> }
>
> + if (byte == VMS_MARKER_PTR_VALID) {
> + /* We need to load this field right after the marker */
s/this/the/
> + *load_field = true;
> + return true;
> + }
> +
> error_setg(errp, "Unexpected ptr marker: %d", byte);
> return false;
> }
> @@ -234,6 +240,22 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
> return true;
> }
>
> +
> +/*
> + * If we will use a ptr marker in the stream for a field? Two use cases:
> + *
> + * (1) When used with VMS_ARRAY_OF_POINTER_ALLOW_NULL, it must always be
AUTO_ALLOC
> + * present to imply the population status of the pointer.
> + *
> + * (2) When used with normal VMSD array fields, only emit a null ptr marker
> + * if the pointer is NULL.
> + */
> +static inline bool
> +vmstate_use_marker_field(void *ptr, int size, bool dynamic_array)
> +{
> + return (!ptr && size) || dynamic_array;
> +}
> +
> bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, int version_id, Error **errp)
> {
> @@ -271,6 +293,12 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
This is the load side, I'm not sure "push a ptr marker" makes sense
here.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> vmstate_handle_alloc(first_elem, field, opaque);
> if (field->flags & VMS_POINTER) {
> @@ -282,18 +310,37 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> /* If we will process the load of field? */
> bool load_field = true;
> bool ok = true;
> - void *curr_elem = first_elem + size * i;
> + bool use_marker_field;
> + void *curr_elem_p = first_elem + size * i;
> + void *curr_elem = curr_elem_p;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - curr_elem = *(void **)curr_elem;
> + curr_elem = *(void **)curr_elem_p;
> }
>
> - if (!curr_elem && size) {
> - /* Read the marker instead of VMSD itself */
> + use_marker_field = vmstate_use_marker_field(curr_elem, size,
> + use_dynamic_array);
> + if (use_marker_field) {
> + /* Read the marker instead of VMSD first */
> if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> trace_vmstate_load_field_error(field->name, -EINVAL);
> return false;
> }
> +
> + if (load_field) {
> + /*
> + * When reaching here, it means we received a
> + * non-NULL ptr marker, so we need to populate the
> + * field before loading it.
> + *
> + * NOTE: do not use vmstate_size() here, because we
> + * need the object size, not entry size of the
> + * array.
> + */
> + curr_elem = g_malloc0(field->size);
> + /* Remember to update the root pointer! */
> + *(void **)curr_elem_p = curr_elem;
> + }
> }
>
> if (load_field) {
> @@ -397,6 +444,16 @@ static bool vmsd_can_compress(const VMStateField *field)
> return false;
> }
>
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * This may involve two VMSD fields to be dumped, one for the
> + * marker to show if the pointer is NULL, followed by the real
> + * vmstate object. To make it simple at least for now, skip
> + * compression for this one.
> + */
> + return false;
> + }
> +
> if (field->flags & VMS_STRUCT) {
> const VMStateField *sfield = field->vmsd->fields;
> while (sfield->name) {
> @@ -578,6 +635,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> bool use_marker_field_prev = false;
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -596,13 +659,10 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> curr_elem = *(void **)curr_elem;
> }
>
> - use_marker_field = !curr_elem && size;
> + use_marker_field = vmstate_use_marker_field(curr_elem, size,
> + use_dynamic_array);
> +
> if (use_marker_field) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> @@ -652,6 +712,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> goto out;
> }
>
> + /*
> + * If we're using dynamic array and the element is
> + * populated, dump the real object right after the marker.
> + */
> + if (use_dynamic_array && curr_elem) {
> + /*
> + * NOTE: do not use vmstate_size() here because we want
> + * to dump the real VMSD object now.
> + */
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> + field->size, vmsd,
> + field, vmdesc_loop,
> + i, max_elems, errp);
> +
> + if (!ok) {
> + error_prepend(errp, "Save of field %s/%s failed: ",
> + vmsd->name, field->name);
> + goto out;
> + }
> + }
> +
> /* Compressed arrays only care about the first element */
> if (vmdesc_loop && vmsd_can_compress(field)) {
> vmdesc_loop = NULL;
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-17 23:23 ` [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-18 10:00 ` Alexander Mikhalitsyn
2026-03-19 22:06 ` Fabiano Rosas
@ 2026-03-20 14:42 ` Fabiano Rosas
2026-03-20 15:37 ` Peter Xu
2 siblings, 1 reply; 34+ messages in thread
From: Fabiano Rosas @ 2026-03-20 14:42 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx
Peter Xu <peterx@redhat.com> writes:
> Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> must be used together with VMS_ARRAY_OF_POINTER.
>
Sorry if I missed it somewhere, I was a bit tired yesterday when I
looked at this series, but why can't we reuse the currently invalid
VMS_ALLOC|VMS_ARRAY_OF_POINTER combination?
/* 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,
> It can be used to allow migration of an array of pointers where the
> pointers may point to NULLs.
>
> Note that we used to allow migration of a NULL pointer within an array that
> is being migrated. That corresponds to the code around vmstate_info_nullptr
> where we may get/put one byte showing that the element of an array is NULL.
>
> That usage is fine but very limited, it's because even if it will migrate a
> NULL pointer with a marker, it still works in a way that both src and dest
> QEMUs must know exactly which elements of the array are non-NULL, so
> instead of dynamically loading an array (which can have NULL pointers), it
> actually only verifies the known NULL pointers are still NULL pointers
> after migration.
>
> Also, in that case since dest QEMU knows exactly which element is NULL,
> which is not NULL, dest QEMU's device code will manage all allocations for
> the elements before invoking vmstate_load_vmsd().
>
> That's not enough per evolving needs of new device states that may want to
> provide real dynamic array of pointers, like what Alexander proposed here
> with the NVMe device migration:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This patch is an alternative approach to address the problem.
>
> Along with the flag, introduce two new macros:
>
> VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>
> Which will be used very soon in the NVMe series.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/migration/vmstate.h | 49 ++++++++++++++++-
> migration/savevm.c | 31 ++++++++++-
> migration/vmstate.c | 101 ++++++++++++++++++++++++++++++++----
> 3 files changed, 169 insertions(+), 12 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2e51b5ea04..70bebc60ed 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -161,8 +161,19 @@ enum VMStateFlags {
> * structure we are referencing to use. */
> VMS_VSTRUCT = 0x8000,
>
> + /*
> + * This is a sub-flag for VMS_ARRAY_OF_POINTER, so VMS_ARRAY_OF_POINTER
> + * must be set altogether. When set, it means array elements can
> + * contain either valid or NULL pointers, vmstate core will sync it
> + * between the two QEMU instances via the stream protocol. When it's a
> + * valid pointer, the vmstate core will be responsible to do the proper
> + * memory allocations. It also means user of this flag must prepare
> + * the array to be all NULLs otherwise memory may leak.
> + */
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> +
> /* Marker for end of list */
> - VMS_END = 0x10000
> + VMS_END = 0x20000,
> };
>
> typedef enum {
> @@ -580,6 +591,42 @@ extern const VMStateInfo vmstate_info_qlist;
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> }
>
> +/*
> + * For migrating a dynamically allocated uint{8,32}-indexed array of
> + * pointers to structures (with NULL entries and with auto memory
> + * allocation).
> + *
> + * _type: type of structure pointed to
> + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
> + * start: size of (_type) pointed to (for auto memory allocation)
> + */
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5a6fd0c66..34223de818 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -869,8 +869,37 @@ static void vmstate_check(const VMStateDescription *vmsd)
> if (field) {
> while (field->name) {
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - assert(field->size == 0);
> + if (VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * Size must be provided because dest QEMU needs that
> + * info to know what to allocate
> + */
> + assert(field->size || field->size_offset);
> + } else {
> + /*
> + * Otherwise size info isn't useful (because it's
> + * always the size of host pointer), enforce accidental
> + * setup of sizes in this case.
> + */
> + assert(field->size == 0 && field->size_offset == 0);
> + }
> + /*
> + * VMS_ARRAY_OF_POINTER must be used only together with one
> + * of VMS_(V)ARRAY* flags.
> + */
> + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> + VMS_VARRAY_UINT32));
> }
> +
> + /*
> + * When VMS_ARRAY_OF_POINTER_ALLOW_NULL is used, we must have
> + * VMS_ARRAY_OF_POINTER set too.
> + */
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + }
> +
> if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> /* Recurse to sub structures */
> vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index d65fc84dfa..7d7d9c7e18 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> return true;
> }
>
> + if (byte == VMS_MARKER_PTR_VALID) {
> + /* We need to load this field right after the marker */
> + *load_field = true;
> + return true;
> + }
> +
> error_setg(errp, "Unexpected ptr marker: %d", byte);
> return false;
> }
> @@ -234,6 +240,22 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
> return true;
> }
>
> +
> +/*
> + * If we will use a ptr marker in the stream for a field? Two use cases:
> + *
> + * (1) When used with VMS_ARRAY_OF_POINTER_ALLOW_NULL, it must always be
> + * present to imply the population status of the pointer.
> + *
> + * (2) When used with normal VMSD array fields, only emit a null ptr marker
> + * if the pointer is NULL.
> + */
> +static inline bool
> +vmstate_use_marker_field(void *ptr, int size, bool dynamic_array)
> +{
> + return (!ptr && size) || dynamic_array;
> +}
> +
> bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, int version_id, Error **errp)
> {
> @@ -271,6 +293,12 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> vmstate_handle_alloc(first_elem, field, opaque);
> if (field->flags & VMS_POINTER) {
> @@ -282,18 +310,37 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> /* If we will process the load of field? */
> bool load_field = true;
> bool ok = true;
> - void *curr_elem = first_elem + size * i;
> + bool use_marker_field;
> + void *curr_elem_p = first_elem + size * i;
> + void *curr_elem = curr_elem_p;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - curr_elem = *(void **)curr_elem;
> + curr_elem = *(void **)curr_elem_p;
> }
>
> - if (!curr_elem && size) {
> - /* Read the marker instead of VMSD itself */
> + use_marker_field = vmstate_use_marker_field(curr_elem, size,
> + use_dynamic_array);
> + if (use_marker_field) {
> + /* Read the marker instead of VMSD first */
> if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> trace_vmstate_load_field_error(field->name, -EINVAL);
> return false;
> }
> +
> + if (load_field) {
> + /*
> + * When reaching here, it means we received a
> + * non-NULL ptr marker, so we need to populate the
> + * field before loading it.
> + *
> + * NOTE: do not use vmstate_size() here, because we
> + * need the object size, not entry size of the
> + * array.
> + */
> + curr_elem = g_malloc0(field->size);
> + /* Remember to update the root pointer! */
> + *(void **)curr_elem_p = curr_elem;
> + }
> }
>
> if (load_field) {
> @@ -397,6 +444,16 @@ static bool vmsd_can_compress(const VMStateField *field)
> return false;
> }
>
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * This may involve two VMSD fields to be dumped, one for the
> + * marker to show if the pointer is NULL, followed by the real
> + * vmstate object. To make it simple at least for now, skip
> + * compression for this one.
> + */
> + return false;
> + }
> +
> if (field->flags & VMS_STRUCT) {
> const VMStateField *sfield = field->vmsd->fields;
> while (sfield->name) {
> @@ -578,6 +635,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> bool use_marker_field_prev = false;
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -596,13 +659,10 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> curr_elem = *(void **)curr_elem;
> }
>
> - use_marker_field = !curr_elem && size;
> + use_marker_field = vmstate_use_marker_field(curr_elem, size,
> + use_dynamic_array);
> +
> if (use_marker_field) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> @@ -652,6 +712,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> goto out;
> }
>
> + /*
> + * If we're using dynamic array and the element is
> + * populated, dump the real object right after the marker.
> + */
> + if (use_dynamic_array && curr_elem) {
> + /*
> + * NOTE: do not use vmstate_size() here because we want
> + * to dump the real VMSD object now.
> + */
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> + field->size, vmsd,
> + field, vmdesc_loop,
> + i, max_elems, errp);
> +
> + if (!ok) {
> + error_prepend(errp, "Save of field %s/%s failed: ",
> + vmsd->name, field->name);
> + goto out;
> + }
> + }
> +
> /* Compressed arrays only care about the first element */
> if (vmdesc_loop && vmsd_can_compress(field)) {
> vmdesc_loop = NULL;
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-20 14:42 ` Fabiano Rosas
@ 2026-03-20 15:37 ` Peter Xu
0 siblings, 0 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-20 15:37 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin
On Fri, Mar 20, 2026 at 11:42:36AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> > must be used together with VMS_ARRAY_OF_POINTER.
> >
>
> Sorry if I missed it somewhere, I was a bit tired yesterday when I
I haven't read it, I will read it, at some point.. Fridays normally aren't
as busy.
Since this one is shorter I'll reply fast,
> looked at this series, but why can't we reuse the currently invalid
> VMS_ALLOC|VMS_ARRAY_OF_POINTER combination?
This only handles allocation of the field as a whole. Say if it's an array
this makes sure the array will be allocated properly on dest. It doesn't
cover the fields in the array.
Btw, it's exactly the given example described below that this wants to do:
>
> /* 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. */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > <------------------- here
> VMS_ALLOC = 0x2000,
--
Peter Xu
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH RFC 10/10] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_ALLOW_NULL
2026-03-17 23:23 [PATCH RFC 00/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (8 preceding siblings ...)
2026-03-17 23:23 ` [PATCH RFC 09/10] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
@ 2026-03-17 23:23 ` Peter Xu
9 siblings, 0 replies; 34+ messages in thread
From: Peter Xu @ 2026-03-17 23:23 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, Juraj Marcin, peterx, Fabiano Rosas,
Alexander Mikhalitsyn
From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Add tests for VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC.
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
[peterx: Removed two tests due to macro not used, rebase, fix warning]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/unit/test-vmstate.c | 86 +++++++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index dae15786aa..df1fb4c778 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -702,6 +702,88 @@ static void test_arr_ptr_prim_0_load(void)
}
}
+static uint8_t wire_arr_ptr_with_nulls[] = {
+ VMS_MARKER_PTR_VALID,
+ 0x00, 0x00, 0x00, 0x00,
+ VMS_MARKER_PTR_NULL,
+ VMS_MARKER_PTR_VALID,
+ 0x00, 0x00, 0x00, 0x02,
+ VMS_MARKER_PTR_VALID,
+ 0x00, 0x00, 0x00, 0x03,
+ QEMU_VM_EOF
+};
+
+typedef struct {
+ uint32_t ar_items_num;
+ TestStructTriv **ar;
+} TestVArrayOfPtrToStuctWithNULLs;
+
+const VMStateDescription vmsd_arps_with_nulls = {
+ .name = "test/arps_with_nulls",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (const VMStateField[]) {
+ VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(
+ ar, TestVArrayOfPtrToStuctWithNULLs, ar_items_num,
+ 0, vmsd_tst, TestStructTriv),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void test_arr_ptr_nulls_str_save(void)
+{
+ TestStructTriv ar[AR_SIZE] = { {.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+ TestVArrayOfPtrToStuctWithNULLs sample = {};
+ int idx;
+
+ sample.ar_items_num = AR_SIZE;
+ sample.ar = g_new0(TestStructTriv*, sample.ar_items_num);
+ sample.ar[0] = g_new0(TestStructTriv, 1);
+ *sample.ar[0] = ar[0];
+ /* note, sample.ar[1] remains NULL */
+ sample.ar[2] = g_new0(TestStructTriv, 1);
+ *sample.ar[2] = ar[2];
+ sample.ar[3] = g_new0(TestStructTriv, 1);
+ *sample.ar[3] = ar[3];
+
+ save_vmstate(&vmsd_arps_with_nulls, &sample);
+ compare_vmstate(wire_arr_ptr_with_nulls, sizeof(wire_arr_ptr_with_nulls));
+
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ g_free(sample.ar[idx]);
+ }
+ g_free(sample.ar);
+}
+
+static void test_arr_ptr_nulls_str_load(void)
+{
+ TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
+ TestVArrayOfPtrToStuctWithNULLs obj = {};
+ int idx;
+
+ obj.ar_items_num = AR_SIZE;
+ obj.ar = g_new0(TestStructTriv*, obj.ar_items_num);
+
+ save_buffer(wire_arr_ptr_with_nulls, sizeof(wire_arr_ptr_with_nulls));
+ SUCCESS(load_vmstate_one(
+ &vmsd_arps_with_nulls, &obj, 1,
+ wire_arr_ptr_with_nulls, sizeof(wire_arr_ptr_with_nulls)));
+
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ if (idx == 1) {
+ g_assert_cmpint((uintptr_t)(obj.ar[idx]), ==, 0);
+ } else {
+ /* compare the target array ar with the ground truth array ar_gt */
+ g_assert_cmpint(ar_gt[idx].i, ==, obj.ar[idx]->i);
+ }
+ }
+
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ g_free(obj.ar[idx]);
+ }
+ g_free(obj.ar);
+}
+
/* test QTAILQ migration */
typedef struct TestQtailqElement TestQtailqElement;
@@ -1568,6 +1650,10 @@ int main(int argc, char **argv)
test_arr_ptr_prim_0_save);
g_test_add_func("/vmstate/array/ptr/prim/0/load",
test_arr_ptr_prim_0_load);
+ g_test_add_func("/vmstate/array/ptr-nulls/str/save",
+ test_arr_ptr_nulls_str_save);
+ g_test_add_func("/vmstate/array/ptr-nulls/str/load",
+ test_arr_ptr_nulls_str_load);
g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q);
g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q);
g_test_add_func("/vmstate/gtree/save/savedomain", test_gtree_save_domain);
--
2.50.1
^ permalink raw reply related [flat|nested] 34+ messages in thread