On Thursday, 17 September 2026 at 13:49, Diego Meretta <[email protected]>
wrote:
> * riscv64/riscv64/locore.S: Replace x86 trap stubs with RISC-V trap entry and
> exit.
> The alltraps entry saves all 31 GPs and 4 CSRs (scause, stval, sepc,
> sstatus)
> into a riscv64_saved_state frame on the kernel stack and calls
> riscv64_trap().
> The return path restores CSRs and registers, then executes sret.
> thread_bootstrap_return, thread_exception_return, and thread_syscall_return
> are implemented as wrappers that jump to the common return path.
> * riscv64/riscv64/trap.c: Remove dead x86 code from riscv64_exception stub
> and implement interrupted_pc to return regs->sepc.
> ---
> riscv64/riscv64/locore.S | 173 ++++++++++++++++++++++++++++-----------
> riscv64/riscv64/trap.c | 14 +---
> 2 files changed, 130 insertions(+), 57 deletions(-)
Hi Diego,
Thanks for the patch! However, I was waiting for the Sv39 implementation
first, since we have not yet established the final high address kernel
stack, sscratch, and trap/vector contracts that this code depends on.
Beyond that, the patch does not link as submitted due to missing symbols,
so it can unfortunately not be merged as is. More below.
>
> diff --git a/riscv64/riscv64/locore.S b/riscv64/riscv64/locore.S
> index 3d0f4030..45d9c690 100644
> --- a/riscv64/riscv64/locore.S
> +++ b/riscv64/riscv64/locore.S
> @@ -168,66 +168,147 @@ ENTRY(t_debug)
> ENTRY(t_page_fault)
>
> /*
> - * All 'exceptions' enter here with:
> - * esp-> trap number
> - * error code
> - * old eip
> - * old cs
> - * old eflags
> - * old esp if trapped from user
> - * old ss if trapped from user
> + * RISC-V trap entry/exit.
> + *
> + * On entry, hardware has set:
> + * scause - trap cause
> + * stval - faulting address or instruction
> + * sepc - interrupted PC
> + * sstatus - previous status (SPP, SPIE, SIE)
> + *
> + * We build a riscv64_saved_state frame (35 * 8 = 280 bytes, padded to 288)
> + * on the kernel stack and call riscv64_trap(regs).
> + *
> + * Frame layout (offsets match struct riscv64_saved_state in thread.h):
> + * 0 t6 8 t5 16 t4 24 t3
> + * 32 s11 40 s10 48 s9 56 s8
> + * 64 s7 72 s6 80 s5 88 s4
> + * 96 s3 104 s2 112 a7 120 a6
> + * 128 a5 136 a4 144 a3 152 a2
> + * 160 a1 168 a0 176 s1 184 fp
> + * 192 t2 200 t1 208 t0 216 tp
> + * 224 gp 232 sp 240 ra
> + * 248 scause 256 stval 264 sepc 272 sstatus
> */
> +
> +#define FRAME_SIZE 288
> +
> ENTRY(alltraps)
> -trap_push_segs:
> -trap_set_segs:
> trap_from_user:
> +trap_from_kernel:
> _take_trap:
> -
> -/*
> - * Return from trap or system call, checking for ASTs.
> - * On PCB stack.
> - */
> + addi sp, sp, -FRAME_SIZE
In trap_from_user case, we are on the user stack, which we should not
use. We need to switch to the proper kernel stack first, which we could
implement after Sv39.
Also, traps from userspace and kernelspace have slightly different
constraints (i.e., we need to swap sp and sscratch for trap from
userspace). Two separate handlers (possibly with shared code) would be
more appropriate.
> +
> + sd t6, 0(sp)
> + sd t5, 8(sp)
> + sd t4, 16(sp)
> + sd t3, 24(sp)
> + sd s11, 32(sp)
> + sd s10, 40(sp)
> + sd s9, 48(sp)
> + sd s8, 56(sp)
> + sd s7, 64(sp)
> + sd s6, 72(sp)
> + sd s5, 80(sp)
> + sd s4, 88(sp)
> + sd s3, 96(sp)
> + sd s2, 104(sp)
> + sd a7, 112(sp)
> + sd a6, 120(sp)
> + sd a5, 128(sp)
> + sd a4, 136(sp)
> + sd a3, 144(sp)
> + sd a2, 152(sp)
> + sd a1, 160(sp)
> + sd a0, 168(sp)
> + sd s1, 176(sp)
> + sd fp, 184(sp)
> + sd t2, 192(sp)
> + sd t1, 200(sp)
> + sd t0, 208(sp)
> + sd tp, 216(sp)
> + sd gp, 224(sp)
> + sd ra, 240(sp)
For a trap from userspace, the complete state should be saved in the
current thread's persistent USER_REGS(thread) area. Also, the offsets
and structure size should be generated through riscv64asm.sym rather
than hardcoded. See wip-aarch64 for reference.
We might need to rework the riscv thread state struct before this, if
it contains stale x86 fields etc.
> +
> + /* Save original sp */
> + addi t0, sp, FRAME_SIZE
> + sd t0, 232(sp)
> +
> + /* Save CSRs */
> + csrr t0, scause
> + sd t0, 248(sp)
> + csrr t0, stval
> + sd t0, 256(sp)
> + csrr t0, sepc
> + sd t0, 264(sp)
> + csrr t0, sstatus
> + sd t0, 272(sp)
> +
> + /* Call C: void riscv64_trap(struct riscv64_saved_state *regs) */
> + mv a0, sp
> + call riscv64_trap
There doesn't seem to be any symbol named riscv64_trap. This fails
during link stage.
>
> _return_from_trap:
> -
> _return_to_user:
> -
> -/*
> - * Return from kernel mode to interrupted thread.
> + /* Restore sstatus and sepc */
> + ld t0, 272(sp)
> + csrw sstatus, t0
> + ld t0, 264(sp)
> + csrw sepc, t0
> +
> + /* Restore caller-saved and callee-saved registers */
> + ld t6, 0(sp)
> + ld t5, 8(sp)
> + ld t4, 16(sp)
> + ld t3, 24(sp)
> + ld s11, 32(sp)
> + ld s10, 40(sp)
> + ld s9, 48(sp)
> + ld s8, 56(sp)
> + ld s7, 64(sp)
> + ld s6, 72(sp)
> + ld s5, 80(sp)
> + ld s4, 88(sp)
> + ld s3, 96(sp)
> + ld s2, 104(sp)
> + ld a7, 112(sp)
> + ld a6, 120(sp)
> + ld a5, 128(sp)
> + ld a4, 136(sp)
> + ld a3, 144(sp)
> + ld a2, 152(sp)
> + ld a1, 160(sp)
> + ld a0, 168(sp)
> + ld s1, 176(sp)
> + ld fp, 184(sp)
> + ld t2, 192(sp)
> + ld t1, 200(sp)
> + ld t0, 208(sp)
> + ld tp, 216(sp)
> + ld gp, 224(sp)
> + ld ra, 240(sp)
> + ld sp, 232(sp)
> +
> + sret
Like above, we should use named constants.
> +END(alltraps)
> +
> +/*
> + * Return from kernel as if from an exception.
> + * Called as a C function; saved state is on the current stack.
> */
> -
> -_return_from_kernel:
> -_kret_popl_gs:
> -_kret_popl_fs:
> -_kret_popl_es:
> -_kret_popl_ds:
> -_kret_iret:
> -
> -
> -/*
> - * Trap from kernel mode. No need to switch stacks.
> - */
> -trap_from_kernel:
> -#if MACH_KDB || MACH_TTD
> -#else /* MACH_KDB || MACH_TTD */
> -#endif /* MACH_KDB || MACH_TTD */
> -
> -
> -/*
> - * Called as a function, makes the current thread
> - * return from the kernel as if from an exception.
> - */
> -
> ENTRY(thread_exception_return)
> ENTRY(thread_bootstrap_return)
> + j _return_from_trap
These are called by shared mach code, and they do not contain the trap
frame _return_from_trap expects. The routine will read invalid values
from the stack.
> +END(thread_bootstrap_return)
>
> /*
> - * Called as a function, makes the current thread
> - * return from the kernel as if from a syscall.
> - * Takes the syscall's return code as an argument.
> + * Return from syscall. a0 = return code.
> + * Stored into the saved state's a0 slot on the stack.
> */
> -
> ENTRY(thread_syscall_return)
> + sd a0, 168(sp)
> + j _return_from_trap
Same as above.
> +END(thread_syscall_return)
>
> ENTRY(call_continuation)
>
> diff --git a/riscv64/riscv64/trap.c b/riscv64/riscv64/trap.c
> index 014e48cf..4e790d68 100644
> --- a/riscv64/riscv64/trap.c
> +++ b/riscv64/riscv64/trap.c
> @@ -169,15 +169,7 @@ riscv64_exception(
> int code,
> long subcode)
> {
> - spl_t s;
> -
> - /*
> - * Turn off delayed FPU error handling.
> - */
> - panic("TODO: not implemented");
> -
> - exception(exc, code, subcode);
> - /*NOTREACHED*/
> + panic("riscv64_exception: exc=%d code=%d", exc, code);
> }
>
> #if MACH_PCSAMPLE > 0
> @@ -187,8 +179,8 @@ riscv64_exception(
> unsigned
> interrupted_pc(const thread_t t)
> {
> - panic("TODO: not implemented");
> - return 0;
> + struct riscv64_saved_state *regs = USER_REGS(t);
> + return regs->sepc;
> }
> #endif /* MACH_PCSAMPLE > 0 */
>
> --
> 2.43.0
>
>
>
Also, it seems the trap handlers are not installed in this patch. The
cover letter was describing a working trap entry/exit, perhaps I am
missing it?
Hakan
