Re: [PATCH] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

[Michael Ellerman <mpe@kernel.org>]

On 24/6/26 9:31 pm, Rui Qi wrote:
> From: Samuel Holland <samuel.holland@sifive.com>
> 
> call_on_irq_stack() uses struct member offsets to set up its link in the
> frame record list. On riscv32, struct stackframe is the wrong size to
> maintain stack pointer alignment, so STACKFRAME_SIZE_ON_STACK includes
> padding. However, the ABI requires the frame record to be placed
> immediately below the address stored in s0, so the padding must come
> before the struct members.
> 
> Fix the layout by making STACKFRAME_FP and STACKFRAME_RA the negative
> offsets from s0, instead of the positive offsets from sp.

The fact that all uses of the defines need to add back STACKFRAME_SIZE_ON_STACK
makes me think the defines don't have the most useful values.

If the values were offset + padding then the uses could be left unchanged.

> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index af827448a609..a75f0cfea1e9 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -501,8 +501,8 @@ void asm_offsets(void)
>   	OFFSET(SBI_HART_BOOT_STACK_PTR_OFFSET, sbi_hart_boot_data, stack_ptr);
>   
>   	DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
> -	OFFSET(STACKFRAME_FP, stackframe, fp);
> -	OFFSET(STACKFRAME_RA, stackframe, ra);
> +	DEFINE(STACKFRAME_FP, offsetof(struct stackframe, fp) - sizeof(struct stackframe));
> +	DEFINE(STACKFRAME_RA, offsetof(struct stackframe, ra) - sizeof(struct stackframe));

ie. if they were defined as:

         DEFINE(STACKFRAME_FP, ALIGN(sizeof(struct stackframe), STACK_ALIGN) -
                               sizeof(struct stackframe) + offsetof(struct stackframe, fp));
         DEFINE(STACKFRAME_RA, ALIGN(sizeof(struct stackframe), STACK_ALIGN) -
                               sizeof(struct stackframe) + offsetof(struct stackframe, ra));

STACKFRAME_SIZE_ON_STACK - sizeof(stackframe) computes the size of the padding,
and then plus the offset gets you the right location.

That would allow the call sites to still use STACKFRAME_RA(sp).

I would probably also rename them to ON_STACK_RA or something, to make it clear
they are not offsets into struct stackframe any more, but maybe that's bike shedding :)

cheers

>   #ifdef CONFIG_FUNCTION_TRACER
>   	DEFINE(FTRACE_OPS_FUNC,		offsetof(struct ftrace_ops, func));
>   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index c6988983cdf7..08df724e13b9 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -386,8 +386,8 @@ SYM_CODE_END(ret_from_fork_user_asm)
>   SYM_FUNC_START(call_on_irq_stack)
>   	/* Create a frame record to save ra and s0 (fp) */
>   	addi	sp, sp, -STACKFRAME_SIZE_ON_STACK
> -	REG_S	ra, STACKFRAME_RA(sp)
> -	REG_S	s0, STACKFRAME_FP(sp)
> +	REG_S	ra, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_RA)(sp)
> +	REG_S	s0, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_FP)(sp)
>   	addi	s0, sp, STACKFRAME_SIZE_ON_STACK
>   
>   	/* Switch to the per-CPU shadow call stack */
> @@ -405,8 +405,8 @@ SYM_FUNC_START(call_on_irq_stack)
>   
>   	/* Switch back to the thread stack and restore ra and s0 */
>   	addi	sp, s0, -STACKFRAME_SIZE_ON_STACK
> -	REG_L	ra, STACKFRAME_RA(sp)
> -	REG_L	s0, STACKFRAME_FP(sp)
> +	REG_L	ra, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_RA)(sp)
> +	REG_L	s0, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_FP)(sp)
>   	addi	sp, sp, STACKFRAME_SIZE_ON_STACK
>   
>   	ret