Messages in this thread | | | Subject | Re: [PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities | From | Marc Zyngier <> | Date | Tue, 7 May 2019 09:44:46 +0100 |
| |
On 29/04/2019 17:00, Julien Thierry wrote: > Using IRQ priority masking to enable/disable interrupts is a bit > sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I. > > Introduce some validity checks to both highlight the states in which > functions dealing with IRQ enabling/disabling can (not) be called, and > bark a warning when called in an unexpected state. > > Since these checks are done on hotpaths, introduce a build option to > choose whether to do the checking. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/Kconfig | 11 +++++++++++ > arch/arm64/include/asm/daifflags.h | 9 +++++++++ > arch/arm64/include/asm/irqflags.h | 14 ++++++++++++++ > 3 files changed, 34 insertions(+) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 7e34b9e..3fb38f3 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI > > If unsure, say N > > +if ARM64_PSEUDO_NMI > +config ARM64_DEBUG_PRIORITY_MASKING > + bool "Debug interrupt priority masking" > + help > + This adds runtime checks to functions enabling/disabling > + interrupts when using priority masking. The additional checks verify > + the validity of ICC_PMR_EL1 when calling concerned functions. > + > + If unsure, say N > +endif > + > config RELOCATABLE > bool > help > diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h > index a32ece9..9512968 100644 > --- a/arch/arm64/include/asm/daifflags.h > +++ b/arch/arm64/include/asm/daifflags.h > @@ -28,6 +28,11 @@ > /* mask/save/unmask/restore all exceptions, including interrupts. */ > static inline void local_daif_mask(void) > { > + WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && > + system_uses_irq_prio_masking() &&
Given that you repeat these conditions all over the place, how about a helper:
#define DEBUG_PRIORITY_MASKING_CHECK(x) \ (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && \ system_uses_irq_prio_masking() && (x))
or some variant thereof.
> + (read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF | > + GIC_PRIO_IGNORE_PMR))); > + > asm volatile( > "msr daifset, #0xf // local_daif_mask\n" > : > @@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags) > { > bool irq_disabled = flags & PSR_I_BIT; > > + WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && > + system_uses_irq_prio_masking() && > + !(read_sysreg(daif) & PSR_I_BIT)); > + > if (!irq_disabled) { > trace_hardirqs_on(); > > diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h > index 516cdfc..a40abc2 100644 > --- a/arch/arm64/include/asm/irqflags.h > +++ b/arch/arm64/include/asm/irqflags.h > @@ -40,6 +40,13 @@ > */ > static inline void arch_local_irq_enable(void) > { > + if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && > + system_uses_irq_prio_masking()) { > + u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1); > + > + WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); > + } > + > asm volatile(ALTERNATIVE( > "msr daifclr, #2 // arch_local_irq_enable\n" > "nop", > @@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void) > > static inline void arch_local_irq_disable(void) > { > + if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && > + system_uses_irq_prio_masking()) { > + u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1); > + > + WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); > + } > + > asm volatile(ALTERNATIVE( > "msr daifset, #2 // arch_local_irq_disable", > "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", > -- > 1.9.1 >
nit: There is also the question of using WARN_ON in a context that will be extremely noisy if we're in a condition where this fires. WARN_ON_ONCE, maybe? This is a debugging thing, so maybe we just don't care.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |