* [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules @ 2014-07-24 5:55 Petri Gynther 2014-08-05 16:41 ` Alan Cooper 0 siblings, 1 reply; 5+ messages in thread From: Petri Gynther @ 2014-07-24 5:55 UTC (permalink / raw) To: linux-mips; +Cc: ralf, rostedt, alcooperx, cminyard Dynamic tracing of kernel modules is broken on 32-bit MIPS. When modules are loaded, the kernel crashes when dynamic tracing is enabled with: cd /sys/kernel/debug/tracing echo > set_ftrace_filter echo function > current_tracer 1) arch/mips/kernel/ftrace.c When the kernel boots, or when a module is initialized, ftrace_make_nop() modifies every _mcount call site to eliminate the ftrace overhead. However, when ftrace is later enabled for a call site, ftrace_make_call() does not currently restore the _mcount call correctly for module call sites. Added ftrace_modify_code_2r() and modified ftrace_make_call() to fix this. 2) arch/mips/kernel/mcount.S _mcount assembly routine is supposed to have the caller's _mcount call site address in register a0. However, a0 is currently not calculated correctly for module call sites. a0 should be (ra - 20) or (ra - 24), depending on whether the kernel was built with KBUILD_MCOUNT_RA_ADDRESS or not. This fix has been tested on Broadcom BMIPS5000 processor. Dynamic tracing now works for both built-in functions and module functions. Signed-off-by: Petri Gynther <pgynther@google.com> --- arch/mips/kernel/ftrace.c | 56 +++++++++++++++++++++++++++++++++++++++-------- arch/mips/kernel/mcount.S | 13 +++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c index 60e7e5e..2a72208 100644 --- a/arch/mips/kernel/ftrace.c +++ b/arch/mips/kernel/ftrace.c @@ -63,7 +63,7 @@ static inline int in_kernel_space(unsigned long ip) ((unsigned int)(JAL | (((addr) >> 2) & ADDR_MASK))) static unsigned int insn_jal_ftrace_caller __read_mostly; -static unsigned int insn_lui_v1_hi16_mcount __read_mostly; +static unsigned int insn_la_mcount[2] __read_mostly; static unsigned int insn_j_ftrace_graph_caller __maybe_unused __read_mostly; static inline void ftrace_dyn_arch_init_insns(void) @@ -71,10 +71,10 @@ static inline void ftrace_dyn_arch_init_insns(void) u32 *buf; unsigned int v1; - /* lui v1, hi16_mcount */ + /* la v1, _mcount */ v1 = 3; - buf = (u32 *)&insn_lui_v1_hi16_mcount; - UASM_i_LA_mostly(&buf, v1, MCOUNT_ADDR); + buf = (u32 *)&insn_la_mcount[0]; + UASM_i_LA(&buf, v1, MCOUNT_ADDR); /* jal (ftrace_caller + 8), jump over the first two instruction */ buf = (u32 *)&insn_jal_ftrace_caller; @@ -111,14 +111,47 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, unsigned int new_code2) { int faulted; + mm_segment_t old_fs; safe_store_code(new_code1, ip, faulted); if (unlikely(faulted)) return -EFAULT; - safe_store_code(new_code2, ip + 4, faulted); + + ip += 4; + safe_store_code(new_code2, ip, faulted); if (unlikely(faulted)) return -EFAULT; + + ip -= 4; + old_fs = get_fs(); + set_fs(get_ds()); flush_icache_range(ip, ip + 8); + set_fs(old_fs); + + return 0; +} + +static int ftrace_modify_code_2r(unsigned long ip, unsigned int new_code1, + unsigned int new_code2) +{ + int faulted; + mm_segment_t old_fs; + + ip += 4; + safe_store_code(new_code2, ip, faulted); + if (unlikely(faulted)) + return -EFAULT; + + ip -= 4; + safe_store_code(new_code1, ip, faulted); + if (unlikely(faulted)) + return -EFAULT; + + old_fs = get_fs(); + set_fs(get_ds()); + flush_icache_range(ip, ip + 8); + set_fs(old_fs); + return 0; } #endif @@ -130,13 +163,14 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, * * move at, ra * jal _mcount --> nop + * sub sp, sp, 8 --> nop (CONFIG_32BIT) * * 2. For modules: * * 2.1 For KBUILD_MCOUNT_RA_ADDRESS and CONFIG_32BIT * * lui v1, hi_16bit_of_mcount --> b 1f (0x10000005) - * addiu v1, v1, low_16bit_of_mcount + * addiu v1, v1, low_16bit_of_mcount --> nop (CONFIG_32BIT) * move at, ra * move $12, ra_address * jalr v1 @@ -145,7 +179,7 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, * 2.2 For the Other situations * * lui v1, hi_16bit_of_mcount --> b 1f (0x10000004) - * addiu v1, v1, low_16bit_of_mcount + * addiu v1, v1, low_16bit_of_mcount --> nop (CONFIG_32BIT) * move at, ra * jalr v1 * nop | move $12, ra_address | sub sp, sp, 8 @@ -184,10 +218,14 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) unsigned int new; unsigned long ip = rec->ip; - new = in_kernel_space(ip) ? insn_jal_ftrace_caller : - insn_lui_v1_hi16_mcount; + new = in_kernel_space(ip) ? insn_jal_ftrace_caller : insn_la_mcount[0]; +#ifdef CONFIG_64BIT return ftrace_modify_code(ip, new); +#else + return ftrace_modify_code_2r(ip, new, in_kernel_space(ip) ? + INSN_NOP : insn_la_mcount[1]); +#endif } #define FTRACE_CALL_IP ((unsigned long)(&ftrace_call)) diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S index 539b629..26ceb3c 100644 --- a/arch/mips/kernel/mcount.S +++ b/arch/mips/kernel/mcount.S @@ -84,6 +84,19 @@ _mcount: #endif PTR_SUBU a0, ra, 8 /* arg1: self address */ + PTR_LA t1, _stext + sltu t2, a0, t1 /* t2 = (a0 < _stext) */ + PTR_LA t1, _etext + sltu t3, t1, a0 /* t3 = (a0 > _etext) */ + or t1, t2, t3 + beqz t1, ftrace_call + nop +#if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT) + PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */ +#else + PTR_SUBU a0, a0, 12 +#endif + .globl ftrace_call ftrace_call: nop /* a placeholder for the call to a real tracing function */ -- 2.0.0.526.g5318336 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules 2014-07-24 5:55 [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules Petri Gynther @ 2014-08-05 16:41 ` Alan Cooper [not found] ` <CAGXr9JE7v9-hS3irmdgeaEU2iGLZHshEr_N-Do1UAsZhyzMe2g@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Alan Cooper @ 2014-08-05 16:41 UTC (permalink / raw) To: Petri Gynther; +Cc: linux-mips, ralf, Steven Rostedt, cminyard Petri, >However, when ftrace is later enabled for a call site, ftrace_make_call() >does not currently restore the _mcount call correctly for module call sites I just did some testing on a BMIPS5000 system with a 3.3 kernel and module tracing worked correctly without these changes. I see that the call site is being restored correctly by writing a single 0x3c038001 which is what the compiler originally generated. What are the first 2 instructions of your modules call site and what version kernel and toolchain are you using? Al On Thu, Jul 24, 2014 at 1:55 AM, Petri Gynther <pgynther@google.com> wrote: > Dynamic tracing of kernel modules is broken on 32-bit MIPS. When modules > are loaded, the kernel crashes when dynamic tracing is enabled with: > cd /sys/kernel/debug/tracing > echo > set_ftrace_filter > echo function > current_tracer > > 1) arch/mips/kernel/ftrace.c > When the kernel boots, or when a module is initialized, ftrace_make_nop() > modifies every _mcount call site to eliminate the ftrace overhead. > However, when ftrace is later enabled for a call site, ftrace_make_call() > does not currently restore the _mcount call correctly for module call sites. > Added ftrace_modify_code_2r() and modified ftrace_make_call() to fix this. > > 2) arch/mips/kernel/mcount.S > _mcount assembly routine is supposed to have the caller's _mcount call site > address in register a0. However, a0 is currently not calculated correctly for > module call sites. a0 should be (ra - 20) or (ra - 24), depending on whether > the kernel was built with KBUILD_MCOUNT_RA_ADDRESS or not. > > This fix has been tested on Broadcom BMIPS5000 processor. Dynamic tracing > now works for both built-in functions and module functions. > > Signed-off-by: Petri Gynther <pgynther@google.com> > --- > arch/mips/kernel/ftrace.c | 56 +++++++++++++++++++++++++++++++++++++++-------- > arch/mips/kernel/mcount.S | 13 +++++++++++ > 2 files changed, 60 insertions(+), 9 deletions(-) > > diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c > index 60e7e5e..2a72208 100644 > --- a/arch/mips/kernel/ftrace.c > +++ b/arch/mips/kernel/ftrace.c > @@ -63,7 +63,7 @@ static inline int in_kernel_space(unsigned long ip) > ((unsigned int)(JAL | (((addr) >> 2) & ADDR_MASK))) > > static unsigned int insn_jal_ftrace_caller __read_mostly; > -static unsigned int insn_lui_v1_hi16_mcount __read_mostly; > +static unsigned int insn_la_mcount[2] __read_mostly; > static unsigned int insn_j_ftrace_graph_caller __maybe_unused __read_mostly; > > static inline void ftrace_dyn_arch_init_insns(void) > @@ -71,10 +71,10 @@ static inline void ftrace_dyn_arch_init_insns(void) > u32 *buf; > unsigned int v1; > > - /* lui v1, hi16_mcount */ > + /* la v1, _mcount */ > v1 = 3; > - buf = (u32 *)&insn_lui_v1_hi16_mcount; > - UASM_i_LA_mostly(&buf, v1, MCOUNT_ADDR); > + buf = (u32 *)&insn_la_mcount[0]; > + UASM_i_LA(&buf, v1, MCOUNT_ADDR); > > /* jal (ftrace_caller + 8), jump over the first two instruction */ > buf = (u32 *)&insn_jal_ftrace_caller; > @@ -111,14 +111,47 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, > unsigned int new_code2) > { > int faulted; > + mm_segment_t old_fs; > > safe_store_code(new_code1, ip, faulted); > if (unlikely(faulted)) > return -EFAULT; > - safe_store_code(new_code2, ip + 4, faulted); > + > + ip += 4; > + safe_store_code(new_code2, ip, faulted); > if (unlikely(faulted)) > return -EFAULT; > + > + ip -= 4; > + old_fs = get_fs(); > + set_fs(get_ds()); > flush_icache_range(ip, ip + 8); > + set_fs(old_fs); > + > + return 0; > +} > + > +static int ftrace_modify_code_2r(unsigned long ip, unsigned int new_code1, > + unsigned int new_code2) > +{ > + int faulted; > + mm_segment_t old_fs; > + > + ip += 4; > + safe_store_code(new_code2, ip, faulted); > + if (unlikely(faulted)) > + return -EFAULT; > + > + ip -= 4; > + safe_store_code(new_code1, ip, faulted); > + if (unlikely(faulted)) > + return -EFAULT; > + > + old_fs = get_fs(); > + set_fs(get_ds()); > + flush_icache_range(ip, ip + 8); > + set_fs(old_fs); > + > return 0; > } > #endif > @@ -130,13 +163,14 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, > * > * move at, ra > * jal _mcount --> nop > + * sub sp, sp, 8 --> nop (CONFIG_32BIT) > * > * 2. For modules: > * > * 2.1 For KBUILD_MCOUNT_RA_ADDRESS and CONFIG_32BIT > * > * lui v1, hi_16bit_of_mcount --> b 1f (0x10000005) > - * addiu v1, v1, low_16bit_of_mcount > + * addiu v1, v1, low_16bit_of_mcount --> nop (CONFIG_32BIT) > * move at, ra > * move $12, ra_address > * jalr v1 > @@ -145,7 +179,7 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, > * 2.2 For the Other situations > * > * lui v1, hi_16bit_of_mcount --> b 1f (0x10000004) > - * addiu v1, v1, low_16bit_of_mcount > + * addiu v1, v1, low_16bit_of_mcount --> nop (CONFIG_32BIT) > * move at, ra > * jalr v1 > * nop | move $12, ra_address | sub sp, sp, 8 > @@ -184,10 +218,14 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > unsigned int new; > unsigned long ip = rec->ip; > > - new = in_kernel_space(ip) ? insn_jal_ftrace_caller : > - insn_lui_v1_hi16_mcount; > + new = in_kernel_space(ip) ? insn_jal_ftrace_caller : insn_la_mcount[0]; > > +#ifdef CONFIG_64BIT > return ftrace_modify_code(ip, new); > +#else > + return ftrace_modify_code_2r(ip, new, in_kernel_space(ip) ? > + INSN_NOP : insn_la_mcount[1]); > +#endif > } > > #define FTRACE_CALL_IP ((unsigned long)(&ftrace_call)) > diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S > index 539b629..26ceb3c 100644 > --- a/arch/mips/kernel/mcount.S > +++ b/arch/mips/kernel/mcount.S > @@ -84,6 +84,19 @@ _mcount: > #endif > > PTR_SUBU a0, ra, 8 /* arg1: self address */ > + PTR_LA t1, _stext > + sltu t2, a0, t1 /* t2 = (a0 < _stext) */ > + PTR_LA t1, _etext > + sltu t3, t1, a0 /* t3 = (a0 > _etext) */ > + or t1, t2, t3 > + beqz t1, ftrace_call > + nop > +#if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT) > + PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */ > +#else > + PTR_SUBU a0, a0, 12 > +#endif > + > .globl ftrace_call > ftrace_call: > nop /* a placeholder for the call to a real tracing function */ > -- > 2.0.0.526.g5318336 > ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAGXr9JE7v9-hS3irmdgeaEU2iGLZHshEr_N-Do1UAsZhyzMe2g@mail.gmail.com>]
* Re: [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules [not found] ` <CAGXr9JE7v9-hS3irmdgeaEU2iGLZHshEr_N-Do1UAsZhyzMe2g@mail.gmail.com> @ 2014-08-06 17:12 ` Alan Cooper 2014-08-07 21:25 ` Ralf Baechle 0 siblings, 1 reply; 5+ messages in thread From: Alan Cooper @ 2014-08-06 17:12 UTC (permalink / raw) To: Petri Gynther; +Cc: linux-mips, ralf, Steven Rostedt, cminyard Petri, 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: diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c index 60e7e5e..643e501 100644 --- a/arch/mips/kernel/ftrace.c +++ b/arch/mips/kernel/ftrace.c @@ -157,25 +157,28 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1, int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) { - unsigned int new; unsigned long ip = rec->ip; /* - * If ip is in kernel space, no long call, otherwise, long call is - * needed. + * If the ip is not in kernel space, the call to mcount is a + * long call consisting of multiple instructions so use + * a branch forward to skip the call. If the ip is in the + * kernel, the call is a single instruction so use a NOP. */ - new = in_kernel_space(ip) ? INSN_NOP : INSN_B_1F; + if (!in_kernel_space(ip)) + return ftrace_modify_code(ip, INSN_B_1F); + #ifdef CONFIG_64BIT - return ftrace_modify_code(ip, new); + return ftrace_modify_code(ip, INSN_NOP); #else /* - * On 32 bit MIPS platforms, gcc adds a stack adjust - * instruction in the delay slot after the branch to - * mcount and expects mcount to restore the sp on return. - * This is based on a legacy API and does nothing but - * waste instructions so it's being removed at runtime. + * For non-module kernel functions on 32 bit MIPS platforms, + * gcc adds a stack adjust instruction in the delay slot + * after the branch to mcount and expects mcount to restore + * the sp on return. This is based on a legacy API and does + * nothing but waste instructions so it's being removed at runtime. */ - return ftrace_modify_code_2(ip, new, INSN_NOP); + return ftrace_modify_code_2(ip, INSN_NOP, INSN_NOP); #endif } -- 1.9.0.138.g2de3478 On Tue, Aug 5, 2014 at 4:28 PM, Petri Gynther <pgynther@google.com> wrote: > Hi Al, > > On Tue, Aug 5, 2014 at 9:41 AM, Alan Cooper <alcooperx@gmail.com> wrote: >> >> Petri, >> >> >However, when ftrace is later enabled for a call site, ftrace_make_call() >> >does not currently restore the _mcount call correctly for module call >> > sites >> >> I just did some testing on a BMIPS5000 system with a 3.3 kernel and >> module tracing worked correctly without these changes. I see that the call >> site is being restored correctly by writing a single 0x3c038001 which is >> what >> the compiler originally generated. What are the first 2 instructions of >> your >> modules call site and what version kernel and toolchain are you using? > > > > I am using 3.15 kernel on BMIPS5000 platform. > > Relevant config is: > CONFIG_32BIT=y > CONFIG_FUNCTION_TRACER=y > CONFIG_FUNCTION_GRAPH_TRACER=y > CONFIG_DYNAMIC_FTRACE=y > > Compiler: > $ mipsel-linux-gcc --version > mipsel-linux-gcc (Broadcom stbgcc-4.5.3-1.3) 4.5.3 > > Two sample callsites from bluetooth.ko kernel module: > $ mipsel-linux-objdump -d bluetooth.ko > 00000000 <bt_seq_stop>: > => 0: 3c030000 lui v1,0x0 > 4: 24630000 addiu v1,v1,0 > 8: 03e00821 move at,ra > c: 00006021 move t4,zero > 10: 0060f809 jalr v1 > 14: 27bdfff8 addiu sp,sp,-8 > 18: 8c820080 lw v0,128(a0) > 1c: 3c190000 lui t9,0x0 > 20: 8c440000 lw a0,0(v0) > 24: 27390000 addiu t9,t9,0 > 28: 03200008 jr t9 > 2c: 24840004 addiu a0,a0,4 > > 00000030 <bt_procfs_cleanup>: > => 30: 3c030000 lui v1,0x0 > 34: 24630000 addiu v1,v1,0 > 38: 03e00821 move at,ra > 3c: 00006021 move t4,zero > 40: 0060f809 jalr v1 > 44: 27bdfff8 addiu sp,sp,-8 > ... > > 00000000 <__mcount_loc>: > 0: 00000000 > 4: 00000030 > ... > > $ mipsel-linux-objdump -r bluetooth.ko > RELOCATION RECORDS FOR [.text]: > OFFSET TYPE VALUE > 00000000 R_MIPS_HI16 _mcount > 00000004 R_MIPS_LO16 _mcount > ... > 00000030 R_MIPS_HI16 _mcount > 00000034 R_MIPS_LO16 _mcount > > > When bluetooth.ko is loaded, ftrace_make_nop() turns the first two > instructions to branch + nop: > 00000000 <bt_seq_stop>: > => 0: 10000005 branch 5 instructions forward to 18: > 4: 00000000 nop > 8: 03e00821 move at,ra > c: 00006021 move t4,zero > 10: 0060f809 jalr v1 > 14: 27bdfff8 addiu sp,sp,-8 > 18: > > However, when tracing is later turned on, ftrace_make_call() only converts > the branch back to "lui v1, HI16(_mcount)": > 00000000 <bt_seq_stop>: > => 0: 3c030000 lui v1,0x0 > 4: 00000000 nop > 8: 03e00821 move at,ra > c: 00006021 move t4,zero > 10: 0060f809 jalr v1 > 14: 27bdfff8 addiu sp,sp,-8 > 18: > > The nop at 4 is not turned back to "addiu v1, v1, LO16(_mcount)", which > leads to bad address in v1, subsequent jalr v1, and kernel crash. > > Also, when the module calls _mcount via "jalr v1", ra will contain address > that is 0x18 greater than the recorded mcount callsite. Thus, a0 needs to be > adjusted accordingly in _mcount code. > > -- Petri > > >> >> >> Al >> >> On Thu, Jul 24, 2014 at 1:55 AM, Petri Gynther <pgynther@google.com> >> wrote: >> > Dynamic tracing of kernel modules is broken on 32-bit MIPS. When modules >> > are loaded, the kernel crashes when dynamic tracing is enabled with: >> > cd /sys/kernel/debug/tracing >> > echo > set_ftrace_filter >> > echo function > current_tracer >> > >> > 1) arch/mips/kernel/ftrace.c >> > When the kernel boots, or when a module is initialized, >> > ftrace_make_nop() >> > modifies every _mcount call site to eliminate the ftrace overhead. >> > However, when ftrace is later enabled for a call site, >> > ftrace_make_call() >> > does not currently restore the _mcount call correctly for module call >> > sites. >> > Added ftrace_modify_code_2r() and modified ftrace_make_call() to fix >> > this. >> > >> > 2) arch/mips/kernel/mcount.S >> > _mcount assembly routine is supposed to have the caller's _mcount call >> > site >> > address in register a0. However, a0 is currently not calculated >> > correctly for >> > module call sites. a0 should be (ra - 20) or (ra - 24), depending on >> > whether >> > the kernel was built with KBUILD_MCOUNT_RA_ADDRESS or not. >> > >> > This fix has been tested on Broadcom BMIPS5000 processor. Dynamic >> > tracing >> > now works for both built-in functions and module functions. >> > >> > Signed-off-by: Petri Gynther <pgynther@google.com> >> > --- >> > arch/mips/kernel/ftrace.c | 56 >> > +++++++++++++++++++++++++++++++++++++++-------- >> > arch/mips/kernel/mcount.S | 13 +++++++++++ >> > 2 files changed, 60 insertions(+), 9 deletions(-) >> > >> > diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c >> > index 60e7e5e..2a72208 100644 >> > --- a/arch/mips/kernel/ftrace.c >> > +++ b/arch/mips/kernel/ftrace.c >> > @@ -63,7 +63,7 @@ static inline int in_kernel_space(unsigned long ip) >> > ((unsigned int)(JAL | (((addr) >> 2) & ADDR_MASK))) >> > >> > static unsigned int insn_jal_ftrace_caller __read_mostly; >> > -static unsigned int insn_lui_v1_hi16_mcount __read_mostly; >> > +static unsigned int insn_la_mcount[2] __read_mostly; >> > static unsigned int insn_j_ftrace_graph_caller __maybe_unused >> > __read_mostly; >> > >> > static inline void ftrace_dyn_arch_init_insns(void) >> > @@ -71,10 +71,10 @@ static inline void ftrace_dyn_arch_init_insns(void) >> > u32 *buf; >> > unsigned int v1; >> > >> > - /* lui v1, hi16_mcount */ >> > + /* la v1, _mcount */ >> > v1 = 3; >> > - buf = (u32 *)&insn_lui_v1_hi16_mcount; >> > - UASM_i_LA_mostly(&buf, v1, MCOUNT_ADDR); >> > + buf = (u32 *)&insn_la_mcount[0]; >> > + UASM_i_LA(&buf, v1, MCOUNT_ADDR); >> > >> > /* jal (ftrace_caller + 8), jump over the first two instruction >> > */ >> > buf = (u32 *)&insn_jal_ftrace_caller; >> > @@ -111,14 +111,47 @@ static int ftrace_modify_code_2(unsigned long ip, >> > unsigned int new_code1, >> > unsigned int new_code2) >> > { >> > int faulted; >> > + mm_segment_t old_fs; >> > >> > safe_store_code(new_code1, ip, faulted); >> > if (unlikely(faulted)) >> > return -EFAULT; >> > - safe_store_code(new_code2, ip + 4, faulted); >> > + >> > + ip += 4; >> > + safe_store_code(new_code2, ip, faulted); >> > if (unlikely(faulted)) >> > return -EFAULT; >> > + >> > + ip -= 4; >> > + old_fs = get_fs(); >> > + set_fs(get_ds()); >> > flush_icache_range(ip, ip + 8); >> > + set_fs(old_fs); >> > + >> > + return 0; >> > +} >> > + >> > +static int ftrace_modify_code_2r(unsigned long ip, unsigned int >> > new_code1, >> > + unsigned int new_code2) >> > +{ >> > + int faulted; >> > + mm_segment_t old_fs; >> > + >> > + ip += 4; >> > + safe_store_code(new_code2, ip, faulted); >> > + if (unlikely(faulted)) >> > + return -EFAULT; >> > + >> > + ip -= 4; >> > + safe_store_code(new_code1, ip, faulted); >> > + if (unlikely(faulted)) >> > + return -EFAULT; >> > + >> > + old_fs = get_fs(); >> > + set_fs(get_ds()); >> > + flush_icache_range(ip, ip + 8); >> > + set_fs(old_fs); >> > + >> > return 0; >> > } >> > #endif >> > @@ -130,13 +163,14 @@ static int ftrace_modify_code_2(unsigned long ip, >> > unsigned int new_code1, >> > * >> > * move at, ra >> > * jal _mcount --> nop >> > + * sub sp, sp, 8 --> nop (CONFIG_32BIT) >> > * >> > * 2. For modules: >> > * >> > * 2.1 For KBUILD_MCOUNT_RA_ADDRESS and CONFIG_32BIT >> > * >> > * lui v1, hi_16bit_of_mcount --> b 1f (0x10000005) >> > - * addiu v1, v1, low_16bit_of_mcount >> > + * addiu v1, v1, low_16bit_of_mcount --> nop (CONFIG_32BIT) >> > * move at, ra >> > * move $12, ra_address >> > * jalr v1 >> > @@ -145,7 +179,7 @@ static int ftrace_modify_code_2(unsigned long ip, >> > unsigned int new_code1, >> > * 2.2 For the Other situations >> > * >> > * lui v1, hi_16bit_of_mcount --> b 1f (0x10000004) >> > - * addiu v1, v1, low_16bit_of_mcount >> > + * addiu v1, v1, low_16bit_of_mcount --> nop (CONFIG_32BIT) >> > * move at, ra >> > * jalr v1 >> > * nop | move $12, ra_address | sub sp, sp, 8 >> > @@ -184,10 +218,14 @@ int ftrace_make_call(struct dyn_ftrace *rec, >> > unsigned long addr) >> > unsigned int new; >> > unsigned long ip = rec->ip; >> > >> > - new = in_kernel_space(ip) ? insn_jal_ftrace_caller : >> > - insn_lui_v1_hi16_mcount; >> > + new = in_kernel_space(ip) ? insn_jal_ftrace_caller : >> > insn_la_mcount[0]; >> > >> > +#ifdef CONFIG_64BIT >> > return ftrace_modify_code(ip, new); >> > +#else >> > + return ftrace_modify_code_2r(ip, new, in_kernel_space(ip) ? >> > + INSN_NOP : >> > insn_la_mcount[1]); >> > +#endif >> > } >> > >> > #define FTRACE_CALL_IP ((unsigned long)(&ftrace_call)) >> > diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S >> > index 539b629..26ceb3c 100644 >> > --- a/arch/mips/kernel/mcount.S >> > +++ b/arch/mips/kernel/mcount.S >> > @@ -84,6 +84,19 @@ _mcount: >> > #endif >> > >> > PTR_SUBU a0, ra, 8 /* arg1: self address */ >> > + PTR_LA t1, _stext >> > + sltu t2, a0, t1 /* t2 = (a0 < _stext) */ >> > + PTR_LA t1, _etext >> > + sltu t3, t1, a0 /* t3 = (a0 > _etext) */ >> > + or t1, t2, t3 >> > + beqz t1, ftrace_call >> > + nop >> > +#if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT) >> > + PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded >> > callsite */ >> > +#else >> > + PTR_SUBU a0, a0, 12 >> > +#endif >> > + >> > .globl ftrace_call >> > ftrace_call: >> > nop /* a placeholder for the call to a real tracing function >> > */ >> > -- >> > 2.0.0.526.g5318336 >> > > > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules 2014-08-06 17:12 ` Alan Cooper @ 2014-08-07 21:25 ` Ralf Baechle 2014-08-08 4:11 ` Tony Wu 0 siblings, 1 reply; 5+ messages in thread From: Ralf Baechle @ 2014-08-07 21:25 UTC (permalink / raw) To: Alan Cooper; +Cc: Petri Gynther, linux-mips, Steven Rostedt, cminyard 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules 2014-08-07 21:25 ` Ralf Baechle @ 2014-08-08 4:11 ` Tony Wu 0 siblings, 0 replies; 5+ messages in thread From: Tony Wu @ 2014-08-08 4:11 UTC (permalink / raw) To: Ralf Baechle Cc: Alan Cooper, Petri Gynther, Jun-Ru Chang, cminyard, Steven Rostedt, linux-mips 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)) ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-08-08 4:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-24 5:55 [PATCH] MIPS: Ftrace: Fix dynamic tracing of kernel modules Petri Gynther
2014-08-05 16:41 ` Alan Cooper
[not found] ` <CAGXr9JE7v9-hS3irmdgeaEU2iGLZHshEr_N-Do1UAsZhyzMe2g@mail.gmail.com>
2014-08-06 17:12 ` Alan Cooper
2014-08-07 21:25 ` Ralf Baechle
2014-08-08 4:11 ` Tony Wu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.