Re: [RFC PATCH v3 06/21] target/arm: Add support for Non-maskable Interrupt

[Richard Henderson <richard.henderson@linaro.org>]

On 2/23/24 00:32, Jinjie Ruan via wrote:
> This only implements the external delivery method via the GICv3.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v3:
> - Not include CPU_INTERRUPT_NMI when FEAT_NMI not enabled
> - Add ARM_CPU_VNMI.
> - Refator nmi mask in arm_excp_unmasked().
> - Test SCTLR_ELx.NMI for ALLINT mask for NMI.
> ---
>   target/arm/cpu-qom.h |  4 +++-
>   target/arm/cpu.c     | 54 ++++++++++++++++++++++++++++++++++++--------
>   target/arm/cpu.h     |  4 ++++
>   target/arm/helper.c  |  2 ++
>   4 files changed, 54 insertions(+), 10 deletions(-)
> 
> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
> index 8e032691db..e0c9e18036 100644
> --- a/target/arm/cpu-qom.h
> +++ b/target/arm/cpu-qom.h
> @@ -36,11 +36,13 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>   #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
>   #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
>   
> -/* Meanings of the ARMCPU object's four inbound GPIO lines */
> +/* Meanings of the ARMCPU object's six inbound GPIO lines */
>   #define ARM_CPU_IRQ 0
>   #define ARM_CPU_FIQ 1
>   #define ARM_CPU_VIRQ 2
>   #define ARM_CPU_VFIQ 3
> +#define ARM_CPU_NMI 4
> +#define ARM_CPU_VNMI 5
>   
>   /* For M profile, some registers are banked secure vs non-secure;
>    * these are represented as a 2-element array where the first element
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5fa86bc8d5..d40ada9c75 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -126,11 +126,20 @@ static bool arm_cpu_has_work(CPUState *cs)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>   
> -    return (cpu->power_state != PSCI_OFF)
> -        && cs->interrupt_request &
> -        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
> -         | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
> -         | CPU_INTERRUPT_EXITTB);
> +    if (cpu_isar_feature(aa64_nmi, cpu)) {
> +        return (cpu->power_state != PSCI_OFF)
> +            && cs->interrupt_request &
> +            (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
> +             | CPU_INTERRUPT_NMI | CPU_INTERRUPT_VNMI
> +             | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
> +             | CPU_INTERRUPT_EXITTB);
> +    } else {
> +        return (cpu->power_state != PSCI_OFF)
> +            && cs->interrupt_request &
> +            (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
> +             | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
> +             | CPU_INTERRUPT_EXITTB);
> +    }

This can be factored better, to avoid repeating everything.

However, I am reconsidering my previous advice to ignore NMI if FEAT_NMI is not present.

Consider R_MHWBP, where IRQ with Superpriority, with SCTLR_ELx.NMI == 0, is masked 
identically with IRQ without Superpriority.  Moreover, if the GIC is configured so that 
FEAT_GICv3_NMI is only set if FEAT_NMI is set, then we won't ever see CPU_INTERRUPT_*NMI 
anyway.

So we might as well accept NMI here unconditionally.  But document this choice here with a 
comment.


> @@ -678,13 +688,26 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>           return false;
>       }
>   
> +    if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +        nmi_unmasked = (cur_el == target_el) &&
> +                       (((env->cp15.sctlr_el[target_el] & SCTLR_NMI) &&
> +                        (env->allint & PSTATE_ALLINT)) ||
> +                        ((env->cp15.sctlr_el[target_el] & SCTLR_SPINTMASK) &&
> +                        (env->pstate & PSTATE_SP)));

In the manual, this is "allintmask".  It is easier to follow the logic if you use this...

> +        nmi_unmasked = !nmi_unmasked;

... and not the inverse.

>       case EXCP_FIQ:
> -        pstate_unmasked = !(env->daif & PSTATE_F);
> +        pstate_unmasked = (!(env->daif & PSTATE_F)) & nmi_unmasked;

Clearer with "&&".

> +    if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +        if (interrupt_request & CPU_INTERRUPT_NMI) {
> +            excp_idx = EXCP_NMI;
> +            target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
> +            if (arm_excp_unmasked(cs, excp_idx, target_el,
> +                                  cur_el, secure, hcr_el2)) {
> +                goto found;
> +            }
> +        }
> +    }

Handling for vNMI?

> @@ -957,6 +992,7 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
>           break;
>       case ARM_CPU_IRQ:
>       case ARM_CPU_FIQ:
> +    case ARM_CPU_NMI:
>           if (level) {
>               cpu_interrupt(cs, mask[irq]);
>           } else {

Likewise.


r~