Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR

[David Gibson]

On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
> On 31/03/16 05:55, David Gibson wrote:
> 
> > On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
> >> This address is changed by the linux kernel using the H_SET_MODE hcall
> >> and needs to be restored when migrating a spapr VM running in
> >> TCG. This can be done using the AIL bits from the LPCR register.
> >>
> >> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> >> returns the effective address offset of the interrupt handler
> >> depending on the LPCR_AIL bits. The same helper can be used in the
> >> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> >> defines.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> > 
> > I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> > since it certainly improves behaviour, although I have a couple of
> > queries:
> > 
> >> ---
> >>
> >>  Changes since v1:
> >>
> >>  - moved helper routine under target-ppc/
> >>  - moved the restore of excp_prefix under cpu_post_load()
> >>
> >>  hw/ppc/spapr_hcall.c        |   13 ++-----------
> >>  include/hw/ppc/spapr.h      |    5 -----
> >>  target-ppc/cpu.h            |    9 +++++++++
> >>  target-ppc/machine.c        |   20 +++++++++++++++++++-
> >>  target-ppc/translate_init.c |   14 ++++++++++++++
> >>  5 files changed, 44 insertions(+), 17 deletions(-)
> >>
> >> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >> ===================================================================
> >> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> >> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> >>          return H_P4;
> >>      }
> >>  
> >> -    switch (mflags) {
> >> -    case H_SET_MODE_ADDR_TRANS_NONE:
> >> -        prefix = 0;
> >> -        break;
> >> -    case H_SET_MODE_ADDR_TRANS_0001_8000:
> >> -        prefix = 0x18000;
> >> -        break;
> >> -    case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> >> -        prefix = 0xC000000000004000ULL;
> >> -        break;
> >> -    default:
> >> +    prefix = cpu_ppc_get_excp_prefix(mflags);
> >> +    if (prefix == (target_ulong) -1ULL) {
> >>          return H_UNSUPPORTED_FLAG;
> >>      }
> >>  
> >> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >> ===================================================================
> >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> >> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> >>      }
> >>  }
> >>  
> >> +
> >> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> >> +{
> >> +    int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> >> +    target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> >> +
> >> +    if (prefix == (target_ulong) -1ULL) {
> >> +        return -EINVAL;
> >> +    }
> >> +    env->excp_prefix = prefix;
> >> +    return 0;
> >> +}
> >> +
> >>  static int cpu_post_load(void *opaque, int version_id)
> >>  {
> >>      PowerPCCPU *cpu = opaque;
> >>      CPUPPCState *env = &cpu->env;
> >>      int i;
> >>      target_ulong msr;
> >> +    int ret = 0;
> >>  
> >>      /*
> >>       * We always ignore the source PVR. The user or management
> >> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
> >>  
> >>      hreg_compute_mem_idx(env);
> >>  
> >> -    return 0;
> >> +    if (env->spr[SPR_LPCR] & LPCR_AIL) {
> >> +        ret = cpu_post_load_excp_prefix(env);
> >> +    }
> > 
> > Why not call this unconditionally?  If AIL == 0 it will still do the
> > right thing.
> > 
> > Aren't there also circumstances where the exception prefix can depend
> > on the MSR?  Do those need to be handled somewhere?
> 
> Yes indeed - this was part of my patchset last year to fix up various
> migration issues for the Mac PPC machines (see commit
> 2360b6e84f78d41fa0f76555a947148b73645259).
> 
> I agree that having the env->excp_prefix logic split like this isn't a
> particularly great idea. Let's just have a single helper function as in
> the patch above and use that in both places (and in fact with recent
> changes I have a feeling env->excp_prefix is one of the few remaining
> reasons that the above change is still required, but please check this).

Right.

So, what I'd really prefer to see here is a single
update_excp_prefix() helper which will set env->excp_prefix based on
whatever registers are relevant (LPCR, MSR and probably the cpu
model).  That would be called on incoming migration, and after
updating any of the relevant registers (so from the mtmsr and mtlpcr
emulations).  H_SET_MODE should be handled by first updating the LPCR
value, then calling the update helper.

Cédric, I'm ok if you provide a version of that which only handles
POWER7 and POWER8 (i.e. spapr compatible models for now).  Mark - if
you can supply corrections to that outline for Macintosh era models,
that would be great.

I do want to get the basic migration problem fixed before 2.6 is
release.

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