On 12/01/2023 09:36, Amit Daniel Kachhap wrote:

Inline assembly instruction as shown below is not recognized in Gcc

Nit: s/instruction/modifier/ (the issue is the "w" in "%w0" here, which is an operand modifier).

Also maybe the commit title could be more descriptive, "Fix inline assembly syntax" for instance.

Assembler and generates error,

asm volatile("adr %w0, #0" : "=r"(cap));

/tmp/cc49utqN.s: Assembler messages: /tmp/cc49utqN.s:1110: Error: operand mismatch -- `adr w0,#0' /tmp/cc49utqN.s:1110: Info: did you mean this? /tmp/cc49utqN.s:1110: Info: adr x0, #0x0 /tmp/cc49utqN.s:1088: Error: undefined symbol c30 used as an immediate value

Fix the above by using the syntax as below which is recognized in both Clang and GAS.

Nit: s/GAS/GCC/ - it is the compiler that generates plain assembly instructions from inline assembly statements, and here the issue is in this transformation, not in the assembly syntax itself. It is the assembler that complains, because GCC does understand %w0, but in a different way from Clang. Similarly just saying "GCC" instead of "GCC assembler" above would make more sense to me.

Kevin

asm volatile("adr %0, #0" : "=C"(cap));

Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com

tools/testing/selftests/arm64/morello/bootstrap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/morello/bootstrap.c b/tools/testing/selftests/arm64/morello/bootstrap.c index 6a91ba39dd83..75c636688c5b 100644 --- a/tools/testing/selftests/arm64/morello/bootstrap.c +++ b/tools/testing/selftests/arm64/morello/bootstrap.c @@ -138,12 +138,12 @@ TEST(test_c64) { uintcap_t cap;

• asm volatile("mov %w0, c30" : "=r"(cap));

• asm volatile("mov %0, c30" : "=C"(cap)); /* LSB is set on LR if we came from C64 */ ASSERT_TRUE(cap & 0x1) TH_LOG("we did not come from C64");

/* will write to Cn register in C64 and Xn in A64, clearing the tag */

• asm volatile("adr %w0, #0" : "=r"(cap));

• asm volatile("adr %0, #0" : "=C"(cap)); ASSERT_TRUE(cheri_tag_get(cap)) TH_LOG("not running in C64");

}

On 12/01/2023 09:36, Amit Daniel Kachhap wrote:

Clang toolchain defines uintcap_t in stdint.h present in the system include path obtained with -print-file-name=include option.

In case of Gcc toolchain uintcap_t, it is not present in the system include path but in the libc include path. As the kselftests does not use libc at the moment so define uintcap_t if not defined.

Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com

tools/testing/selftests/arm64/morello/freestanding.h | 3 +++ .../selftests/arm64/morello/freestanding_init_globals.c | 1 + 2 files changed, 4 insertions(+)

diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index ede290f3ca1b..7b168a64b366 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -21,6 +21,9 @@ typedef __kernel_timer_t timer_t; typedef __kernel_clockid_t clockid_t; typedef __kernel_uid_t uid_t; typedef __kernel_mode_t mode_t; +#ifndef uintcap_t

The condition is always true: uintcap_t is either undefined or a typedef, never a macro. #ifndef __clang__ (as in <linux/compiler_types.h>) should do the trick.

Kevin

+typedef __uintcap_t uintcap_t; +#endif #define EXIT_SUCCESS 0 diff --git a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c index 61e37d042789..6139de724f24 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c +++ b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c @@ -5,6 +5,7 @@ #include <stdbool.h> #include <stddef.h> #include <stdint.h> +#include "freestanding.h" struct cap_reloc { size_t capability_location;

On 12/01/2023 09:36, Amit Daniel Kachhap wrote:

Gcc emits only dynamic relocations at the moment and not cap relocations

I would remove "at the moment": emitting dynamic relocations is the new approach, there is no plan for GCC to emit caprelocs and Clang should be moving to dynamic relocations eventually too.

like Clang toolchain. Hence the existing kselftests compiled in static and standalone mode needs to process the dynamic relocations and fill the entries to relevant capability details.

The code changes are similar to the start up code of the recently released Arm GNU Toolchain for Morello.

Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com

.../arm64/morello/freestanding_init_globals.c | 148 +++++++++++++++++- .../arm64/morello/freestanding_start.S | 5 +- 2 files changed, 146 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c index 6139de724f24..243e9d3323ff 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c +++ b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c @@ -5,8 +5,47 @@ #include <stdbool.h> #include <stddef.h> #include <stdint.h> +#include <linux/auxvec.h> +#include <linux/elf.h>

#include "freestanding.h" +#define CHERI_PERM_MASK_BASE (-1UL ^ ( \

• CHERI_PERM_LOAD | \
• CHERI_PERM_STORE | \
• CHERI_PERM_EXECUTE | \
• CHERI_PERM_LOAD_CAP | \
• ARM_CAP_PERMISSION_MUTABLE_LOAD | \
• CHERI_PERM_STORE_CAP | \
• CHERI_PERM_STORE_LOCAL_CAP | \
• ARM_CAP_PERMISSION_EXECUTIVE | \
• CHERI_PERM_SYSTEM_REGS | \
• CHERI_PERM_SEAL | \
• CHERI_PERM_UNSEAL | \
• ARM_CAP_PERMISSION_COMPARTMENT_ID))

I'm not sure I understand the point of this, can't we just use Global as base?

+#define CHERI_PERM_MASK_R ( \

• CHERI_PERM_MASK_BASE | \
• CHERI_PERM_LOAD | \
• CHERI_PERM_LOAD_CAP | \
• ARM_CAP_PERMISSION_MUTABLE_LOAD)

+#define CHERI_PERM_MASK_RW ( \

• CHERI_PERM_MASK_R | \
• CHERI_PERM_STORE | \
• CHERI_PERM_STORE_CAP | \
• CHERI_PERM_STORE_LOCAL_CAP)

+#define CHERI_PERM_MASK_RX ( \

• CHERI_PERM_MASK_R | \
• CHERI_PERM_EXECUTE | \
• ARM_CAP_PERMISSION_EXECUTIVE | \
• CHERI_PERM_SYSTEM_REGS)

+#ifndef R_MORELLO_RELATIVE +#define R_MORELLO_RELATIVE 59395 +#endif

struct cap_reloc { size_t capability_location; size_t base; @@ -23,7 +62,7 @@ struct cap_reloc {

• because capability relocations must have already been processed in order to
• refer to such symbols.

*/ -void __morello_init_globals(void) +void __morello_init_cap_relocs(void) { const struct cap_reloc *start_cap_relocs, *end_cap_relocs, *reloc; uintcap_t root_cap; @@ -33,11 +72,20 @@ void __morello_init_globals(void) * not be indirected through the GOT, as this would create a capability * relocation. We need assembly to refer to those directly. */

• asm("adrp %0, __start___cap_relocs\n\t"

• asm(".weak __start___cap_relocs\n\t"

•    ".hidden __start___cap_relocs\n\t"
  

•    "adrp %0, __start___cap_relocs\n\t"
   "add %0, %0, #:lo12:__start___cap_relocs\n\t"
  

•    "adrp %1, __stop___cap_relocs\n\t"
  

•    "add %1, %1, #:lo12:__stop___cap_relocs"
  

•    : "=C"(start_cap_relocs), "=C"(end_cap_relocs));
  

•    : "=C"(start_cap_relocs));
  

• asm(".weak __stop___cap_relocs\n\t"

•    ".hidden __stop___cap_relocs\n\t"
  

•    "adrp %0, __stop___cap_relocs\n\t"
  

•    "add %0, %0, #:lo12:__stop___cap_relocs"
  

•    : "=C"(end_cap_relocs));
  

• if (start_cap_relocs >= end_cap_relocs)

• return;
  

Isn't that implicitly tested in the loop below already? This does skip the next line, but since it should be just one instruction I'm not sure it's worth bothering.

root_cap = (uintcap_t)cheri_ddc_get(); @@ -63,3 +111,93 @@ void __morello_init_globals(void) *target = cap; } }

+static void

Nit: it's uncommon to use this style in the kernel, better have the return type on the same line and split the argument list if necessary. Same below.

+get_caps(uintptr_t *cap_rx, uintptr_t *cap_rw, const uintptr_t *auxv) +{

• for (;;) {

• switch ((unsigned long)auxv[0])	{
  

• case AT_NULL:
  

• 	return;
  

• case AT_CHERI_EXEC_RX_CAP:
  

• 	*cap_rx = auxv[1];
  

• 	*cap_rx = cheri_perms_and(*cap_rx, CHERI_PERM_MASK_RX);
  

• 	break;
  

• case AT_CHERI_EXEC_RW_CAP:
  

• 	*cap_rw = auxv[1];
  

• 	*cap_rw = cheri_perms_and(*cap_rw, CHERI_PERM_MASK_RW);
  

• 	break;
  

• }
  

• auxv += 2;
  

• }

+}

+static inline uintptr_t +morello_relative(uint64_t base, uintptr_t cap_rx, uintptr_t cap_rw,

•  Elf64_Rela *reloc, void *reloc_addr)
  

+{

• uint64_t *__attribute__((may_alias)) u64_reloc_addr = reloc_addr;

Nit: space between * and __attribute__.

More importantly though, isn't the attribute mispositioned? AIUI this is a type attribute, meaning that the type may alias another type when accessed through a pointer. The aliasing we want to allow here is between pointers to uint64_t and uintptr_t, not between pointers to uint64_t * and uintptr_t * (see also [1]).

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html

• /*

• * Fragment identified by r_offset has the following information:
  

• * | 64-bit: address | 56-bits: length | 8-bits: permissions |
  

• */
  

• unsigned long loc = u64_reloc_addr[0];
• unsigned long len = u64_reloc_addr[1] & ((1UL << 56) - 1);
• unsigned long perm = u64_reloc_addr[1] >> 56;
• uintptr_t value;
• /*

• * Permissions field is encoded as:
  

• * 4 = executable, 2 = read/write, 1 = read-only.
  

• */
  

• if (perm == 2)

• value = cheri_address_set(cap_rw, base + loc);
  

• else

• value = cheri_address_set(cap_rx, base + loc);
  

• value = cheri_bounds_set_exact(value, len);
• value = value + reloc->r_addend;
• if (perm == 1)

• value = cheri_perms_and(value, CHERI_PERM_MASK_R);
  

• /* Seal executable capabilities with MORELLO_RB. */
• if (perm == 4)

• value = cheri_sentry_create(value);
  

• return value;

+}

+void __morello_init_dynamic_relocs(int __attribute__((unused)) argc,

• 		   char __attribute__((unused)) **argv,
  

• 		   char __attribute__((unused)) **envp,
  

Nit: putting the attribute here seems to work, but that's a rather strange position. After the argument name is more common.

• 		   void *auxv)
  

+{

• Elf64_Rela *rela_dyn_start, *rela_dyn_end, *reloc;
• uintptr_t cap_rx = 0;
• uintptr_t cap_rw = 0;
• asm(".weak __rela_dyn_start\n\t"

•    ".hidden __rela_dyn_start\n\t"
  

•    "adrp %0, __rela_dyn_start\n\t"
  

•    "add %0, %0, #:lo12:__rela_dyn_start\n\t"
  

•    : "=C"(rela_dyn_start));
  

• asm(".weak __rela_dyn_end\n\t"

•    ".hidden __rela_dyn_end\n\t"
  

•    "adrp %0, __rela_dyn_end\n\t"
  

•    "add %0, %0, #:lo12:__rela_dyn_end"
  

•    : "=C"(rela_dyn_end));
  

• if (rela_dyn_start >= rela_dyn_end)

• return;
  

• get_caps(&cap_rx, &cap_rw, auxv);
• for (reloc = rela_dyn_start; reloc < rela_dyn_end; ++reloc) {

• uintptr_t *reloc_addr, value;
  

• if (reloc->r_info != R_MORELLO_RELATIVE)
  

• 	continue;
  

• reloc_addr = (uintptr_t *)cheri_address_set(cap_rw, reloc->r_offset);
  

• value = morello_relative(0, cap_rx, cap_rw, reloc, reloc_addr);
  

• *reloc_addr = value;
  

• }
• /* Compiler barrier after relocs are processed. */
• asm volatile ("" ::: "memory");

This is called from assembly, so no compiler barrier is needed.

+} diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 66dfe2bab676..f04cd7cad368 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -25,7 +25,7 @@

• besides the .space we specify:
• • a space for a capability pointing to it (in the .got section)
• • a capability relocation (in __cap_relocs section) to make the capability

• • valid (see __morello_init_globals() for more details)

• • valid (see __morello_init_cap_relocs() for more details)

Good you spotted this, I would also remove the parentheses in the line above, and here say e.g. __morello__init_{cap,dynamic}_relocs to make it clear there are two possible forms of relocations.

• We use
• adrp c0, :got:name
• ldr c0, [c0, :got_lo12:name]

@@ -72,7 +72,8 @@ FUNCTION_START(_start) mov c22, c2 mov c23, c3

• bl __morello_init_globals

• bl __morello_init_cap_relocs
• bl __morello_init_dynamic_relocs

I think "__morello_process_*_relocs" would make more sense ("init" was about initialising globals, now we say we talk about processing relocations instead). Same observation concerning the commit title for that matter.

Kevin

/* save the initial kernel stack to a global variable */ got_get_ptr stack_from_kernel, c0

On 12/01/2023 09:36, Amit Daniel Kachhap wrote:

This commit allows morello kselftest to be compiled from GCC toolchain. Also, Gcc toolchain complains for the missing memcpy function with -nostdlib linker option so a memcpy implementation is added.

Note: This commit requires CC to be set for Clang toolchain which was not earlier.

"not the case"?

Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com

tools/testing/selftests/arm64/morello/Makefile | 12 +++++++----- tools/testing/selftests/arm64/morello/freestanding.c | 7 +++++++ 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/arm64/morello/Makefile b/tools/testing/selftests/arm64/morello/Makefile index 7dbf5b140695..51e46838e345 100644 --- a/tools/testing/selftests/arm64/morello/Makefile +++ b/tools/testing/selftests/arm64/morello/Makefile @@ -1,16 +1,18 @@ # SPDX-License-Identifier: GPL-2.0 # Copyright (C) 2021 Arm Limited -# lib.mk sets CC. This switch triggers it to clang -LLVM := 1

+# Provide clang as CC otherwise defaults to gcc

This is unclear, maybe "Assume gcc by default"?

+ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) CLANG_FLAGS = --target=aarch64-linux-gnu

Looking more closely at lib.mk, it looks like it already adds both --target=... and -fintegrated-as to CC if LLVM=1 is set. Maybe we are just duplicating this mechanism needlessly by adding those flags to CFLAGS? The instructions to build the kselftests with Clang would then be to pass LLVM=1 to make.

+CFLAGS_CLANG = $(CLANG_FLAGS) -integrated-as +LDFLAGS_CLANG = -fuse-ld=lld +endif

CFLAGS_PURECAP = -march=morello+c64 -mabi=purecap CFLAGS_COMMON = -g -ffreestanding -Wall -Wextra -MMD CFLAGS_COMMON += -nostdinc -isystem $(shell $(CC) -print-file-name=include 2>/dev/null) -CFLAGS_CLANG = $(CLANG_FLAGS) -integrated-as CFLAGS += $(CFLAGS_CLANG) $(CFLAGS_PURECAP) $(CFLAGS_COMMON) -LDFLAGS := -fuse-ld=lld $(CLANG_FLAGS) -nostdlib -static +LDFLAGS := $(LDFLAGS_CLANG) $(CLANG_FLAGS) -nostdlib -static SRCS := $(wildcard *.c) $(wildcard *.S) PROGS := bootstrap clone exit mmap read_write sched signal diff --git a/tools/testing/selftests/arm64/morello/freestanding.c b/tools/testing/selftests/arm64/morello/freestanding.c index 45c0fa8b0914..81599201928d 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.c +++ b/tools/testing/selftests/arm64/morello/freestanding.c @@ -185,3 +185,10 @@ int printf(const char *fmt, ...) return written; }

+void *memcpy(void *dest, const void *src, size_t n) +{

• for (size_t i = 0; i < n; i++)

• ((char *)dest)[i] = ((char *)src)[i];
  

• return dest;

+}

That's not good enough to preserve capabilities I'm afraid. We've got away without an memcpy() implementation so far (potentially relying on Clang providing its inline implementation), but if we do provide an implementation, it needs to be correct. Could we somehow reuse the code imported by "arm64: morello: Use the Morello optimized routine for memcpy"? Failing that, duplicating the implementation here would be acceptable. Either way this belongs to a separate patch.

Note that memmove() should also be implementing in a tag-preserving way (which can be the same as for memcpy(), like in the commit mentioned above), not sure why the linker complains only about memcpy().

Kevin