| Message ID | 20221123142025.1504030-2-suagrfillet@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | riscv/ftrace: add WITH_DIRECT_CALLS support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Guessing tree name failed |
Hi Song, On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > --- > 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. > +} > + > #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
Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > Cool job, thx. > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> >> --- >> 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
On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > Cool job, thx. > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > >> --- > >> 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. ? ftrace_regs_caller->call_direct_funcs-> arch_ftrace_set_direct_caller-> ftrace_regs_caller jr t0. Only call_direct_funcs call arch_ftrace_set_direct_caller ! static void call_direct_funcs(unsigned long ip, unsigned long pip, struct ftrace_ops *ops, struct ftrace_regs *fregs) { struct pt_regs *regs = ftrace_get_regs(fregs); unsigned long addr; addr = ftrace_find_rec_direct(ip); if (!addr) return; arch_ftrace_set_direct_caller(regs, addr); } > > 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
Guo Ren <guoren@kernel.org> 于2022年11月24日周四 02:08写道： > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > Cool job, thx. > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > >> --- > > >> 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. > ? > > ftrace_regs_caller->call_direct_funcs-> > arch_ftrace_set_direct_caller-> ftrace_regs_caller jr t0. > > Only call_direct_funcs call arch_ftrace_set_direct_caller ! > > static void call_direct_funcs(unsigned long ip, unsigned long pip, > struct ftrace_ops *ops, struct ftrace_regs *fregs) > { > struct pt_regs *regs = ftrace_get_regs(fregs); > unsigned long addr; > > addr = ftrace_find_rec_direct(ip); > if (!addr) > return; > > arch_ftrace_set_direct_caller(regs, addr); > } > When direct_caller and function tracer co-hook a function, the simple diagram is like this: ``` func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> function_trace_call // write ringbuffer | -> call_direct_funcs // regs->t1 = direct_caller -> t1 !=0 && jr t1 // goto direct_caller ``` And the direct_caller has a similar implement as ftrace_caller. ``` direct_caller: add sp,sp,-? sd t0,?(sp) sd ra,?(sp) call foo ld t0,?(sp) ld ra,?(sp) add sp,sp,? jr t0 // <- back to function entry ``` If we change regs->t0 as direct_caller and execute `jr t0` directly, the direct_caller will always jump to itself due to its last `jr` inst. ``` func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> function_trace_call // write ringbuffer | -> call_direct_funcs // regs->t0 = direct_caller -> jr t0 // goto direct_caller direct_caller: ... sd t0,?(sp) ... ld t0,?(s0) ... jr t0 // goto direct_caller always ``` Hope I made it clear. > > > > 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 > > > > -- > Best Regards > Guo Ren Thanks, Song
Song Shuai <suagrfillet@gmail.com> 于2022年11月24日周四 02:52写道： > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 02:08写道： > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > Cool job, thx. > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > >> --- > > > >> 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. > > ? > > > > ftrace_regs_caller->call_direct_funcs-> > > arch_ftrace_set_direct_caller-> ftrace_regs_caller jr t0. > > > > Only call_direct_funcs call arch_ftrace_set_direct_caller ! > > > > static void call_direct_funcs(unsigned long ip, unsigned long pip, > > struct ftrace_ops *ops, struct ftrace_regs *fregs) > > { > > struct pt_regs *regs = ftrace_get_regs(fregs); > > unsigned long addr; > > > > addr = ftrace_find_rec_direct(ip); > > if (!addr) > > return; > > > > arch_ftrace_set_direct_caller(regs, addr); > > } > > > When direct_caller and function tracer co-hook a function, the simple > diagram is like this: > > ``` > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > function_trace_call // write ringbuffer > | > -> call_direct_funcs // regs->t1 = direct_caller > -> t1 !=0 && jr t1 // goto direct_caller > ``` > ``` f -- regs_caller -- list_func | | -- function_trace_call | | -- call_direct_funcs // t1 = addr |-- t1 !=0 && jr t1 // goto direct_caller ``` This diagram looks better. > And the direct_caller has a similar implement as ftrace_caller. > > ``` > direct_caller: > add sp,sp,-? > sd t0,?(sp) > sd ra,?(sp) > call foo > ld t0,?(sp) > ld ra,?(sp) > add sp,sp,? > jr t0 // <- back to function entry > ``` > > If we change regs->t0 as direct_caller and execute `jr t0` directly, > the direct_caller will always jump to itself due to its last `jr` inst. > > ``` > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > function_trace_call // write ringbuffer > | > -> call_direct_funcs // regs->t0 = direct_caller > -> jr t0 // goto direct_caller > > direct_caller: > ... > sd t0,?(sp) > ... > ld t0,?(s0) > ... > jr t0 // goto direct_caller always > ``` > > Hope I made it clear. > > > > > > 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 > > > > > > > > -- > > Best Regards > > Guo Ren > Thanks, > Song Thanks, Song
On Thu, Nov 24, 2022 at 11:09 AM Song Shuai <suagrfillet@gmail.com> wrote: > > Song Shuai <suagrfillet@gmail.com> 于2022年11月24日周四 02:52写道： > > > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 02:08写道： > > > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > > > Cool job, thx. > > > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > > >> --- > > > > >> 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. > > > ? > > > > > > ftrace_regs_caller->call_direct_funcs-> > > > arch_ftrace_set_direct_caller-> ftrace_regs_caller jr t0. > > > > > > Only call_direct_funcs call arch_ftrace_set_direct_caller ! > > > > > > static void call_direct_funcs(unsigned long ip, unsigned long pip, > > > struct ftrace_ops *ops, struct ftrace_regs *fregs) > > > { > > > struct pt_regs *regs = ftrace_get_regs(fregs); > > > unsigned long addr; > > > > > > addr = ftrace_find_rec_direct(ip); > > > if (!addr) > > > return; > > > > > > arch_ftrace_set_direct_caller(regs, addr); > > > } > > > > > When direct_caller and function tracer co-hook a function, the simple > > diagram is like this: > > > > ``` > > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > > function_trace_call // write ringbuffer > > | > > -> call_direct_funcs // regs->t1 = direct_caller > > -> t1 !=0 && jr t1 // goto direct_caller > > ``` > > > ``` > f -- regs_caller -- list_func > | | -- function_trace_call > | | -- call_direct_funcs // t1 = addr > |-- t1 !=0 && jr t1 // goto direct_caller Cool diagram! Thx, but we still need a discussion: f -- regs_caller | -- SAVE_ABI_REGS 1 | -- list_func | | -- function_trace_call | | -- call_direct_funcs // t1 = addr | -- RESTORE_ABI_REGS 1 |-- t1 !=0 && jr t1 // goto direct_caller If you set t1 non-zero, then we always go to direct_caller. I think the code is equal to set t0=addr. | | -- call_direct_funcs // t0 = addr | -- RESTORE_ABI_REGS 1 |-- jr t0 // goto direct_caller I think the only problem happens in the below non-existent situation: f -- regs_caller | -- SAVE_ABI_REGS 1 | -- list_func | | -- call_direct_funcs // t0 = addr | | -- function_trace_call //t0 contains direct_caller instead | -- RESTORE_ABI_REGS 1 |-- jr t0 // goto direct_caller The key issue is whether going to direct_caller correctly depends on call_direct_funcs called, right? > ``` > This diagram looks better. > > And the direct_caller has a similar implement as ftrace_caller. > > > > ``` > > direct_caller: > > add sp,sp,-? > > sd t0,?(sp) > > sd ra,?(sp) > > call foo > > ld t0,?(sp) > > ld ra,?(sp) > > add sp,sp,? > > jr t0 // <- back to function entry > > ``` > > > > If we change regs->t0 as direct_caller and execute `jr t0` directly, > > the direct_caller will always jump to itself due to its last `jr` inst. > > > > ``` > > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > > function_trace_call // write ringbuffer > > | > > -> call_direct_funcs // regs->t0 = direct_caller > > -> jr t0 // goto direct_caller > > > > direct_caller: > > ... > > sd t0,?(sp) > > ... > > ld t0,?(s0) > > ... > > jr t0 // goto direct_caller always > > ``` > > > > Hope I made it clear. > > > > > > > > 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 > > > > > > > > > > > > -- > > > Best Regards > > > Guo Ren > > Thanks, > > Song > Thanks, > Song
Guo Ren <guoren@kernel.org> 于2022年11月24日周四 03:40写道： > > On Thu, Nov 24, 2022 at 11:09 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > Song Shuai <suagrfillet@gmail.com> 于2022年11月24日周四 02:52写道： > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 02:08写道： > > > > > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > > > > > Cool job, thx. > > > > > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > > > >> --- > > > > > >> 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. > > > > ? > > > > > > > > ftrace_regs_caller->call_direct_funcs-> > > > > arch_ftrace_set_direct_caller-> ftrace_regs_caller jr t0. > > > > > > > > Only call_direct_funcs call arch_ftrace_set_direct_caller ! > > > > > > > > static void call_direct_funcs(unsigned long ip, unsigned long pip, > > > > struct ftrace_ops *ops, struct ftrace_regs *fregs) > > > > { > > > > struct pt_regs *regs = ftrace_get_regs(fregs); > > > > unsigned long addr; > > > > > > > > addr = ftrace_find_rec_direct(ip); > > > > if (!addr) > > > > return; > > > > > > > > arch_ftrace_set_direct_caller(regs, addr); > > > > } > > > > > > > When direct_caller and function tracer co-hook a function, the simple > > > diagram is like this: > > > > > > ``` > > > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > > > function_trace_call // write ringbuffer > > > | > > > -> call_direct_funcs // regs->t1 = direct_caller > > > -> t1 !=0 && jr t1 // goto direct_caller > > > ``` > > > > > ``` > > f -- regs_caller -- list_func > > | | -- function_trace_call > > | | -- call_direct_funcs // t1 = addr > > |-- t1 !=0 && jr t1 // goto direct_caller > Cool diagram! Thx, but we still need a discussion: > f -- regs_caller > | -- SAVE_ABI_REGS 1 > | -- list_func > | | -- function_trace_call > | | -- call_direct_funcs // t1 = addr > | -- RESTORE_ABI_REGS 1 > |-- t1 !=0 && jr t1 // goto direct_caller > If you set t1 non-zero, then we always go to direct_caller. I think > the code is equal to set t0=addr. If only focusing on the whole ftrace_regs_caller code, you're right. But we should also take direct_caller code into the consideration, if using t0 here, direct_caller will always return to itself as I described before. > | | -- call_direct_funcs // t0 = addr > | -- RESTORE_ABI_REGS 1 > |-- jr t0 // goto direct_caller > > I think the only problem happens in the below non-existent situation: > f -- regs_caller > | -- SAVE_ABI_REGS 1 > | -- list_func > | | -- call_direct_funcs // t0 = addr > | | -- function_trace_call //t0 contains > direct_caller instead function_trace_call is one type of global_ops->func which doesn't take care of the regs->t0. But the func of IPMODIFY ops (eg. livepatch, kprobe with posthandler) will change regs->epc which will then be restored to t0 in ftrace_regs_caller. And then the address of direct_caller will be covered in this situation. I'm not very clear about the process of the livepatch and kprobe in RISCV but I think we should keep t0 for their ipmodifying. > | -- RESTORE_ABI_REGS 1 > |-- jr t0 // goto direct_caller > > The key issue is whether going to direct_caller correctly depends on > call_direct_funcs called, right? Yes, in other words, call_direct_func informs ftrace_regs_caller that there is a direct caller stored in t1, please jump to it first. > > > ``` > > This diagram looks better. > > > And the direct_caller has a similar implement as ftrace_caller. > > > > > > ``` > > > direct_caller: > > > add sp,sp,-? > > > sd t0,?(sp) > > > sd ra,?(sp) > > > call foo > > > ld t0,?(sp) > > > ld ra,?(sp) > > > add sp,sp,? > > > jr t0 // <- back to function entry > > > ``` > > > > > > If we change regs->t0 as direct_caller and execute `jr t0` directly, > > > the direct_caller will always jump to itself due to its last `jr` inst. --- Here is what I described in the previous email. > > > > > > ``` > > > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > > > function_trace_call // write ringbuffer > > > | > > > -> call_direct_funcs // regs->t0 = direct_caller > > > -> jr t0 // goto direct_caller > > > > > > direct_caller: > > > ... > > > sd t0,?(sp) > > > ... > > > ld t0,?(s0) > > > ... > > > jr t0 // goto direct_caller always > > > ``` > > > > > > Hope I made it clear. > > > > > > > > > > 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 > > > > > > > > > > > > > > > > -- > > > > Best Regards > > > > Guo Ren > > > Thanks, > > > Song > > Thanks, > > Song > > > > -- > Best Regards > Guo Ren Thanks, Song
On Thu, Nov 24, 2022 at 7:49 PM Song Shuai <suagrfillet@gmail.com> wrote: > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 03:40写道： > > > > On Thu, Nov 24, 2022 at 11:09 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > Song Shuai <suagrfillet@gmail.com> 于2022年11月24日周四 02:52写道： > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 02:08写道： > > > > > > > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > > > > > > > Cool job, thx. > > > > > > > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > > > > >> --- > > > > > > >> 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. > > > > > ? > > > > > > > > > > ftrace_regs_caller->call_direct_funcs-> > > > > > arch_ftrace_set_direct_caller-> ftrace_regs_caller jr t0. > > > > > > > > > > Only call_direct_funcs call arch_ftrace_set_direct_caller ! > > > > > > > > > > static void call_direct_funcs(unsigned long ip, unsigned long pip, > > > > > struct ftrace_ops *ops, struct ftrace_regs *fregs) > > > > > { > > > > > struct pt_regs *regs = ftrace_get_regs(fregs); > > > > > unsigned long addr; > > > > > > > > > > addr = ftrace_find_rec_direct(ip); > > > > > if (!addr) > > > > > return; > > > > > > > > > > arch_ftrace_set_direct_caller(regs, addr); > > > > > } > > > > > > > > > When direct_caller and function tracer co-hook a function, the simple > > > > diagram is like this: > > > > > > > > ``` > > > > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > > > > function_trace_call // write ringbuffer > > > > | > > > > -> call_direct_funcs // regs->t1 = direct_caller > > > > -> t1 !=0 && jr t1 // goto direct_caller > > > > ``` > > > > > > > ``` > > > f -- regs_caller -- list_func > > > | | -- function_trace_call > > > | | -- call_direct_funcs // t1 = addr > > > |-- t1 !=0 && jr t1 // goto direct_caller > > Cool diagram! Thx, but we still need a discussion: > > f -- regs_caller > > | -- SAVE_ABI_REGS 1 > > | -- list_func > > | | -- function_trace_call > > | | -- call_direct_funcs // t1 = addr > > | -- RESTORE_ABI_REGS 1 > > |-- t1 !=0 && jr t1 // goto direct_caller > > If you set t1 non-zero, then we always go to direct_caller. I think > > the code is equal to set t0=addr. > If only focusing on the whole ftrace_regs_caller code, you're right. Yes, that's the problem I have; I didn't look at the sample code. t0 -> ftrace caller t1 -> my_tramp1 (direct caller) ra -> original func return addr. Okay, I would include these patches in v4. Reviewed-by: Guo Ren <guoren@kernel.org> > > But we should also take direct_caller code into the consideration, > if using t0 here, direct_caller will always return to itself as I > described before. > > | | -- call_direct_funcs // t0 = addr > > | -- RESTORE_ABI_REGS 1 > > |-- jr t0 // goto direct_caller > > > > I think the only problem happens in the below non-existent situation: > > f -- regs_caller > > | -- SAVE_ABI_REGS 1 > > | -- list_func > > | | -- call_direct_funcs // t0 = addr > > | | -- function_trace_call //t0 contains > > direct_caller instead > function_trace_call is one type of global_ops->func which doesn't take care > of the regs->t0. > > But the func of IPMODIFY ops (eg. livepatch, kprobe with posthandler) > will change regs->epc which will then be restored to t0 in ftrace_regs_caller. > And then the address of direct_caller will be covered in this situation. > I'm not very clear about the process of the livepatch and kprobe in RISCV > but I think we should keep t0 for their ipmodifying. > > | -- RESTORE_ABI_REGS 1 > > |-- jr t0 // goto direct_caller > > > > The key issue is whether going to direct_caller correctly depends on > > call_direct_funcs called, right? > Yes, in other words, call_direct_func informs ftrace_regs_caller that > there is a direct caller stored in t1, please jump to it first. > > > > > ``` > > > This diagram looks better. > > > > And the direct_caller has a similar implement as ftrace_caller. > > > > > > > > ``` > > > > direct_caller: > > > > add sp,sp,-? > > > > sd t0,?(sp) > > > > sd ra,?(sp) > > > > call foo > > > > ld t0,?(sp) > > > > ld ra,?(sp) > > > > add sp,sp,? > > > > jr t0 // <- back to function entry > > > > ``` > > > > > > > > If we change regs->t0 as direct_caller and execute `jr t0` directly, > > > > the direct_caller will always jump to itself due to its last `jr` inst. > --- Here is what I described in the previous email. > > > > > > > > ``` > > > > func -> ftrace_regs_caller -> arch_ftrace_ops_list_func -> > > > > function_trace_call // write ringbuffer > > > > | > > > > -> call_direct_funcs // regs->t0 = direct_caller > > > > -> jr t0 // goto direct_caller > > > > > > > > direct_caller: > > > > ... > > > > sd t0,?(sp) > > > > ... > > > > ld t0,?(s0) > > > > ... > > > > jr t0 // goto direct_caller always > > > > ``` > > > > > > > > Hope I made it clear. > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > -- > > > > > Best Regards > > > > > Guo Ren > > > > Thanks, > > > > Song > > > Thanks, > > > Song > > > > > > > > -- > > Best Regards > > Guo Ren > Thanks, > Song
On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > Cool job, thx. > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > >> --- > >> 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) sd ra,?(sp) call foo ld t1,?(sp) 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
Guo Ren <guoren@kernel.org> 于2022年11月24日周四 15:31写道： > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > Cool job, thx. > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > >> --- > > >> 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. > 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
On Fri, Nov 25, 2022 at 9:53 AM Song Shuai <suagrfillet@gmail.com> wrote: > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 15:31写道： > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > Cool job, thx. > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > >> --- > > > >> 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 call foo ld t0,?(sp) ld t1,?(sp) ld ra,?(sp) add sp,sp,? jr t1 // <- back to function entry > > 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
Guo Ren <guoren@kernel.org> 于2022年11月25日周五 03:10写道： > > On Fri, Nov 25, 2022 at 9:53 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 15:31写道： > > > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > > > Cool job, thx. > > > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > > >> --- > > > > >> 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
On Fri, Nov 25, 2022 at 2:33 PM Song Shuai <suagrfillet@gmail.com> wrote: > > Guo Ren <guoren@kernel.org> 于2022年11月25日周五 03:10写道： > > > > On Fri, Nov 25, 2022 at 9:53 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 15:31写道： > > > > > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > > > > > Cool job, thx. > > > > > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > > > >> --- > > > > > >> 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? Here is my modification: [1] [1]: https://lore.kernel.org/linux-riscv/20221128101201.4144527-1-guoren@kernel.org/ I've tested like this: mount -t debugfs none /sys/kernel/debug/ cd /sys/kernel/debug/tracing/ echo handle_mm_fault > set_ftrace_filter echo 'r:myr handle_mm_fault' > kprobe_events echo function > current_tracer echo 1 > events/kprobes/myr/enable insmod /root/ftrace/ftrace-direct-too.ko cat trace cat-137 [000] ..... 3132.231948: handle_mm_fault <-do_page_fault cat-137 [000] ..... 3132.231973: my_direct_func: handle mm fault vma=000000001ba58b17 address=aaaaaaaab689c8 flags=254 cat-137 [000] ..... 3132.232622: myr: (do_page_fault+0x176/0x424 <- handle_mm_fault) cat-137 [000] ..... 3132.232724: handle_mm_fault <-do_page_fault cat-137 [000] ..... 3132.232750: my_direct_func: handle mm fault vma=000000001ba58b17 address=aaaaaaaab7df9c flags=254 cat-137 [000] ..... 3132.233385: myr: (do_page_fault+0x176/0x424 <- handle_mm_fault) cat-137 [000] ..... 3132.233744: handle_mm_fault <-do_page_fault cat-137 [000] ..... 3132.233772: my_direct_func: handle mm fault vma=000000001ba58b17 address=aaaaaaaab2b2c8 flags=354 cat-137 [000] ..... 3132.234392: myr: (do_page_fault+0x176/0x424 <- handle_mm_fault) cat-137 [000] ..... 3132.234480: handle_mm_fault <-do_page_fault cat-137 [000] ..... 3132.234505: my_direct_func: handle mm fault vma=000000001ba58b17 address=aaaaaaaab5afc0 flags=354 cat-137 [000] ..... 3132.235149: myr: (do_page_fault+0x176/0x424 <- handle_mm_fault) cat-137 [000] ..... 3132.235263: handle_mm_fault <-do_page_fault cat-137 [000] ..... 3132.235289: my_direct_func: handle mm fault vma=0000000071a096de address=fffffff7fac530 flags=254 cat-137 [000] ..... 3132.235893: myr: (do_page_fault+0x176/0x424 <- handle_mm_fault) > > 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
On Mon, Nov 28, 2022 at 6:17 PM Guo Ren <guoren@kernel.org> wrote: > > On Fri, Nov 25, 2022 at 2:33 PM Song Shuai <suagrfillet@gmail.com> wrote: > > > > Guo Ren <guoren@kernel.org> 于2022年11月25日周五 03:10写道： > > > > > > On Fri, Nov 25, 2022 at 9:53 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月24日周四 15:31写道： > > > > > > > > > > On Thu, Nov 24, 2022 at 1:27 AM Song Shuai <suagrfillet@gmail.com> wrote: > > > > > > > > > > > > Guo Ren <guoren@kernel.org> 于2022年11月23日周三 23:02写道： > > > > > > > > > > > > > > Cool job, thx. > > > > > > > > > > > > > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > > > > > >> --- > > > > > > >> 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? > Here is my modification: [1] > [1]: https://lore.kernel.org/linux-riscv/20221128101201.4144527-1-guoren@kernel.org/ > > I've tested like this: > mount -t debugfs none /sys/kernel/debug/ > cd /sys/kernel/debug/tracing/ > echo handle_mm_fault > set_ftrace_filter > echo 'r:myr handle_mm_fault' > kprobe_events > echo function > current_tracer > echo 1 > events/kprobes/myr/enable > insmod /root/ftrace/ftrace-direct-too.ko > cat trace > > cat-137 [000] ..... 3132.231948: handle_mm_fault > <-do_page_fault > cat-137 [000] ..... 3132.231973: my_direct_func: > handle mm fault vma=000000001ba58b17 address=aaaaaaaab689c8 flags=254 > cat-137 [000] ..... 3132.232622: myr: > (do_page_fault+0x176/0x424 <- handle_mm_fault) > cat-137 [000] ..... 3132.232724: handle_mm_fault > <-do_page_fault > cat-137 [000] ..... 3132.232750: my_direct_func: > handle mm fault vma=000000001ba58b17 address=aaaaaaaab7df9c flags=254 > cat-137 [000] ..... 3132.233385: myr: > (do_page_fault+0x176/0x424 <- handle_mm_fault) > cat-137 [000] ..... 3132.233744: handle_mm_fault > <-do_page_fault > cat-137 [000] ..... 3132.233772: my_direct_func: > handle mm fault vma=000000001ba58b17 address=aaaaaaaab2b2c8 flags=354 > cat-137 [000] ..... 3132.234392: myr: > (do_page_fault+0x176/0x424 <- handle_mm_fault) > cat-137 [000] ..... 3132.234480: handle_mm_fault > <-do_page_fault > cat-137 [000] ..... 3132.234505: my_direct_func: > handle mm fault vma=000000001ba58b17 address=aaaaaaaab5afc0 flags=354 > cat-137 [000] ..... 3132.235149: myr: > (do_page_fault+0x176/0x424 <- handle_mm_fault) > cat-137 [000] ..... 3132.235263: handle_mm_fault > <-do_page_fault > cat-137 [000] ..... 3132.235289: my_direct_func: > handle mm fault vma=0000000071a096de address=fffffff7fac530 flags=254 > cat-137 [000] ..... 3132.235893: myr: > (do_page_fault+0x176/0x424 <- handle_mm_fault) I got a fail with no ftrace enabled. Yes, your patch is correct. # insmod /root/ftrace/ftrace-direct-too.ko [ 13.586821] [ 13.586970] ********************************************************** [ 13.587186] ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** [ 13.587396] ** ** [ 13.587594] ** trace_printk() being used. Allocating extra memory. ** [ 13.587804] ** ** [ 13.588240] ** This means that this is a DEBUG kernel and it is ** [ 13.588488] ** unsafe for production use. ** [ 13.588735] ** ** [ 13.588966] ** If you see this message and you are not debugging ** [ 13.589200] ** the kernel, report this immediately to your vendor! ** [ 13.589435] ** ** [ 13.589682] ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** [ 13.589922] ********************************************************** [ 13.632876] Unable to handle kernel access to user memory without uaccess routines at virtual address 0000000000000000 [ 13.633717] Oops [#1] [ 13.633858] Modules linked in: ftrace_direct_too [ 13.634294] CPU: 0 PID: 121 Comm: sh Not tainted 6.1.0-rc1-00016-g459bb40f7d97 #388 [ 13.634652] Hardware name: riscv-virtio,qemu (DT) [ 13.634962] epc : 0x0 [ 13.635313] ra : do_page_fault+0x176/0x424 [ 13.635548] epc : 0000000000000000 ra : ffffffff8000c8d0 sp : ff2000000083be80 [ 13.635819] gp : ffffffff815dc778 tp : ff600000033a9680 t0 : 0000000000000000 [ 13.636144] t1 : 0000000000000000 t2 : 0000000000000000 s0 : ff2000000083bee0 [ 13.636427] s1 : ff2000000083bee0 a0 : ff600000038cb660 a1 : 00fffffff7e74790 [ 13.636725] a2 : 0000000000000255 a3 : 0000000000000000 a4 : 0000000000000000 [ 13.637022] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000000 [ 13.637308] s2 : 00fffffff7e74790 s3 : 000000000000000f s4 : ff6000000319b400 [ 13.637600] s5 : ff600000033a9680 s6 : ff600000038cb660 s7 : ff6000000319b460 [ 13.637889] s8 : 0000000000000255 s9 : 0000000000000001 s10: 00fffffffffff4dc [ 13.638172] s11: 0000000000000004 t3 : 0000000000000000 t4 : 0000000000000000 [ 13.638453] t5 : 0000000000000000 t6 : 0000000000000000 [ 13.638679] status: 0000000200000120 badaddr: 0000000000000000 cause: 000000000000000c [ 13.640035] ---[ end trace 0000000000000000 ]--- > > > > > 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 > > > > -- > Best Regards > Guo Ren
On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > --- > 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; > +} > + > #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) ENTRY(ftrace_regs_caller) + move t1, zero SAVE_ABI_REGS 1 PREPARE_ARGS Shall we use a move here, instead? > 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 >
Guo Ren <guoren@kernel.org> 于2022年11月28日周一 13:17写道： > > On Wed, Nov 23, 2022 at 10:20 PM Song Shuai <suagrfillet@gmail.com> 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@gmail.com> > > --- > > 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; > > +} > > + > > #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) > ENTRY(ftrace_regs_caller) > + move t1, zero > SAVE_ABI_REGS 1 > PREPARE_ARGS > > Shall we use a move here, instead? That seems ok to me. Just move it. > > > 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
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; +} + #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)
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@gmail.com> --- arch/riscv/Kconfig | 1 + arch/riscv/include/asm/ftrace.h | 6 ++++++ arch/riscv/kernel/mcount-dyn.S | 4 ++++ 3 files changed, 11 insertions(+)