[v7,25/36] arm64: ftrace: Enable HAVE_FUNCTION_GRAPH_FREGS

Commit Message

Masami Hiramatsu (Google) Feb. 6, 2024, 3:12 p.m. UTC

From: Masami Hiramatsu (Google) <[e-mail]>

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) <[e-mail]>
---
 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(+)

Comments

Steven Rostedt Feb. 15, 2024, 4:10 p.m. UTC | #1

On Wed,  7 Feb 2024 00:12:06 +0900
"Masami Hiramatsu (Google)" <[e-mail]> wrote:

> From: Masami Hiramatsu (Google) <[e-mail]>
> 
> 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) <[e-mail]>
> ---
>  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.

-- 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 */

Masami Hiramatsu (Google) Feb. 16, 2024, 12:49 p.m. UTC | #2

On Thu, 15 Feb 2024 11:10:21 -0500
Steven Rostedt <[e-mail]> wrote:

> On Wed,  7 Feb 2024 00:12:06 +0900
> "Masami Hiramatsu (Google)" <[e-mail]> wrote:
> 
> > From: Masami Hiramatsu (Google) <[e-mail]>
> > 
> > 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) <[e-mail]>
> > ---
> >  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 */
>

Patch

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]
+
+	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 */