QEMU-Devel Archive mirror
 help / color / mirror / Atom feed
From: Fabiano Rosas <farosas@suse.de>
To: marcandre.lureau@redhat.com, qemu-devel@nongnu.org
Cc: "Richard Henderson" <richard.henderson@linaro.org>,
	"Sebastian Ott" <sebott@redhat.com>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Fiona Ebner" <f.ebner@proxmox.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Peter Xu" <peterx@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	peter.maydell@linaro.org, "Michael S. Tsirkin" <mst@redhat.com>,
	"Yanan Wang" <wangyanan55@huawei.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: [PATCH 0/4] Fix "virtio-gpu: fix scanout migration post-load"
Date: Tue, 07 May 2024 17:46:36 -0300	[thread overview]
Message-ID: <87wmo5l58z.fsf@suse.de> (raw)
In-Reply-To: <20240507111920.1594897-1-marcandre.lureau@redhat.com>

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@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.

$ 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.

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.


  parent reply	other threads:[~2024-05-07 20:48 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-07 11:19 [PATCH 0/4] Fix "virtio-gpu: fix scanout migration post-load" marcandre.lureau
2024-05-07 11:19 ` [PATCH 1/4] migration: add "exists" info to load-state-field trace marcandre.lureau
2024-05-07 15:29   ` Peter Xu
2024-05-07 11:19 ` [PATCH 2/4] include/migration: add VMSTATE_VSTRUCT_TEST_VARRAY_UINT32 marcandre.lureau
2024-05-07 11:19 ` [PATCH 3/4] virtio-gpu: use a VMState variant for the scanout field marcandre.lureau
2024-05-07 19:59   ` Peter Xu
2024-05-10  8:39     ` Marc-André Lureau
2024-05-10 17:33       ` Peter Xu
2024-05-11  6:44         ` Marc-André Lureau
2024-05-10 10:25   ` Michael S. Tsirkin
2024-05-10 11:57     ` Marc-André Lureau
2024-05-07 11:19 ` [PATCH 4/4] virtio-gpu: add x-vmstate-version marcandre.lureau
2024-05-07 20:51   ` Fabiano Rosas
2024-05-07 20:46 ` Fabiano Rosas [this message]
2024-05-07 21:24   ` [PATCH 0/4] Fix "virtio-gpu: fix scanout migration post-load" Peter Xu
2024-05-08  9:51 ` Fiona Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87wmo5l58z.fsf@suse.de \
    --to=farosas@suse.de \
    --cc=eduardo@habkost.net \
    --cc=f.ebner@proxmox.com \
    --cc=kraxel@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mst@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=peterx@redhat.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=sebott@redhat.com \
    --cc=wangyanan55@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).