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