Re: [PATCH 1/2] riscv/ftrace: add DYNAMIC_FTRACE_WITH_DIRECT_CALLS support

From: Song Shuai
Date: Fri Nov 25 2022 - 01:35:56 EST

Guo Ren <guoren@xxxxxxxxxx> 于2022年11月25日周五 03:10写道：
>
> On Fri, Nov 25, 2022 at 9:53 AM Song Shuai <suagrfillet@xxxxxxxxx> wrote:
> >
> > Guo Ren <guoren@xxxxxxxxxx> 于2022年11月24日周四 15:31写道：
> > >
> > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@xxxxxxxxx> wrote:
> > > >
> > > > Guo Ren <guoren@xxxxxxxxxx> 于2022年11月23日周三 23:02写道：
> > > > >
> > > > > Cool job, thx.
> > > > >
> > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@xxxxxxxxx> wrote:
> > > > >>
> > > > >> This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V.
> > > > >>
> > > > >> select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the
> > > > >> register_ftrace_direct[_multi] interfaces allowing users to register
> > > > >> the customed trampoline (direct_caller) as the mcount for one or
> > > > >> more target functions. And modify_ftrace_direct[_multi] are also
> > > > >> provided for modifying direct_caller.
> > > > >>
> > > > >> To make the direct_caller and the other ftrace hooks (eg. function/fgraph
> > > > >> tracer, k[ret]probes) co-exist, a temporary register is nominated to
> > > > >> store the address of direct_caller in ftrace_regs_caller. After the
> > > > >> setting of the address direct_caller by direct_ops->func and the
> > > > >> RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to
> > > > >> by the `jr` inst.
> > > > >>
> > > > >> Signed-off-by: Song Shuai <suagrfillet@xxxxxxxxx>
> > > > >> ---
> > > > >> arch/riscv/Kconfig | 1 +
> > > > >> arch/riscv/include/asm/ftrace.h | 6 ++++++
> > > > >> arch/riscv/kernel/mcount-dyn.S | 4 ++++
> > > > >> 3 files changed, 11 insertions(+)
> > > > >>
> > > > >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > >> index 39ec8d628cf6..d083ec08d0b6 100644
> > > > >> --- a/arch/riscv/Kconfig
> > > > >> +++ b/arch/riscv/Kconfig
> > > > >> @@ -278,6 +278,7 @@ config ARCH_RV64I
> > > > >> select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
> > > > >> select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8)
> > > > >> select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
> > > > >> + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > > > >> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > > > >> select HAVE_FUNCTION_GRAPH_TRACER
> > > > >> select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION
> > > > >> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > > > >> index 01bebb28eabe..be4d57566139 100644
> > > > >> --- a/arch/riscv/include/asm/ftrace.h
> > > > >> +++ b/arch/riscv/include/asm/ftrace.h
> > > > >> @@ -114,6 +114,12 @@ struct ftrace_regs;
> > > > >> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> > > > >> struct ftrace_ops *op, struct ftrace_regs *fregs);
> > > > >> #define ftrace_graph_func ftrace_graph_func
> > > > >> +
> > > > >> +static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr)
> > > > >> +{
> > > > >> + regs->t1 = addr;
> > > > >
> > > > > How about regs->t0 = addr; ?
> > > > > And delete all mcount-dyn.S modification.
> > > > >
> > > > The direct_caller has the same program layout as the ftrace_caller, which means
> > > > the reg t0 will never be changed when direct_caller returns.
> > > >
> > > > If regs->t0 changes here and ftrace_regs_caller executes `jr t0`,
> > > > direct_caller will enter the dead loop.
> > > >
> > > > Actually the reg t0 always saves the address of function entry with 8B
> > > > offset, it should only
> > > > changed by the IPMODIFY ops instead of the direct_ops.
> > > How about:
> > > static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs,
> > > unsigned long addr)
> > > {
> > > regs->t1 = regs->t0;
> > > regs->t0 = addr;
> > >
> > > direct_caller:
> > > add sp,sp,-?
> > > sd t1,?(sp)
> > direct_caller also serves as the first trampoline as ftrace_caller, like this:
> > ```
> > func -- direct_caller
> > -- ftrace_[regs]_caller
> > ```
> > So the t1 in this line has to be t0 to save the PC.
>
> direct_caller:
> add sp,sp,-?
> sd t1,?(sp)
> sd t0, ?(so)
> sd ra,?(sp)
> mov t0, t1
This foo is the tracing function along with the direct_caller,
and it has the same parameters as the target function.
So the t0 or t1 here means nothing for this foo function.

No offense, but what's the purpose of this mv inst?
> call foo
> ld t0,?(sp)
> ld t1,?(sp)
> ld ra,?(sp)
> add sp,sp,?
> jr t1 // <- back to function entry
When direct_caller works as the first trampoline
the content of t1 here means nothing for the target function, neither
PC nor PIP.
>
>
> > > sd ra,?(sp)
> > > call foo
> > > ld t1,?(sp)
> > And this line.
> > > ld ra,?(sp)
> > > add sp,sp,?
> > > jr t1 // <- back to function entry
> > >
> > > And delete all mcount-dyn.S modification.
> > >
> > > > >>
> > > > >> +}
> > > > >> +
> > > > >> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> > > > >>
> > > > >> #endif /* __ASSEMBLY__ */
> > > > >> diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
> > > > >> index 466c6ef217b1..b89c85a58569 100644
> > > > >> --- a/arch/riscv/kernel/mcount-dyn.S
> > > > >> +++ b/arch/riscv/kernel/mcount-dyn.S
> > > > >> @@ -233,6 +233,7 @@ ENDPROC(ftrace_caller)
> > > > >> #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> > > > >> ENTRY(ftrace_regs_caller)
> > > > >> SAVE_ABI_REGS 1
> > > > >> + REG_S x0, PT_T1(sp)
> > > > >> PREPARE_ARGS
> > > > >>
> > > > >> ftrace_regs_call:
> > > > >> @@ -241,7 +242,10 @@ ftrace_regs_call:
> > > > >>
> > > > >>
> > > > >> RESTORE_ABI_REGS 1
> > > > >> + bnez t1,.Ldirect
> > > > >> jr t0
> > > > >> +.Ldirect:
> > > > >> + jr t1
> > > > >> ENDPROC(ftrace_regs_caller)
> > > > >>
> > > > >> ENTRY(ftrace_caller)
> > > > >> --
> > > > >> 2.20.1
> > > > >>
> > > > >
> > > > >
> > > > > --
> > > > > Best Regards
> > > > > Guo Ren
> > >
> > >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
> > Thanks,
> > Song
>
>
>
> --
> Best Regards
> Guo Ren