From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org,
"Richard Henderson" <richard.henderson@linaro.org>,
"Sebastian Ott" <sebott@redhat.com>,
"Fabiano Rosas" <farosas@suse.de>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Fiona Ebner" <f.ebner@proxmox.com>,
"Gerd Hoffmann" <kraxel@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>
Subject: Re: [PATCH 3/4] virtio-gpu: use a VMState variant for the scanout field
Date: Fri, 10 May 2024 12:39:34 +0400 [thread overview]
Message-ID: <CAJ+F1CJjNnHoX=LvSsh5M_fiZg-n5K=KEgPRh+2gAjRij4Oq-w@mail.gmail.com> (raw)
In-Reply-To: <ZjqILU7qxlTGN4OD@x1n>
Hi
On Wed, May 8, 2024 at 12:01 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, May 07, 2024 at 03:19:19PM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Depending on the version, use v1 or v2 of the scanout VM state.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@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?
The existing VMSTATE_STRUCT_VARRAY_UINT32 would always use the latest
vmstate_virtio_gpu_scanout version_id (2 in master).
The "struct_version_id" solution allows setting the
vmstate_virtio_gpu_scanout version_id to 1 if the parent struct
(vmstate_virtio_gpu_scanouts) is 1, and 2 if the parent is 2.
Since we don't have per VMSD version information on the wire, nested
struct versioning is quite limited and cumbersome. I am not sure it
can be changed without breaking the stream format, and whether it's
worthwhile.
>
> 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
>
>
--
Marc-André Lureau
next prev parent reply other threads:[~2024-05-10 8:40 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 [this message]
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 ` [PATCH 0/4] Fix "virtio-gpu: fix scanout migration post-load" Fabiano Rosas
2024-05-07 21:24 ` 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='CAJ+F1CJjNnHoX=LvSsh5M_fiZg-n5K=KEgPRh+2gAjRij4Oq-w@mail.gmail.com' \
--to=marcandre.lureau@gmail.com \
--cc=eduardo@habkost.net \
--cc=f.ebner@proxmox.com \
--cc=farosas@suse.de \
--cc=kraxel@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).