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

[Peter Maydell]

On Fri, 22 Mar 2024 at 05:05, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/3/22 2:28, Peter Maydell wrote:
> > On Thu, 21 Mar 2024 at 15:46, Peter Maydell <peter.maydell@linaro.org> 
> > wrote:
> >> Something somewhere needs to implement "if SCTLR_ELx.NMI is 0 then
> >> we don't take EXCP_VINMI etc but instead (maybe) EXCP_VIRQ etc".
> >> At the moment nothing does that:
> >>  * arm_cpu_update_vinmi() doesn't look at the NMI bit before
> >>    deciding whether to set CPU_INTERRUPT_VINMI
> >>  * in arm_excp_unmasked() if NMI is 0 then allIntMask takes its
> >>    default value of false and so arm_excp_unmasked() returns true,
> >>    so VINMI is not masked
> >>  * arm_cpu_exec_interrupt() doesn't look at the NMI bit before
> >>    deciding whether to check the CPU_INTERRUPT_VINMI bit in 
> >> interrupt_request
> >>
> >> So even if SCTLR_ELx.NMI is 0 we'll still try to take a VINMI
> >> if it's set up in the HCR_EL2 bits.
> >>
> >> However we do this the required behaviour is that if NMI is 0
> >> then it is as if the interrupt doesn't have superpriority and
> >> it falls back to being handled as an ordinary IRQ, VIRQ, VFIQ etc.
> >> I think the best place to do this is probably here in
> >> arm_cpu_exec_interrupt() -- if SCTLR_ELx.NMI isn't set then
> >> treat the VFNMI bit like VFIQ, the VINMI bit like VIRQ, and
> >> the NMI bit like IRQ.
> >
> > Folding in something like this I think will work:
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 91c2896de0f..797ae3eb805 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -837,7 +837,8 @@ static bool arm_cpu_exec_interrupt(CPUState *cs,
> > int interrupt_request)
> >
> >      /* The prioritization of interrupts is IMPLEMENTATION DEFINED. */
> >
> > -    if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> > +    if (cpu_isar_feature(aa64_nmi, env_archcpu(env)) &&
> > +        (arm_sctlr(env, cur_el) & SCTLR_NMI)) {
> >          if (interrupt_request & CPU_INTERRUPT_NMI) {
> >              excp_idx = EXCP_NMI;
> >              target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, 
> > secure);
> > @@ -862,7 +863,22 @@ static bool arm_cpu_exec_interrupt(CPUState *cs,
> > int interrupt_request)
> >                  goto found;
> >              }
> >          }
> > +    } else {
> > +        /*
> > +         * NMI disabled: interrupts with superpriority are handled
> > +         * as if they didn't have it
> > +         */
> > +        if (interrupt_request & CPU_INTERRUPT_NMI) {
> > +            interrupt_request |= CPU_INTERRUPT_HARD;
>
> The CPU_INTERRUPT_NMI and CPU_INTERRUPT_HARD are set simultaneously,
> should the CPU_INTERRUPT_NMI be cleared?

interrupt_request here is a local variable, and we will never
check the NMI bit in it beyond this point in the function,
so there's no need to explicitly clear the NMI bit.

thanks
-- PMM