On 04/07/2016 12:45 PM, Laurent Vivier wrote: > > > On 07/04/2016 12:27, Cédric Le Goater wrote: >> Hello Laurent, >> >> On 04/07/2016 11:13 AM, Laurent Vivier wrote: >>> >>> >>> On 05/04/2016 04:17, David Gibson wrote: >>>> From: Cédric Le Goater <c...@fr.ibm.com> >>>> >>>> From: Benjamin Herrenschmidt <b...@kernel.crashing.org> >>>> >>>> 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. >>> >>> I know it's a little bit late to comment this patch but: >>> >>> what about the initialization of the NIP in ppc_cpu_reset()? >>> >>> env->nip = env->hreset_vector | env->excp_prefix; >>> >>> on POWER8 "env->excp_prefix" is always 0, but LPCR can have an AIL defined? >> >> yes. env->spr[SPR_LPCR] still has the previous value at that time and >> it is reseted right below in the same routine. >> >> The cpu should restart in a valid state after that and later on, use the >> H_SET_MODE hcall from the guest kernel to set the AIL bits back in LPCR. >> It looks fine to me but I might be missing something. > > What I mean is if we want to keep the previous behavior we should have > something like: > > env->nip = env->hreset_vector | env->excp_prefix; > #if defined(TARGET_PPC64) > switch((env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT) { > case AIL_0001_8000: > env->nip |= 0x18000; > break; > case AIL_C000_0000_0000_4000: > env->nip |= 0xc000000000004000ull; > break; > } > #endif
Yes. This is correct but the env->nip assigned in ppc_cpu_reset() is ignored and replaced with another value. On spapr guests, the first CPU is started on 0x100, see ppc_spapr_reset(), and then, this CPU starts the others with a firmware call: rtas_start_cpu(). Previously, when env->excp_prefix used to hold the interrupt vector address, the env->nip value was bogus but no one cared. Hopefully, the current state is a little better, the values are correct even if they are not used. But we still need to do a cleanup in that area. Thanks, C. > But I don't know how behaves really a POWER8. > > Laurent > >> Thanks, >> >> C. >> >>> >>> Laurent >>>> >>>> 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> >>>> [dwg: This was written as a cleanup, but it also fixes a real bug >>>> where setting an alternative interrupt location would not be >>>> correctly migrated] >>>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> >>>> --- >>>> 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(-) >>>> >>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >>>> index 2dcb676..8f40602 100644 >>>> --- a/hw/ppc/spapr_hcall.c >>>> +++ b/hw/ppc/spapr_hcall.c >>>> @@ -824,7 +824,6 @@ static target_ulong >>>> h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu, >>>> { >>>> CPUState *cs; >>>> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); >>>> - target_ulong prefix; >>>> >>>> if (!(pcc->insns_flags2 & PPC2_ISA207S)) { >>>> return H_P2; >>>> @@ -836,25 +835,12 @@ static target_ulong >>>> h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu, >>>> 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; >>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>>> index 098d85d..815d5ee 100644 >>>> --- a/include/hw/ppc/spapr.h >>>> +++ b/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 >>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h >>>> index 676081e..9d4e43c 100644 >>>> --- a/target-ppc/cpu.h >>>> +++ b/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) >>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >>>> index c890853..ca4ffe8 100644 >>>> --- a/target-ppc/excp_helper.c >>>> +++ b/target-ppc/excp_helper.c >>>> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int >>>> excp_model, int excp) >>>> 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(PowerPCCPU *cpu, int >>>> excp_model, int excp) >>>> 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(PowerPCCPU *cpu, int >>>> excp_model, int excp) >>>> /* 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(PowerPCCPU *cpu, int >>>> excp_model, int excp) >>>> /* 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(PowerPCCPU *cpu, int >>>> excp_model, int excp) >>>> } >>>> >>>> #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(PowerPCCPU *cpu, int >>>> excp_model, int excp) >>>> 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) { >>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >>>> index 0a33597..f515725 100644 >>>> --- a/target-ppc/translate_init.c >>>> +++ b/target-ppc/translate_init.c >>>> @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) >>>> 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 | >>>> >>> >> >