* [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
* 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.