Message-ID: <4bf64df4-43a4-4207-b7d5-ff07adb98193@ghiti.fr>
Date: Thu, 1 Feb 2024 11:07:48 +0100
From: Alexandre Ghiti <alex@...ti.fr>
To: Zong Li <zong.li@...ive.com>, Conor Dooley <conor.dooley@...rochip.com>
Cc: palmer@...belt.com, paul.walmsley@...ive.com, aou@...s.berkeley.edu,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] riscv: add CALLER_ADDRx support

On 01/02/2024 09:43, Zong Li wrote:
> On Thu, Feb 1, 2024 at 1:10 AM Alexandre Ghiti <alex@...ti.fr> wrote:
>> On 05/12/2023 09:59, Zong Li wrote:
>>> CALLER_ADDRx returns caller's address at specified level, they are used
>>> for several tracers. These macros eventually use
>>> __builtin_return_address(n) to get the caller's address if arch doesn't
>>> define their own implementation.
>>>
>>> In RISC-V, __builtin_return_address(n) only works when n == 0, we need
>>> to walk the stack frame to get the caller's address at specified level.
>>
>> Is that a bug in gcc or something expected for riscv in general?
>>
> I think it isn't supported for riscv in general.
>
>>> data.level started from 'level + 3' due to the call flow of getting
>>> caller's address in RISC-V implementation. If we don't have additional
>>> three iteration, the level is corresponding to follows:
>>>
>>> callsite -> return_address -> arch_stack_walk -> walk_stackframe
>>> |           |                 |                  |
>>> level 3     level 2           level 1            level 0
>>>
>>> Signed-off-by: Zong Li <zong.li@...ive.com>
>>> ---
>>>    arch/riscv/include/asm/ftrace.h    |  5 ++++
>>>    arch/riscv/kernel/Makefile         |  2 ++
>>>    arch/riscv/kernel/return_address.c | 48 ++++++++++++++++++++++++++++++
>>>    3 files changed, 55 insertions(+)
>>>    create mode 100644 arch/riscv/kernel/return_address.c
>>>
>>> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
>>> index 2b2f5df7ef2c..42777f91a9c5 100644
>>> --- a/arch/riscv/include/asm/ftrace.h
>>> +++ b/arch/riscv/include/asm/ftrace.h
>>> @@ -25,6 +25,11 @@
>>>
>>>    #define ARCH_SUPPORTS_FTRACE_OPS 1
>>>    #ifndef __ASSEMBLY__
>>> +
>>> +extern void *return_address(unsigned int level);
>>> +
>>> +#define ftrace_return_address(n) return_address(n)
>>> +
>>>    void MCOUNT_NAME(void);
>>>    static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>>    {
>>> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
>>> index fee22a3d1b53..40d054939ae2 100644
>>> --- a/arch/riscv/kernel/Makefile
>>> +++ b/arch/riscv/kernel/Makefile
>>> @@ -7,6 +7,7 @@ ifdef CONFIG_FTRACE
>>>    CFLAGS_REMOVE_ftrace.o      = $(CC_FLAGS_FTRACE)
>>>    CFLAGS_REMOVE_patch.o       = $(CC_FLAGS_FTRACE)
>>>    CFLAGS_REMOVE_sbi.o = $(CC_FLAGS_FTRACE)
>>> +CFLAGS_REMOVE_return_address.o       = $(CC_FLAGS_FTRACE)
>>>    endif
>>>    CFLAGS_syscall_table.o      += $(call cc-option,-Wno-override-init,)
>>>    CFLAGS_compat_syscall_table.o += $(call cc-option,-Wno-override-init,)
>>> @@ -46,6 +47,7 @@ obj-y       += irq.o
>>>    obj-y       += process.o
>>>    obj-y       += ptrace.o
>>>    obj-y       += reset.o
>>> +obj-y        += return_address.o
>>>    obj-y       += setup.o
>>>    obj-y       += signal.o
>>>    obj-y       += syscall_table.o
>>> diff --git a/arch/riscv/kernel/return_address.c b/arch/riscv/kernel/return_address.c
>>> new file mode 100644
>>> index 000000000000..c2008d4aa6e5
>>> --- /dev/null
>>> +++ b/arch/riscv/kernel/return_address.c
>>> @@ -0,0 +1,48 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * This code come from arch/arm64/kernel/return_address.c
>>> + *
>>> + * Copyright (C) 2023 SiFive.
>>> + */
>>> +
>>> +#include <linux/export.h>
>>> +#include <linux/kprobes.h>
>>> +#include <linux/stacktrace.h>
>>> +
>>> +struct return_address_data {
>>> +     unsigned int level;
>>> +     void *addr;
>>> +};
>>> +
>>> +static bool save_return_addr(void *d, unsigned long pc)
>>> +{
>>> +     struct return_address_data *data = d;
>>> +
>>> +     if (!data->level) {
>>> +             data->addr = (void *)pc;
>>> +             return false;
>>> +     }
>>> +
>>> +     --data->level;
>>> +
>>> +     return true;
>>> +}
>>> +NOKPROBE_SYMBOL(save_return_addr);
>>> +
>>> +void *return_address(unsigned int level)
>>
>> Maybe return_address() should be noinline to make sure it's not inlined
>> as it would break the "+ 3"? Not sure it's necessary though.
> Yes, thanks for pointing it out. We should ensure it's not inlined in
> any case. I will send the next version.
>
>>
>>> +{
>>> +     struct return_address_data data;
>>> +
>>> +     data.level = level + 3;
>>> +     data.addr = NULL;
>>> +
>>> +     arch_stack_walk(save_return_addr, &data, current, NULL);
>>> +
>>> +     if (!data.level)
>>> +             return data.addr;
>>> +     else
>>> +             return NULL;
>>> +
>>> +}
>>> +EXPORT_SYMBOL_GPL(return_address);
>>> +NOKPROBE_SYMBOL(return_address);
>>
>> And I see that with this patch, ftrace_return_address() is now defined
>> whether CONFIG_FRAME_POINTER is set or not, is that correct?
> Yes, that is what I understand. In this patch, the
> ftrace_return_address() is still defined to return_address() when
> CONFIG_FRAME_POINTER isn't enabled, and return_address still works
> because riscv port can walk stackframe without fp.
>
>> This looks like a fix to me so that should go into -fixes with a Fixes
>> tag (but we'll have to make sure that the "+ 3" is correct on all
>> backports...):
>>
>> Fixes: 10626c32e382 ("riscv/ftrace: Add basic support")
> The return_address() invokes arch_stack_walk(), which enabled by the
> '5cb0080f1bfd ("riscv: Enable ARCH_STACKWALK")'
>
> I guess that we are not able to apply it on all backports. Is this
> right? "+3" is correct after enabling ARCH_STACKWALK.


5cb0080f1bfd was introduced in v5.11, so that will make the backport possible up to 5.15, I guess that's ok, nobody will ever use a riscv kernel that old (?).

So I'd add the Fixes tag I proposed above, and let the backport fail for < 5.15. @Conor any opinion?


>
>> And you can finally add for your v2 (or not if you don't respin):
>>
>> Reviewed-by: Alexandre Ghiti <alexghiti@...osinc.com>
> Thanks for the review, Alexandre. I will add it in v2:)


Thanks,

Alex


>
>> Thanks and sorry for the delay!
>>
>> Alex
>>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists