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

[PATCH v3 0/5] 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)

v3:
 - introduce machine_check_version()
 - drop the VMSD version, and use machine version field test

Marc-André Lureau (5):
  migration: add "exists" info to load-state-field trace
  migration: fix a typo
  hw/boards: add machine_check_version()
  Set major/minor for PC and arm machines
  virtio-gpu: fix v2 migration

 include/hw/boards.h     | 14 ++++++++
 include/hw/i386/pc.h    |  4 ++-
 hw/arm/virt.c           |  2 ++
 hw/display/virtio-gpu.c | 21 +++++++-----
 hw/i386/pc_piix.c       | 74 ++++++++++++++++++++---------------------
 hw/i386/pc_q35.c        | 62 +++++++++++++++++-----------------
 migration/vmstate.c     |  7 ++--
 migration/trace-events  |  2 +-
 8 files changed, 105 insertions(+), 81 deletions(-)

-- 
2.41.0.28.gd7d8841f67

[PATCH v3 1/5] 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 v3 2/5] migration: fix a typo

[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>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 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 v3 3/5] hw/boards: add machine_check_version()

[marcandre.lureau]

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

Add optional major/minor version fields to the MachineClass, and a
helper to check if the current machine version is >= (major, minor).

This function can be used to check for extra migration fields, instead
of relying on structure version which are typically associated to a
machine version via compatibility properties.

Notes:
- undefined version is considered latest
- a few machines have a micro version, I don't think it's necessary to
  have here at this point
- there is already a less formal char *hw_version, but it is only used
  for legacy code (< 2.5)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/boards.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2fa800f11a..b36a363fb7 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -248,6 +248,8 @@ struct MachineClass {
     const char *alias;
     const char *desc;
     const char *deprecation_reason;
+    unsigned int major;
+    unsigned int minor;
 
     void (*init)(MachineState *state);
     void (*reset)(MachineState *state, ShutdownCause reason);
@@ -306,6 +308,18 @@ struct MachineClass {
     ram_addr_t (*fixup_ram_size)(ram_addr_t size);
 };
 
+static inline bool machine_check_version(int major, int minor)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+    /* unversioned is latest */
+    if (mc->major == 0 && mc->minor == 0) {
+        return true;
+    }
+
+    return mc->major > major || (mc->major == major && mc->minor >= minor);
+}
+
 /**
  * DeviceMemoryState:
  * @base: address in guest physical address space where the memory
-- 
2.41.0.28.gd7d8841f67

[PATCH v3 4/5] Set major/minor for PC and arm machines

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/i386/pc.h |  4 ++-
 hw/arm/virt.c        |  2 ++
 hw/i386/pc_piix.c    | 74 ++++++++++++++++++++++----------------------
 hw/i386/pc_q35.c     | 62 ++++++++++++++++++-------------------
 4 files changed, 73 insertions(+), 69 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e52290916c..fa91bb7603 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -292,12 +292,14 @@ extern const size_t pc_compat_2_1_len;
 extern GlobalProperty pc_compat_2_0[];
 extern const size_t pc_compat_2_0_len;
 
-#define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
+#define DEFINE_PC_MACHINE(maj, min, suffix, namestr, initfn, optsfn) \
     static void pc_machine_##suffix##_class_init(ObjectClass *oc, void *data) \
     { \
         MachineClass *mc = MACHINE_CLASS(oc); \
         optsfn(mc); \
         mc->init = initfn; \
+        mc->major = maj; \
+        mc->minor = min; \
     } \
     static const TypeInfo pc_machine_type_##suffix = { \
         .name       = namestr TYPE_MACHINE_SUFFIX, \
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3c93c0c0a6..7e3a03b39a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -109,6 +109,8 @@ static void arm_virt_compat_set(MachineClass *mc)
         arm_virt_compat_set(mc); \
         virt_machine_##major##_##minor##_options(mc); \
         mc->desc = "QEMU " # major "." # minor " ARM Virtual Machine"; \
+        mc->ma##jor = major; \
+        mc->mi##nor = minor; \
         if (latest) { \
             mc->alias = "virt"; \
         } \
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 99efb3c45c..bb6767d8d0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -477,7 +477,7 @@ static void pc_xen_hvm_init(MachineState *machine)
 }
 #endif
 
-#define DEFINE_I440FX_MACHINE(suffix, name, compatfn, optionfn) \
+#define DEFINE_I440FX_MACHINE(major, minor, suffix, name, compatfn, optionfn) \
     static void pc_init_##suffix(MachineState *machine) \
     { \
         void (*compat)(MachineState *m) = (compatfn); \
@@ -486,7 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
         } \
         pc_init1(machine, TYPE_I440FX_PCI_DEVICE); \
     } \
-    DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn)
+    DEFINE_PC_MACHINE(major, minor, suffix, name, pc_init_##suffix, optionfn)
 
 static void pc_i440fx_machine_options(MachineClass *m)
 {
@@ -521,7 +521,7 @@ static void pc_i440fx_9_1_machine_options(MachineClass *m)
     m->is_default = true;
 }
 
-DEFINE_I440FX_MACHINE(v9_1, "pc-i440fx-9.1", NULL,
+DEFINE_I440FX_MACHINE(9, 1, v9_1, "pc-i440fx-9.1", NULL,
                       pc_i440fx_9_1_machine_options);
 
 static void pc_i440fx_9_0_machine_options(MachineClass *m)
@@ -534,7 +534,7 @@ static void pc_i440fx_9_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
 }
 
-DEFINE_I440FX_MACHINE(v9_0, "pc-i440fx-9.0", NULL,
+DEFINE_I440FX_MACHINE(9, 0, v9_0, "pc-i440fx-9.0", NULL,
                       pc_i440fx_9_0_machine_options);
 
 static void pc_i440fx_8_2_machine_options(MachineClass *m)
@@ -549,7 +549,7 @@ static void pc_i440fx_8_2_machine_options(MachineClass *m)
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
 }
 
-DEFINE_I440FX_MACHINE(v8_2, "pc-i440fx-8.2", NULL,
+DEFINE_I440FX_MACHINE(8, 2, v8_2, "pc-i440fx-8.2", NULL,
                       pc_i440fx_8_2_machine_options);
 
 static void pc_i440fx_8_1_machine_options(MachineClass *m)
@@ -563,7 +563,7 @@ static void pc_i440fx_8_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len);
 }
 
-DEFINE_I440FX_MACHINE(v8_1, "pc-i440fx-8.1", NULL,
+DEFINE_I440FX_MACHINE(8, 1, v8_1, "pc-i440fx-8.1", NULL,
                       pc_i440fx_8_1_machine_options);
 
 static void pc_i440fx_8_0_machine_options(MachineClass *m)
@@ -578,7 +578,7 @@ static void pc_i440fx_8_0_machine_options(MachineClass *m)
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
 }
 
-DEFINE_I440FX_MACHINE(v8_0, "pc-i440fx-8.0", NULL,
+DEFINE_I440FX_MACHINE(8, 0, v8_0, "pc-i440fx-8.0", NULL,
                       pc_i440fx_8_0_machine_options);
 
 static void pc_i440fx_7_2_machine_options(MachineClass *m)
@@ -588,7 +588,7 @@ static void pc_i440fx_7_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_7_2, pc_compat_7_2_len);
 }
 
-DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
+DEFINE_I440FX_MACHINE(7, 2, v7_2, "pc-i440fx-7.2", NULL,
                       pc_i440fx_7_2_machine_options);
 
 static void pc_i440fx_7_1_machine_options(MachineClass *m)
@@ -598,7 +598,7 @@ static void pc_i440fx_7_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
 }
 
-DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
+DEFINE_I440FX_MACHINE(7, 1, v7_1, "pc-i440fx-7.1", NULL,
                       pc_i440fx_7_1_machine_options);
 
 static void pc_i440fx_7_0_machine_options(MachineClass *m)
@@ -610,7 +610,7 @@ static void pc_i440fx_7_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
 }
 
-DEFINE_I440FX_MACHINE(v7_0, "pc-i440fx-7.0", NULL,
+DEFINE_I440FX_MACHINE(7, 0, v7_0, "pc-i440fx-7.0", NULL,
                       pc_i440fx_7_0_machine_options);
 
 static void pc_i440fx_6_2_machine_options(MachineClass *m)
@@ -620,7 +620,7 @@ static void pc_i440fx_6_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_6_2, pc_compat_6_2_len);
 }
 
-DEFINE_I440FX_MACHINE(v6_2, "pc-i440fx-6.2", NULL,
+DEFINE_I440FX_MACHINE(6, 2, v6_2, "pc-i440fx-6.2", NULL,
                       pc_i440fx_6_2_machine_options);
 
 static void pc_i440fx_6_1_machine_options(MachineClass *m)
@@ -631,7 +631,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
     m->smp_props.prefer_sockets = true;
 }
 
-DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
+DEFINE_I440FX_MACHINE(6, 1, v6_1, "pc-i440fx-6.1", NULL,
                       pc_i440fx_6_1_machine_options);
 
 static void pc_i440fx_6_0_machine_options(MachineClass *m)
@@ -641,7 +641,7 @@ static void pc_i440fx_6_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_6_0, pc_compat_6_0_len);
 }
 
-DEFINE_I440FX_MACHINE(v6_0, "pc-i440fx-6.0", NULL,
+DEFINE_I440FX_MACHINE(6, 0, v6_0, "pc-i440fx-6.0", NULL,
                       pc_i440fx_6_0_machine_options);
 
 static void pc_i440fx_5_2_machine_options(MachineClass *m)
@@ -651,7 +651,7 @@ static void pc_i440fx_5_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_5_2, pc_compat_5_2_len);
 }
 
-DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
+DEFINE_I440FX_MACHINE(5, 2, v5_2, "pc-i440fx-5.2", NULL,
                       pc_i440fx_5_2_machine_options);
 
 static void pc_i440fx_5_1_machine_options(MachineClass *m)
@@ -665,7 +665,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
     pcmc->pci_root_uid = 1;
 }
 
-DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
+DEFINE_I440FX_MACHINE(5, 1, v5_1, "pc-i440fx-5.1", NULL,
                       pc_i440fx_5_1_machine_options);
 
 static void pc_i440fx_5_0_machine_options(MachineClass *m)
@@ -677,7 +677,7 @@ static void pc_i440fx_5_0_machine_options(MachineClass *m)
     m->auto_enable_numa_with_memdev = false;
 }
 
-DEFINE_I440FX_MACHINE(v5_0, "pc-i440fx-5.0", NULL,
+DEFINE_I440FX_MACHINE(5, 0, v5_0, "pc-i440fx-5.0", NULL,
                       pc_i440fx_5_0_machine_options);
 
 static void pc_i440fx_4_2_machine_options(MachineClass *m)
@@ -687,7 +687,7 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
 }
 
-DEFINE_I440FX_MACHINE(v4_2, "pc-i440fx-4.2", NULL,
+DEFINE_I440FX_MACHINE(4, 2, v4_2, "pc-i440fx-4.2", NULL,
                       pc_i440fx_4_2_machine_options);
 
 static void pc_i440fx_4_1_machine_options(MachineClass *m)
@@ -697,7 +697,7 @@ static void pc_i440fx_4_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_4_1, pc_compat_4_1_len);
 }
 
-DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
+DEFINE_I440FX_MACHINE(4, 1, v4_1, "pc-i440fx-4.1", NULL,
                       pc_i440fx_4_1_machine_options);
 
 static void pc_i440fx_4_0_machine_options(MachineClass *m)
@@ -709,7 +709,7 @@ static void pc_i440fx_4_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
 }
 
-DEFINE_I440FX_MACHINE(v4_0, "pc-i440fx-4.0", NULL,
+DEFINE_I440FX_MACHINE(4, 0, v4_0, "pc-i440fx-4.0", NULL,
                       pc_i440fx_4_0_machine_options);
 
 static void pc_i440fx_3_1_machine_options(MachineClass *m)
@@ -723,7 +723,7 @@ static void pc_i440fx_3_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
 }
 
-DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
+DEFINE_I440FX_MACHINE(3, 1, v3_1, "pc-i440fx-3.1", NULL,
                       pc_i440fx_3_1_machine_options);
 
 static void pc_i440fx_3_0_machine_options(MachineClass *m)
@@ -733,7 +733,7 @@ static void pc_i440fx_3_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_3_0, pc_compat_3_0_len);
 }
 
-DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
+DEFINE_I440FX_MACHINE(3, 0, v3_0, "pc-i440fx-3.0", NULL,
                       pc_i440fx_3_0_machine_options);
 
 static void pc_i440fx_2_12_machine_options(MachineClass *m)
@@ -743,7 +743,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_12, pc_compat_2_12_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_12, "pc-i440fx-2.12", NULL,
+DEFINE_I440FX_MACHINE(2, 12, v2_12, "pc-i440fx-2.12", NULL,
                       pc_i440fx_2_12_machine_options);
 
 static void pc_i440fx_2_11_machine_options(MachineClass *m)
@@ -753,7 +753,7 @@ static void pc_i440fx_2_11_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_11, pc_compat_2_11_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
+DEFINE_I440FX_MACHINE(2, 11, v2_11, "pc-i440fx-2.11", NULL,
                       pc_i440fx_2_11_machine_options);
 
 static void pc_i440fx_2_10_machine_options(MachineClass *m)
@@ -764,7 +764,7 @@ static void pc_i440fx_2_10_machine_options(MachineClass *m)
     m->auto_enable_numa_with_memhp = false;
 }
 
-DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
+DEFINE_I440FX_MACHINE(2, 10, v2_10, "pc-i440fx-2.10", NULL,
                       pc_i440fx_2_10_machine_options);
 
 static void pc_i440fx_2_9_machine_options(MachineClass *m)
@@ -774,7 +774,7 @@ static void pc_i440fx_2_9_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_9, pc_compat_2_9_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
+DEFINE_I440FX_MACHINE(2, 9, v2_9, "pc-i440fx-2.9", NULL,
                       pc_i440fx_2_9_machine_options);
 
 static void pc_i440fx_2_8_machine_options(MachineClass *m)
@@ -784,7 +784,7 @@ static void pc_i440fx_2_8_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_8, pc_compat_2_8_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
+DEFINE_I440FX_MACHINE(2, 8, v2_8, "pc-i440fx-2.8", NULL,
                       pc_i440fx_2_8_machine_options);
 
 static void pc_i440fx_2_7_machine_options(MachineClass *m)
@@ -794,7 +794,7 @@ static void pc_i440fx_2_7_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_7, "pc-i440fx-2.7", NULL,
+DEFINE_I440FX_MACHINE(2, 7, v2_7, "pc-i440fx-2.7", NULL,
                       pc_i440fx_2_7_machine_options);
 
 static void pc_i440fx_2_6_machine_options(MachineClass *m)
@@ -809,7 +809,7 @@ static void pc_i440fx_2_6_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
+DEFINE_I440FX_MACHINE(2, 6, v2_6, "pc-i440fx-2.6", NULL,
                       pc_i440fx_2_6_machine_options);
 
 static void pc_i440fx_2_5_machine_options(MachineClass *m)
@@ -823,7 +823,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_5, pc_compat_2_5_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_5, "pc-i440fx-2.5", NULL,
+DEFINE_I440FX_MACHINE(2, 5, v2_5, "pc-i440fx-2.5", NULL,
                       pc_i440fx_2_5_machine_options);
 
 static void pc_i440fx_2_4_machine_options(MachineClass *m)
@@ -837,7 +837,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_4, pc_compat_2_4_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", NULL,
+DEFINE_I440FX_MACHINE(2, 4, v2_4, "pc-i440fx-2.4", NULL,
                       pc_i440fx_2_4_machine_options)
 
 static void pc_i440fx_2_3_machine_options(MachineClass *m)
@@ -849,7 +849,7 @@ static void pc_i440fx_2_3_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_3, pc_compat_2_3_len);
 }
 
-DEFINE_I440FX_MACHINE(v2_3, "pc-i440fx-2.3", pc_compat_2_3_fn,
+DEFINE_I440FX_MACHINE(2, 3, v2_3, "pc-i440fx-2.3", pc_compat_2_3_fn,
                       pc_i440fx_2_3_machine_options);
 
 static void pc_i440fx_2_2_machine_options(MachineClass *m)
@@ -865,7 +865,7 @@ static void pc_i440fx_2_2_machine_options(MachineClass *m)
     pcmc->resizable_acpi_blob = false;
 }
 
-DEFINE_I440FX_MACHINE(v2_2, "pc-i440fx-2.2", pc_compat_2_2_fn,
+DEFINE_I440FX_MACHINE(2, 2, v2_2, "pc-i440fx-2.2", pc_compat_2_2_fn,
                       pc_i440fx_2_2_machine_options);
 
 static void pc_i440fx_2_1_machine_options(MachineClass *m)
@@ -881,7 +881,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
     pcmc->enforce_aligned_dimm = false;
 }
 
-DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1_fn,
+DEFINE_I440FX_MACHINE(2, 1, v2_1, "pc-i440fx-2.1", pc_compat_2_1_fn,
                       pc_i440fx_2_1_machine_options);
 
 static void pc_i440fx_2_0_machine_options(MachineClass *m)
@@ -913,7 +913,7 @@ static void pc_i440fx_2_0_machine_options(MachineClass *m)
     pcmc->acpi_data_size = 0x10000;
 }
 
-DEFINE_I440FX_MACHINE(v2_0, "pc-i440fx-2.0", pc_compat_2_0_fn,
+DEFINE_I440FX_MACHINE(2, 0, v2_0, "pc-i440fx-2.0", pc_compat_2_0_fn,
                       pc_i440fx_2_0_machine_options);
 
 #ifdef CONFIG_ISAPC
@@ -936,7 +936,7 @@ static void isapc_machine_options(MachineClass *m)
     m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
 }
 
-DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
+DEFINE_PC_MACHINE(0, 0, isapc, "isapc", pc_init_isa,
                   isapc_machine_options);
 #endif
 
@@ -949,7 +949,7 @@ static void xenfv_4_2_machine_options(MachineClass *m)
     m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
 }
 
-DEFINE_PC_MACHINE(xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,
+DEFINE_PC_MACHINE(4, 2, xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,
                   xenfv_4_2_machine_options);
 
 static void xenfv_3_1_machine_options(MachineClass *m)
@@ -961,6 +961,6 @@ static void xenfv_3_1_machine_options(MachineClass *m)
     m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
 }
 
-DEFINE_PC_MACHINE(xenfv, "xenfv-3.1", pc_xen_hvm_init,
+DEFINE_PC_MACHINE(3, 1, xenfv, "xenfv-3.1", pc_xen_hvm_init,
                   xenfv_3_1_machine_options);
 #endif
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index bb53a51ac1..df3614b9bc 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -331,7 +331,7 @@ static void pc_q35_init(MachineState *machine)
     }
 }
 
-#define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
+#define DEFINE_Q35_MACHINE(major, minor, suffix, name, compatfn, optionfn) \
     static void pc_init_##suffix(MachineState *machine) \
     { \
         void (*compat)(MachineState *m) = (compatfn); \
@@ -340,7 +340,7 @@ static void pc_q35_init(MachineState *machine)
         } \
         pc_q35_init(machine); \
     } \
-    DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn)
+    DEFINE_PC_MACHINE(major, minor, suffix, name, pc_init_##suffix, optionfn)
 
 
 static void pc_q35_machine_options(MachineClass *m)
@@ -373,7 +373,7 @@ static void pc_q35_9_1_machine_options(MachineClass *m)
     m->alias = "q35";
 }
 
-DEFINE_Q35_MACHINE(v9_1, "pc-q35-9.1", NULL,
+DEFINE_Q35_MACHINE(9, 1, v9_1, "pc-q35-9.1", NULL,
                    pc_q35_9_1_machine_options);
 
 static void pc_q35_9_0_machine_options(MachineClass *m)
@@ -384,7 +384,7 @@ static void pc_q35_9_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
 }
 
-DEFINE_Q35_MACHINE(v9_0, "pc-q35-9.0", NULL,
+DEFINE_Q35_MACHINE(9, 0, v9_0, "pc-q35-9.0", NULL,
                    pc_q35_9_0_machine_options);
 
 static void pc_q35_8_2_machine_options(MachineClass *m)
@@ -398,7 +398,7 @@ static void pc_q35_8_2_machine_options(MachineClass *m)
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
 }
 
-DEFINE_Q35_MACHINE(v8_2, "pc-q35-8.2", NULL,
+DEFINE_Q35_MACHINE(8, 2, v8_2, "pc-q35-8.2", NULL,
                    pc_q35_8_2_machine_options);
 
 static void pc_q35_8_1_machine_options(MachineClass *m)
@@ -410,7 +410,7 @@ static void pc_q35_8_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len);
 }
 
-DEFINE_Q35_MACHINE(v8_1, "pc-q35-8.1", NULL,
+DEFINE_Q35_MACHINE(8, 1, v8_1, "pc-q35-8.1", NULL,
                    pc_q35_8_1_machine_options);
 
 static void pc_q35_8_0_machine_options(MachineClass *m)
@@ -426,7 +426,7 @@ static void pc_q35_8_0_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-DEFINE_Q35_MACHINE(v8_0, "pc-q35-8.0", NULL,
+DEFINE_Q35_MACHINE(8, 0, v8_0, "pc-q35-8.0", NULL,
                    pc_q35_8_0_machine_options);
 
 static void pc_q35_7_2_machine_options(MachineClass *m)
@@ -436,7 +436,7 @@ static void pc_q35_7_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_7_2, pc_compat_7_2_len);
 }
 
-DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
+DEFINE_Q35_MACHINE(7, 2, v7_2, "pc-q35-7.2", NULL,
                    pc_q35_7_2_machine_options);
 
 static void pc_q35_7_1_machine_options(MachineClass *m)
@@ -446,7 +446,7 @@ static void pc_q35_7_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
 }
 
-DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
+DEFINE_Q35_MACHINE(7, 1, v7_1, "pc-q35-7.1", NULL,
                    pc_q35_7_1_machine_options);
 
 static void pc_q35_7_0_machine_options(MachineClass *m)
@@ -458,7 +458,7 @@ static void pc_q35_7_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
 }
 
-DEFINE_Q35_MACHINE(v7_0, "pc-q35-7.0", NULL,
+DEFINE_Q35_MACHINE(7, 0, v7_0, "pc-q35-7.0", NULL,
                    pc_q35_7_0_machine_options);
 
 static void pc_q35_6_2_machine_options(MachineClass *m)
@@ -468,7 +468,7 @@ static void pc_q35_6_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_6_2, pc_compat_6_2_len);
 }
 
-DEFINE_Q35_MACHINE(v6_2, "pc-q35-6.2", NULL,
+DEFINE_Q35_MACHINE(6, 2, v6_2, "pc-q35-6.2", NULL,
                    pc_q35_6_2_machine_options);
 
 static void pc_q35_6_1_machine_options(MachineClass *m)
@@ -479,7 +479,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
     m->smp_props.prefer_sockets = true;
 }
 
-DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
+DEFINE_Q35_MACHINE(6, 1, v6_1, "pc-q35-6.1", NULL,
                    pc_q35_6_1_machine_options);
 
 static void pc_q35_6_0_machine_options(MachineClass *m)
@@ -489,7 +489,7 @@ static void pc_q35_6_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_6_0, pc_compat_6_0_len);
 }
 
-DEFINE_Q35_MACHINE(v6_0, "pc-q35-6.0", NULL,
+DEFINE_Q35_MACHINE(6, 0, v6_0, "pc-q35-6.0", NULL,
                    pc_q35_6_0_machine_options);
 
 static void pc_q35_5_2_machine_options(MachineClass *m)
@@ -499,7 +499,7 @@ static void pc_q35_5_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_5_2, pc_compat_5_2_len);
 }
 
-DEFINE_Q35_MACHINE(v5_2, "pc-q35-5.2", NULL,
+DEFINE_Q35_MACHINE(5, 2, v5_2, "pc-q35-5.2", NULL,
                    pc_q35_5_2_machine_options);
 
 static void pc_q35_5_1_machine_options(MachineClass *m)
@@ -513,7 +513,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
     pcmc->pci_root_uid = 1;
 }
 
-DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
+DEFINE_Q35_MACHINE(5, 1, v5_1, "pc-q35-5.1", NULL,
                    pc_q35_5_1_machine_options);
 
 static void pc_q35_5_0_machine_options(MachineClass *m)
@@ -525,7 +525,7 @@ static void pc_q35_5_0_machine_options(MachineClass *m)
     m->auto_enable_numa_with_memdev = false;
 }
 
-DEFINE_Q35_MACHINE(v5_0, "pc-q35-5.0", NULL,
+DEFINE_Q35_MACHINE(5, 0, v5_0, "pc-q35-5.0", NULL,
                    pc_q35_5_0_machine_options);
 
 static void pc_q35_4_2_machine_options(MachineClass *m)
@@ -535,7 +535,7 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
 }
 
-DEFINE_Q35_MACHINE(v4_2, "pc-q35-4.2", NULL,
+DEFINE_Q35_MACHINE(4, 2, v4_2, "pc-q35-4.2", NULL,
                    pc_q35_4_2_machine_options);
 
 static void pc_q35_4_1_machine_options(MachineClass *m)
@@ -545,7 +545,7 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_4_1, pc_compat_4_1_len);
 }
 
-DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
+DEFINE_Q35_MACHINE(4, 1, v4_1, "pc-q35-4.1", NULL,
                    pc_q35_4_1_machine_options);
 
 static void pc_q35_4_0_1_machine_options(MachineClass *m)
@@ -562,7 +562,7 @@ static void pc_q35_4_0_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
 }
 
-DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
+DEFINE_Q35_MACHINE(4, 0, v4_0_1, "pc-q35-4.0.1", NULL,
                    pc_q35_4_0_1_machine_options);
 
 static void pc_q35_4_0_machine_options(MachineClass *m)
@@ -572,7 +572,7 @@ static void pc_q35_4_0_machine_options(MachineClass *m)
     /* Compat props are applied by the 4.0.1 machine */
 }
 
-DEFINE_Q35_MACHINE(v4_0, "pc-q35-4.0", NULL,
+DEFINE_Q35_MACHINE(4, 0, v4_0, "pc-q35-4.0", NULL,
                    pc_q35_4_0_machine_options);
 
 static void pc_q35_3_1_machine_options(MachineClass *m)
@@ -587,7 +587,7 @@ static void pc_q35_3_1_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
 }
 
-DEFINE_Q35_MACHINE(v3_1, "pc-q35-3.1", NULL,
+DEFINE_Q35_MACHINE(3, 1, v3_1, "pc-q35-3.1", NULL,
                    pc_q35_3_1_machine_options);
 
 static void pc_q35_3_0_machine_options(MachineClass *m)
@@ -597,7 +597,7 @@ static void pc_q35_3_0_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_3_0, pc_compat_3_0_len);
 }
 
-DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
+DEFINE_Q35_MACHINE(3, 0, v3_0, "pc-q35-3.0", NULL,
                     pc_q35_3_0_machine_options);
 
 static void pc_q35_2_12_machine_options(MachineClass *m)
@@ -607,7 +607,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_12, pc_compat_2_12_len);
 }
 
-DEFINE_Q35_MACHINE(v2_12, "pc-q35-2.12", NULL,
+DEFINE_Q35_MACHINE(2, 12, v2_12, "pc-q35-2.12", NULL,
                    pc_q35_2_12_machine_options);
 
 static void pc_q35_2_11_machine_options(MachineClass *m)
@@ -618,7 +618,7 @@ static void pc_q35_2_11_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_11, pc_compat_2_11_len);
 }
 
-DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
+DEFINE_Q35_MACHINE(2, 11, v2_11, "pc-q35-2.11", NULL,
                    pc_q35_2_11_machine_options);
 
 static void pc_q35_2_10_machine_options(MachineClass *m)
@@ -629,7 +629,7 @@ static void pc_q35_2_10_machine_options(MachineClass *m)
     m->auto_enable_numa_with_memhp = false;
 }
 
-DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
+DEFINE_Q35_MACHINE(2, 10, v2_10, "pc-q35-2.10", NULL,
                    pc_q35_2_10_machine_options);
 
 static void pc_q35_2_9_machine_options(MachineClass *m)
@@ -639,7 +639,7 @@ static void pc_q35_2_9_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_9, pc_compat_2_9_len);
 }
 
-DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
+DEFINE_Q35_MACHINE(2, 9, v2_9, "pc-q35-2.9", NULL,
                    pc_q35_2_9_machine_options);
 
 static void pc_q35_2_8_machine_options(MachineClass *m)
@@ -649,7 +649,7 @@ static void pc_q35_2_8_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_8, pc_compat_2_8_len);
 }
 
-DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
+DEFINE_Q35_MACHINE(2, 8, v2_8, "pc-q35-2.8", NULL,
                    pc_q35_2_8_machine_options);
 
 static void pc_q35_2_7_machine_options(MachineClass *m)
@@ -660,7 +660,7 @@ static void pc_q35_2_7_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
 }
 
-DEFINE_Q35_MACHINE(v2_7, "pc-q35-2.7", NULL,
+DEFINE_Q35_MACHINE(2, 7, v2_7, "pc-q35-2.7", NULL,
                    pc_q35_2_7_machine_options);
 
 static void pc_q35_2_6_machine_options(MachineClass *m)
@@ -675,7 +675,7 @@ static void pc_q35_2_6_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
 }
 
-DEFINE_Q35_MACHINE(v2_6, "pc-q35-2.6", NULL,
+DEFINE_Q35_MACHINE(2, 6, v2_6, "pc-q35-2.6", NULL,
                    pc_q35_2_6_machine_options);
 
 static void pc_q35_2_5_machine_options(MachineClass *m)
@@ -689,7 +689,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_5, pc_compat_2_5_len);
 }
 
-DEFINE_Q35_MACHINE(v2_5, "pc-q35-2.5", NULL,
+DEFINE_Q35_MACHINE(2, 5, v2_5, "pc-q35-2.5", NULL,
                    pc_q35_2_5_machine_options);
 
 static void pc_q35_2_4_machine_options(MachineClass *m)
@@ -703,5 +703,5 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     compat_props_add(m->compat_props, pc_compat_2_4, pc_compat_2_4_len);
 }
 
-DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", NULL,
+DEFINE_Q35_MACHINE(2, 4, v2_4, "pc-q35-2.4", NULL,
                    pc_q35_2_4_machine_options);
-- 
2.41.0.28.gd7d8841f67

[PATCH v3 5/5] virtio-gpu: fix v2 migration

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@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 previously introduced check_machine_version() function as a
field test to ensure proper saving/loading based on the machine version.
The VMSD.version is irrelevant now.

Fixes: dfcf74fa ("virtio-gpu: fix scanout migration post-load")
Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index ae831b6b3e..b2d8e5faeb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -20,6 +20,7 @@
 #include "trace.h"
 #include "sysemu/dma.h"
 #include "sysemu/sysemu.h"
+#include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
 #include "hw/virtio/virtio-gpu.h"
@@ -1166,10 +1167,14 @@ static void virtio_gpu_cursor_bh(void *opaque)
     virtio_gpu_handle_cursor(&g->parent_obj.parent_obj, g->cursor_vq);
 }
 
+static bool machine_check_9_0(void *opaque, int version)
+{
+    return machine_check_version(9, 0);
+}
+
 static const VMStateDescription vmstate_virtio_gpu_scanout = {
     .name = "virtio-gpu-one-scanout",
-    .version_id = 2,
-    .minimum_version_id = 1,
+    .version_id = 1,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(resource_id, struct virtio_gpu_scanout),
         VMSTATE_UINT32(width, struct virtio_gpu_scanout),
@@ -1181,12 +1186,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, machine_check_9_0),
+        VMSTATE_UINT32_TEST(fb.bytes_pp, struct virtio_gpu_scanout, machine_check_9_0),
+        VMSTATE_UINT32_TEST(fb.width, struct virtio_gpu_scanout, machine_check_9_0),
+        VMSTATE_UINT32_TEST(fb.height, struct virtio_gpu_scanout, machine_check_9_0),
+        VMSTATE_UINT32_TEST(fb.stride, struct virtio_gpu_scanout, machine_check_9_0),
+        VMSTATE_UINT32_TEST(fb.offset, struct virtio_gpu_scanout, machine_check_9_0),
         VMSTATE_END_OF_LIST()
     },
 };
-- 
2.41.0.28.gd7d8841f67

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Michael S. Tsirkin]

On Wed, May 15, 2024 at 06:15:56PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@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 previously introduced check_machine_version() function as a
> field test to ensure proper saving/loading based on the machine version.
> The VMSD.version is irrelevant now.
> 
> Fixes: dfcf74fa ("virtio-gpu: fix scanout migration post-load")
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I don't get it. Our standard way to do it is:
- add a property (begin name with x- so we don't commit to an API)
- set from compat machinery
- test property value in VMSTATE macros

Big advantage is, it works well with any downstreams
which pick any properties they like.
Why is this not a good fit here?


> ---
>  hw/display/virtio-gpu.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index ae831b6b3e..b2d8e5faeb 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -20,6 +20,7 @@
>  #include "trace.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
>  #include "hw/virtio/virtio-gpu.h"
> @@ -1166,10 +1167,14 @@ static void virtio_gpu_cursor_bh(void *opaque)
>      virtio_gpu_handle_cursor(&g->parent_obj.parent_obj, g->cursor_vq);
>  }
>  
> +static bool machine_check_9_0(void *opaque, int version)
> +{
> +    return machine_check_version(9, 0);
> +}
> +
>  static const VMStateDescription vmstate_virtio_gpu_scanout = {
>      .name = "virtio-gpu-one-scanout",
> -    .version_id = 2,
> -    .minimum_version_id = 1,
> +    .version_id = 1,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32(resource_id, struct virtio_gpu_scanout),
>          VMSTATE_UINT32(width, struct virtio_gpu_scanout),
> @@ -1181,12 +1186,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, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.bytes_pp, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.width, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.height, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.stride, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.offset, struct virtio_gpu_scanout, machine_check_9_0),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> -- 
> 2.41.0.28.gd7d8841f67

Re: [PATCH v3 4/5] Set major/minor for PC and arm machines

[Michael S. Tsirkin]

On Wed, May 15, 2024 at 06:15:55PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>


I would much rather compat machinery was in one place
as opposed to being spread all over the codebase as this
new API would encourage.

> ---
>  include/hw/i386/pc.h |  4 ++-
>  hw/arm/virt.c        |  2 ++
>  hw/i386/pc_piix.c    | 74 ++++++++++++++++++++++----------------------
>  hw/i386/pc_q35.c     | 62 ++++++++++++++++++-------------------
>  4 files changed, 73 insertions(+), 69 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e52290916c..fa91bb7603 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -292,12 +292,14 @@ extern const size_t pc_compat_2_1_len;
>  extern GlobalProperty pc_compat_2_0[];
>  extern const size_t pc_compat_2_0_len;
>  
> -#define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
> +#define DEFINE_PC_MACHINE(maj, min, suffix, namestr, initfn, optsfn) \
>      static void pc_machine_##suffix##_class_init(ObjectClass *oc, void *data) \
>      { \
>          MachineClass *mc = MACHINE_CLASS(oc); \
>          optsfn(mc); \
>          mc->init = initfn; \
> +        mc->major = maj; \
> +        mc->minor = min; \
>      } \
>      static const TypeInfo pc_machine_type_##suffix = { \
>          .name       = namestr TYPE_MACHINE_SUFFIX, \
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3c93c0c0a6..7e3a03b39a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -109,6 +109,8 @@ static void arm_virt_compat_set(MachineClass *mc)
>          arm_virt_compat_set(mc); \
>          virt_machine_##major##_##minor##_options(mc); \
>          mc->desc = "QEMU " # major "." # minor " ARM Virtual Machine"; \
> +        mc->ma##jor = major; \
> +        mc->mi##nor = minor; \
>          if (latest) { \
>              mc->alias = "virt"; \
>          } \
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 99efb3c45c..bb6767d8d0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -477,7 +477,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>  }
>  #endif
>  
> -#define DEFINE_I440FX_MACHINE(suffix, name, compatfn, optionfn) \
> +#define DEFINE_I440FX_MACHINE(major, minor, suffix, name, compatfn, optionfn) \
>      static void pc_init_##suffix(MachineState *machine) \
>      { \
>          void (*compat)(MachineState *m) = (compatfn); \
> @@ -486,7 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>          } \
>          pc_init1(machine, TYPE_I440FX_PCI_DEVICE); \
>      } \
> -    DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn)
> +    DEFINE_PC_MACHINE(major, minor, suffix, name, pc_init_##suffix, optionfn)
>  
>  static void pc_i440fx_machine_options(MachineClass *m)
>  {
> @@ -521,7 +521,7 @@ static void pc_i440fx_9_1_machine_options(MachineClass *m)
>      m->is_default = true;
>  }
>  
> -DEFINE_I440FX_MACHINE(v9_1, "pc-i440fx-9.1", NULL,
> +DEFINE_I440FX_MACHINE(9, 1, v9_1, "pc-i440fx-9.1", NULL,
>                        pc_i440fx_9_1_machine_options);
>  
>  static void pc_i440fx_9_0_machine_options(MachineClass *m)
> @@ -534,7 +534,7 @@ static void pc_i440fx_9_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v9_0, "pc-i440fx-9.0", NULL,
> +DEFINE_I440FX_MACHINE(9, 0, v9_0, "pc-i440fx-9.0", NULL,
>                        pc_i440fx_9_0_machine_options);
>  
>  static void pc_i440fx_8_2_machine_options(MachineClass *m)
> @@ -549,7 +549,7 @@ static void pc_i440fx_8_2_machine_options(MachineClass *m)
>      pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
>  }
>  
> -DEFINE_I440FX_MACHINE(v8_2, "pc-i440fx-8.2", NULL,
> +DEFINE_I440FX_MACHINE(8, 2, v8_2, "pc-i440fx-8.2", NULL,
>                        pc_i440fx_8_2_machine_options);
>  
>  static void pc_i440fx_8_1_machine_options(MachineClass *m)
> @@ -563,7 +563,7 @@ static void pc_i440fx_8_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v8_1, "pc-i440fx-8.1", NULL,
> +DEFINE_I440FX_MACHINE(8, 1, v8_1, "pc-i440fx-8.1", NULL,
>                        pc_i440fx_8_1_machine_options);
>  
>  static void pc_i440fx_8_0_machine_options(MachineClass *m)
> @@ -578,7 +578,7 @@ static void pc_i440fx_8_0_machine_options(MachineClass *m)
>      pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
>  }
>  
> -DEFINE_I440FX_MACHINE(v8_0, "pc-i440fx-8.0", NULL,
> +DEFINE_I440FX_MACHINE(8, 0, v8_0, "pc-i440fx-8.0", NULL,
>                        pc_i440fx_8_0_machine_options);
>  
>  static void pc_i440fx_7_2_machine_options(MachineClass *m)
> @@ -588,7 +588,7 @@ static void pc_i440fx_7_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_7_2, pc_compat_7_2_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
> +DEFINE_I440FX_MACHINE(7, 2, v7_2, "pc-i440fx-7.2", NULL,
>                        pc_i440fx_7_2_machine_options);
>  
>  static void pc_i440fx_7_1_machine_options(MachineClass *m)
> @@ -598,7 +598,7 @@ static void pc_i440fx_7_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
> +DEFINE_I440FX_MACHINE(7, 1, v7_1, "pc-i440fx-7.1", NULL,
>                        pc_i440fx_7_1_machine_options);
>  
>  static void pc_i440fx_7_0_machine_options(MachineClass *m)
> @@ -610,7 +610,7 @@ static void pc_i440fx_7_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v7_0, "pc-i440fx-7.0", NULL,
> +DEFINE_I440FX_MACHINE(7, 0, v7_0, "pc-i440fx-7.0", NULL,
>                        pc_i440fx_7_0_machine_options);
>  
>  static void pc_i440fx_6_2_machine_options(MachineClass *m)
> @@ -620,7 +620,7 @@ static void pc_i440fx_6_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_6_2, pc_compat_6_2_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v6_2, "pc-i440fx-6.2", NULL,
> +DEFINE_I440FX_MACHINE(6, 2, v6_2, "pc-i440fx-6.2", NULL,
>                        pc_i440fx_6_2_machine_options);
>  
>  static void pc_i440fx_6_1_machine_options(MachineClass *m)
> @@ -631,7 +631,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
>      m->smp_props.prefer_sockets = true;
>  }
>  
> -DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
> +DEFINE_I440FX_MACHINE(6, 1, v6_1, "pc-i440fx-6.1", NULL,
>                        pc_i440fx_6_1_machine_options);
>  
>  static void pc_i440fx_6_0_machine_options(MachineClass *m)
> @@ -641,7 +641,7 @@ static void pc_i440fx_6_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_6_0, pc_compat_6_0_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v6_0, "pc-i440fx-6.0", NULL,
> +DEFINE_I440FX_MACHINE(6, 0, v6_0, "pc-i440fx-6.0", NULL,
>                        pc_i440fx_6_0_machine_options);
>  
>  static void pc_i440fx_5_2_machine_options(MachineClass *m)
> @@ -651,7 +651,7 @@ static void pc_i440fx_5_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_5_2, pc_compat_5_2_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
> +DEFINE_I440FX_MACHINE(5, 2, v5_2, "pc-i440fx-5.2", NULL,
>                        pc_i440fx_5_2_machine_options);
>  
>  static void pc_i440fx_5_1_machine_options(MachineClass *m)
> @@ -665,7 +665,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
>      pcmc->pci_root_uid = 1;
>  }
>  
> -DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
> +DEFINE_I440FX_MACHINE(5, 1, v5_1, "pc-i440fx-5.1", NULL,
>                        pc_i440fx_5_1_machine_options);
>  
>  static void pc_i440fx_5_0_machine_options(MachineClass *m)
> @@ -677,7 +677,7 @@ static void pc_i440fx_5_0_machine_options(MachineClass *m)
>      m->auto_enable_numa_with_memdev = false;
>  }
>  
> -DEFINE_I440FX_MACHINE(v5_0, "pc-i440fx-5.0", NULL,
> +DEFINE_I440FX_MACHINE(5, 0, v5_0, "pc-i440fx-5.0", NULL,
>                        pc_i440fx_5_0_machine_options);
>  
>  static void pc_i440fx_4_2_machine_options(MachineClass *m)
> @@ -687,7 +687,7 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v4_2, "pc-i440fx-4.2", NULL,
> +DEFINE_I440FX_MACHINE(4, 2, v4_2, "pc-i440fx-4.2", NULL,
>                        pc_i440fx_4_2_machine_options);
>  
>  static void pc_i440fx_4_1_machine_options(MachineClass *m)
> @@ -697,7 +697,7 @@ static void pc_i440fx_4_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_4_1, pc_compat_4_1_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
> +DEFINE_I440FX_MACHINE(4, 1, v4_1, "pc-i440fx-4.1", NULL,
>                        pc_i440fx_4_1_machine_options);
>  
>  static void pc_i440fx_4_0_machine_options(MachineClass *m)
> @@ -709,7 +709,7 @@ static void pc_i440fx_4_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v4_0, "pc-i440fx-4.0", NULL,
> +DEFINE_I440FX_MACHINE(4, 0, v4_0, "pc-i440fx-4.0", NULL,
>                        pc_i440fx_4_0_machine_options);
>  
>  static void pc_i440fx_3_1_machine_options(MachineClass *m)
> @@ -723,7 +723,7 @@ static void pc_i440fx_3_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
> +DEFINE_I440FX_MACHINE(3, 1, v3_1, "pc-i440fx-3.1", NULL,
>                        pc_i440fx_3_1_machine_options);
>  
>  static void pc_i440fx_3_0_machine_options(MachineClass *m)
> @@ -733,7 +733,7 @@ static void pc_i440fx_3_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_3_0, pc_compat_3_0_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
> +DEFINE_I440FX_MACHINE(3, 0, v3_0, "pc-i440fx-3.0", NULL,
>                        pc_i440fx_3_0_machine_options);
>  
>  static void pc_i440fx_2_12_machine_options(MachineClass *m)
> @@ -743,7 +743,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_12, pc_compat_2_12_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_12, "pc-i440fx-2.12", NULL,
> +DEFINE_I440FX_MACHINE(2, 12, v2_12, "pc-i440fx-2.12", NULL,
>                        pc_i440fx_2_12_machine_options);
>  
>  static void pc_i440fx_2_11_machine_options(MachineClass *m)
> @@ -753,7 +753,7 @@ static void pc_i440fx_2_11_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_11, pc_compat_2_11_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
> +DEFINE_I440FX_MACHINE(2, 11, v2_11, "pc-i440fx-2.11", NULL,
>                        pc_i440fx_2_11_machine_options);
>  
>  static void pc_i440fx_2_10_machine_options(MachineClass *m)
> @@ -764,7 +764,7 @@ static void pc_i440fx_2_10_machine_options(MachineClass *m)
>      m->auto_enable_numa_with_memhp = false;
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
> +DEFINE_I440FX_MACHINE(2, 10, v2_10, "pc-i440fx-2.10", NULL,
>                        pc_i440fx_2_10_machine_options);
>  
>  static void pc_i440fx_2_9_machine_options(MachineClass *m)
> @@ -774,7 +774,7 @@ static void pc_i440fx_2_9_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_9, pc_compat_2_9_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
> +DEFINE_I440FX_MACHINE(2, 9, v2_9, "pc-i440fx-2.9", NULL,
>                        pc_i440fx_2_9_machine_options);
>  
>  static void pc_i440fx_2_8_machine_options(MachineClass *m)
> @@ -784,7 +784,7 @@ static void pc_i440fx_2_8_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_8, pc_compat_2_8_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
> +DEFINE_I440FX_MACHINE(2, 8, v2_8, "pc-i440fx-2.8", NULL,
>                        pc_i440fx_2_8_machine_options);
>  
>  static void pc_i440fx_2_7_machine_options(MachineClass *m)
> @@ -794,7 +794,7 @@ static void pc_i440fx_2_7_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_7, "pc-i440fx-2.7", NULL,
> +DEFINE_I440FX_MACHINE(2, 7, v2_7, "pc-i440fx-2.7", NULL,
>                        pc_i440fx_2_7_machine_options);
>  
>  static void pc_i440fx_2_6_machine_options(MachineClass *m)
> @@ -809,7 +809,7 @@ static void pc_i440fx_2_6_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
> +DEFINE_I440FX_MACHINE(2, 6, v2_6, "pc-i440fx-2.6", NULL,
>                        pc_i440fx_2_6_machine_options);
>  
>  static void pc_i440fx_2_5_machine_options(MachineClass *m)
> @@ -823,7 +823,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_5, pc_compat_2_5_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_5, "pc-i440fx-2.5", NULL,
> +DEFINE_I440FX_MACHINE(2, 5, v2_5, "pc-i440fx-2.5", NULL,
>                        pc_i440fx_2_5_machine_options);
>  
>  static void pc_i440fx_2_4_machine_options(MachineClass *m)
> @@ -837,7 +837,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_4, pc_compat_2_4_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", NULL,
> +DEFINE_I440FX_MACHINE(2, 4, v2_4, "pc-i440fx-2.4", NULL,
>                        pc_i440fx_2_4_machine_options)
>  
>  static void pc_i440fx_2_3_machine_options(MachineClass *m)
> @@ -849,7 +849,7 @@ static void pc_i440fx_2_3_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_3, pc_compat_2_3_len);
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_3, "pc-i440fx-2.3", pc_compat_2_3_fn,
> +DEFINE_I440FX_MACHINE(2, 3, v2_3, "pc-i440fx-2.3", pc_compat_2_3_fn,
>                        pc_i440fx_2_3_machine_options);
>  
>  static void pc_i440fx_2_2_machine_options(MachineClass *m)
> @@ -865,7 +865,7 @@ static void pc_i440fx_2_2_machine_options(MachineClass *m)
>      pcmc->resizable_acpi_blob = false;
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_2, "pc-i440fx-2.2", pc_compat_2_2_fn,
> +DEFINE_I440FX_MACHINE(2, 2, v2_2, "pc-i440fx-2.2", pc_compat_2_2_fn,
>                        pc_i440fx_2_2_machine_options);
>  
>  static void pc_i440fx_2_1_machine_options(MachineClass *m)
> @@ -881,7 +881,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
>      pcmc->enforce_aligned_dimm = false;
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1_fn,
> +DEFINE_I440FX_MACHINE(2, 1, v2_1, "pc-i440fx-2.1", pc_compat_2_1_fn,
>                        pc_i440fx_2_1_machine_options);
>  
>  static void pc_i440fx_2_0_machine_options(MachineClass *m)
> @@ -913,7 +913,7 @@ static void pc_i440fx_2_0_machine_options(MachineClass *m)
>      pcmc->acpi_data_size = 0x10000;
>  }
>  
> -DEFINE_I440FX_MACHINE(v2_0, "pc-i440fx-2.0", pc_compat_2_0_fn,
> +DEFINE_I440FX_MACHINE(2, 0, v2_0, "pc-i440fx-2.0", pc_compat_2_0_fn,
>                        pc_i440fx_2_0_machine_options);
>  
>  #ifdef CONFIG_ISAPC
> @@ -936,7 +936,7 @@ static void isapc_machine_options(MachineClass *m)
>      m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
>  }
>  
> -DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
> +DEFINE_PC_MACHINE(0, 0, isapc, "isapc", pc_init_isa,
>                    isapc_machine_options);
>  #endif
>  
> @@ -949,7 +949,7 @@ static void xenfv_4_2_machine_options(MachineClass *m)
>      m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
>  }
>  
> -DEFINE_PC_MACHINE(xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,
> +DEFINE_PC_MACHINE(4, 2, xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,
>                    xenfv_4_2_machine_options);
>  
>  static void xenfv_3_1_machine_options(MachineClass *m)
> @@ -961,6 +961,6 @@ static void xenfv_3_1_machine_options(MachineClass *m)
>      m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
>  }
>  
> -DEFINE_PC_MACHINE(xenfv, "xenfv-3.1", pc_xen_hvm_init,
> +DEFINE_PC_MACHINE(3, 1, xenfv, "xenfv-3.1", pc_xen_hvm_init,
>                    xenfv_3_1_machine_options);
>  #endif
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index bb53a51ac1..df3614b9bc 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -331,7 +331,7 @@ static void pc_q35_init(MachineState *machine)
>      }
>  }
>  
> -#define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
> +#define DEFINE_Q35_MACHINE(major, minor, suffix, name, compatfn, optionfn) \
>      static void pc_init_##suffix(MachineState *machine) \
>      { \
>          void (*compat)(MachineState *m) = (compatfn); \
> @@ -340,7 +340,7 @@ static void pc_q35_init(MachineState *machine)
>          } \
>          pc_q35_init(machine); \
>      } \
> -    DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn)
> +    DEFINE_PC_MACHINE(major, minor, suffix, name, pc_init_##suffix, optionfn)
>  
>  
>  static void pc_q35_machine_options(MachineClass *m)
> @@ -373,7 +373,7 @@ static void pc_q35_9_1_machine_options(MachineClass *m)
>      m->alias = "q35";
>  }
>  
> -DEFINE_Q35_MACHINE(v9_1, "pc-q35-9.1", NULL,
> +DEFINE_Q35_MACHINE(9, 1, v9_1, "pc-q35-9.1", NULL,
>                     pc_q35_9_1_machine_options);
>  
>  static void pc_q35_9_0_machine_options(MachineClass *m)
> @@ -384,7 +384,7 @@ static void pc_q35_9_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v9_0, "pc-q35-9.0", NULL,
> +DEFINE_Q35_MACHINE(9, 0, v9_0, "pc-q35-9.0", NULL,
>                     pc_q35_9_0_machine_options);
>  
>  static void pc_q35_8_2_machine_options(MachineClass *m)
> @@ -398,7 +398,7 @@ static void pc_q35_8_2_machine_options(MachineClass *m)
>      pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
>  }
>  
> -DEFINE_Q35_MACHINE(v8_2, "pc-q35-8.2", NULL,
> +DEFINE_Q35_MACHINE(8, 2, v8_2, "pc-q35-8.2", NULL,
>                     pc_q35_8_2_machine_options);
>  
>  static void pc_q35_8_1_machine_options(MachineClass *m)
> @@ -410,7 +410,7 @@ static void pc_q35_8_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v8_1, "pc-q35-8.1", NULL,
> +DEFINE_Q35_MACHINE(8, 1, v8_1, "pc-q35-8.1", NULL,
>                     pc_q35_8_1_machine_options);
>  
>  static void pc_q35_8_0_machine_options(MachineClass *m)
> @@ -426,7 +426,7 @@ static void pc_q35_8_0_machine_options(MachineClass *m)
>      m->max_cpus = 288;
>  }
>  
> -DEFINE_Q35_MACHINE(v8_0, "pc-q35-8.0", NULL,
> +DEFINE_Q35_MACHINE(8, 0, v8_0, "pc-q35-8.0", NULL,
>                     pc_q35_8_0_machine_options);
>  
>  static void pc_q35_7_2_machine_options(MachineClass *m)
> @@ -436,7 +436,7 @@ static void pc_q35_7_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_7_2, pc_compat_7_2_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
> +DEFINE_Q35_MACHINE(7, 2, v7_2, "pc-q35-7.2", NULL,
>                     pc_q35_7_2_machine_options);
>  
>  static void pc_q35_7_1_machine_options(MachineClass *m)
> @@ -446,7 +446,7 @@ static void pc_q35_7_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
> +DEFINE_Q35_MACHINE(7, 1, v7_1, "pc-q35-7.1", NULL,
>                     pc_q35_7_1_machine_options);
>  
>  static void pc_q35_7_0_machine_options(MachineClass *m)
> @@ -458,7 +458,7 @@ static void pc_q35_7_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v7_0, "pc-q35-7.0", NULL,
> +DEFINE_Q35_MACHINE(7, 0, v7_0, "pc-q35-7.0", NULL,
>                     pc_q35_7_0_machine_options);
>  
>  static void pc_q35_6_2_machine_options(MachineClass *m)
> @@ -468,7 +468,7 @@ static void pc_q35_6_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_6_2, pc_compat_6_2_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v6_2, "pc-q35-6.2", NULL,
> +DEFINE_Q35_MACHINE(6, 2, v6_2, "pc-q35-6.2", NULL,
>                     pc_q35_6_2_machine_options);
>  
>  static void pc_q35_6_1_machine_options(MachineClass *m)
> @@ -479,7 +479,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
>      m->smp_props.prefer_sockets = true;
>  }
>  
> -DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
> +DEFINE_Q35_MACHINE(6, 1, v6_1, "pc-q35-6.1", NULL,
>                     pc_q35_6_1_machine_options);
>  
>  static void pc_q35_6_0_machine_options(MachineClass *m)
> @@ -489,7 +489,7 @@ static void pc_q35_6_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_6_0, pc_compat_6_0_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v6_0, "pc-q35-6.0", NULL,
> +DEFINE_Q35_MACHINE(6, 0, v6_0, "pc-q35-6.0", NULL,
>                     pc_q35_6_0_machine_options);
>  
>  static void pc_q35_5_2_machine_options(MachineClass *m)
> @@ -499,7 +499,7 @@ static void pc_q35_5_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_5_2, pc_compat_5_2_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v5_2, "pc-q35-5.2", NULL,
> +DEFINE_Q35_MACHINE(5, 2, v5_2, "pc-q35-5.2", NULL,
>                     pc_q35_5_2_machine_options);
>  
>  static void pc_q35_5_1_machine_options(MachineClass *m)
> @@ -513,7 +513,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
>      pcmc->pci_root_uid = 1;
>  }
>  
> -DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
> +DEFINE_Q35_MACHINE(5, 1, v5_1, "pc-q35-5.1", NULL,
>                     pc_q35_5_1_machine_options);
>  
>  static void pc_q35_5_0_machine_options(MachineClass *m)
> @@ -525,7 +525,7 @@ static void pc_q35_5_0_machine_options(MachineClass *m)
>      m->auto_enable_numa_with_memdev = false;
>  }
>  
> -DEFINE_Q35_MACHINE(v5_0, "pc-q35-5.0", NULL,
> +DEFINE_Q35_MACHINE(5, 0, v5_0, "pc-q35-5.0", NULL,
>                     pc_q35_5_0_machine_options);
>  
>  static void pc_q35_4_2_machine_options(MachineClass *m)
> @@ -535,7 +535,7 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v4_2, "pc-q35-4.2", NULL,
> +DEFINE_Q35_MACHINE(4, 2, v4_2, "pc-q35-4.2", NULL,
>                     pc_q35_4_2_machine_options);
>  
>  static void pc_q35_4_1_machine_options(MachineClass *m)
> @@ -545,7 +545,7 @@ static void pc_q35_4_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_4_1, pc_compat_4_1_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
> +DEFINE_Q35_MACHINE(4, 1, v4_1, "pc-q35-4.1", NULL,
>                     pc_q35_4_1_machine_options);
>  
>  static void pc_q35_4_0_1_machine_options(MachineClass *m)
> @@ -562,7 +562,7 @@ static void pc_q35_4_0_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
> +DEFINE_Q35_MACHINE(4, 0, v4_0_1, "pc-q35-4.0.1", NULL,
>                     pc_q35_4_0_1_machine_options);
>  
>  static void pc_q35_4_0_machine_options(MachineClass *m)
> @@ -572,7 +572,7 @@ static void pc_q35_4_0_machine_options(MachineClass *m)
>      /* Compat props are applied by the 4.0.1 machine */
>  }
>  
> -DEFINE_Q35_MACHINE(v4_0, "pc-q35-4.0", NULL,
> +DEFINE_Q35_MACHINE(4, 0, v4_0, "pc-q35-4.0", NULL,
>                     pc_q35_4_0_machine_options);
>  
>  static void pc_q35_3_1_machine_options(MachineClass *m)
> @@ -587,7 +587,7 @@ static void pc_q35_3_1_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v3_1, "pc-q35-3.1", NULL,
> +DEFINE_Q35_MACHINE(3, 1, v3_1, "pc-q35-3.1", NULL,
>                     pc_q35_3_1_machine_options);
>  
>  static void pc_q35_3_0_machine_options(MachineClass *m)
> @@ -597,7 +597,7 @@ static void pc_q35_3_0_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_3_0, pc_compat_3_0_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
> +DEFINE_Q35_MACHINE(3, 0, v3_0, "pc-q35-3.0", NULL,
>                      pc_q35_3_0_machine_options);
>  
>  static void pc_q35_2_12_machine_options(MachineClass *m)
> @@ -607,7 +607,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_12, pc_compat_2_12_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_12, "pc-q35-2.12", NULL,
> +DEFINE_Q35_MACHINE(2, 12, v2_12, "pc-q35-2.12", NULL,
>                     pc_q35_2_12_machine_options);
>  
>  static void pc_q35_2_11_machine_options(MachineClass *m)
> @@ -618,7 +618,7 @@ static void pc_q35_2_11_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_11, pc_compat_2_11_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
> +DEFINE_Q35_MACHINE(2, 11, v2_11, "pc-q35-2.11", NULL,
>                     pc_q35_2_11_machine_options);
>  
>  static void pc_q35_2_10_machine_options(MachineClass *m)
> @@ -629,7 +629,7 @@ static void pc_q35_2_10_machine_options(MachineClass *m)
>      m->auto_enable_numa_with_memhp = false;
>  }
>  
> -DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
> +DEFINE_Q35_MACHINE(2, 10, v2_10, "pc-q35-2.10", NULL,
>                     pc_q35_2_10_machine_options);
>  
>  static void pc_q35_2_9_machine_options(MachineClass *m)
> @@ -639,7 +639,7 @@ static void pc_q35_2_9_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_9, pc_compat_2_9_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
> +DEFINE_Q35_MACHINE(2, 9, v2_9, "pc-q35-2.9", NULL,
>                     pc_q35_2_9_machine_options);
>  
>  static void pc_q35_2_8_machine_options(MachineClass *m)
> @@ -649,7 +649,7 @@ static void pc_q35_2_8_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_8, pc_compat_2_8_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
> +DEFINE_Q35_MACHINE(2, 8, v2_8, "pc-q35-2.8", NULL,
>                     pc_q35_2_8_machine_options);
>  
>  static void pc_q35_2_7_machine_options(MachineClass *m)
> @@ -660,7 +660,7 @@ static void pc_q35_2_7_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_7, "pc-q35-2.7", NULL,
> +DEFINE_Q35_MACHINE(2, 7, v2_7, "pc-q35-2.7", NULL,
>                     pc_q35_2_7_machine_options);
>  
>  static void pc_q35_2_6_machine_options(MachineClass *m)
> @@ -675,7 +675,7 @@ static void pc_q35_2_6_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_6, "pc-q35-2.6", NULL,
> +DEFINE_Q35_MACHINE(2, 6, v2_6, "pc-q35-2.6", NULL,
>                     pc_q35_2_6_machine_options);
>  
>  static void pc_q35_2_5_machine_options(MachineClass *m)
> @@ -689,7 +689,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_5, pc_compat_2_5_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_5, "pc-q35-2.5", NULL,
> +DEFINE_Q35_MACHINE(2, 5, v2_5, "pc-q35-2.5", NULL,
>                     pc_q35_2_5_machine_options);
>  
>  static void pc_q35_2_4_machine_options(MachineClass *m)
> @@ -703,5 +703,5 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      compat_props_add(m->compat_props, pc_compat_2_4, pc_compat_2_4_len);
>  }
>  
> -DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", NULL,
> +DEFINE_Q35_MACHINE(2, 4, v2_4, "pc-q35-2.4", NULL,
>                     pc_q35_2_4_machine_options);
> -- 
> 2.41.0.28.gd7d8841f67

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Daniel P. Berrangé]

On Wed, May 15, 2024 at 06:15:56PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@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 previously introduced check_machine_version() function as a
> field test to ensure proper saving/loading based on the machine version.
> The VMSD.version is irrelevant now.
> 
> Fixes: dfcf74fa ("virtio-gpu: fix scanout migration post-load")
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/virtio-gpu.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index ae831b6b3e..b2d8e5faeb 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -20,6 +20,7 @@
>  #include "trace.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
>  #include "hw/virtio/virtio-gpu.h"
> @@ -1166,10 +1167,14 @@ static void virtio_gpu_cursor_bh(void *opaque)
>      virtio_gpu_handle_cursor(&g->parent_obj.parent_obj, g->cursor_vq);
>  }
>  
> +static bool machine_check_9_0(void *opaque, int version)
> +{
> +    return machine_check_version(9, 0);
> +}

I think applying version number checks to decide machine type
compatibility is a highly undesirable direction for QEMU to
take.

Machine type compatibility is a difficult problem, but one of
the good aspects about our current solution is that it is
clear what the differences are for each version. We can see
all the compatibility properties/flags/values being set in
one place, in the declaration of each machine's class.

Sprinkling version number checks around the codebase in
arbitrary files will harm visibility of what ABI is expressd
by each machine, and thus is liable to increase the liklihood
of mistakes.

This will negatively impact downstream vendors cherry-picking
patches to their stable branches, as the version number logic
may have incorrect semantics. 

It will also create trouble for downstream vendors who define
their own machines with distinct versioning from upstream, as
there will be confusion over whether a version check is for
the base QEMU version, or the downstream version, and such
code added to the tree is less visible than the machine type
definitions.

Above all, I'm failing to see why there's a compelling reason
for virtio_gpu to diverge from our long standing practice of
adding a named property flag "virtio_scanout_vmstate_fix"
on the machine class, and then setting it in machine types
which need it.


> +
>  static const VMStateDescription vmstate_virtio_gpu_scanout = {
>      .name = "virtio-gpu-one-scanout",
> -    .version_id = 2,
> -    .minimum_version_id = 1,
> +    .version_id = 1,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32(resource_id, struct virtio_gpu_scanout),
>          VMSTATE_UINT32(width, struct virtio_gpu_scanout),
> @@ -1181,12 +1186,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, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.bytes_pp, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.width, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.height, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.stride, struct virtio_gpu_scanout, machine_check_9_0),
> +        VMSTATE_UINT32_TEST(fb.offset, struct virtio_gpu_scanout, machine_check_9_0),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> -- 
> 2.41.0.28.gd7d8841f67
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Peter Xu]

On Wed, May 15, 2024 at 06:15:51PM +0400, marcandre.lureau@redhat.com wrote:
> 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)
> 
> v3:
>  - introduce machine_check_version()
>  - drop the VMSD version, and use machine version field test

Thanks for trying this out already.

Last time I mentioned this may for the long term because I remember Dan and
Thomas were trying to work on some machine deprecation work, and maybe such
things may collapse with that work (and perhaps easier with that work
landed, too?).  Just to copy them both here so we know where we are now, as
I didn't follow that discussion.  IOW, patch 3/4 may need separate review
from outside migration..

The simpler solution is we stick with the customized field and simple fix
to the issue first, then whenever we have that new helper later we simply
use the new helper to replace the old, alongside we can drop the new field
/ property too as long as it is declared with "x-".  Might be easier to
backport too in this case.  Marc-Andre, what do you think?

Thanks,

-- 
Peter Xu

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

[Daniel P. Berrangé]

On Wed, May 15, 2024 at 10:07:31AM -0600, Peter Xu wrote:
> On Wed, May 15, 2024 at 06:15:51PM +0400, marcandre.lureau@redhat.com wrote:
> > 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)
> > 
> > v3:
> >  - introduce machine_check_version()
> >  - drop the VMSD version, and use machine version field test
> 
> Thanks for trying this out already.
> 
> Last time I mentioned this may for the long term because I remember Dan and
> Thomas were trying to work on some machine deprecation work, and maybe such
> things may collapse with that work (and perhaps easier with that work
> landed, too?).  Just to copy them both here so we know where we are now, as
> I didn't follow that discussion.  IOW, patch 3/4 may need separate review
> from outside migration..

You'll be refering to my series here:

  https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg00084.html

Note that series very delibrately did *not* expose the version numbers
as accessible fields to code. The version number info is only accessible
within the machine type macros, and once the macros are expanded, the
version digits remains hidden within the opaque machine type name strings,
and/or method names.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Peter Xu]

On Wed, May 15, 2024 at 12:02:49PM -0400, Michael S. Tsirkin wrote:
> On Wed, May 15, 2024 at 06:15:56PM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@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 previously introduced check_machine_version() function as a
> > field test to ensure proper saving/loading based on the machine version.
> > The VMSD.version is irrelevant now.
> > 
> > Fixes: dfcf74fa ("virtio-gpu: fix scanout migration post-load")
> > Suggested-by: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> I don't get it. Our standard way to do it is:
> - add a property (begin name with x- so we don't commit to an API)
> - set from compat machinery
> - test property value in VMSTATE macros
> 
> Big advantage is, it works well with any downstreams
> which pick any properties they like.
> Why is this not a good fit here?

I think it'll simplify upstream to avoid introducing one new field + one
new property for each of such protocol change, which fundamentally are the
same thing.  But it's indeed a good point that such helper can slightly
complicate the backport a bit.. I assume a global replacement of versions
over the helper will be needed after downstream settles on how to map
downstream MCs to upstream's.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Peter Xu]

On Wed, May 15, 2024 at 05:03:44PM +0100, Daniel P. Berrangé wrote:
> Above all, I'm failing to see why there's a compelling reason
> for virtio_gpu to diverge from our long standing practice of
> adding a named property flag "virtio_scanout_vmstate_fix"
> on the machine class, and then setting it in machine types
> which need it.

The reason to introduce that is definitely avoid introducing fields /
properties in similar cases in which case all the fields may represent the
same thing ("return true if MC is older than xxx version").  Especially
when such change is not bound to a new feature so in which case it won't
make sense to allow user to even control that propoerty, even if we
exported this "x-virtio-scanout-fix" property, but now we must export it
because compat fields require it.

However I think agree that having upstream specific MC versions in VMSD
checks is kind of unwanted.  I think the major problem is we don't have
that extra machine type abstract where we can have simply a number showing
the release of QEMU, then we can map that number to whatever
upstream/downstream machine types.  E.g.:

  Release No.         Upstream version       Downstream version
  50                  9.0                    Y.0
  51                  9.1
  52                  9.2                    Y.1
  ...

Then downstream is not mapping to 9.0/... but the release no.  Then here
instead of hard code upstream MC versions we can already provide similar
helpers like:

  machine_type_newer_than_50()

Then device code can use it without polluting that with upstream MC
versioning.  Downstream will simply work if downstream MCs are mapped
alright to the release no. when rebase.

But I'm not sure whether it'll be even worthwhile.. the majority will still
be that the VMSD change is caused by a new feature, and exporting that
property might in most cases be wanted.

In all cases, for now I agree it's at least easier to stick with the simple
way.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Daniel P. Berrangé]

On Wed, May 15, 2024 at 11:03:27AM -0600, Peter Xu wrote:
> On Wed, May 15, 2024 at 05:03:44PM +0100, Daniel P. Berrangé wrote:
> > Above all, I'm failing to see why there's a compelling reason
> > for virtio_gpu to diverge from our long standing practice of
> > adding a named property flag "virtio_scanout_vmstate_fix"
> > on the machine class, and then setting it in machine types
> > which need it.
> 
> The reason to introduce that is definitely avoid introducing fields /
> properties in similar cases in which case all the fields may represent the
> same thing ("return true if MC is older than xxx version").  Especially
> when such change is not bound to a new feature so in which case it won't
> make sense to allow user to even control that propoerty, even if we
> exported this "x-virtio-scanout-fix" property, but now we must export it
> because compat fields require it.
> 
> However I think agree that having upstream specific MC versions in VMSD
> checks is kind of unwanted.  I think the major problem is we don't have
> that extra machine type abstract where we can have simply a number showing
> the release of QEMU, then we can map that number to whatever
> upstream/downstream machine types.  E.g.:
> 
>   Release No.         Upstream version       Downstream version
>   50                  9.0                    Y.0
>   51                  9.1
>   52                  9.2                    Y.1
>   ...

Downstream versions do not map cleanly to individual upstream versions
across the whole code base. If we have two distinct features in upstream
version X, each of them may map to a different downstream release. 

This can happen when downstream skips one or more upstream releases.
One feature from the skipped release might be backported to an earlier
downstream release, while other feature might not arrive downstream
until they later rebase. Version based checks are an inherantly
undesirable idea for a situation where there is any backporting taking
place, whether its machine type versions or something else. Named feature
/ flag based checks are always the way to go.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Michael S. Tsirkin]

On Wed, May 15, 2024 at 10:31:32AM -0600, Peter Xu wrote:
> On Wed, May 15, 2024 at 12:02:49PM -0400, Michael S. Tsirkin wrote:
> > On Wed, May 15, 2024 at 06:15:56PM +0400, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@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 previously introduced check_machine_version() function as a
> > > field test to ensure proper saving/loading based on the machine version.
> > > The VMSD.version is irrelevant now.
> > > 
> > > Fixes: dfcf74fa ("virtio-gpu: fix scanout migration post-load")
> > > Suggested-by: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > I don't get it. Our standard way to do it is:
> > - add a property (begin name with x- so we don't commit to an API)
> > - set from compat machinery
> > - test property value in VMSTATE macros
> > 
> > Big advantage is, it works well with any downstreams
> > which pick any properties they like.
> > Why is this not a good fit here?
> 
> I think it'll simplify upstream to avoid introducing one new field + one
> new property for each of such protocol change, which fundamentally are the
> same thing.  But it's indeed a good point that such helper can slightly
> complicate the backport a bit.. I assume a global replacement of versions
> over the helper will be needed after downstream settles on how to map
> downstream MCs to upstream's.
> 
> Thanks,

There's nothing special about this specific code. If we want to rework
how machine compat is handled we can do it, but I wouldn't start with
this virtio gpu bug.

It's a big if though, I don't like how this patch works at all.

-- 
MST

Re: [PATCH v3 5/5] virtio-gpu: fix v2 migration

[Peter Xu]

On Wed, May 15, 2024 at 06:15:48PM +0100, Daniel P. Berrangé wrote:
> On Wed, May 15, 2024 at 11:03:27AM -0600, Peter Xu wrote:
> > On Wed, May 15, 2024 at 05:03:44PM +0100, Daniel P. Berrangé wrote:
> > > Above all, I'm failing to see why there's a compelling reason
> > > for virtio_gpu to diverge from our long standing practice of
> > > adding a named property flag "virtio_scanout_vmstate_fix"
> > > on the machine class, and then setting it in machine types
> > > which need it.
> > 
> > The reason to introduce that is definitely avoid introducing fields /
> > properties in similar cases in which case all the fields may represent the
> > same thing ("return true if MC is older than xxx version").  Especially
> > when such change is not bound to a new feature so in which case it won't
> > make sense to allow user to even control that propoerty, even if we
> > exported this "x-virtio-scanout-fix" property, but now we must export it
> > because compat fields require it.
> > 
> > However I think agree that having upstream specific MC versions in VMSD
> > checks is kind of unwanted.  I think the major problem is we don't have
> > that extra machine type abstract where we can have simply a number showing
> > the release of QEMU, then we can map that number to whatever
> > upstream/downstream machine types.  E.g.:
> > 
> >   Release No.         Upstream version       Downstream version
> >   50                  9.0                    Y.0
> >   51                  9.1
> >   52                  9.2                    Y.1
> >   ...
> 
> Downstream versions do not map cleanly to individual upstream versions
> across the whole code base. If we have two distinct features in upstream
> version X, each of them may map to a different downstream release. 
> 
> This can happen when downstream skips one or more upstream releases.
> One feature from the skipped release might be backported to an earlier
> downstream release, while other feature might not arrive downstream
> until they later rebase. Version based checks are an inherantly
> undesirable idea for a situation where there is any backporting taking
> place, whether its machine type versions or something else. Named feature
> / flag based checks are always the way to go.

I thought this should work better with things like this where we only want
to fix a break in ABI, and none of downstream should special case things
like such fix.. but I agree even with that in mind such case could be so
rare to bother with above scheme.  I could have raised a bad idea I
suppose. :-( Let's stick with the simple until someone has better idea.

Thanks,

-- 
Peter Xu