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