| Message ID | 20240325084854.3010562-18-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI | expand |
On Mon, 25 Mar 2024 at 08:53, Jinjie Ruan <ruanjinjie@huawei.com> 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 <ruanjinjie@huawei.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > 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 | 115 ++++++++++++++++++++++++++++++++++---- > hw/intc/gicv3_internal.h | 5 ++ > hw/intc/trace-events | 1 + > 3 files changed, 110 insertions(+), 11 deletions(-) > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index e1a60d8c15..76e2286e70 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) > { > /* > @@ -825,11 +832,15 @@ static inline int icc_num_aprs(GICv3CPUState *cs) > return aprmax; > } > > -static int icc_highest_active_prio(GICv3CPUState *cs) > +static uint64_t icc_highest_active_prio(GICv3CPUState *cs) > { > /* Calculate the current running priority based on the set bits > * in the Active Priority Registers. > */ > + ARMCPU *cpu = ARM_CPU(cs->cpu); > + CPUARMState *env = &cpu->env; > + > + uint64_t prio; > int i; > > for (i = 0; i < icc_num_aprs(cs); i++) { > @@ -839,7 +850,32 @@ static int icc_highest_active_prio(GICv3CPUState *cs) > if (!apr) { > continue; > } > - return (i * 32 + ctz32(apr)) << (icc_min_bpr(cs) + 1); > + prio = (i * 32 + ctz32(apr)) << (icc_min_bpr(cs) + 1); > + > + if (cs->gic->nmi_support) { > + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { > + if ((cs->icc_apr[GICV3_G0][i] & ICC_AP1R_EL1_NMI) || > + (cs->icc_apr[GICV3_G1][i] & ICC_AP1R_EL1_NMI) || > + (cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI)) { > + prio |= ICC_RPR_EL1_NMI; > + } > + } else if (!arm_is_secure(env)) { > + if (cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI) { > + prio |= ICC_RPR_EL1_NMI; > + } > + } else { > + if (cs->icc_apr[GICV3_G1][i] & ICC_AP1R_EL1_NMI) { > + prio |= ICC_RPR_EL1_NMI; > + } > + } > + > + if (arm_feature(env, ARM_FEATURE_EL3) && > + cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI) { > + prio |= ICC_RPR_EL1_NSNMI; > + } > + } > + > + return prio; This function is used both for getting the ICC_RPR value, and also in icc_hppi_can_preempt(). So we can't put the special RPR NMI bits in here. Also doing that will not work well with the way the code in icc_rpr_read() adjusts the priority for non-secure accesses. I think we should follow the structure of the pseudocode here, and do the setting of the RPR bits 62 and 63 in icc_rpr_read(). (In the pseudocode this is ICC_RPR_EL1 calling GetHighestActivePriority() and then doing the NMI bits locally.) The NMI bit also exists only in the AP1R0 bit, not in every AP register. So you can check it before the for() loop, something like this: 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. */ prio = 0; 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; } } Then in icc_rpr_read() we can pretty much directly write the same logic that the pseudocode uses to determine whether to set the RPR NMI bits, after the point where we do the shifting of the prio for the NS view: 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_NMI; } } 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; } } } > } > /* No current active interrupts: return idle priority */ > return 0xff; > @@ -896,7 +932,7 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs) > /* Return true if we have a pending interrupt of sufficient > * priority to preempt. > */ > - int rprio; > + uint64_t rprio; You won't need to change the type of this variable with the change above. > uint32_t mask; > > if (icc_no_enabled_hppi(cs)) { icc_hppi_can_preempt() needs more changes than this (check the pseudocode in CanSignalInterrupt(), and the text in 4.8, particularly 4.8.6): * the (cs->hppi.prio >= cs->icc_pmr_el1) check only applies if !cs->hppi.nmi * if this is an NMI and GICD_CTLR.DS is 0 and it's a G1NS interrupt, then we mask if the PMR is < 0x80, or if we're in Secure state and the PMR == 0x80 * if this is an NMI and the (masked) hppi.prio is equal to the (masked) running priority, then we preempt if there's not already an active NMI, ie if the APR NMI bit is clear > @@ -1034,7 +1070,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri, > gicv3_cpuif_update(cs); > } > > -static void icc_activate_irq(GICv3CPUState *cs, int irq) > +static void icc_activate_irq(GICv3CPUState *cs, int irq, bool nmi) > { When activating an interrupt, we set the NMI bit in the priority register based only on the interrupt's config, not on what register was used to activate it. So we don't want a 'bool nmi' argument, we want a local: bool nmi = cs->hppi.nmi; (Compare the pseudocode in the spec: ICC_IAR0_EL1, ICC_IAR1_EL1, and ICC_NMIAR1_EL1 all call AcknowledgeInterrupt(pendID) to activate it.) > /* Move the interrupt from the Pending state to Active, and update > * the Active Priority Registers > @@ -1047,6 +1083,10 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) > > cs->icc_apr[cs->hppi.grp][regno] |= (1 << regbit); > > + if (cs->gic->nmi_support) { > + cs->icc_apr[cs->hppi.grp][regno] |= (nmi ? ICC_AP1R_EL1_NMI : 0); > + } In the APRs, we set only the NMI bit for an NMI and the ordinary priority bit for a non-NMI; so this should be if (cs->gic->nmi_support && nmi) { cs->icc_apr[cs->hppi.grp][regno] |= ICC_AP1R_EL1_NMI; } else { cs->icc_apr[cs->hppi.grp][regno] |= (1 << regbit); } (Otherwise if we had a non-NMI that was interrupted by an NMI at the same priority we wouldn't be able to distinguish this from the NMI interrupting when nothing else was active.) > + > if (irq < GIC_INTERNAL) { > cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); > @@ -1097,7 +1137,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 hppi, > + bool nmi) > { > /* Return the highest priority pending interrupt register value > * for group 1. > @@ -1108,6 +1149,18 @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) > return INTID_SPURIOUS; > } > > + if (!hppi) { > + int el = arm_current_el(env); > + > + if (nmi && (!cs->hppi.nmi)) { > + return INTID_SPURIOUS; > + } > + > + if (!nmi && cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) { > + return INTID_NMI; > + } > + } > + Rather than passing two extra boolean arguments into this function, I think it's better to follow the pseudocode's structure, and have the "should we instead return a special INTID_*" checks be done in the callers of this function. They all end up different so they don't really share code by pushing the checks into this function. > /* 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 > @@ -1149,7 +1202,7 @@ static uint64_t icc_iar0_read(CPUARMState *env, const ARMCPRegInfo *ri) > } > > if (!gicv3_intid_is_special(intid)) { > - icc_activate_irq(cs, intid); > + icc_activate_irq(cs, intid, false); > } > > trace_gicv3_icc_iar0_read(gicv3_redist_affid(cs), intid); > @@ -1168,17 +1221,36 @@ 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); > + icc_activate_irq(cs, intid, false); > } > > 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_nmiar1_read(env, ri); > + } > + > + intid = icc_hppir1_value(cs, env, false, true); > + > + if (!gicv3_intid_is_special(intid)) { > + icc_activate_irq(cs, intid, true); > + } > + > + trace_gicv3_icc_nmiar1_read(gicv3_redist_affid(cs), intid); > + return intid; > +} > + > static void icc_drop_prio(GICv3CPUState *cs, int grp) > { > /* Drop the priority of the currently active interrupt in > @@ -1207,6 +1279,10 @@ static void icc_drop_prio(GICv3CPUState *cs, int grp) > } > /* Clear the lowest set bit */ > *papr &= *papr - 1; > + > + if (cs->gic->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) { > + *papr &= (~ICC_AP1R_EL1_NMI); > + } The NMI bit is only in the AP1R0 register, and if it is set then we should clear only it and not any other AP bits. At the moment this code clears the lowest set bit and also the NMI bit. > break; > } > > @@ -1555,7 +1631,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, true, false); > trace_gicv3_icc_hppir1_read(gicv3_redist_affid(cs), value); > return value; > } > @@ -1693,7 +1769,11 @@ static void icc_ap_write(CPUARMState *env, const ARMCPRegInfo *ri, > return; > } > > - cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU; > + if (cs->gic->nmi_support) { > + cs->icc_apr[grp][regno] = value & (0xFFFFFFFFU | ICC_AP1R_EL1_NMI); > + } else { > + cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU; > + } > gicv3_cpuif_update(cs); > } > > @@ -1783,7 +1863,7 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri, > static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > GICv3CPUState *cs = icc_cs_from_env(env); > - int prio; > + uint64_t prio; > > if (icv_access(env, HCR_FMO | HCR_IMO)) { > return icv_rpr_read(env, ri); > @@ -2482,6 +2562,15 @@ static const ARMCPRegInfo gicv3_cpuif_icc_apxr23_reginfo[] = { > }, > }; > > +static const ARMCPRegInfo gicv3_cpuif_gicv3_nmi_reginfo[] = { > + { .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, > + }, > +}; > + > static uint64_t ich_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > GICv3CPUState *cs = icc_cs_from_env(env); > @@ -2838,6 +2927,10 @@ void gicv3_init_cpuif(GICv3State *s) > */ > define_arm_cp_regs(cpu, gicv3_cpuif_reginfo); > > + if (s->nmi_support) { > + define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); > + } > + > /* > * The CPU implementation specifies the number of supported > * bits of physical priority. For backwards compatibility icc_highest_active_group() also needs to be changed, because if the NMI bit is set in an AP register that is what defines the group of the highest priority active interrupt. Something like this at the top of icc_highest_active_group() should do: + if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) { + return GICV3_G1; + } + if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) { + return GICV3_G1NS; + } thanks -- PMM
On 2024/3/28 22:50, Peter Maydell wrote: > On Mon, 25 Mar 2024 at 08:53, Jinjie Ruan <ruanjinjie@huawei.com> 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 <ruanjinjie@huawei.com> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> 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 | 115 ++++++++++++++++++++++++++++++++++---- >> hw/intc/gicv3_internal.h | 5 ++ >> hw/intc/trace-events | 1 + >> 3 files changed, 110 insertions(+), 11 deletions(-) >> >> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c >> index e1a60d8c15..76e2286e70 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) >> { >> /* >> @@ -825,11 +832,15 @@ static inline int icc_num_aprs(GICv3CPUState *cs) >> return aprmax; >> } >> >> -static int icc_highest_active_prio(GICv3CPUState *cs) >> +static uint64_t icc_highest_active_prio(GICv3CPUState *cs) >> { >> /* Calculate the current running priority based on the set bits >> * in the Active Priority Registers. >> */ >> + ARMCPU *cpu = ARM_CPU(cs->cpu); >> + CPUARMState *env = &cpu->env; >> + >> + uint64_t prio; >> int i; >> >> for (i = 0; i < icc_num_aprs(cs); i++) { >> @@ -839,7 +850,32 @@ static int icc_highest_active_prio(GICv3CPUState *cs) >> if (!apr) { >> continue; >> } >> - return (i * 32 + ctz32(apr)) << (icc_min_bpr(cs) + 1); >> + prio = (i * 32 + ctz32(apr)) << (icc_min_bpr(cs) + 1); >> + >> + if (cs->gic->nmi_support) { >> + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { >> + if ((cs->icc_apr[GICV3_G0][i] & ICC_AP1R_EL1_NMI) || >> + (cs->icc_apr[GICV3_G1][i] & ICC_AP1R_EL1_NMI) || >> + (cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI)) { >> + prio |= ICC_RPR_EL1_NMI; >> + } >> + } else if (!arm_is_secure(env)) { >> + if (cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI) { >> + prio |= ICC_RPR_EL1_NMI; >> + } >> + } else { >> + if (cs->icc_apr[GICV3_G1][i] & ICC_AP1R_EL1_NMI) { >> + prio |= ICC_RPR_EL1_NMI; >> + } >> + } >> + >> + if (arm_feature(env, ARM_FEATURE_EL3) && >> + cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI) { >> + prio |= ICC_RPR_EL1_NSNMI; >> + } >> + } >> + >> + return prio; > > This function is used both for getting the ICC_RPR value, > and also in icc_hppi_can_preempt(). So we can't put the > special RPR NMI bits in here. Also doing that will not work well > with the way the code in icc_rpr_read() adjusts the priority > for non-secure accesses. I think we should follow the structure > of the pseudocode here, and do the setting of the RPR bits 62 and 63 > in icc_rpr_read(). (In the pseudocode this is ICC_RPR_EL1 calling > GetHighestActivePriority() and then doing the NMI bits locally.) > > The NMI bit also exists only in the AP1R0 bit, not in every AP > register. So you can check it before the for() loop, something like this: > > 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. > */ > prio = 0; > 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; > } > } > > Then in icc_rpr_read() we can pretty much directly write the same > logic that the pseudocode uses to determine whether to set the RPR > NMI bits, after the point where we do the shifting of the prio for > the NS view: > > 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_NMI; It seems ICC_RPR_EL1_NSNMI in pseudocode: // GICv3.3 if HaveNMIExt() then if HaveEL(EL3) && (IsNonSecure() || IsRealm()) then pPriority<63> = ICC_AP1R_EL1NS<63>; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ else pPriority<63> = ICC_AP1R_EL1S<63>; pPriority<62> = ICC_AP1R_EL1NS<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; > } > } > } > >> } >> /* No current active interrupts: return idle priority */ >> return 0xff; >> @@ -896,7 +932,7 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs) >> /* Return true if we have a pending interrupt of sufficient >> * priority to preempt. >> */ >> - int rprio; >> + uint64_t rprio; > > You won't need to change the type of this variable with the change above. > >> uint32_t mask; >> >> if (icc_no_enabled_hppi(cs)) { > > icc_hppi_can_preempt() needs more changes than this (check the > pseudocode in CanSignalInterrupt(), and the text in 4.8, particularly > 4.8.6): > > * the (cs->hppi.prio >= cs->icc_pmr_el1) check only applies > if !cs->hppi.nmi > * if this is an NMI and GICD_CTLR.DS is 0 and it's a G1NS > interrupt, then we mask if the PMR is < 0x80, or if > we're in Secure state and the PMR == 0x80 > * if this is an NMI and the (masked) hppi.prio is equal to the > (masked) running priority, then we preempt if there's not > already an active NMI, ie if the APR NMI bit is clear > >> @@ -1034,7 +1070,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> gicv3_cpuif_update(cs); >> } >> >> -static void icc_activate_irq(GICv3CPUState *cs, int irq) >> +static void icc_activate_irq(GICv3CPUState *cs, int irq, bool nmi) >> { > > When activating an interrupt, we set the NMI bit in the > priority register based only on the interrupt's config, > not on what register was used to activate it. So we don't > want a 'bool nmi' argument, we want a local: > bool nmi = cs->hppi.nmi; > > (Compare the pseudocode in the spec: ICC_IAR0_EL1, ICC_IAR1_EL1, > and ICC_NMIAR1_EL1 all call AcknowledgeInterrupt(pendID) > to activate it.) > >> /* Move the interrupt from the Pending state to Active, and update >> * the Active Priority Registers >> @@ -1047,6 +1083,10 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) >> >> cs->icc_apr[cs->hppi.grp][regno] |= (1 << regbit); >> >> + if (cs->gic->nmi_support) { >> + cs->icc_apr[cs->hppi.grp][regno] |= (nmi ? ICC_AP1R_EL1_NMI : 0); >> + } > > In the APRs, we set only the NMI bit for an NMI and the ordinary priority > bit for a non-NMI; so this should be > > if (cs->gic->nmi_support && nmi) { > cs->icc_apr[cs->hppi.grp][regno] |= ICC_AP1R_EL1_NMI; > } else { > cs->icc_apr[cs->hppi.grp][regno] |= (1 << regbit); > } > > (Otherwise if we had a non-NMI that was interrupted by an NMI > at the same priority we wouldn't be able to distinguish this > from the NMI interrupting when nothing else was active.) > >> + >> if (irq < GIC_INTERNAL) { >> cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); >> cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); >> @@ -1097,7 +1137,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 hppi, >> + bool nmi) >> { >> /* Return the highest priority pending interrupt register value >> * for group 1. >> @@ -1108,6 +1149,18 @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) >> return INTID_SPURIOUS; >> } >> >> + if (!hppi) { >> + int el = arm_current_el(env); >> + >> + if (nmi && (!cs->hppi.nmi)) { >> + return INTID_SPURIOUS; >> + } >> + >> + if (!nmi && cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) { >> + return INTID_NMI; >> + } >> + } >> + > > Rather than passing two extra boolean arguments into this > function, I think it's better to follow the pseudocode's > structure, and have the "should we instead return a > special INTID_*" checks be done in the callers of this function. > They all end up different so they don't really share code > by pushing the checks into this function. > >> /* 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 >> @@ -1149,7 +1202,7 @@ static uint64_t icc_iar0_read(CPUARMState *env, const ARMCPRegInfo *ri) >> } >> >> if (!gicv3_intid_is_special(intid)) { >> - icc_activate_irq(cs, intid); >> + icc_activate_irq(cs, intid, false); >> } >> >> trace_gicv3_icc_iar0_read(gicv3_redist_affid(cs), intid); >> @@ -1168,17 +1221,36 @@ 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); >> + icc_activate_irq(cs, intid, false); >> } >> >> 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_nmiar1_read(env, ri); >> + } >> + >> + intid = icc_hppir1_value(cs, env, false, true); >> + >> + if (!gicv3_intid_is_special(intid)) { >> + icc_activate_irq(cs, intid, true); >> + } >> + >> + trace_gicv3_icc_nmiar1_read(gicv3_redist_affid(cs), intid); >> + return intid; >> +} >> + >> static void icc_drop_prio(GICv3CPUState *cs, int grp) >> { >> /* Drop the priority of the currently active interrupt in >> @@ -1207,6 +1279,10 @@ static void icc_drop_prio(GICv3CPUState *cs, int grp) >> } >> /* Clear the lowest set bit */ >> *papr &= *papr - 1; >> + >> + if (cs->gic->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) { >> + *papr &= (~ICC_AP1R_EL1_NMI); >> + } > > The NMI bit is only in the AP1R0 register, and if it is set then > we should clear only it and not any other AP bits. At the moment > this code clears the lowest set bit and also the NMI bit. > >> break; >> } >> >> @@ -1555,7 +1631,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, true, false); >> trace_gicv3_icc_hppir1_read(gicv3_redist_affid(cs), value); >> return value; >> } >> @@ -1693,7 +1769,11 @@ static void icc_ap_write(CPUARMState *env, const ARMCPRegInfo *ri, >> return; >> } >> >> - cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU; >> + if (cs->gic->nmi_support) { >> + cs->icc_apr[grp][regno] = value & (0xFFFFFFFFU | ICC_AP1R_EL1_NMI); >> + } else { >> + cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU; >> + } >> gicv3_cpuif_update(cs); >> } >> >> @@ -1783,7 +1863,7 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri, >> static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri) >> { >> GICv3CPUState *cs = icc_cs_from_env(env); >> - int prio; >> + uint64_t prio; >> >> if (icv_access(env, HCR_FMO | HCR_IMO)) { >> return icv_rpr_read(env, ri); >> @@ -2482,6 +2562,15 @@ static const ARMCPRegInfo gicv3_cpuif_icc_apxr23_reginfo[] = { >> }, >> }; >> >> +static const ARMCPRegInfo gicv3_cpuif_gicv3_nmi_reginfo[] = { >> + { .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, >> + }, >> +}; >> + >> static uint64_t ich_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) >> { >> GICv3CPUState *cs = icc_cs_from_env(env); >> @@ -2838,6 +2927,10 @@ void gicv3_init_cpuif(GICv3State *s) >> */ >> define_arm_cp_regs(cpu, gicv3_cpuif_reginfo); >> >> + if (s->nmi_support) { >> + define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); >> + } >> + >> /* >> * The CPU implementation specifies the number of supported >> * bits of physical priority. For backwards compatibility > > icc_highest_active_group() also needs to be changed, because > if the NMI bit is set in an AP register that is what defines > the group of the highest priority active interrupt. Something > like this at the top of icc_highest_active_group() should do: > > + if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) { > + return GICV3_G1; > + } > + if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) { > + return GICV3_G1NS; > + } > > thanks > -- PMM
On Sat, 30 Mar 2024 at 02:44, Jinjie Ruan via <qemu-arm@nongnu.org> wrote: > > > > On 2024/3/28 22:50, Peter Maydell wrote: > > The NMI bit also exists only in the AP1R0 bit, not in every AP > > register. So you can check it before the for() loop, something like this: > > > > 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. > > */ > > prio = 0; > > 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; > > } > > } > > > > Then in icc_rpr_read() we can pretty much directly write the same > > logic that the pseudocode uses to determine whether to set the RPR > > NMI bits, after the point where we do the shifting of the prio for > > the NS view: > > > > 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_NMI; > > It seems ICC_RPR_EL1_NSNMI in pseudocode: > > // GICv3.3 > if HaveNMIExt() then > if HaveEL(EL3) && (IsNonSecure() || IsRealm()) then > pPriority<63> = ICC_AP1R_EL1NS<63>; > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > else > pPriority<63> = ICC_AP1R_EL1S<63>; > pPriority<62> = ICC_AP1R_EL1NS<63>; I'm not sure what you have in mind here? For QEMU, ICC_AP1R_EL1NS<63> is the ICC_AP1R_EL1_NMI bit in the icc_apr[GICV3_G1NS][0] value, and ICC_RPR_EL1_NMI is bit 63, so the C code seems to me to match up with the pseudocode line that you highlight. thanks -- PMM
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index e1a60d8c15..76e2286e70 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) { /* @@ -825,11 +832,15 @@ static inline int icc_num_aprs(GICv3CPUState *cs) return aprmax; } -static int icc_highest_active_prio(GICv3CPUState *cs) +static uint64_t icc_highest_active_prio(GICv3CPUState *cs) { /* Calculate the current running priority based on the set bits * in the Active Priority Registers. */ + ARMCPU *cpu = ARM_CPU(cs->cpu); + CPUARMState *env = &cpu->env; + + uint64_t prio; int i; for (i = 0; i < icc_num_aprs(cs); i++) { @@ -839,7 +850,32 @@ static int icc_highest_active_prio(GICv3CPUState *cs) if (!apr) { continue; } - return (i * 32 + ctz32(apr)) << (icc_min_bpr(cs) + 1); + prio = (i * 32 + ctz32(apr)) << (icc_min_bpr(cs) + 1); + + if (cs->gic->nmi_support) { + if (cs->gic->gicd_ctlr & GICD_CTLR_DS) { + if ((cs->icc_apr[GICV3_G0][i] & ICC_AP1R_EL1_NMI) || + (cs->icc_apr[GICV3_G1][i] & ICC_AP1R_EL1_NMI) || + (cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI)) { + prio |= ICC_RPR_EL1_NMI; + } + } else if (!arm_is_secure(env)) { + if (cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI) { + prio |= ICC_RPR_EL1_NMI; + } + } else { + if (cs->icc_apr[GICV3_G1][i] & ICC_AP1R_EL1_NMI) { + prio |= ICC_RPR_EL1_NMI; + } + } + + if (arm_feature(env, ARM_FEATURE_EL3) && + cs->icc_apr[GICV3_G1NS][i] & ICC_AP1R_EL1_NMI) { + prio |= ICC_RPR_EL1_NSNMI; + } + } + + return prio; } /* No current active interrupts: return idle priority */ return 0xff; @@ -896,7 +932,7 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs) /* Return true if we have a pending interrupt of sufficient * priority to preempt. */ - int rprio; + uint64_t rprio; uint32_t mask; if (icc_no_enabled_hppi(cs)) { @@ -1034,7 +1070,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri, gicv3_cpuif_update(cs); } -static void icc_activate_irq(GICv3CPUState *cs, int irq) +static void icc_activate_irq(GICv3CPUState *cs, int irq, bool nmi) { /* Move the interrupt from the Pending state to Active, and update * the Active Priority Registers @@ -1047,6 +1083,10 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq) cs->icc_apr[cs->hppi.grp][regno] |= (1 << regbit); + if (cs->gic->nmi_support) { + cs->icc_apr[cs->hppi.grp][regno] |= (nmi ? ICC_AP1R_EL1_NMI : 0); + } + if (irq < GIC_INTERNAL) { cs->gicr_iactiver0 = deposit32(cs->gicr_iactiver0, irq, 1, 1); cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0); @@ -1097,7 +1137,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 hppi, + bool nmi) { /* Return the highest priority pending interrupt register value * for group 1. @@ -1108,6 +1149,18 @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) return INTID_SPURIOUS; } + if (!hppi) { + int el = arm_current_el(env); + + if (nmi && (!cs->hppi.nmi)) { + return INTID_SPURIOUS; + } + + if (!nmi && cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) { + 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 @@ -1149,7 +1202,7 @@ static uint64_t icc_iar0_read(CPUARMState *env, const ARMCPRegInfo *ri) } if (!gicv3_intid_is_special(intid)) { - icc_activate_irq(cs, intid); + icc_activate_irq(cs, intid, false); } trace_gicv3_icc_iar0_read(gicv3_redist_affid(cs), intid); @@ -1168,17 +1221,36 @@ 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); + icc_activate_irq(cs, intid, false); } 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_nmiar1_read(env, ri); + } + + intid = icc_hppir1_value(cs, env, false, true); + + if (!gicv3_intid_is_special(intid)) { + icc_activate_irq(cs, intid, true); + } + + trace_gicv3_icc_nmiar1_read(gicv3_redist_affid(cs), intid); + return intid; +} + static void icc_drop_prio(GICv3CPUState *cs, int grp) { /* Drop the priority of the currently active interrupt in @@ -1207,6 +1279,10 @@ static void icc_drop_prio(GICv3CPUState *cs, int grp) } /* Clear the lowest set bit */ *papr &= *papr - 1; + + if (cs->gic->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) { + *papr &= (~ICC_AP1R_EL1_NMI); + } break; } @@ -1555,7 +1631,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, true, false); trace_gicv3_icc_hppir1_read(gicv3_redist_affid(cs), value); return value; } @@ -1693,7 +1769,11 @@ static void icc_ap_write(CPUARMState *env, const ARMCPRegInfo *ri, return; } - cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU; + if (cs->gic->nmi_support) { + cs->icc_apr[grp][regno] = value & (0xFFFFFFFFU | ICC_AP1R_EL1_NMI); + } else { + cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU; + } gicv3_cpuif_update(cs); } @@ -1783,7 +1863,7 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri, static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri) { GICv3CPUState *cs = icc_cs_from_env(env); - int prio; + uint64_t prio; if (icv_access(env, HCR_FMO | HCR_IMO)) { return icv_rpr_read(env, ri); @@ -2482,6 +2562,15 @@ static const ARMCPRegInfo gicv3_cpuif_icc_apxr23_reginfo[] = { }, }; +static const ARMCPRegInfo gicv3_cpuif_gicv3_nmi_reginfo[] = { + { .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, + }, +}; + static uint64_t ich_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) { GICv3CPUState *cs = icc_cs_from_env(env); @@ -2838,6 +2927,10 @@ void gicv3_init_cpuif(GICv3State *s) */ define_arm_cp_regs(cpu, gicv3_cpuif_reginfo); + if (s->nmi_support) { + define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); + } + /* * The CPU implementation specifies the number of supported * bits of physical priority. For backwards compatibility diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 8d793243f4..81200eb90e 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -194,6 +194,10 @@ FIELD(GICR_VPENDBASER, VALID, 63, 1) #define ICC_CTLR_EL3_A3V (1U << 15) #define ICC_CTLR_EL3_NDS (1U << 17) +#define ICC_AP1R_EL1_NMI (1ULL << 63) +#define ICC_RPR_EL1_NSNMI (1ULL << 62) +#define ICC_RPR_EL1_NMI (1ULL << 63) + #define ICH_VMCR_EL2_VENG0_SHIFT 0 #define ICH_VMCR_EL2_VENG0 (1U << ICH_VMCR_EL2_VENG0_SHIFT) #define ICH_VMCR_EL2_VENG1_SHIFT 1 @@ -511,6 +515,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 */ diff --git a/hw/intc/trace-events b/hw/intc/trace-events index 1ef29d0256..94030550d5 100644 --- a/hw/intc/trace-events +++ b/hw/intc/trace-events @@ -116,6 +116,7 @@ gicv3_cpuif_set_irqs(uint32_t cpuid, int fiqlevel, int irqlevel) "GICv3 CPU i/f gicv3_icc_generate_sgi(uint32_t cpuid, int irq, int irm, uint32_t aff, uint32_t targetlist) "GICv3 CPU i/f 0x%x generating SGI %d IRM %d target affinity 0x%xxx targetlist 0x%x" gicv3_icc_iar0_read(uint32_t cpu, uint64_t val) "GICv3 ICC_IAR0 read cpu 0x%x value 0x%" PRIx64 gicv3_icc_iar1_read(uint32_t cpu, uint64_t val) "GICv3 ICC_IAR1 read cpu 0x%x value 0x%" PRIx64 +gicv3_icc_nmiar1_read(uint32_t cpu, uint64_t val) "GICv3 ICC_NMIAR1 read cpu 0x%x value 0x%" PRIx64 gicv3_icc_eoir_write(int grp, uint32_t cpu, uint64_t val) "GICv3 ICC_EOIR%d write cpu 0x%x value 0x%" PRIx64 gicv3_icc_hppir0_read(uint32_t cpu, uint64_t val) "GICv3 ICC_HPPIR0 read cpu 0x%x value 0x%" PRIx64 gicv3_icc_hppir1_read(uint32_t cpu, uint64_t val) "GICv3 ICC_HPPIR1 read cpu 0x%x value 0x%" PRIx64