* [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
* [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
* [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 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: [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: [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
* 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 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
* 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 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: [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).