Received-SPF: pass (google.com: domain of linux-kernel+bounces-209289-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240530001733.1407654-1-samuel.holland@sifive.com> <20240530001733.1407654-4-samuel.holland@sifive.com>
In-Reply-To: <20240530001733.1407654-4-samuel.holland@sifive.com>
From: Andy Chiu <andy.chiu@sifive.com>
Date: Tue, 11 Jun 2024 13:34:32 +0800
Message-ID: <CABgGipXz77=nyXv9uQ7DKqYH1HkMxxFTbK2qHE202AeXq7Mm-g@mail.gmail.com>
Subject: Re: [PATCH 3/4] riscv: entry: Do not clobber the frame pointer
To: Samuel Holland <samuel.holland@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, linux-kernel@vger.kernel.org, 
	linux-riscv@lists.infradead.org, Matthew Bystrin <dev.mbstr@gmail.com>, 
	Sami Tolvanen <samitolvanen@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Samuel,

On Thu, May 30, 2024 at 8:17=E2=80=AFAM 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 proces=
sor's
>          * state, in the sense that load reservations cannot be shared be=
tween
> @@ -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 k=
ernel_clone_args *args)
>                 /* Supervisor/Machine, irqs on: */
>                 childregs->status =3D SR_PP | SR_PIE;
>
> -               p->thread.s[0] =3D (unsigned long)args->fn;
> -               p->thread.s[1] =3D (unsigned long)args->fn_arg;
> +               p->thread.s[1] =3D (unsigned long)args->fn;
> +               p->thread.s[2] =3D (unsigned long)args->fn_arg;
>         } else {
>                 *childregs =3D *(current_pt_regs());
>                 /* Turn off status.VS */
> @@ -219,7 +219,6 @@ int copy_thread(struct task_struct *p, const struct k=
ernel_clone_args *args)
>                 if (clone_flags & CLONE_SETTLS)
>                         childregs->tp =3D tls;
>                 childregs->a0 =3D 0; /* Return value of fork() */
> -               p->thread.s[0] =3D 0;
>         }
>         p->thread.riscv_v_flags =3D 0;
>         if (has_vector())
> --
> 2.44.1
>

Reviewed-by: Andy Chiu <andy.chiu@sifive.com>

Cheers,
Andy