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

[Peter Xu <peterx@redhat.com>]

On Tue, Jan 16, 2024 at 05:08:28PM +0100, Philippe Mathieu-Daudé wrote:
> 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:

Any attempt to provide more documents there for the macros would be nice if
anyone would like to post a patch.  Though some comments below for this
specific one.

> 
> -- >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

IIUC it's not an array of uint32_t, but a buffer with dynamic size.  the
"uint32_t" is trying to describe the type of the size field, afaict.  Same
issue for below one.

For example, comparing VMSTATE_VBUFFER_UINT32 vs VMSTATE_VBUFFER, we should
see the only difference is:

    .size_offset  = vmstate_offset_value(_state, _field_size, int32_t),\

vs:

    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\

I think it checks the size field to match that type.

Interestingly, when I look at the code to use the size, it looks like a bug
to me, where we always parse the size field to be int32_t..

static int vmstate_size(void *opaque, const VMStateField *field)
{
    int size = field->size;

    if (field->flags & VMS_VBUFFER) {
        size = *(int32_t *)(opaque + field->size_offset);     <----------- see here..
        if (field->flags & VMS_MULTIPLY) {
            size *= field->size;
        }
    }

    return size;
}

I think it'll start to crash things when bit32 start to be used.  And I had
a feeling that this is overlooked in the commit a19cbfb346 ("spice: add qxl
device") where VMSTATE_VBUFFER_UINT32 is introduced.

I don't have a good way to trivially fix it because we don't remember that
type of size in vmsd.  However a simple solution could be that we convert
all existing VMSTATE_VBUFFER() (potentially, VMSTATE_PARTIAL_VBUFFER users;
there seems to have only 1 caller..) to always use uint32_t.  I don't think
it's wise to try using a signed for size anyway, and it should be
compatible change as we doubled the size.

I'll hold a bit to see whether there's some comment, then I can try to post
a patch.

> + * 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,       \
> ---
> 

-- 
Peter Xu