Re: [PATCH 2/4] migration: Introduce VMStateOffset

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 31, 2026 at 11:02:27AM -0300, Fabiano Rosas wrote:
>> Hi Vladimir, I've been looking at this, could you clarify which vmstates
>> do you think we could merge? I don't see it, either VARRAY vs. VBUFFER
>> or VARRAY vs. BUFFER, also ARRAY vs. BUFFER doesn't seem to work.
>> 
>> One main point of difference is the size_offset/num_offset variants are
>> only known at load-time, so we can't convert them between each other at
>> build time because the either the total size or num will not be know.
>
> Something like this?
>
> NOTE: I hid a fix to a comment that is irrelevant.. which I mentioned @size
> and @size_offset can't co-exist, but it can when VMS_MULTIPLY is set.  I
> had a vague feeling we can further clean vmstate flags.

Is there a reason we cannot use .num for MULTIPLY instead of .size?

>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 24631fd678..3a02709650 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -69,17 +69,15 @@ enum VMStateFlags {
>       * }). Dereference the pointer before using it as basis for
>       * further pointer arithmetic (see e.g. VMS_ARRAY). Does not
>       * affect the meaning of VMStateField.num_offset or
> -     * VMStateField.size_offset; see VMS_VARRAY and VMS_VBUFFER for
> -     * those.
> +     * VMStateField.size_offset; see VMS_VARRAY for those.
>       */
>      VMS_POINTER          = 0x002,
>  
>      /* The field is an array of fixed size. VMStateField.num contains
>       * the number of entries in the array. The size of each entry is
>       * given by VMStateField.size and / or opaque +
> -     * VMStateField.size_offset; see VMS_VBUFFER and
> -     * VMS_MULTIPLY. Each array entry will be processed individually
> -     * (VMStateField.info.get()/put() if VMS_STRUCT is not set,
> +     * VMStateField.size_offset. Each array entry will be processed
> +     * individually (VMStateField.info.get()/put() if VMS_STRUCT is not set,
>       * recursion into VMStateField.vmsd if VMS_STRUCT is set). May not
>       * be combined with VMS_VARRAY.
>       */
> @@ -108,18 +106,10 @@ enum VMStateFlags {
>       */
>      VMS_ARRAY_OF_POINTER = 0x040,
>  
> -    /* The size of the individual entries (a single array entry if
> -     * VMS_ARRAY or VMS_VARRAY are set, or the field itself if
> -     * neither is set) is variable (i.e. not known at compile-time),
> -     * but the same for all entries. Use the int32_t at opaque +
> -     * VMStateField.size_offset (subject to VMS_MULTIPLY) to determine
> -     * the size of each (and every) entry. */
> -    VMS_VBUFFER          = 0x100,
> -
>      /* Multiply the entry size given by the int32_t at opaque +
> -     * VMStateField.size_offset (see VMS_VBUFFER description) with
> -     * VMStateField.size to determine the number of bytes to be
> -     * allocated. Only valid in combination with VMS_VBUFFER. */
> +     * VMStateField.size_offset with VMStateField.size to determine the
> +     * number of bytes to be allocated.
> +     */
>      VMS_MULTIPLY         = 0x200,
>  
>      /* Fail loading the serialised VM state if this field is missing
> @@ -181,8 +171,8 @@ struct VMStateField {
>  
>      /*
>       * @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
> +     * the field.  Only one of them should be present never both, except
> +     * VMS_MULTIPLY.  When @size_offset is used, 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
> @@ -696,7 +686,7 @@ extern const VMStateInfo vmstate_info_g_byte_array;
>      .size_offset  = vmstate_field_offset(_state, _field_size),       \
>      .size         = (_multiply),                                      \
>      .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER|VMS_MULTIPLY,            \
> +    .flags        = VMS_VARRAY|VMS_POINTER|VMS_MULTIPLY,              \
>      .offset       = offsetof(_state, _field),                        \
>  }
>  
> @@ -706,7 +696,7 @@ extern const VMStateInfo vmstate_info_g_byte_array;
>      .field_exists = (_test),                                         \
>      .size_offset  = vmstate_field_offset(_state, _field_size),       \
>      .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
> +    .flags        = VMS_VARRAY|VMS_POINTER,                          \
>      .offset       = offsetof(_state, _field),                        \
>  }
>  
> @@ -720,7 +710,7 @@ extern const VMStateInfo vmstate_info_g_byte_array;
>      .field_exists = (_test),                                         \
>      .size_offset  = vmstate_field_offset(_state, _field_size),       \
>      .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC,               \
> +    .flags        = VMS_VARRAY|VMS_POINTER|VMS_ALLOC,                \
>      .offset       = offsetof(_state, _field),                        \
>  }
>  
> @@ -798,7 +788,7 @@ extern const VMStateInfo vmstate_info_g_byte_array;
>      .version_id   = (_version),                                      \
>      .size_offset  = vmstate_field_offset(_state, _field_size),       \
>      .info         = &vmstate_info_bitmap,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
> +    .flags        = VMS_VARRAY|VMS_POINTER,                          \
>      .offset       = offsetof(_state, _field),                        \
>  }
>  
> @@ -1200,9 +1190,6 @@ extern const VMStateInfo vmstate_info_g_byte_array;
>  #define VMSTATE_BUFFER_START_MIDDLE(_f, _s, _start) \
>      VMSTATE_BUFFER_START_MIDDLE_V(_f, _s, _start, 0)
>  
> -#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, _size)
> -
>  #define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
>      VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, _size)
>  
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index bc8eb3d3ca..f7363a8776 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -102,7 +102,7 @@ static uint64_t vmstate_n_elems(void *opaque, const VMStateField *field)
>  
>      if (field->flags & VMS_ARRAY) {
>          n_elems = field->num;
> -    } else if (field->flags & VMS_VARRAY) {
> +    } else if (field->num_offset.size) {

Hmm, is it better to infer from the data rather than having the explicit
flag? And if so, can't we remove VMS_VARRAY flag altogether?

From Vladimir's message I though the intent was not only to remove the
flag, but to remove the definition of VMSTATE_VBUFFER entirely and
somehow fit that case into VMSTATE_VARRAY, removing size_offset along
with it.

>          n_elems = vmstate_read_from_offset(opaque,
>      &field->num_offset); }
>  
> @@ -114,17 +114,17 @@ static uint64_t vmstate_size(void *opaque, const VMStateField *field)
>  {
>      uint64_t size;
>  
> -    if (field->flags & VMS_VBUFFER) {
> -        size = vmstate_read_from_offset(opaque, &field->size_offset);
> -        if (field->flags & VMS_MULTIPLY) {
> -            size *= field->size;
> -        }
> -    } else if (field->flags & VMS_ARRAY_OF_POINTER) {
> +    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 if (field->size_offset.size) {
> +        size = vmstate_read_from_offset(opaque, &field->size_offset);
> +        if (field->flags & VMS_MULTIPLY) {
> +            size *= field->size;
> +        }
>      } else {
>          size = field->size;

Since we're talking about flags, I'd like to be able to assert(size) at
this point, but there is some weirness with vmstate_msix and
vmstate_scsi_device that have no state at all. I'm thinking of
introducing a new flag to identify those cases:

-- >8 --
From: Fabiano Rosas <farosas@suse.de>
Subject: [PATCH] migration: Add VMS_NO_STATE flag

There are a few special cases of vmstate usage:

The vmstate_msix and vmstate_scsi_device have fields that contain no
data, only a vmstate_info structure.

The VMSTATE_VALIDATE macro serves only to invoke the .field_exists
routine for validation.

Regardless whether these scenarios are valid, add a separate flag to
identify them so we can enforce common constraints for the normal
vmstates such as having a size greater than zero.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/pci/msix.c               | 6 +-----
 hw/scsi/scsi-bus.c          | 6 +-----
 include/migration/vmstate.h | 9 +++++++--
 migration/savevm.c          | 3 +--
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 1b23eaf1007..adf76b5bccc 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -711,12 +711,8 @@ const VMStateDescription vmstate_msix = {
     .fields = (const VMStateField[]) {
         {
             .name         = "msix",
-            .version_id   = 0,
-            .field_exists = NULL,
-            .size         = 0,   /* ouch */
             .info         = &vmstate_info_msix,
-            .flags        = VMS_SINGLE,
-            .offset       = 0,
+            .flags        = VMS_SINGLE | VMS_NO_STATE,
         },
         VMSTATE_END_OF_LIST()
     }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index deb43d5560e..aa02ff631b7 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1980,12 +1980,8 @@ const VMStateDescription vmstate_scsi_device = {
         VMSTATE_UINT32(sense_len, SCSIDevice),
         {
             .name         = "requests",
-            .version_id   = 0,
-            .field_exists = NULL,
-            .size         = 0,   /* ouch */
             .info         = &vmstate_info_scsi_requests,
-            .flags        = VMS_SINGLE,
-            .offset       = 0,
+            .flags        = VMS_SINGLE | VMS_NO_STATE,
         },
         VMSTATE_END_OF_LIST()
     },
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 09b2e535645..ada1625f5b6 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -108,6 +108,12 @@ enum VMStateFlags {
      */
     VMS_ARRAY_OF_POINTER = 0x040,
 
+    /*
+     * The field contains no data. Used for special cases such as
+     * invoking a custom VMStateInfo.
+     */
+    VMS_NO_STATE = 0x080,
+
     /* The size of the individual entries (a single array entry if
      * VMS_ARRAY or VMS_VARRAY are set, or the field itself if
      * neither is set) is variable (i.e. not known at compile-time),
@@ -451,8 +457,7 @@ extern const VMStateInfo vmstate_info_g_byte_array;
 #define VMSTATE_VALIDATE(_name, _test) { \
     .name         = (_name),                                         \
     .field_exists = (_test),                                         \
-    .flags        = VMS_ARRAY | VMS_MUST_EXIST,                      \
-    .num          = 0, /* 0 elements: no data, only run _test */     \
+    .flags        = VMS_ARRAY | VMS_MUST_EXIST | VMS_NO_STATE,       \
 }
 
 #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
diff --git a/migration/savevm.c b/migration/savevm.c
index 3e5cce6520d..160c0f2d97d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -617,8 +617,7 @@ static void dump_vmstate_vmsd(FILE *out_file,
         fprintf(out_file, ",\n%*s\"Fields\": [\n", indent, "");
         first = true;
         while (field->name != NULL) {
-            if (field->flags & VMS_MUST_EXIST) {
-                /* Ignore VMSTATE_VALIDATE bits; these don't get migrated */
+            if (field->flags & VMS_NO_STATE) {
                 field++;
                 continue;
             }
-- 
2.53.0