[PATCH 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[PATCH 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Peter Xu]

CI: https://gitlab.com/peterx/qemu/-/pipelines/2424266591
    (one checkpatch warning, not relevant to this series)

rfcv1: https://lore.kernel.org/r/20260317232332.15209-1-peterx@redhat.com
rfcv2: https://lore.kernel.org/r/20260326210532.379027-1-peterx@redhat.com

Changelog rfcv2->v1:

- Rebased to latest fabiano's migration-staging branch (bd698d7730)
- Collected new tags
- Patch 1
  - Add space between "_type*" [Phil]
- Patch 9
  - Slight refactoring on vmstate_load_next() [Fabiano]
  - I collected Alexander's tested-by even if strictly I shouldn't when the
    patch changed; if test fails, it's me to blame.

This is v1, I removed RFC as I we should have reached some form of
consensus in general.  It is based on Fabiano's migration-staging branch.
I've pushed this tree here for easier reference:

https://gitlab.com/peterx/qemu/-/tree/vmstate-array-null

This series implements a new vmstate flag that allows real dynamic array of
pointers to be migrated, so that QEMU vmstate core will be able to identify
NULL and non-NULL pointers pointing to any kind of VMSD (or base types),
then properly allocate memory for them on dest QEMU.

The first planned user of such is Alexander's NVMe migration series here:

https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com

This is another alternative to solve this problem.

I tested analyze-migration.py with this version and it works with an idle
dump from the nvme series (after I rebase).

Thanks,

Alexander Mikhalitsyn (1):
  tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC

Fabiano Rosas (1):
  vmstate: Stop checking size for nullptr compression

Peter Xu (9):
  vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
  vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
  vmstate: Do not set size for VMS_ARRAY_OF_POINTER
  vmstate: Update max_elems early and check field compressable once
  vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
  vmstate: Introduce vmstate_save_field_with_vmdesc()
  vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
  vmstate: Implement load of ptr marker in vmstate core
  vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

 include/hw/intc/riscv_aclint.h |   6 +-
 include/migration/vmstate.h    |  88 ++++++++++--
 migration/savevm.c             |  28 ++++
 migration/vmstate-types.c      |  36 ++---
 migration/vmstate.c            | 247 ++++++++++++++++++++++++++-------
 tests/unit/test-vmstate.c      |  90 +++++++++++-
 roms/seabios-hppa              |   2 +-
 scripts/analyze-migration.py   |  22 +--
 8 files changed, 418 insertions(+), 101 deletions(-)

-- 
2.50.1

[PATCH 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER

[Peter Xu]

Passing in a pointer almost never helps.  Convert it to pass in struct for
further refactoring on VMS_ARRAY_OF_POINTER.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/vmstate.h | 6 +++---
 tests/unit/test-vmstate.c   | 2 +-
 roms/seabios-hppa           | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index d4a39aa794..d4880e75c2 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()
     }
 };
diff --git a/roms/seabios-hppa b/roms/seabios-hppa
index cf3a472f4d..1a8ada1fb7 160000
--- a/roms/seabios-hppa
+++ b/roms/seabios-hppa
@@ -1 +1 @@
-Subproject commit cf3a472f4ddea30d2d483a8ba4b005505f0dabb3
+Subproject commit 1a8ada1fb70643172e251aacbac673c9ecda99e9
-- 
2.50.1

[PATCH 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32

[Peter Xu]

Passing in a pointer almost never helps.  Convert it to pass in struct for
further refactoring on VMS_ARRAY_OF_POINTER.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 d4880e75c2..32bb8e8ebc 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

[PATCH 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER

[Peter Xu]

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.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 32bb8e8ebc..350e62a55a 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

[PATCH 04/11] vmstate: Update max_elems early and check field compressable once

[Peter Xu]

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.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
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

[PATCH 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL

[Peter Xu]

Prepare for a new MARKER for non-NULL pointer.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 350e62a55a..7396e38531 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

[PATCH 06/11] vmstate: Introduce vmstate_save_field_with_vmdesc()

[Peter Xu]

Introduce a helper to do both the JSON blob generations and save vmstate.
This further shrinks the function a bit.  More importantly, we'll need to
save two fields in one loop very soon in the future with the JSON blob.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 05badef42f..b274204e66 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -514,6 +514,35 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
     return true;
 }
 
+/*
+ * Save 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);
+
+    if (!ok) {
+        error_prepend(errp, "Save of field %s/%s failed: ",
+                      vmsd->name, field->name);
+    }
+
+    return ok;
+}
+
 static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                                 void *opaque, JSONWriter *vmdesc,
                                 int version_id, Error **errp)
@@ -542,7 +571,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;
 
@@ -559,7 +587,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                 /* 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) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
@@ -606,15 +633,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) {
@@ -622,8 +643,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                 }
 
                 if (!ok) {
-                    error_prepend(errp, "Save of field %s/%s failed: ",
-                                  vmsd->name, field->name);
                     goto out;
                 }
 
-- 
2.50.1

[PATCH 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers

[Peter Xu]

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.

Update analyze-migration.py to work with the new layout.

No functional change intended yet.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/vmstate.h  |  9 +++++++--
 migration/vmstate-types.c    | 34 ++++++++++++++++------------------
 migration/vmstate.c          | 25 +++++++++++++------------
 scripts/analyze-migration.py | 22 ++++++++++++----------
 4 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7396e38531..492069b8d2 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 b274204e66..b333aa1744 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);
 
@@ -71,12 +71,12 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
     fake->name = field->name;
     fake->version_id = field->version_id;
 
-    /* Do not need "field_exists" check as it always exists (which is null) */
+    /* Do not need "field_exists" check as it always exists */
     fake->field_exists = NULL;
 
-    /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
+    /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
     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;
                 }
@@ -583,26 +583,27 @@ 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;
                 /* maximum number of elements to compress in the JSON blob */
                 int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
+                bool use_marker_field, is_null;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
                 }
 
-                if (!curr_elem && size) {
+                is_null = !curr_elem && size;
+                use_marker_field = is_null;
+
+                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;
                 }
 
                 /*
@@ -638,7 +639,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);
                 }
 
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

[PATCH 08/11] vmstate: Implement load of ptr marker in vmstate core

[Peter Xu]

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.

When at it, not check "size" anymore for existing NULL markers, and move it
under the same VMS_ARRAY_OF_POINTER section because that's the only place
that NULL marker can happen (which guarantess size==host ptr size, which is
non-zero).

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate-types.c | 12 ++++------
 migration/vmstate.c       | 46 ++++++++++++++++++++++++---------------
 2 files changed, 32 insertions(+), 26 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 b333aa1744..47812eb882 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) {
+                        /* 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;
+                        }
+                    }
                 }
 
-                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;
-                }
-
-                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

[PATCH 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Peter Xu]

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.

Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Tested-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/vmstate.h |  51 ++++++++++++-
 migration/savevm.c          |  27 ++++++-
 migration/vmstate.c         | 145 ++++++++++++++++++++++++++++++------
 3 files changed, 199 insertions(+), 24 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 492069b8d2..28e3640e60 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -161,8 +161,21 @@ enum VMStateFlags {
      * structure we are referencing to use. */
     VMS_VSTRUCT           = 0x8000,
 
+    /*
+     * This is a sub-flag for VMS_ARRAY_OF_POINTER.  When this flag is set,
+     * VMS_ARRAY_OF_POINTER must also be set.  When set, it means array
+     * elements can contain either valid or NULL pointers, vmstate core
+     * will be responsible for synchronizing the pointer status, providing
+     * proper memory allocations on the pointer when it is populated on the
+     * source QEMU.  It also means the user of the field must make sure all
+     * the elements in the array are NULL pointers before loading.  This
+     * should also work with VMS_ALLOC when the array itself also needs to
+     * be allocated.
+     */
+    VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
+
     /* Marker for end of list */
-    VMS_END = 0x10000
+    VMS_END                         = 0x20000,
 };
 
 typedef enum {
@@ -580,6 +593,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..765df8ce2d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -869,8 +869,33 @@ 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_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), detect 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));
             }
+
+            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 47812eb882..de2ad822e8 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 the field right after the marker */
+        *load_field = true;
+        return true;
+    }
+
     error_setg(errp, "Unexpected ptr marker: %d", byte);
     return false;
 }
@@ -234,6 +240,76 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
     return true;
 }
 
+/*
+ * Try to prepare loading the next element, the object pointer to be put
+ * into @next_elem.  When @next_elem is NULL, it means we should skip
+ * loading this element.
+ *
+ * Returns false for errors, in which case *errp will be set, migration
+ * must be aborted.
+ */
+static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
+                              void *first_elem, void **next_elem,
+                              int size, int i, Error **errp)
+{
+    bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
+    void *ptr = first_elem + size * i, **pptr;
+    bool load_field;
+
+    if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
+        /* Simplest case, no pointer involved */
+        *next_elem = ptr;
+        return true;
+    }
+
+    /*
+     * We're loading an array of pointers, switch to use pptr to make it
+     * easier to read later
+     */
+    pptr = (void **)ptr;
+
+    /*
+     * If auto_alloc is on, making sure the user provided an array of NULL
+     * pointers to start with
+     */
+    assert(!auto_alloc || *pptr == NULL);
+
+    /*
+     * When pointer is null, we must expect a ptr marker first.  Use cases:
+     *
+     * (1) _AUTO_ALLOC implies a ptr marker will always exist, or,
+     *
+     * (2) the element on destination is NULL, which expects the src to send a
+     *     NULL-only marker.
+     *
+     * Here, checking against a NULL pointer will work for both.
+     */
+    if (!*pptr) {
+        if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
+            trace_vmstate_load_field_error(field->name, -EINVAL);
+            return false;
+        }
+
+        /*
+         * If loading is needed, do pre-allocation first (otherwise keeping
+         * *pptr==NULL to imply a skip below)
+         */
+        if (load_field) {
+            /* Only applies when auto_alloc=on on the field */
+            assert(auto_alloc);
+            /*
+             * NOTE: do not use vmstate_size() here, because we need the
+             * object size, not entry size of the array.
+             */
+            *pptr = g_malloc0(field->size);
+        }
+    }
+
+    /* Move the cursor to the next element for loading */
+    *next_elem = *pptr;
+    return true;
+}
+
 bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id, Error **errp)
 {
@@ -279,27 +355,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
             }
 
             for (i = 0; i < n_elems; i++) {
-                /* If we will process the load of field? */
-                bool load_field = true;
-                bool ok = true;
-                void *curr_elem = first_elem + size * i;
+                void *curr_elem;
+                bool ok;
 
-                if (field->flags & VMS_ARRAY_OF_POINTER) {
-                    curr_elem = *(void **)curr_elem;
-                    if (!curr_elem) {
-                        /* 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_next(f, field, first_elem, &curr_elem,
+                                       size, i, errp);
+                if (!ok) {
+                    return false;
                 }
 
-                if (load_field) {
-                    ok = vmstate_load_field(f, curr_elem, size, field, errp);
+                if (!curr_elem) {
+                    /* Implies a skip */
+                    continue;
                 }
 
+                ok = vmstate_load_field(f, curr_elem, size, field, errp);
+
                 if (ok) {
                     int ret = qemu_file_get_error(f);
                     if (ret < 0) {
@@ -397,6 +468,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 saved, 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) {
@@ -583,6 +664,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
             int size = vmstate_size(opaque, field);
             JSONWriter *vmdesc_loop = vmdesc;
             bool is_prev_null = 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) {
@@ -603,14 +690,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                 }
 
                 is_null = !curr_elem && size;
-                use_marker_field = is_null;
+                use_marker_field = use_dynamic_array || is_null;
 
                 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;
@@ -657,6 +739,25 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                     goto out;
                 }
 
+                /*
+                 * If we're using dynamic array and the element is
+                 * populated, save the real object right after the marker.
+                 */
+                if (use_dynamic_array && curr_elem) {
+                    /*
+                     * NOTE: do not use vmstate_size() here because we want
+                     * to save 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) {
+                        goto out;
+                    }
+                }
+
                 /* Compressed arrays only care about the first element */
                 if (vmdesc_loop && max_elems > 1) {
                     vmdesc_loop = NULL;
-- 
2.50.1

[PATCH 10/11] vmstate: Stop checking size for nullptr compression

[Peter Xu]

From: Fabiano Rosas <farosas@suse.de>

The NULL pointer marker code applies only to VMS_ARRAY_OF_POINTER,
where the size is never NULL. Move the setting of is_null under
VMS_ARRAY_OF_POINTER, so we can stop checking the size.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index de2ad822e8..2f13b48a37 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -682,14 +682,14 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                 const VMStateField *inner_field;
                 /* maximum number of elements to compress in the JSON blob */
                 int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
-                bool use_marker_field, is_null;
+                bool use_marker_field, is_null = false;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
+                    is_null = !curr_elem;
                 }
 
-                is_null = !curr_elem && size;
                 use_marker_field = use_dynamic_array || is_null;
 
                 if (use_marker_field) {
@@ -717,7 +717,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
 
                     for (int j = i + 1; j < n_elems; j++) {
                         void *elem = *(void **)(first_elem + size * j);
-                        bool elem_is_null = !elem && size;
+                        bool elem_is_null = !elem;
 
                         if (is_null != elem_is_null) {
                             max_elems = j - i;
-- 
2.50.1

[PATCH 11/11] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Peter Xu]

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

Re: [PATCH 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Juraj Marcin]

Hi Peter,

On 2026-04-01 16:28, Peter Xu wrote:
> CI: https://gitlab.com/peterx/qemu/-/pipelines/2424266591
>     (one checkpatch warning, not relevant to this series)
> 
> rfcv1: https://lore.kernel.org/r/20260317232332.15209-1-peterx@redhat.com
> rfcv2: https://lore.kernel.org/r/20260326210532.379027-1-peterx@redhat.com
> 
> Changelog rfcv2->v1:
> 
> - Rebased to latest fabiano's migration-staging branch (bd698d7730)
> - Collected new tags
> - Patch 1
>   - Add space between "_type*" [Phil]
> - Patch 9
>   - Slight refactoring on vmstate_load_next() [Fabiano]
>   - I collected Alexander's tested-by even if strictly I shouldn't when the
>     patch changed; if test fails, it's me to blame.
> 
> This is v1, I removed RFC as I we should have reached some form of
> consensus in general.  It is based on Fabiano's migration-staging branch.
> I've pushed this tree here for easier reference:
> 
> https://gitlab.com/peterx/qemu/-/tree/vmstate-array-null
> 
> This series implements a new vmstate flag that allows real dynamic array of
> pointers to be migrated, so that QEMU vmstate core will be able to identify
> NULL and non-NULL pointers pointing to any kind of VMSD (or base types),
> then properly allocate memory for them on dest QEMU.
> 
> The first planned user of such is Alexander's NVMe migration series here:
> 
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
> 
> This is another alternative to solve this problem.
> 
> I tested analyze-migration.py with this version and it works with an idle
> dump from the nvme series (after I rebase).
> 
> Thanks,
> 
> Alexander Mikhalitsyn (1):
>   tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC
> 
> Fabiano Rosas (1):
>   vmstate: Stop checking size for nullptr compression
> 
> Peter Xu (9):
>   vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
>   vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
>   vmstate: Do not set size for VMS_ARRAY_OF_POINTER
>   vmstate: Update max_elems early and check field compressable once
>   vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
>   vmstate: Introduce vmstate_save_field_with_vmdesc()
>   vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
>   vmstate: Implement load of ptr marker in vmstate core
>   vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
> 
>  include/hw/intc/riscv_aclint.h |   6 +-
>  include/migration/vmstate.h    |  88 ++++++++++--
>  migration/savevm.c             |  28 ++++
>  migration/vmstate-types.c      |  36 ++---
>  migration/vmstate.c            | 247 ++++++++++++++++++++++++++-------
>  tests/unit/test-vmstate.c      |  90 +++++++++++-
>  roms/seabios-hppa              |   2 +-
>  scripts/analyze-migration.py   |  22 +--
>  8 files changed, 418 insertions(+), 101 deletions(-)
> 
> -- 
> 2.50.1
> 

For the whole series:

Reviewed-by: Juraj Marcin <jmarcin@redhat.com>

Re: [PATCH 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Alexander Mikhalitsyn]

Dear friends,

when I can expect this to be landed in master? I would like to prepare
and send next version of my
NVMe live migration patchset, that's why I'm asking.

Kind regards,
Alex

Am Mi., 1. Apr. 2026 um 22:28 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> CI: https://gitlab.com/peterx/qemu/-/pipelines/2424266591
>     (one checkpatch warning, not relevant to this series)
>
> rfcv1: https://lore.kernel.org/r/20260317232332.15209-1-peterx@redhat.com
> rfcv2: https://lore.kernel.org/r/20260326210532.379027-1-peterx@redhat.com
>
> Changelog rfcv2->v1:
>
> - Rebased to latest fabiano's migration-staging branch (bd698d7730)
> - Collected new tags
> - Patch 1
>   - Add space between "_type*" [Phil]
> - Patch 9
>   - Slight refactoring on vmstate_load_next() [Fabiano]
>   - I collected Alexander's tested-by even if strictly I shouldn't when the
>     patch changed; if test fails, it's me to blame.
>
> This is v1, I removed RFC as I we should have reached some form of
> consensus in general.  It is based on Fabiano's migration-staging branch.
> I've pushed this tree here for easier reference:
>
> https://gitlab.com/peterx/qemu/-/tree/vmstate-array-null
>
> This series implements a new vmstate flag that allows real dynamic array of
> pointers to be migrated, so that QEMU vmstate core will be able to identify
> NULL and non-NULL pointers pointing to any kind of VMSD (or base types),
> then properly allocate memory for them on dest QEMU.
>
> The first planned user of such is Alexander's NVMe migration series here:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This is another alternative to solve this problem.
>
> I tested analyze-migration.py with this version and it works with an idle
> dump from the nvme series (after I rebase).
>
> Thanks,
>
> Alexander Mikhalitsyn (1):
>   tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC
>
> Fabiano Rosas (1):
>   vmstate: Stop checking size for nullptr compression
>
> Peter Xu (9):
>   vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
>   vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
>   vmstate: Do not set size for VMS_ARRAY_OF_POINTER
>   vmstate: Update max_elems early and check field compressable once
>   vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
>   vmstate: Introduce vmstate_save_field_with_vmdesc()
>   vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
>   vmstate: Implement load of ptr marker in vmstate core
>   vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
>
>  include/hw/intc/riscv_aclint.h |   6 +-
>  include/migration/vmstate.h    |  88 ++++++++++--
>  migration/savevm.c             |  28 ++++
>  migration/vmstate-types.c      |  36 ++---
>  migration/vmstate.c            | 247 ++++++++++++++++++++++++++-------
>  tests/unit/test-vmstate.c      |  90 +++++++++++-
>  roms/seabios-hppa              |   2 +-
>  scripts/analyze-migration.py   |  22 +--
>  8 files changed, 418 insertions(+), 101 deletions(-)
>
> --
> 2.50.1
>

Re: [PATCH 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Fabiano Rosas]

Alexander Mikhalitsyn <alexander@mihalicyn.com> writes:

> Dear friends,
>
> when I can expect this to be landed in master? I would like to prepare
> and send next version of my
> NVMe live migration patchset, that's why I'm asking.
>

I intend to send a pull request as soon as the release happens and the
next development cycle opens.  Probably by the end of this week, unless
we need to have another -rc.

Re: [PATCH 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Alexander Mikhalitsyn]

Am Di., 14. Apr. 2026 um 17:23 Uhr schrieb Fabiano Rosas <farosas@suse.de>:
>
> Alexander Mikhalitsyn <alexander@mihalicyn.com> writes:
>
> > Dear friends,
> >
> > when I can expect this to be landed in master? I would like to prepare
> > and send next version of my
> > NVMe live migration patchset, that's why I'm asking.
> >
>
> I intend to send a pull request as soon as the release happens and the
> next development cycle opens.  Probably by the end of this week, unless
> we need to have another -rc.
>

Thank you, Fabiano! And sorry for bothering with this question ;-)

Kind regards,
Alex

Re: [PATCH 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Passing in a pointer almost never helps.  Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/vmstate.h | 6 +++---
>  tests/unit/test-vmstate.c   | 2 +-
>  roms/seabios-hppa           | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index d4a39aa794..d4880e75c2 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()
>      }
>  };
> diff --git a/roms/seabios-hppa b/roms/seabios-hppa
> index cf3a472f4d..1a8ada1fb7 160000
> --- a/roms/seabios-hppa
> +++ b/roms/seabios-hppa
> @@ -1 +1 @@
> -Subproject commit cf3a472f4ddea30d2d483a8ba4b005505f0dabb3
> +Subproject commit 1a8ada1fb70643172e251aacbac673c9ecda99e9

May I delete this hunk?

Re: [PATCH 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER

[Peter Xu]

On Mon, Apr 20, 2026 at 04:25:43PM -0300, Fabiano Rosas wrote:
> > diff --git a/roms/seabios-hppa b/roms/seabios-hppa
> > index cf3a472f4d..1a8ada1fb7 160000
> > --- a/roms/seabios-hppa
> > +++ b/roms/seabios-hppa
> > @@ -1 +1 @@
> > -Subproject commit cf3a472f4ddea30d2d483a8ba4b005505f0dabb3
> > +Subproject commit 1a8ada1fb70643172e251aacbac673c9ecda99e9
> 
> May I delete this hunk?

:-D

:-(

Yes..

-- 
Peter Xu