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é <phi...@linaro.org>
> > wrote:
> > >
> > > Hi Gerd,
> > >
> > > On 8/1/24 13:53, Philippe Mathieu-Daudé wrote:
> > > > From: Gerd Hoffmann <kra...@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 <kra...@redhat.com>
> > > > Message-ID: <20240105135855.268064-3-kra...@redhat.com>
> > > > Signed-off-by: Philippe Mathieu-Daudé <phi...@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
