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

[Song Shuai]

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.
>>
>> +}
>> +
>>  #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