On Fri, 22 Mar 2024 at 05:05, Jinjie Ruan <ruanjin...@huawei.com> wrote:
>
>
>
> On 2024/3/22 2:28, Peter Maydell wrote:
> > On Thu, 21 Mar 2024 at 15:46, Peter Maydell <peter.mayd...@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
