Hi Catalin,
On 18/01/2019 16:09, Catalin Marinas wrote:
> Hi Julien,
>
> On Tue, Jan 08, 2019 at 02:07:30PM +0000, Julien Thierry wrote:
>> + * Having two ways to control interrupt status is a bit complicated. Some
>> + * locations like exception entries will have PSR.I bit set by the
>> architecture
>> + * while PMR is unmasked.
>> + * We need the irqflags to represent that interrupts are disabled in such
>> cases.
>> + *
>> + * For this, we lower the value read from PMR when the I bit is set so it is
>> + * considered as an irq masking priority. (With PMR, lower value means
>> masking
>> + * more interrupts).
>> + */
>> +#define _get_irqflags(daif_bits, pmr)
>> \
>> +({ \
>> + unsigned long flags; \
>> + \
>> + BUILD_BUG_ON(GIC_PRIO_IRQOFF < (GIC_PRIO_IRQON & ~PSR_I_BIT)); \
>> + asm volatile(ALTERNATIVE( \
>> + "mov %0, %1\n" \
>> + "nop\n" \
>> + "nop", \
>> + "and %0, %1, #" __stringify(PSR_I_BIT) "\n" \
>> + "mvn %0, %0\n" \
>> + "and %0, %0, %2", \
>> + ARM64_HAS_IRQ_PRIO_MASKING) \
>
> Can you write the last two instructions as a single:
>
> bic %0, %2, %0
Yes, makes sense. Although we won't need it anymore with your suggestion
below.
>
>> + : "=&r" (flags) \
>> + : "r" (daif_bits), "r" (pmr) \
>> + : "memory"); \
>> + \
>> + flags; \
>> +})
>> +
>> +/*
>> * Save the current interrupt enable state.
>> */
>> static inline unsigned long arch_local_save_flags(void)
>> {
>> - unsigned long flags;
>> - asm volatile(
>> - "mrs %0, daif // arch_local_save_flags"
>> - : "=r" (flags)
>> + unsigned long daif_bits;
>> + unsigned long pmr; // Only used if alternative is on
>> +
>> + daif_bits = read_sysreg(daif);
>> +
>> + // Get PMR
>
> Nitpick: don't use C++ (or arm asm) comment style in C code.
Noted.
>
>> + 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). 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;
}
(Looks like it removes one nop from the alternative as well, unless I
messed up something)
Does that seem better to you?
Thanks,
--
Julien Thierry
