diff mbox series

[v11,18/23] hw/intc/arm_gicv3: Handle icv_nmiar1_read() for icc_nmiar1_read()

Message ID 20240330103128.3185962-19-ruanjinjie@huawei.com (mailing list archive)
State New, archived
Headers show
Series target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI | expand

Commit Message

Jinjie Ruan March 30, 2024, 10:31 a.m. UTC
Implement icv_nmiar1_read() for icc_nmiar1_read(), so add definition for
ICH_LR_EL2.NMI and ICH_AP1R_EL2.NMI bit.

If FEAT_GICv3_NMI is supported, ich_ap_write() should consider ICV_AP1R_EL1.NMI
bit. In icv_activate_irq() and icv_eoir_write(), the ICV_AP1R_EL1.NMI bit
should be set or clear according to the Non-maskable property. And the RPR
priority should also update the NMI bit according to the APR priority NMI bit.

By the way, add gicv3_icv_nmiar1_read trace event.

If the hpp irq is a NMI, the icv iar read should return 1022 and trap for
NMI again

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
v11:
- Deal with NMI in the callers instead of ich_highest_active_virt_prio().
- Set either NMI or a group-priority bit, not both.
- Only set AP NMI bits in the 0 reg.
- Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
v10:
- Rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
- Add ICV_RPR_EL1_NMI definition.
- Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
  ich_highest_active_virt_prio().
v9:
- Correct the INTID_NMI logic.
v8:
- Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
v7:
- Add Reviewed-by.
v6:
- Implement icv_nmiar1_read().
---
 hw/intc/arm_gicv3_cpuif.c | 97 ++++++++++++++++++++++++++++++++++-----
 hw/intc/gicv3_internal.h  |  4 ++
 hw/intc/trace-events      |  1 +
 3 files changed, 91 insertions(+), 11 deletions(-)

Comments

Peter Maydell April 2, 2024, 4:12 p.m. UTC | #1
On Sat, 30 Mar 2024 at 10:33, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
> Implement icv_nmiar1_read() for icc_nmiar1_read(), so add definition for
> ICH_LR_EL2.NMI and ICH_AP1R_EL2.NMI bit.
>
> If FEAT_GICv3_NMI is supported, ich_ap_write() should consider ICV_AP1R_EL1.NMI
> bit. In icv_activate_irq() and icv_eoir_write(), the ICV_AP1R_EL1.NMI bit
> should be set or clear according to the Non-maskable property. And the RPR
> priority should also update the NMI bit according to the APR priority NMI bit.
>
> By the way, add gicv3_icv_nmiar1_read trace event.
>
> If the hpp irq is a NMI, the icv iar read should return 1022 and trap for
> NMI again
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v11:
> - Deal with NMI in the callers instead of ich_highest_active_virt_prio().
> - Set either NMI or a group-priority bit, not both.
> - Only set AP NMI bits in the 0 reg.
> - Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
> v10:
> - Rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
> - Add ICV_RPR_EL1_NMI definition.
> - Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
>   ich_highest_active_virt_prio().
> v9:
> - Correct the INTID_NMI logic.
> v8:
> - Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
> v7:
> - Add Reviewed-by.
> v6:
> - Implement icv_nmiar1_read().
> ---
>  hw/intc/arm_gicv3_cpuif.c | 97 ++++++++++++++++++++++++++++++++++-----
>  hw/intc/gicv3_internal.h  |  4 ++
>  hw/intc/trace-events      |  1 +
>  3 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index f99f2570a6..a7bc44b30c 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -157,6 +157,10 @@ static int ich_highest_active_virt_prio(GICv3CPUState *cs)
>      int i;
>      int aprmax = ich_num_aprs(cs);
>
> +    if (cs->gic->nmi_support && cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI) {
> +        return 0x80;

This should be 0, not 0x80 -- see the register description for
ICH_LR<n>_EL2 Priority field: when NMI is 1, the virtual interrupt's
priority is treated as 0x0.

> +    }
> +
>      for (i = 0; i < aprmax; i++) {
>          uint32_t apr = cs->ich_apr[GICV3_G0][i] |
>              cs->ich_apr[GICV3_G1NS][i];
> @@ -191,6 +195,7 @@ static int hppvi_index(GICv3CPUState *cs)
>       * correct behaviour.
>       */
>      int prio = 0xff;
> +    bool nmi = false;
>
>      if (!(cs->ich_vmcr_el2 & (ICH_VMCR_EL2_VENG0 | ICH_VMCR_EL2_VENG1))) {
>          /* Both groups disabled, definitely nothing to do */
> @@ -200,6 +205,11 @@ static int hppvi_index(GICv3CPUState *cs)
>      for (i = 0; i < cs->num_list_regs; i++) {
>          uint64_t lr = cs->ich_lr_el2[i];
>          int thisprio;
> +        bool thisnmi = false;
> +
> +        if (cs->gic->nmi_support) {
> +            thisnmi = lr & ICH_LR_EL2_NMI;
> +        }

You could write this a little more concisely as
      bool thisnmi = cs->gic_nmi_support && (lr & ICH_LR_EL2_NMI);

>          if (ich_lr_state(lr) != ICH_LR_EL2_STATE_PENDING) {
>              /* Not Pending */
> @@ -219,9 +229,13 @@ static int hppvi_index(GICv3CPUState *cs)
>
>          thisprio = ich_lr_prio(lr);
>
> -        if (thisprio < prio) {
> +        if ((thisprio < prio) || ((thisprio == prio) && (thisnmi & (!nmi)))) {
>              prio = thisprio;
>              idx = i;
> +
> +            if (cs->gic->nmi_support) {
> +                nmi = thisnmi;
> +            }

You don't need to check nmi_support here because if nmi_support
is false then thisnmi will also be false, and so we will never
set nmi to anything other than false.

>          }
>      }
>
> @@ -326,6 +340,12 @@ static bool icv_hppi_can_preempt(GICv3CPUState *cs, uint64_t lr)
>          return true;
>      }
>
> +    if ((prio & mask) == (rprio & mask) &&
> +        cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI) &&
> +        (!(cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI))) {
> +        return true;
> +    }
> +
>      return false;
>  }

This isn't the only change needed to icv_hppi_can_preempt():
virtual NMIs are never masked by the MPR, so that check also
must be changed. If we pull out a variable:

    bool is_nmi = cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI);

we can use that both to gate the vpmr check:

    if (!is_nmi && prio >= vpmr) {

and also in the priority check you have here.

> @@ -736,13 +765,19 @@ static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp)
>       */
>      uint32_t mask = icv_gprio_mask(cs, grp);
>      int prio = ich_lr_prio(cs->ich_lr_el2[idx]) & mask;
> +    bool nmi = cs->ich_lr_el2[idx] & ICH_LR_EL2_NMI;
>      int aprbit = prio >> (8 - cs->vprebits);
>      int regno = aprbit / 32;
>      int regbit = aprbit % 32;
>
>      cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>      cs->ich_lr_el2[idx] |= ICH_LR_EL2_STATE_ACTIVE_BIT;
> -    cs->ich_apr[grp][regno] |= (1 << regbit);
> +
> +    if (cs->gic->nmi_support && nmi) {
> +        cs->ich_apr[grp][regno] |= ICV_AP1R_EL1_NMI;
> +    } else {
> +        cs->ich_apr[grp][regno] |= (1 << regbit);
> +    }
>  }
>
>  static void icv_activate_vlpi(GICv3CPUState *cs)
> @@ -776,7 +811,11 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
>          if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
>              intid = ich_lr_vintid(lr);
>              if (!gicv3_intid_is_special(intid)) {
> -                icv_activate_irq(cs, idx, grp);
> +                if (!(lr & ICH_LR_EL2_NMI)) {

This is missing checks on both whether the GIC has NMI support and
on whether the SCTLR NMI bit is set (compare pseudocode
VirtualReadIAR1()). I suggest defining a

        bool nmi = cs->gic->nmi_support &&
            (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_NMI) &&
            (lr & ICH_LR_EL2_NMI);

and then you can check "if (!nmi)" here.

> +                    icv_activate_irq(cs, idx, grp);
> +                } else {
> +                    intid = INTID_NMI;
> +                }
>              } else {
>                  /* Interrupt goes from Pending to Invalid */
>                  cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
> @@ -797,8 +836,32 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
>
>  static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> -    /* todo */
> +    GICv3CPUState *cs = icc_cs_from_env(env);
> +    int idx = hppvi_index(cs);
>      uint64_t intid = INTID_SPURIOUS;
> +
> +    if (idx >= 0 && idx != HPPVI_INDEX_VLPI) {
> +        uint64_t lr = cs->ich_lr_el2[idx];
> +        int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
> +
> +        if ((thisgrp == GICV3_G1NS) && (lr & ICH_LR_EL2_NMI)) {
> +            intid = ich_lr_vintid(lr);
> +            if (!gicv3_intid_is_special(intid)) {
> +                icv_activate_irq(cs, idx, GICV3_G1NS);
> +            } else {
> +                /* Interrupt goes from Pending to Invalid */
> +                cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
> +                /* We will now return the (bogus) ID from the list register,
> +                 * as per the pseudocode.
> +                 */

QEMU's coding style wants the /* on a line of its own for a
multiline comment.

> +            }
> +        }
> +    }
> +
> +    trace_gicv3_icv_nmiar1_read(gicv3_redist_affid(cs), intid);
> +
> +    gicv3_cpuif_virt_update(cs);
> +
>      return intid;
>  }

> @@ -1504,6 +1573,7 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      int irq = value & 0xffffff;
>      int grp = ri->crm == 8 ? GICV3_G0 : GICV3_G1NS;
>      int idx, dropprio;
> +    bool nmi = false;
>
>      trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1,
>                                 gicv3_redist_affid(cs), value);
> @@ -1516,8 +1586,8 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>       * error checks" (because that lets us avoid scanning the AP
>       * registers twice).
>       */
> -    dropprio = icv_drop_prio(cs);
> -    if (dropprio == 0xff) {
> +    dropprio = icv_drop_prio(cs, &nmi);
> +    if (dropprio == 0xff && !nmi) {
>          /* No active interrupt. It is CONSTRAINED UNPREDICTABLE
>           * whether the list registers are checked in this
>           * situation; we choose not to.
> @@ -1539,8 +1609,9 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          uint64_t lr = cs->ich_lr_el2[idx];
>          int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
>          int lr_gprio = ich_lr_prio(lr) & icv_gprio_mask(cs, grp);
> +        int thisnmi = lr & ICH_LR_EL2_NMI;

This variable should be "bool". An "int" is 32 bits, so because
ICH_LR_EL2_NMI is bit 59, the value of "lr & ICH_LR_EL2_NMI" when
cast to int is always zero. Using bool avoids this bug and also
is consistent with the type we used for the 'nmi' variable we're
about to compare it with.

> -        if (thisgrp == grp && lr_gprio == dropprio) {
> +        if (thisgrp == grp && (lr_gprio == dropprio || thisnmi == nmi)) {
>              if (!icv_eoi_split(env, cs) || irq >= GICV3_LPI_INTID_START) {
>                  /*
>                   * Priority drop and deactivate not split: deactivate irq now.
> @@ -2626,7 +2697,11 @@ static void ich_ap_write(CPUARMState *env, const ARMCPRegInfo *ri,
>
>      trace_gicv3_ich_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs), value);
>
> -    cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
> +    if (cs->gic->nmi_support) {
> +        cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
> +    } else {
> +        cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
> +    }
>      gicv3_cpuif_virt_irq_fiq_update(cs);
>  }

thanks
-- PMM
Jinjie Ruan April 3, 2024, 2:21 a.m. UTC | #2
On 2024/4/3 0:12, Peter Maydell wrote:
> On Sat, 30 Mar 2024 at 10:33, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>> Implement icv_nmiar1_read() for icc_nmiar1_read(), so add definition for
>> ICH_LR_EL2.NMI and ICH_AP1R_EL2.NMI bit.
>>
>> If FEAT_GICv3_NMI is supported, ich_ap_write() should consider ICV_AP1R_EL1.NMI
>> bit. In icv_activate_irq() and icv_eoir_write(), the ICV_AP1R_EL1.NMI bit
>> should be set or clear according to the Non-maskable property. And the RPR
>> priority should also update the NMI bit according to the APR priority NMI bit.
>>
>> By the way, add gicv3_icv_nmiar1_read trace event.
>>
>> If the hpp irq is a NMI, the icv iar read should return 1022 and trap for
>> NMI again
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> v11:
>> - Deal with NMI in the callers instead of ich_highest_active_virt_prio().
>> - Set either NMI or a group-priority bit, not both.
>> - Only set AP NMI bits in the 0 reg.
>> - Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
>> v10:
>> - Rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
>> - Add ICV_RPR_EL1_NMI definition.
>> - Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
>>   ich_highest_active_virt_prio().
>> v9:
>> - Correct the INTID_NMI logic.
>> v8:
>> - Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
>> v7:
>> - Add Reviewed-by.
>> v6:
>> - Implement icv_nmiar1_read().
>> ---
>>  hw/intc/arm_gicv3_cpuif.c | 97 ++++++++++++++++++++++++++++++++++-----
>>  hw/intc/gicv3_internal.h  |  4 ++
>>  hw/intc/trace-events      |  1 +
>>  3 files changed, 91 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
>> index f99f2570a6..a7bc44b30c 100644
>> --- a/hw/intc/arm_gicv3_cpuif.c
>> +++ b/hw/intc/arm_gicv3_cpuif.c
>> @@ -157,6 +157,10 @@ static int ich_highest_active_virt_prio(GICv3CPUState *cs)
>>      int i;
>>      int aprmax = ich_num_aprs(cs);
>>
>> +    if (cs->gic->nmi_support && cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI) {
>> +        return 0x80;
> 
> This should be 0, not 0x80 -- see the register description for
> ICH_LR<n>_EL2 Priority field: when NMI is 1, the virtual interrupt's
> priority is treated as 0x0.
> 
>> +    }
>> +
>>      for (i = 0; i < aprmax; i++) {
>>          uint32_t apr = cs->ich_apr[GICV3_G0][i] |
>>              cs->ich_apr[GICV3_G1NS][i];
>> @@ -191,6 +195,7 @@ static int hppvi_index(GICv3CPUState *cs)
>>       * correct behaviour.
>>       */
>>      int prio = 0xff;
>> +    bool nmi = false;
>>
>>      if (!(cs->ich_vmcr_el2 & (ICH_VMCR_EL2_VENG0 | ICH_VMCR_EL2_VENG1))) {
>>          /* Both groups disabled, definitely nothing to do */
>> @@ -200,6 +205,11 @@ static int hppvi_index(GICv3CPUState *cs)
>>      for (i = 0; i < cs->num_list_regs; i++) {
>>          uint64_t lr = cs->ich_lr_el2[i];
>>          int thisprio;
>> +        bool thisnmi = false;
>> +
>> +        if (cs->gic->nmi_support) {
>> +            thisnmi = lr & ICH_LR_EL2_NMI;
>> +        }
> 
> You could write this a little more concisely as
>       bool thisnmi = cs->gic_nmi_support && (lr & ICH_LR_EL2_NMI);

If the following if check continues, the operations are unnecessary, so
I think it's more appropriate to put it as thisprio operations do it.

> 
>>          if (ich_lr_state(lr) != ICH_LR_EL2_STATE_PENDING) {
>>              /* Not Pending */
>> @@ -219,9 +229,13 @@ static int hppvi_index(GICv3CPUState *cs)
>>
>>          thisprio = ich_lr_prio(lr);
>>
>> -        if (thisprio < prio) {
>> +        if ((thisprio < prio) || ((thisprio == prio) && (thisnmi & (!nmi)))) {
>>              prio = thisprio;
>>              idx = i;
>> +
>> +            if (cs->gic->nmi_support) {
>> +                nmi = thisnmi;
>> +            }
> 
> You don't need to check nmi_support here because if nmi_support
> is false then thisnmi will also be false, and so we will never
> set nmi to anything other than false.
> 
>>          }
>>      }
>>
>> @@ -326,6 +340,12 @@ static bool icv_hppi_can_preempt(GICv3CPUState *cs, uint64_t lr)
>>          return true;
>>      }
>>
>> +    if ((prio & mask) == (rprio & mask) &&
>> +        cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI) &&
>> +        (!(cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI))) {
>> +        return true;
>> +    }
>> +
>>      return false;
>>  }
> 
> This isn't the only change needed to icv_hppi_can_preempt():
> virtual NMIs are never masked by the MPR, so that check also
> must be changed. If we pull out a variable:
> 
>     bool is_nmi = cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI);
> 
> we can use that both to gate the vpmr check:
> 
>     if (!is_nmi && prio >= vpmr) {
> 
> and also in the priority check you have here.
> 
>> @@ -736,13 +765,19 @@ static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp)
>>       */
>>      uint32_t mask = icv_gprio_mask(cs, grp);
>>      int prio = ich_lr_prio(cs->ich_lr_el2[idx]) & mask;
>> +    bool nmi = cs->ich_lr_el2[idx] & ICH_LR_EL2_NMI;
>>      int aprbit = prio >> (8 - cs->vprebits);
>>      int regno = aprbit / 32;
>>      int regbit = aprbit % 32;
>>
>>      cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>>      cs->ich_lr_el2[idx] |= ICH_LR_EL2_STATE_ACTIVE_BIT;
>> -    cs->ich_apr[grp][regno] |= (1 << regbit);
>> +
>> +    if (cs->gic->nmi_support && nmi) {
>> +        cs->ich_apr[grp][regno] |= ICV_AP1R_EL1_NMI;
>> +    } else {
>> +        cs->ich_apr[grp][regno] |= (1 << regbit);
>> +    }
>>  }
>>
>>  static void icv_activate_vlpi(GICv3CPUState *cs)
>> @@ -776,7 +811,11 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>          if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
>>              intid = ich_lr_vintid(lr);
>>              if (!gicv3_intid_is_special(intid)) {
>> -                icv_activate_irq(cs, idx, grp);
>> +                if (!(lr & ICH_LR_EL2_NMI)) {
> 
> This is missing checks on both whether the GIC has NMI support and
> on whether the SCTLR NMI bit is set (compare pseudocode
> VirtualReadIAR1()). I suggest defining a
> 
>         bool nmi = cs->gic->nmi_support &&
>             (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_NMI) &&
>             (lr & ICH_LR_EL2_NMI);
> 
> and then you can check "if (!nmi)" here.
> 
>> +                    icv_activate_irq(cs, idx, grp);
>> +                } else {
>> +                    intid = INTID_NMI;
>> +                }
>>              } else {
>>                  /* Interrupt goes from Pending to Invalid */
>>                  cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>> @@ -797,8 +836,32 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>
>>  static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>  {
>> -    /* todo */
>> +    GICv3CPUState *cs = icc_cs_from_env(env);
>> +    int idx = hppvi_index(cs);
>>      uint64_t intid = INTID_SPURIOUS;
>> +
>> +    if (idx >= 0 && idx != HPPVI_INDEX_VLPI) {
>> +        uint64_t lr = cs->ich_lr_el2[idx];
>> +        int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
>> +
>> +        if ((thisgrp == GICV3_G1NS) && (lr & ICH_LR_EL2_NMI)) {
>> +            intid = ich_lr_vintid(lr);
>> +            if (!gicv3_intid_is_special(intid)) {
>> +                icv_activate_irq(cs, idx, GICV3_G1NS);
>> +            } else {
>> +                /* Interrupt goes from Pending to Invalid */
>> +                cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>> +                /* We will now return the (bogus) ID from the list register,
>> +                 * as per the pseudocode.
>> +                 */
> 
> QEMU's coding style wants the /* on a line of its own for a
> multiline comment.
> 
>> +            }
>> +        }
>> +    }
>> +
>> +    trace_gicv3_icv_nmiar1_read(gicv3_redist_affid(cs), intid);
>> +
>> +    gicv3_cpuif_virt_update(cs);
>> +
>>      return intid;
>>  }
> 
>> @@ -1504,6 +1573,7 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      int irq = value & 0xffffff;
>>      int grp = ri->crm == 8 ? GICV3_G0 : GICV3_G1NS;
>>      int idx, dropprio;
>> +    bool nmi = false;
>>
>>      trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1,
>>                                 gicv3_redist_affid(cs), value);
>> @@ -1516,8 +1586,8 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>       * error checks" (because that lets us avoid scanning the AP
>>       * registers twice).
>>       */
>> -    dropprio = icv_drop_prio(cs);
>> -    if (dropprio == 0xff) {
>> +    dropprio = icv_drop_prio(cs, &nmi);
>> +    if (dropprio == 0xff && !nmi) {
>>          /* No active interrupt. It is CONSTRAINED UNPREDICTABLE
>>           * whether the list registers are checked in this
>>           * situation; we choose not to.
>> @@ -1539,8 +1609,9 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>          uint64_t lr = cs->ich_lr_el2[idx];
>>          int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
>>          int lr_gprio = ich_lr_prio(lr) & icv_gprio_mask(cs, grp);
>> +        int thisnmi = lr & ICH_LR_EL2_NMI;
> 
> This variable should be "bool". An "int" is 32 bits, so because
> ICH_LR_EL2_NMI is bit 59, the value of "lr & ICH_LR_EL2_NMI" when
> cast to int is always zero. Using bool avoids this bug and also
> is consistent with the type we used for the 'nmi' variable we're
> about to compare it with.
> 
>> -        if (thisgrp == grp && lr_gprio == dropprio) {
>> +        if (thisgrp == grp && (lr_gprio == dropprio || thisnmi == nmi)) {
>>              if (!icv_eoi_split(env, cs) || irq >= GICV3_LPI_INTID_START) {
>>                  /*
>>                   * Priority drop and deactivate not split: deactivate irq now.
>> @@ -2626,7 +2697,11 @@ static void ich_ap_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>
>>      trace_gicv3_ich_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs), value);
>>
>> -    cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
>> +    if (cs->gic->nmi_support) {
>> +        cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
>> +    } else {
>> +        cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
>> +    }
>>      gicv3_cpuif_virt_irq_fiq_update(cs);
>>  }
> 
> thanks
> -- PMM
Jinjie Ruan April 3, 2024, 3:16 a.m. UTC | #3
On 2024/4/3 0:12, Peter Maydell wrote:
> On Sat, 30 Mar 2024 at 10:33, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>> Implement icv_nmiar1_read() for icc_nmiar1_read(), so add definition for
>> ICH_LR_EL2.NMI and ICH_AP1R_EL2.NMI bit.
>>
>> If FEAT_GICv3_NMI is supported, ich_ap_write() should consider ICV_AP1R_EL1.NMI
>> bit. In icv_activate_irq() and icv_eoir_write(), the ICV_AP1R_EL1.NMI bit
>> should be set or clear according to the Non-maskable property. And the RPR
>> priority should also update the NMI bit according to the APR priority NMI bit.
>>
>> By the way, add gicv3_icv_nmiar1_read trace event.
>>
>> If the hpp irq is a NMI, the icv iar read should return 1022 and trap for
>> NMI again
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> v11:
>> - Deal with NMI in the callers instead of ich_highest_active_virt_prio().
>> - Set either NMI or a group-priority bit, not both.
>> - Only set AP NMI bits in the 0 reg.
>> - Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
>> v10:
>> - Rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
>> - Add ICV_RPR_EL1_NMI definition.
>> - Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
>>   ich_highest_active_virt_prio().
>> v9:
>> - Correct the INTID_NMI logic.
>> v8:
>> - Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
>> v7:
>> - Add Reviewed-by.
>> v6:
>> - Implement icv_nmiar1_read().
>> ---
>>  hw/intc/arm_gicv3_cpuif.c | 97 ++++++++++++++++++++++++++++++++++-----
>>  hw/intc/gicv3_internal.h  |  4 ++
>>  hw/intc/trace-events      |  1 +
>>  3 files changed, 91 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
>> index f99f2570a6..a7bc44b30c 100644
>> --- a/hw/intc/arm_gicv3_cpuif.c
>> +++ b/hw/intc/arm_gicv3_cpuif.c
>> @@ -157,6 +157,10 @@ static int ich_highest_active_virt_prio(GICv3CPUState *cs)
>>      int i;
>>      int aprmax = ich_num_aprs(cs);
>>
>> +    if (cs->gic->nmi_support && cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI) {
>> +        return 0x80;
> 
> This should be 0, not 0x80 -- see the register description for
> ICH_LR<n>_EL2 Priority field: when NMI is 1, the virtual interrupt's
> priority is treated as 0x0.
> 
>> +    }
>> +
>>      for (i = 0; i < aprmax; i++) {
>>          uint32_t apr = cs->ich_apr[GICV3_G0][i] |
>>              cs->ich_apr[GICV3_G1NS][i];
>> @@ -191,6 +195,7 @@ static int hppvi_index(GICv3CPUState *cs)
>>       * correct behaviour.
>>       */
>>      int prio = 0xff;
>> +    bool nmi = false;
>>
>>      if (!(cs->ich_vmcr_el2 & (ICH_VMCR_EL2_VENG0 | ICH_VMCR_EL2_VENG1))) {
>>          /* Both groups disabled, definitely nothing to do */
>> @@ -200,6 +205,11 @@ static int hppvi_index(GICv3CPUState *cs)
>>      for (i = 0; i < cs->num_list_regs; i++) {
>>          uint64_t lr = cs->ich_lr_el2[i];
>>          int thisprio;
>> +        bool thisnmi = false;
>> +
>> +        if (cs->gic->nmi_support) {
>> +            thisnmi = lr & ICH_LR_EL2_NMI;
>> +        }
> 
> You could write this a little more concisely as
>       bool thisnmi = cs->gic_nmi_support && (lr & ICH_LR_EL2_NMI);
> 
>>          if (ich_lr_state(lr) != ICH_LR_EL2_STATE_PENDING) {
>>              /* Not Pending */
>> @@ -219,9 +229,13 @@ static int hppvi_index(GICv3CPUState *cs)
>>
>>          thisprio = ich_lr_prio(lr);
>>
>> -        if (thisprio < prio) {
>> +        if ((thisprio < prio) || ((thisprio == prio) && (thisnmi & (!nmi)))) {
>>              prio = thisprio;
>>              idx = i;
>> +
>> +            if (cs->gic->nmi_support) {
>> +                nmi = thisnmi;
>> +            }
> 
> You don't need to check nmi_support here because if nmi_support
> is false then thisnmi will also be false, and so we will never
> set nmi to anything other than false.
> 
>>          }
>>      }
>>
>> @@ -326,6 +340,12 @@ static bool icv_hppi_can_preempt(GICv3CPUState *cs, uint64_t lr)
>>          return true;
>>      }
>>
>> +    if ((prio & mask) == (rprio & mask) &&
>> +        cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI) &&
>> +        (!(cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI))) {
>> +        return true;
>> +    }
>> +
>>      return false;
>>  }
> 
> This isn't the only change needed to icv_hppi_can_preempt():
> virtual NMIs are never masked by the MPR, so that check also
> must be changed. If we pull out a variable:
> 
>     bool is_nmi = cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI);
> 
> we can use that both to gate the vpmr check:
> 
>     if (!is_nmi && prio >= vpmr) {
> 
> and also in the priority check you have here.
> 
>> @@ -736,13 +765,19 @@ static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp)
>>       */
>>      uint32_t mask = icv_gprio_mask(cs, grp);
>>      int prio = ich_lr_prio(cs->ich_lr_el2[idx]) & mask;
>> +    bool nmi = cs->ich_lr_el2[idx] & ICH_LR_EL2_NMI;
>>      int aprbit = prio >> (8 - cs->vprebits);
>>      int regno = aprbit / 32;
>>      int regbit = aprbit % 32;
>>
>>      cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>>      cs->ich_lr_el2[idx] |= ICH_LR_EL2_STATE_ACTIVE_BIT;
>> -    cs->ich_apr[grp][regno] |= (1 << regbit);
>> +
>> +    if (cs->gic->nmi_support && nmi) {
>> +        cs->ich_apr[grp][regno] |= ICV_AP1R_EL1_NMI;
>> +    } else {
>> +        cs->ich_apr[grp][regno] |= (1 << regbit);
>> +    }
>>  }
>>
>>  static void icv_activate_vlpi(GICv3CPUState *cs)
>> @@ -776,7 +811,11 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>          if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
>>              intid = ich_lr_vintid(lr);
>>              if (!gicv3_intid_is_special(intid)) {
>> -                icv_activate_irq(cs, idx, grp);
>> +                if (!(lr & ICH_LR_EL2_NMI)) {
> 
> This is missing checks on both whether the GIC has NMI support and
> on whether the SCTLR NMI bit is set (compare pseudocode
> VirtualReadIAR1()). I suggest defining a
> 
>         bool nmi = cs->gic->nmi_support &&
>             (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_NMI) &&
>             (lr & ICH_LR_EL2_NMI);

The nmi_support check is redundant, as if FEAT_GICv3_NMI is unsupported,
the ICH_LR_EL2.NMI is RES0, so if ICH_LR_EL2.NMI is 1, FEAT_GICv3_NMI
has been surely realized.

> 
> and then you can check "if (!nmi)" here.
> 
>> +                    icv_activate_irq(cs, idx, grp);
>> +                } else {
>> +                    intid = INTID_NMI;
>> +                }
>>              } else {
>>                  /* Interrupt goes from Pending to Invalid */
>>                  cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>> @@ -797,8 +836,32 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>
>>  static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>  {
>> -    /* todo */
>> +    GICv3CPUState *cs = icc_cs_from_env(env);
>> +    int idx = hppvi_index(cs);
>>      uint64_t intid = INTID_SPURIOUS;
>> +
>> +    if (idx >= 0 && idx != HPPVI_INDEX_VLPI) {
>> +        uint64_t lr = cs->ich_lr_el2[idx];
>> +        int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
>> +
>> +        if ((thisgrp == GICV3_G1NS) && (lr & ICH_LR_EL2_NMI)) {
>> +            intid = ich_lr_vintid(lr);
>> +            if (!gicv3_intid_is_special(intid)) {
>> +                icv_activate_irq(cs, idx, GICV3_G1NS);
>> +            } else {
>> +                /* Interrupt goes from Pending to Invalid */
>> +                cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
>> +                /* We will now return the (bogus) ID from the list register,
>> +                 * as per the pseudocode.
>> +                 */
> 
> QEMU's coding style wants the /* on a line of its own for a
> multiline comment.
> 
>> +            }
>> +        }
>> +    }
>> +
>> +    trace_gicv3_icv_nmiar1_read(gicv3_redist_affid(cs), intid);
>> +
>> +    gicv3_cpuif_virt_update(cs);
>> +
>>      return intid;
>>  }
> 
>> @@ -1504,6 +1573,7 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      int irq = value & 0xffffff;
>>      int grp = ri->crm == 8 ? GICV3_G0 : GICV3_G1NS;
>>      int idx, dropprio;
>> +    bool nmi = false;
>>
>>      trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1,
>>                                 gicv3_redist_affid(cs), value);
>> @@ -1516,8 +1586,8 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>       * error checks" (because that lets us avoid scanning the AP
>>       * registers twice).
>>       */
>> -    dropprio = icv_drop_prio(cs);
>> -    if (dropprio == 0xff) {
>> +    dropprio = icv_drop_prio(cs, &nmi);
>> +    if (dropprio == 0xff && !nmi) {
>>          /* No active interrupt. It is CONSTRAINED UNPREDICTABLE
>>           * whether the list registers are checked in this
>>           * situation; we choose not to.
>> @@ -1539,8 +1609,9 @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>          uint64_t lr = cs->ich_lr_el2[idx];
>>          int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
>>          int lr_gprio = ich_lr_prio(lr) & icv_gprio_mask(cs, grp);
>> +        int thisnmi = lr & ICH_LR_EL2_NMI;
> 
> This variable should be "bool". An "int" is 32 bits, so because
> ICH_LR_EL2_NMI is bit 59, the value of "lr & ICH_LR_EL2_NMI" when
> cast to int is always zero. Using bool avoids this bug and also
> is consistent with the type we used for the 'nmi' variable we're
> about to compare it with.
> 
>> -        if (thisgrp == grp && lr_gprio == dropprio) {
>> +        if (thisgrp == grp && (lr_gprio == dropprio || thisnmi == nmi)) {
>>              if (!icv_eoi_split(env, cs) || irq >= GICV3_LPI_INTID_START) {
>>                  /*
>>                   * Priority drop and deactivate not split: deactivate irq now.
>> @@ -2626,7 +2697,11 @@ static void ich_ap_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>
>>      trace_gicv3_ich_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs), value);
>>
>> -    cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
>> +    if (cs->gic->nmi_support) {
>> +        cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
>> +    } else {
>> +        cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
>> +    }
>>      gicv3_cpuif_virt_irq_fiq_update(cs);
>>  }
> 
> thanks
> -- PMM
Peter Maydell April 3, 2024, 11:49 a.m. UTC | #4
On Wed, 3 Apr 2024 at 04:16, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> On 2024/4/3 0:12, Peter Maydell wrote:
> >> @@ -776,7 +811,11 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> >>          if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
> >>              intid = ich_lr_vintid(lr);
> >>              if (!gicv3_intid_is_special(intid)) {
> >> -                icv_activate_irq(cs, idx, grp);
> >> +                if (!(lr & ICH_LR_EL2_NMI)) {
> >
> > This is missing checks on both whether the GIC has NMI support and
> > on whether the SCTLR NMI bit is set (compare pseudocode
> > VirtualReadIAR1()). I suggest defining a
> >
> >         bool nmi = cs->gic->nmi_support &&
> >             (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_NMI) &&
> >             (lr & ICH_LR_EL2_NMI);
>
> The nmi_support check is redundant, as if FEAT_GICv3_NMI is unsupported,
> the ICH_LR_EL2.NMI is RES0, so if ICH_LR_EL2.NMI is 1, FEAT_GICv3_NMI
> has been surely realized.

As far as I can see you haven't changed ich_lr_write() to enforce
that, though, so the guest can write 1 to the NMI bit even if the
GIC doesn't support FEAT_GICv3_NMI. If you want to skip checking
nmi_support here you need to enforce that the NMI bit in the LR
is 0 in ich_lr_write().

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index f99f2570a6..a7bc44b30c 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -157,6 +157,10 @@  static int ich_highest_active_virt_prio(GICv3CPUState *cs)
     int i;
     int aprmax = ich_num_aprs(cs);
 
+    if (cs->gic->nmi_support && cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI) {
+        return 0x80;
+    }
+
     for (i = 0; i < aprmax; i++) {
         uint32_t apr = cs->ich_apr[GICV3_G0][i] |
             cs->ich_apr[GICV3_G1NS][i];
@@ -191,6 +195,7 @@  static int hppvi_index(GICv3CPUState *cs)
      * correct behaviour.
      */
     int prio = 0xff;
+    bool nmi = false;
 
     if (!(cs->ich_vmcr_el2 & (ICH_VMCR_EL2_VENG0 | ICH_VMCR_EL2_VENG1))) {
         /* Both groups disabled, definitely nothing to do */
@@ -200,6 +205,11 @@  static int hppvi_index(GICv3CPUState *cs)
     for (i = 0; i < cs->num_list_regs; i++) {
         uint64_t lr = cs->ich_lr_el2[i];
         int thisprio;
+        bool thisnmi = false;
+
+        if (cs->gic->nmi_support) {
+            thisnmi = lr & ICH_LR_EL2_NMI;
+        }
 
         if (ich_lr_state(lr) != ICH_LR_EL2_STATE_PENDING) {
             /* Not Pending */
@@ -219,9 +229,13 @@  static int hppvi_index(GICv3CPUState *cs)
 
         thisprio = ich_lr_prio(lr);
 
-        if (thisprio < prio) {
+        if ((thisprio < prio) || ((thisprio == prio) && (thisnmi & (!nmi)))) {
             prio = thisprio;
             idx = i;
+
+            if (cs->gic->nmi_support) {
+                nmi = thisnmi;
+            }
         }
     }
 
@@ -326,6 +340,12 @@  static bool icv_hppi_can_preempt(GICv3CPUState *cs, uint64_t lr)
         return true;
     }
 
+    if ((prio & mask) == (rprio & mask) &&
+        cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI) &&
+        (!(cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI))) {
+        return true;
+    }
+
     return false;
 }
 
@@ -550,7 +570,11 @@  static void icv_ap_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
     trace_gicv3_icv_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs), value);
 
-    cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
+    if (cs->gic->nmi_support) {
+        cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
+    } else {
+        cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
+    }
 
     gicv3_cpuif_virt_irq_fiq_update(cs);
     return;
@@ -697,7 +721,12 @@  static void icv_ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static uint64_t icv_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     GICv3CPUState *cs = icc_cs_from_env(env);
-    int prio = ich_highest_active_virt_prio(cs);
+    uint64_t prio = ich_highest_active_virt_prio(cs);
+
+    if (cs->gic->nmi_support &&
+        cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI) {
+        prio |= ICV_RPR_EL1_NMI;
+    }
 
     trace_gicv3_icv_rpr_read(gicv3_redist_affid(cs), prio);
     return prio;
@@ -736,13 +765,19 @@  static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp)
      */
     uint32_t mask = icv_gprio_mask(cs, grp);
     int prio = ich_lr_prio(cs->ich_lr_el2[idx]) & mask;
+    bool nmi = cs->ich_lr_el2[idx] & ICH_LR_EL2_NMI;
     int aprbit = prio >> (8 - cs->vprebits);
     int regno = aprbit / 32;
     int regbit = aprbit % 32;
 
     cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
     cs->ich_lr_el2[idx] |= ICH_LR_EL2_STATE_ACTIVE_BIT;
-    cs->ich_apr[grp][regno] |= (1 << regbit);
+
+    if (cs->gic->nmi_support && nmi) {
+        cs->ich_apr[grp][regno] |= ICV_AP1R_EL1_NMI;
+    } else {
+        cs->ich_apr[grp][regno] |= (1 << regbit);
+    }
 }
 
 static void icv_activate_vlpi(GICv3CPUState *cs)
@@ -776,7 +811,11 @@  static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
         if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
             intid = ich_lr_vintid(lr);
             if (!gicv3_intid_is_special(intid)) {
-                icv_activate_irq(cs, idx, grp);
+                if (!(lr & ICH_LR_EL2_NMI)) {
+                    icv_activate_irq(cs, idx, grp);
+                } else {
+                    intid = INTID_NMI;
+                }
             } else {
                 /* Interrupt goes from Pending to Invalid */
                 cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
@@ -797,8 +836,32 @@  static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri)
 
 static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    /* todo */
+    GICv3CPUState *cs = icc_cs_from_env(env);
+    int idx = hppvi_index(cs);
     uint64_t intid = INTID_SPURIOUS;
+
+    if (idx >= 0 && idx != HPPVI_INDEX_VLPI) {
+        uint64_t lr = cs->ich_lr_el2[idx];
+        int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
+
+        if ((thisgrp == GICV3_G1NS) && (lr & ICH_LR_EL2_NMI)) {
+            intid = ich_lr_vintid(lr);
+            if (!gicv3_intid_is_special(intid)) {
+                icv_activate_irq(cs, idx, GICV3_G1NS);
+            } else {
+                /* Interrupt goes from Pending to Invalid */
+                cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
+                /* We will now return the (bogus) ID from the list register,
+                 * as per the pseudocode.
+                 */
+            }
+        }
+    }
+
+    trace_gicv3_icv_nmiar1_read(gicv3_redist_affid(cs), intid);
+
+    gicv3_cpuif_virt_update(cs);
+
     return intid;
 }
 
@@ -1423,7 +1486,7 @@  static void icv_increment_eoicount(GICv3CPUState *cs)
                                 ICH_HCR_EL2_EOICOUNT_LENGTH, eoicount + 1);
 }
 
-static int icv_drop_prio(GICv3CPUState *cs)
+static int icv_drop_prio(GICv3CPUState *cs, bool *nmi)
 {
     /* Drop the priority of the currently active virtual interrupt
      * (favouring group 0 if there is a set active bit at
@@ -1445,6 +1508,12 @@  static int icv_drop_prio(GICv3CPUState *cs)
             continue;
         }
 
+        if (i == 0 && cs->gic->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
+            *papr1 &= (~ICV_AP1R_EL1_NMI);
+            *nmi = true;
+            return 0xff;
+        }
+
         /* We can't just use the bit-twiddling hack icc_drop_prio() does
          * because we need to return the bit number we cleared so
          * it can be compared against the list register's priority field.
@@ -1504,6 +1573,7 @@  static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
     int irq = value & 0xffffff;
     int grp = ri->crm == 8 ? GICV3_G0 : GICV3_G1NS;
     int idx, dropprio;
+    bool nmi = false;
 
     trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1,
                                gicv3_redist_affid(cs), value);
@@ -1516,8 +1586,8 @@  static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
      * error checks" (because that lets us avoid scanning the AP
      * registers twice).
      */
-    dropprio = icv_drop_prio(cs);
-    if (dropprio == 0xff) {
+    dropprio = icv_drop_prio(cs, &nmi);
+    if (dropprio == 0xff && !nmi) {
         /* No active interrupt. It is CONSTRAINED UNPREDICTABLE
          * whether the list registers are checked in this
          * situation; we choose not to.
@@ -1539,8 +1609,9 @@  static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
         uint64_t lr = cs->ich_lr_el2[idx];
         int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
         int lr_gprio = ich_lr_prio(lr) & icv_gprio_mask(cs, grp);
+        int thisnmi = lr & ICH_LR_EL2_NMI;
 
-        if (thisgrp == grp && lr_gprio == dropprio) {
+        if (thisgrp == grp && (lr_gprio == dropprio || thisnmi == nmi)) {
             if (!icv_eoi_split(env, cs) || irq >= GICV3_LPI_INTID_START) {
                 /*
                  * Priority drop and deactivate not split: deactivate irq now.
@@ -2626,7 +2697,11 @@  static void ich_ap_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
     trace_gicv3_ich_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs), value);
 
-    cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
+    if (cs->gic->nmi_support) {
+        cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
+    } else {
+        cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
+    }
     gicv3_cpuif_virt_irq_fiq_update(cs);
 }
 
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 81200eb90e..bc9f518fe8 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -246,6 +246,7 @@  FIELD(GICR_VPENDBASER, VALID, 63, 1)
 #define ICH_LR_EL2_PRIORITY_SHIFT 48
 #define ICH_LR_EL2_PRIORITY_LENGTH 8
 #define ICH_LR_EL2_PRIORITY_MASK (0xffULL << ICH_LR_EL2_PRIORITY_SHIFT)
+#define ICH_LR_EL2_NMI (1ULL << 59)
 #define ICH_LR_EL2_GROUP (1ULL << 60)
 #define ICH_LR_EL2_HW (1ULL << 61)
 #define ICH_LR_EL2_STATE_SHIFT 62
@@ -277,6 +278,9 @@  FIELD(GICR_VPENDBASER, VALID, 63, 1)
 #define ICH_VTR_EL2_PREBITS_SHIFT 26
 #define ICH_VTR_EL2_PRIBITS_SHIFT 29
 
+#define ICV_AP1R_EL1_NMI (1ULL << 63)
+#define ICV_RPR_EL1_NMI (1ULL << 63)
+
 /* ITS Registers */
 
 FIELD(GITS_BASER, SIZE, 0, 8)
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 94030550d5..47340b5bc1 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -152,6 +152,7 @@  gicv3_icv_rpr_read(uint32_t cpu, uint64_t val) "GICv3 ICV_RPR read cpu 0x%x valu
 gicv3_icv_hppir_read(int grp, uint32_t cpu, uint64_t val) "GICv3 ICV_HPPIR%d read cpu 0x%x value 0x%" PRIx64
 gicv3_icv_dir_write(uint32_t cpu, uint64_t val) "GICv3 ICV_DIR write cpu 0x%x value 0x%" PRIx64
 gicv3_icv_iar_read(int grp, uint32_t cpu, uint64_t val) "GICv3 ICV_IAR%d read cpu 0x%x value 0x%" PRIx64
+gicv3_icv_nmiar1_read(uint32_t cpu, uint64_t val) "GICv3 ICV_NMIAR1 read cpu 0x%x value 0x%" PRIx64
 gicv3_icv_eoir_write(int grp, uint32_t cpu, uint64_t val) "GICv3 ICV_EOIR%d write cpu 0x%x value 0x%" PRIx64
 gicv3_cpuif_virt_update(uint32_t cpuid, int idx, int hppvlpi, int grp, int prio) "GICv3 CPU i/f 0x%x virt HPPI update LR index %d HPPVLPI %d grp %d prio %d"
 gicv3_cpuif_virt_set_irqs(uint32_t cpuid, int fiqlevel, int irqlevel) "GICv3 CPU i/f 0x%x virt HPPI update: setting FIQ %d IRQ %d"