On Tue, Feb 22, 2022 at 07:04:23PM +0100, Mark Kettenis wrote:
> > 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...
All right, trapframe is not the right place for it. The existence of
the "variable" should be visible more clearly nevertheless.
So far the variable has occupied the padding slot after the trapframe.
With the new padding field, the slot is gone and the variable would
reside in the final 16 bytes of u-area.
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 18:20:46 -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 18:20:46 -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);
@@ -74,6 +78,7 @@ cpu_fork(struct proc *p1, struct proc *p
tf = (struct trapframe *)((u_long)p2->p_addr
+ USPACE
- sizeof(struct trapframe)
+ - sizeof(register_t) /* for holding curcpu */
- 0x10);
tf = (struct trapframe *)STACKALIGN(tf);
