HI Julien, Thanks a lot for your reply, since I'm working on this patch in ARM (32 bits), so I have to dig into the details.
Julien Thierry <[email protected]> 於 2018年12月17日 週一 下午5:26寫道： > > 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. Ok, so we have to find a better place to check it. I have no idea so far... > > > 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. > Got it, thanks for the detail! > Thanks, > > -- > Julien Thierry
