[PATCH 0/5] powerpc: dynamic ftrace port

[PATCH 0/5] powerpc: dynamic ftrace port

[Steven Rostedt]

Paul and Ingo,

The following are the changes to get dynamic ftrace working on PowerPC.
I modified these a little from the last postings.

 1) I removed the changes to recordmcount.pl.  This is not a PowerPC
    change, although PowerPC needs it for dynamic ftrace. But
    the changes here can go via linux-tip.

 2) I removed the adding of HAVE_DYNAMIC_FTRACE and HAVE_FTRACE_MCOUNT_RECORD.
    Without these config options, this code will not be compiled.
    I have a separate patch that adds them in when the time is right.

Paul, these patches should not harm anything if you decide to pull them
in. As I stated above, without the above config options, they are
not enabled.

There is one exception and that is for the first patch. The first patch
is actually a fix for the irqsoff latency tracer which is already
in mainline for PowerPC.

These patches only touch PowerPC code.

The following patches are in:

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

    branch: ppc/ftrace


Steven Rostedt (5):
      powerpc: ftrace, do not latency trace idle
      powerpc: ftrace, convert to new dynamic ftrace arch API
      powerpc: ftrace, use probe_kernel API to modify code
      powerpc/ppc64: ftrace, handle module trampolines for dyn ftrace
      powerpc/ppc32: ftrace, dynamic ftrace to handle modules

----
 arch/powerpc/include/asm/ftrace.h |   14 +-
 arch/powerpc/include/asm/module.h |   16 ++-
 arch/powerpc/kernel/ftrace.c      |  473 +++++++++++++++++++++++++++++++++---
 arch/powerpc/kernel/idle.c        |    5 +
 arch/powerpc/kernel/module_32.c   |   10 +
 arch/powerpc/kernel/module_64.c   |   13 +
 6 files changed, 489 insertions(+), 42 deletions(-)

[PATCH 1/5] powerpc: ftrace, do not latency trace idle

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

Impact: fix for irq off latency tracer

When idle is called, interrupts are disabled, but the idle function
will still wake up on an interrupt. The problem is that the interrupt
disabled latency tracer will take this call to idle as a latency.

This patch disables the latency tracing when going into idle.

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

diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
index 31982d0..88d9c1d 100644
--- a/arch/powerpc/kernel/idle.c
+++ b/arch/powerpc/kernel/idle.c
@@ -69,10 +69,15 @@ void cpu_idle(void)
 				smp_mb();
 				local_irq_disable();
 
+				/* Don't trace irqs off for idle */
+				stop_critical_timings();
+
 				/* check again after disabling irqs */
 				if (!need_resched() && !cpu_should_die())
 					ppc_md.power_save();
 
+				start_critical_timings();
+
 				local_irq_enable();
 				set_thread_flag(TIF_POLLING_NRFLAG);
 
-- 
1.5.6.5

-- 

[PATCH 2/5] powerpc: ftrace, convert to new dynamic ftrace arch API

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

Impact: update to PowerPC ftrace arch API

This patch converts PowerPC to use the new dynamic ftrace arch API.

Thanks to Paul Mackennas for pointing out the mistakes of my original
test_24bit_addr function.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/include/asm/ftrace.h |   14 +++++++-
 arch/powerpc/kernel/ftrace.c      |   67 ++++++++++++++++++++++++++++++++++---
 2 files changed, 75 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index b298f7a..17efecc 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -7,7 +7,19 @@
 
 #ifndef __ASSEMBLY__
 extern void _mcount(void);
-#endif
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+static inline unsigned long ftrace_call_adjust(unsigned long addr)
+{
+       /* reloction of mcount call site is the same as the address */
+       return addr;
+}
+
+struct dyn_arch_ftrace {
+	/* nothing yet */
+};
+#endif /*  CONFIG_DYNAMIC_FTRACE */
+#endif /* __ASSEMBLY__ */
 
 #endif
 
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index f4b006e..24c023a 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -33,12 +33,12 @@ static unsigned int ftrace_calc_offset(long ip, long addr)
 	return (int)(addr - ip);
 }
 
-unsigned char *ftrace_nop_replace(void)
+static unsigned char *ftrace_nop_replace(void)
 {
 	return (char *)&ftrace_nop;
 }
 
-unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
+static unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
 {
 	static unsigned int op;
 
@@ -68,7 +68,7 @@ unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
 # define _ASM_PTR	" .long "
 #endif
 
-int
+static int
 ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 		   unsigned char *new_code)
 {
@@ -113,6 +113,62 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 	return faulted;
 }
 
+static int test_24bit_addr(unsigned long ip, unsigned long addr)
+{
+	long diff;
+
+	/*
+	 * 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 true if diff is less than 1 << 25
+	 *  and greater than -1 << 26.
+	 */
+	return (diff < (1 << 25)) && (diff > (-1 << 26));
+}
+
+int ftrace_make_nop(struct module *mod,
+		    struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned char *old, *new;
+
+	/*
+	 * If the calling address is more that 24 bits away,
+	 * then we had to use a trampoline to make the call.
+	 * Otherwise just update the call site.
+	 */
+	if (test_24bit_addr(rec->ip, addr)) {
+		/* within range */
+		old = ftrace_call_replace(rec->ip, addr);
+		new = ftrace_nop_replace();
+		return ftrace_modify_code(rec->ip, old, new);
+	}
+
+	return 0;
+}
+
+int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned char *old, *new;
+
+	/*
+	 * If the calling address is more that 24 bits away,
+	 * then we had to use a trampoline to make the call.
+	 * Otherwise just update the call site.
+	 */
+	if (test_24bit_addr(rec->ip, addr)) {
+		/* within range */
+		old = ftrace_nop_replace();
+		new = ftrace_call_replace(rec->ip, addr);
+		return ftrace_modify_code(rec->ip, old, new);
+	}
+
+	return 0;
+}
+
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
 	unsigned long ip = (unsigned long)(&ftrace_call);
@@ -128,9 +184,10 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
-	/* This is running in kstop_machine */
+	/* caller expects data to be zero */
+	unsigned long *p = data;
 
-	ftrace_mcount_set(data);
+	*p = 0;
 
 	return 0;
 }
-- 
1.5.6.5

-- 

[PATCH 3/5] powerpc: ftrace, use probe_kernel API to modify code

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

Impact: use cleaner probe_kernel API over assembly

Using probe_kernel_read/write interface is a much cleaner approach
than the current assembly version.

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

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 24c023a..1adfbb2 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -9,6 +9,7 @@
 
 #include <linux/spinlock.h>
 #include <linux/hardirq.h>
+#include <linux/uaccess.h>
 #include <linux/ftrace.h>
 #include <linux/percpu.h>
 #include <linux/init.h>
@@ -72,45 +73,33 @@ static int
 ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 		   unsigned char *new_code)
 {
-	unsigned replaced;
-	unsigned old = *(unsigned *)old_code;
-	unsigned new = *(unsigned *)new_code;
-	int faulted = 0;
+	unsigned char replaced[MCOUNT_INSN_SIZE];
 
 	/*
 	 * Note: Due to modules and __init, code can
 	 *  disappear and change, we need to protect against faulting
-	 *  as well as code changing.
+	 *  as well as code changing. We do this by using the
+	 *  probe_kernel_* functions.
 	 *
 	 * No real locking needed, this code is run through
-	 * kstop_machine.
+	 * kstop_machine, or before SMP starts.
 	 */
-	asm volatile (
-		"1: lwz		%1, 0(%2)\n"
-		"   cmpw	%1, %5\n"
-		"   bne		2f\n"
-		"   stwu	%3, 0(%2)\n"
-		"2:\n"
-		".section .fixup, \"ax\"\n"
-		"3:	li %0, 1\n"
-		"	b 2b\n"
-		".previous\n"
-		".section __ex_table,\"a\"\n"
-		_ASM_ALIGN "\n"
-		_ASM_PTR "1b, 3b\n"
-		".previous"
-		: "=r"(faulted), "=r"(replaced)
-		: "r"(ip), "r"(new),
-		  "0"(faulted), "r"(old)
-		: "memory");
-
-	if (replaced != old && replaced != new)
-		faulted = 2;
-
-	if (!faulted)
-		flush_icache_range(ip, ip + 8);
-
-	return faulted;
+
+	/* read the text we want to modify */
+	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)
+		return -EINVAL;
+
+	/* replace the text with the new text */
+	if (probe_kernel_write((void *)ip, new_code, MCOUNT_INSN_SIZE))
+		return -EPERM;
+
+	flush_icache_range(ip, ip + 8);
+
+	return 0;
 }
 
 static int test_24bit_addr(unsigned long ip, unsigned long addr)
-- 
1.5.6.5

-- 

[PATCH 4/5] powerpc/ppc64: ftrace, handle module trampolines for dyn ftrace

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

Impact: Allow 64 bit PowerPC to trace modules with dynamic ftrace

This adds code to handle the PPC64 module trampolines, and allows for
PPC64 to use dynamic ftrace.

Thanks to Paul Mackerras for these updates:

  - fix the mod and rec->arch.mod NULL checks.
  - fix to is_bl_op compare.

Thanks to Milton Miller for:

  - finding the nasty race with using two nops, and recommending
    instead that I use a branch 8 forward.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/include/asm/ftrace.h |    2 +-
 arch/powerpc/include/asm/module.h |   11 ++
 arch/powerpc/kernel/ftrace.c      |  278 +++++++++++++++++++++++++++++++++++--
 arch/powerpc/kernel/module_64.c   |   13 ++
 4 files changed, 293 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index 17efecc..e5f2ae8 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -16,7 +16,7 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 }
 
 struct dyn_arch_ftrace {
-	/* nothing yet */
+	struct module *mod;
 };
 #endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index e5f14b1..340bc69 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -34,6 +34,11 @@ struct mod_arch_specific {
 #ifdef __powerpc64__
 	unsigned int stubs_section;	/* Index of stubs section in module */
 	unsigned int toc_section;	/* What section is the TOC? */
+#ifdef CONFIG_DYNAMIC_FTRACE
+	unsigned long toc;
+	unsigned long tramp;
+#endif
+
 #else
 	/* Indices of PLT sections within module. */
 	unsigned int core_plt_section;
@@ -68,6 +73,12 @@ struct mod_arch_specific {
 #    endif	/* MODULE */
 #endif
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+#    ifdef MODULE
+	asm(".section .ftrace.tramp,\"ax\",@nobits; .align 3; .previous");
+#    endif	/* MODULE */
+#endif
+
 
 struct exception_table_entry;
 void sort_ex_table(struct exception_table_entry *start,
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 1adfbb2..1aec559 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -10,22 +10,29 @@
 #include <linux/spinlock.h>
 #include <linux/hardirq.h>
 #include <linux/uaccess.h>
+#include <linux/module.h>
 #include <linux/ftrace.h>
 #include <linux/percpu.h>
 #include <linux/init.h>
 #include <linux/list.h>
 
 #include <asm/cacheflush.h>
+#include <asm/code-patching.h>
 #include <asm/ftrace.h>
 
+#if 0
+#define DEBUGP printk
+#else
+#define DEBUGP(fmt , ...)	do { } while (0)
+#endif
 
-static unsigned int ftrace_nop = 0x60000000;
+static unsigned int ftrace_nop = PPC_NOP_INSTR;
 
 #ifdef CONFIG_PPC32
 # define GET_ADDR(addr) addr
 #else
 /* PowerPC64's functions are data that points to the functions */
-# define GET_ADDR(addr) *(unsigned long *)addr
+# define GET_ADDR(addr) (*(unsigned long *)addr)
 #endif
 
 
@@ -102,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)
 {
 	long diff;
@@ -119,43 +129,292 @@ static int test_24bit_addr(unsigned long ip, unsigned long addr)
 	return (diff < (1 << 25)) && (diff > (-1 << 26));
 }
 
+static int is_bl_op(unsigned int op)
+{
+	return (op & 0xfc000003) == 0x48000001;
+}
+
+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,
+		  struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned char replaced[MCOUNT_INSN_SIZE * 2];
+	unsigned int *op = (unsigned *)&replaced;
+	unsigned char jmp[8];
+	unsigned long *ptr = (unsigned long *)&jmp;
+	unsigned long ip = rec->ip;
+	unsigned long tramp;
+	int offset;
+
+	/* 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 (!is_bl_op(*op)) {
+		printk(KERN_ERR "Not expected bl: opcode is %x\n", *op);
+		return -EINVAL;
+	}
+
+	/* lets find where the pointer goes */
+	tramp = find_bl_target(ip, *op);
+
+	/*
+	 * On PPC64 the trampoline looks like:
+	 * 0x3d, 0x82, 0x00, 0x00,    addis   r12,r2, <high>
+	 * 0x39, 0x8c, 0x00, 0x00,    addi    r12,r12, <low>
+	 *   Where the bytes 2,3,6 and 7 make up the 32bit offset
+	 *   to the TOC that holds the pointer.
+	 *   to jump to.
+	 * 0xf8, 0x41, 0x00, 0x28,    std     r2,40(r1)
+	 * 0xe9, 0x6c, 0x00, 0x20,    ld      r11,32(r12)
+	 *   The actually address is 32 bytes from the offset
+	 *   into the TOC.
+	 * 0xe8, 0x4c, 0x00, 0x28,    ld      r2,40(r12)
+	 */
+
+	DEBUGP("ip:%lx jumps to %lx r2: %lx", ip, tramp, mod->arch.toc);
+
+	/* Find where the trampoline jumps to */
+	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
+		printk(KERN_ERR "Failed to read %lx\n", tramp);
+		return -EFAULT;
+	}
+
+	DEBUGP(" %08x %08x",
+	       (unsigned)(*ptr >> 32),
+	       (unsigned)*ptr);
+
+	offset = (unsigned)jmp[2] << 24 |
+		(unsigned)jmp[3] << 16 |
+		(unsigned)jmp[6] << 8 |
+		(unsigned)jmp[7];
+
+	DEBUGP(" %x ", offset);
+
+	/* get the address this jumps too */
+	tramp = mod->arch.toc + offset + 32;
+	DEBUGP("toc: %lx", tramp);
+
+	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
+		printk(KERN_ERR "Failed to read %lx\n", tramp);
+		return -EFAULT;
+	}
+
+	DEBUGP(" %08x %08x\n",
+	       (unsigned)(*ptr >> 32),
+	       (unsigned)*ptr);
+
+	/* This should match what was called */
+	if (*ptr != GET_ADDR(addr)) {
+		printk(KERN_ERR "addr does not match %lx\n", *ptr);
+		return -EINVAL;
+	}
+
+	/*
+	 * We want to nop the line, but the next line is
+	 *  0xe8, 0x41, 0x00, 0x28   ld r2,40(r1)
+	 * This needs to be turned to a nop too.
+	 */
+	if (probe_kernel_read(replaced, (void *)(ip+4), MCOUNT_INSN_SIZE))
+		return -EFAULT;
+
+	if (*op != 0xe8410028) {
+		printk(KERN_ERR "Next line is not ld! (%08x)\n", *op);
+		return -EINVAL;
+	}
+
+	/*
+	 * Milton Miller pointed out that we can not blindly do nops.
+	 * If a task was preempted when calling a trace function,
+	 * the nops will remove the way to restore the TOC in r2
+	 * and the r2 TOC will get corrupted.
+	 */
+
+	/*
+	 * Replace:
+	 *   bl <tramp>  <==== will be replaced with "b 1f"
+	 *   ld r2,40(r1)
+	 *  1:
+	 */
+	op[0] = 0x48000008;	/* b +8 */
+
+	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE))
+		return -EPERM;
+
+	return 0;
+}
+
+#else /* !PPC64 */
+static int
+__ftrace_make_nop(struct module *mod,
+		  struct dyn_ftrace *rec, unsigned long addr)
+{
+	/* Ignore modules for PPC32 (for now) */
+	return 0;
+}
+#endif /* PPC64 */
+
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {
 	unsigned char *old, *new;
+	unsigned long ip = rec->ip;
 
 	/*
 	 * If the calling address is more that 24 bits away,
 	 * then we had to use a trampoline to make the call.
 	 * Otherwise just update the call site.
 	 */
-	if (test_24bit_addr(rec->ip, addr)) {
+	if (test_24bit_addr(ip, addr)) {
 		/* within range */
-		old = ftrace_call_replace(rec->ip, addr);
+		old = ftrace_call_replace(ip, addr);
 		new = ftrace_nop_replace();
-		return ftrace_modify_code(rec->ip, old, new);
+		return ftrace_modify_code(ip, old, new);
+	}
+
+#ifdef CONFIG_PPC64
+	/*
+	 * 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) {
+		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;
+#endif /* CONFIG_PPC64 */
+
+	return __ftrace_make_nop(mod, rec, addr);
+
+}
+
+#ifdef CONFIG_PPC64
+static int
+__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned char replaced[MCOUNT_INSN_SIZE * 2];
+	unsigned int *op = (unsigned *)&replaced;
+	unsigned long ip = rec->ip;
+	unsigned long offset;
+
+	/* read where this goes */
+	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE * 2))
+		return -EFAULT;
+
+	/*
+	 * It should be pointing to two nops or
+	 *  b +8; ld r2,40(r1)
+	 */
+	if (((op[0] != 0x48000008) || (op[1] != 0xe8410028)) &&
+	    ((op[0] != PPC_NOP_INSTR) || (op[1] != PPC_NOP_INSTR))) {
+		printk(KERN_ERR "Expected NOPs but have %x %x\n", op[0], op[1]);
+		return -EINVAL;
+	}
+
+	/* If we never set up a trampoline to ftrace_caller, then bail */
+	if (!rec->arch.mod->arch.tramp) {
+		printk(KERN_ERR "No ftrace trampoline\n");
+		return -EINVAL;
+	}
+
+	/* now calculate a jump to the ftrace caller trampoline */
+	offset = rec->arch.mod->arch.tramp - ip;
+
+	if (test_offset(offset)) {
+		printk(KERN_ERR "REL24 %li out of range!\n",
+		       (long int)offset);
+		return -EINVAL;
 	}
 
+	/* Set to "bl addr" */
+	op[0] = branch_offset(offset);
+	/* ld r2,40(r1) */
+	op[1] = 0xe8410028;
+
+	DEBUGP("write to %lx\n", rec->ip);
+
+	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE * 2))
+		return -EPERM;
+
 	return 0;
 }
+#else
+static int
+__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	/* PPC32 ignores modules for now */
+	return 0;
+}
+#endif /* CONFIG_PPC64 */
 
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
 	unsigned char *old, *new;
+	unsigned long ip = rec->ip;
 
 	/*
 	 * If the calling address is more that 24 bits away,
 	 * then we had to use a trampoline to make the call.
 	 * Otherwise just update the call site.
 	 */
-	if (test_24bit_addr(rec->ip, addr)) {
+	if (test_24bit_addr(ip, addr)) {
 		/* within range */
 		old = ftrace_nop_replace();
-		new = ftrace_call_replace(rec->ip, addr);
-		return ftrace_modify_code(rec->ip, old, new);
+		new = ftrace_call_replace(ip, addr);
+		return ftrace_modify_code(ip, old, new);
 	}
 
-	return 0;
+#ifdef CONFIG_PPC64
+	/*
+	 * 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;
+	}
+#endif
+
+	return __ftrace_make_call(rec, addr);
 }
 
 int ftrace_update_ftrace_func(ftrace_func_t func)
@@ -180,4 +439,3 @@ int __init ftrace_dyn_arch_init(void *data)
 
 	return 0;
 }
-
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 1af2377..8992b03 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -20,6 +20,7 @@
 #include <linux/moduleloader.h>
 #include <linux/err.h>
 #include <linux/vmalloc.h>
+#include <linux/ftrace.h>
 #include <linux/bug.h>
 #include <asm/module.h>
 #include <asm/firmware.h>
@@ -163,6 +164,11 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
 		}
 	}
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+	/* make the trampoline to the ftrace_caller */
+	relocs++;
+#endif
+
 	DEBUGP("Looks like a total of %lu stubs, max\n", relocs);
 	return relocs * sizeof(struct ppc64_stub_entry);
 }
@@ -441,5 +447,12 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 		}
 	}
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+	me->arch.toc = my_r2(sechdrs, me);
+	me->arch.tramp = stub_for_addr(sechdrs,
+				       (unsigned long)ftrace_caller,
+				       me);
+#endif
+
 	return 0;
 }
-- 
1.5.6.5

-- 

[PATCH 5/5] powerpc/ppc32: ftrace, dynamic ftrace to handle modules

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

Impact: add ability to trace modules on 32 bit PowerPC

This patch performs the necessary trampoline calls to handle
modules with dynamic ftrace on 32 bit PowerPC.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/include/asm/module.h |    5 ++-
 arch/powerpc/kernel/ftrace.c      |  101 ++++++++++++++++++++++++++++++++++--
 arch/powerpc/kernel/module_32.c   |   10 ++++
 3 files changed, 109 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 340bc69..0845488 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -39,11 +39,14 @@ struct mod_arch_specific {
 	unsigned long tramp;
 #endif
 
-#else
+#else /* powerpc64 */
 	/* Indices of PLT sections within module. */
 	unsigned int core_plt_section;
 	unsigned int init_plt_section;
+#ifdef CONFIG_DYNAMIC_FTRACE
+	unsigned long tramp;
 #endif
+#endif /* powerpc64 */
 
 	/* List of BUG addresses, source line numbers and filenames */
 	struct list_head bug_list;
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 1aec559..3271cd6 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -274,7 +274,63 @@ static int
 __ftrace_make_nop(struct module *mod,
 		  struct dyn_ftrace *rec, unsigned long addr)
 {
-	/* Ignore modules for PPC32 (for now) */
+	unsigned char replaced[MCOUNT_INSN_SIZE];
+	unsigned int *op = (unsigned *)&replaced;
+	unsigned char jmp[8];
+	unsigned int *ptr = (unsigned int *)&jmp;
+	unsigned long ip = rec->ip;
+	unsigned long tramp;
+	int offset;
+
+	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
+		return -EFAULT;
+
+	/* Make sure that that this is still a 24bit jump */
+	if (!is_bl_op(*op)) {
+		printk(KERN_ERR "Not expected bl: opcode is %x\n", *op);
+		return -EINVAL;
+	}
+
+	/* lets find where the pointer goes */
+	tramp = find_bl_target(ip, *op);
+
+	/*
+	 * On PPC32 the trampoline looks like:
+	 * lis r11,sym@ha
+	 * addi r11,r11,sym@l
+	 * mtctr r11
+	 * bctr
+	 */
+
+	DEBUGP("ip:%lx jumps to %lx", ip, tramp);
+
+	/* Find where the trampoline jumps to */
+	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
+		printk(KERN_ERR "Failed to read %lx\n", tramp);
+		return -EFAULT;
+	}
+
+	DEBUGP(" %08x %08x ", ptr[0], ptr[1]);
+
+	tramp = (ptr[1] & 0xffff) |
+		((ptr[0] & 0xffff) << 16);
+	if (tramp & 0x8000)
+		tramp -= 0x10000;
+
+	DEBUGP(" %x ", tramp);
+
+	if (tramp != addr) {
+		printk(KERN_ERR
+		       "Trampoline location %08lx does not match addr\n",
+		       tramp);
+		return -EINVAL;
+	}
+
+	op[0] = PPC_NOP_INSTR;
+
+	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE))
+		return -EPERM;
+
 	return 0;
 }
 #endif /* PPC64 */
@@ -297,7 +353,6 @@ int ftrace_make_nop(struct module *mod,
 		return ftrace_modify_code(ip, old, new);
 	}
 
-#ifdef CONFIG_PPC64
 	/*
 	 * Out of range jumps are called from modules.
 	 * We should either already have a pointer to the module
@@ -320,7 +375,6 @@ int ftrace_make_nop(struct module *mod,
 		/* nothing to do if mod == rec->arch.mod */
 	} else
 		mod = rec->arch.mod;
-#endif /* CONFIG_PPC64 */
 
 	return __ftrace_make_nop(mod, rec, addr);
 
@@ -380,7 +434,44 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 static int
 __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
-	/* PPC32 ignores modules for now */
+	unsigned char replaced[MCOUNT_INSN_SIZE];
+	unsigned int *op = (unsigned *)&replaced;
+	unsigned long ip = rec->ip;
+	unsigned long offset;
+
+	/* read where this goes */
+	if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
+		return -EFAULT;
+
+	/* It should be pointing to a nop */
+	if (op[0] != PPC_NOP_INSTR) {
+		printk(KERN_ERR "Expected NOP but have %x\n", op[0]);
+		return -EINVAL;
+	}
+
+	/* If we never set up a trampoline to ftrace_caller, then bail */
+	if (!rec->arch.mod->arch.tramp) {
+		printk(KERN_ERR "No ftrace trampoline\n");
+		return -EINVAL;
+	}
+
+	/* now calculate a jump to the ftrace caller trampoline */
+	offset = rec->arch.mod->arch.tramp - ip;
+
+	if (test_offset(offset)) {
+		printk(KERN_ERR "REL24 %li out of range!\n",
+		       (long int)offset);
+		return -EINVAL;
+	}
+
+	/* Set to "bl addr" */
+	op[0] = branch_offset(offset);
+
+	DEBUGP("write to %lx\n", rec->ip);
+
+	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE))
+		return -EPERM;
+
 	return 0;
 }
 #endif /* CONFIG_PPC64 */
@@ -402,7 +493,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		return ftrace_modify_code(ip, old, new);
 	}
 
-#ifdef CONFIG_PPC64
 	/*
 	 * Out of range jumps are called from modules.
 	 * Being that we are converting from nop, it had better
@@ -412,7 +502,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		printk(KERN_ERR "No module loaded\n");
 		return -EINVAL;
 	}
-#endif
 
 	return __ftrace_make_call(rec, addr);
 }
diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index 2df91a0..f832773 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -22,6 +22,7 @@
 #include <linux/fs.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
+#include <linux/ftrace.h>
 #include <linux/cache.h>
 #include <linux/bug.h>
 #include <linux/sort.h>
@@ -53,6 +54,9 @@ static unsigned int count_relocs(const Elf32_Rela *rela, unsigned int num)
 			r_addend = rela[i].r_addend;
 		}
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+	_count_relocs++;	/* add one for ftrace_caller */
+#endif
 	return _count_relocs;
 }
 
@@ -306,5 +310,11 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
 			return -ENOEXEC;
 		}
 	}
+#ifdef CONFIG_DYNAMIC_FTRACE
+	module->arch.tramp =
+		do_plt_call(module->module_core,
+			    (unsigned long)ftrace_caller,
+			    sechdrs, module);
+#endif
 	return 0;
 }
-- 
1.5.6.5

-- 

Re: [PATCH 0/5] powerpc: dynamic ftrace port

[Steven Rostedt]

On Thu, 20 Nov 2008, Steven Rostedt wrote:
> 
>  1) I removed the changes to recordmcount.pl.  This is not a PowerPC
>     change, although PowerPC needs it for dynamic ftrace. But
>     the changes here can go via linux-tip.

Ingo,

I found that the recordmcount.pl change conflicts with a commit
in linux-next, which is not in tip nor mainline. The commit is:

   11ebca4a3f8337de2f4d1300bd307851d1191d53
   sh: dynamic ftrace support.

Seems that this change came in through the sh repo. I have already
resolved the conflicts with my updates for PowerPC with this commit.
I can put this in a separate branch against linux-next if you like.

I would like to get the changes to recordmcount.pl for PowerPC
in linux-next soon, so I can also add a small clean up of this code.
Jim Radfort has some updates as well for his port to arm.

-- Steve

Re: [PATCH 0/5] powerpc: dynamic ftrace port

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Paul and Ingo,
> 
> The following are the changes to get dynamic ftrace working on PowerPC.
> I modified these a little from the last postings.
> 
>  1) I removed the changes to recordmcount.pl.  This is not a PowerPC
>     change, although PowerPC needs it for dynamic ftrace. But
>     the changes here can go via linux-tip.
> 
>  2) I removed the adding of HAVE_DYNAMIC_FTRACE and HAVE_FTRACE_MCOUNT_RECORD.
>     Without these config options, this code will not be compiled.
>     I have a separate patch that adds them in when the time is right.
> 
> Paul, these patches should not harm anything if you decide to pull them
> in. As I stated above, without the above config options, they are
> not enabled.
> 
> There is one exception and that is for the first patch. The first patch
> is actually a fix for the irqsoff latency tracer which is already
> in mainline for PowerPC.
> 
> These patches only touch PowerPC code.
> 
> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: ppc/ftrace
> 
> 
> Steven Rostedt (5):
>       powerpc: ftrace, do not latency trace idle
>       powerpc: ftrace, convert to new dynamic ftrace arch API
>       powerpc: ftrace, use probe_kernel API to modify code
>       powerpc/ppc64: ftrace, handle module trampolines for dyn ftrace
>       powerpc/ppc32: ftrace, dynamic ftrace to handle modules
> 
> ----
>  arch/powerpc/include/asm/ftrace.h |   14 +-
>  arch/powerpc/include/asm/module.h |   16 ++-
>  arch/powerpc/kernel/ftrace.c      |  473 +++++++++++++++++++++++++++++++++---
>  arch/powerpc/kernel/idle.c        |    5 +
>  arch/powerpc/kernel/module_32.c   |   10 +
>  arch/powerpc/kernel/module_64.c   |   13 +
>  6 files changed, 489 insertions(+), 42 deletions(-)

looks perfect to me! Thanks Steve for going through this - this was 
really a logistical worst-case-scenario.

Now lets hope it looks good to Paul too and we can get his Acked-by
:-)

	Ingo

Re: [PATCH 0/5] powerpc: dynamic ftrace port

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 20 Nov 2008, Steven Rostedt wrote:
> > 
> >  1) I removed the changes to recordmcount.pl.  This is not a PowerPC
> >     change, although PowerPC needs it for dynamic ftrace. But
> >     the changes here can go via linux-tip.
> 
> Ingo,
> 
> I found that the recordmcount.pl change conflicts with a commit in 
> linux-next, which is not in tip nor mainline. The commit is:
> 
>    11ebca4a3f8337de2f4d1300bd307851d1191d53
>    sh: dynamic ftrace support.
> 
> Seems that this change came in through the sh repo. I have already 
> resolved the conflicts with my updates for PowerPC with this commit. 
> I can put this in a separate branch against linux-next if you like.
> 
> I would like to get the changes to recordmcount.pl for PowerPC in 
> linux-next soon, so I can also add a small clean up of this code. 
> Jim Radfort has some updates as well for his port to arm.

Could you please extract the sh bits from 11ebca4a so that we can 
apply it to tip/tracing/* standalone? It makes sense to carry it 
without having the other bits of the SH changes.

It will collide in linux-next but it will be a same-content collision 
with an easy enough (perhaps even automatic) resolution.

	Ingo

Re: [PATCH 0/5] powerpc: dynamic ftrace port

[Steven Rostedt]

On Thu, 20 Nov 2008, Ingo Molnar wrote:
> > 
> > There is one exception and that is for the first patch. The first patch
> > is actually a fix for the irqsoff latency tracer which is already
> > in mainline for PowerPC.
> > 

> > Steven Rostedt (5):
> >       powerpc: ftrace, do not latency trace idle

> 
> Now lets hope it looks good to Paul too and we can get his Acked-by
> :-)
> 

Hmm, that first patch is really a bug fix. I wonder if we should push to 
get that into 28?  It has a low impact, but it fixes a bug that is in 
current mainline.

-- Steve

Re: [PATCH 2/5] powerpc: ftrace, convert to new dynamic ftrace arch API

[Paul Mackerras]

Steven Rostedt writes:

> Thanks to Paul Mackennas for pointing out the mistakes of my original

Mackerras

> +static int test_24bit_addr(unsigned long ip, unsigned long addr)
> +{
> +	long diff;
> +
> +	/*
> +	 * 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 true if diff is less than 1 << 25
> +	 *  and greater than -1 << 26.
> +	 */
> +	return (diff < (1 << 25)) && (diff > (-1 << 26));

I think this still isn't right, and the comment is one of those ones
that is only useful to people who can't read C, as it's just a
transliteration of the code.

The comment should say something like "Return true if diff can be
represented as a 26-bit twos-complement binary number" and the second
part of the test should be (diff >= (-1 << 25)).  However, since you
define a test_offset() function in patch 4/5 that does the same test
but using only one comparison instead of two, why don't you just say:

	return !test_offset(diff);

(having first moved test_offset() before test_24bit_addr)?

Paul.

Re: [PATCH 4/5] powerpc/ppc64: ftrace, handle module trampolines for dyn ftrace

[Paul Mackerras]

Steven Rostedt writes:

> +#ifdef CONFIG_PPC64
> +static int
> +__ftrace_make_nop(struct module *mod,
> +		  struct dyn_ftrace *rec, unsigned long addr)
> +{
> +	unsigned char replaced[MCOUNT_INSN_SIZE * 2];
> +	unsigned int *op = (unsigned *)&replaced;

This makes me a little nervous, since it looks to me to be breaking
aliasing rules.  I know we use -fno-strict-aliasing, but still it
would be better to avoid doing these casts if possible - and we should
be able to avoid most of them by using unsigned int for instructions
consistently, instead of a mix of unsigned int and unsigned char.

> +	DEBUGP("ip:%lx jumps to %lx r2: %lx", ip, tramp, mod->arch.toc);
> +
> +	/* Find where the trampoline jumps to */
> +	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
> +		printk(KERN_ERR "Failed to read %lx\n", tramp);
> +		return -EFAULT;
> +	}
> +
> +	DEBUGP(" %08x %08x",
> +	       (unsigned)(*ptr >> 32),
> +	       (unsigned)*ptr);
> +
> +	offset = (unsigned)jmp[2] << 24 |
> +		(unsigned)jmp[3] << 16 |
> +		(unsigned)jmp[6] << 8 |
> +		(unsigned)jmp[7];

We don't seem to be checking that these instructions look like the
start of a trampoline created by module_64.c, which makes me a little
nervous.

If the kernel text goes over 32MB, the linker will insert trampolines
automatically.  Those trampolines either look like a direct branch to
the target, or else they look like this:

	addis	r12,r2,xxxx
	ld	r11,yyyy(r12)
	mtctr	r11
	bctr

where xxxx/yyyy gives the offset from the kernel TOC to the procedure
descriptor for the target.

Now, a kernel with > 32MB of text probably won't work for other
reasons at the moment (like the linker putting trampolines before the
interrupt vectors), so in a sense it doesn't matter.  It also doesn't
matter since we only get here for calls in modules (something that
could stand to be mentioned in a comment at the top of the function).
Nevertheless, I think it would be worthwhile to check that the first
two instructions look like the addis and addi that we are expecting.

> +	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE))
> +		return -EPERM;
> +
> +	return 0;
> +}

We don't seem to do anything to ensure I-cache consistency.  I think
we probably need a flush_icache_range call here.  Similarly in
__ftrace_make_call.

Paul.

Re: [PATCH 5/5] powerpc/ppc32: ftrace, dynamic ftrace to handle modules

[Paul Mackerras]

Steven Rostedt writes:

> This patch performs the necessary trampoline calls to handle
> modules with dynamic ftrace on 32 bit PowerPC.

Looks OK except for the need to call flush_icache_range after
probe_kernel_write, like in the 64-bit case.

Paul.

Re: [PATCH 2/5] powerpc: ftrace, convert to new dynamic ftrace arch API

[Michael Ellerman]

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

On Mon, 2008-11-24 at 12:43 +1100, Paul Mackerras wrote:
> Steven Rostedt writes:
> 
> > Thanks to Paul Mackennas for pointing out the mistakes of my original
> 
> Mackerras
> 
> > +static int test_24bit_addr(unsigned long ip, unsigned long addr)
> > +{
> > +	long diff;
> > +
> > +	/*
> > +	 * 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 true if diff is less than 1 << 25
> > +	 *  and greater than -1 << 26.
> > +	 */
> > +	return (diff < (1 << 25)) && (diff > (-1 << 26));
> 
> I think this still isn't right, and the comment is one of those ones
> that is only useful to people who can't read C, as it's just a
> transliteration of the code.
> 
> The comment should say something like "Return true if diff can be
> represented as a 26-bit twos-complement binary number" and the second
> part of the test should be (diff >= (-1 << 25)).  However, since you
> define a test_offset() function in patch 4/5 that does the same test
> but using only one comparison instead of two, why don't you just say:
> 
> 	return !test_offset(diff);
> 
> (having first moved test_offset() before test_24bit_addr)?

Or better still, split out and use the code from create_branch() in
arch/powerpc/lib/code-patching.c .. which is hopefully correct :)

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 4/5] powerpc/ppc64: ftrace, handle module trampolines for dyn ftrace

[Steven Rostedt]

On Mon, 24 Nov 2008, Paul Mackerras wrote:

> Steven Rostedt writes:
> 
> > +#ifdef CONFIG_PPC64
> > +static int
> > +__ftrace_make_nop(struct module *mod,
> > +		  struct dyn_ftrace *rec, unsigned long addr)
> > +{
> > +	unsigned char replaced[MCOUNT_INSN_SIZE * 2];
> > +	unsigned int *op = (unsigned *)&replaced;
> 
> This makes me a little nervous, since it looks to me to be breaking
> aliasing rules.  I know we use -fno-strict-aliasing, but still it
> would be better to avoid doing these casts if possible - and we should
> be able to avoid most of them by using unsigned int for instructions
> consistently, instead of a mix of unsigned int and unsigned char.

OK, I'll update this. We did a cleanup of all archs to use a 
char[MCOUNT_INSN_SIZE] array, and I've just been keeping it.

> 
> > +	DEBUGP("ip:%lx jumps to %lx r2: %lx", ip, tramp, mod->arch.toc);
> > +
> > +	/* Find where the trampoline jumps to */
> > +	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
> > +		printk(KERN_ERR "Failed to read %lx\n", tramp);
> > +		return -EFAULT;
> > +	}
> > +
> > +	DEBUGP(" %08x %08x",
> > +	       (unsigned)(*ptr >> 32),
> > +	       (unsigned)*ptr);
> > +
> > +	offset = (unsigned)jmp[2] << 24 |
> > +		(unsigned)jmp[3] << 16 |
> > +		(unsigned)jmp[6] << 8 |
> > +		(unsigned)jmp[7];
> 
> We don't seem to be checking that these instructions look like the
> start of a trampoline created by module_64.c, which makes me a little
> nervous.

I'll add this check.

> 
> If the kernel text goes over 32MB, the linker will insert trampolines
> automatically.  Those trampolines either look like a direct branch to
> the target, or else they look like this:
> 
> 	addis	r12,r2,xxxx
> 	ld	r11,yyyy(r12)
> 	mtctr	r11
> 	bctr
> 
> where xxxx/yyyy gives the offset from the kernel TOC to the procedure
> descriptor for the target.
> 
> Now, a kernel with > 32MB of text probably won't work for other
> reasons at the moment (like the linker putting trampolines before the
> interrupt vectors), so in a sense it doesn't matter.  It also doesn't
> matter since we only get here for calls in modules (something that
> could stand to be mentioned in a comment at the top of the function).
> Nevertheless, I think it would be worthwhile to check that the first
> two instructions look like the addis and addi that we are expecting.

I'll add the "module only" comment. If the linker ever decided to add
more trampolines to the core kernel, then ftrace would detect that and
print a warning and disable itself.

> 
> > +	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> 
> We don't seem to do anything to ensure I-cache consistency.  I think
> we probably need a flush_icache_range call here.  Similarly in
> __ftrace_make_call.

Crap! you are right. I forgot to do that. Will fix.

Thanks,

-- Steve

Re: [PATCH 2/5] powerpc: ftrace, convert to new dynamic ftrace arch API

[Steven Rostedt]

On Mon, 24 Nov 2008, Paul Mackerras wrote:

> Steven Rostedt writes:
> 
> > Thanks to Paul Mackennas for pointing out the mistakes of my original
> 
> Mackerras

Heh, I have two reasons for that typo.

 1) I reference Paul McKenney a lot, and my fingers are programmed.
 2) I type with the dvorak layout, and the 'r' is just above the 'n'.
    qwerty 'o' == dvorak 'r'
    qwerty 'l' == dvorak 'n'

;-)

> 
> > +static int test_24bit_addr(unsigned long ip, unsigned long addr)
> > +{
> > +	long diff;
> > +
> > +	/*
> > +	 * 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 true if diff is less than 1 << 25
> > +	 *  and greater than -1 << 26.
> > +	 */
> > +	return (diff < (1 << 25)) && (diff > (-1 << 26));
> 
> I think this still isn't right, and the comment is one of those ones
> that is only useful to people who can't read C, as it's just a
> transliteration of the code.

Argh!!! That must be a misfold.  I fixed that code.  The final version 
ad the same shift for both.

> 
> The comment should say something like "Return true if diff can be
> represented as a 26-bit twos-complement binary number" and the second
> part of the test should be (diff >= (-1 << 25)).  However, since you
> define a test_offset() function in patch 4/5 that does the same test
> but using only one comparison instead of two, why don't you just say:
> 
> 	return !test_offset(diff);
> 
> (having first moved test_offset() before test_24bit_addr)?

OK, will fix. Thanks. Hmm, maybe I ment to do that, and miss the patch :-/

-- Steve

Re: [PATCH 4/5] powerpc/ppc64: ftrace, handle module trampolines for dyn ftrace

[Steven Rostedt]

Paul and Ingo,

Would it be best for me to just refold these changes into the original 
patch series, and update the git repo? Or should I apply these changes 
on top of this series?

Thanks,

-- Steve


On Mon, 24 Nov 2008, Paul Mackerras wrote:

> Steven Rostedt writes:
> 
> > +#ifdef CONFIG_PPC64
> > +static int
> > +__ftrace_make_nop(struct module *mod,
> > +		  struct dyn_ftrace *rec, unsigned long addr)
> > +{
> > +	unsigned char replaced[MCOUNT_INSN_SIZE * 2];
> > +	unsigned int *op = (unsigned *)&replaced;
> 
> This makes me a little nervous, since it looks to me to be breaking
> aliasing rules.  I know we use -fno-strict-aliasing, but still it
> would be better to avoid doing these casts if possible - and we should
> be able to avoid most of them by using unsigned int for instructions
> consistently, instead of a mix of unsigned int and unsigned char.
> 
> > +	DEBUGP("ip:%lx jumps to %lx r2: %lx", ip, tramp, mod->arch.toc);
> > +
> > +	/* Find where the trampoline jumps to */
> > +	if (probe_kernel_read(jmp, (void *)tramp, 8)) {
> > +		printk(KERN_ERR "Failed to read %lx\n", tramp);
> > +		return -EFAULT;
> > +	}
> > +
> > +	DEBUGP(" %08x %08x",
> > +	       (unsigned)(*ptr >> 32),
> > +	       (unsigned)*ptr);
> > +
> > +	offset = (unsigned)jmp[2] << 24 |
> > +		(unsigned)jmp[3] << 16 |
> > +		(unsigned)jmp[6] << 8 |
> > +		(unsigned)jmp[7];
> 
> We don't seem to be checking that these instructions look like the
> start of a trampoline created by module_64.c, which makes me a little
> nervous.
> 
> If the kernel text goes over 32MB, the linker will insert trampolines
> automatically.  Those trampolines either look like a direct branch to
> the target, or else they look like this:
> 
> 	addis	r12,r2,xxxx
> 	ld	r11,yyyy(r12)
> 	mtctr	r11
> 	bctr
> 
> where xxxx/yyyy gives the offset from the kernel TOC to the procedure
> descriptor for the target.
> 
> Now, a kernel with > 32MB of text probably won't work for other
> reasons at the moment (like the linker putting trampolines before the
> interrupt vectors), so in a sense it doesn't matter.  It also doesn't
> matter since we only get here for calls in modules (something that
> could stand to be mentioned in a comment at the top of the function).
> Nevertheless, I think it would be worthwhile to check that the first
> two instructions look like the addis and addi that we are expecting.
> 
> > +	if (probe_kernel_write((void *)ip, replaced, MCOUNT_INSN_SIZE))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> 
> We don't seem to do anything to ensure I-cache consistency.  I think
> we probably need a flush_icache_range call here.  Similarly in
> __ftrace_make_call.
> 
> Paul.
> 
>