On Wed, May 15, 2024 at 06:15:56PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > Commit dfcf74fa ("virtio-gpu: fix scanout migration post-load") broke > forward/backward version migration. Versioning of nested VMSD structures > is not straightforward, as the wire format doesn't have nested > structures versions. > > Use the previously introduced check_machine_version() function as a > field test to ensure proper saving/loading based on the machine version. > The VMSD.version is irrelevant now. > > Fixes: dfcf74fa ("virtio-gpu: fix scanout migration post-load") > Suggested-by: Peter Xu <pet...@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- > hw/display/virtio-gpu.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index ae831b6b3e..b2d8e5faeb 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -20,6 +20,7 @@ > #include "trace.h" > #include "sysemu/dma.h" > #include "sysemu/sysemu.h" > +#include "hw/boards.h" > #include "hw/virtio/virtio.h" > #include "migration/qemu-file-types.h" > #include "hw/virtio/virtio-gpu.h" > @@ -1166,10 +1167,14 @@ static void virtio_gpu_cursor_bh(void *opaque) > virtio_gpu_handle_cursor(&g->parent_obj.parent_obj, g->cursor_vq); > } > > +static bool machine_check_9_0(void *opaque, int version) > +{ > + return machine_check_version(9, 0); > +}
I think applying version number checks to decide machine type compatibility is a highly undesirable direction for QEMU to take. Machine type compatibility is a difficult problem, but one of the good aspects about our current solution is that it is clear what the differences are for each version. We can see all the compatibility properties/flags/values being set in one place, in the declaration of each machine's class. Sprinkling version number checks around the codebase in arbitrary files will harm visibility of what ABI is expressd by each machine, and thus is liable to increase the liklihood of mistakes. This will negatively impact downstream vendors cherry-picking patches to their stable branches, as the version number logic may have incorrect semantics. It will also create trouble for downstream vendors who define their own machines with distinct versioning from upstream, as there will be confusion over whether a version check is for the base QEMU version, or the downstream version, and such code added to the tree is less visible than the machine type definitions. Above all, I'm failing to see why there's a compelling reason for virtio_gpu to diverge from our long standing practice of adding a named property flag "virtio_scanout_vmstate_fix" on the machine class, and then setting it in machine types which need it. > + > static const VMStateDescription vmstate_virtio_gpu_scanout = { > .name = "virtio-gpu-one-scanout", > - .version_id = 2, > - .minimum_version_id = 1, > + .version_id = 1, > .fields = (const VMStateField[]) { > VMSTATE_UINT32(resource_id, struct virtio_gpu_scanout), > VMSTATE_UINT32(width, struct virtio_gpu_scanout), > @@ -1181,12 +1186,12 @@ static const VMStateDescription > vmstate_virtio_gpu_scanout = { > VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout), > VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout), > VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout), > - 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), > + VMSTATE_UINT32_TEST(fb.format, struct virtio_gpu_scanout, > machine_check_9_0), > + VMSTATE_UINT32_TEST(fb.bytes_pp, struct virtio_gpu_scanout, > machine_check_9_0), > + VMSTATE_UINT32_TEST(fb.width, struct virtio_gpu_scanout, > machine_check_9_0), > + VMSTATE_UINT32_TEST(fb.height, struct virtio_gpu_scanout, > machine_check_9_0), > + VMSTATE_UINT32_TEST(fb.stride, struct virtio_gpu_scanout, > machine_check_9_0), > + VMSTATE_UINT32_TEST(fb.offset, struct virtio_gpu_scanout, > machine_check_9_0), > VMSTATE_END_OF_LIST() > }, > }; > -- > 2.41.0.28.gd7d8841f67 > > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|