> Date: Tue, 22 Feb 2022 17:45:26 +0000 > From: Visa Hankala <v...@hankala.org> > > On Tue, Feb 22, 2022 at 06:13:54PM +0100, Mark Kettenis wrote: > > > Date: Tue, 22 Feb 2022 16:59:24 +0000 > > > From: Visa Hankala <v...@hankala.org> > > > > > > On Tue, Feb 22, 2022 at 05:31:31PM +0100, Mark Kettenis wrote: > > > > > Date: Tue, 22 Feb 2022 15:58:31 +0000 > > > > > From: Visa Hankala <v...@hankala.org> > > > > > > > > > > The standard RISC-V calling convention says that the stack pointer > > > > > should be 16-byte aligned. > > > > > > > > > > The patch below corrects the alignment of the kernel stack in context > > > > > switching and exception handling. > > > > > > > > > > OK? > > > > > > > > Is there a reason why we don't simply extend struct switchframe to > > > > include another register_t such that it is a multiple of 16 bytes? > > > > > > > > (and then add a compile-time assertion somewhere to make sure we don't > > > > mess that up again) > > > > > > Well, that is another way to do it. > > > > > > > > The diff reveals that curcpu is stored in an unnamed spot near the > > > > > upper > > > > > end of u-area when running in user mode. Should struct trapframe > > > > > contain > > > > > a field where curcpu is stored, so that the usage would become more > > > > > apparent? I guess the pointer could also be stored in one of the > > > > > existing fields with an appropriate comment in the struct definition. > > > > > > > > Sorry, but I don't understand you; curpcu is kept in the tp register... > > > > > > In kernel, tp holds curcpu. In userland, tp holds TCB pointer. The > > > exception entry and exit code has to switch the register's content. > > > curcpu is stored in kernel stack when the CPU runs userland. > > > > Ah, right. So RISC-V doesn't have a special register to hold this. I > > don't think adding a field to struct trapframe would make this easier > > to understand. > > Here is a version that shows how a curcpu field would look like, > now that a new field is being added anyway. > > Still not tested...
The weird thing about that is that curcpu doesn't actually live in the trapframe. And it would only live at that location for a userland -> kernel trapframe. So I think this would make things more confusing... > Index: arch/riscv64/include/frame.h > =================================================================== > RCS file: src/sys/arch/riscv64/include/frame.h,v > retrieving revision 1.2 > diff -u -p -r1.2 frame.h > --- arch/riscv64/include/frame.h 12 May 2021 01:20:52 -0000 1.2 > +++ arch/riscv64/include/frame.h 22 Feb 2022 17:40:43 -0000 > @@ -64,6 +64,8 @@ typedef struct trapframe { > register_t tf_sstatus; > register_t tf_stval; > register_t tf_scause; > + /* Holds curcpu when the CPU runs in user mode. */ > + register_t tf_curcpu; > } trapframe_t; > > /* > @@ -85,6 +87,7 @@ struct sigframe { > struct switchframe { > register_t sf_s[12]; > register_t sf_ra; > + register_t sf_pad; > }; > > struct callframe { > Index: arch/riscv64/riscv64/exception.S > =================================================================== > RCS file: src/sys/arch/riscv64/riscv64/exception.S,v > retrieving revision 1.5 > diff -u -p -r1.5 exception.S > --- arch/riscv64/riscv64/exception.S 20 May 2021 04:22:33 -0000 1.5 > +++ arch/riscv64/riscv64/exception.S 22 Feb 2022 17:40:43 -0000 > @@ -54,7 +54,7 @@ > > /* Load our pcpu */ > sd tp, (TF_TP)(sp) > - ld tp, (TRAPFRAME_SIZEOF)(sp) > + ld tp, (TF_CURCPU)(sp) > .endif > > sd t0, (TF_T + 0 * 8)(sp) > @@ -142,7 +142,7 @@ > csrw sscratch, t0 > > /* Store our pcpu */ > - sd tp, (TRAPFRAME_SIZEOF)(sp) > + sd tp, (TF_CURCPU)(sp) > ld tp, (TF_TP)(sp) > > /* And restore the user's global pointer */ > Index: arch/riscv64/riscv64/genassym.cf > =================================================================== > RCS file: src/sys/arch/riscv64/riscv64/genassym.cf,v > retrieving revision 1.6 > diff -u -p -r1.6 genassym.cf > --- arch/riscv64/riscv64/genassym.cf 2 Jul 2021 10:42:22 -0000 1.6 > +++ arch/riscv64/riscv64/genassym.cf 22 Feb 2022 17:40:43 -0000 > @@ -36,6 +36,7 @@ member tf_sepc > member tf_sstatus > member tf_stval > member tf_scause > +member tf_curcpu > > struct switchframe > member sf_s > Index: arch/riscv64/riscv64/vm_machdep.c > =================================================================== > RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v > retrieving revision 1.8 > diff -u -p -r1.8 vm_machdep.c > --- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 -0000 1.8 > +++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 17:40:43 -0000 > @@ -62,6 +62,10 @@ cpu_fork(struct proc *p1, struct proc *p > struct trapframe *tf; > struct switchframe *sf; > > + /* Ensure proper stack alignment. */ > + CTASSERT((sizeof(struct trapframe) & STACKALIGNBYTES) == 0); > + CTASSERT((sizeof(struct switchframe) & STACKALIGNBYTES) == 0); > + > /* Save FPU state to PCB if necessary. */ > if (pcb1->pcb_flags & PCB_FPU) > fpu_save(p1, pcb1->pcb_tf); >