Messages in this thread | | | Subject | Re: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking | From | Julien Thierry <> | Date | Mon, 21 Jan 2019 08:45:21 +0000 |
| |
On 18/01/2019 17:33, Catalin Marinas wrote: > On Fri, Jan 18, 2019 at 05:30:02PM +0000, Catalin Marinas wrote: >> On Fri, Jan 18, 2019 at 04:57:32PM +0000, Julien Thierry wrote: >>> On 18/01/2019 16:09, Catalin Marinas wrote: >>>> On Tue, Jan 08, 2019 at 02:07:30PM +0000, Julien Thierry wrote: >>>>> + asm volatile(ALTERNATIVE( >>>>> + "nop", >>>>> + "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1), >>>>> + ARM64_HAS_IRQ_PRIO_MASKING) >>>>> + : "=&r" (pmr) >>>>> : >>>>> : "memory"); >>>>> + >>>>> + return _get_irqflags(daif_bits, pmr); >>>>> +} >>>> >>>> I find this confusing spread over two inline asm statements. IIUC, you >>>> want something like below (it could be written as inline asm but I need >>>> to understand it first): >>>> >>>> daif_bits = read_sysreg(daif); >>>> >>>> if (system_uses_irq_prio_masking()) { >>>> pmr = read_gicreg(ICC_PMR_EL1); >>>> flags = pmr & ~(daif_bits & PSR_I_BIT); >>>> } else { >>>> flags = daif_bits; >>>> } >>>> >>>> return flags; >>>> >>>> In the case where the interrupts are disabled at the PSR level, is the >>>> PMR value still relevant? Could we just return the GIC_PRIO_IRQOFF? >>>> Something like: >>>> >>>> flags = read_sysreg(daif); >>>> >>>> if (system_uses_irq_prio_masking()) >>>> flags = flags & PSR_I_BIT ? >>>> GIC_PRIO_IRQOFF : read_gicreg(ICC_PMR_EL1); >>>> >>> >>> You're right, returning GIC_PRIO_IRQOFF should be good enough (it is >>> actually what happens in this version because GIC_PRIO_IRQOFF == >>> GIC_PRIO_IRQON & ~PSR_I_BIT happens to be true). >> >> This wasn't entirely clear to me, I got confused by: >> >> + BUILD_BUG_ON(GIC_PRIO_IRQOFF < (GIC_PRIO_IRQON & ~PSR_I_BIT)); \ >> >> and I thought there isn't necessarily an equality between the two. >> >>> Your suggestion would >>> make things easier to reason about. Maybe something like: >>> >>> >>> static inline unsigned long arch_local_save_flags(void) >>> { >>> unsigned long daif_bits; >>> unsigned long prio_off = GIC_PRIO_IRQOFF; >>> >>> daif_bits = read_sysreg(daif); >>> >>> asm volatile(ALTERNATIVE( >>> "mov %0, %1\n" >>> "nop\n" >>> "nop", >>> "mrs %0, SYS_ICC_PMR_EL1\n" >>> "ands %1, %1, PSR_I_BIT\n" >>> "csel %0, %0, %2, eq") >>> : "=&r" (flags) >>> : "r" (daif_bits), "r" (prio_off) >>> : "memory"); >>> >>> return flags; >>> } >> >> It looks fine. If you turn the BUILD_BUG_ON into a !=, you could >> probably simplify the asm a bit (though the number of instructions >> generated would probably be the same). Untested: >> >> static inline unsigned long arch_local_save_flags(void) >> { >> unsigned long flags; >> >> flags = read_sysreg(daif); >> >> asm volatile(ALTERNATIVE( >> "nop", >> "bic %0, %1, %2") >> : "=&r" (flags) >> : "r" (flags & PSR_I_BIT), "r" (GIC_PRIO_IRQOFF) >> : "memory"); > > Ah, I missed a read from SYS_ICC_PMR_EL1 here. Anyway, the idea was that > you don't need to set prio_off to a variable, just pass "r" (constant) > here and the compiler does the trick. >
I see, thanks. I'll avoid that superfluous variable.
Thanks,
-- Julien Thierry
| |