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

[David Gibson]

On Thu, Nov 10, 2016 at 05:55:36PM -0600, Michael Roth wrote:
> 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 <address@hidden>
> > > >>> ---
> > > >>>  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.

So, since I've already given up on backwards migration in other cases,
I've changed this in my next version to simply (cpu->vhyp != NULL), so
the compat mode (including raw) is always transferred on pseries.

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

Not enough that it's worth the considerable extra difficulty it
entails, I think.

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

signature.asc
Description: PGP signature