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

Fabiano Rosas <[email protected]> writes:

> Peter Xu <[email protected]> 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 size can legitimately be 0 I guess. Nevermind.