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

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> This only implements the external delivery method via the GICv3.
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu-qom.h |  3 ++-
>   target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
>   target/arm/cpu.h     |  2 ++
>   target/arm/helper.c  |  1 +
>   4 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
> index 8e032691db..66d555a605 100644
> --- a/target/arm/cpu-qom.h
> +++ b/target/arm/cpu-qom.h
> @@ -36,11 +36,12 @@ 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 five 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
>   
>   /* 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 5e5978c302..055670343e 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -128,7 +128,7 @@ static bool arm_cpu_has_work(CPUState *cs)
>   
>       return (cpu->power_state != PSCI_OFF)
>           && cs->interrupt_request &
> -        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
> +        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI
>            | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
>            | CPU_INTERRUPT_EXITTB);
>   }

I think you should not include CPU_INTERRUPT_NMI when it cannot be delivered, e.g. 
FEAT_NMI not enabled.


> @@ -668,6 +668,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned 
> int excp_idx,
>       CPUARMState *env = cpu_env(cs);
>       bool pstate_unmasked;
>       bool unmasked = false;
> +    bool nmi_unmasked = false;
>   
>       /*
>        * Don't take exceptions if they target a lower EL.
> @@ -678,13 +679,29 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>           return false;
>       }
>   
> +    nmi_unmasked = (!(env->allint & PSTATE_ALLINT)) &
> +                   (!((env->cp15.sctlr_el[target_el] & SCTLR_SPINTMASK) &&
> +                   (env->pstate & PSTATE_SP) && cur_el == target_el));

I don't see SCTLR_ELx.NMI being tested anywhere, which is required to enable 
everything else.

>       case EXCP_FIQ:
> -        pstate_unmasked = !(env->daif & PSTATE_F);
> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +            pstate_unmasked = (!(env->daif & PSTATE_F)) & nmi_unmasked;
> +        } else {
> +            pstate_unmasked = !(env->daif & PSTATE_F);
> +        }
>           break;
>   
>       case EXCP_IRQ:
> -        pstate_unmasked = !(env->daif & PSTATE_I);
> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +            pstate_unmasked = (!(env->daif & PSTATE_I)) & nmi_unmasked;
> +        } else {
> +            pstate_unmasked = !(env->daif & PSTATE_I);
> +        }
>           break;

I don't see what this is doing.  While Superpriority is IMPLEMENTATION DEFINED, how are 
you defining it for QEMU?  Is there a definition from real hw which makes sense under 
emulation?


r~