lkml.org 
[lkml]   [2019]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking
From
Date
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

\
 
 \ /
  Last update: 2019-01-18 17:58    [W:0.154 / U:6.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site