powerpc: dynamic ftrace port

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

arch/powerpc/kernel/ftrace.c +5 -0

+static int test_24bit_addr(unsigned long ip, unsigned long addr)
+{
+	long diff;
+
+	/*

Quoted 1 line · Steven Rostedt

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

Mackerras

Quoted 5 lines

arch/powerpc/kernel/ftrace.c +5 -0

+static int test_24bit_addr(unsigned long ip, unsigned long addr)
+{
+	long diff;
+
+	/*

arch/powerpc/kernel/ftrace.c ftrace_modify_code(unsigned long ip, unsigned char *old_code, · line 130

Quoted 10 lines

arch/powerpc/kernel/ftrace.c +10 -0

+	 * 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)?

Quoted 1 line · Steven Rostedt

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

Mackerras

Quoted 12 lines

arch/powerpc/kernel/ftrace.c +11 -0

+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;
+
+	/*

arch/powerpc/kernel/ftrace.c ftrace_modify_code(unsigned long ip, unsigned char *old_code, · line 130

Quoted 4 lines

arch/powerpc/kernel/ftrace.c +4 -0

+	 * 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)?

arch/powerpc/kernel/ftrace.c +6 -0

+	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;

arch/powerpc/kernel/ftrace.c static int test_24bit_addr(unsigned long ip, unsigned long addr) · line 166

Quoted 7 lines · Steven Rostedt

arch/powerpc/kernel/ftrace.c +7 -0

+#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.

Quoted 13 lines

arch/powerpc/kernel/ftrace.c +12 -0

+	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);
+

arch/powerpc/kernel/ftrace.c static int test_24bit_addr(unsigned long ip, unsigned long addr) · line 215

Quoted 4 lines

arch/powerpc/kernel/ftrace.c +4 -0

+	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.

arch/powerpc/kernel/ftrace.c static int test_24bit_addr(unsigned long ip, unsigned long addr) · line 270

Quoted 6 lines

arch/powerpc/kernel/ftrace.c +5 -0

+	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.

arch/powerpc/kernel/ftrace.c static int test_24bit_addr(unsigned long ip, unsigned long addr) · line 166

Quoted 7 lines · Steven Rostedt

arch/powerpc/kernel/ftrace.c +7 -0

+#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.

Quoted 6 lines

arch/powerpc/kernel/ftrace.c +6 -0

+	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;

arch/powerpc/kernel/ftrace.c static int test_24bit_addr(unsigned long ip, unsigned long addr) · line 215

Quoted 10 lines

arch/powerpc/kernel/ftrace.c +10 -0

+	}
+
+	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.

arch/powerpc/kernel/ftrace.c static int test_24bit_addr(unsigned long ip, unsigned long addr) · line 270

Quoted 5 lines

arch/powerpc/kernel/ftrace.c +5 -0

+	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.

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

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.

Quoted 4 lines · Steven Rostedt

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.

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(-)

Quoted 5 lines

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.

Quoted 2 lines

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 :-)