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 [off-list ref] 於 2018年12月17日 週一 下午5:26寫道：

Hi Jian-Lin,

Thanks for looking at this.

On 16/12/2018 14:47, Jian-Lin Chen wrote:

quoted

From: Jian-Lin Chen <redacted>


On Wed, 12 Dec 2018 at 17:48, Julien Thierry [off-list ref] wrote:

quoted

 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...

quoted

If I bit was masked and someone calls arch_local_irq_enable(), they still
couldn't recieve any interrupt.

quoted

+       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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel