* [PATCH v1 0/3] POWER9/POWER10 exception model fixes
@ 2021-04-13 12:54 Nicholas Piggin
2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw)
To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson
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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour 2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin @ 2021-04-13 12:54 ` Nicholas Piggin 2021-04-13 13:48 ` Fabiano Rosas 2021-04-13 14:31 ` [EXTERNAL] " Cédric Le Goater 2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin 2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin 2 siblings, 2 replies; 13+ messages in thread From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw) To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour 2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin @ 2021-04-13 13:48 ` Fabiano Rosas 2021-04-14 3:26 ` Nicholas Piggin 2021-04-13 14:31 ` [EXTERNAL] " Cédric Le Goater 1 sibling, 1 reply; 13+ messages in thread From: Fabiano Rosas @ 2021-04-13 13:48 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson 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", ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour 2021-04-13 13:48 ` Fabiano Rosas @ 2021-04-14 3:26 ` Nicholas Piggin 0 siblings, 0 replies; 13+ messages in thread From: Nicholas Piggin @ 2021-04-14 3:26 UTC (permalink / raw) To: Fabiano Rosas, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson 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", > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [EXTERNAL] [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour 2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin 2021-04-13 13:48 ` Fabiano Rosas @ 2021-04-13 14:31 ` Cédric Le Goater 1 sibling, 0 replies; 13+ messages in thread From: Cédric Le Goater @ 2021-04-13 14:31 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson 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", > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 2/3] target/ppc: POWER10 supports scv 2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin 2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin @ 2021-04-13 12:54 ` Nicholas Piggin 2021-04-13 14:18 ` [EXTERNAL] " Cédric Le Goater 2021-04-13 15:54 ` Fabiano Rosas 2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin 2 siblings, 2 replies; 13+ messages in thread From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw) To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [EXTERNAL] [PATCH v1 2/3] target/ppc: POWER10 supports scv 2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin @ 2021-04-13 14:18 ` Cédric Le Goater 2021-04-13 15:54 ` Fabiano Rosas 1 sibling, 0 replies; 13+ messages in thread From: Cédric Le Goater @ 2021-04-13 14:18 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson 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; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/3] target/ppc: POWER10 supports scv 2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin 2021-04-13 14:18 ` [EXTERNAL] " Cédric Le Goater @ 2021-04-13 15:54 ` Fabiano Rosas 1 sibling, 0 replies; 13+ messages in thread From: Fabiano Rosas @ 2021-04-13 15:54 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 3/3] target/ppc: Add POWER10 exception model 2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin 2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin 2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin @ 2021-04-13 12:54 ` Nicholas Piggin 2021-04-13 15:53 ` Fabiano Rosas 2 siblings, 1 reply; 13+ messages in thread From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw) To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model 2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin @ 2021-04-13 15:53 ` Fabiano Rosas 2021-04-13 17:09 ` Cédric Le Goater 2021-04-14 3:28 ` Nicholas Piggin 0 siblings, 2 replies; 13+ messages in thread From: Fabiano Rosas @ 2021-04-13 15:53 UTC (permalink / raw) To: Nicholas Piggin, qemu-ppc Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model 2021-04-13 15:53 ` Fabiano Rosas @ 2021-04-13 17:09 ` Cédric Le Goater 2021-04-14 1:56 ` Nicholas Piggin 2021-04-14 3:28 ` Nicholas Piggin 1 sibling, 1 reply; 13+ messages in thread From: Cédric Le Goater @ 2021-04-13 17:09 UTC (permalink / raw) To: Fabiano Rosas, Nicholas Piggin, qemu-ppc Cc: Cédric Le Goater, qemu-devel, David Gibson 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model 2021-04-13 17:09 ` Cédric Le Goater @ 2021-04-14 1:56 ` Nicholas Piggin 0 siblings, 0 replies; 13+ messages in thread From: Nicholas Piggin @ 2021-04-14 1:56 UTC (permalink / raw) To: Cédric Le Goater, Fabiano Rosas, qemu-ppc Cc: Cédric Le Goater, qemu-devel, David Gibson 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model 2021-04-13 15:53 ` Fabiano Rosas 2021-04-13 17:09 ` Cédric Le Goater @ 2021-04-14 3:28 ` Nicholas Piggin 1 sibling, 0 replies; 13+ messages in thread From: Nicholas Piggin @ 2021-04-14 3:28 UTC (permalink / raw) To: Fabiano Rosas, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-04-14 3:29 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin 2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin 2021-04-13 13:48 ` Fabiano Rosas 2021-04-14 3:26 ` Nicholas Piggin 2021-04-13 14:31 ` [EXTERNAL] " Cédric Le Goater 2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin 2021-04-13 14:18 ` [EXTERNAL] " Cédric Le Goater 2021-04-13 15:54 ` Fabiano Rosas 2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin 2021-04-13 15:53 ` Fabiano Rosas 2021-04-13 17:09 ` Cédric Le Goater 2021-04-14 1:56 ` Nicholas Piggin 2021-04-14 3:28 ` Nicholas Piggin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).