[PATCH 0/2] [git-pull] powerpc, ftrace clean ups

[PATCH 0/2] [git-pull] powerpc, ftrace clean ups

[Steven Rostedt]

Benjamin,

Please pull the latest ppc/ftrace tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
ppc/ftrace


Steven Rostedt (2):
      powerpc, ftrace: use unsigned int for instruction manipulation
      powerpc, ftrace: use create_branch lib function

----
 arch/powerpc/kernel/ftrace.c |   59 +++++++++++++----------------------------
 1 files changed, 19 insertions(+), 40 deletions(-)
-- 

[PATCH 1/2] powerpc, ftrace: use unsigned int for instruction manipulation

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

The original port of ftrace to PowerPC kept a lot of the code used
by x86. Some of this code was to handle x86's 5 byte instruction.
This was handled by using character arrays to manipulate the
code.

PowerPC has a consistent 4 byte instruction. Using unsigned ints
makes the code more efficient as well as more readable.
By converting to use unsigned ints to represent instructions,
I was able to remove the side effects that were needed for
manipulating character strings.

  i.e. memcpy and memcmp

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/kernel/ftrace.c |   45 ++++++++++++++++-------------------------
 1 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index dddd99b..610c852 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -31,22 +31,20 @@
 #endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-static unsigned int ftrace_nop = PPC_NOP_INSTR;
-
 static unsigned int ftrace_calc_offset(long ip, long addr)
 {
 	return (int)(addr - ip);
 }
 
-static unsigned char *ftrace_nop_replace(void)
+static unsigned int ftrace_nop_replace(void)
 {
-	return (char *)&ftrace_nop;
+	return PPC_NOP_INSTR;
 }
 
-static unsigned char *
+static unsigned int
 ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
 {
-	static unsigned int op;
+	unsigned int op;
 
 	/*
 	 * It would be nice to just use create_function_call, but that will
@@ -60,11 +58,7 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
 	op = 0x48000000 | (link ? 1 : 0);
 	op |= (ftrace_calc_offset(ip, addr) & 0x03fffffc);
 
-	/*
-	 * No locking needed, this must be called via kstop_machine
-	 * which in essence is like running on a uniprocessor machine.
-	 */
-	return (unsigned char *)&op;
+	return op;
 }
 
 #ifdef CONFIG_PPC64
@@ -76,10 +70,9 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
 #endif
 
 static int
-ftrace_modify_code(unsigned long ip, unsigned char *old_code,
-		   unsigned char *new_code)
+ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
 {
-	unsigned char replaced[MCOUNT_INSN_SIZE];
+	unsigned int replaced;
 
 	/*
 	 * Note: Due to modules and __init, code can
@@ -92,15 +85,15 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 	 */
 
 	/* read the text we want to modify */
-	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
+	if (probe_kernel_read(&replaced, (void *)ip, MCOUNT_INSN_SIZE))
 		return -EFAULT;
 
 	/* Make sure it is what we expect it to be */
-	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
+	if (replaced != old)
 		return -EINVAL;
 
 	/* replace the text with the new text */
-	if (probe_kernel_write((void *)ip, new_code, MCOUNT_INSN_SIZE))
+	if (probe_kernel_write((void *)ip, &new, MCOUNT_INSN_SIZE))
 		return -EPERM;
 
 	flush_icache_range(ip, ip + 8);
@@ -336,8 +329,8 @@ __ftrace_make_nop(struct module *mod,
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {
-	unsigned char *old, *new;
 	unsigned long ip = rec->ip;
+	unsigned int old, new;
 
 	/*
 	 * If the calling address is more that 24 bits away,
@@ -475,8 +468,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
-	unsigned char *old, *new;
 	unsigned long ip = rec->ip;
+	unsigned int old, new;
 
 	/*
 	 * If the calling address is more that 24 bits away,
@@ -511,10 +504,10 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
 	unsigned long ip = (unsigned long)(&ftrace_call);
-	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	unsigned int old, new;
 	int ret;
 
-	memcpy(old, &ftrace_call, MCOUNT_INSN_SIZE);
+	old = *(unsigned int *)&ftrace_call;
 	new = ftrace_call_replace(ip, (unsigned long)func, 1);
 	ret = ftrace_modify_code(ip, old, new);
 
@@ -543,10 +536,9 @@ int ftrace_enable_ftrace_graph_caller(void)
 	unsigned long ip = (unsigned long)(&ftrace_graph_call);
 	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
 	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	unsigned int old, new;
 
-	new = ftrace_call_replace(ip, stub, 0);
-	memcpy(old, new, MCOUNT_INSN_SIZE);
+	old = ftrace_call_replace(ip, stub, 0);
 	new = ftrace_call_replace(ip, addr, 0);
 
 	return ftrace_modify_code(ip, old, new);
@@ -557,10 +549,9 @@ int ftrace_disable_ftrace_graph_caller(void)
 	unsigned long ip = (unsigned long)(&ftrace_graph_call);
 	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
 	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	unsigned int old, new;
 
-	new = ftrace_call_replace(ip, addr, 0);
-	memcpy(old, new, MCOUNT_INSN_SIZE);
+	old = ftrace_call_replace(ip, addr, 0);
 	new = ftrace_call_replace(ip, stub, 0);
 
 	return ftrace_modify_code(ip, old, new);
-- 
1.5.6.5

-- 

[PATCH 2/2] powerpc, ftrace: use create_branch lib function

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up, remove duplicate code

When ftrace was first ported to PowerPC, there existed a
create_function_call that would create the instruction to make a call
to a given address. Unfortunately, this call expected to write to
the address it was given, and since it used the address to calculate
the offset, it could not be faked.

ftrace needed a way to create the instruction without actually writing
that instruction to the text section. So ftrace had to implement its
own code.

Now we have create_branch in the code patching library, which does
exactly what ftrace needs. This patch replaces ftrace's implementation
with the library function.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/kernel/ftrace.c |   14 +-------------
 1 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 610c852..4c75a1c 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -31,11 +31,6 @@
 #endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-static unsigned int ftrace_calc_offset(long ip, long addr)
-{
-	return (int)(addr - ip);
-}
-
 static unsigned int ftrace_nop_replace(void)
 {
 	return PPC_NOP_INSTR;
@@ -46,17 +41,10 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
 {
 	unsigned int op;
 
-	/*
-	 * It would be nice to just use create_function_call, but that will
-	 * update the code itself. Here we need to just return the
-	 * instruction that is going to be modified, without modifying the
-	 * code.
-	 */
 	addr = GET_ADDR(addr);
 
 	/* if (link) set op to 'bl' else 'b' */
-	op = 0x48000000 | (link ? 1 : 0);
-	op |= (ftrace_calc_offset(ip, addr) & 0x03fffffc);
+	op = create_branch((unsigned int *)ip, addr, link ? 1 : 0);
 
 	return op;
 }
-- 
1.5.6.5

-- 

Re: [PATCH 2/2] powerpc, ftrace: use create_branch lib function

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Fri, 2009-02-13 at 10:00 -0500, Steven Rostedt wrote:
> plain text document attachment
> (0002-powerpc-ftrace-use-create_branch-lib-function.patch)
> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: clean up, remove duplicate code
> 
> When ftrace was first ported to PowerPC, there existed a
> create_function_call that would create the instruction to make a call
> to a given address. Unfortunately, this call expected to write to
> the address it was given, and since it used the address to calculate
> the offset, it could not be faked.
> 
> ftrace needed a way to create the instruction without actually writing
> that instruction to the text section. So ftrace had to implement its
> own code.
> 
> Now we have create_branch in the code patching library, which does
> exactly what ftrace needs. This patch replaces ftrace's implementation
> with the library function.

Thanks for doing this, I was going to once the ftrace code had settled a
little but you beat me to it.

> @@ -46,17 +41,10 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
>  {
>  	unsigned int op;
>  
> -	/*
> -	 * It would be nice to just use create_function_call, but that will
> -	 * update the code itself. Here we need to just return the
> -	 * instruction that is going to be modified, without modifying the
> -	 * code.
> -	 */
>  	addr = GET_ADDR(addr);
>  
>  	/* if (link) set op to 'bl' else 'b' */
> -	op = 0x48000000 | (link ? 1 : 0);
> -	op |= (ftrace_calc_offset(ip, addr) & 0x03fffffc);
> +	op = create_branch((unsigned int *)ip, addr, link ? 1 : 0);

If I was feeling nit-picky I'd say you should use:

op = create_branch((unsigned int *)ip, addr, link ? BRANCH_SET_LINK : 0);


But admittedly we're unlikely to ever change the flag handling, so it's
probably not worth the effort of a respin (or this email :).

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] powerpc, ftrace: use create_branch lib function

[Steven Rostedt]

On Sun, 15 Feb 2009, Michael Ellerman wrote:
> >  
> >  	/* if (link) set op to 'bl' else 'b' */
> > -	op = 0x48000000 | (link ? 1 : 0);
> > -	op |= (ftrace_calc_offset(ip, addr) & 0x03fffffc);
> > +	op = create_branch((unsigned int *)ip, addr, link ? 1 : 0);
> 
> If I was feeling nit-picky I'd say you should use:
> 
> op = create_branch((unsigned int *)ip, addr, link ? BRANCH_SET_LINK : 0);
> 
> 
> But admittedly we're unlikely to ever change the flag handling, so it's
> probably not worth the effort of a respin (or this email :).

Not worth the effort of a respin, I agree.

But it would be worth the effort of a clean up patch ;-)

Not for "oh this flag might change", but for reviewers looking at the
code and knowing exactly what that '1' means.

-- Steve