[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[Rabin Vincent]

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 <rabin@rab.in>
---
 arch/arm/kernel/ftrace.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index c0062ad..cdceb63 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -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,
@@ -125,11 +125,20 @@ static int ftrace_modify_code(unsigned long pc, unsigned long old,
 {
 	unsigned long replaced;
 
-	if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
-		return -EFAULT;
+#ifndef __ARMEB__
+	if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
+		old = (old >> 16) | (old << 16);
+		new = (new >> 16) | (new << 16);
+	}
+#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);
 
 #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);
 
-		ret = ftrace_modify_code(pc, old, new);
+		ret = ftrace_modify_code(pc, 0, new);
 	}
 #endif
 
-- 
1.7.7.3

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Rabin Vincent]

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

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 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

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 16eed6a..a2f34a3 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -7,6 +7,7 @@ AFLAGS_head.o        := -DTEXT_OFFSET=$(TEXT_OFFSET)
 
 ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_ftrace.o = -pg
+CFLAGS_REMOVE_insn.o = -pg
 endif
 
 CFLAGS_REMOVE_return_address.o = -pg
@@ -34,8 +35,8 @@ obj-$(CONFIG_HAVE_SCHED_CLOCK)	+= sched_clock.o
 obj-$(CONFIG_SMP)		+= smp.o smp_tlb.o
 obj-$(CONFIG_HAVE_ARM_SCU)	+= smp_scu.o
 obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
-obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.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
 ifdef CONFIG_THUMB2_KERNEL
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index cdceb63..c46d18b 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -18,6 +18,8 @@
 #include <asm/cacheflush.h>
 #include <asm/ftrace.h>
 
+#include "insn.h"
+
 #ifdef CONFIG_THUMB2_KERNEL
 #define	NOP		0xf85deb04	/* pop.w {lr} */
 #else
@@ -60,64 +62,9 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr)
 }
 #endif
 
-#ifdef CONFIG_THUMB2_KERNEL
-static unsigned long ftrace_gen_branch(unsigned long pc, unsigned long addr,
-				       bool link)
-{
-	unsigned long s, j1, j2, i1, i2, imm10, imm11;
-	unsigned long first, second;
-	long offset;
-
-	offset = (long)addr - (long)(pc + 4);
-	if (offset < -16777216 || offset > 16777214) {
-		WARN_ON_ONCE(1);
-		return 0;
-	}
-
-	s	= (offset >> 24) & 0x1;
-	i1	= (offset >> 23) & 0x1;
-	i2	= (offset >> 22) & 0x1;
-	imm10	= (offset >> 12) & 0x3ff;
-	imm11	= (offset >>  1) & 0x7ff;
-
-	j1 = (!i1) ^ s;
-	j2 = (!i2) ^ s;
-
-	first = 0xf000 | (s << 10) | imm10;
-	second = 0x9000 | (j1 << 13) | (j2 << 11) | imm11;
-	if (link)
-		second |= 1 << 14;
-
-	return (first << 16) | second;
-}
-#else
-static unsigned long ftrace_gen_branch(unsigned long pc, unsigned long addr,
-				       bool link)
-{
-	unsigned long opcode = 0xea000000;
-	long offset;
-
-	if (link)
-		opcode |= 1 << 24;
-
-	offset = (long)addr - (long)(pc + 8);
-	if (unlikely(offset < -33554432 || offset > 33554428)) {
-		/* Can't generate branches that far (from ARM ARM). Ftrace
-		 * doesn't generate branches outside of kernel text.
-		 */
-		WARN_ON_ONCE(1);
-		return 0;
-	}
-
-	offset = (offset >> 2) & 0x00ffffff;
-
-	return opcode | offset;
-}
-#endif
-
 static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
 {
-	return ftrace_gen_branch(pc, addr, true);
+	return arm_gen_branch_link(pc, addr);
 }
 
 static int ftrace_modify_code(unsigned long pc, unsigned long old,
@@ -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 */
 	unsigned long old = enable ? nop : branch;
 	unsigned long new = enable ? branch : nop;
diff --git a/arch/arm/kernel/insn.c b/arch/arm/kernel/insn.c
new file mode 100644
index 0000000..f65b68c
--- /dev/null
+++ b/arch/arm/kernel/insn.c
@@ -0,0 +1,60 @@
+#include <linux/kernel.h>
+
+static unsigned long
+__arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link)
+{
+	unsigned long s, j1, j2, i1, i2, imm10, imm11;
+	unsigned long first, second;
+	long offset;
+
+	offset = (long)addr - (long)(pc + 4);
+	if (offset < -16777216 || offset > 16777214) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	s	= (offset >> 24) & 0x1;
+	i1	= (offset >> 23) & 0x1;
+	i2	= (offset >> 22) & 0x1;
+	imm10	= (offset >> 12) & 0x3ff;
+	imm11	= (offset >>  1) & 0x7ff;
+
+	j1 = (!i1) ^ s;
+	j2 = (!i2) ^ s;
+
+	first = 0xf000 | (s << 10) | imm10;
+	second = 0x9000 | (j1 << 13) | (j2 << 11) | imm11;
+	if (link)
+		second |= 1 << 14;
+
+	return (first << 16) | second;
+}
+
+static unsigned long
+__arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link)
+{
+	unsigned long opcode = 0xea000000;
+	long offset;
+
+	if (link)
+		opcode |= 1 << 24;
+
+	offset = (long)addr - (long)(pc + 8);
+	if (unlikely(offset < -33554432 || offset > 33554428)) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	offset = (offset >> 2) & 0x00ffffff;
+
+	return opcode | offset;
+}
+
+unsigned long
+__arm_gen_branch(unsigned long pc, unsigned long addr, bool link)
+{
+	if (IS_ENABLED(CONFIG_THUMB2_KERNEL))
+		return __arm_gen_branch_thumb2(pc, addr, link);
+	else
+		return __arm_gen_branch_arm(pc, addr, link);
+}
diff --git a/arch/arm/kernel/insn.h b/arch/arm/kernel/insn.h
new file mode 100644
index 0000000..994d60f
--- /dev/null
+++ b/arch/arm/kernel/insn.h
@@ -0,0 +1,19 @@
+#ifndef __ASM_ARM_INSN_H
+#define __ASM_ARM_INSN_H
+
+unsigned long
+__arm_gen_branch(unsigned long pc, unsigned long addr, bool link);
+
+static inline unsigned long
+arm_gen_branch(unsigned long pc, unsigned long addr)
+{
+	return __arm_gen_branch(pc, addr, false);
+}
+
+static inline unsigned long
+arm_gen_branch_link(unsigned long pc, unsigned long addr)
+{
+	return __arm_gen_branch(pc, addr, true);
+}
+
+#endif
-- 
1.7.7.3

[PATCH 3/4] ARM: extract out code patch function from kprobes

[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

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

Cc: Jon Medhurst <tixy@yxit.co.uk>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 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

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index a2f34a3..adf02ed 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -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
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 129c116..ab1869d 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -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;
 }
 
diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
new file mode 100644
index 0000000..3f93190
--- /dev/null
+++ b/arch/arm/kernel/patch.c
@@ -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;
+};
+
+void __kprobes __patch_text(void *addr, unsigned int insn)
+{
+	bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
+	int size;
+
+	if (thumb2 && !is_wide_instruction(insn)) {
+		*(u16 *)addr = insn;
+		size = sizeof(u16);
+	} else if (thumb2 && ((unsigned long)addr & 2)) {
+		u16 *addrw = addr;
+
+		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)
+		       && is_wide_instruction(insn)
+		       && ((unsigned long)addr & 2));
+
+	if (!stopmachine)
+		__patch_text(addr, insn);
+	else if (cache_ops_need_broadcast())
+		stop_machine(patch_text_stop_machine, &patch, cpu_online_mask);
+	else
+		stop_machine(patch_text_stop_machine, &patch, NULL);
+}
diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
new file mode 100644
index 0000000..b4731f2
--- /dev/null
+++ b/arch/arm/kernel/patch.h
@@ -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
-- 
1.7.7.3

[PATCH 4/4] ARM: add jump label support

[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 <jbaron@redhat.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/Kconfig                  |    1 +
 arch/arm/include/asm/jump_label.h |   41 +++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/Makefile          |    1 +
 arch/arm/kernel/insn.h            |   12 ++++++++++
 arch/arm/kernel/jump_label.c      |   35 +++++++++++++++++++++++++++++++
 5 files changed, 90 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/jump_label.h
 create mode 100644 arch/arm/kernel/jump_label.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 44789ef..ae6d31c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM
 	select SYS_SUPPORTS_APM_EMULATION
 	select GENERIC_ATOMIC64 if (CPU_V6 || !CPU_32v6K || !AEABI)
 	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
+	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL
 	select HAVE_ARCH_KGDB
 	select HAVE_KPROBES if !XIP_KERNEL
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h
new file mode 100644
index 0000000..5c5ca2e
--- /dev/null
+++ b/arch/arm/include/asm/jump_label.h
@@ -0,0 +1,41 @@
+#ifndef _ASM_ARM_JUMP_LABEL_H
+#define _ASM_ARM_JUMP_LABEL_H
+
+#ifdef __KERNEL__
+
+#include <linux/types.h>
+#include <asm/system.h>
+
+#define JUMP_LABEL_NOP_SIZE 4
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define JUMP_LABEL_NOP	"nop.w"
+#else
+#define JUMP_LABEL_NOP	"nop"
+#endif
+
+static __always_inline bool arch_static_branch(struct jump_label_key *key)
+{
+	asm goto("1:\n\t"
+		 JUMP_LABEL_NOP "\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection\n\t"
+		 : :  "i" (key) :  : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+#endif /* __KERNEL__ */
+
+typedef u32 jump_label_t;
+
+struct jump_entry {
+	jump_label_t code;
+	jump_label_t target;
+	jump_label_t key;
+};
+
+#endif
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index adf02ed..be200ad 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_HAVE_ARM_SCU)	+= smp_scu.o
 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_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o patch.o
 ifdef CONFIG_THUMB2_KERNEL
diff --git a/arch/arm/kernel/insn.h b/arch/arm/kernel/insn.h
index 994d60f..c32d271 100644
--- a/arch/arm/kernel/insn.h
+++ b/arch/arm/kernel/insn.h
@@ -1,6 +1,18 @@
 #ifndef __ASM_ARM_INSN_H
 #define __ASM_ARM_INSN_H
 
+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 */
+#endif
+}
+
 unsigned long
 __arm_gen_branch(unsigned long pc, unsigned long addr, bool link);
 
diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
new file mode 100644
index 0000000..7900914
--- /dev/null
+++ b/arch/arm/kernel/jump_label.c
@@ -0,0 +1,35 @@
+#include <linux/kernel.h>
+#include <linux/jump_label.h>
+
+#include "insn.h"
+#include "patch.h"
+
+static void __arch_jump_label_transform(struct jump_entry *entry,
+					enum jump_label_type type,
+					bool is_static)
+{
+	void *addr = (void *)entry->code;
+	unsigned int insn;
+
+	if (type == JUMP_LABEL_ENABLE)
+		insn = arm_gen_branch(entry->code, entry->target);
+	else
+		insn = arm_gen_nop();
+
+	if (is_static)
+		__patch_text(addr, insn);
+	else
+		patch_text(addr, insn);
+}
+
+void arch_jump_label_transform(struct jump_entry *entry,
+			       enum jump_label_type type)
+{
+	__arch_jump_label_transform(entry, type, false);
+}
+
+void arch_jump_label_transform_static(struct jump_entry *entry,
+				      enum jump_label_type type)
+{
+	__arch_jump_label_transform(entry, type, true);
+}
-- 
1.7.7.3

[PATCH 3/4] ARM: extract out code patch function from kprobes

[Tixy]

On Mon, 2011-11-21 at 20:43 +0530, Rabin Vincent wrote:
> 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 <tixy@yxit.co.uk>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---

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

>  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
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index a2f34a3..adf02ed 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -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
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index 129c116..ab1869d 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -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;
>  }
>  
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> new file mode 100644
> index 0000000..3f93190
> --- /dev/null
> +++ b/arch/arm/kernel/patch.c
> @@ -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;
> +};
> +
> +void __kprobes __patch_text(void *addr, unsigned int insn)
> +{
> +	bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
> +	int size;
> +
> +	if (thumb2 && !is_wide_instruction(insn)) {
> +		*(u16 *)addr = insn;
> +		size = sizeof(u16);
> +	} else if (thumb2 && ((unsigned long)addr & 2)) {

For pointer arithmetic, is casting to (uintptr_t) preferred to (unsigned
long)? (There are several other places in this file where this comment
applies.)

> +		u16 *addrw = addr;

Here's some very pedantic nit-picking...
I guess the 'w' in 'addrw' comes from X86 land meaning 'word'?
'h' for 'half-word' would be more appropriate for ARM.

> +
> +		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)
> +		       && is_wide_instruction(insn)
> +		       && ((unsigned long)addr & 2));
> +
> +	if (!stopmachine)
> +		__patch_text(addr, insn);
> +	else if (cache_ops_need_broadcast())
> +		stop_machine(patch_text_stop_machine, &patch, cpu_online_mask);
> +	else
> +		stop_machine(patch_text_stop_machine, &patch, NULL);
> +}

This inlines the cache_ops_need_broadcast() code twice. We could instead
either cache the result in a local variable, or rewrite if/else clause
to something like...

	if (cache_ops_need_broadcast())
		stop_machine(patch_text_stop_machine, &patch, cpu_online_mask);
	else {
		bool straddles_word = IS_ENABLED(CONFIG_THUMB2_KERNEL)
					&& is_wide_instruction(insn)
					&& ((unsigned long)addr & 2))
		if (straddles_word)
			stop_machine(patch_text_stop_machine, &patch, NULL);
		else
			__patch_text(addr, insn);
	}

> diff --git a/arch/arm/kernel/patch.h b/arch/arm/kernel/patch.h
> new file mode 100644
> index 0000000..b4731f2
> --- /dev/null
> +++ b/arch/arm/kernel/patch.h
> @@ -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

[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[Dave Martin]

On Mon, Nov 21, 2011 at 08:43:46PM +0530, Rabin Vincent wrote:
> 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 <rabin@rab.in>
> ---
>  arch/arm/kernel/ftrace.c |   29 ++++++++++++++++++-----------
>  1 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index c0062ad..cdceb63 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -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,
> @@ -125,11 +125,20 @@ static int ftrace_modify_code(unsigned long pc, unsigned long old,
>  {
>  	unsigned long replaced;
>  
> -	if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
> -		return -EFAULT;
> +#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.


> +	}
> +#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);
>  
>  #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);
>  
> -		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...)


Cheers
---Dave

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Dave Martin]

On Mon, Nov 21, 2011 at 08:43:47PM +0530, Rabin Vincent wrote:
> Extract out the instruction generation code so that it can be used
> for jump labels too.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  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
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile

[...]

>  static int ftrace_modify_code(unsigned long pc, unsigned long old,
> @@ -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.

Cheers
---Dave

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Rabin Vincent]

On Tue, Nov 22, 2011 at 17:32, Dave Martin <dave.martin@linaro.org> wrote:
> On Mon, Nov 21, 2011 at 08:43:47PM +0530, Rabin Vincent wrote:
>> ?static int ftrace_modify_code(unsigned long pc, unsigned long old,
>> @@ -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.

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Dave Martin]

On Tue, Nov 22, 2011 at 07:02:13PM +0530, Rabin Vincent wrote:
> On Tue, Nov 22, 2011 at 17:32, Dave Martin <dave.martin@linaro.org> wrote:
> > On Mon, Nov 21, 2011 at 08:43:47PM +0530, Rabin Vincent wrote:
> >> ?static int ftrace_modify_code(unsigned long pc, unsigned long old,
> >> @@ -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.

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?

Are there any particular problems blocking this?

Cheers
---Dave

[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[Rabin Vincent]

On Tue, Nov 22, 2011 at 17:32, Dave Martin <dave.martin@linaro.org> wrote:
> On Mon, Nov 21, 2011 at 08:43:46PM +0530, Rabin Vincent wrote:
>> +#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.

> 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 particular, we really want those macros to encapsulate the
> #ifdef __ARMEB__ stuff.

The ifdef for ARMEB is a bit jarring, especially since now we can even
get rid of ifdefs for CONFIG_THUMB2_KERNEL.  An arm_is_bigendian() or
somesuch macro in a header would be preferable for this, I think.

>
>
>> + ? ? }
>> +#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);
>>
>> ?#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);
>>
>> - ? ? ? ? ? ? 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?

The ftrace_modify_code() checks the location its modifying to ensure
that the old value was what it expected it to be -- this is useful for
example if we have problems with the recordmcount code that makes it
record wrong locations.

In this particular case however, what we're modifying is not code at an
mcount location but some of the code in entry-common.S which is at the
location of a known symbol.  The safety check for the old value was
quite useless in this case because we were just memcpy()ing the content
of the location and passing that in as the old value.

The reason it's done in this patch is that because of converting to the
canonical format, we'd need an additional ifdef-ARMEB/swahw32() here
after doing the memcpy(), for the memcmp() in ftrace_modify_code() to
succeed.  Instead, we just avoid the comparison for this case.

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

Agreed.  Perhaps it should even be done in a separate patch.

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

With this patch series:
 - kprobes and jump labels use the common text patching code
 - jump labels and ftrace use the common instruction generation code

kprobes and ftrace, however, don't share anything.  We could make ftrace
use __patch_text(), although that would only save half-a-dozen lines of
code and also remove the use of probe_kernel_write(), which all the
other arch ftrace implementations use.

[PATCH 3/4] ARM: extract out code patch function from kprobes

[Rabin Vincent]

On Tue, Nov 22, 2011 at 14:18, Tixy <tixy@yxit.co.uk> wrote:
> On Mon, 2011-11-21 at 20:43 +0530, Rabin Vincent wrote:
>> 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 <tixy@yxit.co.uk>
>> Signed-off-by: Rabin Vincent <rabin@rab.in>
>> ---
>
> Looks good. I've a couple of minor inline comments below.

Thank you for the review.  I'll address all your comments in v2.

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Rabin Vincent]

On Tue, Nov 22, 2011 at 19:26, Dave Martin <dave.martin@linaro.org> wrote:
> On Tue, Nov 22, 2011 at 07:02:13PM +0530, Rabin Vincent wrote:
>> 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.

[PATCH 4/4] ARM: add jump label support

[Russell King - ARM Linux]

On Mon, Nov 21, 2011 at 08:43:49PM +0530, Rabin Vincent wrote:
> 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 <jbaron@redhat.com>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

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

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

[PATCH 4/4] ARM: add jump label support

[Stephen Boyd]

On 11/22/11 11:42, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2011 at 08:43:49PM +0530, Rabin Vincent wrote:
>> 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 <jbaron@redhat.com>
>> Signed-off-by: Rabin Vincent <rabin@rab.in>
> 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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 4/4] ARM: add jump label support

[Jason Baron]

On Tue, Nov 22, 2011 at 03:02:31PM -0800, Stephen Boyd wrote:
> On 11/22/11 11:42, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2011 at 08:43:49PM +0530, Rabin Vincent wrote:
> >> 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 <jbaron@redhat.com>
> >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> > 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

[PATCH 4/4] ARM: add jump label support

[Jason Baron]

On Tue, Nov 22, 2011 at 06:39:50PM -0500, Jason Baron wrote:
> 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

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

-Jason

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Dave Martin]

On Tue, Nov 22, 2011 at 11:55:49PM +0530, Rabin Vincent wrote:
> On Tue, Nov 22, 2011 at 19:26, Dave Martin <dave.martin@linaro.org> wrote:
> > On Tue, Nov 22, 2011 at 07:02:13PM +0530, Rabin Vincent wrote:
> >> 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.

The unwind tables contain the information required to find the link
register.  This is how the backtracer works in Thumb-2 kernels or with
CONFIG_ARM_UNWIND.   This adds some complexity, but it also has some
advantages such as elimiating the runtime stack and instruction overhead
associated with maintaining a framepointer chain.

Really, "find/change frame's stored return address" is a generic
operation which is closely related to stack unwinding, and would best
be implemented along with the unwinder code.  Ideally, ftrace should
not need to know or care _how_ this is accomplished.  Indeed, at this
level the operation isn't even arch-specific.

I will take a look at the ARM unwind/backtrace code and think about
whether this could be factored out or exposed in a helpful way.
Certainly we shouldn't implement a second unwind table parser in
ftrace...

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

Cheers
---Dave

[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[Dave Martin]

On Tue, Nov 22, 2011 at 11:30:11PM +0530, Rabin Vincent wrote:
> On Tue, Nov 22, 2011 at 17:32, Dave Martin <dave.martin@linaro.org> wrote:
> > On Mon, Nov 21, 2011 at 08:43:46PM +0530, Rabin Vincent wrote:
> >> +#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.
> 
> > 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

Re: [PATCH 4/4] ARM: add jump label support

[Rabin Vincent]

On Tue, Nov 22, 2011 at 06:50:41PM -0500, Jason Baron wrote:
> On Tue, Nov 22, 2011 at 06:39:50PM -0500, Jason Baron wrote:
> > 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
> 
> 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...

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

8<----------------------------
>From ea211ce77e0ea9c5b8cd32b99bfb24e0140f0068 Mon Sep 17 00:00:00 2001
From: Rabin Vincent <rabin@rab.in>
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 <rabin@rab.in>
---
 scripts/gcc-goto.sh |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
index 98cffcb..9b744de 100644
--- a/scripts/gcc-goto.sh
+++ b/scripts/gcc-goto.sh
@@ -2,4 +2,20 @@
 # Test for gcc 'asm goto' support
 # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
 
-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).
+	 */
+	static struct { int dummy; int state; } tp;
+	asm ("@ %c0" :: "i" (&tp.state));
+#endif
+
+entry:
+	asm goto ("" :::: entry);
+	return 0;
+}
+END
-- 
1.7.7.3

[PATCH 4/4] ARM: add jump label support

[Rabin Vincent]

On Tue, Nov 22, 2011 at 06:50:41PM -0500, Jason Baron wrote:
> On Tue, Nov 22, 2011 at 06:39:50PM -0500, Jason Baron wrote:
> > 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
> 
> 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...

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

8<----------------------------
>From ea211ce77e0ea9c5b8cd32b99bfb24e0140f0068 Mon Sep 17 00:00:00 2001
From: Rabin Vincent <rabin@rab.in>
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 <rabin@rab.in>
---
 scripts/gcc-goto.sh |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
index 98cffcb..9b744de 100644
--- a/scripts/gcc-goto.sh
+++ b/scripts/gcc-goto.sh
@@ -2,4 +2,20 @@
 # Test for gcc 'asm goto' support
 # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
 
-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).
+	 */
+	static struct { int dummy; int state; } tp;
+	asm ("@ %c0" :: "i" (&tp.state));
+#endif
+
+entry:
+	asm goto ("" :::: entry);
+	return 0;
+}
+END
-- 
1.7.7.3

Re: [PATCH 4/4] ARM: add jump label support

[Russell King - ARM Linux]

On Wed, Nov 23, 2011 at 08:25:09PM +0530, Rabin Vincent wrote:
> Here's a patch.  Atleast on MIPS %c means something else so I had to
> ifdef it for ARM:
> 
> 8<----------------------------
> >From ea211ce77e0ea9c5b8cd32b99bfb24e0140f0068 Mon Sep 17 00:00:00 2001
> From: Rabin Vincent <rabin@rab.in>
> 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 <rabin@rab.in>
> ---
>  scripts/gcc-goto.sh |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
> index 98cffcb..9b744de 100644
> --- a/scripts/gcc-goto.sh
> +++ b/scripts/gcc-goto.sh
> @@ -2,4 +2,20 @@
>  # Test for gcc 'asm goto' support
>  # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
>  
> -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).
> +	 */
> +	static struct { int dummy; int state; } tp;
> +	asm ("@ %c0" :: "i" (&tp.state));
> +#endif
> +
> +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.

[PATCH 4/4] ARM: add jump label support

[Russell King - ARM Linux]

On Wed, Nov 23, 2011 at 08:25:09PM +0530, Rabin Vincent wrote:
> Here's a patch.  Atleast on MIPS %c means something else so I had to
> ifdef it for ARM:
> 
> 8<----------------------------
> >From ea211ce77e0ea9c5b8cd32b99bfb24e0140f0068 Mon Sep 17 00:00:00 2001
> From: Rabin Vincent <rabin@rab.in>
> 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 <rabin@rab.in>
> ---
>  scripts/gcc-goto.sh |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
> index 98cffcb..9b744de 100644
> --- a/scripts/gcc-goto.sh
> +++ b/scripts/gcc-goto.sh
> @@ -2,4 +2,20 @@
>  # Test for gcc 'asm goto' support
>  # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
>  
> -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).
> +	 */
> +	static struct { int dummy; int state; } tp;
> +	asm ("@ %c0" :: "i" (&tp.state));
> +#endif
> +
> +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.

[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[Bi Junxiao]

on 11/23/2011 10:42 PM Dave Martin wrote:
> On Tue, Nov 22, 2011 at 11:30:11PM +0530, Rabin Vincent wrote:
>    
>> On Tue, Nov 22, 2011 at 17:32, Dave Martin<dave.martin@linaro.org>  wrote:
>>      
>>> On Mon, Nov 21, 2011 at 08:43:46PM +0530, Rabin Vincent wrote:
>>>        
>>>> +#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.
>>
>>      
>>> 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.
>
>    
Yes, these macros are useful for be8 support. Do they deserve a 
separated header file as there is no appropriate one for them now?
> If this looks sensible, it would also make sense for Junxiao Bi to build
> his big-endian fixes on it.
>
> Cheers
> ---Dave
>
>    

[PATCH 2/4] ARM: extract out insn generation code from ftrace

[Rabin Vincent]

On Wed, Nov 23, 2011 at 11:50:12AM +0000, Dave Martin wrote:
> Are there any other dependencies on framepointers, or is that the only
> thing?

That's the only thing.

[PATCH 1/4] ARM: ftrace: use canonical Thumb-2 wide instruction format

[Dave Martin]

On Thu, Nov 24, 2011 at 03:22:34PM +0800, Bi Junxiao wrote:
> on 11/23/2011 10:42 PM Dave Martin wrote:
> >On Tue, Nov 22, 2011 at 11:30:11PM +0530, Rabin Vincent wrote:
> >>On Tue, Nov 22, 2011 at 17:32, Dave Martin<dave.martin@linaro.org>  wrote:
> >>>On Mon, Nov 21, 2011 at 08:43:46PM +0530, Rabin Vincent wrote:
> >>>>+#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.
> >>
> >>>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.
> >
> Yes, these macros are useful for be8 support. Do they deserve a
> separated header file as there is no appropriate one for them now?
> >If this looks sensible, it would also make sense for Junxiao Bi to build
> >his big-endian fixes on it.

I don't really see a header where this stuff fits.

I'll post an RFC.

Cheers
---Dave

Re: [PATCH 4/4] ARM: add jump label support

[Rabin Vincent]

On Wed, Nov 23, 2011 at 23:23, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Nov 23, 2011 at 08:25:09PM +0530, Rabin Vincent wrote:
>> +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
>> +
>> +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.

Why would we want to do it separately?

The only reason I see is because the results of this test are assigned
to a variable called CC_HAVE_ASM_GOTO and that is not an entirely
accurate name (on ARM) after the addition of this test.  If that's the
concern, perhaps this variable could just be renamed?

[PATCH 4/4] ARM: add jump label support

[Rabin Vincent]

On Wed, Nov 23, 2011 at 23:23, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Nov 23, 2011 at 08:25:09PM +0530, Rabin Vincent wrote:
>> +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
>> +
>> +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.

Why would we want to do it separately?

The only reason I see is because the results of this test are assigned
to a variable called CC_HAVE_ASM_GOTO and that is not an entirely
accurate name (on ARM) after the addition of this test.  If that's the
concern, perhaps this variable could just be renamed?

[PATCH 4/4] ARM: add jump label support

[Rabin Vincent]

On Tue, Nov 22, 2011 at 07:42:13PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2011 at 08:43:49PM +0530, Rabin Vincent wrote:
> > +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.

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.

[PATCH 4/4] ARM: add jump label support

[Russell King - ARM Linux]

On Tue, Jan 24, 2012 at 09:13:53PM +0530, Rabin Vincent wrote:
> On Tue, Nov 22, 2011 at 07:42:13PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2011 at 08:43:49PM +0530, Rabin Vincent wrote:
> > > +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.
> 
> 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?)

[PATCH 4/4] ARM: add jump label support

[Rabin Vincent]

On Tue, Jan 24, 2012 at 04:05:16PM +0000, Russell King - ARM Linux wrote:
> 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?)

I was thinking not of end user products but of distro kernels.  For
example Debian does have tracepoints built in into the standard kernels
(at least on x86). 

I will drop the "real nop" usage from v2 of this patchset as you
suggested since the power impact of these few nops is vanishingly
negligible.