Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

[Julien Thierry <julien.thierry@arm.com>]

Hi Jian-Lin,

Thanks for looking at this.

On 16/12/2018 14:47, Jian-Lin Chen wrote:
> From: Jian-Lin Chen <lecopzer.chen@mediatek.com>
> 
> 
> On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@arm.com> 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