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

From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Sami Mujawar <Sami.Mujawar@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH v2 12/14] arm64/nmi: Add handling of superpriority interrupts as NMIs
Date: Wed, 07 Dec 2022 18:57:32 +0000	[thread overview]
Message-ID: <87zgbzrqhv.wl-maz@kernel.org> (raw)
In-Reply-To: <Y5CUA+yZDn5wqHQF@sirena.org.uk>

On Wed, 07 Dec 2022 13:24:19 +0000,
Mark Brown <broonie@kernel.org> wrote:
> 
> 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.

The FIQ user doesn't even have such concept in the interrupt
controller, nor does it have the corresponding CPU feature. Let's keep
it minimal for now. As you said, bringing it back is pretty easy.

> 
> > > +			/*
> > > +			 * 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. 

"Hypothetically", CPUs that advertise CSV2 could subsequently be found
to actually require extra handling, and I really wouldn't take such a
bet.

The reasoning by which CPU designers follow the ARM feature dependency
rules doesn't hold any water either, and hasn't for years (ARM itself
has been backporting features into CPUs that have a much older base
architecture). You don't have to look very far to find implementations
that cherry-pick whatever they want. The sad reality is that nobody
gives a damn about this rule, and ultimately pick whatever they see
fit.

And given that this is only one static branch away, that the runtime
cost is likely to be a big fat zero for non-affected platforms, for an
event that is vanishingly rare anyway, I'd rather we stay consistent
in the whole interrupt path and keep the mitigation code in.

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel