Date: Tue, 4 Feb 2025 17:30:24 GMT From: Jessica Clarke <jrtc27@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 21c534b9f5d5 - main - riscv: Fix and generalise saving TP (PCPU pointer) whilst in userspace Message-ID: <202502041730.514HUOrA014930@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by jrtc27: URL: https://cgit.FreeBSD.org/src/commit/?id=21c534b9f5d5de07137f9a42e016876dd6ae66c1 commit 21c534b9f5d5de07137f9a42e016876dd6ae66c1 Author: Jessica Clarke <jrtc27@FreeBSD.org> AuthorDate: 2025-02-04 17:28:42 +0000 Commit: Jessica Clarke <jrtc27@FreeBSD.org> CommitDate: 2025-02-04 17:28:42 +0000 riscv: Fix and generalise saving TP (PCPU pointer) whilst in userspace In cpu_fork, we allocate sizeof(struct pcb) + sizeof(struct trapframe) space on the stack, then round it for stack alignment. This not only fails to include the space needed for TP but also doesn't round up the trapframe allocation to be stack-aligned, yet TF_SIZE does, as is the expectation of fork_trampoline and cpu_exception_handler. Given that sizeof(struct pcb) + sizeof(struct trapframe) is a multiple of 16, this causes the saved TP to be stored in the PCB's pcb_sp (the intended trapframe padding aliasing pcb_ra), which is probably harmless in practice as the PCB isn't expected to be current, but definitely not intended. In cpu_thread_alloc, we do include the 8 bytes for TP and then stack align that. This again fails to include the padding for trapframe as present in TF_SIZE, but sizeof(struct pcb) + sizeof(struct trapframe) happens to be a multiple of 16, as above, so adding 8 then rounding to stack alignment (16) includes an extra 8 bytes of padding, giving the right result for the wrong reason (and could be broken by future struct growth). Extract the calculation into a shared function that rounds correctly regardless of the struct layouts. Also introduce a new struct kernframe to encapsulate and clearly document this shared state rather than using magic constants, and also enable it to be easily extended in future, as we intend to do downstream in CheriBSD. Reviewed by: jhb MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D48799 --- sys/riscv/include/frame.h | 14 ++++++++++++++ sys/riscv/riscv/exception.S | 4 ++-- sys/riscv/riscv/genassym.c | 4 +++- sys/riscv/riscv/swtch.S | 2 +- sys/riscv/riscv/vm_machdep.c | 35 ++++++++++++++++++++++++----------- 5 files changed, 44 insertions(+), 15 deletions(-) diff --git a/sys/riscv/include/frame.h b/sys/riscv/include/frame.h index f134842d1c45..c70a3a48ee0a 100644 --- a/sys/riscv/include/frame.h +++ b/sys/riscv/include/frame.h @@ -57,6 +57,10 @@ struct trapframe { uint64_t tf_scause; }; +#ifdef _KERNEL +#define TF_SIZE (roundup2(sizeof(struct trapframe), STACKALIGNBYTES + 1)) +#endif + /* * Signal frame. Pushed onto user stack before calling sigcode. */ @@ -65,6 +69,16 @@ struct sigframe { ucontext_t sf_uc; /* actual saved ucontext */ }; +#ifdef _KERNEL +/* + * Kernel frame. Reserved near the top of kernel stacks for saving kernel + * state while in userspace. + */ +struct kernframe { + register_t kf_tp; +}; +#endif + #endif /* !LOCORE */ /* Definitions for syscalls */ diff --git a/sys/riscv/riscv/exception.S b/sys/riscv/riscv/exception.S index 8acb5cd691d4..eb8d92b55d05 100644 --- a/sys/riscv/riscv/exception.S +++ b/sys/riscv/riscv/exception.S @@ -53,7 +53,7 @@ .option pop /* Load our pcpu */ - ld tp, (TF_SIZE)(sp) + ld tp, (TF_SIZE + KF_TP)(sp) .endif sd t0, (TF_T + 0 * 8)(sp) @@ -139,7 +139,7 @@ csrw sscratch, t0 /* Store our pcpu */ - sd tp, (TF_SIZE)(sp) + sd tp, (TF_SIZE + KF_TP)(sp) ld tp, (TF_TP)(sp) /* And restore the user's global pointer */ diff --git a/sys/riscv/riscv/genassym.c b/sys/riscv/riscv/genassym.c index 77966913fd1b..c216c686db9a 100644 --- a/sys/riscv/riscv/genassym.c +++ b/sys/riscv/riscv/genassym.c @@ -85,7 +85,7 @@ ASSYM(TD_FRAME, offsetof(struct thread, td_frame)); ASSYM(TD_MD, offsetof(struct thread, td_md)); ASSYM(TD_LOCK, offsetof(struct thread, td_lock)); -ASSYM(TF_SIZE, roundup2(sizeof(struct trapframe), STACKALIGNBYTES + 1)); +ASSYM(TF_SIZE, TF_SIZE); ASSYM(TF_RA, offsetof(struct trapframe, tf_ra)); ASSYM(TF_SP, offsetof(struct trapframe, tf_sp)); ASSYM(TF_GP, offsetof(struct trapframe, tf_gp)); @@ -98,6 +98,8 @@ ASSYM(TF_STVAL, offsetof(struct trapframe, tf_stval)); ASSYM(TF_SCAUSE, offsetof(struct trapframe, tf_scause)); ASSYM(TF_SSTATUS, offsetof(struct trapframe, tf_sstatus)); +ASSYM(KF_TP, offsetof(struct kernframe, kf_tp)); + ASSYM(HYP_H_RA, offsetof(struct hypctx, host_regs.hyp_ra)); ASSYM(HYP_H_SP, offsetof(struct hypctx, host_regs.hyp_sp)); ASSYM(HYP_H_GP, offsetof(struct hypctx, host_regs.hyp_gp)); diff --git a/sys/riscv/riscv/swtch.S b/sys/riscv/riscv/swtch.S index 8312a6b5f347..cfd19a2d76d6 100644 --- a/sys/riscv/riscv/swtch.S +++ b/sys/riscv/riscv/swtch.S @@ -419,7 +419,7 @@ ENTRY(fork_trampoline) * Store our pcpup on stack, we will load it back * on kernel mode trap. */ - sd tp, (TF_SIZE)(sp) + sd tp, (TF_SIZE + KF_TP)(sp) ld tp, (TF_TP)(sp) /* Save kernel stack so we can use it doing a user trap */ diff --git a/sys/riscv/riscv/vm_machdep.c b/sys/riscv/riscv/vm_machdep.c index bd510080e02c..cf9b85493e39 100644 --- a/sys/riscv/riscv/vm_machdep.c +++ b/sys/riscv/riscv/vm_machdep.c @@ -57,6 +57,26 @@ #define TP_OFFSET 16 /* sizeof(struct tcb) */ #endif +static void +cpu_set_pcb_frame(struct thread *td) +{ + td->td_pcb = (struct pcb *)((char *)td->td_kstack + + td->td_kstack_pages * PAGE_SIZE) - 1; + + /* + * td->td_frame + TF_SIZE will be the saved kernel stack pointer whilst + * in userspace, so keep it aligned so it's also aligned when we + * subtract TF_SIZE in the trap handler (and here for the initial stack + * pointer). This also keeps the struct kernframe just afterwards + * aligned no matter what's in it or struct pcb. + * + * NB: TF_SIZE not sizeof(struct trapframe) as we need the rounded + * value to match the trap handler. + */ + td->td_frame = (struct trapframe *)(STACKALIGN( + (char *)td->td_pcb - sizeof(struct kernframe)) - TF_SIZE); +} + /* * Finish a fork operation, with process p2 nearly set up. * Copy and update the pcb, set up the stack so that the child @@ -73,13 +93,12 @@ cpu_fork(struct thread *td1, struct proc *p2, struct thread *td2, int flags) /* RISCVTODO: save the FPU state here */ - pcb2 = (struct pcb *)(td2->td_kstack + - td2->td_kstack_pages * PAGE_SIZE) - 1; + cpu_set_pcb_frame(td2); - td2->td_pcb = pcb2; + pcb2 = td2->td_pcb; bcopy(td1->td_pcb, pcb2, sizeof(*pcb2)); - tf = (struct trapframe *)STACKALIGN((struct trapframe *)pcb2 - 1); + tf = td2->td_frame; bcopy(td1->td_frame, tf, sizeof(*tf)); /* Clear syscall error flag */ @@ -91,8 +110,6 @@ cpu_fork(struct thread *td1, struct proc *p2, struct thread *td2, int flags) tf->tf_sstatus |= (SSTATUS_SPIE); /* Enable interrupts. */ tf->tf_sstatus &= ~(SSTATUS_SPP); /* User mode. */ - td2->td_frame = tf; - /* Set the return value registers for fork() */ td2->td_pcb->pcb_s[0] = (uintptr_t)fork_return; td2->td_pcb->pcb_s[1] = (uintptr_t)td2; @@ -206,11 +223,7 @@ cpu_thread_exit(struct thread *td) void cpu_thread_alloc(struct thread *td) { - - td->td_pcb = (struct pcb *)(td->td_kstack + - td->td_kstack_pages * PAGE_SIZE) - 1; - td->td_frame = (struct trapframe *)STACKALIGN( - (caddr_t)td->td_pcb - 8 - sizeof(struct trapframe)); + cpu_set_pcb_frame(td); } void
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202502041730.514HUOrA014930>