[PATCH 0/4] ftrace: PPC port of dynamic ftrace fixes

[PATCH 0/4] ftrace: PPC port of dynamic ftrace fixes

[Steven Rostedt]

This series of patches addresses the problems with PPC dynamic ftrace
updates that Paul Mackerras pointed out.

The following patches are in:

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

    branch: tip/ppc


Steven Rostedt (4):
      ftrace,ppc: consolidate dyn ftrace ppc code between 32 and 64 bit
      ftrace,ppc: fix test of 24bit jump
      ftrace,ppc: fix module check in dynamic ftrace code
      ftrace,ppc: fix test of branch link

----
 arch/powerpc/kernel/ftrace.c |  168 +++++++++++++++++++++---------------------
 1 files changed, 83 insertions(+), 85 deletions(-)

[PATCH 1/4] ftrace,ppc: consolidate dyn ftrace ppc code between 32 and 64 bit

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-ppc-consolidate-dyn-ftrace-ppc-code-between.patch --]
[-- Type: text/plain, Size: 7453 bytes --]

Impact: code clean up between PPC 32 and 64 bit platforms

The modification of code for dynamic ftrace between 32 bit and 64 bit PPC
platforms is a bit different. But there is also several things that are
similar. This patch consolidates some of the code between the two
algorithms so that fixes to one make it to the other.

Paul Mackerras pointed out a few mistakes in the code. This patch does
not fix those mistakes. This patch only consolidates the 32bit and 64bit
with helper routines that the fixes can be made in one place.

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

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 57fdda8..9360fd1 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -109,6 +109,9 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 	return 0;
 }
 
+/*
+ * Helper functions that are the same for both PPC64 and PPC32.
+ */
 static int test_24bit_addr(unsigned long ip, unsigned long addr)
 {
 	unsigned long diff;
@@ -119,6 +122,34 @@ static int test_24bit_addr(unsigned long ip, unsigned long addr)
 	return !(diff & ((unsigned long)-1 << 24));
 }
 
+static int is_bl_op(unsigned int op)
+{
+	return (op & 0xff000000) == 0x48000000;
+}
+
+static int test_offset(unsigned long offset)
+{
+	return (offset + 0x2000000 > 0x3ffffff) || ((offset & 3) != 0);
+}
+
+static unsigned long find_bl_target(unsigned long ip, unsigned int op)
+{
+	static int offset;
+
+	offset = (op & 0x03fffffc);
+	/* make it signed */
+	if (offset & 0x02000000)
+		offset |= 0xfe000000;
+
+	return ip + (long)offset;
+}
+
+static unsigned int branch_offset(unsigned long offset)
+{
+	/* return "bl ip+offset" */
+	return 0x48000001 | (offset & 0x03fffffc);
+}
+
 #ifdef CONFIG_PPC64
 static int
 __ftrace_make_nop(struct module *mod,
@@ -132,43 +163,18 @@ __ftrace_make_nop(struct module *mod,
 	unsigned long tramp;
 	int offset;
 
-	/*
-	 * Out of range jumps are called from modules.
-	 * We should either already have a pointer to the module
-	 * or it has been passed in.
-	 */
-	if (!rec->arch.mod) {
-		if (!mod) {
-			printk(KERN_ERR "No module loaded addr=%lx\n",
-			       addr);
-			return -EFAULT;
-		}
-		rec->arch.mod = mod;
-	} else if (mod) {
-		printk(KERN_ERR
-		       "Record mod %p not equal to passed in mod %p\n",
-		       rec->arch.mod, mod);
-		return -EINVAL;
-	} else
-		mod = rec->arch.mod;
-
 	/* read where this goes */
 	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
 		return -EFAULT;
 
 	/* Make sure that that this is still a 24bit jump */
-	if ((*op & 0xff000000) != 0x48000000) {
+	if (!is_bl_op(*op)) {
 		printk(KERN_ERR "Not expected bl: opcode is %x\n", *op);
 		return -EINVAL;
 	}
 
 	/* lets find where the pointer goes */
-	offset = (*op & 0x03fffffc);
-	/* make it signed */
-	if (offset & 0x02000000)
-		offset |= 0xfe000000;
-
-	tramp = ip + (long)offset;
+	tramp = find_bl_target(ip, *op);
 
 	/*
 	 * On PPC64 the trampoline looks like:
@@ -257,43 +263,17 @@ __ftrace_make_nop(struct module *mod,
 	unsigned long tramp;
 	int offset;
 
-	/*
-	 * Out of range jumps are called from modules.
-	 * We should either already have a pointer to the module
-	 * or it has been passed in.
-	 */
-	if (!rec->arch.mod) {
-		if (!mod) {
-			printk(KERN_ERR "No module loaded addr=%lx\n",
-			       addr);
-			return -EFAULT;
-		}
-		rec->arch.mod = mod;
-	} else if (mod) {
-		printk(KERN_ERR
-		       "Record mod %p not equal to passed in mod %p\n",
-		       rec->arch.mod, mod);
-		return -EINVAL;
-	} else
-		mod = rec->arch.mod;
-
-	/* read where this goes */
 	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
 		return -EFAULT;
 
 	/* Make sure that that this is still a 24bit jump */
-	if ((*op & 0xff000000) != 0x48000000) {
+	if (!is_bl_op(*op)) {
 		printk(KERN_ERR "Not expected bl: opcode is %x\n", *op);
 		return -EINVAL;
 	}
 
 	/* lets find where the pointer goes */
-	offset = (*op & 0x03fffffc);
-	/* make it signed */
-	if (offset & 0x02000000)
-		offset |= 0xfe000000;
-
-	tramp = ip + (long)offset;
+	tramp = find_bl_target(ip, *op);
 
 	/*
 	 * On PPC32 the trampoline looks like:
@@ -354,6 +334,26 @@ int ftrace_make_nop(struct module *mod,
 		return ftrace_modify_code(ip, old, new);
 	}
 
+	/*
+	 * Out of range jumps are called from modules.
+	 * We should either already have a pointer to the module
+	 * or it has been passed in.
+	 */
+	if (!rec->arch.mod) {
+		if (!mod) {
+			printk(KERN_ERR "No module loaded addr=%lx\n",
+			       addr);
+			return -EFAULT;
+		}
+		rec->arch.mod = mod;
+	} else if (mod) {
+		printk(KERN_ERR
+		       "Record mod %p not equal to passed in mod %p\n",
+		       rec->arch.mod, mod);
+		return -EINVAL;
+	} else
+		mod = rec->arch.mod;
+
 	return __ftrace_make_nop(mod, rec, addr);
 
 }
@@ -367,16 +367,6 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned long ip = rec->ip;
 	unsigned long offset;
 
-	/*
-	 * Out of range jumps are called from modules.
-	 * Being that we are converting from nop, it had better
-	 * already have a module defined.
-	 */
-	if (!rec->arch.mod) {
-		printk(KERN_ERR "No module loaded\n");
-		return -EINVAL;
-	}
-
 	/* read where this goes */
 	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE * 2))
 		return -EFAULT;
@@ -397,15 +387,14 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	/* now calculate a jump to the ftrace caller trampoline */
 	offset = rec->arch.mod->arch.tramp - ip;
 
-	if (offset + 0x2000000 > 0x3ffffff || (offset & 3) != 0) {
+	if (test_offset(offset)) {
 		printk(KERN_ERR "REL24 %li out of range!\n",
 		       (long int)offset);
 		return -EINVAL;
 	}
 
-
 	/* Set to "bl addr" */
-	op[0] = 0x48000001 | (offset & 0x03fffffc);
+	op[0] = branch_offset(offset);
 	/* ld r2,40(r1) */
 	op[1] = 0xe8410028;
 
@@ -425,16 +414,6 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned long ip = rec->ip;
 	unsigned long offset;
 
-	/*
-	 * Out of range jumps are called from modules.
-	 * Being that we are converting from nop, it had better
-	 * already have a module defined.
-	 */
-	if (!rec->arch.mod) {
-		printk(KERN_ERR "No module loaded\n");
-		return -EINVAL;
-	}
-
 	/* read where this goes */
 	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
 		return -EFAULT;
@@ -454,15 +433,14 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	/* now calculate a jump to the ftrace caller trampoline */
 	offset = rec->arch.mod->arch.tramp - ip;
 
-	if (offset + 0x2000000 > 0x3ffffff || (offset & 3) != 0) {
+	if (test_offset(offset)) {
 		printk(KERN_ERR "REL24 %li out of range!\n",
 		       (long int)offset);
 		return -EINVAL;
 	}
 
-
 	/* Set to "bl addr" */
-	op[0] = 0x48000001 | (offset & 0x03fffffc);
+	op[0] = branch_offset(offset);
 
 	DEBUGP("write to %lx\n", rec->ip);
 
@@ -490,6 +468,16 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		return ftrace_modify_code(ip, old, new);
 	}
 
+	/*
+	 * Out of range jumps are called from modules.
+	 * Being that we are converting from nop, it had better
+	 * already have a module defined.
+	 */
+	if (!rec->arch.mod) {
+		printk(KERN_ERR "No module loaded\n");
+		return -EINVAL;
+	}
+
 	return __ftrace_make_call(rec, addr);
 
 }
-- 
1.5.6.5

-- 

[PATCH 2/4] ftrace,ppc: fix test of 24bit jump

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-ppc-fix-test-of-24bit-jump.patch --]
[-- Type: text/plain, Size: 1385 bytes --]

Impact: fix of test if an address is 26 bits away or not

Paul Mackerras pointed out that the test of the 24bit offset jump was
incorrect. For one thing, it is a 26 bit distance since we multiply
by 4 to account for the alignment. Another is that it may produce an
incorrect result on a negative jump of exactly the offset size.

Reported-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/kernel/ftrace.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 9360fd1..918a5d2 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -114,12 +114,19 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
  */
 static int test_24bit_addr(unsigned long ip, unsigned long addr)
 {
-	unsigned long diff;
+	long diff;
 
-	/* can we get to addr from ip in 24 bits? */
-	diff = ip > addr ? ip - addr : addr - ip;
+	/*
+	 * Can we get to addr from ip in 24 bits?
+	 *  (26 really, since we mulitply by 4 for 4 byte alignment)
+	 */
+	diff = addr - ip;
 
-	return !(diff & ((unsigned long)-1 << 24));
+	/*
+	 * Return true if diff is less than 1 << 25
+	 *  and greater than -1 << 26.
+	 */
+	return (diff < (1 << 25)) && (diff > (-1 << 26));
 }
 
 static int is_bl_op(unsigned int op)
-- 
1.5.6.5

-- 

[PATCH 3/4] ftrace,ppc: fix module check in dynamic ftrace code

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-ppc-fix-module-check-in-dynamic-ftrace-code.patch --]
[-- Type: text/plain, Size: 1583 bytes --]

Impact: fix to module check in making nops in dynamic ftrace for PPC

Paul Mackerras pointed out that the mod test in ftrace_make_nop looks
wrong. It is, but it did not trigger because the logic to ftrace just
happens to miss the bad case.

When ftrace first changes all the calls to mcount into nops, it passes
in the "mod" parameter, and the rec->arch.mod is NULL. This case assigns
the rec->arch.mod to mod and succeeds. From then on, ftrace will pass
in a NULL for mod and the check if mod and rec->arch.mod is the same
is skipped.

The failure of the current test may produce a false positive failuer,
which will cause ftrace to shutdown even if the code was correct. This
patch fixes the test.

Reported-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/kernel/ftrace.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 918a5d2..81e338c 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -354,10 +354,13 @@ int ftrace_make_nop(struct module *mod,
 		}
 		rec->arch.mod = mod;
 	} else if (mod) {
-		printk(KERN_ERR
-		       "Record mod %p not equal to passed in mod %p\n",
-		       rec->arch.mod, mod);
-		return -EINVAL;
+		if (mod != rec->arch.mod) {
+			printk(KERN_ERR
+			       "Record mod %p not equal to passed in mod %p\n",
+			       rec->arch.mod, mod);
+			return -EINVAL;
+		}
+		/* nothing to do if mod == rec->arch.mod */
 	} else
 		mod = rec->arch.mod;
 
-- 
1.5.6.5

-- 

[PATCH 4/4] ftrace,ppc: fix test of branch link

[Steven Rostedt]

[-- Attachment #1: 0004-ftrace-ppc-fix-test-of-branch-link.patch --]
[-- Type: text/plain, Size: 863 bytes --]

Impact: fix positive failure of branch link on PPC

Paul Mackerras pointed out that the test to verify that the modify
code is a banch link is incorrect. It was testing the wrong bits.
This patch corrects the test.

Reported-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/kernel/ftrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 81e338c..cc901b1 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -131,7 +131,7 @@ static int test_24bit_addr(unsigned long ip, unsigned long addr)
 
 static int is_bl_op(unsigned int op)
 {
-	return (op & 0xff000000) == 0x48000000;
+	return (op & 0xfc000003) == 0x48000001;
 }
 
 static int test_offset(unsigned long offset)
-- 
1.5.6.5

--