Re: [Qemu-devel] [RFC 12/17] ppc: Migrate compatibility mode

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting David Gibson (2016-11-09 19:59:37)
> On Tue, Nov 08, 2016 at 04:51:10PM +1100, Alexey Kardashevskiy wrote:
> > On 08/11/16 16:19, David Gibson wrote:
> > > On Fri, Nov 04, 2016 at 04:58:47PM +1100, Alexey Kardashevskiy wrote:
> > >> On 30/10/16 22:12, David Gibson wrote:
> > >>> Server-class POWER CPUs can be put into several compatibility modes.  These
> > >>> can be specified on the command line, or negotiated by the guest during
> > >>> boot.
> > >>>
> > >>> Currently we don't migrate the compatibility mode, which means after a
> > >>> migration the guest will revert to running with whatever compatibility
> > >>> mode (or none) specified on the command line.
> > >>>
> > >>> With the limited range of CPUs currently used, this doesn't usually cause
> > >>> a problem, but it could.  Fix this by adding the compatibility mode (if
> > >>> set) to the migration stream.
> > >>>
> > >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > >>> ---
> > >>>  target-ppc/machine.c | 34 ++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 34 insertions(+)
> > >>>
> > >>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> > >>> index 4820f22..5d87ff6 100644
> > >>> --- a/target-ppc/machine.c
> > >>> +++ b/target-ppc/machine.c
> > >>> @@ -9,6 +9,7 @@
> > >>>  #include "mmu-hash64.h"
> > >>>  #include "migration/cpu.h"
> > >>>  #include "exec/exec-all.h"
> > >>> +#include "qapi/error.h"
> > >>>  
> > >>>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> > >>>  {
> > >>> @@ -176,6 +177,20 @@ static int cpu_post_load(void *opaque, int version_id)
> > >>>       * software has to take care of running QEMU in a compatible mode.
> > >>>       */
> > >>>      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;
> > >>> +        }
> > >>> +    }
> > >>> +#endif
> > >>> +
> > >>>      env->lr = env->spr[SPR_LR];
> > >>>      env->ctr = env->spr[SPR_CTR];
> > >>>      cpu_write_xer(env, env->spr[SPR_XER]);
> > >>> @@ -528,6 +543,24 @@ static const VMStateDescription vmstate_tlbmas = {
> > >>>      }
> > >>>  };
> > >>>  
> > >>> +static bool compat_needed(void *opaque)
> > >>> +{
> > >>> +    PowerPCCPU *cpu = opaque;
> > >>> +
> > >>> +    return cpu->compat_pvr != 0;
> > >>
> > >>
> > >> Finally got to trying how this affects migration :)
> > >>
> > >> This breaks migration to QEMU <=2.7, and it should not at least when both
> > >> source and destination are running with  -cpu host,compat=power7.
> > > 
> > > IIUC, we don't generally try to maintain backwards migration, even for
> > > old machine types.
> > 
> > 
> > I thought the opposite - we generally try to maintain it, this is pretty
> > much why we use these subsections in cases like this; otherwise you could
> > just add a new field and bump the vmstate_ppc_cpu.version.
> 
> Hm, I guess that's true.  We do at least try to allow backwards
> migration, we just don't insist on it.  The example here partially
> suceeds - it will allow backwards migration for CPUs in raw mode.

What if we changed the condition to cpu->compat_pvr != cpu->max_compat?
Assuming each end sets compat=powerX (which seems like a reasonable
requirement for migration), it seems like in  most cases the guest would end
up negotiating max_compat anyway. It's only in cases where it doesn't that we
end up in an ambiguous state.

Newer QEMU would guard against this by migrating it's compat_pvr
accordingly (which will probably become more important with p9 guests
potentially running older kernels), but otherwise it seems like we could
maintain backwards migration in most cases.

Although with the proposed version bump for vmstate_spapr_pci I guess
the discussion is somewhat moot unless we decide we rely want to
maintain backward migration...

> 
> I'll look at just bumping the version instead.
> 
> > 
> > 
> > > 
> > >>
> > >>
> > >>> +}
> > >>> +
> > >>> +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,
> > >>> @@ -580,6 +613,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