Message-ID: <25e2faa5-908c-4391-87db-8540d1f0e904@ghiti.fr>
Date: Wed, 31 Jan 2024 15:46:27 +0100
From: Alexandre Ghiti <alex@...ti.fr>
To: Zong Li <zong.li@...ive.com>, palmer@...belt.com,
 Palmer Dabbelt <palmer@...osinc.com>, paul.walmsley@...ive.com,
 aou@...s.berkeley.edu, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] riscv: add CALLER_ADDRx support

Hi Zong,

On 31/01/2024 15:24, Zong Li wrote:
> On Thu, Jan 18, 2024 at 8:50 AM Zong Li <zong.li@...ive.com> wrote:
>> On Wed, Jan 10, 2024 at 11:33 AM Zong Li <zong.li@...ive.com> wrote:
>>> On Fri, Dec 29, 2023 at 2:34 PM Zong Li <zong.li@...ive.com> wrote:
>>>> On Tue, Dec 5, 2023 at 5:00 PM Zong Li <zong.li@...ive.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@...ive.com>
>>>>> ---
>>>>>   arch/riscv/include/asm/ftrace.h    |  5 ++++
>>>>>   arch/riscv/kernel/Makefile         |  2 ++
>>>>>   arch/riscv/kernel/return_address.c | 48 ++++++++++++++++++++++++++++++
>>>>>   3 files changed, 55 insertions(+)
>>>>>   create mode 100644 arch/riscv/kernel/return_address.c
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
>>>>> index 2b2f5df7ef2c..42777f91a9c5 100644
>>>>> --- a/arch/riscv/include/asm/ftrace.h
>>>>> +++ b/arch/riscv/include/asm/ftrace.h
>>>>> @@ -25,6 +25,11 @@
>>>>>
>>>>>   #define ARCH_SUPPORTS_FTRACE_OPS 1
>>>>>   #ifndef __ASSEMBLY__
>>>>> +
>>>>> +extern void *return_address(unsigned int level);
>>>>> +
>>>>> +#define ftrace_return_address(n) return_address(n)
>>>>> +
>>>>>   void MCOUNT_NAME(void);
>>>>>   static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>>>>   {
>>>>> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
>>>>> index fee22a3d1b53..40d054939ae2 100644
>>>>> --- a/arch/riscv/kernel/Makefile
>>>>> +++ b/arch/riscv/kernel/Makefile
>>>>> @@ -7,6 +7,7 @@ ifdef CONFIG_FTRACE
>>>>>   CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
>>>>>   CFLAGS_REMOVE_patch.o  = $(CC_FLAGS_FTRACE)
>>>>>   CFLAGS_REMOVE_sbi.o    = $(CC_FLAGS_FTRACE)
>>>>> +CFLAGS_REMOVE_return_address.o = $(CC_FLAGS_FTRACE)
>>>>>   endif
>>>>>   CFLAGS_syscall_table.o += $(call cc-option,-Wno-override-init,)
>>>>>   CFLAGS_compat_syscall_table.o += $(call cc-option,-Wno-override-init,)
>>>>> @@ -46,6 +47,7 @@ obj-y += irq.o
>>>>>   obj-y  += process.o
>>>>>   obj-y  += ptrace.o
>>>>>   obj-y  += reset.o
>>>>> +obj-y  += return_address.o
>>>>>   obj-y  += setup.o
>>>>>   obj-y  += signal.o
>>>>>   obj-y  += syscall_table.o
>>>>> diff --git a/arch/riscv/kernel/return_address.c b/arch/riscv/kernel/return_address.c
>>>>> new file mode 100644
>>>>> index 000000000000..c2008d4aa6e5
>>>>> --- /dev/null
>>>>> +++ b/arch/riscv/kernel/return_address.c
>>>>> @@ -0,0 +1,48 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +/*
>>>>> + * This code come from arch/arm64/kernel/return_address.c
>>>>> + *
>>>>> + * Copyright (C) 2023 SiFive.
>>>>> + */
>>>>> +
>>>>> +#include <linux/export.h>
>>>>> +#include <linux/kprobes.h>
>>>>> +#include <linux/stacktrace.h>
>>>>> +
>>>>> +struct return_address_data {
>>>>> +       unsigned int level;
>>>>> +       void *addr;
>>>>> +};
>>>>> +
>>>>> +static bool save_return_addr(void *d, unsigned long pc)
>>>>> +{
>>>>> +       struct return_address_data *data = d;
>>>>> +
>>>>> +       if (!data->level) {
>>>>> +               data->addr = (void *)pc;
>>>>> +               return false;
>>>>> +       }
>>>>> +
>>>>> +       --data->level;
>>>>> +
>>>>> +       return true;
>>>>> +}
>>>>> +NOKPROBE_SYMBOL(save_return_addr);
>>>>> +
>>>>> +void *return_address(unsigned int level)
>>>>> +{
>>>>> +       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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists