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

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

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>