From 012687b46e3d2d56038cd3bddbd21710df57ce57 Mon Sep 17 00:00:00 2001 From: Ville Juven Date: Tue, 30 Jul 2024 15:16:03 +0300 Subject: [PATCH 1/4] riscv/syscall: Add dependency to RISCV_PERCPU_SCRATCH when LIB_SYSCALL=y The per CPU scratch register is needed by system calls -> enable it by default. --- arch/risc-v/Kconfig | 2 +- arch/risc-v/src/common/riscv_exception_common.S | 8 ++++++++ arch/risc-v/src/common/riscv_fork.c | 4 ++-- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/arch/risc-v/Kconfig b/arch/risc-v/Kconfig index 0d0da346c7722..3f04a4738ae89 100644 --- a/arch/risc-v/Kconfig +++ b/arch/risc-v/Kconfig @@ -520,7 +520,7 @@ config RISCV_MISALIGNED_HANDLER config RISCV_PERCPU_SCRATCH bool "Enable Scratch-based Per-CPU storage" - default n + default y if LIB_SYSCALL ---help--- In some special chipsets, multiple CPUs may be bundled in one hardware thread cluster, which results in hartid and cpuindex not being exactly diff --git a/arch/risc-v/src/common/riscv_exception_common.S b/arch/risc-v/src/common/riscv_exception_common.S index 7bcf2d2d72ac9..bbec8a61b2659 100644 --- a/arch/risc-v/src/common/riscv_exception_common.S +++ b/arch/risc-v/src/common/riscv_exception_common.S @@ -55,6 +55,14 @@ # endif #endif +/* System calls require the per CPU scratch area */ + +#ifdef CONFIG_LIB_SYSCALL +# ifndef CONFIG_RISCV_PERCPU_SCRATCH +# error "CONFIG_RISCV_PERCPU_SCRATCH is needed for handling system calls" +# endif +#endif + /* Provide a default section for the exeception handler. */ #ifndef EXCEPTION_SECTION diff --git a/arch/risc-v/src/common/riscv_fork.c b/arch/risc-v/src/common/riscv_fork.c index 6b7705028c6e9..54aeca33b5849 100644 --- a/arch/risc-v/src/common/riscv_fork.c +++ b/arch/risc-v/src/common/riscv_fork.c @@ -246,9 +246,9 @@ pid_t riscv_fork(const struct fork_s *context) fregs[REG_FS11] = context->fs11; /* Saved register fs11 */ #endif -#ifdef CONFIG_BUILD_PROTECTED +#ifdef CONFIG_LIB_SYSCALL /* Forked task starts at `dispatch_syscall()`, which requires TP holding - * TCB pointer as per e6973c764c, so we please it here to support vfork. + * TCB, in this case the child's TCB is needed. */ child->cmn.xcp.regs[REG_TP] = (uintptr_t)child; From c45ba5ac03d86bc99ce69c2d8d172046d4c83a40 Mon Sep 17 00:00:00 2001 From: Ville Juven Date: Wed, 31 Jul 2024 13:32:57 +0300 Subject: [PATCH 2/4] risc-v/riscv_swint.c: Simplify implementation of dispatch_syscall Simplifies the implementation of dispatch_syscall, making it easier to understand and maintain. Let the C-compiler do most of the work, instead of doing everything as inline assembly. --- arch/risc-v/src/common/riscv_swint.c | 112 ++++++++++++++++++--------- 1 file changed, 75 insertions(+), 37 deletions(-) diff --git a/arch/risc-v/src/common/riscv_swint.c b/arch/risc-v/src/common/riscv_swint.c index 43654b5f3396e..3cae985f96695 100644 --- a/arch/risc-v/src/common/riscv_swint.c +++ b/arch/risc-v/src/common/riscv_swint.c @@ -48,16 +48,74 @@ * Pre-processor Definitions ****************************************************************************/ -#ifdef CONFIG_LIB_SYSCALL -# define TCB_FLAGS_OFFSET offsetof(struct tcb_s, flags) -#endif - /**************************************************************************** * Private Functions ****************************************************************************/ #ifdef CONFIG_LIB_SYSCALL +/**************************************************************************** + * Name: do_syscall + * + * Description: + * Call the stub function corresponding to the system call. NOTE the non- + * standard parameter passing: + * + * A0 = SYS_ call number + * A1 = parm0 + * A2 = parm1 + * A3 = parm2 + * A4 = parm3 + * A5 = parm4 + * A6 = parm5 + * + * Note: + * Do not allow the compiler to inline this function, as it does a jump to + * another procedure which can clobber any register and the compiler will + * not understand it happens. + * + ****************************************************************************/ + +static uintptr_t do_syscall(unsigned int nbr, uintptr_t parm1, + uintptr_t parm2, uintptr_t parm3, + uintptr_t parm4, uintptr_t parm5, + uintptr_t parm6) noinline_function; +static uintptr_t do_syscall(unsigned int nbr, uintptr_t parm1, + uintptr_t parm2, uintptr_t parm3, + uintptr_t parm4, uintptr_t parm5, + uintptr_t parm6) +{ + register long a0 asm("a0") = (long)(nbr); + register long a1 asm("a1") = (long)(parm1); + register long a2 asm("a2") = (long)(parm2); + register long a3 asm("a3") = (long)(parm3); + register long a4 asm("a4") = (long)(parm4); + register long a5 asm("a5") = (long)(parm5); + register long a6 asm("a6") = (long)(parm6); + + asm volatile + ( + "la t0, g_stublookup\n" /* t0=The base of the stub lookup table */ +#ifdef CONFIG_ARCH_RV32 + "slli a0, a0, 2\n" /* a0=Offset for the stub lookup table */ +#else + "slli a0, a0, 3\n" /* a0=Offset for the stub lookup table */ +#endif + "add t0, t0, a0\n" /* t0=The address in the table */ + REGLOAD " t0, 0(t0)\n" /* t0=The address of the stub for this syscall */ + "jalr ra, t0\n" /* Call the stub (modifies ra) */ + : "+r"(a0) + : "r"(a1), "r"(a2), "r"(a3), "r"(a4), "r"(a5), "r"(a6) + : "t0", "ra", "memory" + ); + + return a0; +} + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + /**************************************************************************** * Name: dispatch_syscall * @@ -87,7 +145,7 @@ uintptr_t dispatch_syscall(unsigned int nbr, uintptr_t parm1, register long a4 asm("a4") = (long)(parm4); register long a5 asm("a5") = (long)(parm5); register long a6 asm("a6") = (long)(parm6); - + register struct tcb_s *rtcb asm("tp"); uintptr_t ret; /* Valid system call ? */ @@ -99,37 +157,21 @@ uintptr_t dispatch_syscall(unsigned int nbr, uintptr_t parm1, return -ENOSYS; } - /* ra gets clobbered below, but it does not matter */ + /* Indicate that we are in a syscall handler */ - asm volatile - ( - REGLOAD " t0, %1(tp)\n" /* Load tcb->flags */ - "ori t0, t0, %2\n" /* tcb->flags |= TCB_FLAG_SYSCALL */ - REGSTORE " t0, %1(tp)\n" - "addi a0, a0, -%3\n" /* Offset a0 to account for the reserved syscalls */ - "la t0, g_stublookup\n" /* t0=The base of the stub lookup table */ -#ifdef CONFIG_ARCH_RV32 - "slli a0, a0, 2\n" /* a0=Offset for the stub lookup table */ -#else - "slli a0, a0, 3\n" /* a0=Offset for the stub lookup table */ -#endif - "add t0, t0, a0\n" /* t0=The address in the table */ - REGLOAD " t0, 0(t0)\n" /* t0=The address of the stub for this syscall */ - "jalr ra, t0\n" /* Call the stub (modifies ra) */ - REGLOAD " t0, %1(tp)\n" /* Load tcb->flags */ - "andi t0, t0, ~%2\n" /* tcb->flags &= ~TCB_FLAG_SYSCALL */ - REGSTORE " t0, %1(tp)\n" - : "+r"(a0) - : "i"(TCB_FLAGS_OFFSET), - "i"(TCB_FLAG_SYSCALL), - "i"(CONFIG_SYS_RESERVED), - "r"(a1), "r"(a2), "r"(a3), "r"(a4), "r"(a5), "r"(a6) - : "t0", "memory" - ); + rtcb->flags |= TCB_FLAG_SYSCALL; + + /* Offset a0 to account for the reserved syscalls */ - /* a0 gets clobbered below, save it locally here */ + a0 -= CONFIG_SYS_RESERVED; - ret = a0; + /* Run the system call, save return value locally */ + + ret = do_syscall(a0, a1, a2, a3, a4, a5, a6); + + /* System call is now done */ + + rtcb->flags &= ~TCB_FLAG_SYSCALL; /* Unmask any pending signals now */ @@ -139,10 +181,6 @@ uintptr_t dispatch_syscall(unsigned int nbr, uintptr_t parm1, } #endif -/**************************************************************************** - * Public Functions - ****************************************************************************/ - /**************************************************************************** * Name: riscv_swint * From aeb7447d64a93b588c542e1bbf1128a666bab9b0 Mon Sep 17 00:00:00 2001 From: Ville Juven Date: Wed, 31 Jul 2024 15:52:15 +0300 Subject: [PATCH 3/4] riscv/syscall: Fix fork() system call When executing fork() via a system call, the parent's stack gets corrupted by the child, as during exception return the child loads the parent's stack pointer from the context save area. This happens because the full parent stack (including what has been pushed during the system call) is copied to the child. What should be copied, is only the user stack of the parent (the kernel stack is not interesting). Fix this by only copying the parent's user stack to the child; and make the child return directly to userspace (not via dispatch_syscall). --- arch/risc-v/src/common/fork.S | 7 ++ .../src/common/riscv_exception_common.S | 5 +- arch/risc-v/src/common/riscv_fork.c | 95 ++++++++++++++++--- arch/risc-v/src/common/riscv_swint.c | 7 +- 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/arch/risc-v/src/common/fork.S b/arch/risc-v/src/common/fork.S index 9070e93b211d4..9134266feca0a 100644 --- a/arch/risc-v/src/common/fork.S +++ b/arch/risc-v/src/common/fork.S @@ -90,6 +90,12 @@ .type up_fork, function up_fork: + +#ifdef CONFIG_LIB_SYSCALL + /* When coming via system call, everything is in place already */ + + tail riscv_fork +#else /* Create a stack frame */ addi sp, sp, -FORK_SIZEOF @@ -148,6 +154,7 @@ up_fork: REGLOAD x1, FORK_RA_OFFSET(sp) addi sp, sp, FORK_SIZEOF ret +#endif .size up_fork, .-up_fork .end diff --git a/arch/risc-v/src/common/riscv_exception_common.S b/arch/risc-v/src/common/riscv_exception_common.S index bbec8a61b2659..4f7064216bb0a 100644 --- a/arch/risc-v/src/common/riscv_exception_common.S +++ b/arch/risc-v/src/common/riscv_exception_common.S @@ -155,6 +155,7 @@ exception_common: csrr tp, CSR_SCRATCH /* Load kernel TP */ REGLOAD tp, RISCV_PERCPU_TCB(tp) + mv a7, sp /* a7 = context */ call x1, dispatch_syscall /* Dispatch the system call */ return_from_syscall: @@ -239,11 +240,7 @@ return_from_exception: load_ctx sp -#ifdef CONFIG_ARCH_KERNEL_STACK REGLOAD sp, REG_SP(sp) /* restore original sp */ -#else - addi sp, sp, XCPTCONTEXT_SIZE -#endif /* Return from exception */ diff --git a/arch/risc-v/src/common/riscv_fork.c b/arch/risc-v/src/common/riscv_fork.c index 54aeca33b5849..9187cc6ab5ef7 100644 --- a/arch/risc-v/src/common/riscv_fork.c +++ b/arch/risc-v/src/common/riscv_fork.c @@ -39,6 +39,8 @@ #include "sched/sched.h" +#ifdef CONFIG_ARCH_HAVE_FORK + /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ @@ -96,7 +98,80 @@ * ****************************************************************************/ -#ifdef CONFIG_ARCH_HAVE_FORK +#ifdef CONFIG_LIB_SYSCALL + +pid_t riscv_fork(const struct fork_s *context) +{ + struct tcb_s *parent = this_task(); + struct task_tcb_s *child; + uintptr_t newsp; + uintptr_t newtop; + uintptr_t stacktop; + uintptr_t stackutil; +#ifdef CONFIG_SCHED_THREAD_LOCAL + uintptr_t tp; +#endif + UNUSED(context); + + /* Allocate and initialize a TCB for the child task. */ + + child = nxtask_setup_fork((start_t)parent->xcp.regs[REG_RA]); + if (!child) + { + sinfo("nxtask_setup_fork failed\n"); + return (pid_t)ERROR; + } + + /* Copy parent user stack to child */ + + stacktop = (uintptr_t)parent->stack_base_ptr + parent->adj_stack_size; + DEBUGASSERT(stacktop > parent->xcp.regs[REG_SP]); + stackutil = stacktop - parent->xcp.regs[REG_SP]; + + /* Copy the parent stack contents (overwrites child's SP and TP) */ + + newtop = (uintptr_t)child->cmn.stack_base_ptr + child->cmn.adj_stack_size; + newsp = newtop - stackutil; + +#ifdef CONFIG_SCHED_THREAD_LOCAL + /* Save child's thread pointer */ + + tp = child->cmn.xcp.regs[REG_TP]; +#endif + + /* Set up frame for context and copy the parent's user context there */ + + memcpy((void *)(newsp - XCPTCONTEXT_SIZE), + parent->xcp.regs, XCPTCONTEXT_SIZE); + + /* Save FPU */ + + riscv_savefpu(child->cmn.xcp.regs, riscv_fpuregs(&child->cmn)); + + /* Copy the parent stack contents */ + + memcpy((void *)newsp, (const void *)parent->xcp.regs[REG_SP], stackutil); + + /* Set the new register restore area to the new stack top */ + + child->cmn.xcp.regs = (void *)(newsp - XCPTCONTEXT_SIZE); + + /* Return 0 to child */ + + child->cmn.xcp.regs[REG_A0] = 0; + child->cmn.xcp.regs[REG_SP] = newsp; +#ifdef CONFIG_SCHED_THREAD_LOCAL + child->cmn.xcp.regs[REG_TP] = tp; +#endif + + /* And, finally, start the child task. On a failure, nxtask_start_fork() + * will discard the TCB by calling nxtask_abort_fork(). + */ + + return nxtask_start_fork(child); +} + +#else pid_t riscv_fork(const struct fork_s *context) { @@ -171,14 +246,19 @@ pid_t riscv_fork(const struct fork_s *context) newtop = (uintptr_t)child->cmn.stack_base_ptr + child->cmn.adj_stack_size; newsp = newtop - stackutil; - /* Set up frame for context */ + /* Set up frame for context and copy the initial context there */ memcpy((void *)(newsp - XCPTCONTEXT_SIZE), child->cmn.xcp.regs, XCPTCONTEXT_SIZE); - child->cmn.xcp.regs = (void *)(newsp - XCPTCONTEXT_SIZE); + /* Copy the parent stack contents (overwrites child's SP and TP) */ + memcpy((void *)newsp, (const void *)(uintptr_t)context->sp, stackutil); + /* Set the new register restore area to the new stack top */ + + child->cmn.xcp.regs = (void *)(newsp - XCPTCONTEXT_SIZE); + /* Was there a frame pointer in place before? */ #ifdef CONFIG_RISCV_FRAMEPOINTER @@ -246,14 +326,6 @@ pid_t riscv_fork(const struct fork_s *context) fregs[REG_FS11] = context->fs11; /* Saved register fs11 */ #endif -#ifdef CONFIG_LIB_SYSCALL - /* Forked task starts at `dispatch_syscall()`, which requires TP holding - * TCB, in this case the child's TCB is needed. - */ - - child->cmn.xcp.regs[REG_TP] = (uintptr_t)child; -#endif - /* And, finally, start the child task. On a failure, nxtask_start_fork() * will discard the TCB by calling nxtask_abort_fork(). */ @@ -261,4 +333,5 @@ pid_t riscv_fork(const struct fork_s *context) return nxtask_start_fork(child); } +#endif /* CONFIG_LIB_SYSCALL */ #endif /* CONFIG_ARCH_HAVE_FORK */ diff --git a/arch/risc-v/src/common/riscv_swint.c b/arch/risc-v/src/common/riscv_swint.c index 3cae985f96695..aebeef1cb9c28 100644 --- a/arch/risc-v/src/common/riscv_swint.c +++ b/arch/risc-v/src/common/riscv_swint.c @@ -130,13 +130,14 @@ static uintptr_t do_syscall(unsigned int nbr, uintptr_t parm1, * A4 = parm3 * A5 = parm4 * A6 = parm5 + * A7 = context (aka SP) * ****************************************************************************/ uintptr_t dispatch_syscall(unsigned int nbr, uintptr_t parm1, uintptr_t parm2, uintptr_t parm3, uintptr_t parm4, uintptr_t parm5, - uintptr_t parm6) + uintptr_t parm6, void *context) { register long a0 asm("a0") = (long)(nbr); register long a1 asm("a1") = (long)(parm1); @@ -157,6 +158,10 @@ uintptr_t dispatch_syscall(unsigned int nbr, uintptr_t parm1, return -ENOSYS; } + /* Set the user register context to TCB */ + + rtcb->xcp.regs = context; + /* Indicate that we are in a syscall handler */ rtcb->flags |= TCB_FLAG_SYSCALL; From fe61344f91cedbcd71eddf1909ada854a3dc9769 Mon Sep 17 00:00:00 2001 From: Ville Juven Date: Tue, 6 Aug 2024 10:06:45 +0300 Subject: [PATCH 4/4] rv-virt/pnsh: Refresh PROTECTED mode configs --- boards/risc-v/qemu-rv/rv-virt/configs/pnsh/defconfig | 1 - boards/risc-v/qemu-rv/rv-virt/configs/pnsh64/defconfig | 1 - 2 files changed, 2 deletions(-) diff --git a/boards/risc-v/qemu-rv/rv-virt/configs/pnsh/defconfig b/boards/risc-v/qemu-rv/rv-virt/configs/pnsh/defconfig index 8cb3545cb5ad9..2cdd56a8e7c7c 100644 --- a/boards/risc-v/qemu-rv/rv-virt/configs/pnsh/defconfig +++ b/boards/risc-v/qemu-rv/rv-virt/configs/pnsh/defconfig @@ -60,7 +60,6 @@ CONFIG_PATH_INITIAL="/system/bin" CONFIG_RAM_SIZE=3145728 CONFIG_RAM_START=0x80000000 CONFIG_READLINE_CMD_HISTORY=y -CONFIG_RISCV_PERCPU_SCRATCH=y CONFIG_RISCV_SEMIHOSTING_HOSTFS=y CONFIG_RR_INTERVAL=200 CONFIG_SCHED_WAITPID=y diff --git a/boards/risc-v/qemu-rv/rv-virt/configs/pnsh64/defconfig b/boards/risc-v/qemu-rv/rv-virt/configs/pnsh64/defconfig index f04ed94d0e898..d282e6ee5c8a8 100644 --- a/boards/risc-v/qemu-rv/rv-virt/configs/pnsh64/defconfig +++ b/boards/risc-v/qemu-rv/rv-virt/configs/pnsh64/defconfig @@ -59,7 +59,6 @@ CONFIG_PATH_INITIAL="/system/bin" CONFIG_RAM_SIZE=3145728 CONFIG_RAM_START=0x80000000 CONFIG_READLINE_CMD_HISTORY=y -CONFIG_RISCV_PERCPU_SCRATCH=y CONFIG_RISCV_SEMIHOSTING_HOSTFS=y CONFIG_RR_INTERVAL=200 CONFIG_SCHED_WAITPID=y