linux-kernel - Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Dec 2018 09:26:02 +0000
From:   Julien Thierry <julien.thierry@....com>
To:     Jian-Lin Chen <lecopzer@...il.com>
Cc:     Jian-Lin Chen <lecopzer.chen@...iatek.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        daniel.thompson@...aro.org, joel@...lfernandes.org,
        marc.zyngier@....com, christoffer.dall@....com,
        james.morse@....com, catalin.marinas@....com, will.deacon@....com,
        mark.rutland@....com, Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR_EL1 for interrupt
 masking

Hi Jian-Lin,

Thanks for looking at this.

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