Re: [PATCH v7 24/36] x86/ftrace: Enable HAVE_FUNCTION_GRAPH_FREGS

Masami Hiramatsu
2024-02-16T08:54:29Z

On Thu, 15 Feb 2024 11:08:08 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 7 Feb 2024 00:11:56 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Support HAVE_FUNCTION_GRAPH_FREGS on x86-64, which saves ftrace_regs
> > on the stack in ftrace_graph return trampoline so that the callbacks
> > can access registers via ftrace_regs APIs.
> >
> > Note that this only recovers 'rax' and 'rdx' registers because other
> > registers are not used anymore and recovered by caller. 'rax' and
> > 'rdx' will be used for passing the return value.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> > Changes in v3:
> > - Add a comment about rip.
> > Changes in v2:
> > - Save rsp register and drop clearing orig_ax.
> > ---
> > arch/x86/Kconfig | 3 ++-
> > arch/x86/kernel/ftrace_64.S | 37 +++++++++++++++++++++++++++++--------
> > 2 files changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 5edec175b9bf..ccf17d8b6f5f 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -223,7 +223,8 @@ config X86
> > select HAVE_FAST_GUP
> > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE
> > select HAVE_FTRACE_MCOUNT_RECORD
> > - select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> > + select HAVE_FUNCTION_GRAPH_FREGS if HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > + select HAVE_FUNCTION_GRAPH_RETVAL if !HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > select HAVE_FUNCTION_GRAPH_TRACER if X86_32 || (X86_64 && DYNAMIC_FTRACE)
> > select HAVE_FUNCTION_TRACER
> > select HAVE_GCC_PLUGINS
> > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> > index 214f30e9f0c0..8a16f774604e 100644
> > --- a/arch/x86/kernel/ftrace_64.S
> > +++ b/arch/x86/kernel/ftrace_64.S
> > @@ -348,21 +348,42 @@ STACK_FRAME_NON_STANDARD_FP(__fentry__)
> > SYM_CODE_START(return_to_handler)
> > UNWIND_HINT_UNDEFINED
> > ANNOTATE_NOENDBR
> > - subq $24, %rsp
> > + /*
> > + * Save the registers requires for ftrace_regs;
> > + * rax, rcx, rdx, rdi, rsi, r8, r9 and rbp
> > + */
> > + subq $(FRAME_SIZE), %rsp
> > + movq %rax, RAX(%rsp)
> > + movq %rcx, RCX(%rsp)
> > + movq %rdx, RDX(%rsp)
> > + movq %rsi, RSI(%rsp)
> > + movq %rdi, RDI(%rsp)
> > + movq %r8, R8(%rsp)
> > + movq %r9, R9(%rsp)
> > + movq %rbp, RBP(%rsp)
>
> This unconditionally slows down function graph tracer for no good reason.
>
> Most of the above is going to be garbage anyway, except the rax and rdx.
>
> I would recommend than we set something else in the ftrace regs that states
> this only holds return values. Anything else will just get invalid.
>
> I'm really against saving garbage. The purpose of ftrace_regs is that it
> can hold incomplete data.

Ah, OK. I misunderstood. I thought ftrace_regs was expected to be filled
with reduced (arch-defined) register set. But it just ensures that holds
some registers depends on the context.

Thank you,

>
> -- Steve
>
>
> > + /*
> > + * orig_ax is not cleared because it is used for indicating the direct
> > + * trampoline in the fentry. And rip is not set because we don't know
> > + * the correct return address here.
> > + */
> > +
> > + leaq FRAME_SIZE(%rsp), %rcx
> > + movq %rcx, RSP(%rsp)
> >
> > - /* Save the return values */
> > - movq %rax, (%rsp)
> > - movq %rdx, 8(%rsp)
> > - movq %rbp, 16(%rsp)
> > movq %rsp, %rdi
> >
> > call ftrace_return_to_handler
> >
> > movq %rax, %rdi
> > - movq 8(%rsp), %rdx
> > - movq (%rsp), %rax
> >
> > - addq $24, %rsp
> > + /*
> > + * Restore only rax and rdx because other registers are not used
> > + * for return value nor callee saved. Caller will reuse/recover it.
> > + */
> > + movq RDX(%rsp), %rdx
> > + movq RAX(%rsp), %rax
> > +
> > + addq $(FRAME_SIZE), %rsp
> > /*
> > * Jump back to the old return address. This cannot be JMP_NOSPEC rdi
> > * since IBT would demand that contain ENDBR, which simply isn't so for
>

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>