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

From: Peter Maydell <peter.maydell@linaro.org>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: eduardo@habkost.net, marcel.apfelbaum@gmail.com,
	philmd@linaro.org,  wangyanan55@huawei.com,
	richard.henderson@linaro.org, qemu-devel@nongnu.org,
	 qemu-arm@nongnu.org
Subject: Re: [RFC PATCH v9 06/23] target/arm: Add support for Non-maskable Interrupt
Date: Fri, 22 Mar 2024 10:56:43 +0000	[thread overview]
Message-ID: <CAFEAcA8qSq0x_tu7zDfpghzZNSJtoHE4fZ7Hhu5nMJbUKviUtA@mail.gmail.com> (raw)
In-Reply-To: <1b4f0b6a-f1b1-8531-e117-fb85aedfa440@huawei.com>

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