Hi Jian-Lin, Thanks for looking at this.
On 16/12/2018 14:47, Jian-Lin Chen wrote: > From: Jian-Lin Chen <[email protected]> > > > On Wed, 12 Dec 2018 at 17:48, Julien Thierry <[email protected]> wrote: >> static inline void arch_local_irq_enable(void) >> { >> - asm volatile( >> - "msr daifclr, #2 // arch_local_irq_enable" >> - : >> + unsigned long unmasked = GIC_PRIO_IRQON; >> + > > Should we need a WARN_ON() to check if the daif_I bit is masked, or > explicitly unmasked I bit here? > While I would agree, adding the WARN_ON() will add some non-negligible overhead, especially if we need to read the daif flags to check it. Since these functions are called often in the whole system and using PMR already makes things a bit slower, I'd prefer to avoid checks in here. > If I bit was masked and someone calls arch_local_irq_enable(), they still > couldn't recieve any interrupt. > > >> + asm volatile(ALTERNATIVE( >> + "msr daifclr, #2 // arch_local_irq_enable\n" >> + "nop", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" >> + "dsb sy", >> + ARM64_HAS_IRQ_PRIO_MASKING) >> : >> + : "r" (unmasked) >> : "memory"); >> } >> >> static inline void arch_local_irq_disable(void) >> { >> - asm volatile( >> - "msr daifset, #2 // arch_local_irq_disable" >> - : >> + unsigned long masked = GIC_PRIO_IRQOFF; >> + >> + asm volatile(ALTERNATIVE( >> + "msr daifset, #2 // arch_local_irq_disable", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", > > May be a "dsb sy" here? So, we need a "dsb sy" when unmasking interrupts because this ensures the redistributor sees the latest PMR value and starts forwarding lower priority interrupts again. When we disable interrupts however, the GIC CPU interface guarantees that no interrupts of lower priority than the current value of PMR will be taken. So we don't really need the redistributor to immediately see the new value of PMR as the logic in the GIC CPU interface is good enough for our goal. Thanks, -- Julien Thierry
