[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
|
From: |
Fabiano Rosas |
|
Subject: |
Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model |
|
Date: |
Tue, 13 Apr 2021 12:53:55 -0300 |
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 |