On 04/04/2016 06:16 AM, David Gibson wrote: > On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric Le Goater wrote: >> On 04/01/2016 04:43 AM, David Gibson wrote: >>> 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 <c...@fr.ibm.com> >>>>> >>>>> 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). >> >> Sure. >> >> But first, could you please take a look at a reworked patch of Ben who >> had forseen the issue ? I kept the AIL fix, added some extra for ILE >> and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine. >> >> If you think this is too risky for 2.6, I will work on what you asked for. > > Ah, yes this approach does seem more correct. It looks like it leaves > room for further cleanups to excp_prefix later (such as completely > removing it in favour of calculating it from reg values at exception > time).
Yes. There is still a : vector |= env->excp_prefix; which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used. The code section calculating 'vector' in powerpc_excp() could be isolated in its own routine I guess. We could clarify things there. > But for now it looks like it will address the migration issue at hand. > > I've applied it to ppc-for-2.6. Tell me if you want a proper resend by mail. Thanks, C. >> >>> 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. >> >> >> Thanks, >> >> C. >> >> >From bb9d1e06a0ea2132902db724f61c8dc655aa1a96 Mon Sep 17 00:00:00 2001 >> From: Benjamin Herrenschmidt <b...@kernel.crashing.org> >> Date: Thu, 4 Jun 2015 03:39:19 +1000 >> Subject: [PATCH 18/77] ppc: Rework POWER7 & POWER8 exception model >> >> This patch fixes the current AIL implementation for POWER8. The >> interrupt vector address can be calculated directly from LPCR when the >> exception is handled. The excp_prefix update becomes useless and we >> can cleanup the H_SET_MODE hcall. >> >> Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org> >> [clg: Removed LPES0/1 handling for HV vs. !HV >> Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ] >> Signed-off-by: Cédric Le Goater <c...@fr.ibm.com> >> --- >> hw/ppc/spapr_hcall.c | 16 -------------- >> include/hw/ppc/spapr.h | 5 ---- >> target-ppc/cpu.h | 10 ++++++++ >> target-ppc/excp_helper.c | 49 >> ++++++++++++++++++++++++++++++++++++++++++-- >> target-ppc/translate_init.c | 2 - >> 5 files changed, 59 insertions(+), 23 deletions(-) >> >> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h >> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h >> @@ -167,6 +167,8 @@ enum powerpc_excp_t { >> POWERPC_EXCP_970, >> /* POWER7 exception model */ >> POWERPC_EXCP_POWER7, >> + /* POWER8 exception model */ >> + POWERPC_EXCP_POWER8, >> #endif /* defined(TARGET_PPC64) */ >> }; >> >> @@ -2277,6 +2279,14 @@ enum { >> HMER_XSCOM_STATUS_LSH = (63 - 23), >> }; >> >> +/* Alternate Interrupt Location (AIL) */ >> +enum { >> + AIL_NONE = 0, >> + AIL_RESERVED = 1, >> + AIL_0001_8000 = 2, >> + AIL_C000_0000_0000_4000 = 3, >> +}; >> + >> >> /*****************************************************************************/ >> >> static inline target_ulong cpu_read_xer(CPUPPCState *env) >> Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c >> +++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c >> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC >> CPUPPCState *env = &cpu->env; >> target_ulong msr, new_msr, vector; >> int srr0, srr1, asrr0, asrr1; >> - int lpes0, lpes1, lev; >> + int lpes0, lpes1, lev, ail; >> >> if (0) { >> /* XXX: find a suitable condition to enable the hypervisor mode */ >> @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC >> asrr0 = -1; >> asrr1 = -1; >> >> + /* Exception targetting modifiers >> + * >> + * AIL is initialized here but can be cleared by >> + * selected exceptions >> + */ >> +#if defined(TARGET_PPC64) >> + if (excp_model == POWERPC_EXCP_POWER7 || >> + excp_model == POWERPC_EXCP_POWER8) { >> + if (excp_model == POWERPC_EXCP_POWER8) { >> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; >> + } else { >> + ail = 0; >> + } >> + } else >> +#endif /* defined(TARGET_PPC64) */ >> + { >> + ail = 0; >> + } >> + >> switch (excp) { >> case POWERPC_EXCP_NONE: >> /* Should never happen */ >> @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC >> /* XXX: find a suitable condition to enable the hypervisor mode >> */ >> new_msr |= (target_ulong)MSR_HVB; >> } >> + ail = 0; >> >> /* machine check exceptions don't have ME set */ >> new_msr &= ~((target_ulong)1 << MSR_ME); >> @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC >> /* XXX: find a suitable condition to enable the hypervisor mode >> */ >> new_msr |= (target_ulong)MSR_HVB; >> } >> + ail = 0; >> goto store_next; >> case POWERPC_EXCP_DSEG: /* Data segment exception >> */ >> if (lpes1 == 0) { >> @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC >> } >> >> #ifdef TARGET_PPC64 >> - if (excp_model == POWERPC_EXCP_POWER7) { >> + if (excp_model == POWERPC_EXCP_POWER7 || >> + excp_model == POWERPC_EXCP_POWER8) { >> if (env->spr[SPR_LPCR] & LPCR_ILE) { >> new_msr |= (target_ulong)1 << MSR_LE; >> } >> @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC >> excp); >> } >> vector |= env->excp_prefix; >> + >> + /* AIL only works if there is no HV transition and we are running with >> + * translations enabled >> + */ >> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { >> + ail = 0; >> + } >> + /* Handle AIL */ >> + if (ail) { >> + new_msr |= (1 << MSR_IR) | (1 << MSR_DR); >> + switch(ail) { >> + case AIL_0001_8000: >> + vector |= 0x18000; >> + break; >> + case AIL_C000_0000_0000_4000: >> + vector |= 0xc000000000004000ull; >> + break; >> + default: >> + cpu_abort(cs, "Invalid AIL combination %d\n", ail); >> + break; >> + } >> + } >> + >> #if defined(TARGET_PPC64) >> if (excp_model == POWERPC_EXCP_BOOKE) { >> if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) { >> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c >> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c >> @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, >> pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; >> pcc->sps = &POWER7_POWER8_sps; >> #endif >> - pcc->excp_model = POWERPC_EXCP_POWER7; >> + pcc->excp_model = POWERPC_EXCP_POWER8; >> pcc->bus_model = PPC_FLAGS_INPUT_POWER7; >> pcc->bfd_mach = bfd_mach_ppc64; >> pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | >> 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 >> @@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_ >> { >> CPUState *cs; >> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); >> - target_ulong prefix; >> >> if (!(pcc->insns_flags2 & PPC2_ISA207S)) { >> return H_P2; >> @@ -835,25 +834,12 @@ 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: >> + if (mflags == AIL_RESERVED) { >> return H_UNSUPPORTED_FLAG; >> } >> >> CPU_FOREACH(cs) { >> - CPUPPCState *env = &POWERPC_CPU(cpu)->env; >> - >> set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL); >> - env->excp_prefix = prefix; >> } >> >> return H_SUCCESS; >> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h >> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h >> @@ -204,11 +204,6 @@ struct sPAPRMachineState { >> #define H_SET_MODE_ENDIAN_BIG 0 >> #define H_SET_MODE_ENDIAN_LITTLE 1 >> >> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */ >> -#define H_SET_MODE_ADDR_TRANS_NONE 0 >> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2 >> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3 >> - >> /* VASI States */ >> #define H_VASI_INVALID 0 >> #define H_VASI_ENABLED 1 >> >