Re: [PATCH v2 2/2] hw/pflash: implement update buffer for block writes

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 12/1/24 17:54, Peter Maydell wrote:
> On Mon, 8 Jan 2024 at 13:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Gerd,
>>
>> On 8/1/24 13:53, Philippe Mathieu-Daudé wrote:
>>> From: Gerd Hoffmann <kraxel@redhat.com>
>>>
>>> Add an update buffer where all block updates are staged.
>>> Flush or discard updates properly, so we should never see
>>> half-completed block writes in pflash storage.
>>>
>>> Drop a bunch of FIXME comments ;)
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> Message-ID: <20240105135855.268064-3-kraxel@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/block/pflash_cfi01.c | 106 ++++++++++++++++++++++++++++++----------
>>>    1 file changed, 80 insertions(+), 26 deletions(-)


>>> +static const VMStateDescription vmstate_pflash_blk_write = {
>>> +    .name = "pflash_cfi01_blk_write",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = pflash_blk_write_state_needed,
>>> +    .fields = (const VMStateField[]) {
>>> +        VMSTATE_VBUFFER_UINT32(blk_bytes, PFlashCFI01, 0, NULL, writeblock_size),
>>
>> I don't get the difference with VMSTATE_VBUFFER_ALLOC_UINT32() which
>> sets VMS_ALLOC. In this case pflash_cfi01_realize() does the alloc so
>> we don't need VMS_ALLOC?
> 
> Yes, that's the idea. A VMS_ALLOC vmstate type means "this
> block of memory is dynamically sized at runtime, so when the
> migration code is doing inbound migration it needs to
> allocate a buffer of the right size first (based on some
> state struct field we've already migrated) and then put the
> incoming data into it". VMS_VBUFFER means "the size of the buffer
> isn't a compile-time constant, so we need to fish it out of
> some other state struct field". So:
> 
>   VMSTATE_VBUFFER_UINT32: we need to migrate (a pointer to) an array
>   of uint32_t; the size of that is in some other struct field,
>   but it's a runtime constant and we can assume the memory has
>   already been allocated
> 
>   VMSTATE_VBUFFER_ALLOC_UINT32: we need to migrate an array
>   of uint32_t of variable size dependent on the inbound migration
>   data, and so the migration code must allocate it

Thanks Peter!

Do you mind if we commit your explanation as is? As:

-- >8 --
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 294d2d8486..5c6f6c5c32 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -573,4 +573,6 @@ extern const VMStateInfo vmstate_info_qlist;

-/* a variable length array (i.e. _type *_field) but we know the
- * length
+/**
+ * VMSTATE_STRUCT_VARRAY_POINTER_KNOWN:
+ *
+ * A variable length array (i.e. _type *_field) but we know the length.
   */
@@ -678,2 +680,10 @@ extern const VMStateInfo vmstate_info_qlist;

+/**
+ * VMSTATE_VBUFFER_UINT32:
+ *
+ * We need to migrate (a pointer to) an array of uint32_t; the size of
+ * that is in some other struct field, but it's a runtime constant and
+ * we can assume the memory has already been allocated.
+*/
+
  #define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, 
_field_size) { \
@@ -688,2 +698,9 @@ extern const VMStateInfo vmstate_info_qlist;

+/**
+ * VMSTATE_VBUFFER_ALLOC_UINT32:
+ *
+ * We need to migrate an array of uint32_t of variable size dependent
+ * on the inbound migration data, and so the migration code must
+ * allocate it.
+*/
  #define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version,       \
---