| Message ID | 1459822643-4770-2-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-04-05 at 12:17 +1000, David Gibson wrote: > From: Cédric Le Goater <clg@fr.ibm.com> > > From: Benjamin Herrenschmidt <benh@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. Beware, iirc, this depends on the new cpu_set_papr() stuff I did so we get the right LPCR values in PAPR mode. Cheers, Ben. > Signed-off-by: Benjamin Herrenschmidt <benh@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 <clg@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 <david@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 |
On Tue, Apr 05, 2016 at 12:19:50PM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2016-04-05 at 12:17 +1000, David Gibson wrote: > > From: Cédric Le Goater <clg@fr.ibm.com> > > > > From: Benjamin Herrenschmidt <benh@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. > > Beware, iirc, this depends on the new cpu_set_papr() stuff I did so we > get the right LPCR values in PAPR mode. Right, Cédric already submitted that before the 2.6 freeze, and it was merged as 26a7f129, AFAICT.
On 04/05/2016 05:25 AM, David Gibson wrote: > On Tue, Apr 05, 2016 at 12:19:50PM +1000, Benjamin Herrenschmidt wrote: >> On Tue, 2016-04-05 at 12:17 +1000, David Gibson wrote: >>> From: Cédric Le Goater <clg@fr.ibm.com> >>> >>> From: Benjamin Herrenschmidt <benh@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. >> >> Beware, iirc, this depends on the new cpu_set_papr() stuff I did so we >> get the right LPCR values in PAPR mode. > > Right, Cédric already submitted that before the 2.6 freeze, and it was > merged as 26a7f129, AFAICT. Well, yes, but cpu_ppc_set_papr() only handles the AMOR setting, the LPCR settings were kept for later as they were not bug fixes. As for now, powerpc_excp() checks the ILE bit and uses the AIL bits to calculate the vector address, which was done before in the H_SET_MODE hcall. This allows some simplification, getting rid of excp_prefix, and fixes a migration bug in TCG. C.
On Tue, 2016-04-05 at 09:03 +0200, Cédric Le Goater wrote: > > Well, yes, but cpu_ppc_set_papr() only handles the AMOR setting, the LPCR > settings were kept for later as they were not bug fixes. > > As for now, powerpc_excp() checks the ILE bit and uses the AIL bits to > calculate the vector address, which was done before in the H_SET_MODE > hcall. This allows some simplification, getting rid of excp_prefix, > and fixes a migration bug in TCG. Ah I see, the patch I'm commenting on doesn't actually have my lpes0/1 changes, so it should be ok. Cheers, Ben.
On 05/04/2016 04:17, David Gibson wrote: > From: Cédric Le Goater <clg@fr.ibm.com> > > From: Benjamin Herrenschmidt <benh@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? Laurent > > Signed-off-by: Benjamin Herrenschmidt <benh@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 <clg@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 <david@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 | >
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 <clg@fr.ibm.com> >> >> From: Benjamin Herrenschmidt <benh@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. Thanks, C. > > Laurent >> >> Signed-off-by: Benjamin Herrenschmidt <benh@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 <clg@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 <david@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 | >> >
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 <clg@fr.ibm.com> >>> >>> From: Benjamin Herrenschmidt <benh@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 But I don't know how behaves really a POWER8. Laurent > Thanks, > > C. > >> >> Laurent >>> >>> Signed-off-by: Benjamin Herrenschmidt <benh@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 <clg@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 <david@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 | >>> >> >
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 <clg@fr.ibm.com> >>>> >>>> From: Benjamin Herrenschmidt <benh@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 <benh@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 <clg@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 <david@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 | >>>> >>> >> >
On Thu, Apr 07, 2016 at 12:27:34PM +0200, 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 <clg@fr.ibm.com> > >> > >> From: Benjamin Herrenschmidt <benh@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. Right.. it's kind of a case of two bugs cancelling each other out, but the end result is correct. The initial NIP should include the AIL from the LPCR.. but the AIL should be cleared on reset, so it makes no practical difference. So in 2.7 it would certainly be good to clean this up for clarity if nothing else, but I don't see something that needs fixing in the 2.6 timeframe.
On Thu, Apr 07, 2016 at 12:45:41PM +0200, 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 <clg@fr.ibm.com> > >>> > >>> From: Benjamin Herrenschmidt <benh@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 > > But I don't know how behaves really a POWER8. I'm pretty certain the previous behaviour was wrong. The LPCR AIL bits shouldn't survive a reset, so the new NIP after the reset should be based on a cleared AIL.
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 |