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 <[email protected]>
---
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
On Tue, Dec 5, 2023 at 5:00 PM Zong Li <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> On Tue, Dec 5, 2023 at 5:00 PM Zong Li <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> On Fri, Dec 29, 2023 at 2:34 PM Zong Li <[email protected]> wrote:
> >
> > On Tue, Dec 5, 2023 at 5:00 PM Zong Li <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> wrote:
>
> On Wed, Jan 10, 2024 at 11:33 AM Zong Li <[email protected]> wrote:
> >
> > On Fri, Dec 29, 2023 at 2:34 PM Zong Li <[email protected]> wrote:
> > >
> > > On Tue, Dec 5, 2023 at 5:00 PM Zong Li <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>> On Wed, Jan 10, 2024 at 11:33 AM Zong Li <[email protected]> wrote:
>>> On Fri, Dec 29, 2023 at 2:34 PM Zong Li <[email protected]> wrote:
>>>> On Tue, Dec 5, 2023 at 5:00 PM Zong Li <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Jan 31, 2024 at 10:46 PM Alexandre Ghiti <[email protected]> wrote:
>
> Hi Zong,
>
> On 31/01/2024 15:24, Zong Li wrote:
> > On Thu, Jan 18, 2024 at 8:50 AM Zong Li <[email protected]> wrote:
> >> On Wed, Jan 10, 2024 at 11:33 AM Zong Li <[email protected]> wrote:
> >>> On Fri, Dec 29, 2023 at 2:34 PM Zong Li <[email protected]> wrote:
> >>>> On Tue, Dec 5, 2023 at 5:00 PM Zong Li <[email protected]> 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 <[email protected]>
> >>>>> ---
> >>>>> 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
> > [email protected]
> > 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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks and sorry for the delay!
Alex
On Thu, Feb 1, 2024 at 1:10 AM Alexandre Ghiti <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
> Thanks for the review, Alexandre. I will add it in v2:)
Thanks,
Alex
>
>> Thanks and sorry for the delay!
>>
>> Alex
>>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> 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.