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

[David Gibson]

On Mon, Nov 21, 2016 at 04:26:10PM +0100, Greg Kurz wrote:
> On Mon, 21 Nov 2016 16:31:40 +1100
> 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
>   ^
> 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 
> > ---
> 
> Reviewed-by: Greg Kurz 
> 
> >  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),
> > +

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

Reviewed-by: Greg Kurz 

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