| Message ID | 20240530001733.1407654-4-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: Frame pointer fixes and enhancements | expand |
Hi Samuel, On Thu, May 30, 2024 at 8:17 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > s0 is reserved for the frame pointer, so it should not be used as a > temporary register. Clobbering the frame pointer breaks stack traces. > > - In handle_exception() and ret_from_exception(), use a2 for the saved > stack pointer. a2 is chosen because r2 is the stack pointer register. > - In ret_from_exception(), use s1 for the saved status CSR value. Avoid > clobbering s1 in the privilege mode check so it does not need to be > reloaded later in the function. > - Use s1 and s2 in ret_from_fork() instead of s0 and s1. The entire > p->thread.s array is zeroed at the beginning of copy_thread(), so the > registers do not need to be zeroed separately for kernel threads. > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > --- > > arch/riscv/kernel/entry.S | 29 ++++++++++++++--------------- > arch/riscv/kernel/process.c | 5 ++--- > 2 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index d13d1aad7649..bd1c5621df45 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -58,13 +58,13 @@ SYM_CODE_START(handle_exception) > */ > li t0, SR_SUM | SR_FS_VS > > - REG_L s0, TASK_TI_USER_SP(tp) > + REG_L a2, TASK_TI_USER_SP(tp) > csrrc s1, CSR_STATUS, t0 > csrr s2, CSR_EPC > csrr s3, CSR_TVAL > csrr s4, CSR_CAUSE > csrr s5, CSR_SCRATCH > - REG_S s0, PT_SP(sp) > + REG_S a2, PT_SP(sp) > REG_S s1, PT_STATUS(sp) > REG_S s2, PT_EPC(sp) > REG_S s3, PT_BADADDR(sp) > @@ -125,19 +125,19 @@ SYM_CODE_START_NOALIGN(ret_from_exception) > call riscv_v_context_nesting_end > #endif > > - REG_L s0, PT_STATUS(sp) > + REG_L s1, PT_STATUS(sp) > #ifdef CONFIG_RISCV_M_MODE > /* the MPP value is too large to be used as an immediate arg for addi */ > li t0, SR_MPP > - and s0, s0, t0 > + and t0, s1, t0 > #else > - andi s0, s0, SR_SPP > + andi t0, s1, SR_SPP > #endif > - bnez s0, 1f > + bnez t0, 1f > > /* Save unwound kernel stack pointer in thread_info */ > - addi s0, sp, PT_SIZE_ON_STACK > - REG_S s0, TASK_TI_KERNEL_SP(tp) > + addi t0, sp, PT_SIZE_ON_STACK > + REG_S t0, TASK_TI_KERNEL_SP(tp) > > /* Save the kernel shadow call stack pointer */ > scs_save_current > @@ -148,7 +148,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception) > */ > csrw CSR_SCRATCH, tp > 1: > - REG_L a0, PT_STATUS(sp) > /* > * The current load reservation is effectively part of the processor's > * state, in the sense that load reservations cannot be shared between > @@ -169,7 +168,7 @@ SYM_CODE_START_NOALIGN(ret_from_exception) > REG_L a2, PT_EPC(sp) > REG_SC x0, a2, PT_EPC(sp) > > - csrw CSR_STATUS, a0 > + csrw CSR_STATUS, s1 > csrw CSR_EPC, a2 > > REG_L x1, PT_RA(sp) > @@ -207,13 +206,13 @@ SYM_CODE_START_LOCAL(handle_kernel_stack_overflow) > REG_S x5, PT_T0(sp) > save_from_x6_to_x31 > > - REG_L s0, TASK_TI_KERNEL_SP(tp) > + REG_L a2, TASK_TI_KERNEL_SP(tp) > csrr s1, CSR_STATUS > csrr s2, CSR_EPC > csrr s3, CSR_TVAL > csrr s4, CSR_CAUSE > csrr s5, CSR_SCRATCH > - REG_S s0, PT_SP(sp) > + REG_S a2, PT_SP(sp) > REG_S s1, PT_STATUS(sp) > REG_S s2, PT_EPC(sp) > REG_S s3, PT_BADADDR(sp) > @@ -227,10 +226,10 @@ ASM_NOKPROBE(handle_kernel_stack_overflow) > > SYM_CODE_START(ret_from_fork) > call schedule_tail > - beqz s0, 1f /* not from kernel thread */ > + beqz s1, 1f /* not from kernel thread */ > /* Call fn(arg) */ > - move a0, s1 > - jalr s0 > + move a0, s2 > + jalr s1 > 1: > move a0, sp /* pt_regs */ > la ra, ret_from_exception > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c > index e4bc61c4e58a..5512c31e1256 100644 > --- a/arch/riscv/kernel/process.c > +++ b/arch/riscv/kernel/process.c > @@ -208,8 +208,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) > /* Supervisor/Machine, irqs on: */ > childregs->status = SR_PP | SR_PIE; > > - p->thread.s[0] = (unsigned long)args->fn; > - p->thread.s[1] = (unsigned long)args->fn_arg; > + p->thread.s[1] = (unsigned long)args->fn; > + p->thread.s[2] = (unsigned long)args->fn_arg; > } else { > *childregs = *(current_pt_regs()); > /* Turn off status.VS */ > @@ -219,7 +219,6 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) > if (clone_flags & CLONE_SETTLS) > childregs->tp = tls; > childregs->a0 = 0; /* Return value of fork() */ > - p->thread.s[0] = 0; > } > p->thread.riscv_v_flags = 0; > if (has_vector()) > -- > 2.44.1 > Reviewed-by: Andy Chiu <andy.chiu@sifive.com> Cheers, Andy
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S index d13d1aad7649..bd1c5621df45 100644 --- a/arch/riscv/kernel/entry.S +++ b/arch/riscv/kernel/entry.S @@ -58,13 +58,13 @@ SYM_CODE_START(handle_exception) */ li t0, SR_SUM | SR_FS_VS - REG_L s0, TASK_TI_USER_SP(tp) + REG_L a2, TASK_TI_USER_SP(tp) csrrc s1, CSR_STATUS, t0 csrr s2, CSR_EPC csrr s3, CSR_TVAL csrr s4, CSR_CAUSE csrr s5, CSR_SCRATCH - REG_S s0, PT_SP(sp) + REG_S a2, PT_SP(sp) REG_S s1, PT_STATUS(sp) REG_S s2, PT_EPC(sp) REG_S s3, PT_BADADDR(sp) @@ -125,19 +125,19 @@ SYM_CODE_START_NOALIGN(ret_from_exception) call riscv_v_context_nesting_end #endif - REG_L s0, PT_STATUS(sp) + REG_L s1, PT_STATUS(sp) #ifdef CONFIG_RISCV_M_MODE /* the MPP value is too large to be used as an immediate arg for addi */ li t0, SR_MPP - and s0, s0, t0 + and t0, s1, t0 #else - andi s0, s0, SR_SPP + andi t0, s1, SR_SPP #endif - bnez s0, 1f + bnez t0, 1f /* Save unwound kernel stack pointer in thread_info */ - addi s0, sp, PT_SIZE_ON_STACK - REG_S s0, TASK_TI_KERNEL_SP(tp) + addi t0, sp, PT_SIZE_ON_STACK + REG_S t0, TASK_TI_KERNEL_SP(tp) /* Save the kernel shadow call stack pointer */ scs_save_current @@ -148,7 +148,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception) */ csrw CSR_SCRATCH, tp 1: - REG_L a0, PT_STATUS(sp) /* * The current load reservation is effectively part of the processor's * state, in the sense that load reservations cannot be shared between @@ -169,7 +168,7 @@ SYM_CODE_START_NOALIGN(ret_from_exception) REG_L a2, PT_EPC(sp) REG_SC x0, a2, PT_EPC(sp) - csrw CSR_STATUS, a0 + csrw CSR_STATUS, s1 csrw CSR_EPC, a2 REG_L x1, PT_RA(sp) @@ -207,13 +206,13 @@ SYM_CODE_START_LOCAL(handle_kernel_stack_overflow) REG_S x5, PT_T0(sp) save_from_x6_to_x31 - REG_L s0, TASK_TI_KERNEL_SP(tp) + REG_L a2, TASK_TI_KERNEL_SP(tp) csrr s1, CSR_STATUS csrr s2, CSR_EPC csrr s3, CSR_TVAL csrr s4, CSR_CAUSE csrr s5, CSR_SCRATCH - REG_S s0, PT_SP(sp) + REG_S a2, PT_SP(sp) REG_S s1, PT_STATUS(sp) REG_S s2, PT_EPC(sp) REG_S s3, PT_BADADDR(sp) @@ -227,10 +226,10 @@ ASM_NOKPROBE(handle_kernel_stack_overflow) SYM_CODE_START(ret_from_fork) call schedule_tail - beqz s0, 1f /* not from kernel thread */ + beqz s1, 1f /* not from kernel thread */ /* Call fn(arg) */ - move a0, s1 - jalr s0 + move a0, s2 + jalr s1 1: move a0, sp /* pt_regs */ la ra, ret_from_exception diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index e4bc61c4e58a..5512c31e1256 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -208,8 +208,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) /* Supervisor/Machine, irqs on: */ childregs->status = SR_PP | SR_PIE; - p->thread.s[0] = (unsigned long)args->fn; - p->thread.s[1] = (unsigned long)args->fn_arg; + p->thread.s[1] = (unsigned long)args->fn; + p->thread.s[2] = (unsigned long)args->fn_arg; } else { *childregs = *(current_pt_regs()); /* Turn off status.VS */ @@ -219,7 +219,6 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (clone_flags & CLONE_SETTLS) childregs->tp = tls; childregs->a0 = 0; /* Return value of fork() */ - p->thread.s[0] = 0; } p->thread.riscv_v_flags = 0; if (has_vector())
s0 is reserved for the frame pointer, so it should not be used as a temporary register. Clobbering the frame pointer breaks stack traces. - In handle_exception() and ret_from_exception(), use a2 for the saved stack pointer. a2 is chosen because r2 is the stack pointer register. - In ret_from_exception(), use s1 for the saved status CSR value. Avoid clobbering s1 in the privilege mode check so it does not need to be reloaded later in the function. - Use s1 and s2 in ret_from_fork() instead of s0 and s1. The entire p->thread.s array is zeroed at the beginning of copy_thread(), so the registers do not need to be zeroed separately for kernel threads. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- arch/riscv/kernel/entry.S | 29 ++++++++++++++--------------- arch/riscv/kernel/process.c | 5 ++--- 2 files changed, 16 insertions(+), 18 deletions(-)