[Qemu-devel] [PATCHv2 0/5] Last minute ppc migration fixes

[Qemu-devel] [PATCHv2 0/5] Last minute ppc migration fixes

[David Gibson]

Unfortunately, a bug already slipped into the qemu-2.7 release which
breaks migration of the pseries-2.6 machine type from qemu v2.6.0 to
qemu v2.7.0.  That bug also breaks migration from v2.6.0 to current
master.  In addition there have been several more changes which have
inadvertently broken migration from v2.7.0 to current master for both
pseries-2.6 and pseries-2.7 machine types.

One of those was fixed - in a way - by 9b54ca0, however it was done in
such a way as to break backwards migration from current master to
v2.7.0.  It turns out that supporting backwards migration is more
important than I realized at the time.

So, this series contains several fixes for migration.  Between them,
they allow migration both forwards and backwards between qemu v2.6.0
and current master (and therefore, I hope, v2.8.0).

Unfortunately, because of the already released bug, there's no
reasonable way to fix migration from current master back to v2.7.0 as
well, and, obviously, no way at all to fix the existing breakage
between v2.6.0 and v2.7.0.  However, I'm also hoping to get patch 1/5
of this series applied to the 2.7 stable branch.  If that happens, it
should be possible to freely migrate between v2.6.0, v2.7.1 and
current master / 2.8, which is about the best we can hope for.

I've done basic testing of these with all the combinations of source
and destination being either v2.6.0, v2.7.0 + patch 1/5 and
ppc-for-2.8 + this series, and with all the applicable machine types
for each of those combinations.  Each test was with just a basic
default hardware setup, running a RHEL7.3 guest, no fancier
combinations.

Dave, could you please sanity check my compatibility sheanningans and
send an R-b (and an acked-by for patch 2/5, which isn't strictly ppc
related).

Michael, if you could test this on any combinations that occur to you,
that would be most helpful.

I'm hoping to get some reviews and squeeze this into v2.8.0, which
will mean a pull request within a couple of days.

Changes since v1:
  * Removed some debug code from 1/5
  * Fixed some trivial style errors

David Gibson (5):
  target-ppc: Fix CPU migration from qemu-2.6 <-> later versions
  migration: Add VMSTATE_UINTTL_TEST()
  target-ppc: Allow eventual removal of old migration mistakes
  Revert "spapr: Fix migration of PCI host bridges from qemu-2.7"
  spapr: Fix 2.7<->2.8 migration of PCI host bridge

 hw/ppc/spapr.c              | 10 ++++++++++
 hw/ppc/spapr_pci.c          | 35 ++++++++++++++++++++++++++---------
 include/hw/pci-host/spapr.h |  6 ++++++
 include/migration/cpu.h     |  4 ++++
 target-ppc/cpu.h            |  7 +++++++
 target-ppc/machine.c        | 39 +++++++++++++++++++++++++++++++++++----
 target-ppc/translate_init.c |  6 ++++++
 7 files changed, 94 insertions(+), 13 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCHv2 1/5] target-ppc: Fix CPU migration from qemu-2.6 <-> later versions

[David Gibson]

When migration for target-ppc was converted to vmstate, several
VMSTATE_EQUAL() checks were foolishly included of things that really
should be internal state.  Specifically we verified equality of the
insns_flags and insns_flags2 fields, which are used within TCG to
determine which groups of instructions are available on this cpu
model.  Between qemu-2.6 and qemu-2.7 we made some changes to these
classes which broke migration.

This path fixes migration both forwards and backwards.  On migration
from 2.6 to later versions we import the fields into teporary
variables, which we then ignore.  In migration backwards, we populate
the temporary fields from the runtime fields, but mask out the bits
which were added after qemu-2.6, allowing the VMSTATE_EQUAL in
qemu-2.6 to accept the stream.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/cpu.h     |  6 ++++++
 target-ppc/machine.c | 29 +++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 1c90adb..7798b2e 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1166,6 +1166,12 @@ struct PowerPCCPU {
     int cpu_dt_id;
     uint32_t max_compat;
     uint32_t cpu_version;
+
+    /* fields used only during migration for compatibility hacks */
+    target_ulong mig_msr_mask;
+    uint64_t mig_insns_flags;
+    uint64_t mig_insns_flags2;
+    uint32_t mig_nb_BATs;
 };
 
 static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index e43cb6c..fcac263 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -140,6 +140,21 @@ static void cpu_pre_save(void *opaque)
     PowerPCCPU *cpu = opaque;
     CPUPPCState *env = &cpu->env;
     int i;
+    uint64_t insns_compat_mask =
+        PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB
+        | PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES
+        | PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | PPC_FLOAT_FRSQRTES
+        | PPC_FLOAT_STFIWX | PPC_FLOAT_EXT
+        | PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ
+        | PPC_MEM_SYNC | PPC_MEM_EIEIO | PPC_MEM_TLBIE | PPC_MEM_TLBSYNC
+        | PPC_64B | PPC_64BX | PPC_ALTIVEC
+        | PPC_SEGMENT_64B | PPC_SLBI | PPC_POPCNTB | PPC_POPCNTWD;
+    uint64_t insns_compat_mask2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX
+        | PPC2_PERM_ISA206 | PPC2_DIVE_ISA206
+        | PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206
+        | PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207
+        | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207
+        | PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 | PPC2_TM;
 
     env->spr[SPR_LR] = env->lr;
     env->spr[SPR_CTR] = env->ctr;
@@ -161,6 +176,12 @@ static void cpu_pre_save(void *opaque)
         env->spr[SPR_IBAT4U + 2*i] = env->IBAT[0][i+4];
         env->spr[SPR_IBAT4U + 2*i + 1] = env->IBAT[1][i+4];
     }
+
+    /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
+    cpu->mig_msr_mask = env->msr_mask;
+    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
+    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
+    cpu->mig_nb_BATs = env->nb_BATs;
 }
 
 static int cpu_post_load(void *opaque, int version_id)
@@ -561,10 +582,10 @@ const VMStateDescription vmstate_ppc_cpu = {
         /* FIXME: access_type? */
 
         /* Sanity checking */
-        VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
-        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
-        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
-        VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
+        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
+        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
+        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
+        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
-- 
2.7.4

[Qemu-devel] [PATCHv2 2/5] migration: Add VMSTATE_UINTTL_TEST()

[David Gibson]

include/migration/cpu.h defines VMSTATE_UINTTL() and several variants
for migrating target_ulong fields.  It's defined in terms of
VMSTATE_UINT32() or VMSTATE_UINT64() as appropriate.

It doesn't, however, include a VMSTATE_UINTTL_TEST() variant, which
I'm going to need shortly.  So, add it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 include/migration/cpu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/migration/cpu.h b/include/migration/cpu.h
index f3abbab..f3d5dfc 100644
--- a/include/migration/cpu.h
+++ b/include/migration/cpu.h
@@ -18,6 +18,8 @@
     VMSTATE_UINT64_EQUAL_V(_f, _s, _v)
 #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
     VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)
+#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
+    VMSTATE_UINT64_TEST(_f, _s, _t)
 #define vmstate_info_uinttl vmstate_info_uint64
 #else
 #define qemu_put_betl qemu_put_be32
@@ -35,6 +37,8 @@
     VMSTATE_UINT32_EQUAL_V(_f, _s, _v)
 #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
     VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
+#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
+    VMSTATE_UINT32_TEST(_f, _s, _t)
 #define vmstate_info_uinttl vmstate_info_uint32
 #endif
 
-- 
2.7.4

[Qemu-devel] [PATCHv2 3/5] target-ppc: Allow eventual removal of old migration mistakes

[David Gibson]

Until very recently, the vmstate for ppc cpus included some poorly
thought out VMSTATE_EQUAL() components, that can easily break
migration compatibility, and did so between qemu-2.6 and later
versions.  A hack was recently added which fixes this migration
breakage, but it leaves the unhelpful cruft of these fields in the
migration stream.

dThis patch adds a new cpu property allowing these fields to be
removed from the stream entirely.  This property is enabled by default
for the pseries-2.8 machine type - which comes after the fix - and for
all non-pseries machine types - which aren't mature enough to care
about cross-version migration.

The migration hack remains in place for pseries-2.7 and earlier
machine types, allowing backwards and forwards migration with the
older machine types.

This restricts the migration compatibility cruft to older machine
types, and at least opens the possibility of eventually deprecating
and removing it entirely.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              |  5 +++++
 target-ppc/cpu.h            |  3 ++-
 target-ppc/machine.c        | 26 ++++++++++++++++++--------
 target-ppc/translate_init.c |  6 ++++++
 4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 54b88d3..775ad2e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2767,6 +2767,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
         .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,     \
         .property = "mem64_win_size",               \
         .value    = "0",                            \
+    },                                              \
+    {                                               \
+        .driver = TYPE_POWERPC_CPU,                 \
+        .property = "pre-2.8-migration",            \
+        .value    = "on",                           \
     },
 
 static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 7798b2e..2a50c43 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1167,7 +1167,8 @@ struct PowerPCCPU {
     uint32_t max_compat;
     uint32_t cpu_version;
 
-    /* fields used only during migration for compatibility hacks */
+    /* Fields related to migration compatibility hacks */
+    bool pre_2_8_migration;
     target_ulong mig_msr_mask;
     uint64_t mig_insns_flags;
     uint64_t mig_insns_flags2;
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index fcac263..18c16d2 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -135,6 +135,13 @@ static const VMStateInfo vmstate_info_avr = {
 #define VMSTATE_AVR_ARRAY(_f, _s, _n)                             \
     VMSTATE_AVR_ARRAY_V(_f, _s, _n, 0)
 
+static bool cpu_pre_2_8_migration(void *opaque, int version_id)
+{
+    PowerPCCPU *cpu = opaque;
+
+    return cpu->pre_2_8_migration;
+}
+
 static void cpu_pre_save(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
@@ -178,10 +185,12 @@ static void cpu_pre_save(void *opaque)
     }
 
     /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
-    cpu->mig_msr_mask = env->msr_mask;
-    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
-    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
-    cpu->mig_nb_BATs = env->nb_BATs;
+    if (cpu->pre_2_8_migration) {
+        cpu->mig_msr_mask = env->msr_mask;
+        cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
+        cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
+        cpu->mig_nb_BATs = env->nb_BATs;
+    }
 }
 
 static int cpu_post_load(void *opaque, int version_id)
@@ -582,10 +591,11 @@ const VMStateDescription vmstate_ppc_cpu = {
         /* FIXME: access_type? */
 
         /* Sanity checking */
-        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
-        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
-        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
-        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
+        VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, cpu_pre_2_8_migration),
+        VMSTATE_UINT64_TEST(mig_insns_flags, PowerPCCPU, cpu_pre_2_8_migration),
+        VMSTATE_UINT64_TEST(mig_insns_flags2, PowerPCCPU,
+                            cpu_pre_2_8_migration),
+        VMSTATE_UINT32_TEST(mig_nb_BATs, PowerPCCPU, cpu_pre_2_8_migration),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 208fa1e..626e031 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10520,6 +10520,11 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
 #endif
 }
 
+static Property ppc_cpu_properties[] = {
+    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
@@ -10532,6 +10537,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
     dc->realize = ppc_cpu_realizefn;
     dc->unrealize = ppc_cpu_unrealizefn;
+    dc->props = ppc_cpu_properties;
 
     pcc->parent_reset = cc->reset;
     cc->reset = ppc_cpu_reset;
-- 
2.7.4

[Qemu-devel] [PATCHv2 4/5] Revert "spapr: Fix migration of PCI host bridges from qemu-2.7"

[David Gibson]

This reverts commit 9b54ca0ba781012eeea4237b7c4832ba2ea81d89.

The commit above corrected a migration breakage between qemu-2.7 and
qemu-2.8.  However it did so by advancing the migration version for
the PCI host bridge, which obviously breaks migration backwards to
earlier qemu versions.

Although it's not totally essential, we'd like to maintain the
possibility for backwards migration, so revert the change in
preparation for a better fix.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 661f7d8..e429c94 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1680,25 +1680,19 @@ static int spapr_pci_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static bool version_before_3(void *opaque, int version_id)
-{
-    return version_id < 3;
-}
-
 static const VMStateDescription vmstate_spapr_pci = {
     .name = "spapr_pci",
-    .version_id = 3,
+    .version_id = 2,
     .minimum_version_id = 2,
     .pre_save = spapr_pci_pre_save,
     .post_load = spapr_pci_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
-        VMSTATE_UNUSED_TEST(version_before_3,
-                            sizeof(uint32_t) /* dma_liobn[0] */
-                            + sizeof(uint64_t) /* mem_win_addr */
-                            + sizeof(uint64_t) /* mem_win_size */
-                            + sizeof(uint64_t) /* io_win_addr */
-                            + sizeof(uint64_t) /* io_win_size */),
+        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
+        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
+        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
+        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
+        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
         VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
                              vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
         VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
-- 
2.7.4

[Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[David Gibson]

daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
from qemu-2.7 to the current version.  It split the device's MMIO
window into two pieces for 32-bit and 64-bit MMIO.

The patch included backwards compatibility code to convert the old
property into the new format.  However, the property value was also
transferred in the migration stream and compared with a (probably
unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
the new style converted value from (pre-)2.8 giving a mismatch and
migration failure.

Along with the actual field that caused the breakage, there are
several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
migration, we read the values in the stream into scratch variables and
ignore them, instead of comparing for equality.  To fix backwards
migration, we populate those scratch variables in pre_save() with
adjusted values to match the old behaviour.

To permit the eventual possibility of removing this cruft from the
stream, we only include these compatibility fields if a new
'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
machine type, which obviously can't be migrated backwards, but set it
on earlier machine type versions.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              |  5 +++++
 hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
 include/hw/pci-host/spapr.h |  6 ++++++
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 775ad2e..c3269c7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
         .driver = TYPE_POWERPC_CPU,                 \
         .property = "pre-2.8-migration",            \
         .value    = "on",                           \
+    },                                              \
+    {                                               \
+        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
+        .property = "pre-2.8-migration",            \
+        .value    = "on",                           \
     },
 
 static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e429c94..c62c1cb 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
     DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
                        (1ULL << 12) | (1ULL << 16)),
     DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
+    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
+                     pre_2_8_migration, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
         sphb->msi_devs[i].key = *(uint32_t *) key;
         sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
     }
+
+    if (sphb->pre_2_8_migration) {
+        sphb->mig_liobn = sphb->dma_liobn[0];
+        sphb->mig_mem_win_addr = sphb->mem_win_addr;
+        sphb->mig_mem_win_size = sphb->mem_win_size;
+        sphb->mig_io_win_addr = sphb->io_win_addr;
+        sphb->mig_io_win_size = sphb->io_win_size;
+
+        if ((sphb->mem64_win_size != 0)
+            && (sphb->mem64_win_addr
+                == (sphb->mem_win_addr + sphb->mem_win_size))) {
+            sphb->mig_mem_win_size += sphb->mem64_win_size;
+        }
+    }
 }
 
 /*
@@ -1680,6 +1696,13 @@ static int spapr_pci_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool pre_2_8_migration(void *opaque, int version_id)
+{
+    sPAPRPHBState *sphb = opaque;
+
+    return sphb->pre_2_8_migration;
+}
+
 static const VMStateDescription vmstate_spapr_pci = {
     .name = "spapr_pci",
     .version_id = 2,
@@ -1688,11 +1711,11 @@ static const VMStateDescription vmstate_spapr_pci = {
     .post_load = spapr_pci_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
-        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
-        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
-        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
-        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
-        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
+        VMSTATE_UINT32_TEST(mig_liobn, sPAPRPHBState, pre_2_8_migration),
+        VMSTATE_UINT64_TEST(mig_mem_win_addr, sPAPRPHBState, pre_2_8_migration),
+        VMSTATE_UINT64_TEST(mig_mem_win_size, sPAPRPHBState, pre_2_8_migration),
+        VMSTATE_UINT64_TEST(mig_io_win_addr, sPAPRPHBState, pre_2_8_migration),
+        VMSTATE_UINT64_TEST(mig_io_win_size, sPAPRPHBState, pre_2_8_migration),
         VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
                              vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
         VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index b92c1b5..092294e 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -79,6 +79,12 @@ struct sPAPRPHBState {
     uint64_t dma64_win_addr;
 
     uint32_t numa_node;
+
+    /* Fields for migration compatibility hacks */
+    bool pre_2_8_migration;
+    uint32_t mig_liobn;
+    hwaddr mig_mem_win_addr, mig_mem_win_size;
+    hwaddr mig_io_win_addr, mig_io_win_size;
 };
 
 #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
-- 
2.7.4

Re: [Qemu-devel] [PATCHv2 2/5] migration: Add VMSTATE_UINTTL_TEST()

[Dr. David Alan Gilbert]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> include/migration/cpu.h defines VMSTATE_UINTTL() and several variants
> for migrating target_ulong fields.  It's defined in terms of
> VMSTATE_UINT32() or VMSTATE_UINT64() as appropriate.
> 
> It doesn't, however, include a VMSTATE_UINTTL_TEST() variant, which
> I'm going to need shortly.  So, add it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/migration/cpu.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/migration/cpu.h b/include/migration/cpu.h
> index f3abbab..f3d5dfc 100644
> --- a/include/migration/cpu.h
> +++ b/include/migration/cpu.h
> @@ -18,6 +18,8 @@
>      VMSTATE_UINT64_EQUAL_V(_f, _s, _v)
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)
> +#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
> +    VMSTATE_UINT64_TEST(_f, _s, _t)
>  #define vmstate_info_uinttl vmstate_info_uint64
>  #else
>  #define qemu_put_betl qemu_put_be32
> @@ -35,6 +37,8 @@
>      VMSTATE_UINT32_EQUAL_V(_f, _s, _v)
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
> +#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
> +    VMSTATE_UINT32_TEST(_f, _s, _t)
>  #define vmstate_info_uinttl vmstate_info_uint32
>  #endif
>  
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCHv2 1/5] target-ppc: Fix CPU migration from qemu-2.6 <-> later versions

[Dr. David Alan Gilbert]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> When migration for target-ppc was converted to vmstate, several
> VMSTATE_EQUAL() checks were foolishly included of things that really
> should be internal state.  Specifically we verified equality of the
> insns_flags and insns_flags2 fields, which are used within TCG to
> determine which groups of instructions are available on this cpu
> model.  Between qemu-2.6 and qemu-2.7 we made some changes to these
> classes which broke migration.
> 
> This path fixes migration both forwards and backwards.  On migration
> from 2.6 to later versions we import the fields into teporary
> variables, which we then ignore.  In migration backwards, we populate
> the temporary fields from the runtime fields, but mask out the bits
> which were added after qemu-2.6, allowing the VMSTATE_EQUAL in
> qemu-2.6 to accept the stream.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

From the migration side of things without knowing the ppc flags:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target-ppc/cpu.h     |  6 ++++++
>  target-ppc/machine.c | 29 +++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1c90adb..7798b2e 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1166,6 +1166,12 @@ struct PowerPCCPU {
>      int cpu_dt_id;
>      uint32_t max_compat;
>      uint32_t cpu_version;
> +
> +    /* fields used only during migration for compatibility hacks */
> +    target_ulong mig_msr_mask;
> +    uint64_t mig_insns_flags;
> +    uint64_t mig_insns_flags2;
> +    uint32_t mig_nb_BATs;
>  };
>  
>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index e43cb6c..fcac263 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -140,6 +140,21 @@ static void cpu_pre_save(void *opaque)
>      PowerPCCPU *cpu = opaque;
>      CPUPPCState *env = &cpu->env;
>      int i;
> +    uint64_t insns_compat_mask =
> +        PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB
> +        | PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES
> +        | PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | PPC_FLOAT_FRSQRTES
> +        | PPC_FLOAT_STFIWX | PPC_FLOAT_EXT
> +        | PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ
> +        | PPC_MEM_SYNC | PPC_MEM_EIEIO | PPC_MEM_TLBIE | PPC_MEM_TLBSYNC
> +        | PPC_64B | PPC_64BX | PPC_ALTIVEC
> +        | PPC_SEGMENT_64B | PPC_SLBI | PPC_POPCNTB | PPC_POPCNTWD;
> +    uint64_t insns_compat_mask2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX
> +        | PPC2_PERM_ISA206 | PPC2_DIVE_ISA206
> +        | PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206
> +        | PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207
> +        | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207
> +        | PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 | PPC2_TM;
>  
>      env->spr[SPR_LR] = env->lr;
>      env->spr[SPR_CTR] = env->ctr;
> @@ -161,6 +176,12 @@ static void cpu_pre_save(void *opaque)
>          env->spr[SPR_IBAT4U + 2*i] = env->IBAT[0][i+4];
>          env->spr[SPR_IBAT4U + 2*i + 1] = env->IBAT[1][i+4];
>      }
> +
> +    /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> +    cpu->mig_msr_mask = env->msr_mask;
> +    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +    cpu->mig_nb_BATs = env->nb_BATs;
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -561,10 +582,10 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> +        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCHv2 3/5] target-ppc: Allow eventual removal of old migration mistakes

[Dr. David Alan Gilbert]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> Until very recently, the vmstate for ppc cpus included some poorly
> thought out VMSTATE_EQUAL() components, that can easily break
> migration compatibility, and did so between qemu-2.6 and later
> versions.  A hack was recently added which fixes this migration
> breakage, but it leaves the unhelpful cruft of these fields in the
> migration stream.
> 
> dThis patch adds a new cpu property allowing these fields to be
> removed from the stream entirely.  This property is enabled by default
> for the pseries-2.8 machine type - which comes after the fix - and for
> all non-pseries machine types - which aren't mature enough to care
> about cross-version migration.
> 
> The migration hack remains in place for pseries-2.7 and earlier
> machine types, allowing backwards and forwards migration with the
> older machine types.
> 
> This restricts the migration compatibility cruft to older machine
> types, and at least opens the possibility of eventually deprecating
> and removing it entirely.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/ppc/spapr.c              |  5 +++++
>  target-ppc/cpu.h            |  3 ++-
>  target-ppc/machine.c        | 26 ++++++++++++++++++--------
>  target-ppc/translate_init.c |  6 ++++++
>  4 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 54b88d3..775ad2e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2767,6 +2767,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,     \
>          .property = "mem64_win_size",               \
>          .value    = "0",                            \
> +    },                                              \
> +    {                                               \
> +        .driver = TYPE_POWERPC_CPU,                 \
> +        .property = "pre-2.8-migration",            \
> +        .value    = "on",                           \
>      },
>  
>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 7798b2e..2a50c43 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1167,7 +1167,8 @@ struct PowerPCCPU {
>      uint32_t max_compat;
>      uint32_t cpu_version;
>  
> -    /* fields used only during migration for compatibility hacks */
> +    /* Fields related to migration compatibility hacks */
> +    bool pre_2_8_migration;
>      target_ulong mig_msr_mask;
>      uint64_t mig_insns_flags;
>      uint64_t mig_insns_flags2;
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index fcac263..18c16d2 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -135,6 +135,13 @@ static const VMStateInfo vmstate_info_avr = {
>  #define VMSTATE_AVR_ARRAY(_f, _s, _n)                             \
>      VMSTATE_AVR_ARRAY_V(_f, _s, _n, 0)
>  
> +static bool cpu_pre_2_8_migration(void *opaque, int version_id)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    return cpu->pre_2_8_migration;
> +}
> +
>  static void cpu_pre_save(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -178,10 +185,12 @@ static void cpu_pre_save(void *opaque)
>      }
>  
>      /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> -    cpu->mig_msr_mask = env->msr_mask;
> -    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> -    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> -    cpu->mig_nb_BATs = env->nb_BATs;
> +    if (cpu->pre_2_8_migration) {
> +        cpu->mig_msr_mask = env->msr_mask;
> +        cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +        cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +        cpu->mig_nb_BATs = env->nb_BATs;
> +    }
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -582,10 +591,11 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, cpu_pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_insns_flags, PowerPCCPU, cpu_pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_insns_flags2, PowerPCCPU,
> +                            cpu_pre_2_8_migration),
> +        VMSTATE_UINT32_TEST(mig_nb_BATs, PowerPCCPU, cpu_pre_2_8_migration),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 208fa1e..626e031 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10520,6 +10520,11 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static Property ppc_cpu_properties[] = {
> +    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10532,6 +10537,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
>      dc->realize = ppc_cpu_realizefn;
>      dc->unrealize = ppc_cpu_unrealizefn;
> +    dc->props = ppc_cpu_properties;
>  
>      pcc->parent_reset = cc->reset;
>      cc->reset = ppc_cpu_reset;
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCHv2 1/5] target-ppc: Fix CPU migration from qemu-2.6 <-> later versions

[Thomas Huth]

On 21.11.2016 06:31, David Gibson wrote:
> When migration for target-ppc was converted to vmstate, several
> VMSTATE_EQUAL() checks were foolishly included of things that really
> should be internal state.  Specifically we verified equality of the
> insns_flags and insns_flags2 fields, which are used within TCG to
> determine which groups of instructions are available on this cpu
> model.  Between qemu-2.6 and qemu-2.7 we made some changes to these
> classes which broke migration.
> 
> This path fixes migration both forwards and backwards.  On migration
> from 2.6 to later versions we import the fields into teporary
> variables, which we then ignore.  In migration backwards, we populate
> the temporary fields from the runtime fields, but mask out the bits
> which were added after qemu-2.6, allowing the VMSTATE_EQUAL in
> qemu-2.6 to accept the stream.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target-ppc/cpu.h     |  6 ++++++
>  target-ppc/machine.c | 29 +++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1c90adb..7798b2e 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1166,6 +1166,12 @@ struct PowerPCCPU {
>      int cpu_dt_id;
>      uint32_t max_compat;
>      uint32_t cpu_version;
> +
> +    /* fields used only during migration for compatibility hacks */
> +    target_ulong mig_msr_mask;
> +    uint64_t mig_insns_flags;
> +    uint64_t mig_insns_flags2;
> +    uint32_t mig_nb_BATs;
>  };
>  
>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index e43cb6c..fcac263 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -140,6 +140,21 @@ static void cpu_pre_save(void *opaque)
>      PowerPCCPU *cpu = opaque;
>      CPUPPCState *env = &cpu->env;
>      int i;
> +    uint64_t insns_compat_mask =
> +        PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB
> +        | PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES
> +        | PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | PPC_FLOAT_FRSQRTES
> +        | PPC_FLOAT_STFIWX | PPC_FLOAT_EXT
> +        | PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ
> +        | PPC_MEM_SYNC | PPC_MEM_EIEIO | PPC_MEM_TLBIE | PPC_MEM_TLBSYNC
> +        | PPC_64B | PPC_64BX | PPC_ALTIVEC
> +        | PPC_SEGMENT_64B | PPC_SLBI | PPC_POPCNTB | PPC_POPCNTWD;
> +    uint64_t insns_compat_mask2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX
> +        | PPC2_PERM_ISA206 | PPC2_DIVE_ISA206
> +        | PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206
> +        | PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207
> +        | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207
> +        | PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 | PPC2_TM;
>  
>      env->spr[SPR_LR] = env->lr;
>      env->spr[SPR_CTR] = env->ctr;
> @@ -161,6 +176,12 @@ static void cpu_pre_save(void *opaque)
>          env->spr[SPR_IBAT4U + 2*i] = env->IBAT[0][i+4];
>          env->spr[SPR_IBAT4U + 2*i + 1] = env->IBAT[1][i+4];
>      }
> +
> +    /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> +    cpu->mig_msr_mask = env->msr_mask;
> +    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +    cpu->mig_nb_BATs = env->nb_BATs;
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -561,10 +582,10 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> +        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> 

Looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv2 2/5] migration: Add VMSTATE_UINTTL_TEST()

[Thomas Huth]

On 21.11.2016 06:31, David Gibson wrote:
> include/migration/cpu.h defines VMSTATE_UINTTL() and several variants
> for migrating target_ulong fields.  It's defined in terms of
> VMSTATE_UINT32() or VMSTATE_UINT64() as appropriate.
> 
> It doesn't, however, include a VMSTATE_UINTTL_TEST() variant, which
> I'm going to need shortly.  So, add it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  include/migration/cpu.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/migration/cpu.h b/include/migration/cpu.h
> index f3abbab..f3d5dfc 100644
> --- a/include/migration/cpu.h
> +++ b/include/migration/cpu.h
> @@ -18,6 +18,8 @@
>      VMSTATE_UINT64_EQUAL_V(_f, _s, _v)
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)
> +#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
> +    VMSTATE_UINT64_TEST(_f, _s, _t)
>  #define vmstate_info_uinttl vmstate_info_uint64
>  #else
>  #define qemu_put_betl qemu_put_be32
> @@ -35,6 +37,8 @@
>      VMSTATE_UINT32_EQUAL_V(_f, _s, _v)
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
> +#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
> +    VMSTATE_UINT32_TEST(_f, _s, _t)
>  #define vmstate_info_uinttl vmstate_info_uint32
>  #endif
>  

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[Dr. David Alan Gilbert]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> from qemu-2.7 to the current version.  It split the device's MMIO
> window into two pieces for 32-bit and 64-bit MMIO.
> 
> The patch included backwards compatibility code to convert the old
> property into the new format.  However, the property value was also
> transferred in the migration stream and compared with a (probably
> unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> the new style converted value from (pre-)2.8 giving a mismatch and
> migration failure.
> 
> Along with the actual field that caused the breakage, there are
> several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> migration, we read the values in the stream into scratch variables and
> ignore them, instead of comparing for equality.  To fix backwards
> migration, we populate those scratch variables in pre_save() with
> adjusted values to match the old behaviour.
> 
> To permit the eventual possibility of removing this cruft from the
> stream, we only include these compatibility fields if a new
> 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> machine type, which obviously can't be migrated backwards, but set it
> on earlier machine type versions.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/ppc/spapr.c              |  5 +++++
>  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
>  include/hw/pci-host/spapr.h |  6 ++++++
>  3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 775ad2e..c3269c7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>          .driver = TYPE_POWERPC_CPU,                 \
>          .property = "pre-2.8-migration",            \
>          .value    = "on",                           \
> +    },                                              \
> +    {                                               \
> +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> +        .property = "pre-2.8-migration",            \
> +        .value    = "on",                           \
>      },
>  
>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e429c94..c62c1cb 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
>                         (1ULL << 12) | (1ULL << 16)),
>      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> +                     pre_2_8_migration, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
>          sphb->msi_devs[i].key = *(uint32_t *) key;
>          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
>      }
> +
> +    if (sphb->pre_2_8_migration) {
> +        sphb->mig_liobn = sphb->dma_liobn[0];
> +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> +        sphb->mig_mem_win_size = sphb->mem_win_size;
> +        sphb->mig_io_win_addr = sphb->io_win_addr;
> +        sphb->mig_io_win_size = sphb->io_win_size;
> +
> +        if ((sphb->mem64_win_size != 0)
> +            && (sphb->mem64_win_addr
> +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> +        }
> +    }
>  }
>  
>  /*
> @@ -1680,6 +1696,13 @@ static int spapr_pci_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool pre_2_8_migration(void *opaque, int version_id)
> +{
> +    sPAPRPHBState *sphb = opaque;
> +
> +    return sphb->pre_2_8_migration;
> +}
> +
>  static const VMStateDescription vmstate_spapr_pci = {
>      .name = "spapr_pci",
>      .version_id = 2,
> @@ -1688,11 +1711,11 @@ static const VMStateDescription vmstate_spapr_pci = {
>      .post_load = spapr_pci_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> -        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
> +        VMSTATE_UINT32_TEST(mig_liobn, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_mem_win_addr, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_mem_win_size, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_io_win_addr, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_io_win_size, sPAPRPHBState, pre_2_8_migration),
>          VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
>                               vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
>          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index b92c1b5..092294e 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -79,6 +79,12 @@ struct sPAPRPHBState {
>      uint64_t dma64_win_addr;
>  
>      uint32_t numa_node;
> +
> +    /* Fields for migration compatibility hacks */
> +    bool pre_2_8_migration;
> +    uint32_t mig_liobn;
> +    hwaddr mig_mem_win_addr, mig_mem_win_size;
> +    hwaddr mig_io_win_addr, mig_io_win_size;
>  };
>  
>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCHv2 3/5] target-ppc: Allow eventual removal of old migration mistakes

[Thomas Huth]

On 21.11.2016 06:31, David Gibson wrote:
> Until very recently, the vmstate for ppc cpus included some poorly
> thought out VMSTATE_EQUAL() components, that can easily break
> migration compatibility, and did so between qemu-2.6 and later
> versions.  A hack was recently added which fixes this migration
> breakage, but it leaves the unhelpful cruft of these fields in the
> migration stream.
> 
> dThis patch adds a new cpu property allowing these fields to be
> removed from the stream entirely.  This property is enabled by default
> for the pseries-2.8 machine type - which comes after the fix - and for
> all non-pseries machine types - which aren't mature enough to care
> about cross-version migration.
> 
> The migration hack remains in place for pseries-2.7 and earlier
> machine types, allowing backwards and forwards migration with the
> older machine types.
> 
> This restricts the migration compatibility cruft to older machine
> types, and at least opens the possibility of eventually deprecating
> and removing it entirely.

I like that idea! Patch looks fine, too, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv2 4/5] Revert "spapr: Fix migration of PCI host bridges from qemu-2.7"

[Thomas Huth]

On 21.11.2016 06:31, David Gibson wrote:
> This reverts commit 9b54ca0ba781012eeea4237b7c4832ba2ea81d89.
> 
> The commit above corrected a migration breakage between qemu-2.7 and
> qemu-2.8.  However it did so by advancing the migration version for
> the PCI host bridge, which obviously breaks migration backwards to
> earlier qemu versions.
> 
> Although it's not totally essential, we'd like to maintain the
> possibility for backwards migration, so revert the change in
> preparation for a better fix.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 661f7d8..e429c94 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1680,25 +1680,19 @@ static int spapr_pci_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static bool version_before_3(void *opaque, int version_id)
> -{
> -    return version_id < 3;
> -}
> -
>  static const VMStateDescription vmstate_spapr_pci = {
>      .name = "spapr_pci",
> -    .version_id = 3,
> +    .version_id = 2,
>      .minimum_version_id = 2,
>      .pre_save = spapr_pci_pre_save,
>      .post_load = spapr_pci_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> -        VMSTATE_UNUSED_TEST(version_before_3,
> -                            sizeof(uint32_t) /* dma_liobn[0] */
> -                            + sizeof(uint64_t) /* mem_win_addr */
> -                            + sizeof(uint64_t) /* mem_win_size */
> -                            + sizeof(uint64_t) /* io_win_addr */
> -                            + sizeof(uint64_t) /* io_win_size */),
> +        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
>          VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
>                               vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
>          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[Thomas Huth]

On 21.11.2016 06:31, David Gibson wrote:
> daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> from qemu-2.7 to the current version.  It split the device's MMIO
> window into two pieces for 32-bit and 64-bit MMIO.
> 
> The patch included backwards compatibility code to convert the old
> property into the new format.  However, the property value was also
> transferred in the migration stream and compared with a (probably
> unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> the new style converted value from (pre-)2.8 giving a mismatch and
> migration failure.
> 
> Along with the actual field that caused the breakage, there are
> several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> migration, we read the values in the stream into scratch variables and
> ignore them, instead of comparing for equality.  To fix backwards
> migration, we populate those scratch variables in pre_save() with
> adjusted values to match the old behaviour.
> 
> To permit the eventual possibility of removing this cruft from the
> stream, we only include these compatibility fields if a new
> 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> machine type, which obviously can't be migrated backwards, but set it
> on earlier machine type versions.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c              |  5 +++++
>  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
>  include/hw/pci-host/spapr.h |  6 ++++++
>  3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 775ad2e..c3269c7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>          .driver = TYPE_POWERPC_CPU,                 \
>          .property = "pre-2.8-migration",            \
>          .value    = "on",                           \
> +    },                                              \
> +    {                                               \
> +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> +        .property = "pre-2.8-migration",            \
> +        .value    = "on",                           \
>      },
>  
>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e429c94..c62c1cb 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
>                         (1ULL << 12) | (1ULL << 16)),
>      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> +                     pre_2_8_migration, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
>          sphb->msi_devs[i].key = *(uint32_t *) key;
>          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
>      }
> +
> +    if (sphb->pre_2_8_migration) {
> +        sphb->mig_liobn = sphb->dma_liobn[0];
> +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> +        sphb->mig_mem_win_size = sphb->mem_win_size;
> +        sphb->mig_io_win_addr = sphb->io_win_addr;
> +        sphb->mig_io_win_size = sphb->io_win_size;
> +
> +        if ((sphb->mem64_win_size != 0)
> +            && (sphb->mem64_win_addr
> +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> +        }

Should we maybe print a warning/error message in case

 sphb->mem64_win_size != 0 &&
 sphb->mem64_win_addr != sphb->mem_win_addr + sphb->mem_win_size

... assuming that this means a configuration which can not be migrated
backwards?

> +    }
>  }

With or without warning message, patch looks fine to me, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 1/5] target-ppc: Fix CPU migration from qemu-2.6 <-> later versions

[Greg Kurz]

On Mon, 21 Nov 2016 16:31:38 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> When migration for target-ppc was converted to vmstate, several
> VMSTATE_EQUAL() checks were foolishly included of things that really
> should be internal state.  Specifically we verified equality of the
> insns_flags and insns_flags2 fields, which are used within TCG to
> determine which groups of instructions are available on this cpu
> model.  Between qemu-2.6 and qemu-2.7 we made some changes to these
> classes which broke migration.
> 
> This path fixes migration both forwards and backwards.  On migration
> from 2.6 to later versions we import the fields into teporary
> variables, which we then ignore.  In migration backwards, we populate
> the temporary fields from the runtime fields, but mask out the bits
> which were added after qemu-2.6, allowing the VMSTATE_EQUAL in
> qemu-2.6 to accept the stream.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  target-ppc/cpu.h     |  6 ++++++
>  target-ppc/machine.c | 29 +++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1c90adb..7798b2e 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1166,6 +1166,12 @@ struct PowerPCCPU {
>      int cpu_dt_id;
>      uint32_t max_compat;
>      uint32_t cpu_version;
> +
> +    /* fields used only during migration for compatibility hacks */
> +    target_ulong mig_msr_mask;
> +    uint64_t mig_insns_flags;
> +    uint64_t mig_insns_flags2;
> +    uint32_t mig_nb_BATs;
>  };
>  
>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index e43cb6c..fcac263 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -140,6 +140,21 @@ static void cpu_pre_save(void *opaque)
>      PowerPCCPU *cpu = opaque;
>      CPUPPCState *env = &cpu->env;
>      int i;
> +    uint64_t insns_compat_mask =
> +        PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB
> +        | PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES
> +        | PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | PPC_FLOAT_FRSQRTES
> +        | PPC_FLOAT_STFIWX | PPC_FLOAT_EXT
> +        | PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ
> +        | PPC_MEM_SYNC | PPC_MEM_EIEIO | PPC_MEM_TLBIE | PPC_MEM_TLBSYNC
> +        | PPC_64B | PPC_64BX | PPC_ALTIVEC
> +        | PPC_SEGMENT_64B | PPC_SLBI | PPC_POPCNTB | PPC_POPCNTWD;
> +    uint64_t insns_compat_mask2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX
> +        | PPC2_PERM_ISA206 | PPC2_DIVE_ISA206
> +        | PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206
> +        | PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207
> +        | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207
> +        | PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 | PPC2_TM;
>  
>      env->spr[SPR_LR] = env->lr;
>      env->spr[SPR_CTR] = env->ctr;
> @@ -161,6 +176,12 @@ static void cpu_pre_save(void *opaque)
>          env->spr[SPR_IBAT4U + 2*i] = env->IBAT[0][i+4];
>          env->spr[SPR_IBAT4U + 2*i + 1] = env->IBAT[1][i+4];
>      }
> +
> +    /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> +    cpu->mig_msr_mask = env->msr_mask;
> +    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +    cpu->mig_nb_BATs = env->nb_BATs;
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -561,10 +582,10 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> +        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 2/5] migration: Add VMSTATE_UINTTL_TEST()

[Greg Kurz]

On Mon, 21 Nov 2016 16:31:39 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> include/migration/cpu.h defines VMSTATE_UINTTL() and several variants
> for migrating target_ulong fields.  It's defined in terms of
> VMSTATE_UINT32() or VMSTATE_UINT64() as appropriate.
> 
> It doesn't, however, include a VMSTATE_UINTTL_TEST() variant, which
> I'm going to need shortly.  So, add it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/migration/cpu.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/migration/cpu.h b/include/migration/cpu.h
> index f3abbab..f3d5dfc 100644
> --- a/include/migration/cpu.h
> +++ b/include/migration/cpu.h
> @@ -18,6 +18,8 @@
>      VMSTATE_UINT64_EQUAL_V(_f, _s, _v)
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)
> +#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
> +    VMSTATE_UINT64_TEST(_f, _s, _t)
>  #define vmstate_info_uinttl vmstate_info_uint64
>  #else
>  #define qemu_put_betl qemu_put_be32
> @@ -35,6 +37,8 @@
>      VMSTATE_UINT32_EQUAL_V(_f, _s, _v)
>  #define VMSTATE_UINTTL_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)
> +#define VMSTATE_UINTTL_TEST(_f, _s, _t)                               \
> +    VMSTATE_UINT32_TEST(_f, _s, _t)
>  #define vmstate_info_uinttl vmstate_info_uint32
>  #endif
>  

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 3/5] target-ppc: Allow eventual removal of old migration mistakes

[Greg Kurz]

On Mon, 21 Nov 2016 16:31:40 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Until very recently, the vmstate for ppc cpus included some poorly
> thought out VMSTATE_EQUAL() components, that can easily break
> migration compatibility, and did so between qemu-2.6 and later
> versions.  A hack was recently added which fixes this migration
> breakage, but it leaves the unhelpful cruft of these fields in the
> migration stream.
> 
> dThis patch adds a new cpu property allowing these fields to be
  ^
typo

> removed from the stream entirely.  This property is enabled by default
> for the pseries-2.8 machine type - which comes after the fix - and for
> all non-pseries machine types - which aren't mature enough to care
> about cross-version migration.
> 

It is a bit confusing (at least for me) that "property is enabled" means it is
actually set to off. No big deal though.

> The migration hack remains in place for pseries-2.7 and earlier
> machine types, allowing backwards and forwards migration with the
> older machine types.
> 
> This restricts the migration compatibility cruft to older machine
> types, and at least opens the possibility of eventually deprecating
> and removing it entirely.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c              |  5 +++++
>  target-ppc/cpu.h            |  3 ++-
>  target-ppc/machine.c        | 26 ++++++++++++++++++--------
>  target-ppc/translate_init.c |  6 ++++++
>  4 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 54b88d3..775ad2e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2767,6 +2767,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,     \
>          .property = "mem64_win_size",               \
>          .value    = "0",                            \
> +    },                                              \
> +    {                                               \
> +        .driver = TYPE_POWERPC_CPU,                 \
> +        .property = "pre-2.8-migration",            \
> +        .value    = "on",                           \
>      },
>  
>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 7798b2e..2a50c43 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1167,7 +1167,8 @@ struct PowerPCCPU {
>      uint32_t max_compat;
>      uint32_t cpu_version;
>  
> -    /* fields used only during migration for compatibility hacks */
> +    /* Fields related to migration compatibility hacks */
> +    bool pre_2_8_migration;
>      target_ulong mig_msr_mask;
>      uint64_t mig_insns_flags;
>      uint64_t mig_insns_flags2;
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index fcac263..18c16d2 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -135,6 +135,13 @@ static const VMStateInfo vmstate_info_avr = {
>  #define VMSTATE_AVR_ARRAY(_f, _s, _n)                             \
>      VMSTATE_AVR_ARRAY_V(_f, _s, _n, 0)
>  
> +static bool cpu_pre_2_8_migration(void *opaque, int version_id)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    return cpu->pre_2_8_migration;
> +}
> +
>  static void cpu_pre_save(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -178,10 +185,12 @@ static void cpu_pre_save(void *opaque)
>      }
>  
>      /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> -    cpu->mig_msr_mask = env->msr_mask;
> -    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> -    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> -    cpu->mig_nb_BATs = env->nb_BATs;
> +    if (cpu->pre_2_8_migration) {
> +        cpu->mig_msr_mask = env->msr_mask;
> +        cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +        cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +        cpu->mig_nb_BATs = env->nb_BATs;
> +    }
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -582,10 +591,11 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, cpu_pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_insns_flags, PowerPCCPU, cpu_pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_insns_flags2, PowerPCCPU,
> +                            cpu_pre_2_8_migration),
> +        VMSTATE_UINT32_TEST(mig_nb_BATs, PowerPCCPU, cpu_pre_2_8_migration),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 208fa1e..626e031 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10520,6 +10520,11 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static Property ppc_cpu_properties[] = {
> +    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10532,6 +10537,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
>      dc->realize = ppc_cpu_realizefn;
>      dc->unrealize = ppc_cpu_unrealizefn;
> +    dc->props = ppc_cpu_properties;
>  
>      pcc->parent_reset = cc->reset;
>      cc->reset = ppc_cpu_reset;

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 4/5] Revert "spapr: Fix migration of PCI host bridges from qemu-2.7"

[Greg Kurz]

On Mon, 21 Nov 2016 16:31:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This reverts commit 9b54ca0ba781012eeea4237b7c4832ba2ea81d89.
> 
> The commit above corrected a migration breakage between qemu-2.7 and
> qemu-2.8.  However it did so by advancing the migration version for
> the PCI host bridge, which obviously breaks migration backwards to
> earlier qemu versions.
> 
> Although it's not totally essential, we'd like to maintain the
> possibility for backwards migration, so revert the change in
> preparation for a better fix.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_pci.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 661f7d8..e429c94 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1680,25 +1680,19 @@ static int spapr_pci_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static bool version_before_3(void *opaque, int version_id)
> -{
> -    return version_id < 3;
> -}
> -
>  static const VMStateDescription vmstate_spapr_pci = {
>      .name = "spapr_pci",
> -    .version_id = 3,
> +    .version_id = 2,
>      .minimum_version_id = 2,
>      .pre_save = spapr_pci_pre_save,
>      .post_load = spapr_pci_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> -        VMSTATE_UNUSED_TEST(version_before_3,
> -                            sizeof(uint32_t) /* dma_liobn[0] */
> -                            + sizeof(uint64_t) /* mem_win_addr */
> -                            + sizeof(uint64_t) /* mem_win_size */
> -                            + sizeof(uint64_t) /* io_win_addr */
> -                            + sizeof(uint64_t) /* io_win_size */),
> +        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
>          VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
>                               vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
>          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[Greg Kurz]

On Mon, 21 Nov 2016 13:02:53 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 21.11.2016 06:31, David Gibson wrote:
> > daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> > from qemu-2.7 to the current version.  It split the device's MMIO
> > window into two pieces for 32-bit and 64-bit MMIO.
> > 
> > The patch included backwards compatibility code to convert the old
> > property into the new format.  However, the property value was also
> > transferred in the migration stream and compared with a (probably
> > unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> > the new style converted value from (pre-)2.8 giving a mismatch and
> > migration failure.
> > 
> > Along with the actual field that caused the breakage, there are
> > several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> > migration, we read the values in the stream into scratch variables and
> > ignore them, instead of comparing for equality.  To fix backwards
> > migration, we populate those scratch variables in pre_save() with
> > adjusted values to match the old behaviour.
> > 
> > To permit the eventual possibility of removing this cruft from the
> > stream, we only include these compatibility fields if a new
> > 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> > machine type, which obviously can't be migrated backwards, but set it
> > on earlier machine type versions.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c              |  5 +++++
> >  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
> >  include/hw/pci-host/spapr.h |  6 ++++++
> >  3 files changed, 39 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 775ad2e..c3269c7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> >          .driver = TYPE_POWERPC_CPU,                 \
> >          .property = "pre-2.8-migration",            \
> >          .value    = "on",                           \
> > +    },                                              \
> > +    {                                               \
> > +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> > +        .property = "pre-2.8-migration",            \
> > +        .value    = "on",                           \
> >      },
> >  
> >  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index e429c94..c62c1cb 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
> >      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
> >                         (1ULL << 12) | (1ULL << 16)),
> >      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> > +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> > +                     pre_2_8_migration, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
> >          sphb->msi_devs[i].key = *(uint32_t *) key;
> >          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> >      }
> > +
> > +    if (sphb->pre_2_8_migration) {
> > +        sphb->mig_liobn = sphb->dma_liobn[0];
> > +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> > +        sphb->mig_mem_win_size = sphb->mem_win_size;
> > +        sphb->mig_io_win_addr = sphb->io_win_addr;
> > +        sphb->mig_io_win_size = sphb->io_win_size;
> > +
> > +        if ((sphb->mem64_win_size != 0)
> > +            && (sphb->mem64_win_addr
> > +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> > +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> > +        }  
> 
> Should we maybe print a warning/error message in case
> 
>  sphb->mem64_win_size != 0 &&
>  sphb->mem64_win_addr != sphb->mem_win_addr + sphb->mem_win_size
> 
> ... assuming that this means a configuration which can not be migrated
> backwards?
> 

Then shouldn't we forbid pre_2_8_migration to be set when we have a
non-contiguous window ?

> > +    }
> >  }  
> 
> With or without warning message, patch looks fine to me, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 3/5] target-ppc: Allow eventual removal of old migration mistakes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6592 bytes --]

On Mon, Nov 21, 2016 at 04:26:10PM +0100, Greg Kurz wrote:
> On Mon, 21 Nov 2016 16:31:40 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Until very recently, the vmstate for ppc cpus included some poorly
> > thought out VMSTATE_EQUAL() components, that can easily break
> > migration compatibility, and did so between qemu-2.6 and later
> > versions.  A hack was recently added which fixes this migration
> > breakage, but it leaves the unhelpful cruft of these fields in the
> > migration stream.
> > 
> > dThis patch adds a new cpu property allowing these fields to be
>   ^
> typo

Oops, fixed.

> > removed from the stream entirely.  This property is enabled by default
> > for the pseries-2.8 machine type - which comes after the fix - and for
> > all non-pseries machine types - which aren't mature enough to care
> > about cross-version migration.
> > 
> 
> It is a bit confusing (at least for me) that "property is enabled" means it is
> actually set to off. No big deal though.

Uh, yes, I've reworded this a bit.

> 
> > The migration hack remains in place for pseries-2.7 and earlier
> > machine types, allowing backwards and forwards migration with the
> > older machine types.
> > 
> > This restricts the migration compatibility cruft to older machine
> > types, and at least opens the possibility of eventually deprecating
> > and removing it entirely.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >  hw/ppc/spapr.c              |  5 +++++
> >  target-ppc/cpu.h            |  3 ++-
> >  target-ppc/machine.c        | 26 ++++++++++++++++++--------
> >  target-ppc/translate_init.c |  6 ++++++
> >  4 files changed, 31 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 54b88d3..775ad2e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2767,6 +2767,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> >          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,     \
> >          .property = "mem64_win_size",               \
> >          .value    = "0",                            \
> > +    },                                              \
> > +    {                                               \
> > +        .driver = TYPE_POWERPC_CPU,                 \
> > +        .property = "pre-2.8-migration",            \
> > +        .value    = "on",                           \
> >      },
> >  
> >  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > index 7798b2e..2a50c43 100644
> > --- a/target-ppc/cpu.h
> > +++ b/target-ppc/cpu.h
> > @@ -1167,7 +1167,8 @@ struct PowerPCCPU {
> >      uint32_t max_compat;
> >      uint32_t cpu_version;
> >  
> > -    /* fields used only during migration for compatibility hacks */
> > +    /* Fields related to migration compatibility hacks */
> > +    bool pre_2_8_migration;
> >      target_ulong mig_msr_mask;
> >      uint64_t mig_insns_flags;
> >      uint64_t mig_insns_flags2;
> > diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> > index fcac263..18c16d2 100644
> > --- a/target-ppc/machine.c
> > +++ b/target-ppc/machine.c
> > @@ -135,6 +135,13 @@ static const VMStateInfo vmstate_info_avr = {
> >  #define VMSTATE_AVR_ARRAY(_f, _s, _n)                             \
> >      VMSTATE_AVR_ARRAY_V(_f, _s, _n, 0)
> >  
> > +static bool cpu_pre_2_8_migration(void *opaque, int version_id)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +
> > +    return cpu->pre_2_8_migration;
> > +}
> > +
> >  static void cpu_pre_save(void *opaque)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > @@ -178,10 +185,12 @@ static void cpu_pre_save(void *opaque)
> >      }
> >  
> >      /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> > -    cpu->mig_msr_mask = env->msr_mask;
> > -    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> > -    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> > -    cpu->mig_nb_BATs = env->nb_BATs;
> > +    if (cpu->pre_2_8_migration) {
> > +        cpu->mig_msr_mask = env->msr_mask;
> > +        cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> > +        cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> > +        cpu->mig_nb_BATs = env->nb_BATs;
> > +    }
> >  }
> >  
> >  static int cpu_post_load(void *opaque, int version_id)
> > @@ -582,10 +591,11 @@ const VMStateDescription vmstate_ppc_cpu = {
> >          /* FIXME: access_type? */
> >  
> >          /* Sanity checking */
> > -        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> > -        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> > -        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> > -        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
> > +        VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, cpu_pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_insns_flags, PowerPCCPU, cpu_pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_insns_flags2, PowerPCCPU,
> > +                            cpu_pre_2_8_migration),
> > +        VMSTATE_UINT32_TEST(mig_nb_BATs, PowerPCCPU, cpu_pre_2_8_migration),
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription*[]) {
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 208fa1e..626e031 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -10520,6 +10520,11 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
> >  #endif
> >  }
> >  
> > +static Property ppc_cpu_properties[] = {
> > +    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >  {
> >      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> > @@ -10532,6 +10537,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
> >      dc->realize = ppc_cpu_realizefn;
> >      dc->unrealize = ppc_cpu_unrealizefn;
> > +    dc->props = ppc_cpu_properties;
> >  
> >      pcc->parent_reset = cc->reset;
> >      cc->reset = ppc_cpu_reset;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5339 bytes --]

On Mon, Nov 21, 2016 at 05:02:20PM +0100, Greg Kurz wrote:
> On Mon, 21 Nov 2016 13:02:53 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 21.11.2016 06:31, David Gibson wrote:
> > > daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> > > from qemu-2.7 to the current version.  It split the device's MMIO
> > > window into two pieces for 32-bit and 64-bit MMIO.
> > > 
> > > The patch included backwards compatibility code to convert the old
> > > property into the new format.  However, the property value was also
> > > transferred in the migration stream and compared with a (probably
> > > unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> > > the new style converted value from (pre-)2.8 giving a mismatch and
> > > migration failure.
> > > 
> > > Along with the actual field that caused the breakage, there are
> > > several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> > > migration, we read the values in the stream into scratch variables and
> > > ignore them, instead of comparing for equality.  To fix backwards
> > > migration, we populate those scratch variables in pre_save() with
> > > adjusted values to match the old behaviour.
> > > 
> > > To permit the eventual possibility of removing this cruft from the
> > > stream, we only include these compatibility fields if a new
> > > 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> > > machine type, which obviously can't be migrated backwards, but set it
> > > on earlier machine type versions.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr.c              |  5 +++++
> > >  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
> > >  include/hw/pci-host/spapr.h |  6 ++++++
> > >  3 files changed, 39 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 775ad2e..c3269c7 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> > >          .driver = TYPE_POWERPC_CPU,                 \
> > >          .property = "pre-2.8-migration",            \
> > >          .value    = "on",                           \
> > > +    },                                              \
> > > +    {                                               \
> > > +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> > > +        .property = "pre-2.8-migration",            \
> > > +        .value    = "on",                           \
> > >      },
> > >  
> > >  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index e429c94..c62c1cb 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
> > >      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
> > >                         (1ULL << 12) | (1ULL << 16)),
> > >      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> > > +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> > > +                     pre_2_8_migration, false),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
> > >          sphb->msi_devs[i].key = *(uint32_t *) key;
> > >          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> > >      }
> > > +
> > > +    if (sphb->pre_2_8_migration) {
> > > +        sphb->mig_liobn = sphb->dma_liobn[0];
> > > +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> > > +        sphb->mig_mem_win_size = sphb->mem_win_size;
> > > +        sphb->mig_io_win_addr = sphb->io_win_addr;
> > > +        sphb->mig_io_win_size = sphb->io_win_size;
> > > +
> > > +        if ((sphb->mem64_win_size != 0)
> > > +            && (sphb->mem64_win_addr
> > > +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> > > +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> > > +        }  
> > 
> > Should we maybe print a warning/error message in case
> > 
> >  sphb->mem64_win_size != 0 &&
> >  sphb->mem64_win_addr != sphb->mem_win_addr + sphb->mem_win_size
> > 
> > ... assuming that this means a configuration which can not be migrated
> > backwards?
> > 
> 
> Then shouldn't we forbid pre_2_8_migration to be set when we have a
> non-contiguous window ?

So, yes, we could do either of these, but really I don't think it's
worth it.  It will only happen if you have a custom constructed PHB,
in which case it's pretty much on you to ensure that there's something
compatible at the far end.  Restricting it here has somewhat the same
problem as VMSTATE_EQUAL()s did - they make assumptions about what is
and isn't sane which could be broken by future changes (in this case
changes in the 2.7 stable tree).  They might be unlikely to change,
but if they do things break, and the only benefit is a marginally
better error message in cases that won't work anyway.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[Alexey Kardashevskiy]

On 21/11/16 16:31, David Gibson wrote:
> daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> from qemu-2.7 to the current version.  It split the device's MMIO
> window into two pieces for 32-bit and 64-bit MMIO.
> 
> The patch included backwards compatibility code to convert the old
> property into the new format.  However, the property value was also
> transferred in the migration stream and compared with a (probably
> unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> the new style converted value from (pre-)2.8 giving a mismatch and
> migration failure.
> 
> Along with the actual field that caused the breakage, there are
> several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> migration, we read the values in the stream into scratch variables and
> ignore them, instead of comparing for equality.  To fix backwards
> migration, we populate those scratch variables in pre_save() with
> adjusted values to match the old behaviour.
> 
> To permit the eventual possibility of removing this cruft from the
> stream, we only include these compatibility fields if a new
> 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> machine type, which obviously can't be migrated backwards, but set it
> on earlier machine type versions.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c              |  5 +++++
>  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
>  include/hw/pci-host/spapr.h |  6 ++++++
>  3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 775ad2e..c3269c7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>          .driver = TYPE_POWERPC_CPU,                 \
>          .property = "pre-2.8-migration",            \
>          .value    = "on",                           \
> +    },                                              \
> +    {                                               \
> +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> +        .property = "pre-2.8-migration",            \
> +        .value    = "on",                           \
>      },
>  
>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e429c94..c62c1cb 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
>                         (1ULL << 12) | (1ULL << 16)),
>      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> +                     pre_2_8_migration, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
>          sphb->msi_devs[i].key = *(uint32_t *) key;
>          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
>      }
> +
> +    if (sphb->pre_2_8_migration) {


You check for pre_2_8_migration here but you do not in 3/5, what is the
difference?

Also this chunk did not apply on top of dwg/master or qemu.org/master (some
whitespace issue? not sure) so I tested dwg/ppc-for-2.8, it works.


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> +        sphb->mig_liobn = sphb->dma_liobn[0];
> +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> +        sphb->mig_mem_win_size = sphb->mem_win_size;
> +        sphb->mig_io_win_addr = sphb->io_win_addr;
> +        sphb->mig_io_win_size = sphb->io_win_size;
> +
> +        if ((sphb->mem64_win_size != 0)
> +            && (sphb->mem64_win_addr
> +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> +        }
> +    }
>  }
>  
>  /*
> @@ -1680,6 +1696,13 @@ static int spapr_pci_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool pre_2_8_migration(void *opaque, int version_id)
> +{
> +    sPAPRPHBState *sphb = opaque;
> +
> +    return sphb->pre_2_8_migration;
> +}
> +
>  static const VMStateDescription vmstate_spapr_pci = {
>      .name = "spapr_pci",
>      .version_id = 2,
> @@ -1688,11 +1711,11 @@ static const VMStateDescription vmstate_spapr_pci = {
>      .post_load = spapr_pci_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> -        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> -        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
> +        VMSTATE_UINT32_TEST(mig_liobn, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_mem_win_addr, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_mem_win_size, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_io_win_addr, sPAPRPHBState, pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_io_win_size, sPAPRPHBState, pre_2_8_migration),
>          VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
>                               vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
>          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index b92c1b5..092294e 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -79,6 +79,12 @@ struct sPAPRPHBState {
>      uint64_t dma64_win_addr;
>  
>      uint32_t numa_node;
> +
> +    /* Fields for migration compatibility hacks */
> +    bool pre_2_8_migration;
> +    uint32_t mig_liobn;
> +    hwaddr mig_mem_win_addr, mig_mem_win_size;
> +    hwaddr mig_io_win_addr, mig_io_win_size;
>  };
>  
>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> 


-- 
Alexey

Re: [Qemu-devel] [PATCHv2 1/5] target-ppc: Fix CPU migration from qemu-2.6 <-> later versions

[Alexey Kardashevskiy]

On 21/11/16 16:31, David Gibson wrote:
> When migration for target-ppc was converted to vmstate, several
> VMSTATE_EQUAL() checks were foolishly included of things that really
> should be internal state.  Specifically we verified equality of the
> insns_flags and insns_flags2 fields, which are used within TCG to
> determine which groups of instructions are available on this cpu
> model.  Between qemu-2.6 and qemu-2.7 we made some changes to these
> classes which broke migration.
> 
> This path fixes migration both forwards and backwards.  On migration
> from 2.6 to later versions we import the fields into teporary
> variables, which we then ignore.  In migration backwards, we populate
> the temporary fields from the runtime fields, but mask out the bits
> which were added after qemu-2.6, allowing the VMSTATE_EQUAL in
> qemu-2.6 to accept the stream.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

One question though - do we care about TCG migration at all? If so, we
could want to migrate these bits, just not with _EQUAL but rather check
that the source does not have bits which are not supported by the destination.



> ---
>  target-ppc/cpu.h     |  6 ++++++
>  target-ppc/machine.c | 29 +++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1c90adb..7798b2e 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1166,6 +1166,12 @@ struct PowerPCCPU {
>      int cpu_dt_id;
>      uint32_t max_compat;
>      uint32_t cpu_version;
> +
> +    /* fields used only during migration for compatibility hacks */
> +    target_ulong mig_msr_mask;
> +    uint64_t mig_insns_flags;
> +    uint64_t mig_insns_flags2;
> +    uint32_t mig_nb_BATs;
>  };
>  
>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index e43cb6c..fcac263 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -140,6 +140,21 @@ static void cpu_pre_save(void *opaque)
>      PowerPCCPU *cpu = opaque;
>      CPUPPCState *env = &cpu->env;
>      int i;
> +    uint64_t insns_compat_mask =
> +        PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB
> +        | PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES
> +        | PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | PPC_FLOAT_FRSQRTES
> +        | PPC_FLOAT_STFIWX | PPC_FLOAT_EXT
> +        | PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ
> +        | PPC_MEM_SYNC | PPC_MEM_EIEIO | PPC_MEM_TLBIE | PPC_MEM_TLBSYNC
> +        | PPC_64B | PPC_64BX | PPC_ALTIVEC
> +        | PPC_SEGMENT_64B | PPC_SLBI | PPC_POPCNTB | PPC_POPCNTWD;
> +    uint64_t insns_compat_mask2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX
> +        | PPC2_PERM_ISA206 | PPC2_DIVE_ISA206
> +        | PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206
> +        | PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207
> +        | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207
> +        | PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 | PPC2_TM;
>  
>      env->spr[SPR_LR] = env->lr;
>      env->spr[SPR_CTR] = env->ctr;
> @@ -161,6 +176,12 @@ static void cpu_pre_save(void *opaque)
>          env->spr[SPR_IBAT4U + 2*i] = env->IBAT[0][i+4];
>          env->spr[SPR_IBAT4U + 2*i + 1] = env->IBAT[1][i+4];
>      }
> +
> +    /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> +    cpu->mig_msr_mask = env->msr_mask;
> +    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +    cpu->mig_nb_BATs = env->nb_BATs;
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -561,10 +582,10 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> +        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> +        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> 


-- 
Alexey

Re: [Qemu-devel] [PATCHv2 3/5] target-ppc: Allow eventual removal of old migration mistakes

[Alexey Kardashevskiy]

On 21/11/16 16:31, David Gibson wrote:
> Until very recently, the vmstate for ppc cpus included some poorly
> thought out VMSTATE_EQUAL() components, that can easily break
> migration compatibility, and did so between qemu-2.6 and later
> versions.  A hack was recently added which fixes this migration
> breakage, but it leaves the unhelpful cruft of these fields in the
> migration stream.
> 
> dThis patch adds a new cpu property allowing these fields to be
> removed from the stream entirely.  This property is enabled by default
> for the pseries-2.8 machine type - which comes after the fix - and for
> all non-pseries machine types - which aren't mature enough to care
> about cross-version migration.
> 
> The migration hack remains in place for pseries-2.7 and earlier
> machine types, allowing backwards and forwards migration with the
> older machine types.
> 
> This restricts the migration compatibility cruft to older machine
> types, and at least opens the possibility of eventually deprecating
> and removing it entirely.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>  hw/ppc/spapr.c              |  5 +++++
>  target-ppc/cpu.h            |  3 ++-
>  target-ppc/machine.c        | 26 ++++++++++++++++++--------
>  target-ppc/translate_init.c |  6 ++++++
>  4 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 54b88d3..775ad2e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2767,6 +2767,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>          .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,     \
>          .property = "mem64_win_size",               \
>          .value    = "0",                            \
> +    },                                              \
> +    {                                               \
> +        .driver = TYPE_POWERPC_CPU,                 \
> +        .property = "pre-2.8-migration",            \
> +        .value    = "on",                           \
>      },
>  
>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 7798b2e..2a50c43 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1167,7 +1167,8 @@ struct PowerPCCPU {
>      uint32_t max_compat;
>      uint32_t cpu_version;
>  
> -    /* fields used only during migration for compatibility hacks */
> +    /* Fields related to migration compatibility hacks */
> +    bool pre_2_8_migration;
>      target_ulong mig_msr_mask;
>      uint64_t mig_insns_flags;
>      uint64_t mig_insns_flags2;
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index fcac263..18c16d2 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -135,6 +135,13 @@ static const VMStateInfo vmstate_info_avr = {
>  #define VMSTATE_AVR_ARRAY(_f, _s, _n)                             \
>      VMSTATE_AVR_ARRAY_V(_f, _s, _n, 0)
>  
> +static bool cpu_pre_2_8_migration(void *opaque, int version_id)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    return cpu->pre_2_8_migration;
> +}
> +
>  static void cpu_pre_save(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -178,10 +185,12 @@ static void cpu_pre_save(void *opaque)
>      }
>  
>      /* Hacks for migration compatibility between 2.6, 2.7 & 2.8 */
> -    cpu->mig_msr_mask = env->msr_mask;
> -    cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> -    cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> -    cpu->mig_nb_BATs = env->nb_BATs;
> +    if (cpu->pre_2_8_migration) {
> +        cpu->mig_msr_mask = env->msr_mask;
> +        cpu->mig_insns_flags = env->insns_flags & insns_compat_mask;
> +        cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> +        cpu->mig_nb_BATs = env->nb_BATs;
> +    }
>  }
>  
>  static int cpu_post_load(void *opaque, int version_id)
> @@ -582,10 +591,11 @@ const VMStateDescription vmstate_ppc_cpu = {
>          /* FIXME: access_type? */
>  
>          /* Sanity checking */
> -        VMSTATE_UINTTL(mig_msr_mask, PowerPCCPU),
> -        VMSTATE_UINT64(mig_insns_flags, PowerPCCPU),
> -        VMSTATE_UINT64(mig_insns_flags2, PowerPCCPU),
> -        VMSTATE_UINT32(mig_nb_BATs, PowerPCCPU),
> +        VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, cpu_pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_insns_flags, PowerPCCPU, cpu_pre_2_8_migration),
> +        VMSTATE_UINT64_TEST(mig_insns_flags2, PowerPCCPU,
> +                            cpu_pre_2_8_migration),
> +        VMSTATE_UINT32_TEST(mig_nb_BATs, PowerPCCPU, cpu_pre_2_8_migration),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 208fa1e..626e031 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10520,6 +10520,11 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static Property ppc_cpu_properties[] = {
> +    DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10532,6 +10537,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
>      dc->realize = ppc_cpu_realizefn;
>      dc->unrealize = ppc_cpu_unrealizefn;
> +    dc->props = ppc_cpu_properties;
>  
>      pcc->parent_reset = cc->reset;
>      cc->reset = ppc_cpu_reset;
> 


-- 
Alexey

Re: [Qemu-devel] [PATCHv2 4/5] Revert "spapr: Fix migration of PCI host bridges from qemu-2.7"

[Alexey Kardashevskiy]

On 21/11/16 16:31, David Gibson wrote:
> This reverts commit 9b54ca0ba781012eeea4237b7c4832ba2ea81d89.
> 
> The commit above corrected a migration breakage between qemu-2.7 and
> qemu-2.8.  However it did so by advancing the migration version for
> the PCI host bridge, which obviously breaks migration backwards to
> earlier qemu versions.
> 
> Although it's not totally essential, we'd like to maintain the
> possibility for backwards migration, so revert the change in
> preparation for a better fix.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>  hw/ppc/spapr_pci.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 661f7d8..e429c94 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1680,25 +1680,19 @@ static int spapr_pci_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static bool version_before_3(void *opaque, int version_id)
> -{
> -    return version_id < 3;
> -}
> -
>  static const VMStateDescription vmstate_spapr_pci = {
>      .name = "spapr_pci",
> -    .version_id = 3,
> +    .version_id = 2,
>      .minimum_version_id = 2,
>      .pre_save = spapr_pci_pre_save,
>      .post_load = spapr_pci_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> -        VMSTATE_UNUSED_TEST(version_before_3,
> -                            sizeof(uint32_t) /* dma_liobn[0] */
> -                            + sizeof(uint64_t) /* mem_win_addr */
> -                            + sizeof(uint64_t) /* mem_win_size */
> -                            + sizeof(uint64_t) /* io_win_addr */
> -                            + sizeof(uint64_t) /* io_win_size */),
> +        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> +        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
>          VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
>                               vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
>          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> 


-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 5550 bytes --]

On Tue, 22 Nov 2016 10:15:59 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 21, 2016 at 05:02:20PM +0100, Greg Kurz wrote:
> > On Mon, 21 Nov 2016 13:02:53 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> > > On 21.11.2016 06:31, David Gibson wrote:  
> > > > daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> > > > from qemu-2.7 to the current version.  It split the device's MMIO
> > > > window into two pieces for 32-bit and 64-bit MMIO.
> > > > 
> > > > The patch included backwards compatibility code to convert the old
> > > > property into the new format.  However, the property value was also
> > > > transferred in the migration stream and compared with a (probably
> > > > unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> > > > the new style converted value from (pre-)2.8 giving a mismatch and
> > > > migration failure.
> > > > 
> > > > Along with the actual field that caused the breakage, there are
> > > > several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> > > > migration, we read the values in the stream into scratch variables and
> > > > ignore them, instead of comparing for equality.  To fix backwards
> > > > migration, we populate those scratch variables in pre_save() with
> > > > adjusted values to match the old behaviour.
> > > > 
> > > > To permit the eventual possibility of removing this cruft from the
> > > > stream, we only include these compatibility fields if a new
> > > > 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> > > > machine type, which obviously can't be migrated backwards, but set it
> > > > on earlier machine type versions.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/ppc/spapr.c              |  5 +++++
> > > >  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
> > > >  include/hw/pci-host/spapr.h |  6 ++++++
> > > >  3 files changed, 39 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 775ad2e..c3269c7 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> > > >          .driver = TYPE_POWERPC_CPU,                 \
> > > >          .property = "pre-2.8-migration",            \
> > > >          .value    = "on",                           \
> > > > +    },                                              \
> > > > +    {                                               \
> > > > +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> > > > +        .property = "pre-2.8-migration",            \
> > > > +        .value    = "on",                           \
> > > >      },
> > > >  
> > > >  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > > index e429c94..c62c1cb 100644
> > > > --- a/hw/ppc/spapr_pci.c
> > > > +++ b/hw/ppc/spapr_pci.c
> > > > @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
> > > >      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
> > > >                         (1ULL << 12) | (1ULL << 16)),
> > > >      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> > > > +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> > > > +                     pre_2_8_migration, false),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > > >  
> > > > @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
> > > >          sphb->msi_devs[i].key = *(uint32_t *) key;
> > > >          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> > > >      }
> > > > +
> > > > +    if (sphb->pre_2_8_migration) {
> > > > +        sphb->mig_liobn = sphb->dma_liobn[0];
> > > > +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> > > > +        sphb->mig_mem_win_size = sphb->mem_win_size;
> > > > +        sphb->mig_io_win_addr = sphb->io_win_addr;
> > > > +        sphb->mig_io_win_size = sphb->io_win_size;
> > > > +
> > > > +        if ((sphb->mem64_win_size != 0)
> > > > +            && (sphb->mem64_win_addr
> > > > +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> > > > +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> > > > +        }    
> > > 
> > > Should we maybe print a warning/error message in case
> > > 
> > >  sphb->mem64_win_size != 0 &&
> > >  sphb->mem64_win_addr != sphb->mem_win_addr + sphb->mem_win_size
> > > 
> > > ... assuming that this means a configuration which can not be migrated
> > > backwards?
> > >   
> > 
> > Then shouldn't we forbid pre_2_8_migration to be set when we have a
> > non-contiguous window ?  
> 
> So, yes, we could do either of these, but really I don't think it's
> worth it.  It will only happen if you have a custom constructed PHB,
> in which case it's pretty much on you to ensure that there's something
> compatible at the far end.  Restricting it here has somewhat the same
> problem as VMSTATE_EQUAL()s did - they make assumptions about what is
> and isn't sane which could be broken by future changes (in this case
> changes in the 2.7 stable tree).  They might be unlikely to change,
> but if they do things break, and the only benefit is a marginally
> better error message in cases that won't work anyway.
> 

Makes sense indeed, hence:

Reviewed-by: Greg Kurz <groug@kaod.org>

Cheers.

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv2 1/5] target-ppc: Fix CPU migration from qemu-2.6 <-> later versions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Tue, Nov 22, 2016 at 07:19:38PM +1100, Alexey Kardashevskiy wrote:
> On 21/11/16 16:31, David Gibson wrote:
> > When migration for target-ppc was converted to vmstate, several
> > VMSTATE_EQUAL() checks were foolishly included of things that really
> > should be internal state.  Specifically we verified equality of the
> > insns_flags and insns_flags2 fields, which are used within TCG to
> > determine which groups of instructions are available on this cpu
> > model.  Between qemu-2.6 and qemu-2.7 we made some changes to these
> > classes which broke migration.
> > 
> > This path fixes migration both forwards and backwards.  On migration
> > from 2.6 to later versions we import the fields into teporary
> > variables, which we then ignore.  In migration backwards, we populate
> > the temporary fields from the runtime fields, but mask out the bits
> > which were added after qemu-2.6, allowing the VMSTATE_EQUAL in
> > qemu-2.6 to accept the stream.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> One question though - do we care about TCG migration at all? If so, we
> could want to migrate these bits, just not with _EQUAL but rather check
> that the source does not have bits which are not supported by the
> destination.

Yes, we do care about TCG migration, but the conclusion doesn't
follow.  insns_flags is a qemu internal representation, not anything
architected, and it never changes during runtime.  We should be free
to change that representation, and as long as the CPUs are compatible
it should still work.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6779 bytes --]

On Tue, Nov 22, 2016 at 07:17:27PM +1100, Alexey Kardashevskiy wrote:
> On 21/11/16 16:31, David Gibson wrote:
> > daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
> > from qemu-2.7 to the current version.  It split the device's MMIO
> > window into two pieces for 32-bit and 64-bit MMIO.
> > 
> > The patch included backwards compatibility code to convert the old
> > property into the new format.  However, the property value was also
> > transferred in the migration stream and compared with a (probably
> > unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
> > the new style converted value from (pre-)2.8 giving a mismatch and
> > migration failure.
> > 
> > Along with the actual field that caused the breakage, there are
> > several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> > migration, we read the values in the stream into scratch variables and
> > ignore them, instead of comparing for equality.  To fix backwards
> > migration, we populate those scratch variables in pre_save() with
> > adjusted values to match the old behaviour.
> > 
> > To permit the eventual possibility of removing this cruft from the
> > stream, we only include these compatibility fields if a new
> > 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> > machine type, which obviously can't be migrated backwards, but set it
> > on earlier machine type versions.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c              |  5 +++++
> >  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
> >  include/hw/pci-host/spapr.h |  6 ++++++
> >  3 files changed, 39 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 775ad2e..c3269c7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> >          .driver = TYPE_POWERPC_CPU,                 \
> >          .property = "pre-2.8-migration",            \
> >          .value    = "on",                           \
> > +    },                                              \
> > +    {                                               \
> > +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> > +        .property = "pre-2.8-migration",            \
> > +        .value    = "on",                           \
> >      },
> >  
> >  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index e429c94..c62c1cb 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
> >      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
> >                         (1ULL << 12) | (1ULL << 16)),
> >      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> > +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> > +                     pre_2_8_migration, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
> >          sphb->msi_devs[i].key = *(uint32_t *) key;
> >          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> >      }
> > +
> > +    if (sphb->pre_2_8_migration) {
> 
> 
> You check for pre_2_8_migration here but you do not in 3/5, what is the
> difference?

Uh.. where don't I in 3/5?

> Also this chunk did not apply on top of dwg/master or qemu.org/master (some
> whitespace issue? not sure) so I tested dwg/ppc-for-2.8, it works.

Yes, the series was based on ppc-for-2.8

> 
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> 
> 
> > +        sphb->mig_liobn = sphb->dma_liobn[0];
> > +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> > +        sphb->mig_mem_win_size = sphb->mem_win_size;
> > +        sphb->mig_io_win_addr = sphb->io_win_addr;
> > +        sphb->mig_io_win_size = sphb->io_win_size;
> > +
> > +        if ((sphb->mem64_win_size != 0)
> > +            && (sphb->mem64_win_addr
> > +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> > +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> > +        }
> > +    }
> >  }
> >  
> >  /*
> > @@ -1680,6 +1696,13 @@ static int spapr_pci_post_load(void *opaque, int version_id)
> >      return 0;
> >  }
> >  
> > +static bool pre_2_8_migration(void *opaque, int version_id)
> > +{
> > +    sPAPRPHBState *sphb = opaque;
> > +
> > +    return sphb->pre_2_8_migration;
> > +}
> > +
> >  static const VMStateDescription vmstate_spapr_pci = {
> >      .name = "spapr_pci",
> >      .version_id = 2,
> > @@ -1688,11 +1711,11 @@ static const VMStateDescription vmstate_spapr_pci = {
> >      .post_load = spapr_pci_post_load,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> > -        VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> > -        VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> > -        VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> > -        VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> > -        VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
> > +        VMSTATE_UINT32_TEST(mig_liobn, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_mem_win_addr, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_mem_win_size, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_io_win_addr, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_io_win_size, sPAPRPHBState, pre_2_8_migration),
> >          VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
> >                               vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
> >          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index b92c1b5..092294e 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -79,6 +79,12 @@ struct sPAPRPHBState {
> >      uint64_t dma64_win_addr;
> >  
> >      uint32_t numa_node;
> > +
> > +    /* Fields for migration compatibility hacks */
> > +    bool pre_2_8_migration;
> > +    uint32_t mig_liobn;
> > +    hwaddr mig_mem_win_addr, mig_mem_win_size;
> > +    hwaddr mig_io_win_addr, mig_io_win_size;
> >  };
> >  
> >  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> > 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

[Alexey Kardashevskiy]

[-- Attachment #1: Type: text/plain, Size: 3821 bytes --]

On 23/11/16 11:17, David Gibson wrote:
> On Tue, Nov 22, 2016 at 07:17:27PM +1100, Alexey Kardashevskiy wrote:
>> On 21/11/16 16:31, David Gibson wrote:
>>> daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration
>>> from qemu-2.7 to the current version.  It split the device's MMIO
>>> window into two pieces for 32-bit and 64-bit MMIO.
>>>
>>> The patch included backwards compatibility code to convert the old
>>> property into the new format.  However, the property value was also
>>> transferred in the migration stream and compared with a (probably
>>> unwise) VMSTATE_EQUAL.  So, the "raw" value from 2.7 is compared to
>>> the new style converted value from (pre-)2.8 giving a mismatch and
>>> migration failure.
>>>
>>> Along with the actual field that caused the breakage, there are
>>> several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
>>> migration, we read the values in the stream into scratch variables and
>>> ignore them, instead of comparing for equality.  To fix backwards
>>> migration, we populate those scratch variables in pre_save() with
>>> adjusted values to match the old behaviour.
>>>
>>> To permit the eventual possibility of removing this cruft from the
>>> stream, we only include these compatibility fields if a new
>>> 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
>>> machine type, which obviously can't be migrated backwards, but set it
>>> on earlier machine type versions.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  hw/ppc/spapr.c              |  5 +++++
>>>  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
>>>  include/hw/pci-host/spapr.h |  6 ++++++
>>>  3 files changed, 39 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 775ad2e..c3269c7 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>>>          .driver = TYPE_POWERPC_CPU,                 \
>>>          .property = "pre-2.8-migration",            \
>>>          .value    = "on",                           \
>>> +    },                                              \
>>> +    {                                               \
>>> +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
>>> +        .property = "pre-2.8-migration",            \
>>> +        .value    = "on",                           \
>>>      },
>>>  
>>>  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index e429c94..c62c1cb 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
>>>      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
>>>                         (1ULL << 12) | (1ULL << 16)),
>>>      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
>>> +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
>>> +                     pre_2_8_migration, false),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>  
>>> @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
>>>          sphb->msi_devs[i].key = *(uint32_t *) key;
>>>          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
>>>      }
>>> +
>>> +    if (sphb->pre_2_8_migration) {
>>
>>
>> You check for pre_2_8_migration here but you do not in 3/5, what is the
>> difference?
> 
> Uh.. where don't I in 3/5?


Never mind, I did not sleep well and while switching between patches, I
confused 1/5 (which does not have a check) with 3/5 (which does). My bad,
sorry for the noise. I run some more tests today, just to make sure I slept
well, all good :)



-- 
Alexey


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]