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 <[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.
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...
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);
