Re: [RFC PATCH v3 6/7] arm64: irqflags: Use ICC sysregs to implement IRQ masking

[Daniel Thompson]

On 22/08/16 16:06, Marc Zyngier wrote:
> > > > diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> > > > index fc2a0cb47b2c..a4ad69e2831b 100644
> > > > --- a/arch/arm64/include/asm/arch_gicv3.h
> > > > +++ b/arch/arm64/include/asm/arch_gicv3.h
> > > > @@ -80,6 +80,9 @@
> > > >  #include <linux/stringify.h>
> > > >  #include <asm/barrier.h>
> > > >
> > > > +/* Our default, arbitrary priority value. Linux only uses one anyway. */
> > > > +#define DEFAULT_PMR_VALUE	0xf0
> > > > +
> > > >  /*
> > > >   * Low-level accessors
> > > >   *
> > > > @@ -102,9 +105,35 @@ static inline void gic_write_dir(u32 irq)
> > > >  static inline u64 gic_read_iar_common(void)
> > > >  {
> > > >  	u64 irqstat;
> > > > +	u64 __maybe_unused daif;
> > > > +	u64 __maybe_unused pmr;
> > > > +	u64 __maybe_unused default_pmr_value = DEFAULT_PMR_VALUE;
> > > >
> > > > +#ifndef CONFIG_USE_ICC_SYSREGS_FOR_IRQFLAGS
> > > >  	asm volatile("mrs_s %0, " __stringify(ICC_IAR1_EL1) : "=r" (irqstat));
> > > >  	dsb(sy);
> > > > +#else
> > > > +	/*
> > > > +	 * The PMR may be configured to mask interrupts when this code is
> > > > +	 * called, thus in order to acknowledge interrupts we must set the
> > > > +	 * PMR to its default value before reading from the IAR.
> > > > +	 *
> > > > +	 * To do this without taking an interrupt we also ensure the I bit
> > > > +	 * is set whilst we are interfering with the value of the PMR.
> > > > +	 */
> > > > +	asm volatile(
> > > > +		"mrs	%1, daif\n\t"			     /* save I bit  */
> > > > +		"msr	daifset, #2\n\t"		     /* set I bit   */
> > > > +		"mrs_s  %2, " __stringify(ICC_PMR_EL1) "\n\t"/* save PMR    */
> > > > +		"msr_s	" __stringify(ICC_PMR_EL1) ",%3\n\t" /* set PMR     */
> > > > +		"mrs_s	%0, " __stringify(ICC_IAR1_EL1) "\n\t" /* ack int   */
> > > > +		"msr_s  " __stringify(ICC_PMR_EL1) ",%2\n\t" /* restore PMR */
> > > > +		"isb\n\t"
> > > > +		"msr	daif, %1"			     /* restore I   */
> > > > +		: "=r" (irqstat), "=&r" (daif), "=&r" (pmr)
> > > > +		: "r" (default_pmr_value));
> > >
> > > I'm a bit puzzled by this. Why would you have to mess with PMR or DAIF
> > > at this stage? Why can't we just let the GIC priority mechanism do its
> > > thing? I'd expect the initial setting of PMR (indicating whether or not
> > > we have normal interrupts enabled or not) to be enough to perform the
> > > filtering.
> > >
> > > You may get an NMI while you're already handling an interrupt, but
> > > that's fair game, and that's not something we should prevent.
> > >
> > > What am I missing?
> >
> > This code *always* executes with interrupts masked at the PMR and, when
> > interrupts are masked in this way, they cannot be acknowledged.
> >
> > So to acknowledge the interrupt we must first modify the PMR to unmask
> > it. If we did that without first ensuring the I-bit is masked we would
> > end up constantly re-trapping.
>
> Right, I see your problem now. I guess that I had a slightly different
> view of how to implement it. My own take is that we'd be better off
> leaving the I bit alone until we reach the point where we've accessed
> the IAR register (hence making the interrupt active and this particular
> priority unable to fire:
>
> 	irqstat = read_iar();
> 	clear_i_bit();
>
> At that stage, the only interrupt that can fire is an NMI (or nothing at
> all if we're already handling one). The I bit must also be set again on
> priority drop (before writing to the EOI register).

Relying on intack to raise the priority does sound a lot simpler.

The forceful mask of PMR inside kernel_entry was so we model, as closely 
as possible. the way the I-bit is set by hardware when we take a trap.

However the update of the PMR can easily be shifted into the enable_nmi 
macro leaving the interrupt controller (which cannot enable_nmi before 
calling the handler) to look after itself.


> This will save you most, if not all, of the faffing with PMR (which
> requires an ISB/DSB pair on each update to be guaranteed to have an
> effect, and is a good way to kill your performance).

Removing the code is a good thing although I thought the PMR was 
self-synchronizing (the code used to have barriers all over the shop 
until Dave Martin pointed that out).


> Of course, the drawback is that we still cover a large section of code
> using the I bit, but that'd be a good start.

To be honest I think that is forced on us anyway. We cannot know if the 
IRQ trap is a pseudo-NMI or an IRQ until we read the intack register.


> > Note, we could probably avoid saving the DAIF bits and PMR since they
> > both have a known state at this stage in gic_handle_irq(). However it
> > would be such a nasty potential side-effect I think I'd have to refactor
> > things a bit so the mask/unmask dance doesn't happen in the register
> > access functions.
>
> Yeah, there is clearly some redundancy here. And to be completely
> honest, I think you should try to split this series in two parts:
> - one that implements interrupt masking using PMR
> - one that takes care of NMIs and possibly backtracing
>
> At the moment, things are a bit mixed and it is hard to pinpoint what is
> really required for what.

Well... the patches are already teased out just as you say but the 
ordering *is* a bit weird now you mention it (patches 3, 4, 5 & 6 do the 
interrupt masking, whilst 1, 2 and 7 do the NMI).

Do you think its acceptable to reorder the patch set and put a clear 
description of the covering letter describing when the code related to 
NMI starts? I find it hard to see the point of PMR masking without 
example code to drive it.


Daniel.