Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 15, 2024 at 10:31:32AM -0600, Peter Xu wrote:
> On Wed, May 15, 2024 at 12:02:49PM -0400, Michael S. Tsirkin wrote:
> > On Wed, May 15, 2024 at 06:15:56PM +0400, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@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 <peterx@redhat.com>
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > I don't get it. Our standard way to do it is:
> > - add a property (begin name with x- so we don't commit to an API)
> > - set from compat machinery
> > - test property value in VMSTATE macros
> > 
> > Big advantage is, it works well with any downstreams
> > which pick any properties they like.
> > Why is this not a good fit here?
> 
> I think it'll simplify upstream to avoid introducing one new field + one
> new property for each of such protocol change, which fundamentally are the
> same thing.  But it's indeed a good point that such helper can slightly
> complicate the backport a bit.. I assume a global replacement of versions
> over the helper will be needed after downstream settles on how to map
> downstream MCs to upstream's.
> 
> Thanks,

There's nothing special about this specific code. If we want to rework
how machine compat is handled we can do it, but I wouldn't start with
this virtio gpu bug.

It's a big if though, I don't like how this patch works at all.

-- 
MST