Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 31 Jan 2024 18:10:48 +0100 | Subject | Re: [PATCH] riscv: add CALLER_ADDRx support | From | Alexandre Ghiti <> |
| |
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?
> > 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@sifive.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.
> +{ > + 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?
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")
And you can finally add for your v2 (or not if you don't respin):
Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Thanks and sorry for the delay!
Alex
| ![\](/images/icornerr.gif) |