Migrating between different CPU versions is a bit complicated for ppc.
A long time ago, we ensured identical CPU versions at either end by
checking the PVR had the same value.  However, this breaks under KVM
HV, because we always have to use the host's PVR - it's not
virtualized.  That would mean we couldn't migrate between hosts with
different PVRs, even if the CPUs are close enough to compatible in
practice (sometimes identical cores with different surrounding logic
have different PVRs, so this happens in practice quite often).

So, we removed the PVR check, but instead checked that several flags
indicating supported instructions matched.  This turns out to be a bad
idea, because those instruction masks are not architected information, but
essentially a TCG implementation detail.  So changes to qemu internal CPU
modelling can break migration - this happened between qemu-2.6 and
qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
removal of old migration mistakes".

Now, verification of CPU compatibility across a migration basically doesn't
happen.  We simply ignore the PVR of the incoming migration, and hope the
cpu on the destination is close enough to work.

Now that we've cleaned up handling of processor compatibility modes for
pseries machine type, we can do better.  We allow migration if:

    * The source and destination PVRs are for the same type of CPU, as
      determined by CPU class's pvr_match function
OR  * When the source was in a compatibility mode, and the destination CPU
      supports the same compatibility mode

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 6cb3a48..20a46c9 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -8,6 +8,7 @@
 #include "helper_regs.h"
 #include "mmu-hash64.h"
 #include "migration/cpu.h"
+#include "qapi/error.h"
 
 static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
 {
@@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
     }
 }
 
+/*
+ * Determine if a given PVR is a "close enough" match to the CPU
+ * object.  For TCG and KVM PR it would probably be sufficient to
+ * require an exact PVR match.  However for KVM HV the user is
+ * restricted to a PVR exactly matching the host CPU.  The correct way
+ * to handle this is to put the guest into an architected
+ * compatibility mode.  However, to allow a more forgiving transition
+ * and migration from before this was widely done, we allow migration
+ * between sufficiently similar PVRs, as determined by the CPU class's
+ * pvr_match() hook.
+ */
+static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    if (pvr == pcc->pvr) {
+        return true;
+    }
+    if (pcc->pvr_match) {
+        return pcc->pvr_match(pcc, pvr);
+    }
+    return false;
+}
+
 static int cpu_post_load(void *opaque, int version_id)
 {
     PowerPCCPU *cpu = opaque;
@@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
     target_ulong msr;
 
     /*
-     * We always ignore the source PVR. The user or management
-     * software has to take care of running QEMU in a compatible mode.
+     * If we're operating in compat mode, we should be ok as long as
+     * the destination supports the same compatiblity mode.
+     *
+     * Otherwise, however, we require that the destination has exactly
+     * the same CPU model as the source.
      */
-    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
+
+#if defined(TARGET_PPC64)
+    if (cpu->compat_pvr) {
+        Error *local_err = NULL;
+
+        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            error_free(local_err);
+            return -1;
+        }
+    } else
+#endif
+    {
+        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
+            return -1;
+        }
+    }
+
     env->lr = env->spr[SPR_LR];
     env->ctr = env->spr[SPR_CTR];
     cpu_write_xer(env, env->spr[SPR_XER]);
@@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
     }
 };
 
+static bool compat_needed(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+
+    return cpu->vhyp != NULL;
+}
+
+static const VMStateDescription vmstate_compat = {
+    .name = "cpu/compat",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = compat_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
     .version_id = 5,
@@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
         &vmstate_tlb6xx,
         &vmstate_tlbemb,
         &vmstate_tlbmas,
+        &vmstate_compat,
         NULL
     }
 };
-- 
2.9.3

Quoting David Gibson (2017-04-27 02:28:43)
> Migrating between different CPU versions is a bit complicated for ppc.
> A long time ago, we ensured identical CPU versions at either end by
> checking the PVR had the same value.  However, this breaks under KVM
> HV, because we always have to use the host's PVR - it's not
> virtualized.  That would mean we couldn't migrate between hosts with
> different PVRs, even if the CPUs are close enough to compatible in
> practice (sometimes identical cores with different surrounding logic
> have different PVRs, so this happens in practice quite often).
> 
> So, we removed the PVR check, but instead checked that several flags
> indicating supported instructions matched.  This turns out to be a bad
> idea, because those instruction masks are not architected information, but
> essentially a TCG implementation detail.  So changes to qemu internal CPU
> modelling can break migration - this happened between qemu-2.6 and
> qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> removal of old migration mistakes".
> 
> Now, verification of CPU compatibility across a migration basically doesn't
> happen.  We simply ignore the PVR of the incoming migration, and hope the
> cpu on the destination is close enough to work.
> 
> Now that we've cleaned up handling of processor compatibility modes for
> pseries machine type, we can do better.  We allow migration if:
> 
>     * The source and destination PVRs are for the same type of CPU, as
>       determined by CPU class's pvr_match function
> OR  * When the source was in a compatibility mode, and the destination CPU
>       supports the same compatibility mode
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 6cb3a48..20a46c9 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -8,6 +8,7 @@
>  #include "helper_regs.h"
>  #include "mmu-hash64.h"
>  #include "migration/cpu.h"
> +#include "qapi/error.h"
> 
>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>  {
> @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
>      }
>  }
> 
> +/*
> + * Determine if a given PVR is a "close enough" match to the CPU
> + * object.  For TCG and KVM PR it would probably be sufficient to
> + * require an exact PVR match.  However for KVM HV the user is
> + * restricted to a PVR exactly matching the host CPU.  The correct way
> + * to handle this is to put the guest into an architected
> + * compatibility mode.  However, to allow a more forgiving transition
> + * and migration from before this was widely done, we allow migration
> + * between sufficiently similar PVRs, as determined by the CPU class's
> + * pvr_match() hook.
> + */
> +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    if (pvr == pcc->pvr) {
> +        return true;
> +    }
> +    if (pcc->pvr_match) {
> +        return pcc->pvr_match(pcc, pvr);
> +    }
> +    return false;
> +}
> +
>  static int cpu_post_load(void *opaque, int version_id)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
>      target_ulong msr;
> 
>      /*
> -     * We always ignore the source PVR. The user or management
> -     * software has to take care of running QEMU in a compatible mode.
> +     * If we're operating in compat mode, we should be ok as long as
> +     * the destination supports the same compatiblity mode.
> +     *
> +     * Otherwise, however, we require that the destination has exactly
> +     * the same CPU model as the source.
>       */
> -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> +
> +#if defined(TARGET_PPC64)
> +    if (cpu->compat_pvr) {
> +        Error *local_err = NULL;
> +
> +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(local_err);
> +            return -1;
> +        }
> +    } else
> +#endif
> +    {
> +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> +            return -1;
> +        }
> +    }
> +
>      env->lr = env->spr[SPR_LR];
>      env->ctr = env->spr[SPR_CTR];
>      cpu_write_xer(env, env->spr[SPR_XER]);
> @@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
>      }
>  };
> 
> +static bool compat_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    return cpu->vhyp != NULL;
> +}

Would it make sense to relax this to:

  cpu->vhyp != NULL && cpu->compat_pvr

? This would at least allow cross-version migration in cases where we
weren't previously running in a compatibility mode. As it stands it
seems like this would rule out both new->old and old->new migration.

Another possibility might be to only enable migration/checking of
compat_pvr for 2.10 and later, similar to the cpu_pre_2_8_migration
stuff.

> +
> +static const VMStateDescription vmstate_compat = {
> +    .name = "cpu/compat",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = compat_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
>      .version_id = 5,
> @@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>          &vmstate_tlb6xx,
>          &vmstate_tlbemb,
>          &vmstate_tlbmas,
> +        &vmstate_compat,
>          NULL
>      }
>  };
> -- 
> 2.9.3
> 

On Thu, Apr 27, 2017 at 02:51:31PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-04-27 02:28:43)
> > Migrating between different CPU versions is a bit complicated for ppc.
> > A long time ago, we ensured identical CPU versions at either end by
> > checking the PVR had the same value.  However, this breaks under KVM
> > HV, because we always have to use the host's PVR - it's not
> > virtualized.  That would mean we couldn't migrate between hosts with
> > different PVRs, even if the CPUs are close enough to compatible in
> > practice (sometimes identical cores with different surrounding logic
> > have different PVRs, so this happens in practice quite often).
> > 
> > So, we removed the PVR check, but instead checked that several flags
> > indicating supported instructions matched.  This turns out to be a bad
> > idea, because those instruction masks are not architected information, but
> > essentially a TCG implementation detail.  So changes to qemu internal CPU
> > modelling can break migration - this happened between qemu-2.6 and
> > qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> > removal of old migration mistakes".
> > 
> > Now, verification of CPU compatibility across a migration basically doesn't
> > happen.  We simply ignore the PVR of the incoming migration, and hope the
> > cpu on the destination is close enough to work.
> > 
> > Now that we've cleaned up handling of processor compatibility modes for
> > pseries machine type, we can do better.  We allow migration if:
> > 
> >     * The source and destination PVRs are for the same type of CPU, as
> >       determined by CPU class's pvr_match function
> > OR  * When the source was in a compatibility mode, and the destination CPU
> >       supports the same compatibility mode
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 68 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..20a46c9 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -8,6 +8,7 @@
> >  #include "helper_regs.h"
> >  #include "mmu-hash64.h"
> >  #include "migration/cpu.h"
> > +#include "qapi/error.h"
> > 
> >  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> >  {
> > @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
> >      }
> >  }
> > 
> > +/*
> > + * Determine if a given PVR is a "close enough" match to the CPU
> > + * object.  For TCG and KVM PR it would probably be sufficient to
> > + * require an exact PVR match.  However for KVM HV the user is
> > + * restricted to a PVR exactly matching the host CPU.  The correct way
> > + * to handle this is to put the guest into an architected
> > + * compatibility mode.  However, to allow a more forgiving transition
> > + * and migration from before this was widely done, we allow migration
> > + * between sufficiently similar PVRs, as determined by the CPU class's
> > + * pvr_match() hook.
> > + */
> > +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> > +{
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +
> > +    if (pvr == pcc->pvr) {
> > +        return true;
> > +    }
> > +    if (pcc->pvr_match) {
> > +        return pcc->pvr_match(pcc, pvr);
> > +    }
> > +    return false;
> > +}
> > +
> >  static int cpu_post_load(void *opaque, int version_id)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
> >      target_ulong msr;
> > 
> >      /*
> > -     * We always ignore the source PVR. The user or management
> > -     * software has to take care of running QEMU in a compatible mode.
> > +     * If we're operating in compat mode, we should be ok as long as
> > +     * the destination supports the same compatiblity mode.
> > +     *
> > +     * Otherwise, however, we require that the destination has exactly
> > +     * the same CPU model as the source.
> >       */
> > -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> > +
> > +#if defined(TARGET_PPC64)
> > +    if (cpu->compat_pvr) {
> > +        Error *local_err = NULL;
> > +
> > +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            error_free(local_err);
> > +            return -1;
> > +        }
> > +    } else
> > +#endif
> > +    {
> > +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> > +            return -1;
> > +        }
> > +    }
> > +
> >      env->lr = env->spr[SPR_LR];
> >      env->ctr = env->spr[SPR_CTR];
> >      cpu_write_xer(env, env->spr[SPR_XER]);
> > @@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
> >      }
> >  };
> > 
> > +static bool compat_needed(void *opaque)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +
> > +    return cpu->vhyp != NULL;
> > +}
> 
> Would it make sense to relax this to:
> 
>   cpu->vhyp != NULL && cpu->compat_pvr

Hm, so that's equivalent to just cpu->compat_pvr != 0, since
compat_pvr should never be set when !vhyp.

> ? This would at least allow cross-version migration in cases where we
> weren't previously running in a compatibility mode. As it stands it
> seems like this would rule out both new->old and old->new migration.

Hrm.  I thought ther section needed test just controlled whether the
section was created on outgoing migration, not whether it was
mandatory on an incoming migration.  So old->new migration should I
think work (well, no worse than it ever did).

new->old would be broken by this though.  So I guess I need some sort
of compat flag so the older machine types don't generate this section.

> Another possibility might be to only enable migration/checking of
> compat_pvr for 2.10 and later, similar to the cpu_pre_2_8_migration
> stuff.
> 
> > +
> > +static const VMStateDescription vmstate_compat = {
> > +    .name = "cpu/compat",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = compat_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  const VMStateDescription vmstate_ppc_cpu = {
> >      .name = "cpu",
> >      .version_id = 5,
> > @@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
> >          &vmstate_tlb6xx,
> >          &vmstate_tlbemb,
> >          &vmstate_tlbmas,
> > +        &vmstate_compat,
> >          NULL
> >      }
> >  };
> 

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

On Thu, Apr 27, 2017 at 02:51:31PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-04-27 02:28:43)
> > Migrating between different CPU versions is a bit complicated for ppc.
> > A long time ago, we ensured identical CPU versions at either end by
> > checking the PVR had the same value.  However, this breaks under KVM
> > HV, because we always have to use the host's PVR - it's not
> > virtualized.  That would mean we couldn't migrate between hosts with
> > different PVRs, even if the CPUs are close enough to compatible in
> > practice (sometimes identical cores with different surrounding logic
> > have different PVRs, so this happens in practice quite often).
> > 
> > So, we removed the PVR check, but instead checked that several flags
> > indicating supported instructions matched.  This turns out to be a bad
> > idea, because those instruction masks are not architected information, but
> > essentially a TCG implementation detail.  So changes to qemu internal CPU
> > modelling can break migration - this happened between qemu-2.6 and
> > qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> > removal of old migration mistakes".
> > 
> > Now, verification of CPU compatibility across a migration basically doesn't
> > happen.  We simply ignore the PVR of the incoming migration, and hope the
> > cpu on the destination is close enough to work.
> > 
> > Now that we've cleaned up handling of processor compatibility modes for
> > pseries machine type, we can do better.  We allow migration if:
> > 
> >     * The source and destination PVRs are for the same type of CPU, as
> >       determined by CPU class's pvr_match function
> > OR  * When the source was in a compatibility mode, and the destination CPU
> >       supports the same compatibility mode
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 68 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..20a46c9 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -8,6 +8,7 @@
> >  #include "helper_regs.h"
> >  #include "mmu-hash64.h"
> >  #include "migration/cpu.h"
> > +#include "qapi/error.h"
> > 
> >  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> >  {
> > @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
> >      }
> >  }
> > 
> > +/*
> > + * Determine if a given PVR is a "close enough" match to the CPU
> > + * object.  For TCG and KVM PR it would probably be sufficient to
> > + * require an exact PVR match.  However for KVM HV the user is
> > + * restricted to a PVR exactly matching the host CPU.  The correct way
> > + * to handle this is to put the guest into an architected
> > + * compatibility mode.  However, to allow a more forgiving transition
> > + * and migration from before this was widely done, we allow migration
> > + * between sufficiently similar PVRs, as determined by the CPU class's
> > + * pvr_match() hook.
> > + */
> > +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> > +{
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +
> > +    if (pvr == pcc->pvr) {
> > +        return true;
> > +    }
> > +    if (pcc->pvr_match) {
> > +        return pcc->pvr_match(pcc, pvr);
> > +    }
> > +    return false;
> > +}
> > +
> >  static int cpu_post_load(void *opaque, int version_id)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
> >      target_ulong msr;
> > 
> >      /*
> > -     * We always ignore the source PVR. The user or management
> > -     * software has to take care of running QEMU in a compatible mode.
> > +     * If we're operating in compat mode, we should be ok as long as
> > +     * the destination supports the same compatiblity mode.
> > +     *
> > +     * Otherwise, however, we require that the destination has exactly
> > +     * the same CPU model as the source.
> >       */
> > -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> > +
> > +#if defined(TARGET_PPC64)
> > +    if (cpu->compat_pvr) {
> > +        Error *local_err = NULL;
> > +
> > +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            error_free(local_err);
> > +            return -1;
> > +        }
> > +    } else
> > +#endif
> > +    {
> > +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> > +            return -1;
> > +        }
> > +    }
> > +
> >      env->lr = env->spr[SPR_LR];
> >      env->ctr = env->spr[SPR_CTR];
> >      cpu_write_xer(env, env->spr[SPR_XER]);
> > @@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
> >      }
> >  };
> > 
> > +static bool compat_needed(void *opaque)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +
> > +    return cpu->vhyp != NULL;
> > +}
> 
> Would it make sense to relax this to:
> 
>   cpu->vhyp != NULL && cpu->compat_pvr

So, that's equivalent to just cpu->compat_pvr, since it can't be
non-zero if cpu->vhyp isn't set.

> ? This would at least allow cross-version migration in cases where we
> weren't previously running in a compatibility mode. As it stands it
> seems like this would rule out both new->old and old->new migration.

Ah, yes that's a good idea.

> Another possibility might be to only enable migration/checking of
> compat_pvr for 2.10 and later, similar to the cpu_pre_2_8_migration
> stuff.

True.. and that might be a bit more robust, too.  I'll look into it.

> 
> > +
> > +static const VMStateDescription vmstate_compat = {
> > +    .name = "cpu/compat",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = compat_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  const VMStateDescription vmstate_ppc_cpu = {
> >      .name = "cpu",
> >      .version_id = 5,
> > @@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
> >          &vmstate_tlb6xx,
> >          &vmstate_tlbemb,
> >          &vmstate_tlbmas,
> > +        &vmstate_compat,
> >          NULL
> >      }
> >  };
> 

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

On Thu, 27 Apr 2017 17:28:43 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Migrating between different CPU versions is a bit complicated for ppc.
> A long time ago, we ensured identical CPU versions at either end by
> checking the PVR had the same value.  However, this breaks under KVM
> HV, because we always have to use the host's PVR - it's not
> virtualized.  That would mean we couldn't migrate between hosts with
> different PVRs, even if the CPUs are close enough to compatible in
> practice (sometimes identical cores with different surrounding logic
> have different PVRs, so this happens in practice quite often).
> 
> So, we removed the PVR check, but instead checked that several flags
> indicating supported instructions matched.  This turns out to be a bad
> idea, because those instruction masks are not architected information, but
> essentially a TCG implementation detail.  So changes to qemu internal CPU
> modelling can break migration - this happened between qemu-2.6 and
> qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> removal of old migration mistakes".
> 
> Now, verification of CPU compatibility across a migration basically doesn't
> happen.  We simply ignore the PVR of the incoming migration, and hope the
> cpu on the destination is close enough to work.
> 
> Now that we've cleaned up handling of processor compatibility modes for
> pseries machine type, we can do better.  We allow migration if:
> 
>     * The source and destination PVRs are for the same type of CPU, as
>       determined by CPU class's pvr_match function
> OR  * When the source was in a compatibility mode, and the destination CPU
>       supports the same compatibility mode
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 6cb3a48..20a46c9 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -8,6 +8,7 @@
>  #include "helper_regs.h"
>  #include "mmu-hash64.h"
>  #include "migration/cpu.h"
> +#include "qapi/error.h"
>  
>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>  {
> @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
>      }
>  }
>  
> +/*
> + * Determine if a given PVR is a "close enough" match to the CPU
> + * object.  For TCG and KVM PR it would probably be sufficient to
> + * require an exact PVR match.  However for KVM HV the user is
> + * restricted to a PVR exactly matching the host CPU.  The correct way
> + * to handle this is to put the guest into an architected
> + * compatibility mode.  However, to allow a more forgiving transition
> + * and migration from before this was widely done, we allow migration
> + * between sufficiently similar PVRs, as determined by the CPU class's
> + * pvr_match() hook.
> + */
> +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    if (pvr == pcc->pvr) {
> +        return true;
> +    }
> +    if (pcc->pvr_match) {
> +        return pcc->pvr_match(pcc, pvr);
> +    }
> +    return false;
> +}
> +
>  static int cpu_post_load(void *opaque, int version_id)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
>      target_ulong msr;
>  
>      /*
> -     * We always ignore the source PVR. The user or management
> -     * software has to take care of running QEMU in a compatible mode.
> +     * If we're operating in compat mode, we should be ok as long as
> +     * the destination supports the same compatiblity mode.
> +     *
> +     * Otherwise, however, we require that the destination has exactly
> +     * the same CPU model as the source.
>       */
> -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> +
> +#if defined(TARGET_PPC64)
> +    if (cpu->compat_pvr) {
> +        Error *local_err = NULL;
> +
> +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);

As already mentioned during the review of RFCv2, this calls
cpu_synchronize_state(CPU(cpu)) and trashes the registers.

The following changes avoid that:

--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -118,7 +118,8 @@ bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
     return true;
 }
 
-void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
+                    Error **errp)
 {
     const CompatInfo *compat = compat_by_pvr(compat_pvr);
     CPUPPCState *env = &cpu->env;
@@ -138,7 +139,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
         pcr = compat->pcr;
     }
 
-    cpu_synchronize_state(CPU(cpu));
+    if (sync_needed) {
+        cpu_synchronize_state(CPU(cpu));
+    }
 
     cpu->compat_pvr = compat_pvr;
     env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
@@ -162,7 +165,7 @@ static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     SetCompatState *s = arg.host_ptr;
 
-    ppc_set_compat(cpu, s->compat_pvr, &s->err);
+    ppc_set_compat(cpu, s->compat_pvr, true, &s->err);
 }
 
 void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1d8f2fcd4a46..057785347820 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1364,7 +1364,8 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
 #if defined(TARGET_PPC64)
 bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
                       uint32_t min_compat_pvr, uint32_t max_compat_pvr);
-void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
+                    Error **errp);
 #if !defined(CONFIG_USER_ONLY)
 void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
 #endif
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 20a46c95a596..fda63532b041 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -239,7 +239,7 @@ static int cpu_post_load(void *opaque, int version_id)
     if (cpu->compat_pvr) {
         Error *local_err = NULL;
 
-        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
+        ppc_set_compat(cpu, cpu->compat_pvr, false, &local_err);
         if (local_err) {
             error_report_err(local_err);
             error_free(local_err);


> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(local_err);
> +            return -1;
> +        }
> +    } else
> +#endif
> +    {
> +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> +            return -1;
> +        }
> +    }
> +
>      env->lr = env->spr[SPR_LR];
>      env->ctr = env->spr[SPR_CTR];
>      cpu_write_xer(env, env->spr[SPR_XER]);
> @@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
>      }
>  };
>  
> +static bool compat_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    return cpu->vhyp != NULL;
> +}
> +
> +static const VMStateDescription vmstate_compat = {
> +    .name = "cpu/compat",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = compat_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
>      .version_id = 5,
> @@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>          &vmstate_tlb6xx,
>          &vmstate_tlbemb,
>          &vmstate_tlbmas,
> +        &vmstate_compat,
>          NULL
>      }
>  };

On Thu, May 04, 2017 at 12:07:47PM +0200, Greg Kurz wrote:
> On Thu, 27 Apr 2017 17:28:43 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Migrating between different CPU versions is a bit complicated for ppc.
> > A long time ago, we ensured identical CPU versions at either end by
> > checking the PVR had the same value.  However, this breaks under KVM
> > HV, because we always have to use the host's PVR - it's not
> > virtualized.  That would mean we couldn't migrate between hosts with
> > different PVRs, even if the CPUs are close enough to compatible in
> > practice (sometimes identical cores with different surrounding logic
> > have different PVRs, so this happens in practice quite often).
> > 
> > So, we removed the PVR check, but instead checked that several flags
> > indicating supported instructions matched.  This turns out to be a bad
> > idea, because those instruction masks are not architected information, but
> > essentially a TCG implementation detail.  So changes to qemu internal CPU
> > modelling can break migration - this happened between qemu-2.6 and
> > qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> > removal of old migration mistakes".
> > 
> > Now, verification of CPU compatibility across a migration basically doesn't
> > happen.  We simply ignore the PVR of the incoming migration, and hope the
> > cpu on the destination is close enough to work.
> > 
> > Now that we've cleaned up handling of processor compatibility modes for
> > pseries machine type, we can do better.  We allow migration if:
> > 
> >     * The source and destination PVRs are for the same type of CPU, as
> >       determined by CPU class's pvr_match function
> > OR  * When the source was in a compatibility mode, and the destination CPU
> >       supports the same compatibility mode
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 68 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..20a46c9 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -8,6 +8,7 @@
> >  #include "helper_regs.h"
> >  #include "mmu-hash64.h"
> >  #include "migration/cpu.h"
> > +#include "qapi/error.h"
> >  
> >  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> >  {
> > @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
> >      }
> >  }
> >  
> > +/*
> > + * Determine if a given PVR is a "close enough" match to the CPU
> > + * object.  For TCG and KVM PR it would probably be sufficient to
> > + * require an exact PVR match.  However for KVM HV the user is
> > + * restricted to a PVR exactly matching the host CPU.  The correct way
> > + * to handle this is to put the guest into an architected
> > + * compatibility mode.  However, to allow a more forgiving transition
> > + * and migration from before this was widely done, we allow migration
> > + * between sufficiently similar PVRs, as determined by the CPU class's
> > + * pvr_match() hook.
> > + */
> > +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> > +{
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +
> > +    if (pvr == pcc->pvr) {
> > +        return true;
> > +    }
> > +    if (pcc->pvr_match) {
> > +        return pcc->pvr_match(pcc, pvr);
> > +    }
> > +    return false;
> > +}
> > +
> >  static int cpu_post_load(void *opaque, int version_id)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
> >      target_ulong msr;
> >  
> >      /*
> > -     * We always ignore the source PVR. The user or management
> > -     * software has to take care of running QEMU in a compatible mode.
> > +     * If we're operating in compat mode, we should be ok as long as
> > +     * the destination supports the same compatiblity mode.
> > +     *
> > +     * Otherwise, however, we require that the destination has exactly
> > +     * the same CPU model as the source.
> >       */
> > -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> > +
> > +#if defined(TARGET_PPC64)
> > +    if (cpu->compat_pvr) {
> > +        Error *local_err = NULL;
> > +
> > +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> 
> As already mentioned during the review of RFCv2, this calls
> cpu_synchronize_state(CPU(cpu)) and trashes the registers.
> 
> The following changes avoid that:

This is a really ugly fix, and I think it misses the point.

If a synchronize_state() trashes state here, it means we've already
altered register state while not synchronized, which is a pre-existing
bug.

> 
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -118,7 +118,8 @@ bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
>      return true;
>  }
>  
> -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> +                    Error **errp)
>  {
>      const CompatInfo *compat = compat_by_pvr(compat_pvr);
>      CPUPPCState *env = &cpu->env;
> @@ -138,7 +139,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
>          pcr = compat->pcr;
>      }
>  
> -    cpu_synchronize_state(CPU(cpu));
> +    if (sync_needed) {
> +        cpu_synchronize_state(CPU(cpu));
> +    }
>  
>      cpu->compat_pvr = compat_pvr;
>      env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> @@ -162,7 +165,7 @@ static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      SetCompatState *s = arg.host_ptr;
>  
> -    ppc_set_compat(cpu, s->compat_pvr, &s->err);
> +    ppc_set_compat(cpu, s->compat_pvr, true, &s->err);
>  }
>  
>  void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 1d8f2fcd4a46..057785347820 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1364,7 +1364,8 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
>  #if defined(TARGET_PPC64)
>  bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
>                        uint32_t min_compat_pvr, uint32_t max_compat_pvr);
> -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> +                    Error **errp);
>  #if !defined(CONFIG_USER_ONLY)
>  void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
>  #endif
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 20a46c95a596..fda63532b041 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -239,7 +239,7 @@ static int cpu_post_load(void *opaque, int version_id)
>      if (cpu->compat_pvr) {
>          Error *local_err = NULL;
>  
> -        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> +        ppc_set_compat(cpu, cpu->compat_pvr, false, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              error_free(local_err);
> 
> 
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            error_free(local_err);
> > +            return -1;
> > +        }
> > +    } else
> > +#endif
> > +    {
> > +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> > +            return -1;
> > +        }
> > +    }
> > +
> >      env->lr = env->spr[SPR_LR];
> >      env->ctr = env->spr[SPR_CTR];
> >      cpu_write_xer(env, env->spr[SPR_XER]);
> > @@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
> >      }
> >  };
> >  
> > +static bool compat_needed(void *opaque)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +
> > +    return cpu->vhyp != NULL;
> > +}
> > +
> > +static const VMStateDescription vmstate_compat = {
> > +    .name = "cpu/compat",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = compat_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  const VMStateDescription vmstate_ppc_cpu = {
> >      .name = "cpu",
> >      .version_id = 5,
> > @@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
> >          &vmstate_tlb6xx,
> >          &vmstate_tlbemb,
> >          &vmstate_tlbmas,
> > +        &vmstate_compat,
> >          NULL
> >      }
> >  };
> 



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

On Fri, 26 May 2017 14:16:30 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 04, 2017 at 12:07:47PM +0200, Greg Kurz wrote:
> > On Thu, 27 Apr 2017 17:28:43 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > Migrating between different CPU versions is a bit complicated for ppc.
> > > A long time ago, we ensured identical CPU versions at either end by
> > > checking the PVR had the same value.  However, this breaks under KVM
> > > HV, because we always have to use the host's PVR - it's not
> > > virtualized.  That would mean we couldn't migrate between hosts with
> > > different PVRs, even if the CPUs are close enough to compatible in
> > > practice (sometimes identical cores with different surrounding logic
> > > have different PVRs, so this happens in practice quite often).
> > > 
> > > So, we removed the PVR check, but instead checked that several flags
> > > indicating supported instructions matched.  This turns out to be a bad
> > > idea, because those instruction masks are not architected information, but
> > > essentially a TCG implementation detail.  So changes to qemu internal CPU
> > > modelling can break migration - this happened between qemu-2.6 and
> > > qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> > > removal of old migration mistakes".
> > > 
> > > Now, verification of CPU compatibility across a migration basically doesn't
> > > happen.  We simply ignore the PVR of the incoming migration, and hope the
> > > cpu on the destination is close enough to work.
> > > 
> > > Now that we've cleaned up handling of processor compatibility modes for
> > > pseries machine type, we can do better.  We allow migration if:
> > > 
> > >     * The source and destination PVRs are for the same type of CPU, as
> > >       determined by CPU class's pvr_match function
> > > OR  * When the source was in a compatibility mode, and the destination CPU
> > >       supports the same compatibility mode
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 68 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > > index 6cb3a48..20a46c9 100644
> > > --- a/target/ppc/machine.c
> > > +++ b/target/ppc/machine.c
> > > @@ -8,6 +8,7 @@
> > >  #include "helper_regs.h"
> > >  #include "mmu-hash64.h"
> > >  #include "migration/cpu.h"
> > > +#include "qapi/error.h"
> > >  
> > >  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> > >  {
> > > @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
> > >      }
> > >  }
> > >  
> > > +/*
> > > + * Determine if a given PVR is a "close enough" match to the CPU
> > > + * object.  For TCG and KVM PR it would probably be sufficient to
> > > + * require an exact PVR match.  However for KVM HV the user is
> > > + * restricted to a PVR exactly matching the host CPU.  The correct way
> > > + * to handle this is to put the guest into an architected
> > > + * compatibility mode.  However, to allow a more forgiving transition
> > > + * and migration from before this was widely done, we allow migration
> > > + * between sufficiently similar PVRs, as determined by the CPU class's
> > > + * pvr_match() hook.
> > > + */
> > > +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> > > +{
> > > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > +
> > > +    if (pvr == pcc->pvr) {
> > > +        return true;
> > > +    }
> > > +    if (pcc->pvr_match) {
> > > +        return pcc->pvr_match(pcc, pvr);
> > > +    }
> > > +    return false;
> > > +}
> > > +
> > >  static int cpu_post_load(void *opaque, int version_id)
> > >  {
> > >      PowerPCCPU *cpu = opaque;
> > > @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
> > >      target_ulong msr;
> > >  
> > >      /*
> > > -     * We always ignore the source PVR. The user or management
> > > -     * software has to take care of running QEMU in a compatible mode.
> > > +     * If we're operating in compat mode, we should be ok as long as
> > > +     * the destination supports the same compatiblity mode.
> > > +     *
> > > +     * Otherwise, however, we require that the destination has exactly
> > > +     * the same CPU model as the source.
> > >       */
> > > -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> > > +
> > > +#if defined(TARGET_PPC64)
> > > +    if (cpu->compat_pvr) {
> > > +        Error *local_err = NULL;
> > > +
> > > +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);  
> > 
> > As already mentioned during the review of RFCv2, this calls
> > cpu_synchronize_state(CPU(cpu)) and trashes the registers.
> > 
> > The following changes avoid that:  
> 
> This is a really ugly fix, and I think it misses the point.
> 
> If a synchronize_state() trashes state here, it means we've already
> altered register state while not synchronized, which is a pre-existing
> bug.
> 

This is exactly what happens when processing an incoming migration:
1) reset the cpu (clears the cpu dirty flags)
2) "alter register state" according to the migration stream
3) synchronize all registers with cpu_synchronize_all_post_init()

So I'm not sure where we have a pre-existing bug... or maybe document
that cpu_synchronize_state() shouldn't be called when processing
incoming migration (and why should it be since we synchronize all
registers at the end?).

> > 
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -118,7 +118,8 @@ bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> >      return true;
> >  }
> >  
> > -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> > +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> > +                    Error **errp)
> >  {
> >      const CompatInfo *compat = compat_by_pvr(compat_pvr);
> >      CPUPPCState *env = &cpu->env;
> > @@ -138,7 +139,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> >          pcr = compat->pcr;
> >      }
> >  
> > -    cpu_synchronize_state(CPU(cpu));
> > +    if (sync_needed) {
> > +        cpu_synchronize_state(CPU(cpu));
> > +    }
> >  
> >      cpu->compat_pvr = compat_pvr;
> >      env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> > @@ -162,7 +165,7 @@ static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      SetCompatState *s = arg.host_ptr;
> >  
> > -    ppc_set_compat(cpu, s->compat_pvr, &s->err);
> > +    ppc_set_compat(cpu, s->compat_pvr, true, &s->err);
> >  }
> >  
> >  void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 1d8f2fcd4a46..057785347820 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1364,7 +1364,8 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
> >  #if defined(TARGET_PPC64)
> >  bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> >                        uint32_t min_compat_pvr, uint32_t max_compat_pvr);
> > -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
> > +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> > +                    Error **errp);
> >  #if !defined(CONFIG_USER_ONLY)
> >  void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
> >  #endif
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 20a46c95a596..fda63532b041 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -239,7 +239,7 @@ static int cpu_post_load(void *opaque, int version_id)
> >      if (cpu->compat_pvr) {
> >          Error *local_err = NULL;
> >  
> > -        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> > +        ppc_set_compat(cpu, cpu->compat_pvr, false, &local_err);
> >          if (local_err) {
> >              error_report_err(local_err);
> >              error_free(local_err);
> > 
> >   
> > > +        if (local_err) {
> > > +            error_report_err(local_err);
> > > +            error_free(local_err);
> > > +            return -1;
> > > +        }
> > > +    } else
> > > +#endif
> > > +    {
> > > +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> > > +            return -1;
> > > +        }
> > > +    }
> > > +
> > >      env->lr = env->spr[SPR_LR];
> > >      env->ctr = env->spr[SPR_CTR];
> > >      cpu_write_xer(env, env->spr[SPR_XER]);
> > > @@ -560,6 +606,24 @@ static const VMStateDescription vmstate_tlbmas = {
> > >      }
> > >  };
> > >  
> > > +static bool compat_needed(void *opaque)
> > > +{
> > > +    PowerPCCPU *cpu = opaque;
> > > +
> > > +    return cpu->vhyp != NULL;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_compat = {
> > > +    .name = "cpu/compat",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = compat_needed,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >  const VMStateDescription vmstate_ppc_cpu = {
> > >      .name = "cpu",
> > >      .version_id = 5,
> > > @@ -613,6 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
> > >          &vmstate_tlb6xx,
> > >          &vmstate_tlbemb,
> > >          &vmstate_tlbmas,
> > > +        &vmstate_compat,
> > >          NULL
> > >      }
> > >  };  
> >   
> 
> 
>