Re: [PATCH v10 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers
On Sat, 30 Mar 2024 at 02:44, Jinjie Ruan via 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
Re: [PATCH v10 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers
On 2024/3/28 22:50, Peter Maydell wrote:
> On Mon, 25 Mar 2024 at 08:53, 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
>> ---
>> 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 = >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
>
Re: [PATCH v10 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers
On Mon, 25 Mar 2024 at 08:53, 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
> ---
> 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 = >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
[PATCH v10 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
---
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 = >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)
{
/*
