> Date: Tue, 22 Feb 2022 16:59:24 +0000
> From: Visa Hankala <[email protected]>
>
> 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 <[email protected]>
> > >
> > > 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.
>
> I have not tested the following patch yet. It takes a while
> to compile...
I think this looks much better.
> 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 16:50:53 -0000
> @@ -64,6 +64,7 @@ typedef struct trapframe {
> register_t tf_sstatus;
> register_t tf_stval;
> register_t tf_scause;
> + register_t tf_pad;
> } trapframe_t;
>
> /*
> @@ -85,6 +86,7 @@ struct sigframe {
> struct switchframe {
> register_t sf_s[12];
> register_t sf_ra;
> + register_t sf_pad;
> };
>
> struct callframe {
> 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 16:50:53 -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);
>
