Re: [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules

[Tony Wu <tung7970@gmail.com>]

On Thu, Aug 07, 2014 at 11:25:14PM +0200, Ralf Baechle wrote:
> On Wed, Aug 06, 2014 at 01:12:06PM -0400, Alan Cooper wrote:
> 
> > Actually , there's no reason to write the second NOP when nop'ing the
> > mcount call site in a module. This was done to remove the stack adjust
> > instruction which only exists at this location for internal kernel
> > routines. The following diff seems like a simpler way to solve issue
> > #1:
> 
> Oh?
> 
> $ mips-linux-objdump -d --reloc net/sctp/sctp.ko
> [...]
> 00000000 <sctp_sm_lookup_event>:
>        0:       27bdffe8        addiu   sp,sp,-24
>        4:       afbf0014        sw      ra,20(sp)
>        8:       3c030000        lui     v1,0x0
>                         8: R_MIPS_HI16  _mcount
>        c:       24630000        addiu   v1,v1,0
>                         c: R_MIPS_LO16  _mcount
>       10:       03e00821        move    at,ra
>       14:       27ac0014        addiu   t4,sp,20
>       18:       0060f809        jalr    v1
>       1c:       27bdfff8        addiu   sp,sp,-8  <====
> [...]
>       64:       27bd0018        addiu   sp,sp,24
>       68:       03e00008        jr      ra
> [...]
> 
> So the stack adjustment also exists for modules.
> 
> Or am I missunderstanding something?
> 
>   Ralf
> 

We have a workaround for dynamic ftrace under 32bit mode back in Feburary. I thought
it is not good enough but maybe this is the right solution for issue #1 ?

Tony

commit d3167328b1bd63c22abb129a17fcb658e11c2a7b
Author: Jun-Ru Chang <jrjang@gmail.com>
Date:   Thu Feb 27 15:23:34 2014 +0800

    mips: ftrace: fix ftrace_make_call long call restore
    
    In case of long call under 32bit mode, two instructions,
    lui and addiu, are required to load the mcount address into
    the jump register.
    
    In ftrace_make_nop, both instructions are marked as nop, so
    in ftrace_make_call, we have to restore both of them.
    
    Signed-off-by: Jun-Ru Chang <jrjang@gmail.com>
    Signed-off-by: Tony Wu <tung7970@gmail.com>

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 9bb8bcd..b12858c 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -61,6 +61,9 @@ static inline int in_kernel_space(unsigned long ip)
 
 static unsigned int insn_jal_ftrace_caller __read_mostly;
 static unsigned int insn_lui_v1_hi16_mcount __read_mostly;
+#ifndef CONFIG_64BIT
+static unsigned int insn_add_v1_lo16_mcount __read_mostly;
+#endif
 static unsigned int insn_j_ftrace_graph_caller __maybe_unused __read_mostly;
 
 static inline void ftrace_dyn_arch_init_insns(void)
@@ -73,6 +76,12 @@ static inline void ftrace_dyn_arch_init_insns(void)
 	buf = (u32 *)&insn_lui_v1_hi16_mcount;
 	UASM_i_LA_mostly(&buf, v1, MCOUNT_ADDR);
 
+#ifndef CONFIG_64BIT
+	/* addiu v1, vi, lo16_mcount */
+	buf = (u32 *)&insn_add_v1_lo16_mcount;
+	UASM_i_LA(&buf, v1, MCOUNT_ADDR);
+#endif
+
 	/* jal (ftrace_caller + 8), jump over the first two instruction */
 	buf = (u32 *)&insn_jal_ftrace_caller;
 	uasm_i_jal(&buf, (FTRACE_ADDR + 8) & JUMP_RANGE_MASK);
@@ -180,7 +189,10 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
 		insn_lui_v1_hi16_mcount;
 
-	return ftrace_modify_code(ip, new);
+	if (IS_BUILTIN(CONFIG_64BIT) || new == insn_jal_ftrace_caller)
+		return ftrace_modify_code(ip, new);
+	else
+		return ftrace_modify_code_2(ip, new, insn_add_v1_lo16_mcount);
 }
 
 #define FTRACE_CALL_IP ((unsigned long)(&ftrace_call))