| Message ID | 20240223103221.1142518-18-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI | expand |
On 2/23/24 00:32, Jinjie Ruan via 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 super priority. And for ICC_NMIAR1_EL1 > register, it should return 1023 if the intid do not have super priority. > Howerever, these are not necessary for ICC_HPPIR1_EL1 register. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > hw/intc/arm_gicv3_cpuif.c | 46 ++++++++++++++++++++++++++++++++++++--- > hw/intc/gicv3_internal.h | 1 + > 2 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index e1a60d8c15..f5bf8df32b 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -1097,7 +1097,8 @@ static uint64_t icc_hppir0_value(GICv3CPUState *cs, CPUARMState *env) > return cs->hppi.irq; > } > > -static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) > +static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env, > + bool is_nmi, bool is_hppi) > { > /* Return the highest priority pending interrupt register value > * for group 1. > @@ -1108,6 +1109,16 @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) > return INTID_SPURIOUS; > } > > + if (!is_hppi) { > + if (is_nmi && (!cs->hppi.superprio)) { > + return INTID_SPURIOUS; > + } > + > + if ((!is_nmi) && cs->hppi.superprio) { > + return INTID_NMI; > + } > + } > + > /* Check whether we can return the interrupt or if we should return > * a special identifier, as per the CheckGroup1ForSpecialIdentifiers > * pseudocode. (We can simplify a little because for us ICC_SRE_EL1.RM > @@ -1168,7 +1179,30 @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri) > if (!icc_hppi_can_preempt(cs)) { > intid = INTID_SPURIOUS; > } else { > - intid = icc_hppir1_value(cs, env); > + intid = icc_hppir1_value(cs, env, false, false); > + } > + > + if (!gicv3_intid_is_special(intid)) { > + icc_activate_irq(cs, intid); > + } > + > + trace_gicv3_icc_iar1_read(gicv3_redist_affid(cs), intid); > + return intid; > +} This is incorrect. For icc_iar1_read, you need something like if (!is_hppi && cs->hppi.superprio && env->cp15.sctlr_el[current_el] & SCTLR_NMI) { return INTID_NMI; } I think that if SCTLR_NMI is not set, the whole system ignores Superpriority entirely, so returning SPURIOUS here would be incorrect. This would make sense, letting an OS that is not configured for FEAT_NMI to run on ARMv8.8 hardware without modification. > + > +static uint64_t icc_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + GICv3CPUState *cs = icc_cs_from_env(env); > + uint64_t intid; > + > + if (icv_access(env, HCR_IMO)) { > + return icv_iar_read(env, ri); > + } > + > + if (!icc_hppi_can_preempt(cs)) { > + intid = INTID_SPURIOUS; > + } else { > + intid = icc_hppir1_value(cs, env, true, false); Here... believe that the result *should* only consider superpriority. I guess SPURIOUS is the correct result when there is no pending interrupt with superpriority? It's really unclear to me from the register description. Peter? > @@ -2344,6 +2378,12 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = { > .access = PL1_R, .accessfn = gicv3_irq_access, > .readfn = icc_iar1_read, > }, > + { .name = "ICC_NMIAR1_EL1", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 9, .opc2 = 5, > + .type = ARM_CP_IO | ARM_CP_NO_RAW, > + .access = PL1_R, .accessfn = gicv3_irq_access, > + .readfn = icc_nmiar1_read, > + }, This register is UNDEFINED if FEAT_GICv3_NMI is not implemented. You need to register this separately. r~
On 2024/2/24 4:52, Richard Henderson wrote: > On 2/23/24 00:32, Jinjie Ruan via 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 super priority. And for >> ICC_NMIAR1_EL1 >> register, it should return 1023 if the intid do not have super priority. >> Howerever, these are not necessary for ICC_HPPIR1_EL1 register. >> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> hw/intc/arm_gicv3_cpuif.c | 46 ++++++++++++++++++++++++++++++++++++--- >> hw/intc/gicv3_internal.h | 1 + >> 2 files changed, 44 insertions(+), 3 deletions(-) >> >> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c >> index e1a60d8c15..f5bf8df32b 100644 >> --- a/hw/intc/arm_gicv3_cpuif.c >> +++ b/hw/intc/arm_gicv3_cpuif.c >> @@ -1097,7 +1097,8 @@ static uint64_t icc_hppir0_value(GICv3CPUState >> *cs, CPUARMState *env) >> return cs->hppi.irq; >> } >> -static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) >> +static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env, >> + bool is_nmi, bool is_hppi) >> { >> /* Return the highest priority pending interrupt register value >> * for group 1. >> @@ -1108,6 +1109,16 @@ static uint64_t icc_hppir1_value(GICv3CPUState >> *cs, CPUARMState *env) >> return INTID_SPURIOUS; >> } >> + if (!is_hppi) { >> + if (is_nmi && (!cs->hppi.superprio)) { >> + return INTID_SPURIOUS; >> + } >> + >> + if ((!is_nmi) && cs->hppi.superprio) { >> + return INTID_NMI; >> + } >> + } >> + >> /* Check whether we can return the interrupt or if we should return >> * a special identifier, as per the >> CheckGroup1ForSpecialIdentifiers >> * pseudocode. (We can simplify a little because for us >> ICC_SRE_EL1.RM >> @@ -1168,7 +1179,30 @@ static uint64_t icc_iar1_read(CPUARMState *env, >> const ARMCPRegInfo *ri) >> if (!icc_hppi_can_preempt(cs)) { >> intid = INTID_SPURIOUS; >> } else { >> - intid = icc_hppir1_value(cs, env); >> + intid = icc_hppir1_value(cs, env, false, false); >> + } >> + >> + if (!gicv3_intid_is_special(intid)) { >> + icc_activate_irq(cs, intid); >> + } >> + >> + trace_gicv3_icc_iar1_read(gicv3_redist_affid(cs), intid); >> + return intid; >> +} > > This is incorrect. For icc_iar1_read, you need something like > > if (!is_hppi > && cs->hppi.superprio > && env->cp15.sctlr_el[current_el] & SCTLR_NMI) { > return INTID_NMI; > } > > I think that if SCTLR_NMI is not set, the whole system ignores > Superpriority entirely, so returning SPURIOUS here would be incorrect. > This would make sense, letting an OS that is not configured for FEAT_NMI > to run on ARMv8.8 hardware without modification. You are right. SCTLR_ELx.NMI decide whether IRQ and FIQ interrupts to have Superpriority as an additional attribute. > > >> + >> +static uint64_t icc_nmiar1_read(CPUARMState *env, const ARMCPRegInfo >> *ri) >> +{ >> + GICv3CPUState *cs = icc_cs_from_env(env); >> + uint64_t intid; >> + >> + if (icv_access(env, HCR_IMO)) { >> + return icv_iar_read(env, ri); >> + } >> + >> + if (!icc_hppi_can_preempt(cs)) { >> + intid = INTID_SPURIOUS; >> + } else { >> + intid = icc_hppir1_value(cs, env, true, false); > > Here... believe that the result *should* only consider superpriority. I > guess SPURIOUS is the correct result when there is no pending interrupt > with superpriority? It's really unclear to me from the register > description. > > Peter? > >> @@ -2344,6 +2378,12 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] >> = { >> .access = PL1_R, .accessfn = gicv3_irq_access, >> .readfn = icc_iar1_read, >> }, >> + { .name = "ICC_NMIAR1_EL1", .state = ARM_CP_STATE_BOTH, >> + .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 9, .opc2 = 5, >> + .type = ARM_CP_IO | ARM_CP_NO_RAW, >> + .access = PL1_R, .accessfn = gicv3_irq_access, >> + .readfn = icc_nmiar1_read, >> + }, > > This register is UNDEFINED if FEAT_GICv3_NMI is not implemented. > You need to register this separately. > > > r~
On Fri, 23 Feb 2024 at 20:53, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/23/24 00:32, Jinjie Ruan via 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 super priority. And for ICC_NMIAR1_EL1 > > register, it should return 1023 if the intid do not have super priority. > > Howerever, these are not necessary for ICC_HPPIR1_EL1 register. > > > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > > +static uint64_t icc_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri) > > +{ > > + GICv3CPUState *cs = icc_cs_from_env(env); > > + uint64_t intid; > > + > > + if (icv_access(env, HCR_IMO)) { > > + return icv_iar_read(env, ri); > > + } > > + > > + if (!icc_hppi_can_preempt(cs)) { > > + intid = INTID_SPURIOUS; > > + } else { > > + intid = icc_hppir1_value(cs, env, true, false); > > Here... believe that the result *should* only consider superpriority. I guess SPURIOUS is > the correct result when there is no pending interrupt with superpriority? It's really > unclear to me from the register description. Should be 1023: the ICC_NMIAR1_EL1[] pseudocode in the GIC architecture spec (13.1.8) does this: if !IsNMI(intID) then return ZeroExtend(INTID_SPURIOUS); (Note that the logic is "find the highest priority pending interrupt, and then see if it is an NMI or not", not "find the highest priority pending NMI".) -- PMM
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index e1a60d8c15..f5bf8df32b 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -1097,7 +1097,8 @@ static uint64_t icc_hppir0_value(GICv3CPUState *cs, CPUARMState *env) return cs->hppi.irq; } -static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) +static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env, + bool is_nmi, bool is_hppi) { /* Return the highest priority pending interrupt register value * for group 1. @@ -1108,6 +1109,16 @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) return INTID_SPURIOUS; } + if (!is_hppi) { + if (is_nmi && (!cs->hppi.superprio)) { + return INTID_SPURIOUS; + } + + if ((!is_nmi) && cs->hppi.superprio) { + return INTID_NMI; + } + } + /* Check whether we can return the interrupt or if we should return * a special identifier, as per the CheckGroup1ForSpecialIdentifiers * pseudocode. (We can simplify a little because for us ICC_SRE_EL1.RM @@ -1168,7 +1179,30 @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri) if (!icc_hppi_can_preempt(cs)) { intid = INTID_SPURIOUS; } else { - intid = icc_hppir1_value(cs, env); + intid = icc_hppir1_value(cs, env, false, false); + } + + if (!gicv3_intid_is_special(intid)) { + icc_activate_irq(cs, intid); + } + + trace_gicv3_icc_iar1_read(gicv3_redist_affid(cs), intid); + return intid; +} + +static uint64_t icc_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri) +{ + GICv3CPUState *cs = icc_cs_from_env(env); + uint64_t intid; + + if (icv_access(env, HCR_IMO)) { + return icv_iar_read(env, ri); + } + + if (!icc_hppi_can_preempt(cs)) { + intid = INTID_SPURIOUS; + } else { + intid = icc_hppir1_value(cs, env, true, false); } if (!gicv3_intid_is_special(intid)) { @@ -1555,7 +1589,7 @@ static uint64_t icc_hppir1_read(CPUARMState *env, const ARMCPRegInfo *ri) return icv_hppir_read(env, ri); } - value = icc_hppir1_value(cs, env); + value = icc_hppir1_value(cs, env, false, true); trace_gicv3_icc_hppir1_read(gicv3_redist_affid(cs), value); return value; } @@ -2344,6 +2378,12 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = { .access = PL1_R, .accessfn = gicv3_irq_access, .readfn = icc_iar1_read, }, + { .name = "ICC_NMIAR1_EL1", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 9, .opc2 = 5, + .type = ARM_CP_IO | ARM_CP_NO_RAW, + .access = PL1_R, .accessfn = gicv3_irq_access, + .readfn = icc_nmiar1_read, + }, { .name = "ICC_EOIR1_EL1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 12, .opc2 = 1, .type = ARM_CP_IO | ARM_CP_NO_RAW, diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 8d793243f4..93e56b3726 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -511,6 +511,7 @@ FIELD(VTE, RDBASE, 42, RDBASE_PROCNUM_LENGTH) /* Special interrupt IDs */ #define INTID_SECURE 1020 #define INTID_NONSECURE 1021 +#define INTID_NMI 1022 #define INTID_SPURIOUS 1023 /* Functions internal to the emulated GICv3 */
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 super priority. And for ICC_NMIAR1_EL1 register, it should return 1023 if the intid do not have super priority. Howerever, these are not necessary for ICC_HPPIR1_EL1 register. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- hw/intc/arm_gicv3_cpuif.c | 46 ++++++++++++++++++++++++++++++++++++--- hw/intc/gicv3_internal.h | 1 + 2 files changed, 44 insertions(+), 3 deletions(-)