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"); return flags; } -- Catalin