Re: [patch 36/38] x86/ftrace: Make it call depth tracking aware

From: Peter Zijlstra <peterz@infradead.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	x86@kernel.org, Linus Torvalds <torvalds@linux-foundation.org>,
	Tim Chen <tim.c.chen@linux.intel.com>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
	Johannes Wikner <kwikner@ethz.ch>,
	Alyssa Milburn <alyssa.milburn@linux.intel.com>,
	Jann Horn <jannh@google.com>, "H.J. Lu" <hjl.tools@gmail.com>,
	Joao Moreira <joao.moreira@intel.com>,
	Joseph Nuzman <joseph.nuzman@intel.com>
Subject: Re: [patch 36/38] x86/ftrace: Make it call depth tracking aware
Date: Tue, 19 Jul 2022 10:46:48 +0200	[thread overview]
Message-ID: <YtZveA3tTi902Dbv@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <20220718170123.4d4bae4a@gandalf.local.home>

On Mon, Jul 18, 2022 at 05:01:23PM -0400, Steven Rostedt wrote:

> From 533f10bd48ffbc4ee5d2a07f0a7fe99aeb1c823a Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> Date: Mon, 18 Jul 2022 16:01:07 -0400
> Subject: [PATCH] ftrace/x86: Remove jumps to ftrace_epilogue
> 
> The jumps to ftrace_epilogue were done as a way to make sure all the
> function tracing trampolines ended at the function graph trampoline, as
> the ftrace_epilogue was the location that it would handle that.
> 
> With the advent of function graph tracer now being just one of the
> callbacks of the function tracer there is no more requirement that all
> trampolines go to a single location.
> 
> Remove the jumps to the ftrace_epilogue and replace them with return
> statements.
> 
> Note, the ftrace_epilogue can probably be renamed to ftrace_stub and the
> weak logic for that could probably be removed. But lets leave that as a
> separate change.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/ftrace_64.S | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index dfeb227de561..8f225fafa5fb 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -173,7 +173,9 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>  SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
>  	ANNOTATE_NOENDBR
>  
> -	jmp ftrace_epilogue
> +	UNWIND_HINT_FUNC
> +	ENDBR

Only the RET should do I think; you definitely don't need an ENDBR here
nor do you need to override the unwind hint. Lemme try..

Yeah, the below is sufficient:

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index dfeb227de561..d6679b65b6f2 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -172,8 +172,7 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	 */
 SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
 	ANNOTATE_NOENDBR
-
-	jmp ftrace_epilogue
+	RET
 SYM_FUNC_END(ftrace_caller);
 STACK_FRAME_NON_STANDARD_FP(ftrace_caller)
 
@@ -269,7 +268,7 @@ SYM_INNER_LABEL(ftrace_regs_caller_jmp, SYM_L_GLOBAL)
 	 */
 SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
 	ANNOTATE_NOENDBR
-	jmp ftrace_epilogue
+	RET
 
 	/* Swap the flags with orig_rax */
 1:	movq MCOUNT_REG_SIZE(%rsp), %rdi
@@ -280,7 +279,7 @@ SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
 	/* Restore flags */
 	popfq
 	UNWIND_HINT_FUNC
-	jmp	ftrace_epilogue
+	RET
 
 SYM_FUNC_END(ftrace_regs_caller)
 STACK_FRAME_NON_STANDARD_FP(ftrace_regs_caller)
