Re: [PATCH v11 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers
On Sat, 30 Mar 2024 at 10:34, Jinjie Ruan wrote:
>
> Add the NMIAR CPU interface registers which deal with acknowledging NMI.
>
> When introduce NMI interrupt, there are some updates to the semantics for the
> register ICC_IAR1_EL1 and ICC_HPPIR1_EL1. For ICC_IAR1_EL1 register, it
> should return 1022 if the intid has non-maskable property. And for
> ICC_NMIAR1_EL1 register, it should return 1023 if the intid do not have
> non-maskable property. Howerever, these are not necessary for ICC_HPPIR1_EL1
> register.
>
> And the APR and RPR has NMI bits which should be handled correctly.
>
> Signed-off-by: Jinjie Ruan
> Reviewed-by: Richard Henderson
I have a few more comments below but otherwise I think this
patch is looking OK.
> @@ -898,12 +922,24 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs)
> */
> int rprio;
> uint32_t mask;
> +ARMCPU *cpu = ARM_CPU(cs->cpu);
> +CPUARMState *env = &cpu->env;
>
> if (icc_no_enabled_hppi(cs)) {
> return false;
> }
>
> -if (cs->hppi.prio >= cs->icc_pmr_el1) {
> +if (cs->gic->nmi_support && cs->hppi.nmi) {
If cs->hppi.nmi is set then nmi_support must be true, as we won't
ever set hppi.nmi if the GIC doesn't have NMI support (because the
registers where the guest can set the NMI bit will not be writeable
and so the nmi bits will always be zero). Elsewhere in the code
we assume this (eg in icc_iar1_read() below), so I think we can
also assume it here.
> +if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS) &&
> +cs->hppi.grp == GICV3_G1NS) {
> +if (cs->icc_pmr_el1 < 0x80) {
> +return false;
> +}
> +if (arm_is_secure(env) && cs->icc_pmr_el1 == 0x80) {
> +return false;
> +}
> +}
> +} else if (cs->hppi.prio >= cs->icc_pmr_el1) {
> /* Priority mask masks this interrupt */
> return false;
> }
> @@ -923,6 +959,18 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs)
> return true;
> }
>
> +if (cs->gic->nmi_support && cs->hppi.nmi &&
> +(cs->hppi.prio & mask) == (rprio & mask)) {
> +if ((cs->hppi.grp == GICV3_G1NS) &&
> +!(cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI)) {
> +return true;
> +}
> +if ((cs->hppi.grp == GICV3_G1) &&
> +!(cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI)) {
> +return true;
> +}
You can write this part a bit more concisely:
if (!(cs->icc_apr[cs->hppi.grp][0] & ICC_AP1R_EL1_NMI)) {
return true;
}
rather than writing out the G1 and G1NS cases separately.
> +}
> +
> return false;
> }
> @@ -1159,13 +1212,16 @@ static uint64_t icc_iar0_read(CPUARMState *env, const
> ARMCPRegInfo *ri)
> static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
> {
> GICv3CPUState *cs = icc_cs_from_env(env);
> +int el = arm_current_el(env);
> uint64_t intid;
>
> if (icv_access(env, HCR_IMO)) {
> return icv_iar_read(env, ri);
> }
>
> -if (!icc_hppi_can_preempt(cs)) {
> +if (cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) {
> +intid = INTID_NMI;
> +} else if (!icc_hppi_can_preempt(cs)) {
> intid = INTID_SPURIOUS;
> } else {
> intid = icc_hppir1_value(cs, env);
This is the wrong order -- the cases where icc_hppi_can_preempt()
returns false need to take priority over the case where we return
INTID_NMI. You can get that by structuring it similarly to the
pseudocode:
if (!icc_hppi_can_preempt(cs)) {
intid = INTID_SPURIOUS;
} else {
intid = icc_hppir1_value(cs, env);
}
if (!gicv3_intid_is_special(intid)) {
if (cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) {
intid = INTID_NMI;
} else {
icc_activate_irq(cs, intid);
}
}
> @@ -1803,6 +1901,22 @@ static uint64_t icc_rpr_read(CPUARMState *env, const
> ARMCPRegInfo *ri)
> }
> }
>
> +if (cs->gic->nmi_support) {
> +/* NMI info is reported in the high bits of RPR */
> +if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
> +if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
> +prio |= ICC_RPR_EL1_NSNMI;
This should be ICC_RPR_EL1_NMI. Compare the pseudocode for ICC_RPR_EL1:
in the EL3-nonsecure case we set pPriority<63>.
> +}
> +} else {
> +if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
> +prio |= ICC_RPR_EL1_NSNMI;
> +}
> +if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) {
> +prio |= ICC_RPR_EL1_NMI;
> +}
> +}
> +}
> +
> trace_gicv3_icc_rpr_read(gicv3_redist_affid(cs), prio);
> return prio;
> }
thanks
-- PMM
[PATCH v11 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers
Add the NMIAR CPU interface registers which deal with acknowledging NMI.
When introduce NMI interrupt, there are some updates to the semantics for the
register ICC_IAR1_EL1 and ICC_HPPIR1_EL1. For ICC_IAR1_EL1 register, it
should return 1022 if the intid has non-maskable property. And for
ICC_NMIAR1_EL1 register, it should return 1023 if the intid do not have
non-maskable property. Howerever, these are not necessary for ICC_HPPIR1_EL1
register.
And the APR and RPR has NMI bits which should be handled correctly.
Signed-off-by: Jinjie Ruan
Reviewed-by: Richard Henderson
---
v11:
- Handle NMI priority in icc_highest_active_prio() and handle NMI RPR in
icc_rpr_read() separately.
- Only set NMI bit for a NMI and and ordinary priority bit for a non-NMI in
icc_activate_irq().
- Only clear APR bit for AP1R0 in icc_drop_prio().
- Check special INTID_* in callers instead of passing two extra boolean args
for ack functions.
- Handle NMI in icc_hppi_can_preempt() and icc_highest_active_group().
- Also check icc_hppi_can_preempt() for icc_nmiar1_read().
v10:
- is_nmi -> nmi.
- is_hppi -> hppi.
- Exchange the order of nmi and hppi parameters.
- superprio -> nmi.
- Handle APR and RPR NMI bits.
- Update the commit message, super priority -> non-maskable property.
v7:
- Add Reviewed-by.
v4:
- Define ICC_NMIAR1_EL1 only if FEAT_GICv3_NMI is implemented.
- Check sctrl_elx.SCTLR_NMI to return 1022 for icc_iar1_read().
- Add gicv3_icc_nmiar1_read() trace event.
- Do not check icc_hppi_can_preempt() for icc_nmiar1_read().
- Add icv_nmiar1_read() and call it when EL2Enabled() and HCR_EL2.IMO == '1'
---
hw/intc/arm_gicv3_cpuif.c | 137 --
hw/intc/gicv3_internal.h | 5 ++
hw/intc/trace-events | 1 +
3 files changed, 138 insertions(+), 5 deletions(-)
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index e1a60d8c15..f99f2570a6 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -795,6 +795,13 @@ static uint64_t icv_iar_read(CPUARMState *env, const
ARMCPRegInfo *ri)
return intid;
}
+static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+/* todo */
+uint64_t intid = INTID_SPURIOUS;
+return intid;
+}
+
static uint32_t icc_fullprio_mask(GICv3CPUState *cs)
{
/*
@@ -832,6 +839,23 @@ static int icc_highest_active_prio(GICv3CPUState *cs)
*/
int i;
+if (cs->gic->nmi_support) {
+/*
+ * If an NMI is active this takes precedence over anything else
+ * for priority purposes; the NMI bit is only in the AP1R0 bit.
+ * We return here the effective priority of the NMI, which is
+ * either 0x0 or 0x80. Callers will need to check NMI again for
+ * purposes of either setting the RPR register bits or for
+ * prioritization of NMI vs non-NMI.
+ */
+if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) {
+return 0;
+}
+if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
+return (cs->gic->gicd_ctlr & GICD_CTLR_DS) ? 0 : 0x80;
+}
+}
+
for (i = 0; i < icc_num_aprs(cs); i++) {
uint32_t apr = cs->icc_apr[GICV3_G0][i] |
cs->icc_apr[GICV3_G1][i] | cs->icc_apr[GICV3_G1NS][i];
@@ -898,12 +922,24 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs)
*/
int rprio;
uint32_t mask;
+ARMCPU *cpu = ARM_CPU(cs->cpu);
+CPUARMState *env = &cpu->env;
if (icc_no_enabled_hppi(cs)) {
return false;
}
-if (cs->hppi.prio >= cs->icc_pmr_el1) {
+if (cs->gic->nmi_support && cs->hppi.nmi) {
+if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS) &&
+cs->hppi.grp == GICV3_G1NS) {
+if (cs->icc_pmr_el1 < 0x80) {
+return false;
+}
+if (arm_is_secure(env) && cs->icc_pmr_el1 == 0x80) {
+return false;
+}
+}
+} else if (cs->hppi.prio >= cs->icc_pmr_el1) {
/* Priority mask masks this interrupt */
return false;
}
@@ -923,6 +959,18 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs)
return true;
}
+if (cs->gic->nmi_support && cs->hppi.nmi &&
+(cs->hppi.prio & mask) == (rprio & mask)) {
+if ((cs->hppi.grp == GICV3_G1NS) &&
+!(cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI)) {
+return true;
+}
+if ((cs->hppi.grp == GICV3_G1) &&
+!(cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI)) {
+return true;
+}
+}
+
return false;
}
@@ -1044,8 +1092,13 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq)
int aprbit = prio >> (8 - cs->prebits);
int regno = aprbit / 32;
int regbit = aprbit % 32;
+bool nmi = cs->hppi.nmi;
-cs->icc_apr[cs->hppi.grp][regno] |= (1 << regbit);
+if (cs->gic->nmi_support && nmi) {
+cs->icc_apr[cs->hppi.grp][regno] |= ICC_AP1R_EL1_NMI;
+} els
