Re: [PATCH v5 15/27] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking
From: Julien Thierry
Date: Fri Sep 21 2018 - 13:55:08 EST
On 21/09/18 18:39, Julien Thierry wrote:
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?
One sequence that would cause trouble this is:
- { PSR.I = 1, PMR = unmasked }
- flags = local_irq_save()
- local_daif_unmask()
- // do stuff
- local_irq_disable()
- // do stuff
- local_irq_restore(flags)
However, local_daif_unmask() is not called anywhere currently. So I am
tempted to remove it as the function become a bit of a casualty waiting
to happen once we start having PMR.
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