Re: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

[Julien Thierry]

On 18/01/2019 16:35, 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.thierry@xxxxxxx>
>> Suggested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
>> ---
>>  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/efi.h b/arch/arm64/include/asm/efi.h
>> index 7ed3208..134ff6e 100644
>> --- a/arch/arm64/include/asm/efi.h
>> +++ b/arch/arm64/include/asm/efi.h
>> @@ -44,6 +44,17 @@
>>  
>>  #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
>>  
>> +#define arch_efi_save_flags(state_flags)		\
>> +	do {						\
>> +		(state_flags) =	read_sysreg(daif);	\
>> +	} while (0)
>> +
>> +#define arch_efi_restore_flags(state_flags)		\
>> +	do {						\
>> +		write_sysreg(state_flags, daif);	\
>> +	} while (0)
>> +
>> +
> 
> Randomly commenting a few minor nits as I glance down my mailbox...
> 
> There's no need to protect single statements with do { } while(0).
> 
> Just protect an expression statement that could be misparsed with ( ).
> 
> ->
> 
> #define arch_efi_save_flags(state_flags) ((state_flags) = read_sysreg(daif))

For the efi_save_flags(), I wanted to avoid it getting used as an
expression.

Would casting the assignment expression to (void) be acceptable?

> #define arch_efi_restore_flags(state_flags) write_sysreg(state_flags, daif)

For this one, write_sysreg() is already a statement, so yes, I
definitely don't need a do { } while (0) here.

> 
> [...]
> 
>> 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 @@
>>  
>>  #ifdef __KERNEL__
>>  
>> +#include <asm/alternative.h>
>>  #include <asm/ptrace.h>
>> +#include <asm/sysreg.h>
>>  
>>  /*
>>   * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
>> @@ -36,47 +38,96 @@
> 
> [...]
> 
>>  /*
>> + * 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)				\
>> +		: "=&r" (flags)						\
>> +		: "r" (daif_bits), "r" (pmr)				\
>> +		: "memory");						\
>> +									\
>> +	flags;								\
>> +})
> 
> Nit: does this need to be a macro?
> 
> ({ ... }) is mildly gross and it's preferable to avoid it if the code
> works just as well without...
> 
> pmr would need to be passed as a pointer, with "r" (*pmr) in the asm,
> but I think it would compile down to precisely the same code.
> 

The only motivation for it to be a macro was to be able to #undef it
after its use.

But with Catalin's suggestion, looks like we can makes things simple and
avoid having a separate macro/function.

>> +
>> +/*
>>   * 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
>> +	asm volatile(ALTERNATIVE(
>> +			"nop",
>> +			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1),
>> +			ARM64_HAS_IRQ_PRIO_MASKING)
>> +		: "=&r" (pmr)
> 
> Why earlyclobber?
>>>  		:
>>  		: "memory");
> 
> [...]
> 
>> @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void)
> 
> [...]
> 
>>  static inline int arch_irqs_disabled_flags(unsigned long flags)
>>  {
>> -	return flags & PSR_I_BIT;
>> +	int res;
>> +
>> +	asm volatile(ALTERNATIVE(
>> +			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>> +			"nop",
>> +			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
>> +			"cset	%w0, ls",
>> +			ARM64_HAS_IRQ_PRIO_MASKING)
>> +		: "=&r" (res)
> 
> Why earlyclobber?  %0 is not written before the reading of any input
> argument so far as I can see, in either alternative.
> 

I didn't really understand what the earlyclobber semantic was, thanks
for explaining it.

Thanks,

-- 
Julien Thierry