On Thu, 15 Feb 2024 11:08:08 -0500 Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > On Wed, 7 Feb 2024 00:11:56 +0900 > "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote: > > > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx>
