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

[Samuel Holland]

On 2026-06-25 3:49 AM, Nam Cao wrote:
> Michael Ellerman <mpe@xxxxxxxxxx> writes:
>> On 24/6/26 9:31 pm, Rui Qi wrote:
>>> 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.

The reason for choosing the definitions in the patch is to match the ABI[1],
which defines the fields at negative offsets from the frame pointer: "This puts
the return address at fp - PTRSZ/8, and the previous frame pointer at fp - 2 *
PTRSZ/8"

In call_on_irq_stack(), the frame pointer is (sp + STACKFRAME_SIZE_ON_STACK). So
the restore code could equivalently be written as:

    REG_L    ra, STACKFRAME_RA(s0)
    REG_L    s0, STACKFRAME_FP(s0)

which is nice and clean and works everywhere, but the save code can't be as
clean due to needing both the old and new frame pointer value at the same time.
So it either needs a temporary register for the new stack pointer, or to do the
calculation in the assembly, and I chose the latter; and I preferred to make the
save/restore code visually consistent.

[1]:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#frame-pointer-convention

> At the moment we only have only one user which has nothing except the
> stack frame on the stack. Other potential users do not necessarily add back
> STACKFRAME_SIZE_ON_STACK.
> 
> The offset value depends on the function. So I think it's better to
> leave that offset calculation to users.

Indeed, a later patch in the original series allocated space for both struct
stackframe and struct pt_regs at the same time.

Regards,
Samuel