Re: [PATCH v2 12/14] arm64/nmi: Add handling of superpriority interrupts as NMIs

Mark Brown
2022-12-07T13:24:24Z

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Wed, Dec 07, 2022 at 11:03:26AM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > +int set_handle_nmi_irq(void (*handle_irq)(struct pt_regs *));
> > +int set_handle_nmi_fiq(void (*handle_fiq)(struct pt_regs *));

> I'm not overly keen on adding hooks that are not used, and I can't
> really foresee a use case for a FIQ NMI at the moment (there is no
> plan to use Group-0 interrupts in VMs when the GIC is enabled, and the
> only interrupt controller we have that uses FIQ doesn't even have
> priorities, let alone NMIs).

Sure, I don't care either way - I wasn't sure if people would prefer
symmetry/completeness or minimal usage so took a guess. I did consider
that the FIQ user might decide to implement NMIs on the basis that
they're easier to use than priorities but it's five minutes work to add
the API back when needed if that does happen.

> > + /*
> > + * Any system with FEAT_NMI should not be
> > + * affected by Spectre v2 so we don't mitigate
> > + * here.
> > + */

> Why? I don't see a good reason not to mitigate it, specially when the
> mitigation is guarded by cpus_have_const_cap(ARM64_SPECTRE_V2). Maybe
> you can explain what the rationale is for this.

Any CPU new enough to have FEAT_NMI is architecturally required to also
have FEAT_CSV2 since that's mandatory since v8.5 and FEAT_NMI is a v8.8
feature. FEAT_CSV2 means the hardware doesn't need the mitigation, and
we check for it in spectre_v2_get_cpu_hw_mitigation_state(). I was
trying to thread the needle between doing it for a combination of
symmetry and defensive programming and people seeing that the test would
always be false and should therefore be removed, especially in a hot
path like this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]