[PATCH v9 0/4] arm64: mte: allow async MTE to be upgraded to sync on a per-CPU basis

On some CPUs the performance of MTE in synchronous mode is similar
to that of asynchronous mode. This makes it worthwhile to enable
synchronous mode on those CPUs when asynchronous mode is requested,
in order to gain the error detection benefits of synchronous mode
without the performance downsides. Therefore, make it possible for
user programs to opt into upgrading to synchronous mode on those CPUs.

This is done by introducing a notion of a preferred TCF mode, which is
controlled on a per-CPU basis by a sysfs node. The existing SYNC and
ASYNC TCF settings are repurposed as bitfields that specify a set of
possible modes. If the preferred TCF mode for a particular CPU is in
the user-provided mode set (this will always be the case for mode sets
containing more than one mode because the kernel only supports two tag
checking modes, but future kernels may support more modes) then that
mode is used when running on that CPU, otherwise one of the modes in
the task's mode set will be selected in a currently unspecified manner.

v8:
- split into multiple patches
- remove MTE_CTRL_TCF_NONE
- improve documentation
- disable preemption and add comment to mte_update_sctlr_user
- bring back PR_MTE_TCF_SHIFT for source compatibility
- address formatting nit

v7:
- switch to new API proposed on list

v6:
- switch to strings in sysfs nodes instead of TCF values

v5:
- updated documentation
- address some nits in mte.c

v4:
- switch to new mte_ctrl field
- make register_mte_upgrade_async_sysctl return an int
- change the sysctl to take 0 or 1 instead of raw TCF values
- "same as" -> "similar to"

v3:
- drop the device tree support
- add documentation
- add static_assert to ensure no overlap with real HW bits
- move per-CPU variable initialization to mte.c
- use smp_call_function_single instead of stop_machine

v2:
- make it an opt-in behavior
- change the format of the device tree node
- also allow controlling the feature via sysfs

Peter Collingbourne (4):
  arm64: mte: rename gcr_user_excl to mte_ctrl
  arm64: mte: change ASYNC and SYNC TCF settings into bitfields
  arm64: mte: introduce a per-CPU tag checking mode preference
  Documentation: document the preferred tag checking mode feature

 .../ABI/testing/sysfs-devices-system-cpu      |  16 ++
 .../arm64/memory-tagging-extension.rst        |  48 +++++-
 arch/arm64/include/asm/processor.h            |   8 +-
 arch/arm64/kernel/asm-offsets.c               |   2 +-
 arch/arm64/kernel/entry.S                     |   4 +-
 arch/arm64/kernel/mte.c                       | 149 ++++++++++++------
 include/uapi/linux/prctl.h                    |  11 +-
 7 files changed, 178 insertions(+), 60 deletions(-)

-- 
2.32.0.93.g670b81a890-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

We are going to use this field to store more data. To prepare for
that, rename it and change the users to rely on the bit position of
gcr_user_excl in mte_ctrl.

Link: https://linux-review.googlesource.com/id/Ie1fd18e480100655f5d22137f5b22f4f3a9f9e2e
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/processor.h |  5 ++++-
 arch/arm64/kernel/asm-offsets.c    |  2 +-
 arch/arm64/kernel/entry.S          |  4 ++--
 arch/arm64/kernel/mte.c            | 14 ++++++++------
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 9df3feeee890..6322fb1714d5 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h

@@ -16,6 +16,9 @@
  */
 #define NET_IP_ALIGN	0
 
+#define MTE_CTRL_GCR_USER_EXCL_SHIFT	0
+#define MTE_CTRL_GCR_USER_EXCL_MASK	0xffff
+
 #ifndef __ASSEMBLY__
 
 #include <linux/build_bug.h>

@@ -151,7 +154,7 @@ struct thread_struct {
 	struct ptrauth_keys_kernel	keys_kernel;
 #endif
 #ifdef CONFIG_ARM64_MTE
-	u64			gcr_user_excl;
+	u64			mte_ctrl;
 #endif
 	u64			sctlr_user;
 };

diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 0cb34ccb6e73..63d02cd67b44 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c

@@ -49,7 +49,7 @@ int main(void)
   DEFINE(THREAD_KEYS_KERNEL,	offsetof(struct task_struct, thread.keys_kernel));
 #endif
 #ifdef CONFIG_ARM64_MTE
-  DEFINE(THREAD_GCR_EL1_USER,	offsetof(struct task_struct, thread.gcr_user_excl));
+  DEFINE(THREAD_MTE_CTRL,	offsetof(struct task_struct, thread.mte_ctrl));
 #endif
   BLANK();
   DEFINE(S_X0,			offsetof(struct pt_regs, regs[0]));

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 3513984a88bd..ce59280355c5 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S

@@ -182,7 +182,7 @@ alternative_else_nop_endif
 	 * the RRND (bit[16]) setting.
 	 */
 	mrs_s	\tmp2, SYS_GCR_EL1
-	bfi	\tmp2, \tmp, #0, #16
+	bfxil	\tmp2, \tmp, #MTE_CTRL_GCR_USER_EXCL_SHIFT, #16
 	msr_s	SYS_GCR_EL1, \tmp2
 #endif
 	.endm

@@ -205,7 +205,7 @@ alternative_else_nop_endif
 alternative_if_not ARM64_MTE
 	b	1f
 alternative_else_nop_endif
-	ldr	\tmp, [\tsk, #THREAD_GCR_EL1_USER]
+	ldr	\tmp, [\tsk, #THREAD_MTE_CTRL]
 
 	mte_set_gcr \tmp, \tmp2
 1:

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 125a10e413e9..d3884d09513d 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -199,7 +199,7 @@ static void update_gcr_el1_excl(u64 excl)
 
 static void set_gcr_el1_excl(u64 excl)
 {
-	current->thread.gcr_user_excl = excl;
+	current->thread.mte_ctrl = excl;
 
 	/*
 	 * SYS_GCR_EL1 will be set to current->thread.gcr_user_excl value

@@ -263,8 +263,8 @@ void mte_suspend_exit(void)
 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
 	u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
-	u64 gcr_excl = ~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
-		       SYS_GCR_EL1_EXCL_MASK;
+	u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
+			SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
 
 	if (!system_supports_mte())
 		return 0;

@@ -285,10 +285,10 @@ long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 
 	if (task != current) {
 		task->thread.sctlr_user = sctlr;
-		task->thread.gcr_user_excl = gcr_excl;
+		task->thread.mte_ctrl = mte_ctrl;
 	} else {
 		set_task_sctlr_el1(sctlr);
-		set_gcr_el1_excl(gcr_excl);
+		set_gcr_el1_excl(mte_ctrl);
 	}
 
 	return 0;

@@ -297,7 +297,9 @@ long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 long get_mte_ctrl(struct task_struct *task)
 {
 	unsigned long ret;
-	u64 incl = ~task->thread.gcr_user_excl & SYS_GCR_EL1_EXCL_MASK;
+	u64 mte_ctrl = task->thread.mte_ctrl;
+	u64 incl = (~mte_ctrl >> MTE_CTRL_GCR_USER_EXCL_SHIFT) &
+		   SYS_GCR_EL1_EXCL_MASK;
 
 	if (!system_supports_mte())
 		return 0;

-- 
2.32.0.93.g670b81a890-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add a per-CPU sysfs node, mte_tcf_preferred, that allows the preferred
tag checking mode to be configured. The current possible values are
async and sync.

Link: https://linux-review.googlesource.com/id/I7493dcd533a2785a1437b16c3f6b50919f840854
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/mte.c | 77 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 53d89915029d..48f218e070cc 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -4,6 +4,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/prctl.h>

@@ -26,6 +27,8 @@ u64 gcr_kernel_excl __ro_after_init;
 
 static bool report_fault_once = true;
 
+static DEFINE_PER_CPU_READ_MOSTLY(u64, mte_tcf_preferred);
+
 #ifdef CONFIG_KASAN_HW_TAGS
 /* Whether the MTE asynchronous mode is enabled. */
 DEFINE_STATIC_KEY_FALSE(mte_async_mode);

@@ -199,17 +202,24 @@ static void update_gcr_el1_excl(u64 excl)
 
 static void mte_update_sctlr_user(struct task_struct *task)
 {
+	/*
+	 * This can only be called on the current or next task since the CPU
+	 * must match where the thread is going to run.
+	 */
 	unsigned long sctlr = task->thread.sctlr_user;
-	unsigned long pref = MTE_CTRL_TCF_ASYNC;
 	unsigned long mte_ctrl = task->thread.mte_ctrl;
-	unsigned long resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
+	unsigned long pref, resolved_mte_tcf;
 
+	preempt_disable();
+	pref = __this_cpu_read(mte_tcf_preferred);
+	resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
 	sctlr &= ~SCTLR_EL1_TCF0_MASK;
 	if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
 		sctlr |= SCTLR_EL1_TCF0_ASYNC;
 	else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
 		sctlr |= SCTLR_EL1_TCF0_SYNC;
 	task->thread.sctlr_user = sctlr;
+	preempt_enable();
 }
 
 void mte_thread_init_user(void)

@@ -441,3 +451,66 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,
 
 	return ret;
 }
+
+static ssize_t mte_tcf_preferred_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	switch (per_cpu(mte_tcf_preferred, dev->id)) {
+	case MTE_CTRL_TCF_ASYNC:
+		return sysfs_emit(buf, "async\n");
+	case MTE_CTRL_TCF_SYNC:
+		return sysfs_emit(buf, "sync\n");
+	default:
+		return sysfs_emit(buf, "???\n");
+	}
+}
+
+static void sync_sctlr(void *arg)
+{
+	mte_update_sctlr_user(current);
+	set_task_sctlr_el1(current->thread.sctlr_user);
+}
+
+static ssize_t mte_tcf_preferred_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	ssize_t ret = 0;
+	u64 tcf;
+
+	if (sysfs_streq(buf, "async"))
+		tcf = MTE_CTRL_TCF_ASYNC;
+	else if (sysfs_streq(buf, "sync"))
+		tcf = MTE_CTRL_TCF_SYNC;
+	else
+		return -EINVAL;
+
+	device_lock(dev);
+	per_cpu(mte_tcf_preferred, dev->id) = tcf;
+
+	if (cpu_online(dev->id))
+		ret = smp_call_function_single(dev->id, sync_sctlr, NULL, 0);
+	if (ret == 0)
+		ret = count;
+	device_unlock(dev);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(mte_tcf_preferred);
+
+static int register_mte_tcf_preferred_sysctl(void)
+{
+	unsigned int cpu;
+
+	if (!system_supports_mte())
+		return 0;
+
+	for_each_possible_cpu(cpu) {
+		per_cpu(mte_tcf_preferred, cpu) = MTE_CTRL_TCF_ASYNC;
+		device_create_file(get_cpu_device(cpu),
+				   &dev_attr_mte_tcf_preferred);
+	}
+
+	return 0;
+}
+subsys_initcall(register_mte_tcf_preferred_sysctl);

-- 
2.32.0.93.g670b81a890-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Allow the user program to specify both ASYNC and SYNC TCF modes by
repurposing the existing constants as bitfields. This will allow the
kernel to select one of the modes on behalf of the user program. With
this patch the kernel will always select async mode, but a subsequent
patch will make this configurable.

Link: https://linux-review.googlesource.com/id/Icc5923c85a8ea284588cc399ae74fd19ec291230
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
v9:
- make mte_update_sctlr_user static

 arch/arm64/include/asm/processor.h |  3 ++
 arch/arm64/kernel/mte.c            | 70 ++++++++++++------------------
 include/uapi/linux/prctl.h         | 11 ++---
 3 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 6322fb1714d5..80ceb9cbdd60 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h

@@ -19,6 +19,9 @@
 #define MTE_CTRL_GCR_USER_EXCL_SHIFT	0
 #define MTE_CTRL_GCR_USER_EXCL_MASK	0xffff
 
+#define MTE_CTRL_TCF_SYNC		(1UL << 16)
+#define MTE_CTRL_TCF_ASYNC		(1UL << 17)
+
 #ifndef __ASSEMBLY__
 
 #include <linux/build_bug.h>

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index d3884d09513d..53d89915029d 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -197,14 +197,19 @@ static void update_gcr_el1_excl(u64 excl)
 	sysreg_clear_set_s(SYS_GCR_EL1, SYS_GCR_EL1_EXCL_MASK, excl);
 }
 
-static void set_gcr_el1_excl(u64 excl)
+static void mte_update_sctlr_user(struct task_struct *task)
 {
-	current->thread.mte_ctrl = excl;
+	unsigned long sctlr = task->thread.sctlr_user;
+	unsigned long pref = MTE_CTRL_TCF_ASYNC;
+	unsigned long mte_ctrl = task->thread.mte_ctrl;
+	unsigned long resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
 
-	/*
-	 * SYS_GCR_EL1 will be set to current->thread.gcr_user_excl value
-	 * by mte_set_user_gcr() in kernel_exit,
-	 */
+	sctlr &= ~SCTLR_EL1_TCF0_MASK;
+	if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
+		sctlr |= SCTLR_EL1_TCF0_ASYNC;
+	else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
+		sctlr |= SCTLR_EL1_TCF0_SYNC;
+	task->thread.sctlr_user = sctlr;
 }
 
 void mte_thread_init_user(void)

@@ -216,15 +221,16 @@ void mte_thread_init_user(void)
 	dsb(ish);
 	write_sysreg_s(0, SYS_TFSRE0_EL1);
 	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
-	/* disable tag checking */
-	set_task_sctlr_el1((current->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK) |
-			   SCTLR_EL1_TCF0_NONE);
-	/* reset tag generation mask */
-	set_gcr_el1_excl(SYS_GCR_EL1_EXCL_MASK);
+	/* disable tag checking and reset tag generation mask */
+	current->thread.mte_ctrl = MTE_CTRL_GCR_USER_EXCL_MASK;
+	mte_update_sctlr_user(current);
+	set_task_sctlr_el1(current->thread.sctlr_user);
 }
 
 void mte_thread_switch(struct task_struct *next)
 {
+	mte_update_sctlr_user(next);
+
 	/*
 	 * Check if an async tag exception occurred at EL1.
 	 *

@@ -262,33 +268,21 @@ void mte_suspend_exit(void)
 
 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
-	u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
 	u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
 			SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
 
 	if (!system_supports_mte())
 		return 0;
 
-	switch (arg & PR_MTE_TCF_MASK) {
-	case PR_MTE_TCF_NONE:
-		sctlr |= SCTLR_EL1_TCF0_NONE;
-		break;
-	case PR_MTE_TCF_SYNC:
-		sctlr |= SCTLR_EL1_TCF0_SYNC;
-		break;
-	case PR_MTE_TCF_ASYNC:
-		sctlr |= SCTLR_EL1_TCF0_ASYNC;
-		break;
-	default:
-		return -EINVAL;
-	}
+	if (arg & PR_MTE_TCF_ASYNC)
+		mte_ctrl |= MTE_CTRL_TCF_ASYNC;
+	if (arg & PR_MTE_TCF_SYNC)
+		mte_ctrl |= MTE_CTRL_TCF_SYNC;
 
-	if (task != current) {
-		task->thread.sctlr_user = sctlr;
-		task->thread.mte_ctrl = mte_ctrl;
-	} else {
-		set_task_sctlr_el1(sctlr);
-		set_gcr_el1_excl(mte_ctrl);
+	task->thread.mte_ctrl = mte_ctrl;
+	if (task == current) {
+		mte_update_sctlr_user(task);
+		set_task_sctlr_el1(task->thread.sctlr_user);
 	}
 
 	return 0;

@@ -305,18 +299,10 @@ long get_mte_ctrl(struct task_struct *task)
 		return 0;
 
 	ret = incl << PR_MTE_TAG_SHIFT;
-
-	switch (task->thread.sctlr_user & SCTLR_EL1_TCF0_MASK) {
-	case SCTLR_EL1_TCF0_NONE:
-		ret |= PR_MTE_TCF_NONE;
-		break;
-	case SCTLR_EL1_TCF0_SYNC:
-		ret |= PR_MTE_TCF_SYNC;
-		break;
-	case SCTLR_EL1_TCF0_ASYNC:
+	if (mte_ctrl & MTE_CTRL_TCF_ASYNC)
 		ret |= PR_MTE_TCF_ASYNC;
-		break;
-	}
+	if (mte_ctrl & MTE_CTRL_TCF_SYNC)
+		ret |= PR_MTE_TCF_SYNC;
 
 	return ret;
 }

diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 18a9f59dc067..d3a5afb4c1ae 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h

@@ -234,14 +234,15 @@ struct prctl_mm_map {
 #define PR_GET_TAGGED_ADDR_CTRL		56
 # define PR_TAGGED_ADDR_ENABLE		(1UL << 0)
 /* MTE tag check fault modes */
-# define PR_MTE_TCF_SHIFT		1
-# define PR_MTE_TCF_NONE		(0UL << PR_MTE_TCF_SHIFT)
-# define PR_MTE_TCF_SYNC		(1UL << PR_MTE_TCF_SHIFT)
-# define PR_MTE_TCF_ASYNC		(2UL << PR_MTE_TCF_SHIFT)
-# define PR_MTE_TCF_MASK		(3UL << PR_MTE_TCF_SHIFT)
+# define PR_MTE_TCF_NONE		0
+# define PR_MTE_TCF_SYNC		(1UL << 1)
+# define PR_MTE_TCF_ASYNC		(1UL << 2)
+# define PR_MTE_TCF_MASK		(PR_MTE_TCF_SYNC | PR_MTE_TCF_ASYNC)
 /* MTE tag inclusion mask */
 # define PR_MTE_TAG_SHIFT		3
 # define PR_MTE_TAG_MASK		(0xffffUL << PR_MTE_TAG_SHIFT)
+/* Unused; kept only for source compatibility */
+# define PR_MTE_TCF_SHIFT		1
 
 /* Control reclaim behavior when allocating memory */
 #define PR_SET_IO_FLUSHER		57

-- 
2.32.0.93.g670b81a890-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Document the functionality added in the previous patches.

Link: https://linux-review.googlesource.com/id/I48217cc3e8b8da33abc08cbaddc11cf4360a1b86
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
v9:
- add documentation for sysfs node under Documentation/ABI

 .../ABI/testing/sysfs-devices-system-cpu      | 16 +++++++
 .../arm64/memory-tagging-extension.rst        | 48 ++++++++++++++++---
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index fe13baa53c59..195587b7c7ec 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu

@@ -640,3 +640,19 @@ Description:	SPURR ticks for cpuX when it was idle.
 
 		This sysfs interface exposes the number of SPURR ticks
 		for cpuX when it was idle.
+
+What: 		/sys/devices/system/cpu/cpuX/mte_tcf_preferred
+Date:		Jul 2021
+Contact:	Linux ARM Kernel Mailing list <linux-arm-kernel@lists.infradead.org>
+Description:	Preferred MTE tag checking mode
+
+		When a user program specifies more than one MTE tag checking
+		mode, this sysfs node is used to specify which mode should
+		be preferred when running on that CPU. Possible values:
+
+		================  ==============================================
+		"sync"	  	  Prefer synchronous mode
+		"async"	  	  Prefer asynchronous mode
+		================  ==============================================
+
+		See also: Documentation/arm64/memory-tagging-extension.rst

diff --git a/Documentation/arm64/memory-tagging-extension.rst b/Documentation/arm64/memory-tagging-extension.rst
index b540178a93f8..7b99c8f428eb 100644
--- a/Documentation/arm64/memory-tagging-extension.rst
+++ b/Documentation/arm64/memory-tagging-extension.rst

@@ -77,14 +77,20 @@ configurable behaviours:
   address is unknown).
 
 The user can select the above modes, per thread, using the
-``prctl(PR_SET_TAGGED_ADDR_CTRL, flags, 0, 0, 0)`` system call where
-``flags`` contain one of the following values in the ``PR_MTE_TCF_MASK``
+``prctl(PR_SET_TAGGED_ADDR_CTRL, flags, 0, 0, 0)`` system call where ``flags``
+contains any number of the following values in the ``PR_MTE_TCF_MASK``
 bit-field:
 
-- ``PR_MTE_TCF_NONE``  - *Ignore* tag check faults
+- ``PR_MTE_TCF_NONE``  - *Ignore* tag check faults
+                         (ignored if combined with other options)
 - ``PR_MTE_TCF_SYNC``  - *Synchronous* tag check fault mode
 - ``PR_MTE_TCF_ASYNC`` - *Asynchronous* tag check fault mode
 
+If no modes are specified, tag check faults are ignored. If a single
+mode is specified, the program will run in that mode. If multiple
+modes are specified, the mode is selected as described in the "Per-CPU
+preferred tag checking modes" section below.
+
 The current tag check fault mode can be read using the
 ``prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0, 0)`` system call.
 

@@ -120,13 +126,39 @@ in the ``PR_MTE_TAG_MASK`` bit-field.
 interface provides an include mask. An include mask of ``0`` (exclusion
 mask ``0xffff``) results in the CPU always generating tag ``0``.
 
+Per-CPU preferred tag checking mode
+-----------------------------------
+
+On some CPUs the performance of MTE in stricter tag checking modes
+is similar to that of less strict tag checking modes. This makes it
+worthwhile to enable stricter checks on those CPUs when a less strict
+checking mode is requested, in order to gain the error detection
+benefits of the stricter checks without the performance downsides. To
+support this scenario, a privileged user may configure a stricter
+tag checking mode as the CPU's preferred tag checking mode.
+
+The preferred tag checking mode for each CPU is controlled by
+``/sys/devices/system/cpu/cpu<N>/mte_tcf_preferred``, to which a
+privileged user may write the value ``async`` or ``sync``.  The default
+preferred mode for each CPU is ``async``.
+
+To allow a program to potentially run in the CPU's preferred tag
+checking mode, the user program may set multiple tag check fault mode
+bits in the ``flags`` argument to the ``prctl(PR_SET_TAGGED_ADDR_CTRL,
+flags, 0, 0, 0)`` system call. If the CPU's preferred tag checking
+mode is in the task's set of provided tag checking modes (this will
+always be the case at present because the kernel only supports two
+tag checking modes, but future kernels may support more modes), that
+mode will be selected. Otherwise, one of the modes in the task's mode
+set will be selected in a currently unspecified manner.
+
 Initial process state
 ---------------------
 
 On ``execve()``, the new process has the following configuration:
 
 - ``PR_TAGGED_ADDR_ENABLE`` set to 0 (disabled)
-- Tag checking mode set to ``PR_MTE_TCF_NONE``
+- No tag checking modes are selected (tag check faults ignored)
 - ``PR_MTE_TAG_MASK`` set to 0 (all tags excluded)
 - ``PSTATE.TCO`` set to 0
 - ``PROT_MTE`` not set on any of the initial memory maps

@@ -251,11 +283,13 @@ Example of correct usage
                     return EXIT_FAILURE;
 
             /*
-             * Enable the tagged address ABI, synchronous MTE tag check faults and
-             * allow all non-zero tags in the randomly generated set.
+             * Enable the tagged address ABI, synchronous or asynchronous MTE
+             * tag check faults (based on per-CPU preference) and allow all
+             * non-zero tags in the randomly generated set.
              */
             if (prctl(PR_SET_TAGGED_ADDR_CTRL,
-                      PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC | (0xfffe << PR_MTE_TAG_SHIFT),
+                      PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC | PR_MTE_TCF_ASYNC |
+                      (0xfffe << PR_MTE_TAG_SHIFT),
                       0, 0, 0)) {
                     perror("prctl() failed");
                     return EXIT_FAILURE;

-- 
2.32.0.93.g670b81a890-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:

Allow the user program to specify both ASYNC and SYNC TCF modes by
repurposing the existing constants as bitfields. This will allow the
kernel to select one of the modes on behalf of the user program. With
this patch the kernel will always select async mode, but a subsequent
patch will make this configurable.

Link: https://linux-review.googlesource.com/id/Icc5923c85a8ea284588cc399ae74fd19ec291230
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
v9:
- make mte_update_sctlr_user static

 arch/arm64/include/asm/processor.h |  3 ++
 arch/arm64/kernel/mte.c            | 70 ++++++++++++------------------
 include/uapi/linux/prctl.h         | 11 ++---
 3 files changed, 37 insertions(+), 47 deletions(-)

...

 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
-	u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
 	u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
 			SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
 
 	if (!system_supports_mte())
 		return 0;
 
-	switch (arg & PR_MTE_TCF_MASK) {
-	case PR_MTE_TCF_NONE:
-		sctlr |= SCTLR_EL1_TCF0_NONE;
-		break;
-	case PR_MTE_TCF_SYNC:
-		sctlr |= SCTLR_EL1_TCF0_SYNC;
-		break;
-	case PR_MTE_TCF_ASYNC:
-		sctlr |= SCTLR_EL1_TCF0_ASYNC;
-		break;
-	default:
-		return -EINVAL;
-	}
+	if (arg & PR_MTE_TCF_ASYNC)
+		mte_ctrl |= MTE_CTRL_TCF_ASYNC;
+	if (arg & PR_MTE_TCF_SYNC)
+		mte_ctrl |= MTE_CTRL_TCF_SYNC;
 
-	if (task != current) {
-		task->thread.sctlr_user = sctlr;
-		task->thread.mte_ctrl = mte_ctrl;
-	} else {
-		set_task_sctlr_el1(sctlr);
-		set_gcr_el1_excl(mte_ctrl);
+	task->thread.mte_ctrl = mte_ctrl;
+	if (task == current) {
+		mte_update_sctlr_user(task);

In conjunction with the next patch, what happens if we migrate at this
point? I worry that we can install a stale sctlr_user value.

+		set_task_sctlr_el1(task->thread.sctlr_user);

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Jul 02, 2021 at 12:41:09PM -0700, Peter Collingbourne wrote:

Hmm, so this call could land right in the middle of a concurrent prctl().
What happens in that case?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Jul 7, 2021 at 4:15 AM Will Deacon [off-list ref] wrote:

On Fri, Jul 02, 2021 at 12:41:09PM -0700, Peter Collingbourne wrote:

quoted

Add a per-CPU sysfs node, mte_tcf_preferred, that allows the preferred
tag checking mode to be configured. The current possible values are
async and sync.

Link: https://linux-review.googlesource.com/id/I7493dcd533a2785a1437b16c3f6b50919f840854
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/mte.c | 77 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 53d89915029d..48f218e070cc 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -4,6 +4,7 @@
  */

 #include <linux/bitops.h>
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/prctl.h>

@@ -26,6 +27,8 @@ u64 gcr_kernel_excl __ro_after_init;

 static bool report_fault_once = true;

+static DEFINE_PER_CPU_READ_MOSTLY(u64, mte_tcf_preferred);
+
 #ifdef CONFIG_KASAN_HW_TAGS
 /* Whether the MTE asynchronous mode is enabled. */
 DEFINE_STATIC_KEY_FALSE(mte_async_mode);

@@ -199,17 +202,24 @@ static void update_gcr_el1_excl(u64 excl)

 static void mte_update_sctlr_user(struct task_struct *task)
 {
+     /*
+      * This can only be called on the current or next task since the CPU
+      * must match where the thread is going to run.
+      */
      unsigned long sctlr = task->thread.sctlr_user;
-     unsigned long pref = MTE_CTRL_TCF_ASYNC;
      unsigned long mte_ctrl = task->thread.mte_ctrl;
-     unsigned long resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
+     unsigned long pref, resolved_mte_tcf;

+     preempt_disable();
+     pref = __this_cpu_read(mte_tcf_preferred);
+     resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
      sctlr &= ~SCTLR_EL1_TCF0_MASK;
      if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
              sctlr |= SCTLR_EL1_TCF0_ASYNC;
      else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
              sctlr |= SCTLR_EL1_TCF0_SYNC;
      task->thread.sctlr_user = sctlr;
+     preempt_enable();
 }

 void mte_thread_init_user(void)

@@ -441,3 +451,66 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,

      return ret;
 }
+
+static ssize_t mte_tcf_preferred_show(struct device *dev,
+                                   struct device_attribute *attr, char *buf)
+{
+     switch (per_cpu(mte_tcf_preferred, dev->id)) {
+     case MTE_CTRL_TCF_ASYNC:
+             return sysfs_emit(buf, "async\n");
+     case MTE_CTRL_TCF_SYNC:
+             return sysfs_emit(buf, "sync\n");
+     default:
+             return sysfs_emit(buf, "???\n");
+     }
+}
+
+static void sync_sctlr(void *arg)
+{
+     mte_update_sctlr_user(current);
+     set_task_sctlr_el1(current->thread.sctlr_user);
+}
+
+static ssize_t mte_tcf_preferred_store(struct device *dev,
+                                    struct device_attribute *attr,
+                                    const char *buf, size_t count)
+{
+     ssize_t ret = 0;
+     u64 tcf;
+
+     if (sysfs_streq(buf, "async"))
+             tcf = MTE_CTRL_TCF_ASYNC;
+     else if (sysfs_streq(buf, "sync"))
+             tcf = MTE_CTRL_TCF_SYNC;
+     else
+             return -EINVAL;
+
+     device_lock(dev);
+     per_cpu(mte_tcf_preferred, dev->id) = tcf;
+
+     if (cpu_online(dev->id))
+             ret = smp_call_function_single(dev->id, sync_sctlr, NULL, 0);

Hmm, so this call could land right in the middle of a concurrent prctl().
What happens in that case?

I don't think anything can go wrong. When the prctl sets mte_ctrl we
then need to call mte_update_sctlr_user followed by set_task_sctlr_el1
and it doesn't matter if it happens multiple times (either
mte_update_sctlr_user/set_task_sctlr_el1/mte_update_sctlr_user/set_task_sctlr_el1
or even the less likely
mte_update_sctlr_user/mte_update_sctlr_user/set_task_sctlr_el1/set_task_sctlr_el1).

Actually, I'm not sure we need any code in the sync_sctlr function at
all. Simply scheduling an empty function onto the CPU will cause
mte_update_sctlr_user and then update_sctlr_el1 to be called when the
task that was originally running on the CPU gets rescheduled onto it,
as a result of the change to mte_thread_switch.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Jul 7, 2021 at 4:11 AM Will Deacon [off-list ref] wrote:

On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:

quoted

Allow the user program to specify both ASYNC and SYNC TCF modes by
repurposing the existing constants as bitfields. This will allow the
kernel to select one of the modes on behalf of the user program. With
this patch the kernel will always select async mode, but a subsequent
patch will make this configurable.

Link: https://linux-review.googlesource.com/id/Icc5923c85a8ea284588cc399ae74fd19ec291230
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
v9:
- make mte_update_sctlr_user static

 arch/arm64/include/asm/processor.h |  3 ++
 arch/arm64/kernel/mte.c            | 70 ++++++++++++------------------
 include/uapi/linux/prctl.h         | 11 ++---
 3 files changed, 37 insertions(+), 47 deletions(-)

...

quoted

 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
-     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
      u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
                      SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;

      if (!system_supports_mte())
              return 0;

-     switch (arg & PR_MTE_TCF_MASK) {
-     case PR_MTE_TCF_NONE:
-             sctlr |= SCTLR_EL1_TCF0_NONE;
-             break;
-     case PR_MTE_TCF_SYNC:
-             sctlr |= SCTLR_EL1_TCF0_SYNC;
-             break;
-     case PR_MTE_TCF_ASYNC:
-             sctlr |= SCTLR_EL1_TCF0_ASYNC;
-             break;
-     default:
-             return -EINVAL;
-     }
+     if (arg & PR_MTE_TCF_ASYNC)
+             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
+     if (arg & PR_MTE_TCF_SYNC)
+             mte_ctrl |= MTE_CTRL_TCF_SYNC;

-     if (task != current) {
-             task->thread.sctlr_user = sctlr;
-             task->thread.mte_ctrl = mte_ctrl;
-     } else {
-             set_task_sctlr_el1(sctlr);
-             set_gcr_el1_excl(mte_ctrl);
+     task->thread.mte_ctrl = mte_ctrl;
+     if (task == current) {
+             mte_update_sctlr_user(task);

In conjunction with the next patch, what happens if we migrate at this
point? I worry that we can install a stale sctlr_user value.

quoted

+             set_task_sctlr_el1(task->thread.sctlr_user);

In this case, we will call mte_update_sctlr_user when scheduled onto
the new CPU as a result of the change to mte_thread_switch, and both
the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
for the current CPU.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jul 12, 2021 at 12:04:39PM -0700, Peter Collingbourne wrote:

On Wed, Jul 7, 2021 at 4:11 AM Will Deacon [off-list ref] wrote:

quoted

On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:

quoted

 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
-     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
      u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
                      SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;

      if (!system_supports_mte())
              return 0;

-     switch (arg & PR_MTE_TCF_MASK) {
-     case PR_MTE_TCF_NONE:
-             sctlr |= SCTLR_EL1_TCF0_NONE;
-             break;
-     case PR_MTE_TCF_SYNC:
-             sctlr |= SCTLR_EL1_TCF0_SYNC;
-             break;
-     case PR_MTE_TCF_ASYNC:
-             sctlr |= SCTLR_EL1_TCF0_ASYNC;
-             break;
-     default:
-             return -EINVAL;
-     }
+     if (arg & PR_MTE_TCF_ASYNC)
+             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
+     if (arg & PR_MTE_TCF_SYNC)
+             mte_ctrl |= MTE_CTRL_TCF_SYNC;

-     if (task != current) {
-             task->thread.sctlr_user = sctlr;
-             task->thread.mte_ctrl = mte_ctrl;
-     } else {
-             set_task_sctlr_el1(sctlr);
-             set_gcr_el1_excl(mte_ctrl);
+     task->thread.mte_ctrl = mte_ctrl;
+     if (task == current) {
+             mte_update_sctlr_user(task);

In conjunction with the next patch, what happens if we migrate at this
point? I worry that we can install a stale sctlr_user value.

quoted

+             set_task_sctlr_el1(task->thread.sctlr_user);

In this case, we will call mte_update_sctlr_user when scheduled onto
the new CPU as a result of the change to mte_thread_switch, and both
the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
for the current CPU.

Doesn't that rely on task->thread.sctlr_user being explicitly read on the
new CPU? For example, the following rough sequence is what I'm worried
about:


CPU x (prefer ASYNC)
set_mte_ctrl(ASYNC | SYNC)
	current->thread.mte_ctrl = ASYNC | SYNC;
	mte_update_sctlr_user
		current->thread.sctlr_user = ASYNC;
	Register Xn = current->thread.sctlr_user; // ASYNC
	<migration to CPU y>

CPU y (prefer SYNC)
mte_thread_switch
	mte_update_sctlr_user
		next->thread.sctlr_user = SYNC;
	update_sctlr_el1
		SCTLR_EL1 = SYNC;

	<resume next back in set_mte_ctrl>
	set_task_sctlr_el1(Xn); // ASYNC
	current->thread.sctlr_user = Xn; // ASYNC  XXX: also superfluous?
	SCTLR_EL1 = ASYNC;


Does that make sense?

I'm thinking set_mte_ctrl() should be using update_sctlr_el1() and disabling
preemption around the whole thing, which would make it a lot closer to the
context-switch path.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jul 12, 2021 at 11:59:39AM -0700, Peter Collingbourne wrote:

On Wed, Jul 7, 2021 at 4:15 AM Will Deacon [off-list ref] wrote:

quoted

On Fri, Jul 02, 2021 at 12:41:09PM -0700, Peter Collingbourne wrote:

quoted

Add a per-CPU sysfs node, mte_tcf_preferred, that allows the preferred
tag checking mode to be configured. The current possible values are
async and sync.

Link: https://linux-review.googlesource.com/id/I7493dcd533a2785a1437b16c3f6b50919f840854
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/mte.c | 77 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 53d89915029d..48f218e070cc 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -4,6 +4,7 @@
  */

 #include <linux/bitops.h>
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/prctl.h>

@@ -26,6 +27,8 @@ u64 gcr_kernel_excl __ro_after_init;

 static bool report_fault_once = true;

+static DEFINE_PER_CPU_READ_MOSTLY(u64, mte_tcf_preferred);
+
 #ifdef CONFIG_KASAN_HW_TAGS
 /* Whether the MTE asynchronous mode is enabled. */
 DEFINE_STATIC_KEY_FALSE(mte_async_mode);

@@ -199,17 +202,24 @@ static void update_gcr_el1_excl(u64 excl)

 static void mte_update_sctlr_user(struct task_struct *task)
 {
+     /*
+      * This can only be called on the current or next task since the CPU
+      * must match where the thread is going to run.
+      */
      unsigned long sctlr = task->thread.sctlr_user;
-     unsigned long pref = MTE_CTRL_TCF_ASYNC;
      unsigned long mte_ctrl = task->thread.mte_ctrl;
-     unsigned long resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
+     unsigned long pref, resolved_mte_tcf;

+     preempt_disable();
+     pref = __this_cpu_read(mte_tcf_preferred);
+     resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
      sctlr &= ~SCTLR_EL1_TCF0_MASK;
      if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
              sctlr |= SCTLR_EL1_TCF0_ASYNC;
      else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
              sctlr |= SCTLR_EL1_TCF0_SYNC;
      task->thread.sctlr_user = sctlr;
+     preempt_enable();
 }

 void mte_thread_init_user(void)

@@ -441,3 +451,66 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,

      return ret;
 }
+
+static ssize_t mte_tcf_preferred_show(struct device *dev,
+                                   struct device_attribute *attr, char *buf)
+{
+     switch (per_cpu(mte_tcf_preferred, dev->id)) {
+     case MTE_CTRL_TCF_ASYNC:
+             return sysfs_emit(buf, "async\n");
+     case MTE_CTRL_TCF_SYNC:
+             return sysfs_emit(buf, "sync\n");
+     default:
+             return sysfs_emit(buf, "???\n");
+     }
+}
+
+static void sync_sctlr(void *arg)
+{
+     mte_update_sctlr_user(current);
+     set_task_sctlr_el1(current->thread.sctlr_user);
+}
+
+static ssize_t mte_tcf_preferred_store(struct device *dev,
+                                    struct device_attribute *attr,
+                                    const char *buf, size_t count)
+{
+     ssize_t ret = 0;
+     u64 tcf;
+
+     if (sysfs_streq(buf, "async"))
+             tcf = MTE_CTRL_TCF_ASYNC;
+     else if (sysfs_streq(buf, "sync"))
+             tcf = MTE_CTRL_TCF_SYNC;
+     else
+             return -EINVAL;
+
+     device_lock(dev);
+     per_cpu(mte_tcf_preferred, dev->id) = tcf;
+
+     if (cpu_online(dev->id))
+             ret = smp_call_function_single(dev->id, sync_sctlr, NULL, 0);

Hmm, so this call could land right in the middle of a concurrent prctl().
What happens in that case?

I don't think anything can go wrong. When the prctl sets mte_ctrl we
then need to call mte_update_sctlr_user followed by set_task_sctlr_el1
and it doesn't matter if it happens multiple times (either
mte_update_sctlr_user/set_task_sctlr_el1/mte_update_sctlr_user/set_task_sctlr_el1
or even the less likely
mte_update_sctlr_user/mte_update_sctlr_user/set_task_sctlr_el1/set_task_sctlr_el1).

I think you're still (at least) relying on the compiler not to tear or cache
accesses to ->thread.sctlr_user, no? It seems like it would be a lot simpler
just to leave the change until the next context switch and not send the IPI
at all. I think that's a reasonable behaviour because the write to sysfs is
racy anyway, so we can just document this.

Actually, I'm not sure we need any code in the sync_sctlr function at
all. Simply scheduling an empty function onto the CPU will cause
mte_update_sctlr_user and then update_sctlr_el1 to be called when the
task that was originally running on the CPU gets rescheduled onto it,
as a result of the change to mte_thread_switch.

I'm not sure I agree here -- I thought smp_call_function_single() ran the
target function directly from the IPI handler in interrupt context, without
the need for a context-switch.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Jul 02, 2021 at 12:41:07PM -0700, Peter Collingbourne wrote:

We are going to use this field to store more data. To prepare for
that, rename it and change the users to rely on the bit position of
gcr_user_excl in mte_ctrl.

Link: https://linux-review.googlesource.com/id/Ie1fd18e480100655f5d22137f5b22f4f3a9f9e2e
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/processor.h |  5 ++++-
 arch/arm64/kernel/asm-offsets.c    |  2 +-
 arch/arm64/kernel/entry.S          |  4 ++--
 arch/arm64/kernel/mte.c            | 14 ++++++++------
 4 files changed, 15 insertions(+), 10 deletions(-)

Makes sense to me:

Acked-by: Will Deacon <will@kernel.org>

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Jul 02, 2021 at 12:41:10PM -0700, Peter Collingbourne wrote:

nit: Seems like the other entries here spell out the full name of the month.

With that fixed:

Acked-by: Will Deacon <will@kernel.org>

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jul 13, 2021 at 10:48 AM Will Deacon [off-list ref] wrote:

On Mon, Jul 12, 2021 at 11:59:39AM -0700, Peter Collingbourne wrote:

quoted

On Wed, Jul 7, 2021 at 4:15 AM Will Deacon [off-list ref] wrote:

quoted

On Fri, Jul 02, 2021 at 12:41:09PM -0700, Peter Collingbourne wrote:

quoted

Add a per-CPU sysfs node, mte_tcf_preferred, that allows the preferred
tag checking mode to be configured. The current possible values are
async and sync.

Link: https://linux-review.googlesource.com/id/I7493dcd533a2785a1437b16c3f6b50919f840854
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/mte.c | 77 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 53d89915029d..48f218e070cc 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c

@@ -4,6 +4,7 @@
  */

 #include <linux/bitops.h>
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/prctl.h>

@@ -26,6 +27,8 @@ u64 gcr_kernel_excl __ro_after_init;

 static bool report_fault_once = true;

+static DEFINE_PER_CPU_READ_MOSTLY(u64, mte_tcf_preferred);
+
 #ifdef CONFIG_KASAN_HW_TAGS
 /* Whether the MTE asynchronous mode is enabled. */
 DEFINE_STATIC_KEY_FALSE(mte_async_mode);

@@ -199,17 +202,24 @@ static void update_gcr_el1_excl(u64 excl)

 static void mte_update_sctlr_user(struct task_struct *task)
 {
+     /*
+      * This can only be called on the current or next task since the CPU
+      * must match where the thread is going to run.
+      */
      unsigned long sctlr = task->thread.sctlr_user;
-     unsigned long pref = MTE_CTRL_TCF_ASYNC;
      unsigned long mte_ctrl = task->thread.mte_ctrl;
-     unsigned long resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
+     unsigned long pref, resolved_mte_tcf;

+     preempt_disable();
+     pref = __this_cpu_read(mte_tcf_preferred);
+     resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
      sctlr &= ~SCTLR_EL1_TCF0_MASK;
      if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
              sctlr |= SCTLR_EL1_TCF0_ASYNC;
      else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
              sctlr |= SCTLR_EL1_TCF0_SYNC;
      task->thread.sctlr_user = sctlr;
+     preempt_enable();
 }

 void mte_thread_init_user(void)

@@ -441,3 +451,66 @@ int mte_ptrace_copy_tags(struct task_struct *child, long request,

      return ret;
 }
+
+static ssize_t mte_tcf_preferred_show(struct device *dev,
+                                   struct device_attribute *attr, char *buf)
+{
+     switch (per_cpu(mte_tcf_preferred, dev->id)) {
+     case MTE_CTRL_TCF_ASYNC:
+             return sysfs_emit(buf, "async\n");
+     case MTE_CTRL_TCF_SYNC:
+             return sysfs_emit(buf, "sync\n");
+     default:
+             return sysfs_emit(buf, "???\n");
+     }
+}
+
+static void sync_sctlr(void *arg)
+{
+     mte_update_sctlr_user(current);
+     set_task_sctlr_el1(current->thread.sctlr_user);
+}
+
+static ssize_t mte_tcf_preferred_store(struct device *dev,
+                                    struct device_attribute *attr,
+                                    const char *buf, size_t count)
+{
+     ssize_t ret = 0;
+     u64 tcf;
+
+     if (sysfs_streq(buf, "async"))
+             tcf = MTE_CTRL_TCF_ASYNC;
+     else if (sysfs_streq(buf, "sync"))
+             tcf = MTE_CTRL_TCF_SYNC;
+     else
+             return -EINVAL;
+
+     device_lock(dev);
+     per_cpu(mte_tcf_preferred, dev->id) = tcf;
+
+     if (cpu_online(dev->id))
+             ret = smp_call_function_single(dev->id, sync_sctlr, NULL, 0);

Hmm, so this call could land right in the middle of a concurrent prctl().
What happens in that case?

I don't think anything can go wrong. When the prctl sets mte_ctrl we
then need to call mte_update_sctlr_user followed by set_task_sctlr_el1
and it doesn't matter if it happens multiple times (either
mte_update_sctlr_user/set_task_sctlr_el1/mte_update_sctlr_user/set_task_sctlr_el1
or even the less likely
mte_update_sctlr_user/mte_update_sctlr_user/set_task_sctlr_el1/set_task_sctlr_el1).

I think you're still (at least) relying on the compiler not to tear or cache
accesses to ->thread.sctlr_user, no? It seems like it would be a lot simpler
just to leave the change until the next context switch and not send the IPI
at all. I think that's a reasonable behaviour because the write to sysfs is
racy anyway, so we can just document this.

Works for me. I removed the call to smp_call_function_single here and
updated the documentation patch to say that changes won't take effect
immediately.

quoted

Actually, I'm not sure we need any code in the sync_sctlr function at
all. Simply scheduling an empty function onto the CPU will cause
mte_update_sctlr_user and then update_sctlr_el1 to be called when the
task that was originally running on the CPU gets rescheduled onto it,
as a result of the change to mte_thread_switch.

I'm not sure I agree here -- I thought smp_call_function_single() ran the
target function directly from the IPI handler in interrupt context, without
the need for a context-switch.

I see -- I thought that they ran as something like a kthread, but you
may be right.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jul 13, 2021 at 10:27 AM Will Deacon [off-list ref] wrote:

On Mon, Jul 12, 2021 at 12:04:39PM -0700, Peter Collingbourne wrote:

quoted

On Wed, Jul 7, 2021 at 4:11 AM Will Deacon [off-list ref] wrote:

quoted

On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:

quoted

 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
-     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
      u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
                      SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;

      if (!system_supports_mte())
              return 0;

-     switch (arg & PR_MTE_TCF_MASK) {
-     case PR_MTE_TCF_NONE:
-             sctlr |= SCTLR_EL1_TCF0_NONE;
-             break;
-     case PR_MTE_TCF_SYNC:
-             sctlr |= SCTLR_EL1_TCF0_SYNC;
-             break;
-     case PR_MTE_TCF_ASYNC:
-             sctlr |= SCTLR_EL1_TCF0_ASYNC;
-             break;
-     default:
-             return -EINVAL;
-     }
+     if (arg & PR_MTE_TCF_ASYNC)
+             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
+     if (arg & PR_MTE_TCF_SYNC)
+             mte_ctrl |= MTE_CTRL_TCF_SYNC;

-     if (task != current) {
-             task->thread.sctlr_user = sctlr;
-             task->thread.mte_ctrl = mte_ctrl;
-     } else {
-             set_task_sctlr_el1(sctlr);
-             set_gcr_el1_excl(mte_ctrl);
+     task->thread.mte_ctrl = mte_ctrl;
+     if (task == current) {
+             mte_update_sctlr_user(task);

In conjunction with the next patch, what happens if we migrate at this
point? I worry that we can install a stale sctlr_user value.

quoted

+             set_task_sctlr_el1(task->thread.sctlr_user);

In this case, we will call mte_update_sctlr_user when scheduled onto
the new CPU as a result of the change to mte_thread_switch, and both
the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
for the current CPU.

Doesn't that rely on task->thread.sctlr_user being explicitly read on the
new CPU? For example, the following rough sequence is what I'm worried
about:


CPU x (prefer ASYNC)
set_mte_ctrl(ASYNC | SYNC)
        current->thread.mte_ctrl = ASYNC | SYNC;
        mte_update_sctlr_user
                current->thread.sctlr_user = ASYNC;
        Register Xn = current->thread.sctlr_user; // ASYNC
        <migration to CPU y>

CPU y (prefer SYNC)
mte_thread_switch
        mte_update_sctlr_user
                next->thread.sctlr_user = SYNC;
        update_sctlr_el1
                SCTLR_EL1 = SYNC;

        <resume next back in set_mte_ctrl>
        set_task_sctlr_el1(Xn); // ASYNC
        current->thread.sctlr_user = Xn; // ASYNC  XXX: also superfluous?
        SCTLR_EL1 = ASYNC;


Does that make sense?

I'm thinking set_mte_ctrl() should be using update_sctlr_el1() and disabling
preemption around the whole thing, which would make it a lot closer to the
context-switch path.

Okay, I see what you mean. I also noticed that
prctl(PR_PAC_SET_ENABLED_KEYS) would now have the same problem. In v10
I've addressed this issue by inserting a patch after this one that
disables preemption in both prctl implementations.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jul 13, 2021 at 10:53 AM Will Deacon [off-list ref] wrote:

On Fri, Jul 02, 2021 at 12:41:10PM -0700, Peter Collingbourne wrote:

quoted

Document the functionality added in the previous patches.

Link: https://linux-review.googlesource.com/id/I48217cc3e8b8da33abc08cbaddc11cf4360a1b86
Signed-off-by: Peter Collingbourne <redacted>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
v9:
- add documentation for sysfs node under Documentation/ABI

 .../ABI/testing/sysfs-devices-system-cpu      | 16 +++++++
 .../arm64/memory-tagging-extension.rst        | 48 ++++++++++++++++---
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index fe13baa53c59..195587b7c7ec 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu

@@ -640,3 +640,19 @@ Description:     SPURR ticks for cpuX when it was idle.

              This sysfs interface exposes the number of SPURR ticks
              for cpuX when it was idle.
+
+What:                /sys/devices/system/cpu/cpuX/mte_tcf_preferred
+Date:                Jul 2021

nit: Seems like the other entries here spell out the full name of the month.

FWIW, it seems to vary based on which part of the file you're looking
at (e.g. some entries near the end abbreviate it, so I followed that
pattern since my addition was at the end). But it looks like it isn't
abbreviated in the majority of entries, so I changed it.

With that fixed:

Acked-by: Will Deacon <will@kernel.org>

Thanks.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel