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

[Peter Maydell]

On Fri, 22 Mar 2024 at 03:56, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/3/21 23:46, Peter Maydell 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.
>
> A PE that implements FEAT_NMI and FEAT_GICv3 also implements
> FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not
> implement FEAT_GICv3_NMI.
>
> As the GIC side has checked the FEAT_GICv3_NMI is implemented or Not. So
> if cpu_isar_feature(aa64_nmi, env_archcpu(env)) is false, the
> FEAT_GICv3_NMI will also not implemented,the CPU_INTERRUPT_NMI/VINMI can
> not come from GIC, so we only need to check cpu_isar_feature(aa64_nmi,
> env_archcpu(env)) and SCTLR_ELx.NMI is set in hcr_write() and
> hcrx_write() for CPU side.

If the CPU implements FEAT_NMI then the guest is allowed to
write to HCRX_EL2.VINMI even if SCTLR_ELx.NMI is 0. (It
might for example choose to set up HCRX_EL2.VINMI first
and then set NMI = 1 afterwards.) So you can't fix this
by checking for NMI in hcr_write/hcrx_write.

If you wanted you could not set the VINMI etc bits in
arm_cpu_update_vinmi() and friends if SCTLR_ELx.NMI is 0,
as long as you ensure that the update functions all get
called again when the NMI bit is written and on exception
handling change.

-- PMM