[RFC PATCH v4 0/5] Simplify kernel-mode NEON

[RFC PATCH v4 0/5] Simplify kernel-mode NEON

[Dave Martin]

This series aims to simplify kernel-mode NEON.

The key changes are:

 * Kernel-mode NEON is no longer nestable.

 * Kernel-mode NEON is no longer permitted in hardirq or nmi context.

 * Users of kernel-mode NEON must now call may_use_simd(), to determine
   whether NEON may be used.  Since this function is no longer trivial
   and can return false, the caller must typically provide a fallback
   implementation of the optimised algoritm for this situation, or must
   otherwise defer the algorithm execution to another context where it
   can execute.

 * EFI runtime servies save/restore NEON to/from a dedicated percpu
   buffer if called in hardirq or nmi context.

This series is motivated by SVE, which adds cost and complexity to
management of kernel-mode NEON.  Since the most problematic features of
KMN don't bring a substantial benefit even without SVE, it makes sense
to simplify.

This series depends on Ard's v4.14 crypto rework [2].

AFAIK, that series makes all the required changes to the arm64 crypto
library code.


Changes since RFC v3:

 * Rebased onto Ard's new series [2].

 * Added EFI helpers so that EFI runtime service use still works from
   interrupt context.

Changes since RFC v2:

 * Re-enable softirqs between kernel_neon_begin() and kernel_mode_end()

   (This was the original intention, but I was overtaken by irrational
   paranoia when drafting the previous version of the patch.)

 * softirq disable/enable is removed from fpsimd_thread_switch() (which
   runs with hardirqs disabled in any case, thus local_bh_enable() is
   treated as an error by the core code).  The comment block is updated
   to clarify this situation.

   This also means that no cost is added to the context switch critical
   path after all.

 * Move the this_cpu_write(fpsimd_last_state, NULL) back outside the
   conditional in kernel_neon_begin() (i.e., where it was originally
   before this series).  RFC v2 accidentally moved this invalidation
   inside the conditional, which leads to state corruption if switching
   back to a user task previously running on the same CPU, after
   kernel_mode_neon() is used by a kernel thread.

 * Minor formatting and spurious #include tidyups.

Testing:

 * For now, I've hacked linux/kernel/time/timer.c:run_timer_softirq() to
   call kernel_mode_neon_begin() and erase the FPSIMD registers, and
   jacked CONFIG_HZ to 1000.  I also added a test syscall that userspace
   can hammer to trigger the nested kernel_neon_begin() scenario.

   This is not hugely realistic, but was sufficient to diagnose the
   corruption problem described above, when running on a model.


Original blurb:

The main motivation for these changes is that supporting kernel-mode
NEON alongside SVE is tricky with the current framework: the current
partial save/restore mechanisms would need additional porting to
save/restore the extended SVE vector bits, and this renders the cost
saving of partial save/restore rather doubtful -- even if not all vector
registers are saved, the save/restore cost will still grow with
increasing vector length.  We could get the mechanics of this to work in
principle, but it doesn't feel like the right thing to do.

If we withdraw kernel-mode NEON support for hardirq context, accept some
extra softirq latency and disable nesting, then we can simplify the code
by always saving the task context directly into task_struct and
deferring all restore to ret_to_user.  Previous discussions with Ard
Biesheuvel suggest that this is a feasible approach and reasonably
aligned with other architectures.

The main impact on client code is that callers must check that
kernel-mode NEON is usable in the current context and fall back to a
non-NEON when necessary.  Ard has already done some work on this. [1]

The interesting changes are all in patch 2: the first patch just adds a
header inclusion guard that I noted was missing.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git kernel-mode-neon

[2] [PATCH resend 00/18] crypto: ARM/arm64 roundup for v4.14
lists.infradead.org/pipermail/linux-arm-kernel/2017-July/520664.html

Ard Biesheuvel (1):
  arm64: neon: replace generic definition of may_use_simd()

Dave Martin (4):
  arm64: neon: Add missing header guard in <asm/neon.h>
  arm64: fpsimd: Consistently use __this_cpu_ ops where appropriate
  arm64: neon: Remove support for nested or hardirq kernel-mode NEON
  arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

 arch/arm64/include/asm/Kbuild         |   1 -
 arch/arm64/include/asm/efi.h          |   5 +-
 arch/arm64/include/asm/fpsimd.h       |  16 +---
 arch/arm64/include/asm/fpsimdmacros.h |  56 ------------
 arch/arm64/include/asm/neon.h         |   9 +-
 arch/arm64/include/asm/simd.h         |  54 +++++++++++
 arch/arm64/kernel/entry-fpsimd.S      |  24 -----
 arch/arm64/kernel/fpsimd.c            | 168 ++++++++++++++++++++++++++--------
 8 files changed, 196 insertions(+), 137 deletions(-)
 create mode 100644 arch/arm64/include/asm/simd.h

-- 
2.1.4

[RFC PATCH v4 1/5] arm64: neon: replace generic definition of may_use_simd()

[Dave Martin]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

In preparation of modifying the logic that decides whether kernel mode
NEON is allowable, which is required for SVE support, introduce an
implementation of may_use_simd() that reflects the current reality, i.e.,
that SIMD is allowed in any context.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/Kbuild |  1 -
 arch/arm64/include/asm/simd.h | 23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/simd.h

diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
index f81c7b6..2326e39 100644
--- a/arch/arm64/include/asm/Kbuild
+++ b/arch/arm64/include/asm/Kbuild
@@ -20,7 +20,6 @@ generic-y += rwsem.h
 generic-y += segment.h
 generic-y += serial.h
 generic-y += set_memory.h
-generic-y += simd.h
 generic-y += sizes.h
 generic-y += switch_to.h
 generic-y += trace_clock.h
diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h
new file mode 100644
index 0000000..96959b5
--- /dev/null
+++ b/arch/arm64/include/asm/simd.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2017 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#ifndef __ASM_SIMD_H
+#define __ASM_SIMD_H
+
+#include <linux/types.h>
+
+/*
+ * may_use_simd - whether it is allowable at this time to issue SIMD
+ *                instructions or access the SIMD register file
+ */
+static __must_check inline bool may_use_simd(void)
+{
+	return true;
+}
+
+#endif
-- 
2.1.4

[RFC PATCH v4 2/5] arm64: neon: Add missing header guard in <asm/neon.h>

[Dave Martin]

asm/neon.h doesn't have a header inclusion guard, but it should
have one for consistency with other headers.

This patch adds a suitable guard.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/neon.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/include/asm/neon.h b/arch/arm64/include/asm/neon.h
index ad4cdc9..5368bd0 100644
--- a/arch/arm64/include/asm/neon.h
+++ b/arch/arm64/include/asm/neon.h
@@ -8,6 +8,9 @@
  * published by the Free Software Foundation.
  */
 
+#ifndef __ASM_NEON_H
+#define __ASM_NEON_H
+
 #include <linux/types.h>
 #include <asm/fpsimd.h>
 
@@ -17,3 +20,5 @@
 
 void kernel_neon_begin_partial(u32 num_regs);
 void kernel_neon_end(void);
+
+#endif /* ! __ASM_NEON_H */
-- 
2.1.4

[RFC PATCH v4 3/5] arm64: fpsimd: Consistently use __this_cpu_ ops where appropriate

[Dave Martin]

__this_cpu_ ops are not used consistently with regard to this_cpu_
ops in a couple of places in fpsimd.c.

Since preemption is explicitly disabled in
fpsimd_restore_current_state() and fpsimd_update_current_state(),
this patch converts this_cpu_ ops in those functions to __this_cpu_
ops.  This doesn't save cost on arm64, but benefits from additional
assertions in the core code.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/fpsimd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 06da8ea..d7e5f8a 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -194,7 +194,7 @@ void fpsimd_restore_current_state(void)
 		struct fpsimd_state *st = &current->thread.fpsimd_state;
 
 		fpsimd_load_state(st);
-		this_cpu_write(fpsimd_last_state, st);
+		__this_cpu_write(fpsimd_last_state, st);
 		st->cpu = smp_processor_id();
 	}
 	preempt_enable();
@@ -214,7 +214,7 @@ void fpsimd_update_current_state(struct fpsimd_state *state)
 	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
 		struct fpsimd_state *st = &current->thread.fpsimd_state;
 
-		this_cpu_write(fpsimd_last_state, st);
+		__this_cpu_write(fpsimd_last_state, st);
 		st->cpu = smp_processor_id();
 	}
 	preempt_enable();
-- 
2.1.4

[RFC PATCH v4 4/5] arm64: neon: Remove support for nested or hardirq kernel-mode NEON

[Dave Martin]

Support for kernel-mode NEON to be nested and/or used in hardirq
context adds significant complexity, and the benefits may be
marginal.  In practice, kernel-mode NEON is not used in hardirq
context, and is rarely used in softirq context (by certain mac80211
drivers).

This patch implements an arm64 may_use_simd() function to allow
clients to check whether kernel-mode NEON is usable in the current
context, and simplifies kernel_neon_{begin,end}() to handle only
saving of the task FPSIMD state (if any).  Without nesting, there
is no other state to save.

The partial fpsimd save/restore functions become redundant as a
result of these changes, so they are removed too.

The save/restore model is changed to operate directly on
task_struct without additional percpu storage.  This simplifies the
code and saves a bit of memory, but means that softirqs must now be
disabled when manipulating the task fpsimd state from task context:
correspondingly, preempt_{en,dis}sable() calls are upgraded to
local_bh_{en,dis}able() as appropriate.  fpsimd_thread_switch()
already runs with hardirqs disabled and so is already protected
from softirqs.

These changes should make it easier to support kernel-mode NEON in
the presence of the Scalable Vector extension in the future.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/fpsimd.h       |  14 -----
 arch/arm64/include/asm/fpsimdmacros.h |  56 -----------------
 arch/arm64/include/asm/neon.h         |   4 +-
 arch/arm64/include/asm/simd.h         |  33 +++++++++-
 arch/arm64/kernel/entry-fpsimd.S      |  24 -------
 arch/arm64/kernel/fpsimd.c            | 115 +++++++++++++++++++++++-----------
 6 files changed, 112 insertions(+), 134 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 50f559f..ff2f6cd 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -41,16 +41,6 @@ struct fpsimd_state {
 	unsigned int cpu;
 };
 
-/*
- * Struct for stacking the bottom 'n' FP/SIMD registers.
- */
-struct fpsimd_partial_state {
-	u32		fpsr;
-	u32		fpcr;
-	u32		num_regs;
-	__uint128_t	vregs[32];
-};
-
 
 #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
 /* Masks for extracting the FPSR and FPCR from the FPSCR */
@@ -77,10 +67,6 @@ extern void fpsimd_update_current_state(struct fpsimd_state *state);
 
 extern void fpsimd_flush_task_state(struct task_struct *target);
 
-extern void fpsimd_save_partial_state(struct fpsimd_partial_state *state,
-				      u32 num_regs);
-extern void fpsimd_load_partial_state(struct fpsimd_partial_state *state);
-
 #endif
 
 #endif
diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h
index a2daf12..0f5fdd3 100644
--- a/arch/arm64/include/asm/fpsimdmacros.h
+++ b/arch/arm64/include/asm/fpsimdmacros.h
@@ -75,59 +75,3 @@
 	ldr	w\tmpnr, [\state, #16 * 2 + 4]
 	fpsimd_restore_fpcr x\tmpnr, \state
 .endm
-
-.macro fpsimd_save_partial state, numnr, tmpnr1, tmpnr2
-	mrs	x\tmpnr1, fpsr
-	str	w\numnr, [\state, #8]
-	mrs	x\tmpnr2, fpcr
-	stp	w\tmpnr1, w\tmpnr2, [\state]
-	adr	x\tmpnr1, 0f
-	add	\state, \state, x\numnr, lsl #4
-	sub	x\tmpnr1, x\tmpnr1, x\numnr, lsl #1
-	br	x\tmpnr1
-	stp	q30, q31, [\state, #-16 * 30 - 16]
-	stp	q28, q29, [\state, #-16 * 28 - 16]
-	stp	q26, q27, [\state, #-16 * 26 - 16]
-	stp	q24, q25, [\state, #-16 * 24 - 16]
-	stp	q22, q23, [\state, #-16 * 22 - 16]
-	stp	q20, q21, [\state, #-16 * 20 - 16]
-	stp	q18, q19, [\state, #-16 * 18 - 16]
-	stp	q16, q17, [\state, #-16 * 16 - 16]
-	stp	q14, q15, [\state, #-16 * 14 - 16]
-	stp	q12, q13, [\state, #-16 * 12 - 16]
-	stp	q10, q11, [\state, #-16 * 10 - 16]
-	stp	q8, q9, [\state, #-16 * 8 - 16]
-	stp	q6, q7, [\state, #-16 * 6 - 16]
-	stp	q4, q5, [\state, #-16 * 4 - 16]
-	stp	q2, q3, [\state, #-16 * 2 - 16]
-	stp	q0, q1, [\state, #-16 * 0 - 16]
-0:
-.endm
-
-.macro fpsimd_restore_partial state, tmpnr1, tmpnr2
-	ldp	w\tmpnr1, w\tmpnr2, [\state]
-	msr	fpsr, x\tmpnr1
-	fpsimd_restore_fpcr x\tmpnr2, x\tmpnr1
-	adr	x\tmpnr1, 0f
-	ldr	w\tmpnr2, [\state, #8]
-	add	\state, \state, x\tmpnr2, lsl #4
-	sub	x\tmpnr1, x\tmpnr1, x\tmpnr2, lsl #1
-	br	x\tmpnr1
-	ldp	q30, q31, [\state, #-16 * 30 - 16]
-	ldp	q28, q29, [\state, #-16 * 28 - 16]
-	ldp	q26, q27, [\state, #-16 * 26 - 16]
-	ldp	q24, q25, [\state, #-16 * 24 - 16]
-	ldp	q22, q23, [\state, #-16 * 22 - 16]
-	ldp	q20, q21, [\state, #-16 * 20 - 16]
-	ldp	q18, q19, [\state, #-16 * 18 - 16]
-	ldp	q16, q17, [\state, #-16 * 16 - 16]
-	ldp	q14, q15, [\state, #-16 * 14 - 16]
-	ldp	q12, q13, [\state, #-16 * 12 - 16]
-	ldp	q10, q11, [\state, #-16 * 10 - 16]
-	ldp	q8, q9, [\state, #-16 * 8 - 16]
-	ldp	q6, q7, [\state, #-16 * 6 - 16]
-	ldp	q4, q5, [\state, #-16 * 4 - 16]
-	ldp	q2, q3, [\state, #-16 * 2 - 16]
-	ldp	q0, q1, [\state, #-16 * 0 - 16]
-0:
-.endm
diff --git a/arch/arm64/include/asm/neon.h b/arch/arm64/include/asm/neon.h
index 5368bd0..fb9d137 100644
--- a/arch/arm64/include/asm/neon.h
+++ b/arch/arm64/include/asm/neon.h
@@ -16,9 +16,7 @@
 
 #define cpu_has_neon()		system_supports_fpsimd()
 
-#define kernel_neon_begin()	kernel_neon_begin_partial(32)
-
-void kernel_neon_begin_partial(u32 num_regs);
+void kernel_neon_begin(void);
 void kernel_neon_end(void);
 
 #endif /* ! __ASM_NEON_H */
diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h
index 96959b5..5a1a927 100644
--- a/arch/arm64/include/asm/simd.h
+++ b/arch/arm64/include/asm/simd.h
@@ -9,15 +9,46 @@
 #ifndef __ASM_SIMD_H
 #define __ASM_SIMD_H
 
+#include <linux/compiler.h>
+#include <linux/percpu.h>
+#include <linux/preempt.h>
 #include <linux/types.h>
 
+#ifdef CONFIG_KERNEL_MODE_NEON
+
+DECLARE_PER_CPU(bool, kernel_neon_busy);
+
 /*
  * may_use_simd - whether it is allowable at this time to issue SIMD
  *                instructions or access the SIMD register file
+ *
+ * Callers must not assume that the result remains true beyond the next
+ * preempt_enable() or return from softirq context.
  */
 static __must_check inline bool may_use_simd(void)
 {
-	return true;
+	/*
+	 * The raw_cpu_read() is racy if called with preemption enabled.
+	 * This is not a bug: kernel_neon_busy is only set when
+	 * preemption is disabled, so we cannot migrate to another CPU
+	 * while it is set, nor can we migrate to a CPU where it is set.
+	 * So, if we find it clear on some CPU then we're guaranteed to
+	 * find it clear on any CPU we could migrate to.
+	 *
+	 * If we are in between kernel_neon_begin()...kernel_neon_end(),
+	 * the flag will be set, but preemption is also disabled, so we
+	 * can't migrate to another CPU and spuriously see it become
+	 * false.
+	 */
+	return !in_irq() && !in_nmi() && !raw_cpu_read(kernel_neon_busy);
 }
 
+#else /* ! CONFIG_KERNEL_MODE_NEON */
+
+static __must_check inline bool may_use_simd(void) {
+	return false;
+}
+
+#endif /* ! CONFIG_KERNEL_MODE_NEON */
+
 #endif
diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
index c44a82f..6a27cd6 100644
--- a/arch/arm64/kernel/entry-fpsimd.S
+++ b/arch/arm64/kernel/entry-fpsimd.S
@@ -41,27 +41,3 @@ ENTRY(fpsimd_load_state)
 	fpsimd_restore x0, 8
 	ret
 ENDPROC(fpsimd_load_state)
-
-#ifdef CONFIG_KERNEL_MODE_NEON
-
-/*
- * Save the bottom n FP registers.
- *
- * x0 - pointer to struct fpsimd_partial_state
- */
-ENTRY(fpsimd_save_partial_state)
-	fpsimd_save_partial x0, 1, 8, 9
-	ret
-ENDPROC(fpsimd_save_partial_state)
-
-/*
- * Load the bottom n FP registers.
- *
- * x0 - pointer to struct fpsimd_partial_state
- */
-ENTRY(fpsimd_load_partial_state)
-	fpsimd_restore_partial x0, 8, 9
-	ret
-ENDPROC(fpsimd_load_partial_state)
-
-#endif
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index d7e5f8a..14329d6 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -17,16 +17,18 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/bottom_half.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/percpu.h>
 #include <linux/sched/signal.h>
 #include <linux/signal.h>
-#include <linux/hardirq.h>
 
 #include <asm/fpsimd.h>
 #include <asm/cputype.h>
+#include <asm/simd.h>
 
 #define FPEXC_IOF	(1 << 0)
 #define FPEXC_DZF	(1 << 1)
@@ -62,6 +64,13 @@
  * CPU currently contain the most recent userland FPSIMD state of the current
  * task.
  *
+ * In order to allow softirq handlers to use FPSIMD, kernel_neon_begin() may
+ * save the task's FPSIMD context back to task_struct from softirq context.
+ * To prevent this from racing with the manipulation of the task's FPSIMD state
+ * from task context and thereby corrupting the state, it is necessary to
+ * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE
+ * flag with local_bh_disable() unless softirqs are already masked.
+ *
  * For a certain task, the sequence may look something like this:
  * - the task gets scheduled in; if both the task's fpsimd_state.cpu field
  *   contains the id of the current CPU, and the CPU's fpsimd_last_state per-cpu
@@ -161,9 +170,14 @@ void fpsimd_flush_thread(void)
 {
 	if (!system_supports_fpsimd())
 		return;
+
+	local_bh_disable();
+
 	memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
 	fpsimd_flush_task_state(current);
 	set_thread_flag(TIF_FOREIGN_FPSTATE);
+
+	local_bh_enable();
 }
 
 /*
@@ -174,10 +188,13 @@ void fpsimd_preserve_current_state(void)
 {
 	if (!system_supports_fpsimd())
 		return;
-	preempt_disable();
+
+	local_bh_disable();
+
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
 		fpsimd_save_state(&current->thread.fpsimd_state);
-	preempt_enable();
+
+	local_bh_enable();
 }
 
 /*
@@ -189,7 +206,9 @@ void fpsimd_restore_current_state(void)
 {
 	if (!system_supports_fpsimd())
 		return;
-	preempt_disable();
+
+	local_bh_disable();
+
 	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
 		struct fpsimd_state *st = &current->thread.fpsimd_state;
 
@@ -197,7 +216,8 @@ void fpsimd_restore_current_state(void)
 		__this_cpu_write(fpsimd_last_state, st);
 		st->cpu = smp_processor_id();
 	}
-	preempt_enable();
+
+	local_bh_enable();
 }
 
 /*
@@ -209,7 +229,9 @@ void fpsimd_update_current_state(struct fpsimd_state *state)
 {
 	if (!system_supports_fpsimd())
 		return;
-	preempt_disable();
+
+	local_bh_disable();
+
 	fpsimd_load_state(state);
 	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
 		struct fpsimd_state *st = &current->thread.fpsimd_state;
@@ -217,7 +239,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state)
 		__this_cpu_write(fpsimd_last_state, st);
 		st->cpu = smp_processor_id();
 	}
-	preempt_enable();
+
+	local_bh_enable();
 }
 
 /*
@@ -230,49 +253,69 @@ void fpsimd_flush_task_state(struct task_struct *t)
 
 #ifdef CONFIG_KERNEL_MODE_NEON
 
-static DEFINE_PER_CPU(struct fpsimd_partial_state, hardirq_fpsimdstate);
-static DEFINE_PER_CPU(struct fpsimd_partial_state, softirq_fpsimdstate);
+DEFINE_PER_CPU(bool, kernel_neon_busy);
 
 /*
  * Kernel-side NEON support functions
  */
-void kernel_neon_begin_partial(u32 num_regs)
+
+/*
+ * kernel_neon_begin(): obtain the CPU FPSIMD registers for use by the calling
+ * context
+ *
+ * Must not be called unless may_use_simd() returns true.
+ * Task context in the FPSIMD registers is saved back to memory as necessary.
+ *
+ * A matching call to kernel_neon_end() must be made before returning from the
+ * calling context.
+ *
+ * The caller may freely use the FPSIMD registers until kernel_neon_end() is
+ * called.
+ */
+void kernel_neon_begin(void)
 {
 	if (WARN_ON(!system_supports_fpsimd()))
 		return;
-	if (in_interrupt()) {
-		struct fpsimd_partial_state *s = this_cpu_ptr(
-			in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
 
-		BUG_ON(num_regs > 32);
-		fpsimd_save_partial_state(s, roundup(num_regs, 2));
-	} else {
-		/*
-		 * Save the userland FPSIMD state if we have one and if we
-		 * haven't done so already. Clear fpsimd_last_state to indicate
-		 * that there is no longer userland FPSIMD state in the
-		 * registers.
-		 */
-		preempt_disable();
-		if (current->mm &&
-		    !test_and_set_thread_flag(TIF_FOREIGN_FPSTATE))
-			fpsimd_save_state(&current->thread.fpsimd_state);
-		this_cpu_write(fpsimd_last_state, NULL);
-	}
+	BUG_ON(!may_use_simd());
+
+	local_bh_disable();
+
+	__this_cpu_write(kernel_neon_busy, true);
+
+	/* Save unsaved task fpsimd state, if any: */
+	if (current->mm && !test_and_set_thread_flag(TIF_FOREIGN_FPSTATE))
+		fpsimd_save_state(&current->thread.fpsimd_state);
+
+	/* Invalidate any task state remaining in the fpsimd regs: */
+	__this_cpu_write(fpsimd_last_state, NULL);
+
+	preempt_disable();
+
+	local_bh_enable();
 }
-EXPORT_SYMBOL(kernel_neon_begin_partial);
+EXPORT_SYMBOL(kernel_neon_begin);
 
+/*
+ * kernel_neon_end(): give the CPU FPSIMD registers back to the current task
+ *
+ * Must be called from a context in which kernel_neon_begin() was previously
+ * called, with no call to kernel_neon_end() in the meantime.
+ *
+ * The caller must not use the FPSIMD registers after this function is called,
+ * unless kernel_neon_begin() is called again in the meantime.
+ */
 void kernel_neon_end(void)
 {
+	bool busy;
+
 	if (!system_supports_fpsimd())
 		return;
-	if (in_interrupt()) {
-		struct fpsimd_partial_state *s = this_cpu_ptr(
-			in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
-		fpsimd_load_partial_state(s);
-	} else {
-		preempt_enable();
-	}
+
+	busy = __this_cpu_xchg(kernel_neon_busy, false);
+	WARN_ON(!busy);	/* No matching kernel_neon_begin()? */
+
+	preempt_enable();
 }
 EXPORT_SYMBOL(kernel_neon_end);
 
-- 
2.1.4

[RFC PATCH v4 5/5] arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

[Dave Martin]

Now that kernel-mode NEON use is non-nestable and not allowed in
hardirq/nmi context, we have a problem if the kernel decides to
call into EFI during an interrupt that interrupted a
kernel_neon_begin()..._end() block.  This will occur if the kernel
tries to write diagnostic data to EFI persistent storage during
a panic triggered by an NMI for example.

EFI runtime services specify an ABI that clobbers the FPSIMD state,
rather than being able to use it optionally as an accelerator.
This means that EFI is really a special case and can be handled
separately.

To enable EFI calls from interrupts, this patch creates dedicated
__efi_fpsimd_{begin,end}() helpers solely for this purpose, which
save/restore to a separate percpu buffer if called in a context
where kernel_neon_begin() is not usable.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/efi.h    |  5 +++--
 arch/arm64/include/asm/fpsimd.h |  4 ++++
 arch/arm64/kernel/fpsimd.c      | 49 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8f3043a..8358222 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -3,6 +3,7 @@
 
 #include <asm/boot.h>
 #include <asm/cpufeature.h>
+#include <asm/fpsimd.h>
 #include <asm/io.h>
 #include <asm/mmu_context.h>
 #include <asm/neon.h>
@@ -20,8 +21,8 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 
 #define arch_efi_call_virt_setup()					\
 ({									\
-	kernel_neon_begin();						\
 	efi_virtmap_load();						\
+	__efi_fpsimd_begin();						\
 })
 
 #define arch_efi_call_virt(p, f, args...)				\
@@ -33,8 +34,8 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 
 #define arch_efi_call_virt_teardown()					\
 ({									\
+	__efi_fpsimd_end();						\
 	efi_virtmap_unload();						\
-	kernel_neon_end();						\
 })
 
 #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index ff2f6cd..410c481 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -67,6 +67,10 @@ extern void fpsimd_update_current_state(struct fpsimd_state *state);
 
 extern void fpsimd_flush_task_state(struct task_struct *target);
 
+/* For use by EFI runtime services calls only */
+extern void __efi_fpsimd_begin(void);
+extern void __efi_fpsimd_end(void);
+
 #endif
 
 #endif
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 14329d6..601063c 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/percpu.h>
+#include <linux/preempt.h>
 #include <linux/sched/signal.h>
 #include <linux/signal.h>
 
@@ -319,6 +320,54 @@ void kernel_neon_end(void)
 }
 EXPORT_SYMBOL(kernel_neon_end);
 
+DEFINE_PER_CPU(struct fpsimd_state, efi_fpsimd_state);
+DEFINE_PER_CPU(bool, efi_fpsimd_state_used);
+
+/*
+ * EFI runtime services support functions
+ *
+ * The ABI for EFI runtime services allows EFI to use FPSIMD during the call.
+ * This means that for EFI (and only for EFI), we have to assume that FPSIMD
+ * is always used rather than being an optional accelerator.
+ *
+ * These functions provide the necessary support for ensuring FPSIMD
+ * save/restore in the contexts from which EFI is used.
+ *
+ * Do not use them for any other purpose -- if tempted to do so, you are
+ * either doing something wrong or you need to propose some refactoring.
+ */
+
+/*
+ * __efi_fpsimd_begin(): prepare FPSIMD for making an EFI runtime services call
+ */
+void __efi_fpsimd_begin(void)
+{
+	if (!system_supports_fpsimd())
+		return;
+
+	WARN_ON(preemptible());
+
+	if (may_use_simd()) {
+		kernel_neon_begin();
+		__this_cpu_write(efi_fpsimd_state_used, true);
+	} else
+		fpsimd_save_state(this_cpu_ptr(&efi_fpsimd_state));
+}
+
+/*
+ * __efi_fpsimd_end(): clean up FPSIMD after an EFI runtime services call
+ */
+void __efi_fpsimd_end(void)
+{
+	if (!system_supports_fpsimd())
+		return;
+
+	if (__this_cpu_xchg(efi_fpsimd_state_used, false))
+		kernel_neon_end();
+	else
+		fpsimd_load_state(this_cpu_ptr(&efi_fpsimd_state));
+}
+
 #endif /* CONFIG_KERNEL_MODE_NEON */
 
 #ifdef CONFIG_CPU_PM
-- 
2.1.4

[PATCH] squash! arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

[Dave Martin]

Fix misleading usage of efi_fpsimd_state_used.

Currently it's used to mean "efi_fpsimd_state not used", which is
confusing to say the least.  This patch inverts the sense of usage.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/fpsimd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 601063c..138fcfa 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -347,11 +347,12 @@ void __efi_fpsimd_begin(void)
 
 	WARN_ON(preemptible());
 
-	if (may_use_simd()) {
+	if (may_use_simd())
 		kernel_neon_begin();
-		__this_cpu_write(efi_fpsimd_state_used, true);
-	} else
+	else {
 		fpsimd_save_state(this_cpu_ptr(&efi_fpsimd_state));
+		__this_cpu_write(efi_fpsimd_state_used, true);
+	}
 }
 
 /*
@@ -363,9 +364,9 @@ void __efi_fpsimd_end(void)
 		return;
 
 	if (__this_cpu_xchg(efi_fpsimd_state_used, false))
-		kernel_neon_end();
-	else
 		fpsimd_load_state(this_cpu_ptr(&efi_fpsimd_state));
+	else
+		kernel_neon_end();
 }
 
 #endif /* CONFIG_KERNEL_MODE_NEON */
-- 
2.1.4

[RFC PATCH v4 0/5] Simplify kernel-mode NEON

[Ard Biesheuvel]

(+ Herbert)

On 28 July 2017 at 14:50, Dave Martin <Dave.Martin@arm.com> wrote:
> This series aims to simplify kernel-mode NEON.
>
> The key changes are:
>
>  * Kernel-mode NEON is no longer nestable.
>
>  * Kernel-mode NEON is no longer permitted in hardirq or nmi context.
>
>  * Users of kernel-mode NEON must now call may_use_simd(), to determine
>    whether NEON may be used.  Since this function is no longer trivial
>    and can return false, the caller must typically provide a fallback
>    implementation of the optimised algoritm for this situation, or must
>    otherwise defer the algorithm execution to another context where it
>    can execute.
>
>  * EFI runtime servies save/restore NEON to/from a dedicated percpu
>    buffer if called in hardirq or nmi context.
>
> This series is motivated by SVE, which adds cost and complexity to
> management of kernel-mode NEON.  Since the most problematic features of
> KMN don't bring a substantial benefit even without SVE, it makes sense
> to simplify.
>
> This series depends on Ard's v4.14 crypto rework [2].
>

This looks all good to me, including the squash. I'd like to
coordinate with Herbert here, whether we can get this into the same
release (v4.14 preferably).

The reason is that, although the crypto rework patches and these
patches have no build time interdepencies, there is a change in
behavior in the generic SIMD helpers that I would like to avoid: if we
take one without the other, we may either fall back to non-SIMD code
in softirq context unnecessarily, or handle all async calls in the
caller's context using eager preserve/restore rather than deferring to
the kthread.

For patches 2-5

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


> AFAIK, that series makes all the required changes to the arm64 crypto
> library code.
>
>
> Changes since RFC v3:
>
>  * Rebased onto Ard's new series [2].
>
>  * Added EFI helpers so that EFI runtime service use still works from
>    interrupt context.
>
> Changes since RFC v2:
>
>  * Re-enable softirqs between kernel_neon_begin() and kernel_mode_end()
>
>    (This was the original intention, but I was overtaken by irrational
>    paranoia when drafting the previous version of the patch.)
>
>  * softirq disable/enable is removed from fpsimd_thread_switch() (which
>    runs with hardirqs disabled in any case, thus local_bh_enable() is
>    treated as an error by the core code).  The comment block is updated
>    to clarify this situation.
>
>    This also means that no cost is added to the context switch critical
>    path after all.
>
>  * Move the this_cpu_write(fpsimd_last_state, NULL) back outside the
>    conditional in kernel_neon_begin() (i.e., where it was originally
>    before this series).  RFC v2 accidentally moved this invalidation
>    inside the conditional, which leads to state corruption if switching
>    back to a user task previously running on the same CPU, after
>    kernel_mode_neon() is used by a kernel thread.
>
>  * Minor formatting and spurious #include tidyups.
>
> Testing:
>
>  * For now, I've hacked linux/kernel/time/timer.c:run_timer_softirq() to
>    call kernel_mode_neon_begin() and erase the FPSIMD registers, and
>    jacked CONFIG_HZ to 1000.  I also added a test syscall that userspace
>    can hammer to trigger the nested kernel_neon_begin() scenario.
>
>    This is not hugely realistic, but was sufficient to diagnose the
>    corruption problem described above, when running on a model.
>
>
> Original blurb:
>
> The main motivation for these changes is that supporting kernel-mode
> NEON alongside SVE is tricky with the current framework: the current
> partial save/restore mechanisms would need additional porting to
> save/restore the extended SVE vector bits, and this renders the cost
> saving of partial save/restore rather doubtful -- even if not all vector
> registers are saved, the save/restore cost will still grow with
> increasing vector length.  We could get the mechanics of this to work in
> principle, but it doesn't feel like the right thing to do.
>
> If we withdraw kernel-mode NEON support for hardirq context, accept some
> extra softirq latency and disable nesting, then we can simplify the code
> by always saving the task context directly into task_struct and
> deferring all restore to ret_to_user.  Previous discussions with Ard
> Biesheuvel suggest that this is a feasible approach and reasonably
> aligned with other architectures.
>
> The main impact on client code is that callers must check that
> kernel-mode NEON is usable in the current context and fall back to a
> non-NEON when necessary.  Ard has already done some work on this. [1]
>
> The interesting changes are all in patch 2: the first patch just adds a
> header inclusion guard that I noted was missing.
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git kernel-mode-neon
>
> [2] [PATCH resend 00/18] crypto: ARM/arm64 roundup for v4.14
> lists.infradead.org/pipermail/linux-arm-kernel/2017-July/520664.html
>
> Ard Biesheuvel (1):
>   arm64: neon: replace generic definition of may_use_simd()
>
> Dave Martin (4):
>   arm64: neon: Add missing header guard in <asm/neon.h>
>   arm64: fpsimd: Consistently use __this_cpu_ ops where appropriate
>   arm64: neon: Remove support for nested or hardirq kernel-mode NEON
>   arm64: neon: Allow EFI runtime services to use FPSIMD in irq context
>
>  arch/arm64/include/asm/Kbuild         |   1 -
>  arch/arm64/include/asm/efi.h          |   5 +-
>  arch/arm64/include/asm/fpsimd.h       |  16 +---
>  arch/arm64/include/asm/fpsimdmacros.h |  56 ------------
>  arch/arm64/include/asm/neon.h         |   9 +-
>  arch/arm64/include/asm/simd.h         |  54 +++++++++++
>  arch/arm64/kernel/entry-fpsimd.S      |  24 -----
>  arch/arm64/kernel/fpsimd.c            | 168 ++++++++++++++++++++++++++--------
>  8 files changed, 196 insertions(+), 137 deletions(-)
>  create mode 100644 arch/arm64/include/asm/simd.h
>
> --
> 2.1.4
>

[RFC PATCH v4 4/5] arm64: neon: Remove support for nested or hardirq kernel-mode NEON

[Catalin Marinas]

On Fri, Jul 28, 2017 at 02:50:23PM +0100, Dave P Martin wrote:
> Support for kernel-mode NEON to be nested and/or used in hardirq
> context adds significant complexity, and the benefits may be
> marginal.  In practice, kernel-mode NEON is not used in hardirq
> context, and is rarely used in softirq context (by certain mac80211
> drivers).

Just for clarification, the issue we had with hardirq contexts was that
we could be interrupted while saving the current FPSIMD state as we
don't want to disable IRQs during this. If this was the only aspect,
your patch looks fine. Note that softirqs may be executed as result of
an IRQ (via the irq_exit() function) but local_bh_disable() should
protect against such race.

-- 
Catalin

[RFC PATCH v4 4/5] arm64: neon: Remove support for nested or hardirq kernel-mode NEON

[Dave Martin]

On Wed, Aug 02, 2017 at 11:02:25AM +0100, Catalin Marinas wrote:
> On Fri, Jul 28, 2017 at 02:50:23PM +0100, Dave P Martin wrote:
> > Support for kernel-mode NEON to be nested and/or used in hardirq
> > context adds significant complexity, and the benefits may be
> > marginal.  In practice, kernel-mode NEON is not used in hardirq
> > context, and is rarely used in softirq context (by certain mac80211
> > drivers).
> 
> Just for clarification, the issue we had with hardirq contexts was that
> we could be interrupted while saving the current FPSIMD state as we
> don't want to disable IRQs during this. If this was the only aspect,

Certainly this is an aspect: for SVE we definitely don't want IRQs
disabled for the whole save/restore -- and I will be building SVE
support on top of this.

> your patch looks fine. Note that softirqs may be executed as result of
> an IRQ (via the irq_exit() function) but local_bh_disable() should
> protect against such race.

This is my understanding: any such softirq would get deferred until
local_bh_enable(), or would run in ksoftirqd.

There's an assumption here that EFI runtime services calls won't be made
from softirq context, but I think that's reasonable: softirqs are far
from general-purpose these days.

Was there any other concen here that you're aware of?

Cheers
---Dave

[RFC PATCH v4 4/5] arm64: neon: Remove support for nested or hardirq kernel-mode NEON

[Catalin Marinas]

On Wed, Aug 02, 2017 at 02:00:52PM +0100, Dave P Martin wrote:
> On Wed, Aug 02, 2017 at 11:02:25AM +0100, Catalin Marinas wrote:
> > On Fri, Jul 28, 2017 at 02:50:23PM +0100, Dave P Martin wrote:
> > > Support for kernel-mode NEON to be nested and/or used in hardirq
> > > context adds significant complexity, and the benefits may be
> > > marginal.  In practice, kernel-mode NEON is not used in hardirq
> > > context, and is rarely used in softirq context (by certain mac80211
> > > drivers).
> > 
> > Just for clarification, the issue we had with hardirq contexts was that
> > we could be interrupted while saving the current FPSIMD state as we
> > don't want to disable IRQs during this. If this was the only aspect,
> 
> Certainly this is an aspect: for SVE we definitely don't want IRQs
> disabled for the whole save/restore -- and I will be building SVE
> support on top of this.
> 
> > your patch looks fine. Note that softirqs may be executed as result of
> > an IRQ (via the irq_exit() function) but local_bh_disable() should
> > protect against such race.
> 
> This is my understanding: any such softirq would get deferred until
> local_bh_enable(), or would run in ksoftirqd.
> 
> There's an assumption here that EFI runtime services calls won't be made
> from softirq context, but I think that's reasonable: softirqs are far
> from general-purpose these days.
> 
> Was there any other concen here that you're aware of?

No. Just wanted to double-check my assumptions were right.

-- 
Catalin

[RFC PATCH v4 5/5] arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

[Catalin Marinas]

On Fri, Jul 28, 2017 at 02:50:24PM +0100, Dave P Martin wrote:
> Now that kernel-mode NEON use is non-nestable and not allowed in
> hardirq/nmi context, we have a problem if the kernel decides to
> call into EFI during an interrupt that interrupted a
> kernel_neon_begin()..._end() block.  This will occur if the kernel
> tries to write diagnostic data to EFI persistent storage during
> a panic triggered by an NMI for example.
> 
> EFI runtime services specify an ABI that clobbers the FPSIMD state,
> rather than being able to use it optionally as an accelerator.
> This means that EFI is really a special case and can be handled
> separately.
> 
> To enable EFI calls from interrupts, this patch creates dedicated
> __efi_fpsimd_{begin,end}() helpers solely for this purpose, which
> save/restore to a separate percpu buffer if called in a context
> where kernel_neon_begin() is not usable.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Should this patch be reordered with patch 4? It looks like patch 4 no
longer allows EFI runtime services in IRQ context.

-- 
Catalin

[RFC PATCH v4 5/5] arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

[Dave Martin]

On Wed, Aug 02, 2017 at 03:58:01PM +0100, Catalin Marinas wrote:
> On Fri, Jul 28, 2017 at 02:50:24PM +0100, Dave P Martin wrote:
> > Now that kernel-mode NEON use is non-nestable and not allowed in
> > hardirq/nmi context, we have a problem if the kernel decides to
> > call into EFI during an interrupt that interrupted a
> > kernel_neon_begin()..._end() block.  This will occur if the kernel
> > tries to write diagnostic data to EFI persistent storage during
> > a panic triggered by an NMI for example.
> > 
> > EFI runtime services specify an ABI that clobbers the FPSIMD state,
> > rather than being able to use it optionally as an accelerator.
> > This means that EFI is really a special case and can be handled
> > separately.
> > 
> > To enable EFI calls from interrupts, this patch creates dedicated
> > __efi_fpsimd_{begin,end}() helpers solely for this purpose, which
> > save/restore to a separate percpu buffer if called in a context
> > where kernel_neon_begin() is not usable.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> Should this patch be reordered with patch 4? It looks like patch 4 no
> longer allows EFI runtime services in IRQ context.

Hmmm, yes, patches 4/5 are not bisectable at present.

I can't immediately see why patch 5 won't work if they're swapped:
rather, the !may_use_simd() fallback and efi_fpsimd_state won't get
used, and save/restore around EFI should work much the same as it does
today.

I'm happy to swap the patches, but I don't want to put too much effort
into confirming the above.  How concerned are you about the risk?

Cheers
---Dave

[RFC PATCH v4 5/5] arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

[Catalin Marinas]

On Wed, Aug 02, 2017 at 04:15:21PM +0100, Dave P Martin wrote:
> On Wed, Aug 02, 2017 at 03:58:01PM +0100, Catalin Marinas wrote:
> > On Fri, Jul 28, 2017 at 02:50:24PM +0100, Dave P Martin wrote:
> > > Now that kernel-mode NEON use is non-nestable and not allowed in
> > > hardirq/nmi context, we have a problem if the kernel decides to
> > > call into EFI during an interrupt that interrupted a
> > > kernel_neon_begin()..._end() block.  This will occur if the kernel
> > > tries to write diagnostic data to EFI persistent storage during
> > > a panic triggered by an NMI for example.
> > > 
> > > EFI runtime services specify an ABI that clobbers the FPSIMD state,
> > > rather than being able to use it optionally as an accelerator.
> > > This means that EFI is really a special case and can be handled
> > > separately.
> > > 
> > > To enable EFI calls from interrupts, this patch creates dedicated
> > > __efi_fpsimd_{begin,end}() helpers solely for this purpose, which
> > > save/restore to a separate percpu buffer if called in a context
> > > where kernel_neon_begin() is not usable.
> > > 
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > 
> > Should this patch be reordered with patch 4? It looks like patch 4 no
> > longer allows EFI runtime services in IRQ context.
> 
> Hmmm, yes, patches 4/5 are not bisectable at present.
> 
> I can't immediately see why patch 5 won't work if they're swapped:
> rather, the !may_use_simd() fallback and efi_fpsimd_state won't get
> used, and save/restore around EFI should work much the same as it does
> today.
> 
> I'm happy to swap the patches, but I don't want to put too much effort
> into confirming the above.  How concerned are you about the risk?

I'm not concerned but we could at least test that it boots with patches
1-4.

-- 
Catalin

[RFC PATCH v4 5/5] arm64: neon: Allow EFI runtime services to use FPSIMD in irq context

[Dave Martin]

On Wed, Aug 02, 2017 at 05:02:36PM +0100, Catalin Marinas wrote:
> On Wed, Aug 02, 2017 at 04:15:21PM +0100, Dave P Martin wrote:
> > On Wed, Aug 02, 2017 at 03:58:01PM +0100, Catalin Marinas wrote:
> > > On Fri, Jul 28, 2017 at 02:50:24PM +0100, Dave P Martin wrote:
> > > > Now that kernel-mode NEON use is non-nestable and not allowed in
> > > > hardirq/nmi context, we have a problem if the kernel decides to
> > > > call into EFI during an interrupt that interrupted a
> > > > kernel_neon_begin()..._end() block.  This will occur if the kernel
> > > > tries to write diagnostic data to EFI persistent storage during
> > > > a panic triggered by an NMI for example.
> > > > 
> > > > EFI runtime services specify an ABI that clobbers the FPSIMD state,
> > > > rather than being able to use it optionally as an accelerator.
> > > > This means that EFI is really a special case and can be handled
> > > > separately.
> > > > 
> > > > To enable EFI calls from interrupts, this patch creates dedicated
> > > > __efi_fpsimd_{begin,end}() helpers solely for this purpose, which
> > > > save/restore to a separate percpu buffer if called in a context
> > > > where kernel_neon_begin() is not usable.
> > > > 
> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > 
> > > Should this patch be reordered with patch 4? It looks like patch 4 no
> > > longer allows EFI runtime services in IRQ context.
> > 
> > Hmmm, yes, patches 4/5 are not bisectable at present.
> > 
> > I can't immediately see why patch 5 won't work if they're swapped:
> > rather, the !may_use_simd() fallback and efi_fpsimd_state won't get
> > used, and save/restore around EFI should work much the same as it does
> > today.
> > 
> > I'm happy to swap the patches, but I don't want to put too much effort
> > into confirming the above.  How concerned are you about the risk?
> 
> I'm not concerned but we could at least test that it boots with patches
> 1-4.

Agreed, I'll be checking that, but I won't otherwise kick it too hard.

Cheers
---Dave