On Thu, 15 Feb 2024 11:10:21 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 7 Feb 2024 00:12:06 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Enable CONFIG_HAVE_FUNCTION_GRAPH_FREGS on arm64. Note that this
> > depends on HAVE_DYNAMIC_FTRACE_WITH_ARGS which is enabled if the
> > compiler supports "-fpatchable-function-entry=2". If not, it
> > continue to use ftrace_ret_regs.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> > Changes in v3:
> > - Newly added.
> > ---
> > arch/arm64/Kconfig | 2 ++
> > arch/arm64/include/asm/ftrace.h | 6 ++++++
> > arch/arm64/kernel/entry-ftrace.S | 28 ++++++++++++++++++++++++++++
> > 3 files changed, 36 insertions(+)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index aa7c1d435139..34becd41ae66 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -194,6 +194,8 @@ config ARM64
> > select HAVE_DYNAMIC_FTRACE
> > select HAVE_DYNAMIC_FTRACE_WITH_ARGS \
> > if $(cc-option,-fpatchable-function-entry=2)
> > + select HAVE_FUNCTION_GRAPH_FREGS \
> > + if HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS \
> > if DYNAMIC_FTRACE_WITH_ARGS && DYNAMIC_FTRACE_WITH_CALL_OPS
> > select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS \
> > diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> > index ab158196480c..efd5dbf74dd6 100644
> > --- a/arch/arm64/include/asm/ftrace.h
> > +++ b/arch/arm64/include/asm/ftrace.h
> > @@ -131,6 +131,12 @@ ftrace_regs_set_return_value(struct ftrace_regs *fregs,
> > fregs->regs[0] = ret;
> > }
> >
> > +static __always_inline unsigned long
> > +ftrace_regs_get_frame_pointer(struct ftrace_regs *fregs)
> > +{
> > + return fregs->fp;
> > +}
> > +
> > static __always_inline void
> > ftrace_override_function_with_return(struct ftrace_regs *fregs)
> > {
> > diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> > index f0c16640ef21..d87ccdb9e678 100644
> > --- a/arch/arm64/kernel/entry-ftrace.S
> > +++ b/arch/arm64/kernel/entry-ftrace.S
> > @@ -328,6 +328,33 @@ SYM_FUNC_END(ftrace_stub_graph)
> > * Run ftrace_return_to_handler() before going back to parent.
> > * @fp is checked against the value passed by ftrace_graph_caller().
> > */
> > +#ifdef CONFIG_HAVE_FUNCTION_GRAPH_FREGS
> > +SYM_CODE_START(return_to_handler)
> > + /* save ftrace_regs except for PC */
> > + sub sp, sp, #FREGS_SIZE
> > + stp x0, x1, [sp, #FREGS_X0]
> > + stp x2, x3, [sp, #FREGS_X2]
> > + stp x4, x5, [sp, #FREGS_X4]
> > + stp x6, x7, [sp, #FREGS_X6]
> > + str x8, [sp, #FREGS_X8]
> > + str x29, [sp, #FREGS_FP]
> > + str x9, [sp, #FREGS_LR]
> > + str x10, [sp, #FREGS_SP]
>
> Here too. The above is just garbage.
>
> Let's not fill in garbarge registers. The above is useless on return of a
> function. Heck, adding zeros is better than this. But really, we need to
> have ftrace regs to have some kind of flag that can state what it holds.
> Right now I see three states:
>
> 1 - holds all regs and pt_regs can be retrieved
> 2 - only holds function entry regs (parameters and stack)
> 3 - only holds function exit regs (return value and stack)
>
> Don't save anything else unless needed.
OK, it is reasonable to save the registers depending on the context.
Thanks,
>
> -- Steve
>
> > +
> > + mov x0, sp
> > + bl ftrace_return_to_handler // addr = ftrace_return_to_hander(fregs);
> > + mov x30, x0 // restore the original return address
> > +
> > + /* restore return value regs */
> > + ldp x0, x1, [sp, #FREGS_X0]
> > + ldp x2, x3, [sp, #FREGS_X2]
> > + ldp x4, x5, [sp, #FREGS_X4]
> > + ldp x6, x7, [sp, #FREGS_X6]
> > + add sp, sp, #FREGS_SIZE
> > +
> > + ret
> > +SYM_CODE_END(return_to_handler)
> > +#else /* !CONFIG_HAVE_FUNCTION_GRAPH_FREGS */
> > SYM_CODE_START(return_to_handler)
> > /* save return value regs */
> > sub sp, sp, #FGRET_REGS_SIZE
> > @@ -350,4 +377,5 @@ SYM_CODE_START(return_to_handler)
> >
> > ret
> > SYM_CODE_END(return_to_handler)
> > +#endif /* CONFIG_HAVE_FUNCTION_GRAPH_FREGS */
> > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>
@@ -194,6 +194,8 @@ config ARM64
select HAVE_DYNAMIC_FTRACE
select HAVE_DYNAMIC_FTRACE_WITH_ARGS \
if $(cc-option,-fpatchable-function-entry=2)
+ select HAVE_FUNCTION_GRAPH_FREGS \
+ if HAVE_DYNAMIC_FTRACE_WITH_ARGS
select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS \
if DYNAMIC_FTRACE_WITH_ARGS && DYNAMIC_FTRACE_WITH_CALL_OPS
select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS \
@@ -131,6 +131,12 @@ ftrace_regs_set_return_value(struct ftrace_regs *fregs,
fregs->regs[0] = ret;
}
+static __always_inline unsigned long
+ftrace_regs_get_frame_pointer(struct ftrace_regs *fregs)
+{
+ return fregs->fp;
+}
+
static __always_inline void
ftrace_override_function_with_return(struct ftrace_regs *fregs)
{
@@ -328,6 +328,33 @@ SYM_FUNC_END(ftrace_stub_graph)
* Run ftrace_return_to_handler() before going back to parent.
* @fp is checked against the value passed by ftrace_graph_caller().
*/
+#ifdef CONFIG_HAVE_FUNCTION_GRAPH_FREGS
+SYM_CODE_START(return_to_handler)
+ /* save ftrace_regs except for PC */
+ sub sp, sp, #FREGS_SIZE
+ stp x0, x1, [sp, #FREGS_X0]
+ stp x2, x3, [sp, #FREGS_X2]
+ stp x4, x5, [sp, #FREGS_X4]
+ stp x6, x7, [sp, #FREGS_X6]
+ str x8, [sp, #FREGS_X8]
+ str x29, [sp, #FREGS_FP]
+ str x9, [sp, #FREGS_LR]
+ str x10, [sp, #FREGS_SP]
+
+ mov x0, sp
+ bl ftrace_return_to_handler // addr = ftrace_return_to_hander(fregs);
+ mov x30, x0 // restore the original return address
+
+ /* restore return value regs */
+ ldp x0, x1, [sp, #FREGS_X0]
+ ldp x2, x3, [sp, #FREGS_X2]
+ ldp x4, x5, [sp, #FREGS_X4]
+ ldp x6, x7, [sp, #FREGS_X6]
+ add sp, sp, #FREGS_SIZE
+
+ ret
+SYM_CODE_END(return_to_handler)
+#else /* !CONFIG_HAVE_FUNCTION_GRAPH_FREGS */
SYM_CODE_START(return_to_handler)
/* save return value regs */
sub sp, sp, #FGRET_REGS_SIZE
@@ -350,4 +377,5 @@ SYM_CODE_START(return_to_handler)
ret
SYM_CODE_END(return_to_handler)
+#endif /* CONFIG_HAVE_FUNCTION_GRAPH_FREGS */
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */