Re: [PATCH v11 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers

[Peter Maydell]

On Sat, 30 Mar 2024 at 10:34, 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>

I have a few more comments below but otherwise I think this
patch is looking OK.


> @@ -898,12 +922,24 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs)
>       */
>      int rprio;
>      uint32_t mask;
> +    ARMCPU *cpu = ARM_CPU(cs->cpu);
> +    CPUARMState *env = &cpu->env;
>
>      if (icc_no_enabled_hppi(cs)) {
>          return false;
>      }
>
> -    if (cs->hppi.prio >= cs->icc_pmr_el1) {
> +    if (cs->gic->nmi_support && cs->hppi.nmi) {

If cs->hppi.nmi is set then nmi_support must be true, as we won't
ever set hppi.nmi if the GIC doesn't have NMI support (because the
registers where the guest can set the NMI bit will not be writeable
and so the nmi bits will always be zero). Elsewhere in the code
we assume this (eg in icc_iar1_read() below), so I think we can
also assume it here.

> +        if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS) &&
> +            cs->hppi.grp == GICV3_G1NS) {
> +            if (cs->icc_pmr_el1 < 0x80) {
> +                return false;
> +            }
> +            if (arm_is_secure(env) && cs->icc_pmr_el1 == 0x80) {
> +                return false;
> +            }
> +        }
> +    } else if (cs->hppi.prio >= cs->icc_pmr_el1) {
>          /* Priority mask masks this interrupt */
>          return false;
>      }
> @@ -923,6 +959,18 @@ static bool icc_hppi_can_preempt(GICv3CPUState *cs)
>          return true;
>      }
>
> +    if (cs->gic->nmi_support && cs->hppi.nmi &&
> +        (cs->hppi.prio & mask) == (rprio & mask)) {
> +        if ((cs->hppi.grp == GICV3_G1NS) &&
> +            !(cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI)) {
> +            return true;
> +        }
> +        if ((cs->hppi.grp == GICV3_G1) &&
> +            !(cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI)) {
> +            return true;
> +        }

You can write this part a bit more concisely:

           if (!(cs->icc_apr[cs->hppi.grp][0] & ICC_AP1R_EL1_NMI)) {
               return true;
           }

rather than writing out the G1 and G1NS cases separately.

> +    }
> +
>      return false;
>  }

> @@ -1159,13 +1212,16 @@ static uint64_t icc_iar0_read(CPUARMState *env, const 
> ARMCPRegInfo *ri)
>  static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      GICv3CPUState *cs = icc_cs_from_env(env);
> +    int el = arm_current_el(env);
>      uint64_t intid;
>
>      if (icv_access(env, HCR_IMO)) {
>          return icv_iar_read(env, ri);
>      }
>
> -    if (!icc_hppi_can_preempt(cs)) {
> +    if (cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) {
> +        intid = INTID_NMI;
> +    } else if (!icc_hppi_can_preempt(cs)) {
>          intid = INTID_SPURIOUS;
>      } else {
>          intid = icc_hppir1_value(cs, env);

This is the wrong order -- the cases where icc_hppi_can_preempt()
returns false need to take priority over the case where we return
INTID_NMI. You can get that by structuring it similarly to the
pseudocode:

    if (!icc_hppi_can_preempt(cs)) {
        intid = INTID_SPURIOUS;
    } else {
        intid = icc_hppir1_value(cs, env);
    }

    if (!gicv3_intid_is_special(intid)) {
        if (cs->hppi.nmi && env->cp15.sctlr_el[el] & SCTLR_NMI) {
            intid = INTID_NMI;
        } else {
            icc_activate_irq(cs, intid);
        }
    }

> @@ -1803,6 +1901,22 @@ static uint64_t icc_rpr_read(CPUARMState *env, const 
> ARMCPRegInfo *ri)
>          }
>      }
>
> +    if (cs->gic->nmi_support) {
> +        /* NMI info is reported in the high bits of RPR */
> +        if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
> +            if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
> +                prio |= ICC_RPR_EL1_NSNMI;

This should be ICC_RPR_EL1_NMI. Compare the pseudocode for ICC_RPR_EL1:
in the EL3-nonsecure case we set pPriority<63>.

> +            }
> +        } else {
> +            if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
> +                prio |= ICC_RPR_EL1_NSNMI;
> +            }
> +            if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) {
> +                prio |= ICC_RPR_EL1_NMI;
> +            }
> +        }
> +    }
> +
>      trace_gicv3_icc_rpr_read(gicv3_redist_affid(cs), prio);
>      return prio;
>  }

thanks
-- PMM