Re: [PATCH v5 15/27] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

From: Julien Thierry
Date: Fri Sep 21 2018 - 13:40:11 EST

Next message: Doug Anderson: "Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation"
Previous message: Stephen Boyd: "Re: [PATCH] dma-debug: Check for drivers mapping vmalloc addresses"
Next in thread: Julien Thierry: "Re: [PATCH v5 15/27] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 28/08/18 16:51, 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.

The value chosen for PMR to enable/disable interrupts encodes the status
of interrupts on a single bit. This information is stored in the irqflags
values used when saving/restoring IRQ status.

Tested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
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/assembler.h | 17 ++++++-
arch/arm64/include/asm/efi.h | 3 +-
arch/arm64/include/asm/irqflags.h | 97 ++++++++++++++++++++++++++++++--------
arch/arm64/include/asm/ptrace.h | 10 ++--
arch/arm64/kernel/entry.S | 2 +-
5 files changed, 102 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 0bcc98d..0b2dcfd 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -23,6 +23,7 @@
#ifndef __ASM_ASSEMBLER_H
#define __ASM_ASSEMBLER_H

+#include <asm/alternative.h>
#include <asm/asm-offsets.h>
#include <asm/cpufeature.h>
#include <asm/debug-monitors.h>
@@ -62,12 +63,24 @@
/*
* Enable and disable interrupts.
*/
- .macro disable_irq
+ .macro disable_irq, tmp
+ mov \tmp, #ICC_PMR_EL1_MASKED
+alternative_if_not ARM64_HAS_IRQ_PRIO_MASKING
msr daifset, #2
+alternative_else
+ msr_s SYS_ICC_PMR_EL1, \tmp
+alternative_endif
.endm

- .macro enable_irq
+ .macro enable_irq, tmp
+ mov \tmp, #ICC_PMR_EL1_UNMASKED
+alternative_if_not ARM64_HAS_IRQ_PRIO_MASKING
msr daifclr, #2
+ nop
+alternative_else
+ msr_s SYS_ICC_PMR_EL1, \tmp
+ dsb sy
+alternative_endif
.endm

.macro save_and_disable_irq, flags
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 7ed3208..3e06891 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -42,7 +42,8 @@

efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);

-#define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
+#define ARCH_EFI_IRQ_FLAGS_MASK \
+ (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | ARCH_FLAG_PMR_EN)

/* arch specific definitions used by the stub code */

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 24692ed..193cfd0 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -18,7 +18,27 @@

#ifdef __KERNEL__

+#include <asm/alternative.h>
+#include <asm/cpufeature.h>
#include <asm/ptrace.h>
+#include <asm/sysreg.h>
+
+
+/*
+ * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
+ * whether the normal interrupts are masked is kept along with the daif
+ * flags.
+ */
+#define ARCH_FLAG_PMR_EN 0x1
+
+#define MAKE_ARCH_FLAGS(daif, pmr) \
+ ((daif) | (((pmr) >> ICC_PMR_EL1_EN_SHIFT) & ARCH_FLAG_PMR_EN))
+
+#define ARCH_FLAGS_GET_PMR(flags) \
+ ((((flags) & ARCH_FLAG_PMR_EN) << ICC_PMR_EL1_EN_SHIFT) \
+ | ICC_PMR_EL1_MASKED)
+
+#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)

/*
* Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
@@ -38,31 +58,50 @@
*/
static inline unsigned long arch_local_irq_save(void)
{
- unsigned long flags;
- asm volatile(
+ unsigned long flags, masked = ICC_PMR_EL1_MASKED;
+ unsigned long pmr = 0;
+
+ asm volatile(ALTERNATIVE(
"mrs %0, daif // arch_local_irq_save\n"
- "msr daifset, #2"
- : "=r" (flags)
- :
+ "msr daifset, #2\n"
+ "mov %1, #" __stringify(ICC_PMR_EL1_UNMASKED),
+ /* --- */
+ "mrs %0, daif\n"
+ "mrs_s %1, " __stringify(SYS_ICC_PMR_EL1) "\n"
+ "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %2",
+ ARM64_HAS_IRQ_PRIO_MASKING)
+ : "=&r" (flags), "=&r" (pmr)
+ : "r" (masked)
: "memory");
- return flags;
+
+ return MAKE_ARCH_FLAGS(flags, pmr);
}

static inline void arch_local_irq_enable(void)
{
- asm volatile(
- "msr daifclr, #2 // arch_local_irq_enable"
- :
+ unsigned long unmasked = ICC_PMR_EL1_UNMASKED;
+
+ asm volatile(ALTERNATIVE(
+ "msr daifclr, #2 // arch_local_irq_enable\n"
+ "nop",
+ "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n"
+ "dsb sy",
+ ARM64_HAS_IRQ_PRIO_MASKING)
:
+ : "r" (unmasked)
: "memory");
}

static inline void arch_local_irq_disable(void)
{
- asm volatile(
- "msr daifset, #2 // arch_local_irq_disable"
- :
+ unsigned long masked = ICC_PMR_EL1_MASKED;
+
+ asm volatile(ALTERNATIVE(
+ "msr daifset, #2 // arch_local_irq_disable",
+ "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0",
+ ARM64_HAS_IRQ_PRIO_MASKING)
:
+ : "r" (masked)
: "memory");
}

@@ -72,12 +111,19 @@ static inline void arch_local_irq_disable(void)
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 pmr = 0;
+
+ asm volatile(ALTERNATIVE(
+ "mrs %0, daif // arch_local_save_flags\n"
+ "mov %1, #" __stringify(ICC_PMR_EL1_UNMASKED),
+ "mrs %0, daif\n"
+ "mrs_s %1, " __stringify(SYS_ICC_PMR_EL1),
+ ARM64_HAS_IRQ_PRIO_MASKING)
+ : "=r" (flags), "=r" (pmr)
:
: "memory");
- return flags;
+
+ return MAKE_ARCH_FLAGS(flags, pmr);
}

/*
@@ -85,16 +131,27 @@ static inline unsigned long arch_local_save_flags(void)
*/
static inline void arch_local_irq_restore(unsigned long flags)
{
- asm volatile(
- "msr daif, %0 // arch_local_irq_restore"
+ unsigned long pmr = ARCH_FLAGS_GET_PMR(flags);
+
+ flags = ARCH_FLAGS_GET_DAIF(flags);
+
+ asm volatile(ALTERNATIVE(
+ "msr daif, %0 // arch_local_irq_restore\n"
+ "nop\n"
+ "nop",
+ "msr daif, %0\n"
+ "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%1\n"
+ "dsb sy",

I've come to realize there is an issue with that sequence. If the CPU has { PSR.I = 1, PMR = unmasked }, attempting attempting to restore flags { PSR.I = 0, PMR = masked }, there will be that ever so small window between the two instructions where interrupts get re-enabled while both contexts (the current one and the one being restored) have interrupts disabled...

Does that ever happen? Yes, when coming from a kernel entry or coming back from a VHE guest and doing "local_daif_retore(DAIF_PROCCTX_NOIRQ)".

An obvious, always working solution would be to do:
msr daifset, #2
msr ICC_PMR_EL1, %1
msr daif, %0
dsb sy

This however has some heavy performance hit on hackbench (~4-5%).

So I'd suggest:
msr ICC_PMR_EL1, %1
msr daif, %0
dsb sy

So, this only reverses the issue to the case where we restore { PSR.I = 1, PMR = unmasked } while CPU has { PSR.I = 0, PMR = masked }?
Yes, *but* there is no reason this should happen:

- There is no pre-defined flags values that provide { PSR.I = 1, PMR = unmasked } (and there is no reason to be one once we start using priorities)

- If flags { PMR = unmasked, PSR.I = 1 } where obtained through local_irq_save() or local_irq_save_flags(), from that point one would need to mask PMR *and* explicitly modify PSR.I since local_enable_irq() no longer touches PSR.
The only code that has a reason to do this explicit change is PMR setup code at CPU startup and when coming from exception entry. And code doing such action has no reason to be between local_irq_save/restore() calls since that would simply undo its action...

So my take is that restoring the PMR register first is fine without masking PSR.I with the following comment:

/*
* Code switching from PSR.I interrupt disabling to PMR masking
* should not lie between consecutive calls to local_irq_save()
* and local_irq_restore() in the same context.
*/

Does this approach seem sound? (If my explanation was not to confusing)
Am I missing some corner case?
Any suggestions for better approach? Or better wording?

Thanks,

+ ARM64_HAS_IRQ_PRIO_MASKING)
:
- : "r" (flags)
+ : "r" (flags), "r" (pmr)
: "memory");
}

static inline int arch_irqs_disabled_flags(unsigned long flags)
{
- return flags & PSR_I_BIT;
+ return (ARCH_FLAGS_GET_DAIF(flags) & (PSR_I_BIT)) |
+ !(ARCH_FLAGS_GET_PMR(flags) & ICC_PMR_EL1_EN_BIT);
}
#endif
#endif
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 29ec217..67df46e 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -25,8 +25,11 @@
#define CurrentEL_EL1 (1 << 2)
#define CurrentEL_EL2 (2 << 2)

-/* PMR value use to unmask interrupts */
+/* PMR values used to mask/unmask interrupts */
#define ICC_PMR_EL1_UNMASKED 0xf0
+#define ICC_PMR_EL1_EN_SHIFT 6
+#define ICC_PMR_EL1_EN_BIT (1 << ICC_PMR_EL1_EN_SHIFT) // PMR IRQ enable
+#define ICC_PMR_EL1_MASKED (ICC_PMR_EL1_UNMASKED ^ ICC_PMR_EL1_EN_BIT)

/* AArch32-specific ptrace requests */
#define COMPAT_PTRACE_GETREGS 12
@@ -201,8 +204,9 @@ static inline void forget_syscall(struct pt_regs *regs)
#define processor_mode(regs) \
((regs)->pstate & PSR_MODE_MASK)

-#define interrupts_enabled(regs) \
- (!((regs)->pstate & PSR_I_BIT))
+#define interrupts_enabled(regs) \
+ ((!((regs)->pstate & PSR_I_BIT)) && \
+ ((regs)->pmr_save & ICC_PMR_EL1_EN_BIT))

#define fast_interrupts_enabled(regs) \
(!((regs)->pstate & PSR_F_BIT))
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 79b06af..91e1e3d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -912,7 +912,7 @@ work_pending:
* "slow" syscall return path.
*/
ret_to_user:
- disable_irq // disable interrupts
+ disable_irq x21 // disable interrupts
ldr x1, [tsk, #TSK_TI_FLAGS]
and x2, x1, #_TIF_WORK_MASK
cbnz x2, work_pending
--
1.9.1

--
Julien Thierry

Next message: Doug Anderson: "Re: [PATCH v2 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation"
Previous message: Stephen Boyd: "Re: [PATCH] dma-debug: Check for drivers mapping vmalloc addresses"
Next in thread: Julien Thierry: "Re: [PATCH v5 15/27] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]