[kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Vladimir Murzin]

Test tag storage access and tag mismatch for different MTE modes.

Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---

 v1 -> v2
    - Addressed comments (I hope I did not miss any) from Alexandru

 arm/Makefile.arm64            |  10 +-
 arm/cstart64.S                |   4 +-
 arm/mte.c                     | 299 ++++++++++++++++++++++++++++++++++
 arm/run                       |   3 +-
 arm/unittests.cfg             |  19 +++
 lib/arm64/asm/mmu.h           |   1 +
 lib/arm64/asm/pgtable-hwdef.h |   2 +
 lib/arm64/asm/sysreg.h        |  12 ++
 8 files changed, 347 insertions(+), 3 deletions(-)
 create mode 100644 arm/mte.c

diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
index 3b9034e3..48dcdbd4 100644
--- a/arm/Makefile.arm64
+++ b/arm/Makefile.arm64
@@ -17,10 +17,17 @@ ifneq ($(strip $(sve_flag)),)
 CFLAGS += -DCC_HAS_SVE
 endif
 
+mte_flag := $(call cc-option, -march=armv8.5-a+memtag, "")
+ifneq ($(strip $(mte_flag)),)
+# MTE is supported by the compiler, generate MTE instructions
+CFLAGS += -DCC_HAS_MTE
+endif
+
+
 mno_outline_atomics := $(call cc-option, -mno-outline-atomics, "")
 CFLAGS += $(mno_outline_atomics)
 CFLAGS += -DCONFIG_RELOC
-CFLAGS += -mgeneral-regs-only
+CFLAGS += -mgeneral-regs-only -save-temps
 
 define arch_elf_check =
 	$(if $(shell ! $(READELF) -rW $(1) >&/dev/null && echo "nok"),
@@ -57,6 +64,7 @@ tests += $(TEST_DIR)/micro-bench.$(exe)
 tests += $(TEST_DIR)/cache.$(exe)
 tests += $(TEST_DIR)/debug.$(exe)
 tests += $(TEST_DIR)/fpu.$(exe)
+tests += $(TEST_DIR)/mte.$(exe)
 
 include $(SRCDIR)/$(TEST_DIR)/Makefile.common
 
diff --git a/arm/cstart64.S b/arm/cstart64.S
index b480a552..b9d7a446 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -242,6 +242,7 @@ halt:
  *   NORMAL             100     11111111
  *   NORMAL_WT          101     10111011
  *   DEVICE_nGRE        110     00001000
+ *   NORMAL_TAGGED      111     11110000
  */
 #define MAIR(attr, mt) ((attr) << ((mt) * 8))
 
@@ -275,7 +276,8 @@ asm_mmu_enable:
 		     MAIR(0x44, MT_NORMAL_NC) |		\
 		     MAIR(0xff, MT_NORMAL) |	        \
 		     MAIR(0xbb, MT_NORMAL_WT) |         \
-		     MAIR(0x08, MT_DEVICE_nGRE)
+		     MAIR(0x08, MT_DEVICE_nGRE) |       \
+		     MAIR(0xf0, MT_NORMAL_TAGGED)
 	msr	mair_el1, x1
 
 	/* TTBR0 */
diff --git a/arm/mte.c b/arm/mte.c
new file mode 100644
index 00000000..718f0a61
--- /dev/null
+++ b/arm/mte.c
@@ -0,0 +1,299 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2024 Arm Limited.
+ * All rights reserved.
+ */
+
+#include <libcflat.h>
+#include <alloc_page.h>
+#include <stdlib.h>
+#include <asm/mmu.h>
+#include <asm/sysreg.h>
+#include <asm/pgtable-hwdef.h>
+#include <asm/processor.h>
+#include <asm/thread_info.h>
+
+
+/* Tag Check Faults cause a synchronous exception */
+#define MTE_TCF_SYNC	0b01
+/* Tag Check Faults are asynchronously accumulated */
+#define MTE_TCF_ASYNC	0b10
+/*
+ * Tag Check Faults cause a synchronous exception on reads,
+ * and are asynchronously accumulated on writes
+ */
+#define MTE_TCF_ASYMM	0b11
+
+#define MTE_GRANULE_SIZE        UL(16)
+#define MTE_GRANULE_MASK        (~(MTE_GRANULE_SIZE - 1))
+#define MTE_TAG_SHIFT           56
+
+#define untagged(p)									\
+({											\
+	unsigned long __in = (unsigned long)(p);					\
+	typeof(p) __out = (typeof(p))(__in & ~(MTE_GRANULE_MASK << MTE_TAG_SHIFT));	\
+											\
+	__out;										\
+})
+
+#define tagged(p,t)							\
+({									\
+	unsigned long __in = (unsigned long)(untagged(p));		\
+	unsigned long __tag = (unsigned long)(t) << MTE_TAG_SHIFT;	\
+	typeof(p) __out = (typeof(p))(__in | __tag);			\
+									\
+	__out;								\
+})
+
+/*
+ * If we use a normal (non hand coded inline assembly) load or store
+ * to access a tagged address, the compiler will reasonably assume
+ * that the access succeeded, and the next instruction may do
+ * something based on that assumption.
+ *
+ * But a test might want the tagged access to fail on purpose, and if
+ * we advance the PC to the next instruction, the one added by the
+ * compiler, we might leave the program in an unexpected state.
+ */
+static inline void mem_read(unsigned int *addr, unsigned int tag, unsigned int *res)
+{
+	unsigned int r;
+
+	asm volatile ("ldr %0,[%1]\n"
+		      "str %0,[%2]\n"
+		      : "=r" (r)
+		      : "r" (tagged(addr, tag)), "r" (res));
+}
+
+static inline void mem_write(unsigned int *addr, unsigned int tag, unsigned int val)
+{
+	/* The NOP allows the same exception handler as mem_read() to be used. */
+	asm volatile ("str %0,[%1]\n"
+		      "nop\n"
+		      :
+		      : "r" (val), "r" (tagged(addr, tag))
+		      : "memory");
+}
+
+static volatile bool mte_exception;
+
+static void mte_fault_handler(struct pt_regs *regs, unsigned int esr)
+{
+	unsigned int dfsc = esr & GENMASK(5, 0);
+	unsigned int fnv = esr & BIT(10);
+
+	if ((dfsc == 0b010001) && (fnv == 0))
+		mte_exception = true;
+
+	/*
+	 * mem_read() reads the value from the tagged pointer, then
+	 * stores this value in the untagged 'res' pointer. The
+	 * function that called mem_read() will want to check that the
+	 * initial value of 'res' hasn't changed if a tag check fault
+	 * is reported. Skip over two instructions so 'res' isn't
+	 * overwritten.
+	 */
+	regs->pc += 8;
+}
+
+static inline void mmu_set_tagged(pgd_t *pgtable, unsigned long vaddr)
+{
+	pteval_t *p_pte = follow_pte(pgtable, vaddr);
+
+	if (p_pte) {
+		pteval_t entry = *p_pte;
+
+		entry &= ~PTE_ATTRINDX_MASK;
+		entry |= PTE_ATTRINDX(MT_NORMAL_TAGGED);
+
+		WRITE_ONCE(*p_pte, entry);
+		flush_tlb_page(vaddr);
+	} else {
+		report_abort("Cannot find PTE");
+	}
+}
+
+static void mte_init(void)
+{
+	unsigned long sctlr = read_sysreg(sctlr_el1);
+	unsigned long tcr = read_sysreg(tcr_el1);
+
+	sctlr &= ~(SCTLR_EL1_TCF_MASK | SCTLR_EL1_TCF0_MASK);
+	sctlr &= ~SCTLR_EL1_ATA0;
+	sctlr |= SCTLR_EL1_ATA;
+
+	tcr |= TCR_TBI1 | TCR_TBI0;
+
+	write_sysreg(sctlr, sctlr_el1);
+	write_sysreg(tcr, tcr_el1);
+
+	isb();
+	flush_tlb_all();
+}
+
+static inline void mte_set_tcf(unsigned long tcf)
+{
+	unsigned long sctlr = read_sysreg(sctlr_el1);
+
+	sctlr &= ~(SCTLR_EL1_TCF_MASK | SCTLR_EL1_TCF0_MASK);
+	sctlr |= (tcf << SCTLR_EL1_TCF_SHIFT) & SCTLR_EL1_TCF_MASK ;
+	sctlr |= (tcf << SCTLR_EL1_TCF0_SHIFT) & SCTLR_EL1_TCF0_MASK ;
+
+	write_sysreg(sctlr, sctlr_el1);
+	isb();
+}
+
+
+static inline void mte_set_tag(void *addr, size_t size, unsigned int tag)
+{
+#ifdef CC_HAS_MTE
+	unsigned long in = (unsigned long)untagged(addr);
+	unsigned long start = ALIGN_DOWN(in, 16);
+	unsigned long end = ALIGN(in + size , 16);
+
+	for (unsigned long ptr = start; ptr < end; ptr += 16)
+		asm volatile(".arch   armv8.5-a+memtag\n"
+			     "stg %0, [%0]"
+			     :
+			     : "r"(tagged(ptr, tag))
+			     : "memory");
+#endif
+}
+
+static inline unsigned long get_clear_tfsr(void)
+{
+	unsigned long r;
+
+	dsb(nsh);
+	isb();
+
+	r = read_sysreg_s(TFSR_EL1);
+	write_sysreg_s(0, TFSR_EL1);
+
+	return r;
+}
+
+static void mte_sync_test(void)
+{
+	unsigned int *mem = alloc_page();
+	unsigned int val = 0;
+
+	memset(mem, 0xff, PAGE_SIZE);
+	mte_init();
+	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
+	mte_set_tag(mem, PAGE_SIZE, 0);
+	mte_set_tcf(MTE_TCF_SYNC);
+	mte_exception = false;
+
+	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
+
+	mem_read(mem, 1, &val);
+
+	report((val == 0) && mte_exception, "read");
+
+	mte_exception = false;
+
+	mem_write(mem, 2, 0xbbbbbbbb);
+
+	report((*mem == 0xffffffff) && mte_exception, "write");
+}
+
+static void mte_asymm_test(void)
+{
+	unsigned int *mem = alloc_page();
+	unsigned int val = 0;
+
+	memset(mem, 0xff, PAGE_SIZE);
+	mte_init();
+	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
+	mte_set_tag(mem, PAGE_SIZE, 0);
+	mte_set_tcf(MTE_TCF_ASYMM);
+	mte_exception = false;
+
+	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
+
+	mem_read(mem, 3, &val);
+	report((val == 0) && mte_exception, "read");
+
+	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, NULL);
+
+	mem_write(mem, 4, 0xaaaaaaaa);
+	report((*mem == 0xaaaaaaaa) && (get_clear_tfsr() == 1), "write");
+}
+
+static void mte_async_test(void)
+{
+	unsigned int *mem = alloc_page();
+	unsigned int val = 0;
+
+	memset(mem, 0xff, PAGE_SIZE);
+	mte_init();
+	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
+	mte_set_tag(mem, PAGE_SIZE, 0);
+	mte_set_tcf(MTE_TCF_ASYNC);
+
+	mem_read(mem, 5, &val);
+	report((val == 0xffffffff) && (get_clear_tfsr() == 1), "read");
+
+	mem_write(mem, 6, 0xcccccccc);
+	report((*mem == 0xcccccccc) && (get_clear_tfsr() == 1), "write");
+}
+
+
+static unsigned int mte_version(void)
+{
+#ifdef CC_HAS_MTE
+	uint64_t r;
+
+	asm volatile("mrs %x0, id_aa64pfr1_el1" : "=r"(r));
+
+	return (r >> 8) & 0b1111;
+#else
+	report_info("Compiler lack MTE support");
+	return 0;
+#endif
+}
+
+int main(int argc, char *argv[])
+{
+
+	unsigned int version = mte_version();
+
+	if (version < 2) {
+		report_skip("No MTE support, skip...\n");
+		return -1;
+	}
+
+	if (argc < 2)
+		report_abort("no test specified");
+
+	report_prefix_pushf("mte");
+
+	if (strcmp(argv[1], "sync") == 0) {
+		report_prefix_push(argv[1]);
+		mte_sync_test();
+		report_prefix_pop();
+	} else if (strcmp(argv[1], "async") == 0) {
+		report_prefix_push(argv[1]);
+		if (version < 3) {
+			report_skip("No MTE async, skip...\n");
+			return -1;
+		}
+		mte_async_test();
+		report_prefix_pop();
+
+	} else if (strcmp(argv[1], "asymm") == 0) {
+		report_prefix_push(argv[1]);
+		if (version < 3) {
+			report_skip("No MTE asymm, skip...\n");
+			return -1;
+		}
+		mte_asymm_test();
+		report_prefix_pop();
+
+	} else {
+		report_abort("Unknown sub-test '%s'", argv[1]);
+	}
+
+	return report_summary();
+}
diff --git a/arm/run b/arm/run
index efdd44ce..b129e4e0 100755
--- a/arm/run
+++ b/arm/run
@@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
 	exit 2
 fi
 
-M='-machine virt'
+MACHINE="virt"
+M="-machine $MACHINE$MACHINE_PROPS"
 
 if [ "$ACCEL" = "kvm" ]; then
 	if $qemu $M,\? | grep -q gic-version; then
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 2bdad67d..9b428c02 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -271,3 +271,22 @@ smp = 2
 groups = nodefault
 accel = kvm
 arch = arm64
+
+# MTE tests
+[mte-sync]
+file = mte.flat
+groups = nodefault
+extra_params = -append 'sync'
+arch = arm64
+
+[mte-async]
+file = mte.flat
+groups = nodefault
+extra_params = -append 'async'
+arch = arm64
+
+[mte-asymm]
+file = mte.flat
+groups = nodefault
+extra_params = -append 'asymm'
+arch = arm64
diff --git a/lib/arm64/asm/mmu.h b/lib/arm64/asm/mmu.h
index 5c27edb2..9aedd09a 100644
--- a/lib/arm64/asm/mmu.h
+++ b/lib/arm64/asm/mmu.h
@@ -10,6 +10,7 @@
 #define PMD_SECT_UNCACHED	PMD_ATTRINDX(MT_DEVICE_nGnRE)
 #define PTE_UNCACHED		PTE_ATTRINDX(MT_DEVICE_nGnRE)
 #define PTE_WBWA		PTE_ATTRINDX(MT_NORMAL)
+#define PTE_TAGGED		PTE_ATTRINDX(MT_NORMAL_TAGGED)
 
 static inline void flush_tlb_all(void)
 {
diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
index 8c41fe12..b66b62da 100644
--- a/lib/arm64/asm/pgtable-hwdef.h
+++ b/lib/arm64/asm/pgtable-hwdef.h
@@ -145,6 +145,7 @@
 #define TCR_TG1_64K		(UL(3) << 30)
 #define TCR_ASID16		(UL(1) << 36)
 #define TCR_TBI0		(UL(1) << 37)
+#define TCR_TBI1		(UL(1) << 38)
 
 /*
  * Memory types available.
@@ -156,5 +157,6 @@
 #define MT_NORMAL		4
 #define MT_NORMAL_WT		5
 #define MT_DEVICE_nGRE		6
+#define MT_NORMAL_TAGGED	7
 
 #endif /* _ASMARM64_PGTABLE_HWDEF_H_ */
diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
index f214a4f0..60f51dad 100644
--- a/lib/arm64/asm/sysreg.h
+++ b/lib/arm64/asm/sysreg.h
@@ -28,6 +28,7 @@
 	.endm
 #else
 #include <libcflat.h>
+#include <bitops.h>
 
 #define read_sysreg(r) ({					\
 	u64 __val;						\
@@ -81,7 +82,12 @@ asm(
 #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
 #define ICC_GRPEN1_EL1			sys_reg(3, 0, 12, 12, 7)
 
+#define TFSR_EL1			sys_reg(3, 0, 5, 6, 0)
+
+
 /* System Control Register (SCTLR_EL1) bits */
+#define SCTLR_EL1_ATA		_BITULL(43)
+#define SCTLR_EL1_ATA0		_BITULL(42)
 #define SCTLR_EL1_LSMAOE	_BITULL(29)
 #define SCTLR_EL1_NTLSMD	_BITULL(28)
 #define SCTLR_EL1_EE		_BITULL(25)
@@ -99,6 +105,12 @@ asm(
 #define SCTLR_EL1_A		_BITULL(1)
 #define SCTLR_EL1_M		_BITULL(0)
 
+#define SCTLR_EL1_TCF_SHIFT	40
+#define SCTLR_EL1_TCF_MASK	GENMASK_ULL(41, 40)
+
+#define SCTLR_EL1_TCF0_SHIFT	38
+#define SCTLR_EL1_TCF0_MASK	GENMASK_ULL(39, 38)
+
 #define INIT_SCTLR_EL1_MMU_OFF	\
 			(SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \
 			 SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \
-- 
2.25.1

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei]

Hi Vladimir,

The patch looks good, but it just occured to me, the tests do a great job
checking that tagged accesses fail when they should be failing, but they don't
check that taggedd accesses *succeed* when they should not be failing. I think
that's useful to have at the start of each test, if nothing just as a sanity
check.

On Thu, Dec 12, 2024 at 10:34:47AM +0000, Vladimir Murzin wrote:
> Test tag storage access and tag mismatch for different MTE modes.
> 
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
> 
>  v1 -> v2
>     - Addressed comments (I hope I did not miss any) from Alexandru
> 
>  arm/Makefile.arm64            |  10 +-
>  arm/cstart64.S                |   4 +-
>  arm/mte.c                     | 299 ++++++++++++++++++++++++++++++++++
>  arm/run                       |   3 +-
>  arm/unittests.cfg             |  19 +++
>  lib/arm64/asm/mmu.h           |   1 +
>  lib/arm64/asm/pgtable-hwdef.h |   2 +
>  lib/arm64/asm/sysreg.h        |  12 ++
>  8 files changed, 347 insertions(+), 3 deletions(-)
>  create mode 100644 arm/mte.c
> 
> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> index 3b9034e3..48dcdbd4 100644
> --- a/arm/Makefile.arm64
> +++ b/arm/Makefile.arm64
> @@ -17,10 +17,17 @@ ifneq ($(strip $(sve_flag)),)
>  CFLAGS += -DCC_HAS_SVE
>  endif
>  
> +mte_flag := $(call cc-option, -march=armv8.5-a+memtag, "")
> +ifneq ($(strip $(mte_flag)),)
> +# MTE is supported by the compiler, generate MTE instructions
> +CFLAGS += -DCC_HAS_MTE
> +endif
> +
> +
>  mno_outline_atomics := $(call cc-option, -mno-outline-atomics, "")
>  CFLAGS += $(mno_outline_atomics)
>  CFLAGS += -DCONFIG_RELOC
> -CFLAGS += -mgeneral-regs-only
> +CFLAGS += -mgeneral-regs-only -save-temps

Hmm.. I can't seem to figure out why -save-temps is required for the MTE test.
From man gcc, -save-temps is a knob that tells gcc not to delete the
intermediate files that it generates. Am I missing something?

>  
>  define arch_elf_check =
>  	$(if $(shell ! $(READELF) -rW $(1) >&/dev/null && echo "nok"),
> @@ -57,6 +64,7 @@ tests += $(TEST_DIR)/micro-bench.$(exe)
>  tests += $(TEST_DIR)/cache.$(exe)
>  tests += $(TEST_DIR)/debug.$(exe)
>  tests += $(TEST_DIR)/fpu.$(exe)
> +tests += $(TEST_DIR)/mte.$(exe)
>  
>  include $(SRCDIR)/$(TEST_DIR)/Makefile.common
>  
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index b480a552..b9d7a446 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -242,6 +242,7 @@ halt:
>   *   NORMAL             100     11111111
>   *   NORMAL_WT          101     10111011
>   *   DEVICE_nGRE        110     00001000
> + *   NORMAL_TAGGED      111     11110000
>   */
>  #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>  
> @@ -275,7 +276,8 @@ asm_mmu_enable:
>  		     MAIR(0x44, MT_NORMAL_NC) |		\
>  		     MAIR(0xff, MT_NORMAL) |	        \
>  		     MAIR(0xbb, MT_NORMAL_WT) |         \
> -		     MAIR(0x08, MT_DEVICE_nGRE)
> +		     MAIR(0x08, MT_DEVICE_nGRE) |       \
> +		     MAIR(0xf0, MT_NORMAL_TAGGED)
>  	msr	mair_el1, x1
>  
>  	/* TTBR0 */
> diff --git a/arm/mte.c b/arm/mte.c
> new file mode 100644
> index 00000000..718f0a61
> --- /dev/null
> +++ b/arm/mte.c
> @@ -0,0 +1,299 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Arm Limited.
> + * All rights reserved.
> + */
> +
> +#include <libcflat.h>
> +#include <alloc_page.h>
> +#include <stdlib.h>
> +#include <asm/mmu.h>
> +#include <asm/sysreg.h>
> +#include <asm/pgtable-hwdef.h>
> +#include <asm/processor.h>
> +#include <asm/thread_info.h>
> +
> +
> +/* Tag Check Faults cause a synchronous exception */
> +#define MTE_TCF_SYNC	0b01
> +/* Tag Check Faults are asynchronously accumulated */
> +#define MTE_TCF_ASYNC	0b10
> +/*
> + * Tag Check Faults cause a synchronous exception on reads,
> + * and are asynchronously accumulated on writes
> + */
> +#define MTE_TCF_ASYMM	0b11
> +
> +#define MTE_GRANULE_SIZE        UL(16)
> +#define MTE_GRANULE_MASK        (~(MTE_GRANULE_SIZE - 1))
> +#define MTE_TAG_SHIFT           56
> +
> +#define untagged(p)									\
> +({											\
> +	unsigned long __in = (unsigned long)(p);					\
> +	typeof(p) __out = (typeof(p))(__in & ~(MTE_GRANULE_MASK << MTE_TAG_SHIFT));	\
> +											\
> +	__out;										\
> +})
> +
> +#define tagged(p,t)							\
> +({									\
> +	unsigned long __in = (unsigned long)(untagged(p));		\
> +	unsigned long __tag = (unsigned long)(t) << MTE_TAG_SHIFT;	\
> +	typeof(p) __out = (typeof(p))(__in | __tag);			\
> +									\
> +	__out;								\
> +})
> +
> +/*
> + * If we use a normal (non hand coded inline assembly) load or store
> + * to access a tagged address, the compiler will reasonably assume
> + * that the access succeeded, and the next instruction may do
> + * something based on that assumption.
> + *
> + * But a test might want the tagged access to fail on purpose, and if
> + * we advance the PC to the next instruction, the one added by the
> + * compiler, we might leave the program in an unexpected state.
> + */
> +static inline void mem_read(unsigned int *addr, unsigned int tag, unsigned int *res)
> +{
> +	unsigned int r;
> +
> +	asm volatile ("ldr %0,[%1]\n"
> +		      "str %0,[%2]\n"
> +		      : "=r" (r)
> +		      : "r" (tagged(addr, tag)), "r" (res));
> +}
> +
> +static inline void mem_write(unsigned int *addr, unsigned int tag, unsigned int val)
> +{
> +	/* The NOP allows the same exception handler as mem_read() to be used. */
> +	asm volatile ("str %0,[%1]\n"
> +		      "nop\n"
> +		      :
> +		      : "r" (val), "r" (tagged(addr, tag))
> +		      : "memory");
> +}
> +
> +static volatile bool mte_exception;
> +
> +static void mte_fault_handler(struct pt_regs *regs, unsigned int esr)
> +{
> +	unsigned int dfsc = esr & GENMASK(5, 0);
> +	unsigned int fnv = esr & BIT(10);
> +
> +	if ((dfsc == 0b010001) && (fnv == 0))
> +		mte_exception = true;
> +
> +	/*
> +	 * mem_read() reads the value from the tagged pointer, then
> +	 * stores this value in the untagged 'res' pointer. The
> +	 * function that called mem_read() will want to check that the
> +	 * initial value of 'res' hasn't changed if a tag check fault
> +	 * is reported. Skip over two instructions so 'res' isn't
> +	 * overwritten.
> +	 */
> +	regs->pc += 8;
> +}
> +
> +static inline void mmu_set_tagged(pgd_t *pgtable, unsigned long vaddr)
> +{
> +	pteval_t *p_pte = follow_pte(pgtable, vaddr);
> +
> +	if (p_pte) {
> +		pteval_t entry = *p_pte;
> +
> +		entry &= ~PTE_ATTRINDX_MASK;
> +		entry |= PTE_ATTRINDX(MT_NORMAL_TAGGED);
> +
> +		WRITE_ONCE(*p_pte, entry);
> +		flush_tlb_page(vaddr);
> +	} else {
> +		report_abort("Cannot find PTE");
> +	}
> +}
> +
> +static void mte_init(void)
> +{
> +	unsigned long sctlr = read_sysreg(sctlr_el1);
> +	unsigned long tcr = read_sysreg(tcr_el1);
> +
> +	sctlr &= ~(SCTLR_EL1_TCF_MASK | SCTLR_EL1_TCF0_MASK);
> +	sctlr &= ~SCTLR_EL1_ATA0;
> +	sctlr |= SCTLR_EL1_ATA;
> +
> +	tcr |= TCR_TBI1 | TCR_TBI0;
> +
> +	write_sysreg(sctlr, sctlr_el1);
> +	write_sysreg(tcr, tcr_el1);
> +
> +	isb();
> +	flush_tlb_all();
> +}
> +
> +static inline void mte_set_tcf(unsigned long tcf)
> +{
> +	unsigned long sctlr = read_sysreg(sctlr_el1);
> +
> +	sctlr &= ~(SCTLR_EL1_TCF_MASK | SCTLR_EL1_TCF0_MASK);
> +	sctlr |= (tcf << SCTLR_EL1_TCF_SHIFT) & SCTLR_EL1_TCF_MASK ;
> +	sctlr |= (tcf << SCTLR_EL1_TCF0_SHIFT) & SCTLR_EL1_TCF0_MASK ;
> +
> +	write_sysreg(sctlr, sctlr_el1);
> +	isb();
> +}
> +
> +
> +static inline void mte_set_tag(void *addr, size_t size, unsigned int tag)
> +{
> +#ifdef CC_HAS_MTE
> +	unsigned long in = (unsigned long)untagged(addr);
> +	unsigned long start = ALIGN_DOWN(in, 16);
> +	unsigned long end = ALIGN(in + size , 16);
> +
> +	for (unsigned long ptr = start; ptr < end; ptr += 16)
> +		asm volatile(".arch   armv8.5-a+memtag\n"
> +			     "stg %0, [%0]"
> +			     :
> +			     : "r"(tagged(ptr, tag))
> +			     : "memory");
> +#endif
> +}
> +
> +static inline unsigned long get_clear_tfsr(void)
> +{
> +	unsigned long r;
> +
> +	dsb(nsh);
> +	isb();
> +
> +	r = read_sysreg_s(TFSR_EL1);
> +	write_sysreg_s(0, TFSR_EL1);
> +
> +	return r;
> +}
> +
> +static void mte_sync_test(void)
> +{
> +	unsigned int *mem = alloc_page();
> +	unsigned int val = 0;
> +
> +	memset(mem, 0xff, PAGE_SIZE);
> +	mte_init();
> +	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
> +	mte_set_tag(mem, PAGE_SIZE, 0);
> +	mte_set_tcf(MTE_TCF_SYNC);
> +	mte_exception = false;
> +
> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
> +
> +	mem_read(mem, 1, &val);

When I came back to the patch, I read this and I thought that the value 1
represents what the value of 'val' should be. Do you think the code would be
easier to read if mem_read() took a tagged address directly, i.e:

	mem_read(tagged(mem, 1), &val);

Up to you what you prefer.

> +
> +	report((val == 0) && mte_exception, "read");
> +
> +	mte_exception = false;
> +
> +	mem_write(mem, 2, 0xbbbbbbbb);
> +
> +	report((*mem == 0xffffffff) && mte_exception, "write");
> +}
> +
> +static void mte_asymm_test(void)
> +{
> +	unsigned int *mem = alloc_page();
> +	unsigned int val = 0;
> +
> +	memset(mem, 0xff, PAGE_SIZE);
> +	mte_init();
> +	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
> +	mte_set_tag(mem, PAGE_SIZE, 0);
> +	mte_set_tcf(MTE_TCF_ASYMM);
> +	mte_exception = false;
> +
> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
> +
> +	mem_read(mem, 3, &val);
> +	report((val == 0) && mte_exception, "read");
> +
> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, NULL);
> +
> +	mem_write(mem, 4, 0xaaaaaaaa);
> +	report((*mem == 0xaaaaaaaa) && (get_clear_tfsr() == 1), "write");

Do you think it would be easier to understand the code if TFSR_EL1_TF0 would be
used here instead of 1? Up to you what you prefer, but if you do decide to use
a define, please also add TFSR_EL1_TF1, just in case someone decides to expand
the test some day with an address in the upper virtual address space.

> +}
> +
> +static void mte_async_test(void)
> +{
> +	unsigned int *mem = alloc_page();
> +	unsigned int val = 0;
> +
> +	memset(mem, 0xff, PAGE_SIZE);
> +	mte_init();
> +	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
> +	mte_set_tag(mem, PAGE_SIZE, 0);
> +	mte_set_tcf(MTE_TCF_ASYNC);
> +
> +	mem_read(mem, 5, &val);
> +	report((val == 0xffffffff) && (get_clear_tfsr() == 1), "read");
> +
> +	mem_write(mem, 6, 0xcccccccc);
> +	report((*mem == 0xcccccccc) && (get_clear_tfsr() == 1), "write");
> +}
> +
> +
> +static unsigned int mte_version(void)
> +{
> +#ifdef CC_HAS_MTE
> +	uint64_t r;
> +
> +	asm volatile("mrs %x0, id_aa64pfr1_el1" : "=r"(r));
> +
> +	return (r >> 8) & 0b1111;
> +#else
> +	report_info("Compiler lack MTE support");
> +	return 0;
> +#endif
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +
> +	unsigned int version = mte_version();
> +
> +	if (version < 2) {
> +		report_skip("No MTE support, skip...\n");
> +		return -1;

Can you change the return to:

		return report_summary();

Explanation below.

> +	}
> +
> +	if (argc < 2)
> +		report_abort("no test specified");
> +
> +	report_prefix_pushf("mte");

report_prefix_push() (without the 'f' at the end).

> +
> +	if (strcmp(argv[1], "sync") == 0) {
> +		report_prefix_push(argv[1]);
> +		mte_sync_test();
> +		report_prefix_pop();
> +	} else if (strcmp(argv[1], "async") == 0) {
> +		report_prefix_push(argv[1]);
> +		if (version < 3) {
> +			report_skip("No MTE async, skip...\n");
> +			return -1;
> +		}
> +		mte_async_test();
> +		report_prefix_pop();
> +
> +	} else if (strcmp(argv[1], "asymm") == 0) {
> +		report_prefix_push(argv[1]);
> +		if (version < 3) {
> +			report_skip("No MTE asymm, skip...\n");
> +			return -1;
> +		}
> +		mte_asymm_test();
> +		report_prefix_pop();
> +
> +	} else {
> +		report_abort("Unknown sub-test '%s'", argv[1]);
> +	}
> +
> +	return report_summary();
> +}
> diff --git a/arm/run b/arm/run
> index efdd44ce..b129e4e0 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
>  	exit 2
>  fi
>  
> -M='-machine virt'
> +MACHINE="virt"
> +M="-machine $MACHINE$MACHINE_PROPS"

Sorry, but I still don't understand what $MACHINE_PROPS does, I can't seem to
find where it's initialized :(

>  
>  if [ "$ACCEL" = "kvm" ]; then
>  	if $qemu $M,\? | grep -q gic-version; then
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 2bdad67d..9b428c02 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -271,3 +271,22 @@ smp = 2
>  groups = nodefault
>  accel = kvm
>  arch = arm64
> +
> +# MTE tests
> +[mte-sync]
> +file = mte.flat
> +groups = nodefault
> +extra_params = -append 'sync'
> +arch = arm64
> +
> +[mte-async]
> +file = mte.flat
> +groups = nodefault
> +extra_params = -append 'async'
> +arch = arm64
> +
> +[mte-asymm]
> +file = mte.flat
> +groups = nodefault
> +extra_params = -append 'asymm'
> +arch = arm64

I think it would be better if the tests are not in the nodefault group. As far
as I can tell, they are in the nondefault group because not all hardware and KVM
versions support MTE (please correct me if I'm wrong).

If the tests are in the nodefault group, then a CI administrator must
keep track of those machine that have MTE and manually run them, otherwise they
will never get run, even if the machines support it.

If you remove them from the nondefault group, with the change I proposed to
main() the test runner will not mark them as failed on systems that don't
support MTE:

$ ./run_tests.sh
[..]
SKIP mte-sync (1 tests, 1 skipped)
SKIP mte-async (1 tests, 1 skipped)
SKIP mte-asymm (1 tests, 1 skipped)

but they will get automatically run on systems that support MTE.

Also, would you mind putting the tests in the mte group:

	groups = mte

so they can be run with ./run_tests.sh -g mte.

Thanks,
Alex

> diff --git a/lib/arm64/asm/mmu.h b/lib/arm64/asm/mmu.h
> index 5c27edb2..9aedd09a 100644
> --- a/lib/arm64/asm/mmu.h
> +++ b/lib/arm64/asm/mmu.h
> @@ -10,6 +10,7 @@
>  #define PMD_SECT_UNCACHED	PMD_ATTRINDX(MT_DEVICE_nGnRE)
>  #define PTE_UNCACHED		PTE_ATTRINDX(MT_DEVICE_nGnRE)
>  #define PTE_WBWA		PTE_ATTRINDX(MT_NORMAL)
> +#define PTE_TAGGED		PTE_ATTRINDX(MT_NORMAL_TAGGED)
>  
>  static inline void flush_tlb_all(void)
>  {
> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
> index 8c41fe12..b66b62da 100644
> --- a/lib/arm64/asm/pgtable-hwdef.h
> +++ b/lib/arm64/asm/pgtable-hwdef.h
> @@ -145,6 +145,7 @@
>  #define TCR_TG1_64K		(UL(3) << 30)
>  #define TCR_ASID16		(UL(1) << 36)
>  #define TCR_TBI0		(UL(1) << 37)
> +#define TCR_TBI1		(UL(1) << 38)
>  
>  /*
>   * Memory types available.
> @@ -156,5 +157,6 @@
>  #define MT_NORMAL		4
>  #define MT_NORMAL_WT		5
>  #define MT_DEVICE_nGRE		6
> +#define MT_NORMAL_TAGGED	7
>  
>  #endif /* _ASMARM64_PGTABLE_HWDEF_H_ */
> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
> index f214a4f0..60f51dad 100644
> --- a/lib/arm64/asm/sysreg.h
> +++ b/lib/arm64/asm/sysreg.h
> @@ -28,6 +28,7 @@
>  	.endm
>  #else
>  #include <libcflat.h>
> +#include <bitops.h>
>  
>  #define read_sysreg(r) ({					\
>  	u64 __val;						\
> @@ -81,7 +82,12 @@ asm(
>  #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
>  #define ICC_GRPEN1_EL1			sys_reg(3, 0, 12, 12, 7)
>  
> +#define TFSR_EL1			sys_reg(3, 0, 5, 6, 0)
> +
> +
>  /* System Control Register (SCTLR_EL1) bits */
> +#define SCTLR_EL1_ATA		_BITULL(43)
> +#define SCTLR_EL1_ATA0		_BITULL(42)
>  #define SCTLR_EL1_LSMAOE	_BITULL(29)
>  #define SCTLR_EL1_NTLSMD	_BITULL(28)
>  #define SCTLR_EL1_EE		_BITULL(25)
> @@ -99,6 +105,12 @@ asm(
>  #define SCTLR_EL1_A		_BITULL(1)
>  #define SCTLR_EL1_M		_BITULL(0)
>  
> +#define SCTLR_EL1_TCF_SHIFT	40
> +#define SCTLR_EL1_TCF_MASK	GENMASK_ULL(41, 40)
> +
> +#define SCTLR_EL1_TCF0_SHIFT	38
> +#define SCTLR_EL1_TCF0_MASK	GENMASK_ULL(39, 38)
> +
>  #define INIT_SCTLR_EL1_MMU_OFF	\
>  			(SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \
>  			 SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \
> -- 
> 2.25.1
> 

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Vladimir Murzin]

Hi Alexandru,

On 12/23/24 12:03, Alexandru Elisei wrote:
> Hi Vladimir,
> 
> The patch looks good, but it just occured to me, the tests do a great job
> checking that tagged accesses fail when they should be failing, but they don't
> check that taggedd accesses *succeed* when they should not be failing. I think
> that's useful to have at the start of each test, if nothing just as a sanity
> check.
> 

Well, there are successful tagged access, yet with zero tag :) We can move to
non-zero tag, something like

static inline void mte_memset(void *addr, int val, size_t size)
{
	unsigned long old = mte_set_tcf(MTE_TCF_SYNC);
	memset(addr, val, size);
	mte_set_tcf(old);
}

...

static void mte_async_test(void)
{
	unsigned int *mem = tagged(alloc_page(), 3);
	unsigned int val = 0;

	mte_init();
	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
	mte_set_tag(mem, PAGE_SIZE, 3);
	mte_memset(mem, 0xff, PAGE_SIZE);
	mte_set_tcf(MTE_TCF_ASYNC);
...

<snip>

>> -CFLAGS += -mgeneral-regs-only
>> +CFLAGS += -mgeneral-regs-only -save-temps
> Hmm.. I can't seem to figure out why -save-temps is required for the MTE test.
>>From man gcc, -save-temps is a knob that tells gcc not to delete the
> intermediate files that it generates. Am I missing something?

No, it is not required and just leftover from my other debug...

<snip>

>> +
>> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
>> +
>> +	mem_read(mem, 1, &val);
> When I came back to the patch, I read this and I thought that the value 1
> represents what the value of 'val' should be. Do you think the code would be
> easier to read if mem_read() took a tagged address directly, i.e:
> 
> 	mem_read(tagged(mem, 1), &val);
> 
> Up to you what you prefer.
> 

Ack.

<snip>

>> +	report((*mem == 0xaaaaaaaa) && (get_clear_tfsr() == 1), "write");
> Do you think it would be easier to understand the code if TFSR_EL1_TF0 would be
> used here instead of 1? Up to you what you prefer, but if you do decide to use
> a define, please also add TFSR_EL1_TF1, just in case someone decides to expand
> the test some day with an address in the upper virtual address space.

Ack.

<snip>
>> +int main(int argc, char *argv[])
>> +{
>> +
>> +	unsigned int version = mte_version();
>> +
>> +	if (version < 2) {
>> +		report_skip("No MTE support, skip...\n");
>> +		return -1;
> Can you change the return to:
> 
> 		return report_summary();
> 
> Explanation below.

Ack.

> 
>> +	}
>> +
>> +	if (argc < 2)
>> +		report_abort("no test specified");
>> +
>> +	report_prefix_pushf("mte");
> report_prefix_push() (without the 'f' at the end).
> 

Ack.

<snip>

>> +}
>> diff --git a/arm/run b/arm/run
>> index efdd44ce..b129e4e0 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
>>  	exit 2
>>  fi
>>  
>> -M='-machine virt'
>> +MACHINE="virt"
>> +M="-machine $MACHINE$MACHINE_PROPS"
> Sorry, but I still don't understand what $MACHINE_PROPS does, I can't seem to
> find where it's initialized :(
> 

I'm not sure what you are looking/asking for :( 

My last reply [1] demonstrates usage for $MACHINE_PROPS

QEMU=qemu-system-aarch64 ACCEL=tcg MACHINE_PROPS=",mte=on" arm/run arm/mte.flat -append "sync"

>>  
>>  if [ "$ACCEL" = "kvm" ]; then
>>  	if $qemu $M,\? | grep -q gic-version; then
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index 2bdad67d..9b428c02 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -271,3 +271,22 @@ smp = 2
>>  groups = nodefault
>>  accel = kvm
>>  arch = arm64
>> +
>> +# MTE tests
>> +[mte-sync]
>> +file = mte.flat
>> +groups = nodefault
>> +extra_params = -append 'sync'
>> +arch = arm64
>> +
>> +[mte-async]
>> +file = mte.flat
>> +groups = nodefault
>> +extra_params = -append 'async'
>> +arch = arm64
>> +
>> +[mte-asymm]
>> +file = mte.flat
>> +groups = nodefault
>> +extra_params = -append 'asymm'
>> +arch = arm64
> I think it would be better if the tests are not in the nodefault group. As far
> as I can tell, they are in the nondefault group because not all hardware and KVM
> versions support MTE (please correct me if I'm wrong).
> 
> If the tests are in the nodefault group, then a CI administrator must
> keep track of those machine that have MTE and manually run them, otherwise they
> will never get run, even if the machines support it.
> 
> If you remove them from the nondefault group, with the change I proposed to
> main() the test runner will not mark them as failed on systems that don't
> support MTE:
> 
> $ ./run_tests.sh
> [..]
> SKIP mte-sync (1 tests, 1 skipped)
> SKIP mte-async (1 tests, 1 skipped)
> SKIP mte-asymm (1 tests, 1 skipped)
> 
> but they will get automatically run on systems that support MTE.
> 

Looks really neat!

> Also, would you mind putting the tests in the mte group:
> 
> 	groups = mte
> 
> so they can be run with ./run_tests.sh -g mte.
> 

Ack.

[1] https://lore.kernel.org/all/c2d9b61b-7dce-422b-8a3c-898f1003c9e4@arm.com/T/#t

Cheers
Vladimir

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei]

Hi Vladimir,

On Mon, Dec 23, 2024 at 02:37:40PM +0000, Vladimir Murzin wrote:
> Hi Alexandru,
> 
> On 12/23/24 12:03, Alexandru Elisei wrote:
> > Hi Vladimir,
> > 
> > The patch looks good, but it just occured to me, the tests do a great job
> > checking that tagged accesses fail when they should be failing, but they don't
> > check that taggedd accesses *succeed* when they should not be failing. I think
> > that's useful to have at the start of each test, if nothing just as a sanity
> > check.
> > 
> 
> Well, there are successful tagged access, yet with zero tag :) We can move to
> non-zero tag, something like
> 
> static inline void mte_memset(void *addr, int val, size_t size)
> {
> 	unsigned long old = mte_set_tcf(MTE_TCF_SYNC);
> 	memset(addr, val, size);
> 	mte_set_tcf(old);
> }
> 
> ...
> 
> static void mte_async_test(void)
> {
> 	unsigned int *mem = tagged(alloc_page(), 3);
> 	unsigned int val = 0;
> 
> 	mte_init();
> 	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
> 	mte_set_tag(mem, PAGE_SIZE, 3);
> 	mte_memset(mem, 0xff, PAGE_SIZE);
> 	mte_set_tcf(MTE_TCF_ASYNC);
> ...

I have a few ideas here:

* Tag checked accesses when the address bits [59:55] are 0 are also controlled
with TCR_EL1.TCMA0, and the bit is set to 0 in asm_mmu_enabled() - this is what
we want. But if you want to explicitely check tagged accesses with the zero tag
I would suggest that the bit is explicitely cleared first (probably in
mte_init()), to make sure the tests starts with a known good configuration (very
useful for debugging!) and to avoid surprises if something changes in the
startup sequence of a test.

* All tag checked accesses should probably happen after setting SCTLR_EL1.TCF,
since the field is set to 0b00 in cstart64.S (when SCTLR_EL1 is set to
INIT_SCTLR_EL1_MMU_OFF).

I don't have a preference, up to you to decide (you can even go so far as to
check that TCR_EL1.TCMA0 works as expected if you feel it's useful), I was just
pointing out some of the choices you have.

[..]
> >> +}
> >> diff --git a/arm/run b/arm/run
> >> index efdd44ce..b129e4e0 100755
> >> --- a/arm/run
> >> +++ b/arm/run
> >> @@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
> >>  	exit 2
> >>  fi
> >>  
> >> -M='-machine virt'
> >> +MACHINE="virt"
> >> +M="-machine $MACHINE$MACHINE_PROPS"
> > Sorry, but I still don't understand what $MACHINE_PROPS does, I can't seem to
> > find where it's initialized :(
> > 
> 
> I'm not sure what you are looking/asking for :( 
> 
> My last reply [1] demonstrates usage for $MACHINE_PROPS
> 
> QEMU=qemu-system-aarch64 ACCEL=tcg MACHINE_PROPS=",mte=on" arm/run arm/mte.flat -append "sync"

Sorry about that, I must have missed your reply :( Adding MACHINE_PROPS is a
nifty idea, but I don't think it's good practice to depend on that to be able to
run a test. ./run_tests.sh <testname> should work without user
intervention/configuration.

For your particular example, you don't need to add a new environment
variable if you want to run the test with arm/run, you can do this instead:

QEMU=/path/to/qemu ACCEL=tcg arm/run arm/mte.flat -machine mte=on -append "sync"

arm/run passes all the parameters as they are to qemu.

In general, you can have the machine type in the unit test definition in
arm/unitests.cfg, in the 'extra_params' field, just like for the gicv2 and gicv3
tests. In theory, there's a separate 'machine' field in the test definition (see
docs/unittests.txt), but arm/arm64 ignores it.

This is what I had in mind for the test definition:

diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 9b428c02dabc..fe1011454f88 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -275,18 +275,18 @@ arch = arm64
 # MTE tests
 [mte-sync]
 file = mte.flat
-groups = nodefault
-extra_params = -append 'sync'
+groups = mte
+extra_params = -machine mte=on -append 'sync'
 arch = arm64

 [mte-async]
 file = mte.flat
-groups = nodefault
-extra_params = -append 'async'
+groups = mte
+extra_params = -machine mte=on -append 'async'
 arch = arm64

 [mte-asymm]
 file = mte.flat
-groups = nodefault
-extra_params = -append 'asymm'
+groups = mte
+extra_params = -machine mte=on -append 'asymm'
 arch = arm64

Interestingly though, when I try to run a test (either with ./run_tests.sh
mte-sync, or by trying out your example), I get this error:

qemu-system-aarch64: MTE requested, but not supported by the guest CPU

My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
model CPU. Changing the cpu to 'max' makes the test run. How are you getting
around this on your end?

Thanks,
Alex

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Andrew Jones]

On Mon, Dec 30, 2024 at 03:19:48PM +0000, Alexandru Elisei wrote:
...
> Interestingly though, when I try to run a test (either with ./run_tests.sh
> mte-sync, or by trying out your example), I get this error:
> 
> qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> 
> My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> around this on your end?
>

We can (and I think should) change to 'max' for the default model.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei]

Hi Drew,

On Mon, Dec 30, 2024 at 04:45:01PM +0100, Andrew Jones wrote:
> On Mon, Dec 30, 2024 at 03:19:48PM +0000, Alexandru Elisei wrote:
> ...
> > Interestingly though, when I try to run a test (either with ./run_tests.sh
> > mte-sync, or by trying out your example), I get this error:
> > 
> > qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> > 
> > My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> > model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> > around this on your end?
> >
> 
> We can (and I think should) change to 'max' for the default model.

I think so too, something like this maybe?

diff --git a/configure b/configure
index 86cf1da36467..4babbd5f5e74 100755
--- a/configure
+++ b/configure
@@ -286,7 +286,7 @@ fi
 [ -z "$processor" ] && processor="$arch"

 if [ "$processor" = "arm64" ]; then
-    processor="cortex-a57"
+    processor="max"
 elif [ "$processor" = "arm" ]; then
     processor="cortex-a15"
 fi

Can't do the same for arm, because Makefile.arm passes the processor to
-mcpu and that causes a build error.


Thanks,
Alex

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Andrew Jones]

On Mon, Dec 30, 2024 at 04:28:30PM +0000, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Mon, Dec 30, 2024 at 04:45:01PM +0100, Andrew Jones wrote:
> > On Mon, Dec 30, 2024 at 03:19:48PM +0000, Alexandru Elisei wrote:
> > ...
> > > Interestingly though, when I try to run a test (either with ./run_tests.sh
> > > mte-sync, or by trying out your example), I get this error:
> > > 
> > > qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> > > 
> > > My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> > > model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> > > around this on your end?
> > >
> > 
> > We can (and I think should) change to 'max' for the default model.
> 
> I think so too, something like this maybe?
> 
> diff --git a/configure b/configure
> index 86cf1da36467..4babbd5f5e74 100755
> --- a/configure
> +++ b/configure
> @@ -286,7 +286,7 @@ fi
>  [ -z "$processor" ] && processor="$arch"
> 
>  if [ "$processor" = "arm64" ]; then
> -    processor="cortex-a57"
> +    processor="max"
>  elif [ "$processor" = "arm" ]; then
>      processor="cortex-a15"
>  fi
> 
> Can't do the same for arm, because Makefile.arm passes the processor to
> -mcpu and that causes a build error.

I wonder if we can improve the mcpu thing by just selecting some generic
cpu type at this point? Or just dropping it and counting on the compiler
to use generic cpu type? It'd be nice to use max for both arm and arm64,
but if nobody has time to do the arm work / testing to be sure we can,
then I'm fine with just a patch changing arm64.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei]

Hi Drew,

On Mon, Dec 30, 2024 at 05:52:54PM +0100, Andrew Jones wrote:
> On Mon, Dec 30, 2024 at 04:28:30PM +0000, Alexandru Elisei wrote:
> > Hi Drew,
> > 
> > On Mon, Dec 30, 2024 at 04:45:01PM +0100, Andrew Jones wrote:
> > > On Mon, Dec 30, 2024 at 03:19:48PM +0000, Alexandru Elisei wrote:
> > > ...
> > > > Interestingly though, when I try to run a test (either with ./run_tests.sh
> > > > mte-sync, or by trying out your example), I get this error:
> > > > 
> > > > qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> > > > 
> > > > My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> > > > model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> > > > around this on your end?
> > > >
> > > 
> > > We can (and I think should) change to 'max' for the default model.
> > 
> > I think so too, something like this maybe?
> > 
> > diff --git a/configure b/configure
> > index 86cf1da36467..4babbd5f5e74 100755
> > --- a/configure
> > +++ b/configure
> > @@ -286,7 +286,7 @@ fi
> >  [ -z "$processor" ] && processor="$arch"
> > 
> >  if [ "$processor" = "arm64" ]; then
> > -    processor="cortex-a57"
> > +    processor="max"
> >  elif [ "$processor" = "arm" ]; then
> >      processor="cortex-a15"
> >  fi
> > 
> > Can't do the same for arm, because Makefile.arm passes the processor to
> > -mcpu and that causes a build error.
> 
> I wonder if we can improve the mcpu thing by just selecting some generic
> cpu type at this point? Or just dropping it and counting on the compiler
> to use generic cpu type? It'd be nice to use max for both arm and arm64,

I tried setting the default to 'max' for both arm and arm64. If I pass
'max' as the cpu type to arm, I get the build error above. If I don't pass
-mcpu to the arm compiler when $PROCESSOR=max, I get different errors
because of unsupported instructions:

/tmp/ccOHIrg7.s: Assembler messages:
/tmp/ccOHIrg7.s:46: Error: selected processor does not support `yield' in ARM mode
/tmp/ccGCB0zP.s: Assembler messages:
/tmp/ccGCB0zP.s:55: Error: selected processor does not support `ldrex r2,[r4]' in ARM mode
/tmp/ccGCB0zP.s:59: Error: selected processor does not support `strex r3,r2,[r4]' in ARM mode
/tmp/ccGCB0zP.s:71: Error: selected processor does not support `dmb ish' in ARM mode
/tmp/ccGCB0zP.s:99: Error: selected processor does not support `dmb ish' in ARM mode
/tmp/ccGCB0zP.s:144: Error: selected processor does not support `dmb ish' in ARM mode
[..]

and the errors go on. This means that the default cannot be 'max' for arm.

I also realized that the configure --processor option for arm64:

   --processor=PROCESSOR  processor to compile for (aarch64)

does not work as advertised, because the $PROCESSOR variable is not passed
down to the compiler.

So what I'm proposing is this:

- Patch #1: pass $PROCESSOR to the arm64 compiler, to reconcile the help
  text for --processor and to achieve parity with arm.

- Patch #2: have arm64 ignore $PROCESSOR when compiling if it's 'max.  By
  ignore I mean don't add a -mcpu argument to the compiler invocation.
  Change configure to raise an error if --processor=max is used for arm
  (users might think that if it works for arm64, it also works for arm).

- Patch #3: change the default for arm64 to 'max', but don't touch the
  default for arm.

What do you think?

Thanks,
Alex

> but if nobody has time to do the arm work / testing to be sure we can,
> then I'm fine with just a patch changing arm64.
> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Andrew Jones]

On Thu, Jan 02, 2025 at 12:27:43PM +0000, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Mon, Dec 30, 2024 at 05:52:54PM +0100, Andrew Jones wrote:
> > On Mon, Dec 30, 2024 at 04:28:30PM +0000, Alexandru Elisei wrote:
> > > Hi Drew,
> > > 
> > > On Mon, Dec 30, 2024 at 04:45:01PM +0100, Andrew Jones wrote:
> > > > On Mon, Dec 30, 2024 at 03:19:48PM +0000, Alexandru Elisei wrote:
> > > > ...
> > > > > Interestingly though, when I try to run a test (either with ./run_tests.sh
> > > > > mte-sync, or by trying out your example), I get this error:
> > > > > 
> > > > > qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> > > > > 
> > > > > My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> > > > > model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> > > > > around this on your end?
> > > > >
> > > > 
> > > > We can (and I think should) change to 'max' for the default model.
> > > 
> > > I think so too, something like this maybe?
> > > 
> > > diff --git a/configure b/configure
> > > index 86cf1da36467..4babbd5f5e74 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -286,7 +286,7 @@ fi
> > >  [ -z "$processor" ] && processor="$arch"
> > > 
> > >  if [ "$processor" = "arm64" ]; then
> > > -    processor="cortex-a57"
> > > +    processor="max"
> > >  elif [ "$processor" = "arm" ]; then
> > >      processor="cortex-a15"
> > >  fi
> > > 
> > > Can't do the same for arm, because Makefile.arm passes the processor to
> > > -mcpu and that causes a build error.
> > 
> > I wonder if we can improve the mcpu thing by just selecting some generic
> > cpu type at this point? Or just dropping it and counting on the compiler
> > to use generic cpu type? It'd be nice to use max for both arm and arm64,
> 
> I tried setting the default to 'max' for both arm and arm64. If I pass
> 'max' as the cpu type to arm, I get the build error above. If I don't pass
> -mcpu to the arm compiler when $PROCESSOR=max, I get different errors
> because of unsupported instructions:
> 
> /tmp/ccOHIrg7.s: Assembler messages:
> /tmp/ccOHIrg7.s:46: Error: selected processor does not support `yield' in ARM mode
> /tmp/ccGCB0zP.s: Assembler messages:
> /tmp/ccGCB0zP.s:55: Error: selected processor does not support `ldrex r2,[r4]' in ARM mode
> /tmp/ccGCB0zP.s:59: Error: selected processor does not support `strex r3,r2,[r4]' in ARM mode
> /tmp/ccGCB0zP.s:71: Error: selected processor does not support `dmb ish' in ARM mode
> /tmp/ccGCB0zP.s:99: Error: selected processor does not support `dmb ish' in ARM mode
> /tmp/ccGCB0zP.s:144: Error: selected processor does not support `dmb ish' in ARM mode
> [..]
> 
> and the errors go on. This means that the default cannot be 'max' for arm.
> 
> I also realized that the configure --processor option for arm64:
> 
>    --processor=PROCESSOR  processor to compile for (aarch64)
> 
> does not work as advertised, because the $PROCESSOR variable is not passed
> down to the compiler.
> 
> So what I'm proposing is this:
> 
> - Patch #1: pass $PROCESSOR to the arm64 compiler, to reconcile the help
>   text for --processor and to achieve parity with arm.
> 
> - Patch #2: have arm64 ignore $PROCESSOR when compiling if it's 'max.  By
>   ignore I mean don't add a -mcpu argument to the compiler invocation.
>   Change configure to raise an error if --processor=max is used for arm
>   (users might think that if it works for arm64, it also works for arm).
> 
> - Patch #3: change the default for arm64 to 'max', but don't touch the
>   default for arm.
> 
> What do you think?

Works for me.

Thanks,
drew

> 
> Thanks,
> Alex
> 
> > but if nobody has time to do the arm work / testing to be sure we can,
> > then I'm fine with just a patch changing arm64.
> > 
> > Thanks,
> > drew

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Vladimir Murzin]

Hi Alexandru,

On 12/30/24 15:19, Alexandru Elisei wrote:
> Hi Vladimir,
> 
> On Mon, Dec 23, 2024 at 02:37:40PM +0000, Vladimir Murzin wrote:
>> Hi Alexandru,
>>
>> On 12/23/24 12:03, Alexandru Elisei wrote:
>>> Hi Vladimir,
>>>
>>> The patch looks good, but it just occured to me, the tests do a great job
>>> checking that tagged accesses fail when they should be failing, but they don't
>>> check that taggedd accesses *succeed* when they should not be failing. I think
>>> that's useful to have at the start of each test, if nothing just as a sanity
>>> check.
>>>
>> Well, there are successful tagged access, yet with zero tag :) We can move to
>> non-zero tag, something like
>>
>> static inline void mte_memset(void *addr, int val, size_t size)
>> {
>> 	unsigned long old = mte_set_tcf(MTE_TCF_SYNC);
>> 	memset(addr, val, size);
>> 	mte_set_tcf(old);
>> }
>>
>> ...
>>
>> static void mte_async_test(void)
>> {
>> 	unsigned int *mem = tagged(alloc_page(), 3);
>> 	unsigned int val = 0;
>>
>> 	mte_init();
>> 	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
>> 	mte_set_tag(mem, PAGE_SIZE, 3);
>> 	mte_memset(mem, 0xff, PAGE_SIZE);
>> 	mte_set_tcf(MTE_TCF_ASYNC);
>> ...
> I have a few ideas here:
> 
> * Tag checked accesses when the address bits [59:55] are 0 are also controlled
> with TCR_EL1.TCMA0, and the bit is set to 0 in asm_mmu_enabled() - this is what
> we want. But if you want to explicitely check tagged accesses with the zero tag
> I would suggest that the bit is explicitely cleared first (probably in
> mte_init()), to make sure the tests starts with a known good configuration (very
> useful for debugging!) and to avoid surprises if something changes in the
> startup sequence of a test.
> 

Good point!

> * All tag checked accesses should probably happen after setting SCTLR_EL1.TCF,
> since the field is set to 0b00 in cstart64.S (when SCTLR_EL1 is set to
> INIT_SCTLR_EL1_MMU_OFF).
> 
> I don't have a preference, up to you to decide (you can even go so far as to
> check that TCR_EL1.TCMA0 works as expected if you feel it's useful), I was just
> pointing out some of the choices you have.
> 

I probably go for non-zero tag now and leave other ideas for later...

> [..]
>>>> +}
>>>> diff --git a/arm/run b/arm/run
>>>> index efdd44ce..b129e4e0 100755
>>>> --- a/arm/run
>>>> +++ b/arm/run
>>>> @@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
>>>>  	exit 2
>>>>  fi
>>>>  
>>>> -M='-machine virt'
>>>> +MACHINE="virt"
>>>> +M="-machine $MACHINE$MACHINE_PROPS"
>>> Sorry, but I still don't understand what $MACHINE_PROPS does, I can't seem to
>>> find where it's initialized :(
>>>
>> I'm not sure what you are looking/asking for :( 
>>
>> My last reply [1] demonstrates usage for $MACHINE_PROPS
>>
>> QEMU=qemu-system-aarch64 ACCEL=tcg MACHINE_PROPS=",mte=on" arm/run arm/mte.flat -append "sync"
> Sorry about that, I must have missed your reply :( Adding MACHINE_PROPS is a
> nifty idea, but I don't think it's good practice to depend on that to be able to
> run a test. ./run_tests.sh <testname> should work without user
> intervention/configuration.
> 
> For your particular example, you don't need to add a new environment
> variable if you want to run the test with arm/run, you can do this instead:
> 
> QEMU=/path/to/qemu ACCEL=tcg arm/run arm/mte.flat -machine mte=on -append "sync"
> 
> arm/run passes all the parameters as they are to qemu.
> 
> In general, you can have the machine type in the unit test definition in
> arm/unitests.cfg, in the 'extra_params' field, just like for the gicv2 and gicv3
> tests. In theory, there's a separate 'machine' field in the test definition (see
> docs/unittests.txt), but arm/arm64 ignores it.
> 
> This is what I had in mind for the test definition:
> 
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 9b428c02dabc..fe1011454f88 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -275,18 +275,18 @@ arch = arm64
>  # MTE tests
>  [mte-sync]
>  file = mte.flat
> -groups = nodefault
> -extra_params = -append 'sync'
> +groups = mte
> +extra_params = -machine mte=on -append 'sync'
>  arch = arm64
> 
>  [mte-async]
>  file = mte.flat
> -groups = nodefault
> -extra_params = -append 'async'
> +groups = mte
> +extra_params = -machine mte=on -append 'async'
>  arch = arm64
> 
>  [mte-asymm]
>  file = mte.flat
> -groups = nodefault
> -extra_params = -append 'asymm'
> +groups = mte
> +extra_params = -machine mte=on -append 'asymm'
>  arch = arm64
> 

All that new to me! I followed $ACCEL_PROPS as an example, yet -machine makes
everything neat and easy, I'll drop $MACHINE_PROPS.

> Interestingly though, when I try to run a test (either with ./run_tests.sh
> mte-sync, or by trying out your example), I get this error:
> 
> qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> 
> My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> around this on your end?
> 

./configure --arch=arm64 --processor=max

Thanks
Vladimir

> Thanks,
> Alex
> 

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei]

Hi Vladimir,

On Thu, Jan 02, 2025 at 10:04:37AM +0000, Vladimir Murzin wrote:
[..]
> On 12/30/24 15:19, Alexandru Elisei wrote:
> > Hi Vladimir,
> > Interestingly though, when I try to run a test (either with ./run_tests.sh
> > mte-sync, or by trying out your example), I get this error:
> > 
> > qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> > 
> > My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> > model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> > around this on your end?
> > 
> 
> ./configure --arch=arm64 --processor=max

Ah, so that explains it. Would you mind adding this information to the commit
message, perhaps something like this:

If using TCG as the accelerator, qemu must emulate a CPU that supports MTE
in order for the tests to run. Use configure --processor to set an
appropriate CPU. If unsure about a particular processor, 'max' will most
likely work with a new enough qemu version.

> 
> Thanks
> Vladimir
> 
> > Thanks,
> > Alex
> > 
> 

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Vladimir Murzin]

Hi Alexandru,

On 1/2/25 11:45, Alexandru Elisei wrote:
> Hi Vladimir,
> 
> On Thu, Jan 02, 2025 at 10:04:37AM +0000, Vladimir Murzin wrote:
> [..]
>> On 12/30/24 15:19, Alexandru Elisei wrote:
>>> Hi Vladimir,
>>> Interestingly though, when I try to run a test (either with ./run_tests.sh
>>> mte-sync, or by trying out your example), I get this error:
>>>
>>> qemu-system-aarch64: MTE requested, but not supported by the guest CPU
>>>
>>> My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
>>> model CPU. Changing the cpu to 'max' makes the test run. How are you getting
>>> around this on your end?
>>>
>> ./configure --arch=arm64 --processor=max
> Ah, so that explains it. Would you mind adding this information to the commit
> message, perhaps something like this:
> 
> If using TCG as the accelerator, qemu must emulate a CPU that supports MTE
> in order for the tests to run. Use configure --processor to set an
> appropriate CPU. If unsure about a particular processor, 'max' will most
> likely work with a new enough qemu version.
> 

Yes, I can add it when re-spin v4 (sorry just sent v3). Alternatively, we can
merge change defaulting processor to `max` for arm64 which IMO much better
solution (and does not require me to re-spin the patch) ;-)

Cheers
Vladimir

>> Thanks
>> Vladimir
>>
>>> Thanks,
>>> Alex
>>>

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei]

Hi Vladimir,

On Thu, Jan 02, 2025 at 12:10:05PM +0000, Vladimir Murzin wrote:
> Hi Alexandru,
> 
> On 1/2/25 11:45, Alexandru Elisei wrote:
> > Hi Vladimir,
> > 
> > On Thu, Jan 02, 2025 at 10:04:37AM +0000, Vladimir Murzin wrote:
> > [..]
> >> On 12/30/24 15:19, Alexandru Elisei wrote:
> >>> Hi Vladimir,
> >>> Interestingly though, when I try to run a test (either with ./run_tests.sh
> >>> mte-sync, or by trying out your example), I get this error:
> >>>
> >>> qemu-system-aarch64: MTE requested, but not supported by the guest CPU
> >>>
> >>> My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
> >>> model CPU. Changing the cpu to 'max' makes the test run. How are you getting
> >>> around this on your end?
> >>>
> >> ./configure --arch=arm64 --processor=max
> > Ah, so that explains it. Would you mind adding this information to the commit
> > message, perhaps something like this:
> > 
> > If using TCG as the accelerator, qemu must emulate a CPU that supports MTE
> > in order for the tests to run. Use configure --processor to set an
> > appropriate CPU. If unsure about a particular processor, 'max' will most
> > likely work with a new enough qemu version.
> > 
> 
> Yes, I can add it when re-spin v4 (sorry just sent v3). Alternatively, we can
> merge change defaulting processor to `max` for arm64 which IMO much better
> solution (and does not require me to re-spin the patch) ;-)

I'm going to send patches to make 'max' the default, so no need to respin :)

Thanks,
Alex

> 
> Cheers
> Vladimir
> 
> >> Thanks
> >> Vladimir
> >>
> >>> Thanks,
> >>> Alex
> >>>
> 

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Nikos Nikoleris]

Hi Vladimir,

Thanks for this! Some comments inline.

On 23/12/2024 12:03, Alexandru Elisei wrote:
> Hi Vladimir,
> 
> The patch looks good, but it just occured to me, the tests do a great job
> checking that tagged accesses fail when they should be failing, but they don't
> check that taggedd accesses *succeed* when they should not be failing. I think
> that's useful to have at the start of each test, if nothing just as a sanity
> check.
> 
> On Thu, Dec 12, 2024 at 10:34:47AM +0000, Vladimir Murzin wrote:
>> Test tag storage access and tag mismatch for different MTE modes.
>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>
>>   v1 -> v2
>>      - Addressed comments (I hope I did not miss any) from Alexandru
>>
>>   arm/Makefile.arm64            |  10 +-
>>   arm/cstart64.S                |   4 +-
>>   arm/mte.c                     | 299 ++++++++++++++++++++++++++++++++++
>>   arm/run                       |   3 +-
>>   arm/unittests.cfg             |  19 +++
>>   lib/arm64/asm/mmu.h           |   1 +
>>   lib/arm64/asm/pgtable-hwdef.h |   2 +
>>   lib/arm64/asm/sysreg.h        |  12 ++
>>   8 files changed, 347 insertions(+), 3 deletions(-)
>>   create mode 100644 arm/mte.c
>>
>> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
>> index 3b9034e3..48dcdbd4 100644
>> --- a/arm/Makefile.arm64
>> +++ b/arm/Makefile.arm64
>> @@ -17,10 +17,17 @@ ifneq ($(strip $(sve_flag)),)
>>   CFLAGS += -DCC_HAS_SVE
>>   endif
>>   
>> +mte_flag := $(call cc-option, -march=armv8.5-a+memtag, "")

FEAT_MTE2 is an optional feature from v8.4. Should we lower the 
requirement? I've also tried checking for armv8-a+memtag and that work 
too, since we don't really care about the exact version.

>> +ifneq ($(strip $(mte_flag)),)
>> +# MTE is supported by the compiler, generate MTE instructions
>> +CFLAGS += -DCC_HAS_MTE
>> +endif
>> +
>> +
>>   mno_outline_atomics := $(call cc-option, -mno-outline-atomics, "")
>>   CFLAGS += $(mno_outline_atomics)
>>   CFLAGS += -DCONFIG_RELOC
>> -CFLAGS += -mgeneral-regs-only
>> +CFLAGS += -mgeneral-regs-only -save-temps
> 
> Hmm.. I can't seem to figure out why -save-temps is required for the MTE test.
>  From man gcc, -save-temps is a knob that tells gcc not to delete the
> intermediate files that it generates. Am I missing something?
> 
>>   
>>   define arch_elf_check =
>>   	$(if $(shell ! $(READELF) -rW $(1) >&/dev/null && echo "nok"),
>> @@ -57,6 +64,7 @@ tests += $(TEST_DIR)/micro-bench.$(exe)
>>   tests += $(TEST_DIR)/cache.$(exe)
>>   tests += $(TEST_DIR)/debug.$(exe)
>>   tests += $(TEST_DIR)/fpu.$(exe)
>> +tests += $(TEST_DIR)/mte.$(exe)
>>   
>>   include $(SRCDIR)/$(TEST_DIR)/Makefile.common
>>   
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index b480a552..b9d7a446 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -242,6 +242,7 @@ halt:
>>    *   NORMAL             100     11111111
>>    *   NORMAL_WT          101     10111011
>>    *   DEVICE_nGRE        110     00001000
>> + *   NORMAL_TAGGED      111     11110000
>>    */
>>   #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>>   
>> @@ -275,7 +276,8 @@ asm_mmu_enable:
>>   		     MAIR(0x44, MT_NORMAL_NC) |		\
>>   		     MAIR(0xff, MT_NORMAL) |	        \
>>   		     MAIR(0xbb, MT_NORMAL_WT) |         \
>> -		     MAIR(0x08, MT_DEVICE_nGRE)
>> +		     MAIR(0x08, MT_DEVICE_nGRE) |       \
>> +		     MAIR(0xf0, MT_NORMAL_TAGGED)
>>   	msr	mair_el1, x1
>>   
>>   	/* TTBR0 */
>> diff --git a/arm/mte.c b/arm/mte.c
>> new file mode 100644
>> index 00000000..718f0a61
>> --- /dev/null
>> +++ b/arm/mte.c
>> @@ -0,0 +1,299 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2024 Arm Limited.
>> + * All rights reserved.
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <alloc_page.h>
>> +#include <stdlib.h>
>> +#include <asm/mmu.h>
>> +#include <asm/sysreg.h>
>> +#include <asm/pgtable-hwdef.h>
>> +#include <asm/processor.h>
>> +#include <asm/thread_info.h>
>> +
>> +
>> +/* Tag Check Faults cause a synchronous exception */
>> +#define MTE_TCF_SYNC	0b01
>> +/* Tag Check Faults are asynchronously accumulated */
>> +#define MTE_TCF_ASYNC	0b10
>> +/*
>> + * Tag Check Faults cause a synchronous exception on reads,
>> + * and are asynchronously accumulated on writes
>> + */
>> +#define MTE_TCF_ASYMM	0b11
>> +
>> +#define MTE_GRANULE_SIZE        UL(16)
>> +#define MTE_GRANULE_MASK        (~(MTE_GRANULE_SIZE - 1))
>> +#define MTE_TAG_SHIFT           56
>> +
>> +#define untagged(p)									\
>> +({											\
>> +	unsigned long __in = (unsigned long)(p);					\
>> +	typeof(p) __out = (typeof(p))(__in & ~(MTE_GRANULE_MASK << MTE_TAG_SHIFT));	\
>> +											\
>> +	__out;										\
>> +})
>> +
>> +#define tagged(p,t)							\
>> +({									\
>> +	unsigned long __in = (unsigned long)(untagged(p));		\
>> +	unsigned long __tag = (unsigned long)(t) << MTE_TAG_SHIFT;	\
>> +	typeof(p) __out = (typeof(p))(__in | __tag);			\
>> +									\
>> +	__out;								\
>> +})
>> +
>> +/*
>> + * If we use a normal (non hand coded inline assembly) load or store
>> + * to access a tagged address, the compiler will reasonably assume
>> + * that the access succeeded, and the next instruction may do
>> + * something based on that assumption.
>> + *
>> + * But a test might want the tagged access to fail on purpose, and if
>> + * we advance the PC to the next instruction, the one added by the
>> + * compiler, we might leave the program in an unexpected state.
>> + */
>> +static inline void mem_read(unsigned int *addr, unsigned int tag, unsigned int *res)
>> +{
>> +	unsigned int r;
>> +
>> +	asm volatile ("ldr %0,[%1]\n"
>> +		      "str %0,[%2]\n"
>> +		      : "=r" (r)
>> +		      : "r" (tagged(addr, tag)), "r" (res));
>> +}
>> +
>> +static inline void mem_write(unsigned int *addr, unsigned int tag, unsigned int val)
>> +{
>> +	/* The NOP allows the same exception handler as mem_read() to be used. */
>> +	asm volatile ("str %0,[%1]\n"
>> +		      "nop\n"
>> +		      :
>> +		      : "r" (val), "r" (tagged(addr, tag))
>> +		      : "memory");
>> +}
>> +
>> +static volatile bool mte_exception;
>> +
>> +static void mte_fault_handler(struct pt_regs *regs, unsigned int esr)
>> +{
>> +	unsigned int dfsc = esr & GENMASK(5, 0);
>> +	unsigned int fnv = esr & BIT(10);
>> +
>> +	if ((dfsc == 0b010001) && (fnv == 0))
>> +		mte_exception = true;
>> +
>> +	/*
>> +	 * mem_read() reads the value from the tagged pointer, then
>> +	 * stores this value in the untagged 'res' pointer. The
>> +	 * function that called mem_read() will want to check that the
>> +	 * initial value of 'res' hasn't changed if a tag check fault
>> +	 * is reported. Skip over two instructions so 'res' isn't
>> +	 * overwritten.
>> +	 */
>> +	regs->pc += 8;
>> +}
>> +
>> +static inline void mmu_set_tagged(pgd_t *pgtable, unsigned long vaddr)
>> +{
>> +	pteval_t *p_pte = follow_pte(pgtable, vaddr);
>> +
>> +	if (p_pte) {
>> +		pteval_t entry = *p_pte;
>> +
>> +		entry &= ~PTE_ATTRINDX_MASK;
>> +		entry |= PTE_ATTRINDX(MT_NORMAL_TAGGED);
>> +
>> +		WRITE_ONCE(*p_pte, entry);
>> +		flush_tlb_page(vaddr);
>> +	} else {
>> +		report_abort("Cannot find PTE");
>> +	}
>> +}
>> +
>> +static void mte_init(void)
>> +{
>> +	unsigned long sctlr = read_sysreg(sctlr_el1);
>> +	unsigned long tcr = read_sysreg(tcr_el1);
>> +
>> +	sctlr &= ~(SCTLR_EL1_TCF_MASK | SCTLR_EL1_TCF0_MASK);
>> +	sctlr &= ~SCTLR_EL1_ATA0;
>> +	sctlr |= SCTLR_EL1_ATA;
>> +
>> +	tcr |= TCR_TBI1 | TCR_TBI0;
>> +
>> +	write_sysreg(sctlr, sctlr_el1);
>> +	write_sysreg(tcr, tcr_el1);
>> +
>> +	isb();
>> +	flush_tlb_all();
>> +}
>> +
>> +static inline void mte_set_tcf(unsigned long tcf)
>> +{
>> +	unsigned long sctlr = read_sysreg(sctlr_el1);
>> +
>> +	sctlr &= ~(SCTLR_EL1_TCF_MASK | SCTLR_EL1_TCF0_MASK);
>> +	sctlr |= (tcf << SCTLR_EL1_TCF_SHIFT) & SCTLR_EL1_TCF_MASK ;
>> +	sctlr |= (tcf << SCTLR_EL1_TCF0_SHIFT) & SCTLR_EL1_TCF0_MASK ;
>> +
>> +	write_sysreg(sctlr, sctlr_el1);
>> +	isb();
>> +}
>> +
>> +
>> +static inline void mte_set_tag(void *addr, size_t size, unsigned int tag)
>> +{
>> +#ifdef CC_HAS_MTE
>> +	unsigned long in = (unsigned long)untagged(addr);
>> +	unsigned long start = ALIGN_DOWN(in, 16);
>> +	unsigned long end = ALIGN(in + size , 16);
>> +
>> +	for (unsigned long ptr = start; ptr < end; ptr += 16)
>> +		asm volatile(".arch   armv8.5-a+memtag\n"

Is the .arch directive necessary? IIUC, this is typically used to 
instruct the compiler to use a reduced feature set.

>> +			     "stg %0, [%0]"
>> +			     :
>> +			     : "r"(tagged(ptr, tag))
>> +			     : "memory");
>> +#endif
>> +}
>> +
>> +static inline unsigned long get_clear_tfsr(void)
>> +{
>> +	unsigned long r;
>> +
>> +	dsb(nsh);
>> +	isb();
>> +
>> +	r = read_sysreg_s(TFSR_EL1);
>> +	write_sysreg_s(0, TFSR_EL1);
>> +
>> +	return r;
>> +}
>> +
>> +static void mte_sync_test(void)
>> +{
>> +	unsigned int *mem = alloc_page();
>> +	unsigned int val = 0;
>> +
>> +	memset(mem, 0xff, PAGE_SIZE);
>> +	mte_init();

The mte_init() could also be called by main as it is common across all 
three tests?

>> +	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
>> +	mte_set_tag(mem, PAGE_SIZE, 0);
>> +	mte_set_tcf(MTE_TCF_SYNC);
>> +	mte_exception = false;
>> +
>> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);

Would it make sense to call this from mte_init() and maybe check whether 
we got an exception for sync fault and that we didn't for async faults?

>> +
>> +	mem_read(mem, 1, &val);
> 
> When I came back to the patch, I read this and I thought that the value 1
> represents what the value of 'val' should be. Do you think the code would be
> easier to read if mem_read() took a tagged address directly, i.e:
> 
> 	mem_read(tagged(mem, 1), &val);
> 
> Up to you what you prefer.

I agree with Alex. But up to you Vladimir.

> 
>> +
>> +	report((val == 0) && mte_exception, "read");
>> +
>> +	mte_exception = false;
>> +
>> +	mem_write(mem, 2, 0xbbbbbbbb);
>> +
>> +	report((*mem == 0xffffffff) && mte_exception, "write");

Minor but you might want to call free_pages(mem) as well.

>> +}
>> +
>> +static void mte_asymm_test(void)
>> +{
>> +	unsigned int *mem = alloc_page();
>> +	unsigned int val = 0;
>> +
>> +	memset(mem, 0xff, PAGE_SIZE);
>> +	mte_init();
>> +	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
>> +	mte_set_tag(mem, PAGE_SIZE, 0);
>> +	mte_set_tcf(MTE_TCF_ASYMM);
>> +	mte_exception = false;
>> +
>> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
>> +
>> +	mem_read(mem, 3, &val);
>> +	report((val == 0) && mte_exception, "read");
>> +
>> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, NULL);
>> +
>> +	mem_write(mem, 4, 0xaaaaaaaa);
>> +	report((*mem == 0xaaaaaaaa) && (get_clear_tfsr() == 1), "write");
> 
> Do you think it would be easier to understand the code if TFSR_EL1_TF0 would be
> used here instead of 1? Up to you what you prefer, but if you do decide to use
> a define, please also add TFSR_EL1_TF1, just in case someone decides to expand
> the test some day with an address in the upper virtual address space.
> 
>> +}
>> +
>> +static void mte_async_test(void)
>> +{
>> +	unsigned int *mem = alloc_page();
>> +	unsigned int val = 0;
>> +
>> +	memset(mem, 0xff, PAGE_SIZE);
>> +	mte_init();
>> +	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
>> +	mte_set_tag(mem, PAGE_SIZE, 0);
>> +	mte_set_tcf(MTE_TCF_ASYNC);
>> +
>> +	mem_read(mem, 5, &val);
>> +	report((val == 0xffffffff) && (get_clear_tfsr() == 1), "read");
>> +
>> +	mem_write(mem, 6, 0xcccccccc);
>> +	report((*mem == 0xcccccccc) && (get_clear_tfsr() == 1), "write");
>> +}
>> +
>> +
>> +static unsigned int mte_version(void)
>> +{
>> +#ifdef CC_HAS_MTE
>> +	uint64_t r;
>> +
>> +	asm volatile("mrs %x0, id_aa64pfr1_el1" : "=r"(r));
>> +
>> +	return (r >> 8) & 0b1111;

Sorry, if this very tedious but it might be better if we defined some 
constants here instead?

Thanks,

Nikos

>> +#else
>> +	report_info("Compiler lack MTE support");
>> +	return 0;
>> +#endif
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +
>> +	unsigned int version = mte_version();
>> +
>> +	if (version < 2) {
>> +		report_skip("No MTE support, skip...\n");
>> +		return -1;
> 
> Can you change the return to:
> 
> 		return report_summary();
> 
> Explanation below.
> 
>> +	}
>> +
>> +	if (argc < 2)
>> +		report_abort("no test specified");
>> +
>> +	report_prefix_pushf("mte");
> 
> report_prefix_push() (without the 'f' at the end).
> 
>> +
>> +	if (strcmp(argv[1], "sync") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		mte_sync_test();
>> +		report_prefix_pop();
>> +	} else if (strcmp(argv[1], "async") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		if (version < 3) {
>> +			report_skip("No MTE async, skip...\n");
>> +			return -1;
>> +		}
>> +		mte_async_test();
>> +		report_prefix_pop();
>> +
>> +	} else if (strcmp(argv[1], "asymm") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		if (version < 3) {
>> +			report_skip("No MTE asymm, skip...\n");
>> +			return -1;
>> +		}
>> +		mte_asymm_test();
>> +		report_prefix_pop();
>> +
>> +	} else {
>> +		report_abort("Unknown sub-test '%s'", argv[1]);
>> +	}
>> +
>> +	return report_summary();
>> +}
>> diff --git a/arm/run b/arm/run
>> index efdd44ce..b129e4e0 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
>>   	exit 2
>>   fi
>>   
>> -M='-machine virt'
>> +MACHINE="virt"
>> +M="-machine $MACHINE$MACHINE_PROPS"
> 
> Sorry, but I still don't understand what $MACHINE_PROPS does, I can't seem to
> find where it's initialized :(
> 
>>   
>>   if [ "$ACCEL" = "kvm" ]; then
>>   	if $qemu $M,\? | grep -q gic-version; then
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index 2bdad67d..9b428c02 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -271,3 +271,22 @@ smp = 2
>>   groups = nodefault
>>   accel = kvm
>>   arch = arm64
>> +
>> +# MTE tests
>> +[mte-sync]
>> +file = mte.flat
>> +groups = nodefault
>> +extra_params = -append 'sync'
>> +arch = arm64
>> +
>> +[mte-async]
>> +file = mte.flat
>> +groups = nodefault
>> +extra_params = -append 'async'
>> +arch = arm64
>> +
>> +[mte-asymm]
>> +file = mte.flat
>> +groups = nodefault
>> +extra_params = -append 'asymm'
>> +arch = arm64
> 
> I think it would be better if the tests are not in the nodefault group. As far
> as I can tell, they are in the nondefault group because not all hardware and KVM
> versions support MTE (please correct me if I'm wrong).
> 
> If the tests are in the nodefault group, then a CI administrator must
> keep track of those machine that have MTE and manually run them, otherwise they
> will never get run, even if the machines support it.
> 
> If you remove them from the nondefault group, with the change I proposed to
> main() the test runner will not mark them as failed on systems that don't
> support MTE:
> 
> $ ./run_tests.sh
> [..]
> SKIP mte-sync (1 tests, 1 skipped)
> SKIP mte-async (1 tests, 1 skipped)
> SKIP mte-asymm (1 tests, 1 skipped)
> 
> but they will get automatically run on systems that support MTE.
> 
> Also, would you mind putting the tests in the mte group:
> 
> 	groups = mte
> 
> so they can be run with ./run_tests.sh -g mte.
> 
> Thanks,
> Alex
> 
>> diff --git a/lib/arm64/asm/mmu.h b/lib/arm64/asm/mmu.h
>> index 5c27edb2..9aedd09a 100644
>> --- a/lib/arm64/asm/mmu.h
>> +++ b/lib/arm64/asm/mmu.h
>> @@ -10,6 +10,7 @@
>>   #define PMD_SECT_UNCACHED	PMD_ATTRINDX(MT_DEVICE_nGnRE)
>>   #define PTE_UNCACHED		PTE_ATTRINDX(MT_DEVICE_nGnRE)
>>   #define PTE_WBWA		PTE_ATTRINDX(MT_NORMAL)
>> +#define PTE_TAGGED		PTE_ATTRINDX(MT_NORMAL_TAGGED)
>>   
>>   static inline void flush_tlb_all(void)
>>   {
>> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
>> index 8c41fe12..b66b62da 100644
>> --- a/lib/arm64/asm/pgtable-hwdef.h
>> +++ b/lib/arm64/asm/pgtable-hwdef.h
>> @@ -145,6 +145,7 @@
>>   #define TCR_TG1_64K		(UL(3) << 30)
>>   #define TCR_ASID16		(UL(1) << 36)
>>   #define TCR_TBI0		(UL(1) << 37)
>> +#define TCR_TBI1		(UL(1) << 38)
>>   
>>   /*
>>    * Memory types available.
>> @@ -156,5 +157,6 @@
>>   #define MT_NORMAL		4
>>   #define MT_NORMAL_WT		5
>>   #define MT_DEVICE_nGRE		6
>> +#define MT_NORMAL_TAGGED	7
>>   
>>   #endif /* _ASMARM64_PGTABLE_HWDEF_H_ */
>> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
>> index f214a4f0..60f51dad 100644
>> --- a/lib/arm64/asm/sysreg.h
>> +++ b/lib/arm64/asm/sysreg.h
>> @@ -28,6 +28,7 @@
>>   	.endm
>>   #else
>>   #include <libcflat.h>
>> +#include <bitops.h>
>>   
>>   #define read_sysreg(r) ({					\
>>   	u64 __val;						\
>> @@ -81,7 +82,12 @@ asm(
>>   #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
>>   #define ICC_GRPEN1_EL1			sys_reg(3, 0, 12, 12, 7)
>>   
>> +#define TFSR_EL1			sys_reg(3, 0, 5, 6, 0)
>> +
>> +
>>   /* System Control Register (SCTLR_EL1) bits */
>> +#define SCTLR_EL1_ATA		_BITULL(43)
>> +#define SCTLR_EL1_ATA0		_BITULL(42)
>>   #define SCTLR_EL1_LSMAOE	_BITULL(29)
>>   #define SCTLR_EL1_NTLSMD	_BITULL(28)
>>   #define SCTLR_EL1_EE		_BITULL(25)
>> @@ -99,6 +105,12 @@ asm(
>>   #define SCTLR_EL1_A		_BITULL(1)
>>   #define SCTLR_EL1_M		_BITULL(0)
>>   
>> +#define SCTLR_EL1_TCF_SHIFT	40
>> +#define SCTLR_EL1_TCF_MASK	GENMASK_ULL(41, 40)
>> +
>> +#define SCTLR_EL1_TCF0_SHIFT	38
>> +#define SCTLR_EL1_TCF0_MASK	GENMASK_ULL(39, 38)
>> +
>>   #define INIT_SCTLR_EL1_MMU_OFF	\
>>   			(SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \
>>   			 SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \
>> -- 
>> 2.25.1
>>

Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Vladimir Murzin]

Hi Nikos,

On 12/30/24 17:01, Nikos Nikoleris wrote:
> Hi Vladimir,
> 
> Thanks for this! Some comments inline.

Thanks for your time!

> 
> On 23/12/2024 12:03, Alexandru Elisei wrote:
>> Hi Vladimir,
>>
>> The patch looks good, but it just occured to me, the tests do a great job
>> checking that tagged accesses fail when they should be failing, but they don't
>> check that taggedd accesses *succeed* when they should not be failing. I think
>> that's useful to have at the start of each test, if nothing just as a sanity
>> check.
>>
>> On Thu, Dec 12, 2024 at 10:34:47AM +0000, Vladimir Murzin wrote:
>>> Test tag storage access and tag mismatch for different MTE modes.
>>>
>>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>>> ---
>>>
>>>   v1 -> v2
>>>      - Addressed comments (I hope I did not miss any) from Alexandru
>>>
>>>   arm/Makefile.arm64            |  10 +-
>>>   arm/cstart64.S                |   4 +-
>>>   arm/mte.c                     | 299 ++++++++++++++++++++++++++++++++++
>>>   arm/run                       |   3 +-
>>>   arm/unittests.cfg             |  19 +++
>>>   lib/arm64/asm/mmu.h           |   1 +
>>>   lib/arm64/asm/pgtable-hwdef.h |   2 +
>>>   lib/arm64/asm/sysreg.h        |  12 ++
>>>   8 files changed, 347 insertions(+), 3 deletions(-)
>>>   create mode 100644 arm/mte.c
>>>
>>> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
>>> index 3b9034e3..48dcdbd4 100644
>>> --- a/arm/Makefile.arm64
>>> +++ b/arm/Makefile.arm64
>>> @@ -17,10 +17,17 @@ ifneq ($(strip $(sve_flag)),)
>>>   CFLAGS += -DCC_HAS_SVE
>>>   endif
>>>   +mte_flag := $(call cc-option, -march=armv8.5-a+memtag, "")
> 
> FEAT_MTE2 is an optional feature from v8.4. Should we lower the requirement? I've also tried checking for armv8-a+memtag and that work too, since we don't really care about the exact version.
> 

IIUC, that might demand newer compiler(s) since most of them went
quite conservative originally and only recently removed that
dependency [1]. So it seems better to be conservative to cover broader
compiler base.

<snip>

>>> +
>>> +static inline void mte_set_tag(void *addr, size_t size, unsigned int tag)
>>> +{
>>> +#ifdef CC_HAS_MTE
>>> +    unsigned long in = (unsigned long)untagged(addr);
>>> +    unsigned long start = ALIGN_DOWN(in, 16);
>>> +    unsigned long end = ALIGN(in + size , 16);
>>> +
>>> +    for (unsigned long ptr = start; ptr < end; ptr += 16)
>>> +        asm volatile(".arch   armv8.5-a+memtag\n"
> 
> Is the .arch directive necessary? IIUC, this is typically used to instruct the compiler to use a reduced feature set.
> 

Yes, it is necessary here since we want compiler (more precisely,
assembler) to recognize stg/ldg instructions. That directive is limited
to inline asm block only. Alternative would be to ask user to provide
`-march=armv8.5-a+memtag` option build time, that was discussed in [2]


>>> +                 "stg %0, [%0]"
>>> +                 :
>>> +                 : "r"(tagged(ptr, tag))
>>> +                 : "memory");
>>> +#endif
>>> +}
>>> +
>>> +static inline unsigned long get_clear_tfsr(void)
>>> +{
>>> +    unsigned long r;
>>> +
>>> +    dsb(nsh);
>>> +    isb();
>>> +
>>> +    r = read_sysreg_s(TFSR_EL1);
>>> +    write_sysreg_s(0, TFSR_EL1);
>>> +
>>> +    return r;
>>> +}
>>> +
>>> +static void mte_sync_test(void)
>>> +{
>>> +    unsigned int *mem = alloc_page();
>>> +    unsigned int val = 0;
>>> +
>>> +    memset(mem, 0xff, PAGE_SIZE);
>>> +    mte_init();
> 
> The mte_init() could also be called by main as it is common across all three tests?
> 

Good point! I'll move it into main()

>>> +    mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
>>> +    mte_set_tag(mem, PAGE_SIZE, 0);
>>> +    mte_set_tcf(MTE_TCF_SYNC);
>>> +    mte_exception = false;
>>> +
>>> +    install_exception_handler(EL1H_SYNC, ESR_EL1_EC_DABT_EL1, mte_fault_handler);
> 
> Would it make sense to call this from mte_init() and maybe check whether we got an exception for sync fault and that we didn't for async faults?
> 

There are cases (async and asymm writes) where we do not expect any exception so we
can rely on default exception handler to stop the world and produce output with all
handy information for debug.

>>> +
>>> +    mem_read(mem, 1, &val);
>>
>> When I came back to the patch, I read this and I thought that the value 1
>> represents what the value of 'val' should be. Do you think the code would be
>> easier to read if mem_read() took a tagged address directly, i.e:
>>
>>     mem_read(tagged(mem, 1), &val);
>>
>> Up to you what you prefer.
> 
> I agree with Alex. But up to you Vladimir.
> 

Ack.

>>
>>> +
>>> +    report((val == 0) && mte_exception, "read");
>>> +
>>> +    mte_exception = false;
>>> +
>>> +    mem_write(mem, 2, 0xbbbbbbbb);
>>> +
>>> +    report((*mem == 0xffffffff) && mte_exception, "write");
> 
> Minor but you might want to call free_pages(mem) as well.
> 

Ack.

<snip>

>>> +static unsigned int mte_version(void)
>>> +{
>>> +#ifdef CC_HAS_MTE
>>> +    uint64_t r;
>>> +
>>> +    asm volatile("mrs %x0, id_aa64pfr1_el1" : "=r"(r));
>>> +
>>> +    return (r >> 8) & 0b1111;
> 
> Sorry, if this very tedious but it might be better if we defined some constants here instead?
> 

Ack.


[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622848.html
[2] https://lore.kernel.org/all/c2d9b61b-7dce-422b-8a3c-898f1003c9e4@arm.com/T/#m0a8ae8a0fa6a2f021feb95367d2e71eccc8258d3

Happy (western) New Year!

Vladimir