Hi
On Wed, May 8, 2024 at 12:01 AM Peter Xu <pet...@redhat.com> wrote:
>
> On Tue, May 07, 2024 at 03:19:19PM +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Depending on the version, use v1 or v2 of the scanout VM state.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > hw/display/virtio-gpu.c | 22 +++++++++++++++++-----
> > 1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index ae831b6b3e..4fd72caf3f 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1191,17 +1191,29 @@ static const VMStateDescription
> > vmstate_virtio_gpu_scanout = {
> > },
> > };
> >
> > +static bool vmstate_before_v2(void *opaque, int version)
> > +{
> > + return version <= 1;
> > +}
> > +
> > static const VMStateDescription vmstate_virtio_gpu_scanouts = {
> > .name = "virtio-gpu-scanouts",
> > - .version_id = 1,
> > + .version_id = 2,
> > + .minimum_version_id = 1,
> > .fields = (const VMStateField[]) {
> > VMSTATE_INT32(parent_obj.enable, struct VirtIOGPU),
> > VMSTATE_UINT32_EQUAL(parent_obj.conf.max_outputs,
> > struct VirtIOGPU, NULL),
> > - VMSTATE_STRUCT_VARRAY_UINT32(parent_obj.scanout, struct VirtIOGPU,
> > - parent_obj.conf.max_outputs, 1,
> > - vmstate_virtio_gpu_scanout,
> > - struct virtio_gpu_scanout),
> > + VMSTATE_VSTRUCT_TEST_VARRAY_UINT32(parent_obj.scanout, struct
> > VirtIOGPU,
> > + vmstate_before_v2,
> > + parent_obj.conf.max_outputs, 1,
> > + vmstate_virtio_gpu_scanout,
> > + struct virtio_gpu_scanout, 1),
> > + VMSTATE_VSTRUCT_TEST_VARRAY_UINT32(parent_obj.scanout, struct
> > VirtIOGPU,
> > + NULL,
> > + parent_obj.conf.max_outputs, 2,
> > + vmstate_virtio_gpu_scanout,
> > + struct virtio_gpu_scanout, 2),
>
> Personally I really wished struct_version_id never existed.. After these
> years we only have 1 user of it (hw/ipmi/isa_ipmi_kcs.c), and we need to
> keep that working. I'm wondering whether there's way we can avoid adding
> one more user, and even more complicated..
>
> I think I get the reasoning behind "we define the same thing twice", but
> this is really tricky and definitely needs rich documents, meanwhiel all of
> these seem to rely on so many small details: one set .field_exists
> properly, one leaving it to NULL; also the two versionings used here for
> both parent vmsd, and the struct, and for each entry we need to set
> different versions to different fields..
>
> Would it work if we only make the new fields under control with
> vmstate_before_v2()? IOW, making all below with
> .field_exists=vmstate_before_v2, so skip below when machine type is old?
The existing VMSTATE_STRUCT_VARRAY_UINT32 would always use the latest
vmstate_virtio_gpu_scanout version_id (2 in master).
The "struct_version_id" solution allows setting the
vmstate_virtio_gpu_scanout version_id to 1 if the parent struct
(vmstate_virtio_gpu_scanouts) is 1, and 2 if the parent is 2.
Since we don't have per VMSD version information on the wire, nested
struct versioning is quite limited and cumbersome. I am not sure it
can be changed without breaking the stream format, and whether it's
worthwhile.
>
> VMSTATE_UINT32_V(fb.format, struct virtio_gpu_scanout, 2),
> VMSTATE_UINT32_V(fb.bytes_pp, struct virtio_gpu_scanout, 2),
> VMSTATE_UINT32_V(fb.width, struct virtio_gpu_scanout, 2),
> VMSTATE_UINT32_V(fb.height, struct virtio_gpu_scanout, 2),
> VMSTATE_UINT32_V(fb.stride, struct virtio_gpu_scanout, 2),
> VMSTATE_UINT32_V(fb.offset, struct virtio_gpu_scanout, 2),
>
> Thanks,
>
> --
> Peter Xu
>
>
--
Marc-André Lureau
