[PATCH] mips: ftrace: fix static function graph tracing

[PATCH] mips: ftrace: fix static function graph tracing

[Matthias Schiffer]

ftrace_graph_caller was never run after calling ftrace_trace_function,
breaking the function graph tracer. Fix this, bringing it in line with the
x86 implementation.

While we're at it, also streamline the control flow of _mcount a bit to
reduce the number of branches.

This issue was reported before:
https://www.linux-mips.org/archives/linux-mips/2014-11/msg00295.html

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---

Caveats: I've only tested this on 32bit; it would be great if someone with
MIPS64 hardware or a working emulator setup could give it a spin. My test
device runs on kernel 4.9.y, but I don't expect any problems applying the
fix to newer kernels, given that this code is basically unchanged since
2014.

I'm not sure what the correct Fixes: line would be. I did not bother to
bisect the issue and could not find the commit introducing it by inspecting
the log (assuming the tracer worked at some point).


 arch/mips/kernel/mcount.S | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
index f2ee7e1e3342..cff52b283e03 100644
--- a/arch/mips/kernel/mcount.S
+++ b/arch/mips/kernel/mcount.S
@@ -119,10 +119,20 @@ NESTED(_mcount, PT_SIZE, ra)
 EXPORT_SYMBOL(_mcount)
 	PTR_LA	t1, ftrace_stub
 	PTR_L	t2, ftrace_trace_function /* Prepare t2 for (1) */
-	bne	t1, t2, static_trace
+	beq	t1, t2, fgraph_trace
 	 nop
 
+	MCOUNT_SAVE_REGS
+
+	move	a0, ra		/* arg1: self return address */
+	jalr	t2		/* (1) call *ftrace_trace_function */
+	 move	a1, AT		/* arg2: parent's return address */
+
+	MCOUNT_RESTORE_REGS
+
+fgraph_trace:
 #ifdef	CONFIG_FUNCTION_GRAPH_TRACER
+	PTR_LA	t1, ftrace_stub
 	PTR_L	t3, ftrace_graph_return
 	bne	t1, t3, ftrace_graph_caller
 	 nop
@@ -131,24 +141,11 @@ EXPORT_SYMBOL(_mcount)
 	bne	t1, t3, ftrace_graph_caller
 	 nop
 #endif
-	b	ftrace_stub
-#ifdef CONFIG_32BIT
-	 addiu sp, sp, 8
-#else
-	 nop
-#endif
 
-static_trace:
-	MCOUNT_SAVE_REGS
-
-	move	a0, ra		/* arg1: self return address */
-	jalr	t2		/* (1) call *ftrace_trace_function */
-	 move	a1, AT		/* arg2: parent's return address */
-
-	MCOUNT_RESTORE_REGS
 #ifdef CONFIG_32BIT
 	addiu sp, sp, 8
 #endif
+
 	.globl ftrace_stub
 ftrace_stub:
 	RETURN_BACK
-- 
2.16.2

Re: [PATCH] mips: ftrace: fix static function graph tracing

[Matt Redfearn]

Hi Matthias,

On 24/03/18 16:57, Matthias Schiffer wrote:
> ftrace_graph_caller was never run after calling ftrace_trace_function,
> breaking the function graph tracer. Fix this, bringing it in line with the
> x86 implementation.
> 
> While we're at it, also streamline the control flow of _mcount a bit to
> reduce the number of branches.
> 
> This issue was reported before:
> https://www.linux-mips.org/archives/linux-mips/2014-11/msg00295.html
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
> 
> Caveats: I've only tested this on 32bit; it would be great if someone with
> MIPS64 hardware or a working emulator setup could give it a spin. My test

I've tested this patch fixes static function graph tracing on the 
following platforms:
QEMU (MIPS32r2 P5600)
Creator Ci40 (MIPS32r2 InterAptiv)
Cavium Octeon II (MIPS64r2)
MIPS Boston FPGA with I6400 (MIPS64r6)

So
Tested-by: Matt Redfearn <matt.redfearn@mips.com>

> device runs on kernel 4.9.y, but I don't expect any problems applying the
> fix to newer kernels, given that this code is basically unchanged since
> 2014.

Indeed, applies fine to v4.16-rc7 (which I mainly tested with)

> 
> I'm not sure what the correct Fixes: line would be. I did not bother to
> bisect the issue and could not find the commit introducing it by inspecting
> the log (assuming the tracer worked at some point).

I verified that the patch applies to and fixes the issue as far back as 
3.19 on the malta platform using qemu. Going further back I ran into 
toolchain issues that prevented builds. The earliest version the patch 
applies cleanly to is v3.17.
Like you, I don't see where this got broken from just looking at 
mcount.S, and not being able to bisect with build / test makes it rather 
difficult to find out.

Thanks,
Matt

> 
> 
>   arch/mips/kernel/mcount.S | 27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
> index f2ee7e1e3342..cff52b283e03 100644
> --- a/arch/mips/kernel/mcount.S
> +++ b/arch/mips/kernel/mcount.S
> @@ -119,10 +119,20 @@ NESTED(_mcount, PT_SIZE, ra)
>   EXPORT_SYMBOL(_mcount)
>   	PTR_LA	t1, ftrace_stub
>   	PTR_L	t2, ftrace_trace_function /* Prepare t2 for (1) */
> -	bne	t1, t2, static_trace
> +	beq	t1, t2, fgraph_trace
>   	 nop
>   
> +	MCOUNT_SAVE_REGS
> +
> +	move	a0, ra		/* arg1: self return address */
> +	jalr	t2		/* (1) call *ftrace_trace_function */
> +	 move	a1, AT		/* arg2: parent's return address */
> +
> +	MCOUNT_RESTORE_REGS
> +
> +fgraph_trace:
>   #ifdef	CONFIG_FUNCTION_GRAPH_TRACER
> +	PTR_LA	t1, ftrace_stub
>   	PTR_L	t3, ftrace_graph_return
>   	bne	t1, t3, ftrace_graph_caller
>   	 nop
> @@ -131,24 +141,11 @@ EXPORT_SYMBOL(_mcount)
>   	bne	t1, t3, ftrace_graph_caller
>   	 nop
>   #endif
> -	b	ftrace_stub
> -#ifdef CONFIG_32BIT
> -	 addiu sp, sp, 8
> -#else
> -	 nop
> -#endif
>   
> -static_trace:
> -	MCOUNT_SAVE_REGS
> -
> -	move	a0, ra		/* arg1: self return address */
> -	jalr	t2		/* (1) call *ftrace_trace_function */
> -	 move	a1, AT		/* arg2: parent's return address */
> -
> -	MCOUNT_RESTORE_REGS
>   #ifdef CONFIG_32BIT
>   	addiu sp, sp, 8
>   #endif
> +
>   	.globl ftrace_stub
>   ftrace_stub:
>   	RETURN_BACK
>