qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: marcandre.lureau@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: Tue, 7 May 2024 15:59:41 -0400	[thread overview]
Message-ID: <ZjqILU7qxlTGN4OD@x1n> (raw)
In-Reply-To: <20240507111920.1594897-4-marcandre.lureau@redhat.com>

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?

         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



  reply	other threads:[~2024-05-07 20:01 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 [this message]
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 ` [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=ZjqILU7qxlTGN4OD@x1n \
    --to=peterx@redhat.com \
    --cc=eduardo@habkost.net \
    --cc=f.ebner@proxmox.com \
    --cc=farosas@suse.de \
    --cc=kraxel@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mst@redhat.com \
    --cc=peter.maydell@linaro.org \
    --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 NNTP newsgroup(s).