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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]