Hi Hakan, Thank you for the detailed review. You are right — I jumped ahead to the trap entry without first establishing the Sv39 foundation that it depends on. I should have focused on Sv39 and the kernel stack/sscratch contract before submitting the trap assembly.
I will prepare the Sv39 patches first. Regards, Diego El jue, 17 sept 2026 a la(s) 3:20 p.m., Hakan Candar ([email protected]) escribió: > 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 >
