ARM: ftrace: use canonical Thumb-2 wide instruction format

Extract out the instruction generation code so that it can be used for jump labels too.

Signed-off-by Rabin Vincent <[email protected]>

---
 arch/arm/kernel/Makefile |    5 ++-
 arch/arm/kernel/ftrace.c |   61 +++-------------------------------------------
 arch/arm/kernel/insn.c   |   60 +++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/insn.h   |   19 ++++++++++++++
 4 files changed, 86 insertions(+), 59 deletions(-)
 create mode 100644 arch/arm/kernel/insn.c
 create mode 100644 arch/arm/kernel/insn.h

arch/arm/kernel/Makefile +0 -0

Quoted 8 lines · Rabin Vincent

?static int ftrace_modify_code(unsigned long pc, unsigned long old,

(quoted diff) +1 -1

@@ -256,7 +203,7 @@ static int __ftrace_modify_caller(unsigned long *callsite,
 ?{
 ? ? ? unsigned long caller_fn = (unsigned long) func;
 ? ? ? unsigned long pc = (unsigned long) callsite;
- ? ? unsigned long branch = ftrace_gen_branch(pc, caller_fn, false);
+ ? ? unsigned long branch = arm_gen_branch(pc, caller_fn);

? ? ? unsigned long nop = 0xe1a00000; /* mov r0, r0 */

Does this code get used in Thumb-2 kernels? ?So far as I can tell, this code is used... but it looks like it shouldn't work, due to the ARM specifics.

Quoted 13 lines · Dave Martin

Quoted 8 lines · Rabin Vincent

?static int ftrace_modify_code(unsigned long pc, unsigned long old,

(quoted diff) +1 -1

@@ -256,7 +203,7 @@ static int __ftrace_modify_caller(unsigned long *callsite,
 ?{
 ? ? ? unsigned long caller_fn = (unsigned long) func;
 ? ? ? unsigned long pc = (unsigned long) callsite;
- ? ? unsigned long branch = ftrace_gen_branch(pc, caller_fn, false);
+ ? ? unsigned long branch = arm_gen_branch(pc, caller_fn);

? ? ? unsigned long nop = 0xe1a00000; /* mov r0, r0 */

Does this code get used in Thumb-2 kernels? ?So far as I can tell, this code is used... but it looks like it shouldn't work, due to the ARM specifics.

It's not used on Thumb-2. This is only used by the function graph tracer and that depends on !THUMB2_KERNEL.

Quoted 2 lines · Rabin Vincent

It's not used on Thumb-2. ?This is only used by the function graph tracer and that depends on !THUMB2_KERNEL.

I assume there's no special reason why this doesn't work with a Thumb-2 kernel, other than that it simply hasn't implemented yet?

Quoted 6 lines · Dave Martin

Quoted 2 lines · Rabin Vincent

It's not used on Thumb-2. ?This is only used by the function graph tracer and that depends on !THUMB2_KERNEL.

I assume there's no special reason why this doesn't work with a Thumb-2 kernel, other than that it simply hasn't implemented yet?

The function graph tracer expects to get a pointer to the saved return address of the function (on the stack). It changes this to its own return_hooker function where it records the exit from the function and jumps to the real return address.

We do this for ARM (the instruction set) by building with frame pointers, and the function prologues always save the registers in the expected order and the epilogues restore them out from the saved locations.

However, with Thumb-2, we don't get frame pointers even if we ask for them. So to support Thumb-2 we'd probably need a way to find and modify the LR without it being at a fixed location (MIPS does something similar IIRC). I haven't really looked into it.

Are there any other dependencies on framepointers, or is that the only thing?

Extract out the code patching code from kprobes so that it can be used from the jump label code. Additionally, the separated code:

- Uses the IS_ENABLED() macros instead of the #ifdefs for THUMB2 support

- Unifies the two separate functions in kprobes, providing one function that uses stop_machine() internally, and one that can be called from stop_machine() directly

- Patches the text on all CPUs only on processors requiring software broadcasting of cache operations

Cc Jon Medhurst <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

---

 arch/arm/kernel/Makefile  |    3 +-
 arch/arm/kernel/kprobes.c |   86 ++++++++++++--------------------------------
 arch/arm/kernel/patch.c   |   72 +++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/patch.h   |    7 ++++
 4 files changed, 105 insertions(+), 63 deletions(-)
 create mode 100644 arch/arm/kernel/patch.c
 create mode 100644 arch/arm/kernel/patch.h

arch/arm/kernel/Makefile +2 -1

index a2f34a3..adf02ed 100644
@@ -8,6 +8,7 @@ AFLAGS_head.o        := -DTEXT_OFFSET=$(TEXT_OFFSET)
 ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_ftrace.o = -pg
 CFLAGS_REMOVE_insn.o = -pg
+CFLAGS_REMOVE_patch.o = -pg
 endif
 
 CFLAGS_REMOVE_return_address.o = -pg

@@ -38,7 +39,7 @@ obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
-obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o
+obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o patch.o
 ifdef CONFIG_THUMB2_KERNEL
 obj-$(CONFIG_KPROBES)		+= kprobes-thumb.o
 else

arch/arm/kernel/kprobes.c +24 -62

index 129c116..ab1869d 100644
@@ -29,6 +29,7 @@
 #include <asm/cacheflush.h>
 
 #include "kprobes.h"
+#include "patch.h"
 
 #define MIN_STACK_SIZE(addr) 				\
 	min((unsigned long)MAX_STACK_SIZE,		\

@@ -103,57 +104,33 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	return 0;
 }
 
-#ifdef CONFIG_THUMB2_KERNEL
-
-/*
- * For a 32-bit Thumb breakpoint spanning two memory words we need to take
- * special precautions to insert the breakpoint atomically, especially on SMP
- * systems. This is achieved by calling this arming function using stop_machine.
- */
-static int __kprobes set_t32_breakpoint(void *addr)
-{
-	((u16 *)addr)[0] = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION >> 16;
-	((u16 *)addr)[1] = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION & 0xffff;
-	flush_insns(addr, 2*sizeof(u16));
-	return 0;
-}
-
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
-	uintptr_t addr = (uintptr_t)p->addr & ~1; /* Remove any Thumb flag */
-
-	if (!is_wide_instruction(p->opcode)) {
-		*(u16 *)addr = KPROBE_THUMB16_BREAKPOINT_INSTRUCTION;
-		flush_insns(addr, sizeof(u16));
-	} else if (addr & 2) {
-		/* A 32-bit instruction spanning two words needs special care */
-		stop_machine(set_t32_breakpoint, (void *)addr, &cpu_online_map);
+	unsigned int brkp;
+	void *addr;
+
+	if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+		/* Remove any Thumb flag */
+		addr = (void *)((uintptr_t)p->addr & ~1);
+
+		if (is_wide_instruction(p->opcode))
+			brkp = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION;
+		else
+			brkp = KPROBE_THUMB16_BREAKPOINT_INSTRUCTION;
 	} else {
-		/* Word aligned 32-bit instruction can be written atomically */
-		u32 bkp = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION;
-#ifndef __ARMEB__ /* Swap halfwords for little-endian */
-		bkp = (bkp >> 16) | (bkp << 16);
-#endif
-		*(u32 *)addr = bkp;
-		flush_insns(addr, sizeof(u32));
-	}
-}
+		kprobe_opcode_t insn = p->opcode;
 
-#else /* !CONFIG_THUMB2_KERNEL */
+		addr = p->addr;
+		brkp = KPROBE_ARM_BREAKPOINT_INSTRUCTION;
 
-void __kprobes arch_arm_kprobe(struct kprobe *p)
-{
-	kprobe_opcode_t insn = p->opcode;
-	kprobe_opcode_t brkp = KPROBE_ARM_BREAKPOINT_INSTRUCTION;
-	if (insn >= 0xe0000000)
-		brkp |= 0xe0000000;  /* Unconditional instruction */
-	else
-		brkp |= insn & 0xf0000000;  /* Copy condition from insn */
-	*p->addr = brkp;
-	flush_insns(p->addr, sizeof(p->addr[0]));
-}
+		if (insn >= 0xe0000000)
+			brkp |= 0xe0000000;  /* Unconditional instruction */
+		else
+			brkp |= insn & 0xf0000000;  /* Copy condition from insn */
+	}
 
-#endif /* !CONFIG_THUMB2_KERNEL */
+	patch_text(addr, brkp);
+}
 
 /*
  * The actual disarming is done here on each CPU and synchronized using

@@ -166,25 +143,10 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
 int __kprobes __arch_disarm_kprobe(void *p)
 {
 	struct kprobe *kp = p;
-#ifdef CONFIG_THUMB2_KERNEL
-	u16 *addr = (u16 *)((uintptr_t)kp->addr & ~1);
-	kprobe_opcode_t insn = kp->opcode;
-	unsigned int len;
+	void *addr = (void *)((uintptr_t)kp->addr & ~1);
 
-	if (is_wide_instruction(insn)) {
-		((u16 *)addr)[0] = insn>>16;
-		((u16 *)addr)[1] = insn;
-		len = 2*sizeof(u16);
-	} else {
-		((u16 *)addr)[0] = insn;
-		len = sizeof(u16);
-	}
-	flush_insns(addr, len);
+	__patch_text(addr, kp->opcode);
 
-#else /* !CONFIG_THUMB2_KERNEL */
-	*kp->addr = kp->opcode;
-	flush_insns(kp->addr, sizeof(kp->addr[0]));
-#endif
 	return 0;
 }
 

arch/arm/kernel/patch.c new file +13 -0

new file mode 100644
index 0000000..3f93190
@@ -0,0 +1,72 @@
+#include <linux/kernel.h>
+#include <linux/kprobes.h>
+#include <linux/stop_machine.h>
+
+#include <asm/cacheflush.h>
+#include <asm/smp_plat.h>
+
+#include "patch.h"
+
+struct patch {
+	void *addr;
+	unsigned int insn;
+};

arch/arm/kernel/patch.c new file +38 -0

+
+		addrw[0] = insn >> 16;
+		addrw[1] = insn & 0xffff;
+
+		size = sizeof(u32);
+	} else {
+#ifndef __ARMEB__
+		if (thumb2)
+			insn = (insn >> 16) | (insn << 16);
+#endif
+
+		*(u32 *)addr = insn;
+		size = sizeof(u32);
+	}
+
+	flush_icache_range((unsigned long)(addr),
+			   (unsigned long)(addr) + size);
+}
+
+static int __kprobes patch_text_stop_machine(void *data)
+{
+	struct patch *patch = data;
+
+	__patch_text(patch->addr, patch->insn);
+
+	return 0;
+}
+
+void __kprobes patch_text(void *addr, unsigned int insn)
+{
+	bool stopmachine;
+	struct patch patch = {
+		.addr = addr,
+		.insn = insn,
+	};
+
+	stopmachine = cache_ops_need_broadcast() ||
+		      (IS_ENABLED(CONFIG_THUMB2_KERNEL)

arch/arm/kernel/patch.h new file +7 -0

new file mode 100644
index 0000000..b4731f2
@@ -0,0 +1,7 @@
+#ifndef _ARM_KERNEL_PATCH_H
+#define _ARM_KERNEL_PATCH_H
+
+void patch_text(void *addr, unsigned int insn);
+void __patch_text(void *addr, unsigned int insn);
+
+#endif

Quoted 10 lines · Rabin Vincent

Extract out the code patching code from kprobes so that it can be used from the jump label code. ?Additionally, the separated code:

?- Uses the IS_ENABLED() macros instead of the #ifdefs for THUMB2 ? ?support

?- Unifies the two separate functions in kprobes, providing one function ? ?that uses stop_machine() internally, and one that can be called from ? ?stop_machine() directly

Quoted 6 lines

?- Patches the text on all CPUs only on processors requiring software ? ?broadcasting of cache operations

Cc Jon Medhurst <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

---

Looks good. I've a couple of minor inline comments below.

Add the arch-specific code to support jump labels for ARM and Thumb-2.

Note that to build succesfully it requires (what will be) GCC 4.7.0 because of incomplete support for the '%c' specifier in earlier versions:

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637

Cc Jason Baron <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

Quoted 6 lines · Rabin Vincent

Add the arch-specific code to support jump labels for ARM and Thumb-2.

Note that to build succesfully it requires (what will be) GCC 4.7.0 because of incomplete support for the '%c' specifier in earlier versions:

Quoted 4 lines

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637

Cc Jason Baron <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

This appears to imply that with this patch, we're upping the minimum gcc version for successfully building kernels to 4.7.0. That's _way_ too early (I'm using 4.3.5 here and don't have plans to update.)

Quoted 11 lines · Russell King - ARM Linux

Quoted 10 lines · Rabin Vincent

Add the arch-specific code to support jump labels for ARM and Thumb-2.

Note that to build succesfully it requires (what will be) GCC 4.7.0 because of incomplete support for the '%c' specifier in earlier versions:

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637

Cc Jason Baron <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

Quoted 3 lines

This appears to imply that with this patch, we're upping the minimum gcc version for successfully building kernels to 4.7.0. That's _way_ too early (I'm using 4.3.5 here and don't have plans to update.)

Is there a way to detect that the compiler doesn't support %c? Perhaps we can do that in the same spirit of how we detect asm goto support for jump labels.

Right, for jump labels we have a script at: scripts/gcc-goto.sh, which basically just checks if we can compile 'asm goto' without error, so for %c, you could probably have some simple 'asm()' that uses %c, and see if it compiles. Although, I'm not sure if there's a arch specific dir, for such a thing?

thanks,

-Jason

Quoted 6 lines · Jason Baron

Right, for jump labels we have a script at: scripts/gcc-goto.sh, which basically just checks if we can compile 'asm goto' without error, so for %c, you could probably have some simple 'asm()' that uses %c, and see if it compiles. Although, I'm not sure if there's a arch specific dir, for such a thing?

Quoted 3 lines

thanks,

-Jason

hmm...actually we could add it to the top level scripts/gcc-goto.sh, that would be fine, I can take a look at that...

From ea211ce77e0ea9c5b8cd32b99bfb24e0140f0068 Mon Sep 17 00:00:00 2001

Here's a patch. Atleast on MIPS %c means something else so I had to ifdef it for ARM:

8<----------------------------

Quoted 1 line

From ea211ce77e0ea9c5b8cd32b99bfb24e0140f0068 Mon Sep 17 00:00:00 2001

From: Rabin Vincent <[email protected]>
Date: Wed, 23 Nov 2011 20:05:09 +0530
Subject: [PATCH] jump label: detect %c support for ARM

Some versions of ARM GCC don't support the %c specifier fully, but do support asm goto. Since we need the %c to support jump labels on ARM, detect that too in the asm goto detection script to avoid build errors with these versions.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637

Signed-off-by Rabin Vincent <[email protected]>

---
 scripts/gcc-goto.sh |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

scripts/gcc-goto.sh +7 -1

index 98cffcb..9b744de 100644
@@ -2,4 +2,20 @@
 # Test for gcc 'asm goto' support
 # Copyright (C) 2010, Jason Baron <[email protected]>
 
-echo "int main(void) { entry: asm goto (\"\"::::entry); return 0; }" | $@ -x c - -c -o /dev/null >/dev/null 2>&1 && echo "y"
+cat << "END" | $@ -x c - -c -o /dev/null >/dev/null 2>&1 && echo "y"
+int main(void)
+{
+#ifdef __arm__
+	/*
+	 * Not related to asm goto, but used by jump label
+	 * and broken on some ARM GCC versions (see GCC Bug 48637).

Quoted 12 lines · Rabin Vincent

scripts/gcc-goto.sh +12 -0

+cat << "END" | $@ -x c - -c -o /dev/null >/dev/null 2>&1 && echo "y"
+int main(void)
+{
+#ifdef __arm__
+ ? ? /*
+ ? ? ?* Not related to asm goto, but used by jump label
+ ? ? ?* and broken on some ARM GCC versions (see GCC Bug 48637).
+ ? ? ?*/
+ ? ? static struct { int dummy; int state; } tp;
+ ? ? asm ("@ %c0" :: "i" (&tp.state));
+#endif
+

scripts/gcc-goto.sh line 21

Quoted 5 lines

scripts/gcc-goto.sh +5 -0

+entry:
+ ? ? asm goto ("" :::: entry);
+ ? ? return 0;
+}
+END

Is the presence of asm goto and %c something we want to test together or separately? ?I suspect it's something we want to do separately.

arch/arm/kernel/insn.h line 12

Quoted 9 lines · Rabin Vincent

arch/arm/kernel/insn.h +9 -0

+static inline unsigned long
+arm_gen_nop(void)
+{
+#ifdef CONFIG_THUMB2_KERNEL
+	return 0xf3af8000; /* nop.w */
+#elif defined(CONFIG_CPU_32v6K)
+	return 0xe320f000; /* nop */
+#else
+	return 0xe1a00000; /* mov r0, r0 */

There really is no point making the distinction between the new nop and the old nop instructions. The difference between them is that the new nop is a true 'no operation' whereas the old nop causes exactly what the instruction says to happen - which in effect is a no-op.

Obviously, doing a true no-operation may require in less power, but if you're using this code, you're debugging, so power usage isn't really a concern. So lets keep the code simple and just use the old nop here. It won't go away.

Quoted 16 lines · Russell King - ARM Linux

arch/arm/kernel/insn.h line 12

Quoted 9 lines · Rabin Vincent

arch/arm/kernel/insn.h +9 -0

+static inline unsigned long
+arm_gen_nop(void)
+{
+#ifdef CONFIG_THUMB2_KERNEL
+	return 0xf3af8000; /* nop.w */
+#elif defined(CONFIG_CPU_32v6K)
+	return 0xe320f000; /* nop */
+#else
+	return 0xe1a00000; /* mov r0, r0 */

There really is no point making the distinction between the new nop and the old nop instructions. The difference between them is that the new nop is a true 'no operation' whereas the old nop causes exactly what the instruction says to happen - which in effect is a no-op.

Quoted 4 lines

Obviously, doing a true no-operation may require in less power, but if you're using this code, you're debugging, so power usage isn't really a concern. So lets keep the code simple and just use the old nop here. It won't go away.

Is "if you're using this code, you're debugging" really correct here? Because the the purpose of jump labels is to reduce the overhead of tracepoints when they aren't activated, i.e. when you're not actively debugging.

Let me repeat: the difference is power. Nothing more nothing less. 'nop' was invented to allow some vendors to 'optimize' nop from a power management point of view. Nothing more, nothing less.

'nop' will be executed as a true 'no operation' where as 'mov r0, r0' will tell the CPU to load the value of r0 and store it back into the register - which involves doing an operation.

There's no difference between "nop" vs "mov r0, r0" in terms of performance. It's purely a power thing.

If you have tracepoints enabled, then you're taking a performance hit anyway by having all those sites inserted. So, the only time you'd sanely enable them is if you're doing development work. You wouldn't enable them for production, unless you had some desire to waste CPU cycles needlessly in your end users products (maybe you're trying to go for the worlds slowest device?)

As commit 592201a9f15 (ARM: Thumb-2: Support Thumb-2 in undefined instruction handler) says:

32-bit Thumb instructions are specified in the form:
((first_half << 16 ) | second_half)
which matches the layout used by the ARM ARM.

Convert the ftrace code to use the same format to avoid the usage of different formats in kernel code.

Signed-off-by Rabin Vincent <[email protected]>

---
 arch/arm/kernel/ftrace.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

arch/arm/kernel/ftrace.c +2 -2

index c0062ad..cdceb63 100644
@@ -19,7 +19,7 @@
 #include <asm/ftrace.h>
 
 #ifdef CONFIG_THUMB2_KERNEL
-#define	NOP		0xeb04f85d	/* pop.w {lr} */
+#define	NOP		0xf85deb04	/* pop.w {lr} */
 #else
 #define	NOP		0xe8bd4000	/* pop {lr} */
 #endif

@@ -88,7 +88,7 @@ static unsigned long ftrace_gen_branch(unsigned long pc, unsigned long addr,
 	if (link)
 		second |= 1 << 14;
 
-	return (second << 16) | first;
+	return (first << 16) | second;
 }
 #else
 static unsigned long ftrace_gen_branch(unsigned long pc, unsigned long addr,

arch/arm/kernel/ftrace.c +11 -5

+	}
+#endif
 
-	if (replaced != old)
-		return -EINVAL;
+	if (old) {
+		if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
+			return -EFAULT;
+
+		if (replaced != old)
+			return -EINVAL;
+	}
 
 	if (probe_kernel_write((void *)pc, &new, MCOUNT_INSN_SIZE))
 		return -EPERM;

@@ -141,23 +150,21 @@ static int ftrace_modify_code(unsigned long pc, unsigned long old,
 
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
-	unsigned long pc, old;
+	unsigned long pc;
 	unsigned long new;
 	int ret;
 
 	pc = (unsigned long)&ftrace_call;
-	memcpy(&old, &ftrace_call, MCOUNT_INSN_SIZE);
 	new = ftrace_call_replace(pc, (unsigned long)func);
 
-	ret = ftrace_modify_code(pc, old, new);
+	ret = ftrace_modify_code(pc, 0, new);
 

Quoted 4 lines · Rabin Vincent

+#ifndef __ARMEB__
+ ? ? if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+ ? ? ? ? ? ? old = (old >> 16) | (old << 16);
+ ? ? ? ? ? ? new = (new >> 16) | (new << 16);

I think swahw32() in <linux/swab.h> can be used for this operation.

Really though, we need a common set of "load and store instruction" macros, rather than duplicating this knowledge everywhere.

In particular, we really want those macros to encapsulate the #ifdef __ARMEB__ stuff.

Quoted 39 lines

+ ? ? }

(quoted diff) +8 -2

+#endif
 
- ? ? if (replaced != old)
- ? ? ? ? ? ? return -EINVAL;
+ ? ? if (old) {
+ ? ? ? ? ? ? if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
+ ? ? ? ? ? ? ? ? ? ? return -EFAULT;
+
+ ? ? ? ? ? ? if (replaced != old)
+ ? ? ? ? ? ? ? ? ? ? return -EINVAL;
+ ? ? }

? ? ? if (probe_kernel_write((void *)pc, &new, MCOUNT_INSN_SIZE)) ? ? ? ? ? ? ? return -EPERM;

(quoted diff) +2 -3

@@ -141,23 +150,21 @@ static int ftrace_modify_code(unsigned long pc, unsigned long old,
 
 ?int ftrace_update_ftrace_func(ftrace_func_t func)
 ?{
- ? ? unsigned long pc, old;
+ ? ? unsigned long pc;
 ? ? ? unsigned long new;
 ? ? ? int ret;
 
 ? ? ? pc = (unsigned long)&ftrace_call;
- ? ? memcpy(&old, &ftrace_call, MCOUNT_INSN_SIZE);
 ? ? ? new = ftrace_call_replace(pc, (unsigned long)func);
 
- ? ? ret = ftrace_modify_code(pc, old, new);
+ ? ? ret = ftrace_modify_code(pc, 0, new);
 

?#ifdef CONFIG_OLD_MCOUNT
? ? ? if (!ret) {
? ? ? ? ? ? ? pc = (unsigned long)&ftrace_call_old;
- ? ? ? ? ? ? memcpy(&old, &ftrace_call_old, MCOUNT_INSN_SIZE); ? ? ? ? ? ? ? new = ftrace_call_replace(pc, (unsigned long)func);

Quoted 4 lines

- ? ? ? ? ? ? ret = ftrace_modify_code(pc, old, new);
+ ? ? ? ? ? ? ret = ftrace_modify_code(pc, 0, new); ? ? ? }
?#endif

Why don't we check the old value any more?

It would be good to see something in comments or the commit log clarifying this.

To what extent can this be unified with the kprobes functionality? (if you've already done that, ignore this comment -- I confess I don't understand all the details of these patches...)

Quoted 7 lines · Dave Martin

Quoted 4 lines · Rabin Vincent

+#ifndef __ARMEB__
+ ? ? if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+ ? ? ? ? ? ? old = (old >> 16) | (old << 16);
+ ? ? ? ? ? ? new = (new >> 16) | (new << 16);

I think swahw32() in <linux/swab.h> can be used for this operation.

Yes.

Quoted 2 lines

Really though, we need a common set of "load and store instruction" macros, rather than duplicating this knowledge everywhere.

Yes, though this ftrace code may not be able to use them directly since it uses probe_kernel_read()/write().

Quoted 11 lines · Rabin Vincent

Quoted 9 lines · Dave Martin

arch/arm/kernel/ftrace.c static int ftrace_modify_code(unsigned long pc, unsigned long old,

Quoted 5 lines · Rabin Vincent

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

+#ifndef __ARMEB__
+     if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+             old = (old>>  16) | (old<<  16);
+             new = (new>>  16) | (new<<  16);

I think swahw32() in<linux/swab.h> can be used for this operation.

Quoted 9 lines

Yes.

Quoted 3 lines

Really though, we need a common set of "load and store instruction" macros, rather than duplicating this knowledge everywhere.

Yes, though this ftrace code may not be able to use them directly since it uses probe_kernel_read()/write().

In that case what we would want are macros to convert instructions between integer and in-memory representation, rather than conflating all that together in some memory accessor macros.

How about:

#include<linux/swab.h>

#ifdef __ARMEB__
#define insn_to_mem_arm(insn) swab32(insn)
#define insn_to_mem_thumb16(insn) swab16(insn)
#define insn_to_mem_thumb32(insn) swab16(insn)
#else
#define insn_to_mem_arm(insn) (insn)
#define insn_to_mem_thumb16(insn) (insn)
#define insn_to_mem_thumb32(insn) shahw32(insn)
#endif

#define mem_to_insn_arm(insn) insn_to_mem_asm(insn) #define mem_to_insn_thumb16(insn) insn_to_mem_thumb16(insn) #define mem_to_insn_thumb32(insn) insn_to_mem_thumb32(insn)

Because these are all straightforward swap operations, we don't really need separate forwards and backwards operations, but having the separate names may make calling code easier to read.

I'm still not sure exactly which header this belongs in, but it's a core ARM thing; not local to kprobes or ftrace, etc.

If this looks sensible, it would also make sense for Junxiao Bi to build his big-endian fixes on it.

Cheers
---Dave

Quoted 40 lines · Dave Martin

Quoted 6 lines · Rabin Vincent

Quoted 5 lines · Dave Martin

Quoted 4 lines · Rabin Vincent

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

+#ifndef __ARMEB__
+     if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+             old = (old>>  16) | (old<<  16);
+             new = (new>>  16) | (new<<  16);

Quoted 7 lines

Quoted 1 line

I think swahw32() in<linux/swab.h> can be used for this operation.

Yes.

Quoted 2 lines

Really though, we need a common set of "load and store instruction" macros, rather than duplicating this knowledge everywhere.

Yes, though this ftrace code may not be able to use them directly since it uses probe_kernel_read()/write().

In that case what we would want are macros to convert instructions between integer and in-memory representation, rather than conflating all that together in some memory accessor macros.

How about:

#include<linux/swab.h>

#ifdef __ARMEB__
#define insn_to_mem_arm(insn) swab32(insn)
#define insn_to_mem_thumb16(insn) swab16(insn)
#define insn_to_mem_thumb32(insn) swab16(insn)
#else
#define insn_to_mem_arm(insn) (insn)
#define insn_to_mem_thumb16(insn) (insn)
#define insn_to_mem_thumb32(insn) shahw32(insn)
#endif

#define mem_to_insn_arm(insn) insn_to_mem_asm(insn) #define mem_to_insn_thumb16(insn) insn_to_mem_thumb16(insn) #define mem_to_insn_thumb32(insn) insn_to_mem_thumb32(insn)

Because these are all straightforward swap operations, we don't really need separate forwards and backwards operations, but having the separate names may make calling code easier to read.

Quoted 3 lines

I'm still not sure exactly which header this belongs in, but it's a core ARM thing; not local to kprobes or ftrace, etc.

Yes, these macros are useful for be8 support. Do they deserve a separated header file as there is no appropriate one for them now?

Quoted 2 lines

If this looks sensible, it would also make sense for Junxiao Bi to build his big-endian fixes on it.