[PATCH 0/3][RFC] powerpc/ftrace: Removal of stop machine (and other goodies)

[PATCH 0/3][RFC] powerpc/ftrace: Removal of stop machine (and other goodies)

[Steven Rostedt]

Benjamin,

You once told me on IRC that powerpc has no problem with modifying
code on one CPU that may be executing on another CPU. With the tests I
made on my PPC64 (2 CPUs) box, it seems to be the case.

The first patch removes stop_machine from powerpc. The other patches
add some error handling if ftrace detects an update didn't occur
with 'patch_instruction'.

This is just an RFC, but if it's fine, feel free to pull them into
your tree.

-- Steve

Steven Rostedt (3):
      ftrace/ppc: Have PPC skip updating with stop_machine()
      powerpc: Have patch_instruction detect faults
      ftrace/ppc: Use patch_instruction instead of probe_kernel_write()

----
 arch/powerpc/include/asm/code-patching.h |    4 +-
 arch/powerpc/kernel/ftrace.c             |   69 ++++++++++++++++++++++++------
 arch/powerpc/lib/code-patching.c         |   14 ++++--
 3 files changed, 68 insertions(+), 19 deletions(-)

[PATCH 1/3][RFC] ftrace/ppc: Have PPC skip updating with stop_machine()

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

PPC does not have the synchronization issues that x86 has with
modifying code on one CPU while another CPU is executing it.
The other CPU will either see the old or new code without any
issues, unlike x86 which may issue a GPF.

Instead of calling the heavy stop_machine, just update the code.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/powerpc/kernel/ftrace.c |   52 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index bf99cfa..84a5ddd 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -484,6 +484,58 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ret;
 }
 
+static int __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
+{
+	unsigned long ftrace_addr = (unsigned long)FTRACE_ADDR;
+	int ret;
+
+	ret = ftrace_update_record(rec, enable);
+
+	switch (ret) {
+	case FTRACE_UPDATE_IGNORE:
+		return 0;
+	case FTRACE_UPDATE_MAKE_CALL:
+		return ftrace_make_call(rec, ftrace_addr);
+	case FTRACE_UPDATE_MAKE_NOP:
+		return ftrace_make_nop(NULL, rec, ftrace_addr);
+	}
+
+	return 0;
+}
+
+static void ftrace_replace_code(int enable)
+{
+	struct ftrace_rec_iter *iter;
+	struct dyn_ftrace *rec;
+	int ret;
+
+	for (iter = ftrace_rec_iter_start(); iter;
+	     iter = ftrace_rec_iter_next(iter)) {
+		rec = ftrace_rec_iter_record(iter);
+		ret = __ftrace_replace_code(rec, enable);
+		if (ret) {
+			ftrace_bug(ret, rec->ip);
+			return;
+		}
+	}
+}
+
+void arch_ftrace_update_code(int command)
+{
+	if (command & FTRACE_UPDATE_CALLS)
+		ftrace_replace_code(1);
+	else if (command & FTRACE_DISABLE_CALLS)
+		ftrace_replace_code(0);
+
+	if (command & FTRACE_UPDATE_TRACE_FUNC)
+		ftrace_update_ftrace_func(ftrace_trace_function);
+
+	if (command & FTRACE_START_FUNC_RET)
+		ftrace_enable_ftrace_graph_caller();
+	else if (command & FTRACE_STOP_FUNC_RET)
+		ftrace_disable_ftrace_graph_caller();
+}
+
 int __init ftrace_dyn_arch_init(void *data)
 {
 	/* caller expects data to be zero */
-- 
1.7.9.5

Re: [PATCH 1/3][RFC] ftrace/ppc: Have PPC skip updating with stop_machine()

[Michael Ellerman]

On Thu, 2012-04-26 at 14:31 -0400, Steven Rostedt wrote:
> plain text document attachment
> (0001-ftrace-ppc-Have-PPC-skip-updating-with-stop_machine.patch)
> From: Steven Rostedt <srostedt@redhat.com>
> 
> PPC does not have the synchronization issues that x86 has with
> modifying code on one CPU while another CPU is executing it.
> The other CPU will either see the old or new code without any
> issues, unlike x86 which may issue a GPF.
> 
> Instead of calling the heavy stop_machine, just update the code.

This looks nice, but it's giving me this:

arch/powerpc/kernel/ftrace.c:497:13: error: static declaration of 'ftrace_replace_code' follows non-static declaration                                                
include/linux/ftrace.h:317:13: note: previous declaration of 'ftrace_replace_code' was here

I think the fix is just to make our ftrace_replace_code() non-static?

cheers


> +static void ftrace_replace_code(int enable)
> +{
> +	struct ftrace_rec_iter *iter;
> +	struct dyn_ftrace *rec;
> +	int ret;
> +
> +	for (iter = ftrace_rec_iter_start(); iter;
> +	     iter = ftrace_rec_iter_next(iter)) {
> +		rec = ftrace_rec_iter_record(iter);
> +		ret = __ftrace_replace_code(rec, enable);
> +		if (ret) {
> +			ftrace_bug(ret, rec->ip);
> +			return;
> +		}
> +	}
> +}
> +

[PATCH 2/3][RFC] powerpc: Have patch_instruction detect faults

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

For ftrace to use the patch_instruction code, it needs to check for
faults on write. Ftrace updates code all over the kernel, and we need to
know if code is updated or not due to protections that are placed on
some portions of the kernel. If ftrace does not detect a fault, it will
error later on, and it will be much more difficult to find the problem.

By changing patch_instruction() to detect faults, then ftrace will be
able to make use of it too.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/powerpc/include/asm/code-patching.h |    4 ++--
 arch/powerpc/lib/code-patching.c         |   14 ++++++++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 37c32aba..a6f8c7a 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -26,8 +26,8 @@ unsigned int create_branch(const unsigned int *addr,
 			   unsigned long target, int flags);
 unsigned int create_cond_branch(const unsigned int *addr,
 				unsigned long target, int flags);
-void patch_branch(unsigned int *addr, unsigned long target, int flags);
-void patch_instruction(unsigned int *addr, unsigned int instr);
+int patch_branch(unsigned int *addr, unsigned long target, int flags);
+int patch_instruction(unsigned int *addr, unsigned int instr);
 
 int instr_is_relative_branch(unsigned int instr);
 int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 7c975d4..dd223b3 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -13,17 +13,23 @@
 #include <linux/mm.h>
 #include <asm/page.h>
 #include <asm/code-patching.h>
+#include <asm/uaccess.h>
 
 
-void patch_instruction(unsigned int *addr, unsigned int instr)
+int patch_instruction(unsigned int *addr, unsigned int instr)
 {
-	*addr = instr;
+	int err;
+
+	err = __put_user(instr, addr);
+	if (err)
+		return err;
 	asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
+	return 0;
 }
 
-void patch_branch(unsigned int *addr, unsigned long target, int flags)
+int patch_branch(unsigned int *addr, unsigned long target, int flags)
 {
-	patch_instruction(addr, create_branch(addr, target, flags));
+	return patch_instruction(addr, create_branch(addr, target, flags));
 }
 
 unsigned int create_branch(const unsigned int *addr,
-- 
1.7.9.5

[PATCH 3/3][RFC] ftrace/ppc: Use patch_instruction instead of probe_kernel_write()

[Steven Rostedt]

From: Steven Rostedt <srostedt@redhat.com>

The patch_instruction() interface is made to modify kernel text. It is
safer to use that then the probe_kernel_write() when modifying kernel
code.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/powerpc/kernel/ftrace.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 84a5ddd..4e376bc 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -63,11 +63,9 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
 		return -EINVAL;
 
 	/* replace the text with the new text */
-	if (probe_kernel_write((void *)ip, &new, MCOUNT_INSN_SIZE))
+	if (patch_instruction((unsigned int *)ip, new))
 		return -EPERM;
 
-	flush_icache_range(ip, ip + 8);
-
 	return 0;
 }
 
@@ -212,12 +210,9 @@ __ftrace_make_nop(struct module *mod,
 	 */
 	op = 0x48000008;	/* b +8 */
 
-	if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
+	if (patch_instruction((unsigned int *)ip, op))
 		return -EPERM;
 
-
-	flush_icache_range(ip, ip + 8);
-
 	return 0;
 }
 
@@ -286,11 +281,9 @@ __ftrace_make_nop(struct module *mod,
 
 	op = PPC_INST_NOP;
 
-	if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
+	if (patch_instruction((unsigned int *)ip, op))
 		return -EPERM;
 
-	flush_icache_range(ip, ip + 8);
-
 	return 0;
 }
 #endif /* PPC64 */
@@ -426,11 +419,9 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 	pr_devel("write to %lx\n", rec->ip);
 
-	if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
+	if (patch_instruction((unsigned int *)ip, op))
 		return -EPERM;
 
-	flush_icache_range(ip, ip + 8);
-
 	return 0;
 }
 #endif /* CONFIG_PPC64 */
-- 
1.7.9.5