From 4ad59b360c7dba9bb293a1f54d65ce17a28cc1c8 Mon Sep 17 00:00:00 2001 From: Bradley Bolen Date: Thu, 28 Oct 2021 16:25:36 -0400 Subject: [PATCH 1/3] arch: arm: core: aarch32: Fix the syscall design for Cortex-R When calling a syscall, the SVC routine will now elevate the thread to privileged mode and exit the SVC setting the return address to the syscall handler. When the thread is swapped back in, it will be running z_do_arm_syscall in system mode. That function will run the syscall then automatically return the thread to usr mode. This allows running the syscall in sys mode on a thread so that we can use syscalls that sleep without doing unnatural things. The previous implementation would enable interrupts while still in the SVC call and do weird things with the nesting count. An interrupt could happen during this time when the syscall was still in the exception state, but the nested count had been decremented too soon. Correctness of the nested count is important for future floating point unit work. The Cortex-R behavior now matches that of Cortex-M. Signed-off-by: Bradley Bolen --- arch/arm/core/aarch32/swap_helper.S | 84 +++++++-------------- arch/arm/core/aarch32/userspace.S | 109 ++++++++-------------------- 2 files changed, 56 insertions(+), 137 deletions(-) diff --git a/arch/arm/core/aarch32/swap_helper.S b/arch/arm/core/aarch32/swap_helper.S index b8c70a0f3954a..3ab3cb869739b 100644 --- a/arch/arm/core/aarch32/swap_helper.S +++ b/arch/arm/core/aarch32/swap_helper.S @@ -646,6 +646,7 @@ svc_system_thread: srsdb #MODE_SYS! cps #MODE_SYS push {r0-r3, r12, lr} + mov ip, sp cps #MODE_SVC /* @@ -723,20 +724,14 @@ _oops: #if defined(CONFIG_USERSPACE) /* * System call will setup a jump to the _do_arm_syscall function - * when the SVC returns via the bx lr. + * running in system mode when returning from the exception. * * There is some trickery involved here because we have to preserve * the original PC value so that we can return back to the caller of * the SVC. * * On SVC exception, the USER/SYSTEM stack looks like the following: - * - * sp+0: r0 - * sp+4: r1 - * sp+8: r2 - * sp+12: r3 - * sp+16: r12 - * sp+20: LR_svc (address of opcode just following SVC opcode ) + * r0 - r1 - r2 - r3 - r12 - LR - { possible FPU space } - PC - SPSR * * Registers look like: * r0 - arg1 @@ -749,6 +744,26 @@ _oops: * r8 - saved link register */ _do_syscall: + ldr r8, [ip, #24] /* grab address of LR from stack frame */ + + /* Make the exception return to system state */ + ldr r1, [ip, #28] + + /* If leaving thumb mode, set the return address to thumb mode */ + tst r1, #T_BIT + orrne r8, #1 + + bic r1, #(MODE_MASK | T_BIT) + orr r1, r1, #MODE_SYS + str r1, [ip, #28] + + /* + * Store the address of z_arm_do_syscall for the exit so the exception + * return goes there in system state. + */ + ldr r1, =z_arm_do_syscall + str r1, [ip, #24] /* overwrite the LR to point to z_arm_do_syscall */ + /* validate syscall limit, only set priv mode if valid */ ldr ip, =K_SYSCALL_LIMIT cmp r6, ip @@ -761,7 +776,6 @@ _do_syscall: ldr r6, =K_SYSCALL_BAD valid_syscall_id: - push {r0, r1} ldr r0, =_kernel ldr r0, [r0, #_kernel_offset_to_current] ldr r1, [r0, #_thread_offset_to_mode] @@ -776,56 +790,8 @@ valid_syscall_id: */ isb - /* - * restore r0-r3 from supervisor stack before changing to system mode. - * r0,r1 saved just after valid_syscall_id - * r2,r3 saved just after z_arm_svc - */ - pop {r0-r3} - - add sp,sp,r3 /* un-do stack pointer alignment to double-word boundary */ - - /* Switch to system mode */ - cps #MODE_SYS - - /* - * Restore the nested level. The thread that is doing the system call may - * be put to sleep, as in the case of waiting in k_msgq_get() with - * K_FOREVER, so we don't want the nesting level to be elevated during - * that complete time. - */ - ldr r2, =_kernel - ldr r1, [r2, #_kernel_offset_to_nested] - sub r1, r1, #1 - str r1, [r2, #_kernel_offset_to_nested] - - /* - * restore r0-r3 from stack since we've used them above during demux - */ - ldr r0, [sp, #0] - ldr r1, [sp, #4] - ldr r2, [sp, #8] - ldr r3, [sp, #12] - - /* - * grab return address from USER/SYSTEM stack frame - * (just past the SVC opcode) - */ - ldr r8, [sp, #20] - - /* - * User stack left with: - * - * sp: r0 - * sp+4: r1 - * sp+8: r2 - * sp+12: r3 - * sp+16: r12 - * sp+20: LR_svc (address of opcode just following SVC opcode ) - */ - - /* branch to _arm_do_syscall. We will not return here. */ - b z_arm_do_syscall + /* Return to _arm_do_syscall in system state. */ + b z_arm_int_exit #endif GTEXT(z_arm_cortex_r_svc) diff --git a/arch/arm/core/aarch32/userspace.S b/arch/arm/core/aarch32/userspace.S index c098f9c7e2f06..83b9371375a12 100644 --- a/arch/arm/core/aarch32/userspace.S +++ b/arch/arm/core/aarch32/userspace.S @@ -351,7 +351,7 @@ SECTION_FUNC(TEXT, z_arm_do_syscall) /* Restore user stack and original r0, r1 */ pop {r0, r1} -#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ +#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) /* setup privileged stack */ ldr ip, =_kernel ldr ip, [ip, #_kernel_offset_to_current] @@ -362,16 +362,19 @@ SECTION_FUNC(TEXT, z_arm_do_syscall) subs ip, #8 str sp, [ip, #0] str lr, [ip, #4] -#elif defined(CONFIG_CPU_CORTEX_R) - /* Store current LR at the beginning of the priv stack */ - push {lr} -#endif - -#if !defined(CONFIG_CPU_CORTEX_R) +#elif defined(CONFIG_ARMV7_R) /* - * switch to privileged stack - * The stack switch happens on exception entry for Cortex-R + * The SVC handler has already switched to the privileged stack. + * Store the user SP and LR at the beginning of the priv stack. */ + ldr ip, =_kernel + ldr ip, [ip, #_kernel_offset_to_current] + ldr ip, [ip, #_thread_offset_to_sp_usr] + push {ip, lr} +#endif + +#if !defined(CONFIG_ARMV7_R) + /* switch to privileged stack */ msr PSP, ip #endif @@ -446,7 +449,7 @@ dispatch_syscall: mov r0, ip #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) ldr ip, =K_SYSCALL_BAD cmp r6, ip bne valid_syscall @@ -454,11 +457,7 @@ dispatch_syscall: /* BAD SYSCALL path */ /* fixup stack frame on the privileged stack, adding ssf */ mov ip, sp -#if defined(CONFIG_CPU_CORTEX_R) - push {r4,r5,ip} -#else push {r4,r5,ip,lr} -#endif b dispatch_syscall valid_syscall: @@ -472,36 +471,13 @@ dispatch_syscall: add ip, r6 ldr ip, [ip] /* load table address */ -#if defined(CONFIG_CPU_CORTEX_R) - /* - * We can only be in this system call handling code if interrupts were - * enabled. This is because we would only come down this path if we were - * actively running in user state, and user state CANNOT disable external - * interrupts via irq_lock(). We want external interrupts enabled while - * running the system call handler, so we can blindly enable them now, and - * disable them afterwards. - */ - cpsie i -#endif - /* execute function from dispatch table */ blx ip -#if defined(CONFIG_CPU_CORTEX_R) - /* - * for same reasoning as above: we now disable external interrupts. - */ - cpsid i - - /* restore LR */ - ldr lr, [sp,#12] -#else /* restore LR */ ldr lr, [sp,#16] #endif -#endif - #if defined(CONFIG_BUILTIN_STACK_GUARD) /* @@ -545,9 +521,12 @@ dispatch_syscall: /* Restore r0 */ mov r0, ip -#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ +#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) /* set stack back to unprivileged stack */ ldr ip, [sp,#12] +#endif + +#if !defined(CONFIG_ARMV7_R) msr PSP, ip #endif @@ -574,7 +553,8 @@ dispatch_syscall: orrs r2, r2, r3 msr CONTROL, r2 pop {r2, r3} -#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) +#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ + || defined(CONFIG_CPU_CORTEX_R) ldr r0, =_kernel ldr r0, [r0, #_kernel_offset_to_current] ldr r1, [r0, #_thread_offset_to_mode] @@ -582,10 +562,12 @@ dispatch_syscall: /* Store (unprivileged) mode in thread's mode state variable */ str r1, [r0, #_thread_offset_to_mode] dsb +#if defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) /* drop privileges by setting bit 0 in CONTROL */ mrs ip, CONTROL orrs ip, ip, #1 msr CONTROL, ip +#endif #endif /* ISB is not strictly necessary here (stack pointer is not being @@ -636,42 +618,10 @@ dispatch_syscall: */ mov ip, r8 orrs ip, ip, #1 - -#endif - -#if defined(CONFIG_CPU_CORTEX_R) - /* - * The stack contains (from top) - * spsr lr lr_svc r12 r3 r2 r1 r0 lr sp r5 r4 - * Unwind everything except the return state that will be used for rfeia. - */ - add sp, sp, #(8*4) - ldmia sp!, {r12,lr} - pop {r2, r3} - - cps #MODE_SVC - - /* - * Restore lr_svc stored into the SVC mode stack by the mode entry - * function. This ensures that the return address of the interrupted - * context is preserved in case of interrupt nesting. - */ - pop {lr} - - /* - * Move the return state from the privileged stack to the service - * stack. We need to put the user stack back in $sp, but we cannot - * trust the user stack. Therefore, put the return state on the svc - * stack and return from there. - */ - push {r2, r3} - +#elif defined(CONFIG_ARMV7_R) /* Restore user stack pointer */ - ldr r1, =_kernel - ldr r1, [r1, #_kernel_offset_to_current] - cps #MODE_SYS - ldr sp, [r1, #_thread_offset_to_sp_usr] /* sp_usr */ - cps #MODE_SVC + ldr ip, [sp,#12] + mov sp, ip /* Zero out volatile (caller-saved) registers so as to not leak state from * kernel mode. The C calling convention for the syscall handler will @@ -681,12 +631,15 @@ dispatch_syscall: mov r2, #0 mov r3, #0 - /* return from SVC state to user state. */ - rfeia sp! -#else - bx ip + /* + * return back to original function that called SVC + */ + mov ip, r8 + cps #MODE_USR #endif + bx ip + /* * size_t arch_user_string_nlen(const char *s, size_t maxsize, int *err_arg) From 8f8b22a8426c8a2f0ad4ffe5389a919a6ef0d0fe Mon Sep 17 00:00:00 2001 From: Bradley Bolen Date: Fri, 29 Oct 2021 16:40:03 -0400 Subject: [PATCH 2/3] arch: arm: core: aarch32: Change Cortex-R config check Replace CONFIG_CPU_CORTEX_R with CONFIG_ARMV7_R since it is clearer with respect to the difference between v7 and v8 Cortex-R. Signed-off-by: Bradley Bolen --- arch/arm/core/aarch32/userspace.S | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/arch/arm/core/aarch32/userspace.S b/arch/arm/core/aarch32/userspace.S index 83b9371375a12..904453ca4946c 100644 --- a/arch/arm/core/aarch32/userspace.S +++ b/arch/arm/core/aarch32/userspace.S @@ -14,7 +14,7 @@ #include -#if defined(CONFIG_CPU_CORTEX_R) +#if defined(CONFIG_ARMV7_R) #include #endif @@ -63,7 +63,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) ldr r0, [r0, #_thread_offset_to_priv_stack_start] /* priv stack ptr */ ldr ip, =CONFIG_PRIVILEGED_STACK_SIZE add r0, r0, ip -#elif defined(CONFIG_CPU_CORTEX_R) +#elif defined(CONFIG_ARMV7_R) ldr r0, [r0, #_thread_offset_to_priv_stack_start] /* priv stack ptr */ ldr ip, =CONFIG_PRIVILEGED_STACK_SIZE add r0, r0, ip @@ -79,7 +79,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) */ mov ip, sp -#if defined(CONFIG_CPU_CORTEX_R) +#if defined(CONFIG_ARMV7_R) mov sp, r0 #else /* set stack to privileged stack @@ -113,7 +113,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) mov r1, ip push {r0,r1} #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) push {r0,ip} #endif @@ -145,7 +145,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) push {r0,r3} #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) pop {r0,ip} /* load up stack info from user stack */ @@ -169,7 +169,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) pop {r0, r1} mov ip, r1 #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) pop {r0,ip} #endif @@ -184,11 +184,11 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) mov lr, r4 mov r4, ip #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) pop {r1,r2,r3,lr} #endif -#if defined(CONFIG_CPU_CORTEX_R) +#if defined(CONFIG_ARMV7_R) /* * set stack to user stack. We are in SYSTEM state, so r13 and r14 are * shared with USER state @@ -244,7 +244,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) /* restore r0 */ mov r0, lr -#if defined(CONFIG_CPU_CORTEX_R) +#if defined(CONFIG_ARMV7_R) /* change processor mode to unprivileged, with all interrrupts enabled. */ msr CPSR_c, #MODE_USR #else @@ -296,7 +296,7 @@ SECTION_FUNC(TEXT,z_arm_userspace_enter) mov ip, r0 pop {r0, r1} #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) ldr ip, =z_thread_entry #endif bx ip @@ -554,7 +554,7 @@ dispatch_syscall: msr CONTROL, r2 pop {r2, r3} #elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) \ - || defined(CONFIG_CPU_CORTEX_R) + || defined(CONFIG_ARMV7_R) ldr r0, =_kernel ldr r0, [r0, #_kernel_offset_to_current] ldr r1, [r0, #_thread_offset_to_mode] @@ -665,7 +665,7 @@ z_arm_user_string_nlen_fault_start: ldrb r5, [r0, r3] z_arm_user_string_nlen_fault_end: -#if defined(CONFIG_CPU_CORTEX_R) +#if defined(CONFIG_ARMV7_R) cmp r5, #0 beq strlen_done From 29b665c36044c1e5bec15dbe4d39db135f17d921 Mon Sep 17 00:00:00 2001 From: Bradley Bolen Date: Wed, 22 Dec 2021 20:11:21 -0500 Subject: [PATCH 3/3] arch: arm: core: aarch32: Use cmsis functions These functions help the code to be more self-documenting. Use them to make the code's intent clearer. Signed-off-by: Bradley Bolen --- arch/arm/core/aarch32/mpu/arm_mpu.c | 20 ++++++++++---------- include/arch/arm/aarch32/cortex_a_r/cpu.h | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/arch/arm/core/aarch32/mpu/arm_mpu.c b/arch/arm/core/aarch32/mpu/arm_mpu.c index 30e9098d274ef..04173b02b7894 100644 --- a/arch/arm/core/aarch32/mpu/arm_mpu.c +++ b/arch/arm/core/aarch32/mpu/arm_mpu.c @@ -146,12 +146,12 @@ void arm_core_mpu_enable(void) { uint32_t val; - __asm__ volatile ("mrc p15, 0, %0, c1, c0, 0" : "=r" (val) ::); - val |= SCTRL_MPU_ENABLE; + val = __get_SCTLR(); + val |= SCTLR_MPU_ENABLE; /* Make sure that all the registers are set before proceeding */ - __asm__ volatile ("dsb"); - __asm__ volatile ("mcr p15, 0, %0, c1, c0, 0" :: "r" (val) :); - __asm__ volatile ("isb"); + __DSB(); + __set_SCTLR(val); + __ISB(); } /** @@ -161,12 +161,12 @@ void arm_core_mpu_disable(void) { uint32_t val; - __asm__ volatile ("mrc p15, 0, %0, c1, c0, 0" : "=r" (val) ::); - val &= ~SCTRL_MPU_ENABLE; + val = __get_SCTLR(); + val &= ~SCTLR_MPU_ENABLE; /* Force any outstanding transfers to complete before disabling MPU */ - __asm__ volatile ("dsb"); - __asm__ volatile ("mcr p15, 0, %0, c1, c0, 0" :: "r" (val) :); - __asm__ volatile ("isb"); + __DSB(); + __set_SCTLR(val); + __ISB(); } #else /** diff --git a/include/arch/arm/aarch32/cortex_a_r/cpu.h b/include/arch/arm/aarch32/cortex_a_r/cpu.h index 744d789d853f6..3354ad678c027 100644 --- a/include/arch/arm/aarch32/cortex_a_r/cpu.h +++ b/include/arch/arm/aarch32/cortex_a_r/cpu.h @@ -12,9 +12,9 @@ #endif /* - * SCTRL register bit assignments + * SCTLR register bit assignments */ -#define SCTRL_MPU_ENABLE (1 << 0) +#define SCTLR_MPU_ENABLE (1 << 0) #define MODE_USR 0x10 #define MODE_FIQ 0x11