On Tue, May 07, 2024 at 05:46:36PM -0300, Fabiano Rosas wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Hi,
> >
> > The aforementioned patch breaks virtio-gpu device migrations for versions
> > pre-9.0/9.0, both forwards and backwards. Versioning of `VMS_STRUCT` is more
> > complex than it may initially appear, as evidenced in the problematic commit
> > dfcf74fa68c ("virtio-gpu: fix scanout migration post-load").
> >
> > To resolve this, we need to propagate the `vmstate` `version_id` through the
> > nested structures. Additionally, we should tie specific machine version to a
> > corresponding `version_id` to maintain migration compatibility.
> >
> > `VMS_VSTRUCT` allows specifying the appropriate version of the nested
> > structure
> > to use.
>
> This would have been caught by the migration-compat-x86_64 CI job had we
> added the virtio-gpu device to it.
I had exactly the same thoughts, and actually I added a todo after I
noticed this issue but I forgot to discuss with you today.. actually I have
one more on whether we can allow vmsd versioning to work for bi-direction
(then we can get rid of machine type dependencies), we may need the
handshake work as pre-requisite, so that two qemus need to talk first on
verify the vmsds. But let's leave that for later.
>
> $ cd build-8.2
> $ QTEST_TRACE='vmstate_*' QTEST_DEVICE_OPTS='-device virtio-gpu' \
> QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> QTEST_QEMU_BINARY_DST=../build-9.0/qemu-system-x86_64
> ./tests/qtest/migration-test
> ...
> vmstate_n_elems fb.offset: 1
> vmstate_subsection_load virtio-gpu-one-scanout
> vmstate_subsection_load_good virtio-gpu-one-scanout
> vmstate_load_state_end virtio-gpu-one-scanout end/0
> vmstate_subsection_load virtio-gpu-scanouts
> vmstate_subsection_load_good virtio-gpu-scanouts
> vmstate_load_state_end virtio-gpu-scanouts end/0
> vmstate_subsection_load virtio-gpu
> vmstate_subsection_load_good virtio-gpu
> vmstate_load_state_end virtio-gpu end/0
> vmstate_downtime_load type=non-iterable idstr=0000:00:03.0/virtio-gpu
> instance_id=0 downtime=32118
> qemu-system-x86_64: Missing section footer for 0000:00:03.0/virtio-gpu
> vmstate_downtime_checkpoint dst-precopy-loadvm-completed
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> Some considerations:
>
> 1) Here QTEST_DEVICE_OPTS is a hack I added on top, it doesn't currently
> exist.
>
> 2) This only uncovers relatively simple bugs where we don't need the
> guest to access the device, it just needs to be there.
Right, but having something to cover the basics would still be nice,
because the rarer the bug the less impact to users too (I hope!), so they
can be with lower priority too when tested.
>
> We could take the steps to enable this kind of testing if we think it's
> worthwhile. Some downsides are:
>
> a) the item (2) above - situations that depend on guest behavior are out
> of the picture because migration-test runs only a custom program that
> dirties memory;
>
> b) this test only works in CI or in a pre setup environment because it
> needs the previous QEMU version to be built beforehand;
>
> c) the full set of migration tests already runs a few times in CI via
> make check, plus the compat job. We'll probably need to do some
> simplification to avoid taking too much additional time;
>
> d) there's also the obvious maintenance burden of choosing devices and
> doing the eventual upkeep of the QEMU command line for the
> migration-test.
The last one should be mostly fine, iiuc - we shouldn't add any device that
can easily break ABI, and the list of devices can start with a minumum; an
extreme case is we add one device only if something broke first with a
stable ABI.
Then if the ABI is stable, we need to go through the deprecation procedure,
and that takes two releases. We can drop the device in migration-test in
the release of when deprecation starts.
Thanks,
--
Peter Xu
