Re: [PATCH v2 10/14] arm64/nmi: Manage masking for superpriority interrupts along with DAIF

From: Mark Brown <broonie@kernel.org>
To: Marc Zyngier <maz@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 10/14] arm64/nmi: Manage masking for superpriority interrupts along with DAIF
Date: Mon, 5 Dec 2022 20:52:16 +0000	[thread overview]
Message-ID: <Y45aAOYXqYQB+oHb@sirena.org.uk> (raw)
In-Reply-To: <86o7shlmb2.wl-maz@kernel.org>

[-- Attachment #1.1: Type: text/plain, Size: 2848 bytes --]

On Mon, Dec 05, 2022 at 06:47:45PM +0000, Marc Zyngier wrote:
> On Sat, 12 Nov 2022 15:17:04 +0000,
> Mark Brown <broonie@kernel.org> wrote:

> > As we do for pseudo NMIs add code to our DAIF management which keeps
>                           ,
> > superpriority interrupts unmasked when we have asynchronous exceptions
> > enabled. Since superpriority interrupts are not masked through DAIF like
> > pseduo NMIs are we also need to modify the assembler macros for managing
>                  ,
> 
> > DAIF to ensure that the masking is done in the assembly code. At present
> > users of the assembly macros always mask pseudo NMIs.

> In patch #5, you say:

> "This is not integrated into the existing DAIF macros since we do not
> always wish to manage ALLINT along with DAIF and the use of DAIF in
> the naming of the existing macros might lead to surprises if ALLINT is
> also managed."

> It isn't integrated, and yet it is.

Ah, yes - the note on patch 5 is a bit bitrotted now.  I'll update that.

> > There is a difference to the actual handling between pseudo NMIs
> > and superpriority interrupts in the assembly save_and_disable_irq and
> > restore_irq macros, these cover both interrupts and FIQs using DAIF
> > without regard for the use of pseudo NMIs so also mask those but are not
> > updated here to mask superpriority interrupts. Given the names it is not
> > clear that the behaviour with pseudo NMIs is particularly intentional,

> Pseudo-NMIs are still compatible with the standard DAIF behaviour,
> where setting PSTATE.I is strictly equivalent to setting PSTATE.ALLINT
> when you have architected NMIs.

> So I don't really understand your concern here.

The existing code is fine, the thing here was that unlike the C code
there's no matching management of PMR here where we're adding management
of ALLINT which might raise alarm bells for the reader.  I'll reword a
bit.

> > @@ -131,6 +145,10 @@ static inline void local_daif_inherit(struct pt_regs *regs)
> >  	if (interrupts_enabled(regs))
> >  		trace_hardirqs_on();
> >  
> > +	/* If we can take asynchronous errors we can take NMIs */
> > +	if (system_uses_nmi() && !(flags & PSR_A_BIT))
> > +		_allint_clear();
> > +

> Same remark about the ordering. Also, we don't check for PSTATE.A in
> the pseudo-NMI case. Why is this any different?

For NMIs we're making it track PSTATE.A so I wrote things that way to
make it clear that this should be what the end result is.  I've already
got a change locally which makes this even more explicit by having both
the set and clear cases rather than just only the clear cases.  You're
right though that we should achieve the same effect by restoring what
was saved in regs->pstate which is the equivalent of what pseudo NMIs
are doing so I'll change to do that.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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