On 08/01/2019 15:40, Dave Martin wrote:
> On Tue, Jan 08, 2019 at 02:07:30PM +0000, Julien Thierry wrote:
>> Instead disabling interrupts by setting the PSR.I bit, use a priority
>> higher than the one used for interrupts to mask them via PMR.
>>
>> When using PMR to disable interrupts, the value of PMR will be used
>> instead of PSR.[DAIF] for the irqflags.
>>
>> Signed-off-by: Julien Thierry <julien.thie...@arm.com>
>> Suggested-by: Daniel Thompson <daniel.thomp...@linaro.org>
>> Cc: Catalin Marinas <catalin.mari...@arm.com>
>> Cc: Will Deacon <will.dea...@arm.com>
>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> Cc: Oleg Nesterov <o...@redhat.com>
>> ---
>> arch/arm64/include/asm/efi.h | 11 ++++
>> arch/arm64/include/asm/irqflags.h | 123
>> +++++++++++++++++++++++++++++---------
>> 2 files changed, 106 insertions(+), 28 deletions(-)
>
> [...]
>
>> diff --git a/arch/arm64/include/asm/irqflags.h
>> b/arch/arm64/include/asm/irqflags.h
>> index 24692ed..fa3b06f 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -18,7 +18,9 @@
>
> [...]
>
>> static inline void arch_local_irq_enable(void)
>> {
>> - asm volatile(
>> - "msr daifclr, #2 // arch_local_irq_enable"
>> - :
>> + unsigned long unmasked = GIC_PRIO_IRQON;
>> +
>> + asm volatile(ALTERNATIVE(
>> + "msr daifclr, #2 // arch_local_irq_enable\n"
>> + "nop",
>> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n"
>> + "dsb sy",
>
> I'm still not convinced these dsbs are needed.
>
> Without the dsb, we are probably not guaranteed to take a pending
> interrupt _immediately_ on unmasking, but I'm not sure that's a
> problem.
>
> What goes wrong if we omit them?
Then the GIC doesn't know it can now deliver interrupts of a lower
priority. Only a dsb can guarantee that the GIC's view of PMR will get
updated.
See 9.1.6 (Observability of the effects of accesses to the GIC
registers), which states:
<quote>
Architectural execution of a DSB instruction guarantees that
— The last value written to ICC_PMR_EL1 or GICC_PMR is observed by the
associated Redistributor.
</quote>
So yes, DSB is required.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
