This fixes a powernv9 bug, and several POWER10 bits (which affect powernv10 and pseries power10). I think I got the logic right but I didn't individually test and verify every case, so any glances over it would be appreciated. Thanks, Nick Nicholas Piggin (3): target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour target/ppc: POWER10 supports scv target/ppc: Add POWER10 exception model hw/ppc/spapr_hcall.c | 5 ++ target/ppc/cpu-qom.h | 2 + target/ppc/cpu.h | 5 +- target/ppc/excp_helper.c | 111 ++++++++++++++++++++++++++++---- target/ppc/translate.c | 3 +- target/ppc/translate_init.c.inc | 4 +- 6 files changed, 111 insertions(+), 19 deletions(-) -- 2.23.0
ISA v3.0 radix guest execution has a quirk in AIL behaviour such that the LPCR[AIL] value can apply to hypervisor interrupts. This affects machines that emulate HV=1 mode (i.e., powernv9). Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- target/ppc/excp_helper.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 85de7e6c90..b8881c0f85 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) #endif /* - * AIL only works if there is no HV transition and we are running - * with translations enabled + * AIL only works if MSR[IR] and MSR[DR] are both enabled. */ - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) || - ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) { + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { ail = 0; } + /* + * AIL does not work if there is a MSR[HV] 0->1 transition and the + * partition is in HPT mode. For radix guests, such interrupts are + * allowed to be delivered to the hypervisor in ail mode. + */ + if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { + ail = 0; + } + } + vector = env->excp_vectors[excp]; if (vector == (target_ulong)-1ULL) { cpu_abort(cs, "Raised an exception without defined vector %d\n", -- 2.23.0
This must have slipped through the cracks between adding POWER10 support and scv support. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- target/ppc/translate_init.c.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc index c03a7c4f52..70f9b9b150 100644 --- a/target/ppc/translate_init.c.inc +++ b/target/ppc/translate_init.c.inc @@ -9323,7 +9323,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data) pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | POWERPC_FLAG_BE | POWERPC_FLAG_PMM | POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR | - POWERPC_FLAG_VSX | POWERPC_FLAG_TM; + POWERPC_FLAG_VSX | POWERPC_FLAG_TM | POWERPC_FLAG_SCV; pcc->l1_dcache_size = 0x8000; pcc->l1_icache_size = 0x8000; pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr; -- 2.23.0
POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL], and it removes support for the LPCR[AIL]=0b10 mode. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/ppc/spapr_hcall.c | 5 ++ target/ppc/cpu-qom.h | 2 + target/ppc/cpu.h | 5 +- target/ppc/excp_helper.c | 114 ++++++++++++++++++++++++++------ target/ppc/translate.c | 3 +- target/ppc/translate_init.c.inc | 2 +- 6 files changed, 107 insertions(+), 24 deletions(-) diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 7b5cd3553c..2f65f20f72 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu, return H_UNSUPPORTED_FLAG; } + if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) { + /* AIL 2 is also reserved in ISA v3.1 */ + return H_UNSUPPORTED_FLAG; + } + spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL); return H_SUCCESS; diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h index 118baf8d41..06b6571bc9 100644 --- a/target/ppc/cpu-qom.h +++ b/target/ppc/cpu-qom.h @@ -116,6 +116,8 @@ enum powerpc_excp_t { POWERPC_EXCP_POWER8, /* POWER9 exception model */ POWERPC_EXCP_POWER9, + /* POWER10 exception model */ + POWERPC_EXCP_POWER10, }; /*****************************************************************************/ diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index e73416da68..24e53e0ac3 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t { #define LPCR_PECE_U_SHIFT (63 - 19) #define LPCR_PECE_U_MASK (0x7ull << LPCR_PECE_U_SHIFT) #define LPCR_HVEE PPC_BIT(17) /* Hypervisor Virt Exit Enable */ -#define LPCR_RMLS_SHIFT (63 - 37) +#define LPCR_RMLS_SHIFT (63 - 37) /* RMLS (removed in ISA v3.0) */ #define LPCR_RMLS (0xfull << LPCR_RMLS_SHIFT) +#define LPCR_HAIL PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */ #define LPCR_ILE PPC_BIT(38) -#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */ +#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */ #define LPCR_AIL (3ull << LPCR_AIL_SHIFT) #define LPCR_UPRT PPC_BIT(41) /* Use Process Table */ #define LPCR_EVIRT PPC_BIT(42) /* Enhanced Virtualisation */ diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index b8881c0f85..e8bf0598b4 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -197,7 +197,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) CPUState *cs = CPU(cpu); CPUPPCState *env = &cpu->env; target_ulong msr, new_msr, vector; - int srr0, srr1, asrr0, asrr1, lev = -1, ail; + int srr0, srr1, asrr0, asrr1, lev = -1; + int ail, hail, using_ail; bool lpes0; qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx @@ -239,24 +240,39 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) * On anything else, we behave as if LPES0 is 1 * (externals don't alter MSR:HV) * - * AIL is initialized here but can be cleared by - * selected exceptions + * ail/hail are initialized here but can be cleared by + * selected exceptions, and other conditions afterwards. */ #if defined(TARGET_PPC64) if (excp_model == POWERPC_EXCP_POWER7 || excp_model == POWERPC_EXCP_POWER8 || - excp_model == POWERPC_EXCP_POWER9) { + excp_model == POWERPC_EXCP_POWER9 || + excp_model == POWERPC_EXCP_POWER10) { lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0); if (excp_model != POWERPC_EXCP_POWER7) { ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; } else { ail = 0; } + if (excp_model == POWERPC_EXCP_POWER10) { + if (AIL_0001_8000) { + ail = 0; /* AIL=2 is reserved in ISA v3.1 */ + } + + if (env->spr[SPR_LPCR] & LPCR_HAIL) { + hail = AIL_C000_0000_0000_4000; + } else { + hail = 0; + } + } else { + hail = ail; + } } else #endif /* defined(TARGET_PPC64) */ { lpes0 = true; ail = 0; + hail = 0; } /* @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) new_msr |= (target_ulong)MSR_HVB; } ail = 0; + hail = 0; /* machine check exceptions don't have ME set */ new_msr &= ~((target_ulong)1 << MSR_ME); @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } } ail = 0; + hail = 0; break; case POWERPC_EXCP_DSEG: /* Data segment exception */ case POWERPC_EXCP_ISEG: /* Instruction segment exception */ @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } else if (env->spr[SPR_LPCR] & LPCR_ILE) { new_msr |= (target_ulong)1 << MSR_LE; } - } else if (excp_model == POWERPC_EXCP_POWER9) { + } else if (excp_model == POWERPC_EXCP_POWER9 || + excp_model == POWERPC_EXCP_POWER10) { if (new_msr & MSR_HVB) { if (env->spr[SPR_HID0] & HID0_POWER9_HILE) { new_msr |= (target_ulong)1 << MSR_LE; @@ -791,22 +810,77 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) #endif /* - * AIL only works if MSR[IR] and MSR[DR] are both enabled. + * The ail variable is for AIL behaviour for interrupts that begin + * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for + * interrupts that begin execution with MSR[HV]=1. + * + * There is a large matrix of behaviours depending on the processor and + * the current operating modes when the interrupt is taken, and the + * interrupt type. The below tables lists behaviour for interrupts except + * for SRESET, machine check, and HMI (which are all taken in real mode + * with AIL 0). + * + * POWER8, POWER9 with LPCR[HR]=0 + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | + * +-----------+-------------+---------+-------------+-----+ + * | a | 00/01/10 | x | x | 0 | + * | a | 11 | 0 | 1 | 0 | + * | a | 11 | 1 | 1 | a | + * | a | 11 | 0 | 0 | a | + * +-------------------------------------------------------+ + * + * POWER9 with LPCR[HR]=1 + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | + * +-----------+-------------+---------+-------------+-----+ + * | a | 00/01/10 | x | x | 0 | + * | a | 11 | x | x | a | + * +-------------------------------------------------------+ + * + * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be + * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1). + * This is good for performance but allows the guest to influence the + * AIL of the hypervisor interrupt using its MSR, and also the hypervisor + * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the + * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts. + * + * POWER10 addresses those issues with a new LPCR[HAIL] bit that is + * applied to interrupt that begin execution with MSR[HV]=1 (so both + * MSR[HV] 0->1 and 1->1). + * + * POWER10 behaviour is + * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | + * +-----------+------------+-------------+---------+-------------+-----+ + * | a | h | 00/01/10 | 0 | 0 | 0 | + * | a | h | 11 | 0 | 0 | a | + * | a | h | x | 0 | 1 | h | + * | a | h | 00/01/10 | 1 | 1 | 0 | + * | a | h | 11 | 1 | 1 | h | + * +--------------------------------------------------------------------+ */ - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { - ail = 0; - } - /* - * AIL does not work if there is a MSR[HV] 0->1 transition and the - * partition is in HPT mode. For radix guests, such interrupts are - * allowed to be delivered to the hypervisor in ail mode. - */ - if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { - if (!(env->spr[SPR_LPCR] & LPCR_HR)) { + if (excp_model == POWERPC_EXCP_POWER8 || + excp_model == POWERPC_EXCP_POWER9) { + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { ail = 0; + hail = 0; + } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { + hail = 0; + } + } + } else if (excp_model == POWERPC_EXCP_POWER10) { + if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) { + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { + ail = 0; + hail = 0; + } } } + if (new_msr & MSR_HVB) { + using_ail = hail; + } else { + using_ail = ail; + } vector = env->excp_vectors[excp]; if (vector == (target_ulong)-1ULL) { @@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) env->spr[srr1] = msr; /* Handle AIL */ - if (ail) { + if (using_ail) { new_msr |= (1 << MSR_IR) | (1 << MSR_DR); - vector |= ppc_excp_vector_offset(cs, ail); + vector |= ppc_excp_vector_offset(cs, using_ail); } #if defined(TARGET_PPC64) } else { /* scv AIL is a little different */ - if (ail) { + if (using_ail) { new_msr |= (1 << MSR_IR) | (1 << MSR_DR); } - if (ail == AIL_C000_0000_0000_4000) { + if (using_ail == AIL_C000_0000_0000_4000) { vector |= 0xc000000000003000ull; } else { vector |= 0x0000000000017000ull; diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 0984ce637b..e9ed001229 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -7731,7 +7731,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags) #if defined(TARGET_PPC64) if (env->excp_model == POWERPC_EXCP_POWER7 || env->excp_model == POWERPC_EXCP_POWER8 || - env->excp_model == POWERPC_EXCP_POWER9) { + env->excp_model == POWERPC_EXCP_POWER9 || + env->excp_model == POWERPC_EXCP_POWER10) { qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n", env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]); } diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc index 70f9b9b150..5d427e9d38 100644 --- a/target/ppc/translate_init.c.inc +++ b/target/ppc/translate_init.c.inc @@ -9317,7 +9317,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data) pcc->radix_page_info = &POWER10_radix_page_info; pcc->lrg_decr_bits = 56; #endif - pcc->excp_model = POWERPC_EXCP_POWER9; + pcc->excp_model = POWERPC_EXCP_POWER10; pcc->bus_model = PPC_FLAGS_INPUT_POWER9; pcc->bfd_mach = bfd_mach_ppc64; pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | -- 2.23.0
Nicholas Piggin <npiggin@gmail.com> writes: > ISA v3.0 radix guest execution has a quirk in AIL behaviour such that > the LPCR[AIL] value can apply to hypervisor interrupts. > > This affects machines that emulate HV=1 mode (i.e., powernv9). > Ah ok, so we actually want to replicate the quirk in the pnv machine. Took me a while. Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > target/ppc/excp_helper.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 85de7e6c90..b8881c0f85 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > #endif > > /* > - * AIL only works if there is no HV transition and we are running > - * with translations enabled > + * AIL only works if MSR[IR] and MSR[DR] are both enabled. > */ > - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) || > - ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) { > + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > ail = 0; > } > > + /* > + * AIL does not work if there is a MSR[HV] 0->1 transition and the > + * partition is in HPT mode. For radix guests, such interrupts are > + * allowed to be delivered to the hypervisor in ail mode. > + */ > + if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { > + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { > + ail = 0; > + } > + } > + > vector = env->excp_vectors[excp]; > if (vector == (target_ulong)-1ULL) { > cpu_abort(cs, "Raised an exception without defined vector %d\n",
Nick, I really prefer clg@kaod.org :) On 4/13/21 2:54 PM, Nicholas Piggin wrote: > This must have slipped through the cracks between adding POWER10 support > and scv support. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> and Tested-by: Cédric Le Goater <clg@kaod.org> on a recently enabled scv enabled distro. Thanks, C. > --- > target/ppc/translate_init.c.inc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc > index c03a7c4f52..70f9b9b150 100644 > --- a/target/ppc/translate_init.c.inc > +++ b/target/ppc/translate_init.c.inc > @@ -9323,7 +9323,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data) > pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | > POWERPC_FLAG_BE | POWERPC_FLAG_PMM | > POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR | > - POWERPC_FLAG_VSX | POWERPC_FLAG_TM; > + POWERPC_FLAG_VSX | POWERPC_FLAG_TM | POWERPC_FLAG_SCV; > pcc->l1_dcache_size = 0x8000; > pcc->l1_icache_size = 0x8000; > pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr; >
On 4/13/21 2:54 PM, Nicholas Piggin wrote: > ISA v3.0 radix guest execution has a quirk in AIL behaviour such that > the LPCR[AIL] value can apply to hypervisor interrupts. Shouldn't we test for P9 ? But I think you are using a new exception model for P10 in the next patch. I guess it's ok for now. > > This affects machines that emulate HV=1 mode (i.e., powernv9). > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > target/ppc/excp_helper.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 85de7e6c90..b8881c0f85 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > #endif > > /* > - * AIL only works if there is no HV transition and we are running > - * with translations enabled > + * AIL only works if MSR[IR] and MSR[DR] are both enabled. > */ > - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) || > - ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) { > + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > ail = 0; > } > > + /* > + * AIL does not work if there is a MSR[HV] 0->1 transition and the > + * partition is in HPT mode. For radix guests, such interrupts are > + * allowed to be delivered to the hypervisor in ail mode. > + */ > + if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { > + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { We have a ppc64_v3_radix() helper but this is minor. C. > + ail = 0; > + } > + } > + > vector = env->excp_vectors[excp]; > if (vector == (target_ulong)-1ULL) { > cpu_abort(cs, "Raised an exception without defined vector %d\n", >
Nicholas Piggin <npiggin@gmail.com> writes: > POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL], > and it removes support for the LPCR[AIL]=0b10 mode. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/ppc/spapr_hcall.c | 5 ++ > target/ppc/cpu-qom.h | 2 + > target/ppc/cpu.h | 5 +- > target/ppc/excp_helper.c | 114 ++++++++++++++++++++++++++------ > target/ppc/translate.c | 3 +- > target/ppc/translate_init.c.inc | 2 +- > 6 files changed, 107 insertions(+), 24 deletions(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 7b5cd3553c..2f65f20f72 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu, > return H_UNSUPPORTED_FLAG; > } > > + if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) { > + /* AIL 2 is also reserved in ISA v3.1 */ > + return H_UNSUPPORTED_FLAG; > + } > + > spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL); > > return H_SUCCESS; > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h > index 118baf8d41..06b6571bc9 100644 > --- a/target/ppc/cpu-qom.h > +++ b/target/ppc/cpu-qom.h > @@ -116,6 +116,8 @@ enum powerpc_excp_t { > POWERPC_EXCP_POWER8, > /* POWER9 exception model */ > POWERPC_EXCP_POWER9, > + /* POWER10 exception model */ > + POWERPC_EXCP_POWER10, > }; > > /*****************************************************************************/ > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index e73416da68..24e53e0ac3 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t { > #define LPCR_PECE_U_SHIFT (63 - 19) > #define LPCR_PECE_U_MASK (0x7ull << LPCR_PECE_U_SHIFT) > #define LPCR_HVEE PPC_BIT(17) /* Hypervisor Virt Exit Enable */ > -#define LPCR_RMLS_SHIFT (63 - 37) > +#define LPCR_RMLS_SHIFT (63 - 37) /* RMLS (removed in ISA v3.0) */ > #define LPCR_RMLS (0xfull << LPCR_RMLS_SHIFT) > +#define LPCR_HAIL PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */ > #define LPCR_ILE PPC_BIT(38) > -#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */ > +#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */ > #define LPCR_AIL (3ull << LPCR_AIL_SHIFT) > #define LPCR_UPRT PPC_BIT(41) /* Use Process Table */ > #define LPCR_EVIRT PPC_BIT(42) /* Enhanced Virtualisation */ > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index b8881c0f85..e8bf0598b4 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -197,7 +197,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > CPUState *cs = CPU(cpu); > CPUPPCState *env = &cpu->env; > target_ulong msr, new_msr, vector; > - int srr0, srr1, asrr0, asrr1, lev = -1, ail; > + int srr0, srr1, asrr0, asrr1, lev = -1; > + int ail, hail, using_ail; > bool lpes0; > > qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx > @@ -239,24 +240,39 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > * On anything else, we behave as if LPES0 is 1 > * (externals don't alter MSR:HV) > * > - * AIL is initialized here but can be cleared by > - * selected exceptions > + * ail/hail are initialized here but can be cleared by > + * selected exceptions, and other conditions afterwards. > */ > #if defined(TARGET_PPC64) > if (excp_model == POWERPC_EXCP_POWER7 || > excp_model == POWERPC_EXCP_POWER8 || > - excp_model == POWERPC_EXCP_POWER9) { > + excp_model == POWERPC_EXCP_POWER9 || > + excp_model == POWERPC_EXCP_POWER10) { > lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0); > if (excp_model != POWERPC_EXCP_POWER7) { > ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; > } else { > ail = 0; > } > + if (excp_model == POWERPC_EXCP_POWER10) { > + if (AIL_0001_8000) { if (2) > + ail = 0; /* AIL=2 is reserved in ISA v3.1 */ > + } > + > + if (env->spr[SPR_LPCR] & LPCR_HAIL) { > + hail = AIL_C000_0000_0000_4000; > + } else { > + hail = 0; > + } > + } else { > + hail = ail; I think it's better if we set hail = 0 here. Read on... > + } > } else > #endif /* defined(TARGET_PPC64) */ > { > lpes0 = true; > ail = 0; > + hail = 0; > } > > /* > @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > new_msr |= (target_ulong)MSR_HVB; > } > ail = 0; > + hail = 0; > > /* machine check exceptions don't have ME set */ > new_msr &= ~((target_ulong)1 << MSR_ME); > @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > } > ail = 0; > + hail = 0; > break; > case POWERPC_EXCP_DSEG: /* Data segment exception */ > case POWERPC_EXCP_ISEG: /* Instruction segment exception */ > @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } else if (env->spr[SPR_LPCR] & LPCR_ILE) { > new_msr |= (target_ulong)1 << MSR_LE; > } > - } else if (excp_model == POWERPC_EXCP_POWER9) { > + } else if (excp_model == POWERPC_EXCP_POWER9 || > + excp_model == POWERPC_EXCP_POWER10) { > if (new_msr & MSR_HVB) { > if (env->spr[SPR_HID0] & HID0_POWER9_HILE) { > new_msr |= (target_ulong)1 << MSR_LE; > @@ -791,22 +810,77 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > #endif > > /* > - * AIL only works if MSR[IR] and MSR[DR] are both enabled. > + * The ail variable is for AIL behaviour for interrupts that begin > + * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for > + * interrupts that begin execution with MSR[HV]=1. > + * > + * There is a large matrix of behaviours depending on the processor and > + * the current operating modes when the interrupt is taken, and the > + * interrupt type. The below tables lists behaviour for interrupts except > + * for SRESET, machine check, and HMI (which are all taken in real mode > + * with AIL 0). > + * > + * POWER8, POWER9 with LPCR[HR]=0 > + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | > + * +-----------+-------------+---------+-------------+-----+ > + * | a | 00/01/10 | x | x | 0 | > + * | a | 11 | 0 | 1 | 0 | > + * | a | 11 | 1 | 1 | a | > + * | a | 11 | 0 | 0 | a | > + * +-------------------------------------------------------+ > + * > + * POWER9 with LPCR[HR]=1 > + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | > + * +-----------+-------------+---------+-------------+-----+ > + * | a | 00/01/10 | x | x | 0 | > + * | a | 11 | x | x | a | > + * +-------------------------------------------------------+ > + * > + * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be > + * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1). > + * This is good for performance but allows the guest to influence the > + * AIL of the hypervisor interrupt using its MSR, and also the hypervisor > + * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the > + * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts. > + * > + * POWER10 addresses those issues with a new LPCR[HAIL] bit that is > + * applied to interrupt that begin execution with MSR[HV]=1 (so both > + * MSR[HV] 0->1 and 1->1). I'd put take these two paragraphs in the commit message as well. Just alter the beginning: s/The difference with POWER9 being that/In POWER9,/ > + * > + * POWER10 behaviour is > + * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | > + * +-----------+------------+-------------+---------+-------------+-----+ > + * | a | h | 00/01/10 | 0 | 0 | 0 | > + * | a | h | 11 | 0 | 0 | a | > + * | a | h | x | 0 | 1 | h | > + * | a | h | 00/01/10 | 1 | 1 | 0 | > + * | a | h | 11 | 1 | 1 | h | > + * +--------------------------------------------------------------------+ > */ > - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > - ail = 0; > - } > > - /* > - * AIL does not work if there is a MSR[HV] 0->1 transition and the > - * partition is in HPT mode. For radix guests, such interrupts are > - * allowed to be delivered to the hypervisor in ail mode. > - */ > - if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { > - if (!(env->spr[SPR_LPCR] & LPCR_HR)) { > + if (excp_model == POWERPC_EXCP_POWER8 || > + excp_model == POWERPC_EXCP_POWER9) { > + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > ail = 0; > + hail = 0; > + } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { > + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { > + hail = 0; > + } > + } > + } else if (excp_model == POWERPC_EXCP_POWER10) { > + if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) { > + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > + ail = 0; > + hail = 0; > + } > } > } > + if (new_msr & MSR_HVB) { > + using_ail = hail; > + } else { > + using_ail = ail; > + } So if at the start of the function we set: <= P7: ail = 0; hail = 0; P8,P9: ail = LPCR_AIL; hail = 0; P10: ail = LPCR_AIL, except AIL2; hail = AIL3; Then we could do this, which I think is more readable (comments are just for the email): // If there's an HV transition to 1, then HPT zeroes AIL and radix // doesn't. For P10 we'll use HAIL anyway, so no need to check. if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) && !(env->spr[SPR_LPCR] & LPCR_HR)) { ail = 0; } // If P10 and HV=1, use HAIL instead of AIL if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) { ail = hail; } // The IR|DR check always takes precedence and zeroes AIL/HAIL, no matter the processor version. if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { ail = 0; } (...) vector |= ppc_excp_vector_offset(cs, ail); I think that should work...*squints* What do you think? > > vector = env->excp_vectors[excp]; > if (vector == (target_ulong)-1ULL) { > @@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > env->spr[srr1] = msr; > > /* Handle AIL */ > - if (ail) { > + if (using_ail) { > new_msr |= (1 << MSR_IR) | (1 << MSR_DR); > - vector |= ppc_excp_vector_offset(cs, ail); > + vector |= ppc_excp_vector_offset(cs, using_ail); > } > > #if defined(TARGET_PPC64) > } else { > /* scv AIL is a little different */ > - if (ail) { > + if (using_ail) { > new_msr |= (1 << MSR_IR) | (1 << MSR_DR); > } > - if (ail == AIL_C000_0000_0000_4000) { > + if (using_ail == AIL_C000_0000_0000_4000) { > vector |= 0xc000000000003000ull; > } else { > vector |= 0x0000000000017000ull; > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 0984ce637b..e9ed001229 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -7731,7 +7731,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags) > #if defined(TARGET_PPC64) > if (env->excp_model == POWERPC_EXCP_POWER7 || > env->excp_model == POWERPC_EXCP_POWER8 || > - env->excp_model == POWERPC_EXCP_POWER9) { > + env->excp_model == POWERPC_EXCP_POWER9 || > + env->excp_model == POWERPC_EXCP_POWER10) { > qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n", > env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]); > } > diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc > index 70f9b9b150..5d427e9d38 100644 > --- a/target/ppc/translate_init.c.inc > +++ b/target/ppc/translate_init.c.inc > @@ -9317,7 +9317,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data) > pcc->radix_page_info = &POWER10_radix_page_info; > pcc->lrg_decr_bits = 56; > #endif > - pcc->excp_model = POWERPC_EXCP_POWER9; > + pcc->excp_model = POWERPC_EXCP_POWER10; > pcc->bus_model = PPC_FLAGS_INPUT_POWER9; > pcc->bfd_mach = bfd_mach_ppc64; > pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
Nicholas Piggin <npiggin@gmail.com> writes: > This must have slipped through the cracks between adding POWER10 support > and scv support. > Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > target/ppc/translate_init.c.inc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc > index c03a7c4f52..70f9b9b150 100644 > --- a/target/ppc/translate_init.c.inc > +++ b/target/ppc/translate_init.c.inc > @@ -9323,7 +9323,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data) > pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | > POWERPC_FLAG_BE | POWERPC_FLAG_PMM | > POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR | > - POWERPC_FLAG_VSX | POWERPC_FLAG_TM; > + POWERPC_FLAG_VSX | POWERPC_FLAG_TM | POWERPC_FLAG_SCV; > pcc->l1_dcache_size = 0x8000; > pcc->l1_icache_size = 0x8000; > pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
On 4/13/21 5:53 PM, Fabiano Rosas wrote: > Nicholas Piggin <npiggin@gmail.com> writes: > >> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL], >> and it removes support for the LPCR[AIL]=0b10 mode. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> hw/ppc/spapr_hcall.c | 5 ++ >> target/ppc/cpu-qom.h | 2 + >> target/ppc/cpu.h | 5 +- >> target/ppc/excp_helper.c | 114 ++++++++++++++++++++++++++------ >> target/ppc/translate.c | 3 +- >> target/ppc/translate_init.c.inc | 2 +- >> 6 files changed, 107 insertions(+), 24 deletions(-) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 7b5cd3553c..2f65f20f72 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu, >> return H_UNSUPPORTED_FLAG; >> } >> >> + if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) { >> + /* AIL 2 is also reserved in ISA v3.1 */ >> + return H_UNSUPPORTED_FLAG; >> + } >> + >> spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL); >> >> return H_SUCCESS; >> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h >> index 118baf8d41..06b6571bc9 100644 >> --- a/target/ppc/cpu-qom.h >> +++ b/target/ppc/cpu-qom.h >> @@ -116,6 +116,8 @@ enum powerpc_excp_t { >> POWERPC_EXCP_POWER8, >> /* POWER9 exception model */ >> POWERPC_EXCP_POWER9, >> + /* POWER10 exception model */ >> + POWERPC_EXCP_POWER10, >> }; >> >> /*****************************************************************************/ >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >> index e73416da68..24e53e0ac3 100644 >> --- a/target/ppc/cpu.h >> +++ b/target/ppc/cpu.h >> @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t { >> #define LPCR_PECE_U_SHIFT (63 - 19) >> #define LPCR_PECE_U_MASK (0x7ull << LPCR_PECE_U_SHIFT) >> #define LPCR_HVEE PPC_BIT(17) /* Hypervisor Virt Exit Enable */ >> -#define LPCR_RMLS_SHIFT (63 - 37) >> +#define LPCR_RMLS_SHIFT (63 - 37) /* RMLS (removed in ISA v3.0) */ >> #define LPCR_RMLS (0xfull << LPCR_RMLS_SHIFT) >> +#define LPCR_HAIL PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */ >> #define LPCR_ILE PPC_BIT(38) >> -#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */ >> +#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */ >> #define LPCR_AIL (3ull << LPCR_AIL_SHIFT) >> #define LPCR_UPRT PPC_BIT(41) /* Use Process Table */ >> #define LPCR_EVIRT PPC_BIT(42) /* Enhanced Virtualisation */ >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index b8881c0f85..e8bf0598b4 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -197,7 +197,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> CPUState *cs = CPU(cpu); >> CPUPPCState *env = &cpu->env; >> target_ulong msr, new_msr, vector; >> - int srr0, srr1, asrr0, asrr1, lev = -1, ail; >> + int srr0, srr1, asrr0, asrr1, lev = -1; >> + int ail, hail, using_ail; >> bool lpes0; >> >> qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx >> @@ -239,24 +240,39 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> * On anything else, we behave as if LPES0 is 1 >> * (externals don't alter MSR:HV) >> * >> - * AIL is initialized here but can be cleared by >> - * selected exceptions >> + * ail/hail are initialized here but can be cleared by >> + * selected exceptions, and other conditions afterwards. >> */ >> #if defined(TARGET_PPC64) >> if (excp_model == POWERPC_EXCP_POWER7 || >> excp_model == POWERPC_EXCP_POWER8 || >> - excp_model == POWERPC_EXCP_POWER9) { >> + excp_model == POWERPC_EXCP_POWER9 || >> + excp_model == POWERPC_EXCP_POWER10) { >> lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0); >> if (excp_model != POWERPC_EXCP_POWER7) { >> ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; >> } else { >> ail = 0; >> } >> + if (excp_model == POWERPC_EXCP_POWER10) { >> + if (AIL_0001_8000) { > > if (2) > >> + ail = 0; /* AIL=2 is reserved in ISA v3.1 */ >> + } >> + >> + if (env->spr[SPR_LPCR] & LPCR_HAIL) { >> + hail = AIL_C000_0000_0000_4000; >> + } else { >> + hail = 0; >> + } >> + } else { >> + hail = ail; > > I think it's better if we set hail = 0 here. Read on... > >> + } >> } else >> #endif /* defined(TARGET_PPC64) */ >> { >> lpes0 = true; >> ail = 0; >> + hail = 0; >> } >> >> /* >> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> new_msr |= (target_ulong)MSR_HVB; >> } >> ail = 0; >> + hail = 0; >> >> /* machine check exceptions don't have ME set */ >> new_msr &= ~((target_ulong)1 << MSR_ME); >> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> } >> } >> ail = 0; >> + hail = 0; >> break; >> case POWERPC_EXCP_DSEG: /* Data segment exception */ >> case POWERPC_EXCP_ISEG: /* Instruction segment exception */ >> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> } else if (env->spr[SPR_LPCR] & LPCR_ILE) { >> new_msr |= (target_ulong)1 << MSR_LE; >> } >> - } else if (excp_model == POWERPC_EXCP_POWER9) { >> + } else if (excp_model == POWERPC_EXCP_POWER9 || >> + excp_model == POWERPC_EXCP_POWER10) { >> if (new_msr & MSR_HVB) { >> if (env->spr[SPR_HID0] & HID0_POWER9_HILE) { >> new_msr |= (target_ulong)1 << MSR_LE; >> @@ -791,22 +810,77 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> #endif >> >> /* >> - * AIL only works if MSR[IR] and MSR[DR] are both enabled. >> + * The ail variable is for AIL behaviour for interrupts that begin >> + * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for >> + * interrupts that begin execution with MSR[HV]=1. >> + * >> + * There is a large matrix of behaviours depending on the processor and >> + * the current operating modes when the interrupt is taken, and the >> + * interrupt type. The below tables lists behaviour for interrupts except >> + * for SRESET, machine check, and HMI (which are all taken in real mode >> + * with AIL 0). >> + * >> + * POWER8, POWER9 with LPCR[HR]=0 >> + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | >> + * +-----------+-------------+---------+-------------+-----+ >> + * | a | 00/01/10 | x | x | 0 | >> + * | a | 11 | 0 | 1 | 0 | >> + * | a | 11 | 1 | 1 | a | >> + * | a | 11 | 0 | 0 | a | >> + * +-------------------------------------------------------+ >> + * >> + * POWER9 with LPCR[HR]=1 >> + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | >> + * +-----------+-------------+---------+-------------+-----+ >> + * | a | 00/01/10 | x | x | 0 | >> + * | a | 11 | x | x | a | >> + * +-------------------------------------------------------+ >> + * >> + * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be >> + * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1). >> + * This is good for performance but allows the guest to influence the >> + * AIL of the hypervisor interrupt using its MSR, and also the hypervisor >> + * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the >> + * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts. >> + * >> + * POWER10 addresses those issues with a new LPCR[HAIL] bit that is >> + * applied to interrupt that begin execution with MSR[HV]=1 (so both >> + * MSR[HV] 0->1 and 1->1). > > I'd put take these two paragraphs in the commit message as well. Just > alter the beginning: > s/The difference with POWER9 being that/In POWER9,/ > >> + * >> + * POWER10 behaviour is >> + * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL | >> + * +-----------+------------+-------------+---------+-------------+-----+ >> + * | a | h | 00/01/10 | 0 | 0 | 0 | >> + * | a | h | 11 | 0 | 0 | a | >> + * | a | h | x | 0 | 1 | h | >> + * | a | h | 00/01/10 | 1 | 1 | 0 | >> + * | a | h | 11 | 1 | 1 | h | >> + * +--------------------------------------------------------------------+ >> */ >> - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { >> - ail = 0; >> - } >> >> - /* >> - * AIL does not work if there is a MSR[HV] 0->1 transition and the >> - * partition is in HPT mode. For radix guests, such interrupts are >> - * allowed to be delivered to the hypervisor in ail mode. >> - */ >> - if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { >> - if (!(env->spr[SPR_LPCR] & LPCR_HR)) { >> + if (excp_model == POWERPC_EXCP_POWER8 || >> + excp_model == POWERPC_EXCP_POWER9) { >> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { >> ail = 0; >> + hail = 0; >> + } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { >> + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { >> + hail = 0; >> + } >> + } >> + } else if (excp_model == POWERPC_EXCP_POWER10) { >> + if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) { >> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { >> + ail = 0; >> + hail = 0; >> + } >> } >> } >> + if (new_msr & MSR_HVB) { >> + using_ail = hail; >> + } else { >> + using_ail = ail; >> + } > > So if at the start of the function we set: > > <= P7: > ail = 0; > hail = 0; > > P8,P9: > ail = LPCR_AIL; > hail = 0; > > P10: > ail = LPCR_AIL, except AIL2; > hail = AIL3; > > Then we could do this, which I think is more readable (comments are just > for the email): > > // If there's an HV transition to 1, then HPT zeroes AIL and radix > // doesn't. For P10 we'll use HAIL anyway, so no need to check. > > if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) && > !(env->spr[SPR_LPCR] & LPCR_HR)) { > ail = 0; > } > > // If P10 and HV=1, use HAIL instead of AIL > > if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) { > ail = hail; > } > > // The IR|DR check always takes precedence and zeroes AIL/HAIL, no > matter the processor version. > > if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > ail = 0; > } > > (...) > > vector |= ppc_excp_vector_offset(cs, ail); > > I think that should work...*squints* > What do you think? The powerpc_excp() routine is insane. It feels that we should be duplicating the code in handlers for each of the POWER[7-10] CPUs and keep a default powerpc_excp() for the others. C. > >> >> vector = env->excp_vectors[excp]; >> if (vector == (target_ulong)-1ULL) { >> @@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> env->spr[srr1] = msr; >> >> /* Handle AIL */ >> - if (ail) { >> + if (using_ail) { >> new_msr |= (1 << MSR_IR) | (1 << MSR_DR); >> - vector |= ppc_excp_vector_offset(cs, ail); >> + vector |= ppc_excp_vector_offset(cs, using_ail); >> } >> >> #if defined(TARGET_PPC64) >> } else { >> /* scv AIL is a little different */ >> - if (ail) { >> + if (using_ail) { >> new_msr |= (1 << MSR_IR) | (1 << MSR_DR); >> } >> - if (ail == AIL_C000_0000_0000_4000) { >> + if (using_ail == AIL_C000_0000_0000_4000) { >> vector |= 0xc000000000003000ull; >> } else { >> vector |= 0x0000000000017000ull; >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c >> index 0984ce637b..e9ed001229 100644 >> --- a/target/ppc/translate.c >> +++ b/target/ppc/translate.c >> @@ -7731,7 +7731,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags) >> #if defined(TARGET_PPC64) >> if (env->excp_model == POWERPC_EXCP_POWER7 || >> env->excp_model == POWERPC_EXCP_POWER8 || >> - env->excp_model == POWERPC_EXCP_POWER9) { >> + env->excp_model == POWERPC_EXCP_POWER9 || >> + env->excp_model == POWERPC_EXCP_POWER10) { >> qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n", >> env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]); >> } >> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc >> index 70f9b9b150..5d427e9d38 100644 >> --- a/target/ppc/translate_init.c.inc >> +++ b/target/ppc/translate_init.c.inc >> @@ -9317,7 +9317,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data) >> pcc->radix_page_info = &POWER10_radix_page_info; >> pcc->lrg_decr_bits = 56; >> #endif >> - pcc->excp_model = POWERPC_EXCP_POWER9; >> + pcc->excp_model = POWERPC_EXCP_POWER10; >> pcc->bus_model = PPC_FLAGS_INPUT_POWER9; >> pcc->bfd_mach = bfd_mach_ppc64; >> pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
Excerpts from Cédric Le Goater's message of April 14, 2021 3:09 am:
> On 4/13/21 5:53 PM, Fabiano Rosas wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>
>>> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
>>> and it removes support for the LPCR[AIL]=0b10 mode.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> hw/ppc/spapr_hcall.c | 5 ++
>>> target/ppc/cpu-qom.h | 2 +
>>> target/ppc/cpu.h | 5 +-
>>> target/ppc/excp_helper.c | 114 ++++++++++++++++++++++++++------
>>> target/ppc/translate.c | 3 +-
>>> target/ppc/translate_init.c.inc | 2 +-
>>> 6 files changed, 107 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index 7b5cd3553c..2f65f20f72 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>> return H_UNSUPPORTED_FLAG;
>>> }
>>>
>>> + if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
>>> + /* AIL 2 is also reserved in ISA v3.1 */
>>> + return H_UNSUPPORTED_FLAG;
>>> + }
>>> +
>>> spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>>>
>>> return H_SUCCESS;
>>> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
>>> index 118baf8d41..06b6571bc9 100644
>>> --- a/target/ppc/cpu-qom.h
>>> +++ b/target/ppc/cpu-qom.h
>>> @@ -116,6 +116,8 @@ enum powerpc_excp_t {
>>> POWERPC_EXCP_POWER8,
>>> /* POWER9 exception model */
>>> POWERPC_EXCP_POWER9,
>>> + /* POWER10 exception model */
>>> + POWERPC_EXCP_POWER10,
>>> };
>>>
>>> /*****************************************************************************/
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index e73416da68..24e53e0ac3 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t {
>>> #define LPCR_PECE_U_SHIFT (63 - 19)
>>> #define LPCR_PECE_U_MASK (0x7ull << LPCR_PECE_U_SHIFT)
>>> #define LPCR_HVEE PPC_BIT(17) /* Hypervisor Virt Exit Enable */
>>> -#define LPCR_RMLS_SHIFT (63 - 37)
>>> +#define LPCR_RMLS_SHIFT (63 - 37) /* RMLS (removed in ISA v3.0) */
>>> #define LPCR_RMLS (0xfull << LPCR_RMLS_SHIFT)
>>> +#define LPCR_HAIL PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */
>>> #define LPCR_ILE PPC_BIT(38)
>>> -#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */
>>> +#define LPCR_AIL_SHIFT (63 - 40) /* Alternate interrupt location */
>>> #define LPCR_AIL (3ull << LPCR_AIL_SHIFT)
>>> #define LPCR_UPRT PPC_BIT(41) /* Use Process Table */
>>> #define LPCR_EVIRT PPC_BIT(42) /* Enhanced Virtualisation */
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index b8881c0f85..e8bf0598b4 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -197,7 +197,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> CPUState *cs = CPU(cpu);
>>> CPUPPCState *env = &cpu->env;
>>> target_ulong msr, new_msr, vector;
>>> - int srr0, srr1, asrr0, asrr1, lev = -1, ail;
>>> + int srr0, srr1, asrr0, asrr1, lev = -1;
>>> + int ail, hail, using_ail;
>>> bool lpes0;
>>>
>>> qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>>> @@ -239,24 +240,39 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> * On anything else, we behave as if LPES0 is 1
>>> * (externals don't alter MSR:HV)
>>> *
>>> - * AIL is initialized here but can be cleared by
>>> - * selected exceptions
>>> + * ail/hail are initialized here but can be cleared by
>>> + * selected exceptions, and other conditions afterwards.
>>> */
>>> #if defined(TARGET_PPC64)
>>> if (excp_model == POWERPC_EXCP_POWER7 ||
>>> excp_model == POWERPC_EXCP_POWER8 ||
>>> - excp_model == POWERPC_EXCP_POWER9) {
>>> + excp_model == POWERPC_EXCP_POWER9 ||
>>> + excp_model == POWERPC_EXCP_POWER10) {
>>> lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
>>> if (excp_model != POWERPC_EXCP_POWER7) {
>>> ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>>> } else {
>>> ail = 0;
>>> }
>>> + if (excp_model == POWERPC_EXCP_POWER10) {
>>> + if (AIL_0001_8000) {
>>
>> if (2)
>>
>>> + ail = 0; /* AIL=2 is reserved in ISA v3.1 */
>>> + }
>>> +
>>> + if (env->spr[SPR_LPCR] & LPCR_HAIL) {
>>> + hail = AIL_C000_0000_0000_4000;
>>> + } else {
>>> + hail = 0;
>>> + }
>>> + } else {
>>> + hail = ail;
>>
>> I think it's better if we set hail = 0 here. Read on...
>>
>>> + }
>>> } else
>>> #endif /* defined(TARGET_PPC64) */
>>> {
>>> lpes0 = true;
>>> ail = 0;
>>> + hail = 0;
>>> }
>>>
>>> /*
>>> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> new_msr |= (target_ulong)MSR_HVB;
>>> }
>>> ail = 0;
>>> + hail = 0;
>>>
>>> /* machine check exceptions don't have ME set */
>>> new_msr &= ~((target_ulong)1 << MSR_ME);
>>> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> }
>>> }
>>> ail = 0;
>>> + hail = 0;
>>> break;
>>> case POWERPC_EXCP_DSEG: /* Data segment exception */
>>> case POWERPC_EXCP_ISEG: /* Instruction segment exception */
>>> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
>>> new_msr |= (target_ulong)1 << MSR_LE;
>>> }
>>> - } else if (excp_model == POWERPC_EXCP_POWER9) {
>>> + } else if (excp_model == POWERPC_EXCP_POWER9 ||
>>> + excp_model == POWERPC_EXCP_POWER10) {
>>> if (new_msr & MSR_HVB) {
>>> if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
>>> new_msr |= (target_ulong)1 << MSR_LE;
>>> @@ -791,22 +810,77 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> #endif
>>>
>>> /*
>>> - * AIL only works if MSR[IR] and MSR[DR] are both enabled.
>>> + * The ail variable is for AIL behaviour for interrupts that begin
>>> + * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
>>> + * interrupts that begin execution with MSR[HV]=1.
>>> + *
>>> + * There is a large matrix of behaviours depending on the processor and
>>> + * the current operating modes when the interrupt is taken, and the
>>> + * interrupt type. The below tables lists behaviour for interrupts except
>>> + * for SRESET, machine check, and HMI (which are all taken in real mode
>>> + * with AIL 0).
>>> + *
>>> + * POWER8, POWER9 with LPCR[HR]=0
>>> + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
>>> + * +-----------+-------------+---------+-------------+-----+
>>> + * | a | 00/01/10 | x | x | 0 |
>>> + * | a | 11 | 0 | 1 | 0 |
>>> + * | a | 11 | 1 | 1 | a |
>>> + * | a | 11 | 0 | 0 | a |
>>> + * +-------------------------------------------------------+
>>> + *
>>> + * POWER9 with LPCR[HR]=1
>>> + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
>>> + * +-----------+-------------+---------+-------------+-----+
>>> + * | a | 00/01/10 | x | x | 0 |
>>> + * | a | 11 | x | x | a |
>>> + * +-------------------------------------------------------+
>>> + *
>>> + * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
>>> + * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
>>> + * This is good for performance but allows the guest to influence the
>>> + * AIL of the hypervisor interrupt using its MSR, and also the hypervisor
>>> + * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the
>>> + * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts.
>>> + *
>>> + * POWER10 addresses those issues with a new LPCR[HAIL] bit that is
>>> + * applied to interrupt that begin execution with MSR[HV]=1 (so both
>>> + * MSR[HV] 0->1 and 1->1).
>>
>> I'd put take these two paragraphs in the commit message as well. Just
>> alter the beginning:
>> s/The difference with POWER9 being that/In POWER9,/
>>
>>> + *
>>> + * POWER10 behaviour is
>>> + * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
>>> + * +-----------+------------+-------------+---------+-------------+-----+
>>> + * | a | h | 00/01/10 | 0 | 0 | 0 |
>>> + * | a | h | 11 | 0 | 0 | a |
>>> + * | a | h | x | 0 | 1 | h |
>>> + * | a | h | 00/01/10 | 1 | 1 | 0 |
>>> + * | a | h | 11 | 1 | 1 | h |
>>> + * +--------------------------------------------------------------------+
>>> */
>>> - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>> - ail = 0;
>>> - }
>>>
>>> - /*
>>> - * AIL does not work if there is a MSR[HV] 0->1 transition and the
>>> - * partition is in HPT mode. For radix guests, such interrupts are
>>> - * allowed to be delivered to the hypervisor in ail mode.
>>> - */
>>> - if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
>>> - if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
>>> + if (excp_model == POWERPC_EXCP_POWER8 ||
>>> + excp_model == POWERPC_EXCP_POWER9) {
>>> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>> ail = 0;
>>> + hail = 0;
>>> + } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
>>> + if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
>>> + hail = 0;
>>> + }
>>> + }
>>> + } else if (excp_model == POWERPC_EXCP_POWER10) {
>>> + if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
>>> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>> + ail = 0;
>>> + hail = 0;
>>> + }
>>> }
>>> }
>>> + if (new_msr & MSR_HVB) {
>>> + using_ail = hail;
>>> + } else {
>>> + using_ail = ail;
>>> + }
>>
>> So if at the start of the function we set:
>>
>> <= P7:
>> ail = 0;
>> hail = 0;
>>
>> P8,P9:
>> ail = LPCR_AIL;
>> hail = 0;
>>
>> P10:
>> ail = LPCR_AIL, except AIL2;
>> hail = AIL3;
>>
>> Then we could do this, which I think is more readable (comments are just
>> for the email):
>>
>> // If there's an HV transition to 1, then HPT zeroes AIL and radix
>> // doesn't. For P10 we'll use HAIL anyway, so no need to check.
>>
>> if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) &&
>> !(env->spr[SPR_LPCR] & LPCR_HR)) {
>> ail = 0;
>> }
>>
>> // If P10 and HV=1, use HAIL instead of AIL
>>
>> if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) {
>> ail = hail;
>> }
>>
>> // The IR|DR check always takes precedence and zeroes AIL/HAIL, no
>> matter the processor version.
>>
>> if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>> ail = 0;
>> }
>>
>> (...)
>>
>> vector |= ppc_excp_vector_offset(cs, ail);
>>
>> I think that should work...*squints*
>> What do you think?
>
> The powerpc_excp() routine is insane.
>
> It feels that we should be duplicating the code in handlers for each
> of the POWER[7-10] CPUs and keep a default powerpc_excp() for the
> others.
It's getting pretty wild :(
I was trying to come up with something in the way of a "fix" but I agree
it needs a bigger reorganisation.
What would help a lot actually is moving all the AIL logic together. The
only reason it's split completely in two like this is so a few
interrupts can clear it. I think what would work better is just do all
that in one place, in the AIL logic. Let me see what that looks like. It
might actually be a saner fix in the end.
Thanks,
Nick
Excerpts from Fabiano Rosas's message of April 13, 2021 11:48 pm: > Nicholas Piggin <npiggin@gmail.com> writes: > >> ISA v3.0 radix guest execution has a quirk in AIL behaviour such that >> the LPCR[AIL] value can apply to hypervisor interrupts. >> >> This affects machines that emulate HV=1 mode (i.e., powernv9). >> > > Ah ok, so we actually want to replicate the quirk in the pnv > machine. Took me a while. Yes. Quirk is probably the wrong word for me to use. It's architected behaviour so it must be implemented, it's just slightly surprising / easy to miss. > > Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com> Thanks, Nick > >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> target/ppc/excp_helper.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 85de7e6c90..b8881c0f85 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> #endif >> >> /* >> - * AIL only works if there is no HV transition and we are running >> - * with translations enabled >> + * AIL only works if MSR[IR] and MSR[DR] are both enabled. >> */ >> - if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) || >> - ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) { >> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { >> ail = 0; >> } >> >> + /* >> + * AIL does not work if there is a MSR[HV] 0->1 transition and the >> + * partition is in HPT mode. For radix guests, such interrupts are >> + * allowed to be delivered to the hypervisor in ail mode. >> + */ >> + if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) { >> + if (!(env->spr[SPR_LPCR] & LPCR_HR)) { >> + ail = 0; >> + } >> + } >> + >> vector = env->excp_vectors[excp]; >> if (vector == (target_ulong)-1ULL) { >> cpu_abort(cs, "Raised an exception without defined vector %d\n", >
Excerpts from Fabiano Rosas's message of April 14, 2021 1:53 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
>> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
>> and it removes support for the LPCR[AIL]=0b10 mode.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
[snip]
Thanks for the suggestions, I will consider them if people prefer to go
this way rather than the "cleanup first" approach in the RFC I just
posted.
Thanks,
Nick