Re: [PATCH v6 05/24] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler

[Julien Thierry]

On 29/11/18 18:12, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:56AM +0000, Julien Thierry wrote:
>> Mask the IRQ priority through PMR and re-enable IRQs at CPU level,
>> allowing only higher priority interrupts to be received during interrupt
>> handling.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@xxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
>> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>>  arch/arm/include/asm/arch_gicv3.h   | 17 +++++++++++++++++
>>  arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
>>  drivers/irqchip/irq-gic-v3.c        | 10 ++++++++++
>>  3 files changed, 44 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
>> index bef0b5d..f6f485f 100644
>> --- a/arch/arm/include/asm/arch_gicv3.h
>> +++ b/arch/arm/include/asm/arch_gicv3.h
>> @@ -363,5 +363,22 @@ static inline void gits_write_vpendbaser(u64 val, void * __iomem addr)
>>  
>>  #define gits_read_vpendbaser(c)		__gic_readq_nonatomic(c)
>>  
>> +static inline bool gic_prio_masking_enabled(void)
>> +{
>> +	return false;
>> +}
>> +
>> +static inline void gic_pmr_mask_irqs(void)
>> +{
>> +	/* Should not get called. */
>> +	WARN_ON_ONCE(true);
>> +}
>> +
>> +static inline void gic_arch_enable_irqs(void)
>> +{
>> +	/* Should not get called. */
>> +	WARN_ON_ONCE(true);
>> +}
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* !__ASM_ARCH_GICV3_H */
>> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
>> index 37193e2..3f8d5f4 100644
>> --- a/arch/arm64/include/asm/arch_gicv3.h
>> +++ b/arch/arm64/include/asm/arch_gicv3.h
>> @@ -155,5 +155,22 @@ static inline u32 gic_read_rpr(void)
>>  #define gits_write_vpendbaser(v, c)	writeq_relaxed(v, c)
>>  #define gits_read_vpendbaser(c)		readq_relaxed(c)
>>  
>> +static inline bool gic_prio_masking_enabled(void)
>> +{
>> +	return system_supports_irq_prio_masking();
>> +}
>> +
>> +static inline void gic_pmr_mask_irqs(void)
>> +{
>> +	/* Should not get called yet. */
>> +	WARN_ON_ONCE(true);
>> +}
>> +
>> +static inline void gic_arch_enable_irqs(void)
>> +{
>> +	/* Should not get called yet. */
>> +	WARN_ON_ONCE(true);
>> +}
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* __ASM_ARCH_GICV3_H */
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 8f87f40..e5d8c14 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -353,6 +353,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>>  	if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
>>  		int err;
>>  
>> +		if (gic_prio_masking_enabled()) {
>> +			gic_pmr_mask_irqs();
>> +			gic_arch_enable_irqs();
>> +		}
> 
> IIUC, if we have two pNMIs, this will allow one to preempt another, e.g.
> 
> 	< pNMI#1 asserted >
> 
> 	< CPU takes IRQ exception for pNMI #1>
> 
> 	irqnr = gic_read_iar(); // pNMI #1
> 
> 	< pNMI#2 asserted >
> 
> 	// masks IRQs at GIC, leaves other pNMIs unmasked
> 	gic_pmr_mask_irqs()
> 	gic_arch_enable_irqs();
> 
> 	...
> 
> 	< CPU takes IRQ exception for pNMI #2 >
> 
> ... or is that not a problem? Is the NMI code re-entrant?

At this patch stage, the GICv3 handling code is not supporting
pseudo-NMIs yet, only using interrupt priorities.

I introduce this in patch 18: irqchip/gic-v3: Handle pseudo-NMIs.

And yes, this would be an issue but only after gic_write_eoir. Once an
interrupt is running, its priority becomes the running priority and only
an interrupt with higher priority can preempt it. When we do the eoir,
the running priority is dropped meaning any interrupt can preempt (if
PSR.I is cleared and its priority is not masked by PMR).

What is done in the later patch for pseudo-NMIs is that we don't switch
to PMR masking and leave the I bit set since we don't want anything to
preempt it.

> 
>> +
>>  		if (static_branch_likely(&supports_deactivate_key))
>>  			gic_write_eoir(irqnr);
>>  		else
>> @@ -371,6 +376,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>>  		return;
>>  	}
>>  	if (irqnr < 16) {
>> +		if (gic_prio_masking_enabled()) {
>> +			gic_pmr_mask_irqs();
>> +			gic_arch_enable_irqs();
>> +		}
> 
> Can we pull this above the two cases, or is there a problem with doing
> this for spurious IRQs?
> 

So the reason I split this is to avoid doing it for NMIs. Otherwise we
would mask PMR, clear PSR.I and once we know we have an NMI set PSR.I again.

An alternative to it would be to check whether we have an NMI before we
know the type of interrupt we have, which would be unnecessary for IPIs
and of course spurious interrupts.

Maybe I can always mask PMR and clear PSR.I (after the interrupt ack) at
this stage and then do the necessary modifications once pseudo-NMI
handling code is added?

> Where is the corresponding unmask of the PMR, and disable of IRQs? It's
> difficult to follow the logic if that's in another patch.
> 
In patch 7: arm64: Make PMR part of task context

The arch/arm64 code becomes responsible for saving/restoring the value
of PMR upon exception entry/return.

I'm not really sure whether it makes sense to merge both patches though.
Would explaining that PMR will get restored on irq return in the commit
message be sufficient?

Thanks,

-- 
Julien Thierry