[PATCH v2 0/4] Fix "virtio-gpu: fix scanout migration post-load"

[PATCH v2 0/4] Fix "virtio-gpu: fix scanout migration post-load"

[marcandre.lureau]

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").

v2:
 - use a manual version field test (instead of the more complex struct variant)

Marc-André Lureau (3):
  migration: add "exists" info to load-state-field trace
  migration: fix a typo
  virtio-gpu: add x-vmstate-version

Peter Xu (1):
  virtio-gpu: fix v2 migration

 include/hw/virtio/virtio-gpu.h |  1 +
 hw/core/machine.c              |  1 +
 hw/display/virtio-gpu.c        | 26 ++++++++++++++++++--------
 migration/vmstate.c            |  7 ++++---
 migration/trace-events         |  2 +-
 5 files changed, 25 insertions(+), 12 deletions(-)

-- 
2.41.0.28.gd7d8841f67

[PATCH v2 1/4] migration: add "exists" info to load-state-field trace

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate.c    | 5 +++--
 migration/trace-events | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index ef26f26ccd..b51212a75b 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -128,8 +128,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         }
     }
     while (field->name) {
-        trace_vmstate_load_state_field(vmsd->name, field->name);
-        if (vmstate_field_exists(vmsd, field, opaque, version_id)) {
+        bool exists = vmstate_field_exists(vmsd, field, opaque, version_id);
+        trace_vmstate_load_state_field(vmsd->name, field->name, exists);
+        if (exists) {
             void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
diff --git a/migration/trace-events b/migration/trace-events
index d0c44c3853..0b7c3324fb 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -58,7 +58,7 @@ postcopy_page_req_sync(void *host_addr) "sync page req %p"
 vmstate_load_field_error(const char *field, int ret) "field \"%s\" load failed, ret = %d"
 vmstate_load_state(const char *name, int version_id) "%s v%d"
 vmstate_load_state_end(const char *name, const char *reason, int val) "%s %s/%d"
-vmstate_load_state_field(const char *name, const char *field) "%s:%s"
+vmstate_load_state_field(const char *name, const char *field, bool exists) "%s:%s exists=%d"
 vmstate_n_elems(const char *name, int n_elems) "%s: %d"
 vmstate_subsection_load(const char *parent) "%s"
 vmstate_subsection_load_bad(const char *parent,  const char *sub, const char *sub2) "%s: %s/%s"
-- 
2.41.0.28.gd7d8841f67

[PATCH v2 2/4] migration: fix a typo

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/vmstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index b51212a75b..ff5d589a6d 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -479,7 +479,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
 
         len = qemu_peek_byte(f, 1);
         if (len < strlen(vmsd->name) + 1) {
-            /* subsection name has be be "section_name/a" */
+            /* subsection name has to be "section_name/a" */
             trace_vmstate_subsection_load_bad(vmsd->name, "(short)", "");
             return 0;
         }
-- 
2.41.0.28.gd7d8841f67

[PATCH v2 3/4] virtio-gpu: add x-vmstate-version

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Machine <= 8.2 use v1.

Following patch will adjust to v2 for other machines to fix migration.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 include/hw/virtio/virtio-gpu.h | 1 +
 hw/core/machine.c              | 1 +
 hw/display/virtio-gpu.c        | 6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index ed44cdad6b..af1c77eb3f 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -177,6 +177,7 @@ typedef struct VGPUDMABuf {
 struct VirtIOGPU {
     VirtIOGPUBase parent_obj;
 
+    uint8_t vmstate_version;
     uint64_t conf_max_hostmem;
 
     VirtQueue *ctrl_vq;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c7ceb11501..cf840e0502 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -42,6 +42,7 @@ GlobalProperty hw_compat_8_2[] = {
     { "migration", "zero-page-detection", "legacy"},
     { TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
     { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
+    { "virtio-gpu-device", "x-vmstate-version", "1" },
 };
 const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
 
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index ae831b6b3e..7f9fb5eacc 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1234,7 +1234,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
     }
     qemu_put_be32(f, 0); /* end of list */
 
-    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
+    return vmstate_save_state_v(f, &vmstate_virtio_gpu_scanouts, g,
+                                NULL, g->vmstate_version, NULL);
 }
 
 static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g,
@@ -1339,7 +1340,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
     }
 
     /* load & apply scanout state */
-    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
+    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, g->vmstate_version);
 
     return 0;
 }
@@ -1659,6 +1660,7 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+    DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0.28.gd7d8841f67

[PATCH v2 4/4] virtio-gpu: fix v2 migration

[marcandre.lureau]

From: Peter Xu <peterx@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 x-vmstate-version introduced before and a
field test to save/load appropriately according to the machine version.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/display/virtio-gpu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 7f9fb5eacc..5de90bb62f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1166,6 +1166,14 @@ static void virtio_gpu_cursor_bh(void *opaque)
     virtio_gpu_handle_cursor(&g->parent_obj.parent_obj, g->cursor_vq);
 }
 
+static bool vmstate_after_v2(void *opaque, int version)
+{
+    struct VirtIOGPUBase *base = container_of(opaque, VirtIOGPUBase, scanout);
+    struct VirtIOGPU *gpu = container_of(base, VirtIOGPU, parent_obj);
+
+    return gpu->vmstate_version >= 2;
+}
+
 static const VMStateDescription vmstate_virtio_gpu_scanout = {
     .name = "virtio-gpu-one-scanout",
     .version_id = 2,
@@ -1181,12 +1189,12 @@ static const VMStateDescription vmstate_virtio_gpu_scanout = {
         VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout),
         VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout),
         VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout),
-        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),
+        VMSTATE_UINT32_TEST(fb.format, struct virtio_gpu_scanout, vmstate_after_v2),
+        VMSTATE_UINT32_TEST(fb.bytes_pp, struct virtio_gpu_scanout, vmstate_after_v2),
+        VMSTATE_UINT32_TEST(fb.width, struct virtio_gpu_scanout, vmstate_after_v2),
+        VMSTATE_UINT32_TEST(fb.height, struct virtio_gpu_scanout, vmstate_after_v2),
+        VMSTATE_UINT32_TEST(fb.stride, struct virtio_gpu_scanout, vmstate_after_v2),
+        VMSTATE_UINT32_TEST(fb.offset, struct virtio_gpu_scanout, vmstate_after_v2),
         VMSTATE_END_OF_LIST()
     },
 };
@@ -1660,7 +1668,7 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
-    DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 1),
+    DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 2),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0.28.gd7d8841f67

Re: [PATCH v2 0/4] Fix "virtio-gpu: fix scanout migration post-load"

[Fiona Ebner]

Hi,

Am 13.05.24 um 09:19 schrieb marcandre.lureau@redhat.com:
> 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").
> 
> v2:
>  - use a manual version field test (instead of the more complex struct variant)
> 

Unfortunately, when creating a snapshot with machine type pc-i440fx-9.0
and trying to load it afterwards (both times with patches on top of
current master), it'll fail with:

> qemu-system-x86_64: virtio-gpu-scanouts: incoming version_id 2 is too new for local version_id 1
> qemu-system-x86_64: Missing section footer for 0000:00:02.0/virtio-gpu
> qemu-system-x86_64: Error -22 while loading VM state

Is there a bump to virtio-gpu-scanouts' version_id missing?

Best Regards,
Fiona

Re: [PATCH v2 0/4] Fix "virtio-gpu: fix scanout migration post-load"

[Marc-André Lureau]

Hi Fiona

On Mon, May 13, 2024 at 4:56 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Hi,
>
> Am 13.05.24 um 09:19 schrieb marcandre.lureau@redhat.com:
> > 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").
> >
> > v2:
> >  - use a manual version field test (instead of the more complex struct variant)
> >
>
> Unfortunately, when creating a snapshot with machine type pc-i440fx-9.0
> and trying to load it afterwards (both times with patches on top of
> current master), it'll fail with:
>
> > qemu-system-x86_64: virtio-gpu-scanouts: incoming version_id 2 is too new for local version_id 1
> > qemu-system-x86_64: Missing section footer for 0000:00:02.0/virtio-gpu
> > qemu-system-x86_64: Error -22 while loading VM state
>
> Is there a bump to virtio-gpu-scanouts' version_id missing?
>

Indeed, it needs:

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5de90bb62f..3a88eb5e3a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1201,7 +1201,7 @@ static const VMStateDescription
vmstate_virtio_gpu_scanout = {

 static const VMStateDescription vmstate_virtio_gpu_scanouts = {
     .name = "virtio-gpu-scanouts",
-    .version_id = 1,
+    .version_id = 2,

Re: [PATCH v2 0/4] Fix "virtio-gpu: fix scanout migration post-load"

[Fiona Ebner]

Am 13.05.24 um 15:21 schrieb Marc-André Lureau:
> 
> Indeed, it needs:
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5de90bb62f..3a88eb5e3a 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1201,7 +1201,7 @@ static const VMStateDescription
> vmstate_virtio_gpu_scanout = {
> 
>  static const VMStateDescription vmstate_virtio_gpu_scanouts = {
>      .name = "virtio-gpu-scanouts",
> -    .version_id = 1,
> +    .version_id = 2,
> 
> 

Thanks! With that on top:

Tested-by: Fiona Ebner <f.ebner@proxmox.com>

Tested with an Ubuntu 23.10 VM:

Machine type pc-i440fx-8.2:
1. create snapshot with 8.2, load with patched 9.0
2. create snapshot with patched 9.0, load with patched 9.0 and with 8.2

Machine type pc-i440fx-9.0:
1. create snapshot with patched 9.0, load with patched 9.0

No crashes/failures and didn't notice any other issues 🙂

Best Regards,
Fiona

Re: [PATCH v2 2/4] migration: fix a typo

[Fabiano Rosas]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  migration/vmstate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b51212a75b..ff5d589a6d 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -479,7 +479,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>  
>          len = qemu_peek_byte(f, 1);
>          if (len < strlen(vmsd->name) + 1) {
> -            /* subsection name has be be "section_name/a" */
> +            /* subsection name has to be "section_name/a" */
>              trace_vmstate_subsection_load_bad(vmsd->name, "(short)", "");
>              return 0;
>          }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 3/4] virtio-gpu: add x-vmstate-version

[Peter Xu]

Hey, Marc-Andre,

On Mon, May 13, 2024 at 11:19:04AM +0400, marcandre.lureau@redhat.com wrote:
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index ae831b6b3e..7f9fb5eacc 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1234,7 +1234,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
>      }
>      qemu_put_be32(f, 0); /* end of list */
>  
> -    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> +    return vmstate_save_state_v(f, &vmstate_virtio_gpu_scanouts, g,
> +                                NULL, g->vmstate_version, NULL);
>  }
>  
>  static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g,
> @@ -1339,7 +1340,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
>      }
>  
>      /* load & apply scanout state */
> -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
> +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, g->vmstate_version);

[sorry for a late response; attending a conf, and will reply to the v1
 thread later for the other discussions..]

These two changes shouldn't be needed if we go with the .field_exists()
approach, am I right?  IIUC in that case we can keep the version 1 here and
don't boost anything, because we relied on the machine versions.

IIUC this might be the reason why we found 9.0 mahines are broken on
migration.  E.g, IIUC my original patch should work for 9.0<->9.0 too.

Thanks,

>  
>      return 0;
>  }
> @@ -1659,6 +1660,7 @@ static Property virtio_gpu_properties[] = {
>      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> +    DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 2.41.0.28.gd7d8841f67
> 

-- 
Peter Xu

Re: [PATCH v2 2/4] migration: fix a typo

[Peter Xu]

On Mon, May 13, 2024 at 11:19:03AM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 3/4] virtio-gpu: add x-vmstate-version

[Marc-André Lureau]

Hi

On Tue, May 14, 2024 at 8:35 AM Peter Xu <peterx@redhat.com> wrote:
>
> Hey, Marc-Andre,
>
> On Mon, May 13, 2024 at 11:19:04AM +0400, marcandre.lureau@redhat.com wrote:
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index ae831b6b3e..7f9fb5eacc 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1234,7 +1234,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
> >      }
> >      qemu_put_be32(f, 0); /* end of list */
> >
> > -    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> > +    return vmstate_save_state_v(f, &vmstate_virtio_gpu_scanouts, g,
> > +                                NULL, g->vmstate_version, NULL);
> >  }
> >
> >  static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g,
> > @@ -1339,7 +1340,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >      }
> >
> >      /* load & apply scanout state */
> > -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
> > +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, g->vmstate_version);
>
> [sorry for a late response; attending a conf, and will reply to the v1
>  thread later for the other discussions..]
>
> These two changes shouldn't be needed if we go with the .field_exists()
> approach, am I right?  IIUC in that case we can keep the version 1 here and
> don't boost anything, because we relied on the machine versions.
>
> IIUC this might be the reason why we found 9.0 mahines are broken on
> migration.  E.g, IIUC my original patch should work for 9.0<->9.0 too.
>

Indeed, but for consistency, shouldn't it use the x-vmstate-version
value for the top-level VMSD save/load ?

Otherwise, it feels a bit odd that this x-vmstate-version is only used
for the nested "virtio-gpu-one-scanout" version.

Or perhaps we should rename it to x-scanout-vmstate-version ? wdyt


> Thanks,
>
> >
> >      return 0;
> >  }
> > @@ -1659,6 +1660,7 @@ static Property virtio_gpu_properties[] = {
> >      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> >                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> >      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> > +    DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 1),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > --
> > 2.41.0.28.gd7d8841f67
> >
>
> --
> Peter Xu
>

Re: [PATCH v2 3/4] virtio-gpu: add x-vmstate-version

[Peter Xu]

On Tue, May 14, 2024 at 11:25:26AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, May 14, 2024 at 8:35 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hey, Marc-Andre,
> >
> > On Mon, May 13, 2024 at 11:19:04AM +0400, marcandre.lureau@redhat.com wrote:
> > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > > index ae831b6b3e..7f9fb5eacc 100644
> > > --- a/hw/display/virtio-gpu.c
> > > +++ b/hw/display/virtio-gpu.c
> > > @@ -1234,7 +1234,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
> > >      }
> > >      qemu_put_be32(f, 0); /* end of list */
> > >
> > > -    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> > > +    return vmstate_save_state_v(f, &vmstate_virtio_gpu_scanouts, g,
> > > +                                NULL, g->vmstate_version, NULL);
> > >  }
> > >
> > >  static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g,
> > > @@ -1339,7 +1340,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> > >      }
> > >
> > >      /* load & apply scanout state */
> > > -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
> > > +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, g->vmstate_version);
> >
> > [sorry for a late response; attending a conf, and will reply to the v1
> >  thread later for the other discussions..]
> >
> > These two changes shouldn't be needed if we go with the .field_exists()
> > approach, am I right?  IIUC in that case we can keep the version 1 here and
> > don't boost anything, because we relied on the machine versions.
> >
> > IIUC this might be the reason why we found 9.0 mahines are broken on
> > migration.  E.g, IIUC my original patch should work for 9.0<->9.0 too.
> >
> 
> Indeed, but for consistency, shouldn't it use the x-vmstate-version
> value for the top-level VMSD save/load ?
> 
> Otherwise, it feels a bit odd that this x-vmstate-version is only used
> for the nested "virtio-gpu-one-scanout" version.
> 
> Or perhaps we should rename it to x-scanout-vmstate-version ? wdyt

Makes sense to me.  In another place I used to have a field called
preempt_pre_7_2.. which is pretty wierd but it would be more accurate I
suppose if the field name reflects how that was defined, especially
differenciate that v.s. VMSD versioning as they're confusing indeed when
used together.

So if a rename I suppose we can even "vmstate-version".  I just wished VMSD
versioning could work with bi-directional migration already then we save
all the fuss... we used to have the chance before introducing
field_exists() (I suppose this one came later), but we didn't care or
notice at that time, sign.  We'll just need a handshake between src/dst so
that when src sees dst uses VMSD v1 it sends v1-only fields even if it
knows as far as v2.

For the long run we may be able to have some small helper so we fetch the
machine type globally, then maybe we can in the future pass in the test
function as:

bool test_function (void *opaque)
{
  return MACHINE_TYPE_BEFORE(8, 2);
}

Then it'll also avoid even introducing a variable.  Maybe we can provide
this test_function() directly too, one for each release.

Thanks,

> 
> 
> > Thanks,
> >
> > >
> > >      return 0;
> > >  }
> > > @@ -1659,6 +1660,7 @@ static Property virtio_gpu_properties[] = {
> > >      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> > >                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> > >      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> > > +    DEFINE_PROP_UINT8("x-vmstate-version", VirtIOGPU, vmstate_version, 1),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >
> > > --
> > > 2.41.0.28.gd7d8841f67
> > >
> >
> > --
> > Peter Xu
> >
> 

-- 
Peter Xu