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

[Greg Kurz <groug@kaod.org>]

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;