Message ID | 20231205085959.32177-1-zong.li@sifive.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | riscv: add CALLER_ADDRx support | expand |
On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@sifive.com> 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. > > 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) > +{ > + 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); > -- > 2.17.1 > Hi Palmer and all, I was wondering whether this patch is good for everyone? Thanks
On Fri, Dec 29, 2023 at 2:34 PM Zong Li <zong.li@sifive.com> wrote: > > On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@sifive.com> 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. > > > > 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) > > +{ > > + 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); > > -- > > 2.17.1 > > > > Hi Palmer and all, > I was wondering whether this patch is good for everyone? Thanks Hi Palmer, Is there any chance to include this patch in 6.8-rc1? Thanks
On Wed, Jan 10, 2024 at 11:33 AM Zong Li <zong.li@sifive.com> wrote: > > On Fri, Dec 29, 2023 at 2:34 PM Zong Li <zong.li@sifive.com> wrote: > > > > On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@sifive.com> 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. > > > > > > 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) > > > +{ > > > + 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); > > > -- > > > 2.17.1 > > > > > > > Hi Palmer and all, > > I was wondering whether this patch is good for everyone? Thanks > > Hi Palmer, > Is there any chance to include this patch in 6.8-rc1? Thanks Hi Palmer, I'm not sure if this patch is a feature or bug fix, would you consider it for 6.8-rcX? Thanks
On Thu, Jan 18, 2024 at 8:50 AM Zong Li <zong.li@sifive.com> wrote: > > On Wed, Jan 10, 2024 at 11:33 AM Zong Li <zong.li@sifive.com> wrote: > > > > On Fri, Dec 29, 2023 at 2:34 PM Zong Li <zong.li@sifive.com> wrote: > > > > > > On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@sifive.com> 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. > > > > > > > > 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) > > > > +{ > > > > + 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); > > > > -- > > > > 2.17.1 > > > > > > > > > > Hi Palmer and all, > > > I was wondering whether this patch is good for everyone? Thanks > > > > Hi Palmer, > > Is there any chance to include this patch in 6.8-rc1? Thanks > > Hi Palmer, > I'm not sure if this patch is a feature or bug fix, would you consider > it for 6.8-rcX? Thanks Hi Palmer, Sorry for pinging you again. I'm not sure if you saw this patch. The irqsoff and wakeup tracer will use CALLER_ADDR1 and CALLER_ADDR2 to obtain the caller's return address, but we are currently encountering an issue in the RISC-V port where the wrong caller is identified. I guess you can easily reproduce the situation using ftrace. Could you share your thoughts on this when you have the time to take a look? Thanks
Hi Zong, On 31/01/2024 15:24, Zong Li wrote: > On Thu, Jan 18, 2024 at 8:50 AM Zong Li <zong.li@sifive.com> wrote: >> On Wed, Jan 10, 2024 at 11:33 AM Zong Li <zong.li@sifive.com> wrote: >>> On Fri, Dec 29, 2023 at 2:34 PM Zong Li <zong.li@sifive.com> wrote: >>>> On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@sifive.com> 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. >>>>> >>>>> 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) >>>>> +{ >>>>> + 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); >>>>> -- >>>>> 2.17.1 >>>>> >>>> Hi Palmer and all, >>>> I was wondering whether this patch is good for everyone? Thanks >>> Hi Palmer, >>> Is there any chance to include this patch in 6.8-rc1? Thanks >> Hi Palmer, >> I'm not sure if this patch is a feature or bug fix, would you consider >> it for 6.8-rcX? Thanks > Hi Palmer, > Sorry for pinging you again. I'm not sure if you saw this patch. The > irqsoff and wakeup tracer will use CALLER_ADDR1 and CALLER_ADDR2 to > obtain the caller's return address, but we are currently encountering > an issue in the RISC-V port where the wrong caller is identified. I > guess you can easily reproduce the situation using ftrace. Could you > share your thoughts on this when you have the time to take a look? > Thanks I'm not Palmer but I'll take a look at your patch today :) Thanks, Alex > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Jan 31, 2024 at 10:46 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Zong, > > On 31/01/2024 15:24, Zong Li wrote: > > On Thu, Jan 18, 2024 at 8:50 AM Zong Li <zong.li@sifive.com> wrote: > >> On Wed, Jan 10, 2024 at 11:33 AM Zong Li <zong.li@sifive.com> wrote: > >>> On Fri, Dec 29, 2023 at 2:34 PM Zong Li <zong.li@sifive.com> wrote: > >>>> On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@sifive.com> 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. > >>>>> > >>>>> 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) > >>>>> +{ > >>>>> + 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); > >>>>> -- > >>>>> 2.17.1 > >>>>> > >>>> Hi Palmer and all, > >>>> I was wondering whether this patch is good for everyone? Thanks > >>> Hi Palmer, > >>> Is there any chance to include this patch in 6.8-rc1? Thanks > >> Hi Palmer, > >> I'm not sure if this patch is a feature or bug fix, would you consider > >> it for 6.8-rcX? Thanks > > Hi Palmer, > > Sorry for pinging you again. I'm not sure if you saw this patch. The > > irqsoff and wakeup tracer will use CALLER_ADDR1 and CALLER_ADDR2 to > > obtain the caller's return address, but we are currently encountering > > an issue in the RISC-V port where the wrong caller is identified. I > > guess you can easily reproduce the situation using ftrace. Could you > > share your thoughts on this when you have the time to take a look? > > Thanks > > > I'm not Palmer but I'll take a look at your patch today :) > Hi Alexandre, I appreciate your assistance in reviewing this patch, Thanks a lot. :) > Thanks, > > Alex > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
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
On Thu, Feb 1, 2024 at 1:10 AM Alexandre Ghiti <alex@ghiti.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@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. 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. > > And you can finally add for your v2 (or not if you don't respin): > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks for the review, Alexandre. I will add it in v2:) > > Thanks and sorry for the delay! > > Alex >
On 01/02/2024 09:43, Zong Li wrote: > On Thu, Feb 1, 2024 at 1:10 AM Alexandre Ghiti <alex@ghiti.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@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. > 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@rivosinc.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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, Feb 01, 2024 at 11:07:48AM +0100, Alexandre Ghiti wrote: > 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? The stable kernels are 5.10 and 5.15, so there's no kernels that matter which have the bad commit and are < 5.15.
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) +{ + 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);
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. 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