diff mbox series

riscv: add CALLER_ADDRx support

Message ID	20231205085959.32177-1-zong.li@sifive.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1; From: Zong Li <zong.li@sifive.com> To: palmer@dabbelt.com, paul.walmsley@sifive.com, aou@eecs.berkeley.edu, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Zong Li <zong.li@sifive.com> Subject: [PATCH] riscv: add CALLER_ADDRx support Date: Tue, 5 Dec 2023 08:59:59 +0000 Message-Id: <20231205085959.32177-1-zong.li@sifive.com> Precedence: bulk
Series	riscv: add CALLER_ADDRx support \| riscv: add CALLER_ADDRx support

Commit Message

Zong Li Dec. 5, 2023, 8:59 a.m. UTC

  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

Comments

Zong Li Dec. 29, 2023, 6:34 a.m. UTC | #1

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

Zong Li Jan. 10, 2024, 3:33 a.m. UTC | #2

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

Zong Li Jan. 18, 2024, 12:50 a.m. UTC | #3

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

Zong Li Jan. 31, 2024, 2:24 p.m. UTC | #4

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

Alexandre Ghiti Jan. 31, 2024, 2:46 p.m. UTC | #5

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

Zong Li Jan. 31, 2024, 4:06 p.m. UTC | #6

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

Alexandre Ghiti Jan. 31, 2024, 5:10 p.m. UTC | #7

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

Zong Li Feb. 1, 2024, 8:43 a.m. UTC | #8

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
>

Alexandre Ghiti Feb. 1, 2024, 10:07 a.m. UTC | #9

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

Conor Dooley Feb. 1, 2024, 1:17 p.m. UTC | #10

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 mbox series

Patch

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);