All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] uprobes/core: Make instruction tables volatile.
@ 2012-02-22  9:15 Srikar Dronamraju
  2012-02-22  9:15 ` [PATCH 2/3] uprobes/core: remove uprobe_opcode_sz Srikar Dronamraju
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Srikar Dronamraju @ 2012-02-22  9:15 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Linus Torvalds, Oleg Nesterov, LKML,
	Christoph Hellwig, Steven Rostedt, Thomas Gleixner,
	Masami Hiramatsu, Anton Arapov, Ananth N Mavinakayanahalli,
	Jim Keniston, Jiri Olsa, Josh Stone

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Some versions of gcc spits a warning about the asm operand for test_bit and
also causes the first long of the instruction table to be output.

Fix is similar to 7115e3fc on arch/x86/kernel/kprobes.c

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/x86/kernel/uprobes.c |   61 ++++++++++++++++++++++++---------------------
 1 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index cf2a184..13d616d 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -53,34 +53,12 @@
 	  (bc##UL << 0xc)|(bd##UL << 0xd)|(be##UL << 0xe)|(bf##UL << 0xf))    \
 	 << (row % 32))
 
-#ifdef CONFIG_X86_64
-static u32 good_insns_64[256 / 32] = {
-	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
-	/*      ----------------------------------------------         */
-	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
-	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
-	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
-	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
-	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
-	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
-	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
-	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
-	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
-	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
-	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
-	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
-	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
-	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
-	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
-	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
-	/*      ----------------------------------------------         */
-	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
-};
-#endif
-
-/* Good-instruction tables for 32-bit apps */
-
-static u32 good_insns_32[256 / 32] = {
+/*
+ * Good-instruction tables for 32-bit apps.  This is non-const and volatile
+ * to keep gcc from statically optimizing it out, as variable_test_bit makes
+ * some versions of gcc to think only *(unsigned long*) is used.
+ */
+static volatile u32 good_insns_32[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0) | /* 00 */
@@ -104,7 +82,7 @@ static u32 good_insns_32[256 / 32] = {
 };
 
 /* Using this for both 64-bit and 32-bit apps */
-static u32 good_2byte_insns[256 / 32] = {
+static volatile u32 good_2byte_insns[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1) | /* 00 */
@@ -127,6 +105,31 @@ static u32 good_2byte_insns[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 };
 
+#ifdef CONFIG_X86_64
+/* Good-instruction tables for 64-bit apps */
+static volatile u32 good_insns_64[256 / 32] = {
+	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
+	/*      ----------------------------------------------         */
+	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
+	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
+	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
+	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
+	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
+	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
+	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
+	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
+	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
+	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
+	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
+	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
+	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
+	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
+	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
+	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
+	/*      ----------------------------------------------         */
+	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
+};
+#endif
 #undef W
 
 /*



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 2/3] uprobes/core: remove uprobe_opcode_sz
  2012-02-22  9:15 [PATCH 1/3] uprobes/core: Make instruction tables volatile Srikar Dronamraju
@ 2012-02-22  9:15 ` Srikar Dronamraju
  2012-02-22 16:05   ` [tip:perf/uprobes] uprobes/core: Remove uprobe_opcode_sz tip-bot for Srikar Dronamraju
  2012-02-22  9:16 ` [PATCH 3/3] uprobes/core: move insn to arch specific structure Srikar Dronamraju
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Srikar Dronamraju @ 2012-02-22  9:15 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Linus Torvalds, Oleg Nesterov, LKML,
	Christoph Hellwig, Steven Rostedt, Thomas Gleixner,
	Masami Hiramatsu, Anton Arapov, Ananth N Mavinakayanahalli,
	Jim Keniston, Jiri Olsa, Josh Stone

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

uprobe_opcode_sz refers to the smallest instruction size for that
architecture. UPROBES_BKPT_INSN_SIZE refers to the size of the breakpoint
instruction for that architecture.

For now we are assuming that both uprobe_opcode_sz and
UPROBES_BKPT_INSN_SIZE are the same for all archs and hence removing
uprobe_opcode_sz in favour of UPROBES_BKPT_INSN_SIZE.

However if we have to support architectures where the smallest instruction
size is different from the size of breakpoint instruction, we may have to
re-introduce uprobe_opcode_sz.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 include/linux/uprobes.h |    2 --
 kernel/uprobes.c        |    6 +++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 64e45f1..fd45b70 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -37,8 +37,6 @@ struct uprobe_arch_info {};
 #define MAX_UINSN_BYTES 4
 #endif
 
-#define uprobe_opcode_sz sizeof(uprobe_opcode_t)
-
 /* flags that denote/change uprobes behaviour */
 
 /* Have a copy of original instruction */
diff --git a/kernel/uprobes.c b/kernel/uprobes.c
index 884817f..ee496ad 100644
--- a/kernel/uprobes.c
+++ b/kernel/uprobes.c
@@ -244,8 +244,8 @@ static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
 
 	/* poke the new insn in, ASSUMES we don't cross page boundary */
 	vaddr &= ~PAGE_MASK;
-	BUG_ON(vaddr + uprobe_opcode_sz > PAGE_SIZE);
-	memcpy(vaddr_new + vaddr, &opcode, uprobe_opcode_sz);
+	BUG_ON(vaddr + UPROBES_BKPT_INSN_SIZE > PAGE_SIZE);
+	memcpy(vaddr_new + vaddr, &opcode, UPROBES_BKPT_INSN_SIZE);
 
 	kunmap_atomic(vaddr_new);
 	kunmap_atomic(vaddr_old);
@@ -293,7 +293,7 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
 	lock_page(page);
 	vaddr_new = kmap_atomic(page);
 	vaddr &= ~PAGE_MASK;
-	memcpy(opcode, vaddr_new + vaddr, uprobe_opcode_sz);
+	memcpy(opcode, vaddr_new + vaddr, UPROBES_BKPT_INSN_SIZE);
 	kunmap_atomic(vaddr_new);
 	unlock_page(page);
 



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 3/3] uprobes/core: move insn to arch specific structure.
  2012-02-22  9:15 [PATCH 1/3] uprobes/core: Make instruction tables volatile Srikar Dronamraju
  2012-02-22  9:15 ` [PATCH 2/3] uprobes/core: remove uprobe_opcode_sz Srikar Dronamraju
@ 2012-02-22  9:16 ` Srikar Dronamraju
  2012-02-22 16:06   ` [tip:perf/uprobes] uprobes/core: Move " tip-bot for Srikar Dronamraju
  2012-02-22  9:47 ` [PATCH 1/3] uprobes/core: Make instruction tables volatile Ingo Molnar
  2012-02-22 16:04 ` [tip:perf/uprobes] " tip-bot for Srikar Dronamraju
  3 siblings, 1 reply; 8+ messages in thread
From: Srikar Dronamraju @ 2012-02-22  9:16 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Linus Torvalds, Oleg Nesterov, LKML,
	Christoph Hellwig, Steven Rostedt, Thomas Gleixner,
	Masami Hiramatsu, Anton Arapov, Ananth N Mavinakayanahalli,
	Jim Keniston, Jiri Olsa, Josh Stone

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Few cleanups suggested by Ingo Molnar.

- Rename struct uprobe_arch_info to struct arch_uprobe.
- Move insn from struct uprobe to struct arch_uprobe.
- Make arch specific uprobe functions to accept struct arch_uprobe instead of
  struct uprobe.
- Move struct uprobe to kernel/uprobes.c from include/linux/uprobes.h

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/x86/include/asm/uprobes.h |    6 ++--
 arch/x86/kernel/uprobes.c      |   60 ++++++++++++++++++++--------------------
 include/linux/uprobes.h        |   23 +--------------
 kernel/uprobes.c               |   38 +++++++++++++++++--------
 4 files changed, 61 insertions(+), 66 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 072df39..f7ce310 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -31,13 +31,13 @@ typedef u8 uprobe_opcode_t;
 #define UPROBES_BKPT_INSN		0xcc
 #define UPROBES_BKPT_INSN_SIZE		   1
 
-struct uprobe_arch_info {
+struct arch_uprobe {
 	u16				fixups;
+	u8				insn[MAX_UINSN_BYTES];
 #ifdef CONFIG_X86_64
 	unsigned long			rip_rela_target_address;
 #endif
 };
 
-struct uprobe;
-extern int arch_uprobes_analyze_insn(struct mm_struct *mm, struct uprobe *uprobe);
+extern int arch_uprobes_analyze_insn(struct mm_struct *mm, struct arch_uprobe *arch_uprobe);
 #endif	/* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 13d616d..04dfcef 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -200,9 +200,9 @@ static bool is_prefix_bad(struct insn *insn)
 	return false;
 }
 
-static int validate_insn_32bits(struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_32bits(struct arch_uprobe *auprobe, struct insn *insn)
 {
-	insn_init(insn, uprobe->insn, false);
+	insn_init(insn, auprobe->insn, false);
 
 	/* Skip good instruction prefixes; reject "bad" ones. */
 	insn_get_opcode(insn);
@@ -222,11 +222,11 @@ static int validate_insn_32bits(struct uprobe *uprobe, struct insn *insn)
 
 /*
  * Figure out which fixups post_xol() will need to perform, and annotate
- * uprobe->arch_info.fixups accordingly.  To start with,
- * uprobe->arch_info.fixups is either zero or it reflects rip-related
+ * arch_uprobe->fixups accordingly.  To start with,
+ * arch_uprobe->fixups is either zero or it reflects rip-related
  * fixups.
  */
-static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
+static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
 {
 	bool fix_ip = true, fix_call = false;	/* defaults */
 	int reg;
@@ -269,17 +269,17 @@ static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
 		break;
 	}
 	if (fix_ip)
-		uprobe->arch_info.fixups |= UPROBES_FIX_IP;
+		auprobe->fixups |= UPROBES_FIX_IP;
 	if (fix_call)
-		uprobe->arch_info.fixups |= UPROBES_FIX_CALL;
+		auprobe->fixups |= UPROBES_FIX_CALL;
 }
 
 #ifdef CONFIG_X86_64
 /*
- * If uprobe->insn doesn't use rip-relative addressing, return
+ * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
  * its memory operand indirectly through a scratch register.  Set
- * uprobe->arch_info.fixups and uprobe->arch_info.rip_rela_target_address
+ * arch_uprobe->fixups and arch_uprobe->rip_rela_target_address
  * accordingly.  (The contents of the scratch register will be saved
  * before we single-step the modified instruction, and restored
  * afterward.)
@@ -297,7 +297,7 @@ static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
  *  - There's never a SIB byte.
  *  - The displacement is always 4 bytes.
  */
-static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static void handle_riprel_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
 	u8 *cursor;
 	u8 reg;
@@ -305,7 +305,7 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	if (mm->context.ia32_compat)
 		return;
 
-	uprobe->arch_info.rip_rela_target_address = 0x0;
+	auprobe->rip_rela_target_address = 0x0;
 	if (!insn_rip_relative(insn))
 		return;
 
@@ -315,7 +315,7 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	 * we want to encode rax/rcx, not r8/r9.
 	 */
 	if (insn->rex_prefix.nbytes) {
-		cursor = uprobe->insn + insn_offset_rex_prefix(insn);
+		cursor = auprobe->insn + insn_offset_rex_prefix(insn);
 		*cursor &= 0xfe;	/* Clearing REX.B bit */
 	}
 
@@ -324,7 +324,7 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	 * displacement.  Beyond the displacement, for some instructions,
 	 * is the immediate operand.
 	 */
-	cursor = uprobe->insn + insn_offset_modrm(insn);
+	cursor = auprobe->insn + insn_offset_modrm(insn);
 	insn_get_length(insn);
 
 	/*
@@ -341,18 +341,18 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 		 * is NOT the register operand, so we use %rcx (register
 		 * #1) for the scratch register.
 		 */
-		uprobe->arch_info.fixups = UPROBES_FIX_RIP_CX;
+		auprobe->fixups = UPROBES_FIX_RIP_CX;
 		/* Change modrm from 00 000 101 to 00 000 001. */
 		*cursor = 0x1;
 	} else {
 		/* Use %rax (register #0) for the scratch register. */
-		uprobe->arch_info.fixups = UPROBES_FIX_RIP_AX;
+		auprobe->fixups = UPROBES_FIX_RIP_AX;
 		/* Change modrm from 00 xxx 101 to 00 xxx 000 */
 		*cursor = (reg << 3);
 	}
 
 	/* Target address = address of next instruction + (signed) offset */
-	uprobe->arch_info.rip_rela_target_address = (long)insn->length + insn->displacement.value;
+	auprobe->rip_rela_target_address = (long)insn->length + insn->displacement.value;
 
 	/* Displacement field is gone; slide immediate field (if any) over. */
 	if (insn->immediate.nbytes) {
@@ -362,9 +362,9 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	return;
 }
 
-static int validate_insn_64bits(struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_64bits(struct arch_uprobe *auprobe, struct insn *insn)
 {
-	insn_init(insn, uprobe->insn, true);
+	insn_init(insn, auprobe->insn, true);
 
 	/* Skip good instruction prefixes; reject "bad" ones. */
 	insn_get_opcode(insn);
@@ -381,42 +381,42 @@ static int validate_insn_64bits(struct uprobe *uprobe, struct insn *insn)
 	return -ENOTSUPP;
 }
 
-static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_bits(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
 	if (mm->context.ia32_compat)
-		return validate_insn_32bits(uprobe, insn);
-	return validate_insn_64bits(uprobe, insn);
+		return validate_insn_32bits(auprobe, insn);
+	return validate_insn_64bits(auprobe, insn);
 }
 #else /* 32-bit: */
-static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static void handle_riprel_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
 	/* No RIP-relative addressing on 32-bit */
 }
 
-static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_bits(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
-	return validate_insn_32bits(uprobe, insn);
+	return validate_insn_32bits(auprobe, insn);
 }
 #endif /* CONFIG_X86_64 */
 
 /**
  * arch_uprobes_analyze_insn - instruction analysis including validity and fixups.
  * @mm: the probed address space.
- * @uprobe: the probepoint information.
+ * @arch_uprobe: the probepoint information.
  * Return 0 on success or a -ve number on error.
  */
-int arch_uprobes_analyze_insn(struct mm_struct *mm, struct uprobe *uprobe)
+int arch_uprobes_analyze_insn(struct mm_struct *mm, struct arch_uprobe *auprobe)
 {
 	int ret;
 	struct insn insn;
 
-	uprobe->arch_info.fixups = 0;
-	ret = validate_insn_bits(mm, uprobe, &insn);
+	auprobe->fixups = 0;
+	ret = validate_insn_bits(mm, auprobe, &insn);
 	if (ret != 0)
 		return ret;
 
-	handle_riprel_insn(mm, uprobe, &insn);
-	prepare_fixups(uprobe, &insn);
+	handle_riprel_insn(mm, auprobe, &insn);
+	prepare_fixups(auprobe, &insn);
 
 	return 0;
 }
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index fd45b70..9c6be62 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -29,12 +29,6 @@
 struct vm_area_struct;
 #ifdef CONFIG_ARCH_SUPPORTS_UPROBES
 #include <asm/uprobes.h>
-#else
-
-typedef u8 uprobe_opcode_t;
-struct uprobe_arch_info {};
-
-#define MAX_UINSN_BYTES 4
 #endif
 
 /* flags that denote/change uprobes behaviour */
@@ -56,22 +50,9 @@ struct uprobe_consumer {
 	struct uprobe_consumer *next;
 };
 
-struct uprobe {
-	struct rb_node		rb_node;	/* node in the rb tree */
-	atomic_t		ref;
-	struct rw_semaphore	consumer_rwsem;
-	struct list_head	pending_list;
-	struct uprobe_arch_info arch_info;
-	struct uprobe_consumer	*consumers;
-	struct inode		*inode;		/* Also hold a ref to inode */
-	loff_t			offset;
-	int			flags;
-	u8			insn[MAX_UINSN_BYTES];
-};
-
 #ifdef CONFIG_UPROBES
-extern int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr);
-extern int __weak set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, bool verify);
+extern int __weak set_bkpt(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr);
+extern int __weak set_orig_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr, bool verify);
 extern bool __weak is_bkpt_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer);
diff --git a/kernel/uprobes.c b/kernel/uprobes.c
index ee496ad..0867302 100644
--- a/kernel/uprobes.c
+++ b/kernel/uprobes.c
@@ -65,6 +65,18 @@ struct vma_info {
 	loff_t			vaddr;
 };
 
+struct uprobe {
+	struct rb_node		rb_node;	/* node in the rb tree */
+	atomic_t		ref;
+	struct rw_semaphore	consumer_rwsem;
+	struct list_head	pending_list;
+	struct uprobe_consumer	*consumers;
+	struct inode		*inode;		/* Also hold a ref to inode */
+	loff_t			offset;
+	int			flags;
+	struct arch_uprobe 	arch;
+};
+
 /*
  * valid_vma: Verify if the specified vma is an executable vma
  * Relax restrictions while unregistering: vm_flags might have
@@ -180,7 +192,7 @@ bool __weak is_bkpt_insn(uprobe_opcode_t *insn)
 /*
  * write_opcode - write the opcode at a given virtual address.
  * @mm: the probed process address space.
- * @uprobe: the breakpointing information.
+ * @arch_uprobe: the breakpointing information.
  * @vaddr: the virtual address to store the opcode.
  * @opcode: opcode to be written at @vaddr.
  *
@@ -190,13 +202,14 @@ bool __weak is_bkpt_insn(uprobe_opcode_t *insn)
  * For mm @mm, write the opcode at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
+static int write_opcode(struct mm_struct *mm, struct arch_uprobe *auprobe,
 			unsigned long vaddr, uprobe_opcode_t opcode)
 {
 	struct page *old_page, *new_page;
 	struct address_space *mapping;
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
+	struct uprobe *uprobe;
 	loff_t addr;
 	int ret;
 
@@ -216,6 +229,7 @@ static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
 	if (!valid_vma(vma, is_bkpt_insn(&opcode)))
 		goto put_out;
 
+	uprobe = container_of(auprobe, struct uprobe, arch);
 	mapping = uprobe->inode->i_mapping;
 	if (mapping != vma->vm_file->f_mapping)
 		goto put_out;
@@ -326,7 +340,7 @@ static int is_bkpt_at_addr(struct mm_struct *mm, unsigned long vaddr)
  * For mm @mm, store the breakpoint instruction at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr)
+int __weak set_bkpt(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr)
 {
 	int result;
 
@@ -337,7 +351,7 @@ int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long v
 	if (result)
 		return result;
 
-	return write_opcode(mm, uprobe, vaddr, UPROBES_BKPT_INSN);
+	return write_opcode(mm, auprobe, vaddr, UPROBES_BKPT_INSN);
 }
 
 /**
@@ -351,7 +365,7 @@ int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long v
  * Return 0 (success) or a negative errno.
  */
 int __weak
-set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, bool verify)
+set_orig_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr, bool verify)
 {
 	if (verify) {
 		int result;
@@ -363,7 +377,7 @@ set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, 
 		if (result != 1)
 			return result;
 	}
-	return write_opcode(mm, uprobe, vaddr, *(uprobe_opcode_t *)uprobe->insn);
+	return write_opcode(mm, auprobe, vaddr, *(uprobe_opcode_t *)auprobe->insn);
 }
 
 static int match_uprobe(struct uprobe *l, struct uprobe *r)
@@ -593,13 +607,13 @@ static int copy_insn(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned
 
 	/* Instruction at the page-boundary; copy bytes in second page */
 	if (nbytes < bytes) {
-		if (__copy_insn(mapping, vma, uprobe->insn + nbytes,
+		if (__copy_insn(mapping, vma, uprobe->arch.insn + nbytes,
 				bytes - nbytes, uprobe->offset + nbytes))
 			return -ENOMEM;
 
 		bytes = nbytes;
 	}
-	return __copy_insn(mapping, vma, uprobe->insn, bytes, uprobe->offset);
+	return __copy_insn(mapping, vma, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
 static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
@@ -625,23 +639,23 @@ static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
 		if (ret)
 			return ret;
 
-		if (is_bkpt_insn((uprobe_opcode_t *)uprobe->insn))
+		if (is_bkpt_insn((uprobe_opcode_t *)uprobe->arch.insn))
 			return -EEXIST;
 
-		ret = arch_uprobes_analyze_insn(mm, uprobe);
+		ret = arch_uprobes_analyze_insn(mm, &uprobe->arch);
 		if (ret)
 			return ret;
 
 		uprobe->flags |= UPROBES_COPY_INSN;
 	}
-	ret = set_bkpt(mm, uprobe, addr);
+	ret = set_bkpt(mm, &uprobe->arch, addr);
 
 	return ret;
 }
 
 static void remove_breakpoint(struct mm_struct *mm, struct uprobe *uprobe, loff_t vaddr)
 {
-	set_orig_insn(mm, uprobe, (unsigned long)vaddr, true);
+	set_orig_insn(mm, &uprobe->arch, (unsigned long)vaddr, true);
 }
 
 static void delete_uprobe(struct uprobe *uprobe)



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] uprobes/core: Make instruction tables volatile.
  2012-02-22  9:15 [PATCH 1/3] uprobes/core: Make instruction tables volatile Srikar Dronamraju
  2012-02-22  9:15 ` [PATCH 2/3] uprobes/core: remove uprobe_opcode_sz Srikar Dronamraju
  2012-02-22  9:16 ` [PATCH 3/3] uprobes/core: move insn to arch specific structure Srikar Dronamraju
@ 2012-02-22  9:47 ` Ingo Molnar
  2012-02-22 10:04   ` Srikar Dronamraju
  2012-02-22 16:04 ` [tip:perf/uprobes] " tip-bot for Srikar Dronamraju
  3 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2012-02-22  9:47 UTC (permalink / raw)
  To: Srikar Dronamraju
  Cc: Peter Zijlstra, Linus Torvalds, Oleg Nesterov, LKML,
	Christoph Hellwig, Steven Rostedt, Thomas Gleixner,
	Masami Hiramatsu, Anton Arapov, Ananth N Mavinakayanahalli,
	Jim Keniston, Jiri Olsa, Josh Stone


* Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Some versions of gcc spits a warning about the asm operand for test_bit and
> also causes the first long of the instruction table to be output.
> 
> Fix is similar to 7115e3fc on arch/x86/kernel/kprobes.c
> 
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/x86/kernel/uprobes.c |   61 ++++++++++++++++++++++++---------------------
>  1 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index cf2a184..13d616d 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -53,34 +53,12 @@
>  	  (bc##UL << 0xc)|(bd##UL << 0xd)|(be##UL << 0xe)|(bf##UL << 0xf))    \
>  	 << (row % 32))
>  
> -#ifdef CONFIG_X86_64
> -static u32 good_insns_64[256 / 32] = {
> -	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> -	/*      ----------------------------------------------         */
> -	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
> -	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
> -	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
> -	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
> -	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
> -	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
> -	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
> -	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
> -	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
> -	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
> -	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
> -	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
> -	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
> -	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
> -	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
> -	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
> -	/*      ----------------------------------------------         */
> -	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> -};
> -#endif
> -
> -/* Good-instruction tables for 32-bit apps */
> -
> -static u32 good_insns_32[256 / 32] = {
> +/*
> + * Good-instruction tables for 32-bit apps.  This is non-const and volatile
> + * to keep gcc from statically optimizing it out, as variable_test_bit makes
> + * some versions of gcc to think only *(unsigned long*) is used.
> + */
> +static volatile u32 good_insns_32[256 / 32] = {
>  	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
>  	/*      ----------------------------------------------         */
>  	W(0x00, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0) | /* 00 */
> @@ -104,7 +82,7 @@ static u32 good_insns_32[256 / 32] = {
>  };
>  
>  /* Using this for both 64-bit and 32-bit apps */
> -static u32 good_2byte_insns[256 / 32] = {
> +static volatile u32 good_2byte_insns[256 / 32] = {
>  	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
>  	/*      ----------------------------------------------         */
>  	W(0x00, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1) | /* 00 */
> @@ -127,6 +105,31 @@ static u32 good_2byte_insns[256 / 32] = {
>  	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
>  };
>  
> +#ifdef CONFIG_X86_64
> +/* Good-instruction tables for 64-bit apps */
> +static volatile u32 good_insns_64[256 / 32] = {
> +	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> +	/*      ----------------------------------------------         */
> +	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
> +	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
> +	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
> +	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
> +	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
> +	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
> +	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
> +	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
> +	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
> +	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
> +	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
> +	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
> +	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
> +	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
> +	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
> +	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
> +	/*      ----------------------------------------------         */
> +	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> +};
> +#endif

Hm, why was the table moved around? This makes it hard to see 
whether it's just a single volatile that is added, or something 
more.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] uprobes/core: Make instruction tables volatile.
  2012-02-22  9:47 ` [PATCH 1/3] uprobes/core: Make instruction tables volatile Ingo Molnar
@ 2012-02-22 10:04   ` Srikar Dronamraju
  0 siblings, 0 replies; 8+ messages in thread
From: Srikar Dronamraju @ 2012-02-22 10:04 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Linus Torvalds, Oleg Nesterov, LKML,
	Christoph Hellwig, Steven Rostedt, Thomas Gleixner,
	Masami Hiramatsu, Anton Arapov, Ananth N Mavinakayanahalli,
	Jim Keniston, Jiri Olsa, Josh Stone

> * Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
> 
> > From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > 
> > Some versions of gcc spits a warning about the asm operand for test_bit and
> > also causes the first long of the instruction table to be output.
> > 
> > Fix is similar to 7115e3fc on arch/x86/kernel/kprobes.c
> > 
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/x86/kernel/uprobes.c |   61 ++++++++++++++++++++++++---------------------
> >  1 files changed, 32 insertions(+), 29 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index cf2a184..13d616d 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -53,34 +53,12 @@
> >  	  (bc##UL << 0xc)|(bd##UL << 0xd)|(be##UL << 0xe)|(bf##UL << 0xf))    \
> >  	 << (row % 32))
> >  
> > -#ifdef CONFIG_X86_64
> > -static u32 good_insns_64[256 / 32] = {
> > -	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> > -	/*      ----------------------------------------------         */
> > -	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
> > -	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
> > -	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
> > -	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
> > -	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
> > -	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
> > -	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
> > -	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
> > -	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
> > -	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
> > -	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
> > -	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
> > -	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
> > -	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
> > -	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
> > -	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
> > -	/*      ----------------------------------------------         */
> > -	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> > -};
> > -#endif
> > -
> > -/* Good-instruction tables for 32-bit apps */
> > -
> > -static u32 good_insns_32[256 / 32] = {
> > +/*
> > + * Good-instruction tables for 32-bit apps.  This is non-const and volatile
> > + * to keep gcc from statically optimizing it out, as variable_test_bit makes
> > + * some versions of gcc to think only *(unsigned long*) is used.
> > + */
> > +static volatile u32 good_insns_32[256 / 32] = {
> >  	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> >  	/*      ----------------------------------------------         */
> >  	W(0x00, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0) | /* 00 */
> > @@ -104,7 +82,7 @@ static u32 good_insns_32[256 / 32] = {
> >  };
> >  
> >  /* Using this for both 64-bit and 32-bit apps */
> > -static u32 good_2byte_insns[256 / 32] = {
> > +static volatile u32 good_2byte_insns[256 / 32] = {
> >  	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> >  	/*      ----------------------------------------------         */
> >  	W(0x00, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1) | /* 00 */
> > @@ -127,6 +105,31 @@ static u32 good_2byte_insns[256 / 32] = {
> >  	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> >  };
> >  
> > +#ifdef CONFIG_X86_64
> > +/* Good-instruction tables for 64-bit apps */
> > +static volatile u32 good_insns_64[256 / 32] = {
> > +	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> > +	/*      ----------------------------------------------         */
> > +	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
> > +	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
> > +	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
> > +	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
> > +	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
> > +	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
> > +	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
> > +	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
> > +	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
> > +	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
> > +	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
> > +	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
> > +	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
> > +	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
> > +	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
> > +	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
> > +	/*      ----------------------------------------------         */
> > +	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
> > +};
> > +#endif
> 
> Hm, why was the table moved around? This makes it hard to see 
> whether it's just a single volatile that is added, or something 
> more.
> 

Without the table moving, the volatile was first used under #ifdef
CONFIG_X86_64. I was wondering if I add a comment in #ifdef people
looking at it might assume that its a problem only on x86_64 only.

So I moved the CONFIG_X86_64 specific table down and added the comment
for volatile at the first instance  its used.

-- 
Thanks and Regards
Srikar


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [tip:perf/uprobes] uprobes/core: Make instruction tables volatile
  2012-02-22  9:15 [PATCH 1/3] uprobes/core: Make instruction tables volatile Srikar Dronamraju
                   ` (2 preceding siblings ...)
  2012-02-22  9:47 ` [PATCH 1/3] uprobes/core: Make instruction tables volatile Ingo Molnar
@ 2012-02-22 16:04 ` tip-bot for Srikar Dronamraju
  3 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Srikar Dronamraju @ 2012-02-22 16:04 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, torvalds, peterz, jolsa, anton, jkenisto, rostedt, tglx,
	oleg, hpa, linux-kernel, hch, ananth, jistone,
	masami.hiramatsu.pt, srikar, mingo

Commit-ID:  04a3d984d32e47983770d314cdb4e4d8f38fccb7
Gitweb:     http://git.kernel.org/tip/04a3d984d32e47983770d314cdb4e4d8f38fccb7
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Wed, 22 Feb 2012 14:45:35 +0530
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 22 Feb 2012 11:26:07 +0100

uprobes/core: Make instruction tables volatile

Some versions of gcc spits a warning about the asm operand for
test_bit and also causes the first long of the instruction table
to be output.

Fix is similar to 7115e3fc on arch/x86/kernel/kprobes.c

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Josh Stone <jistone@redhat.com>
Link: http://lkml.kernel.org/r/20120222091535.15880.12502.sendpatchset@srdronam.in.ibm.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/uprobes.c |   61 +++++++++++++++++++++++---------------------
 1 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index cf2a184..13d616d 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -53,34 +53,12 @@
 	  (bc##UL << 0xc)|(bd##UL << 0xd)|(be##UL << 0xe)|(bf##UL << 0xf))    \
 	 << (row % 32))
 
-#ifdef CONFIG_X86_64
-static u32 good_insns_64[256 / 32] = {
-	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
-	/*      ----------------------------------------------         */
-	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
-	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
-	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
-	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
-	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
-	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
-	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
-	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
-	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
-	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
-	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
-	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
-	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
-	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
-	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
-	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
-	/*      ----------------------------------------------         */
-	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
-};
-#endif
-
-/* Good-instruction tables for 32-bit apps */
-
-static u32 good_insns_32[256 / 32] = {
+/*
+ * Good-instruction tables for 32-bit apps.  This is non-const and volatile
+ * to keep gcc from statically optimizing it out, as variable_test_bit makes
+ * some versions of gcc to think only *(unsigned long*) is used.
+ */
+static volatile u32 good_insns_32[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0) | /* 00 */
@@ -104,7 +82,7 @@ static u32 good_insns_32[256 / 32] = {
 };
 
 /* Using this for both 64-bit and 32-bit apps */
-static u32 good_2byte_insns[256 / 32] = {
+static volatile u32 good_2byte_insns[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1) | /* 00 */
@@ -127,6 +105,31 @@ static u32 good_2byte_insns[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 };
 
+#ifdef CONFIG_X86_64
+/* Good-instruction tables for 64-bit apps */
+static volatile u32 good_insns_64[256 / 32] = {
+	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
+	/*      ----------------------------------------------         */
+	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
+	W(0x10, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 10 */
+	W(0x20, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 20 */
+	W(0x30, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) , /* 30 */
+	W(0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | /* 40 */
+	W(0x50, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 50 */
+	W(0x60, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* 60 */
+	W(0x70, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 70 */
+	W(0x80, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* 80 */
+	W(0x90, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* 90 */
+	W(0xa0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) | /* a0 */
+	W(0xb0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* b0 */
+	W(0xc0, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0) | /* c0 */
+	W(0xd0, 1, 1, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1) , /* d0 */
+	W(0xe0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0) | /* e0 */
+	W(0xf0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1)   /* f0 */
+	/*      ----------------------------------------------         */
+	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
+};
+#endif
 #undef W
 
 /*

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [tip:perf/uprobes] uprobes/core: Remove uprobe_opcode_sz
  2012-02-22  9:15 ` [PATCH 2/3] uprobes/core: remove uprobe_opcode_sz Srikar Dronamraju
@ 2012-02-22 16:05   ` tip-bot for Srikar Dronamraju
  0 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Srikar Dronamraju @ 2012-02-22 16:05 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, torvalds, peterz, jolsa, anton, jkenisto, rostedt, tglx,
	oleg, hpa, linux-kernel, hch, ananth, jistone,
	masami.hiramatsu.pt, srikar, mingo

Commit-ID:  96379f60075c75b261328aa7830ef8aa158247ac
Gitweb:     http://git.kernel.org/tip/96379f60075c75b261328aa7830ef8aa158247ac
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Wed, 22 Feb 2012 14:45:49 +0530
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 22 Feb 2012 11:26:08 +0100

uprobes/core: Remove uprobe_opcode_sz

uprobe_opcode_sz refers to the smallest instruction size for
that architecture. UPROBES_BKPT_INSN_SIZE refers to the size of
the breakpoint instruction for that architecture.

For now we are assuming that both uprobe_opcode_sz and
UPROBES_BKPT_INSN_SIZE are the same for all archs and hence
removing uprobe_opcode_sz in favour of UPROBES_BKPT_INSN_SIZE.

However if we have to support architectures where the smallest
instruction size is different from the size of breakpoint
instruction, we may have to re-introduce uprobe_opcode_sz.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Josh Stone <jistone@redhat.com>
Link: http://lkml.kernel.org/r/20120222091549.15880.67020.sendpatchset@srdronam.in.ibm.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/uprobes.h |    2 --
 kernel/events/uprobes.c |    6 +++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 64e45f1..fd45b70 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -37,8 +37,6 @@ struct uprobe_arch_info {};
 #define MAX_UINSN_BYTES 4
 #endif
 
-#define uprobe_opcode_sz sizeof(uprobe_opcode_t)
-
 /* flags that denote/change uprobes behaviour */
 
 /* Have a copy of original instruction */
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 884817f..ee496ad 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -244,8 +244,8 @@ static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
 
 	/* poke the new insn in, ASSUMES we don't cross page boundary */
 	vaddr &= ~PAGE_MASK;
-	BUG_ON(vaddr + uprobe_opcode_sz > PAGE_SIZE);
-	memcpy(vaddr_new + vaddr, &opcode, uprobe_opcode_sz);
+	BUG_ON(vaddr + UPROBES_BKPT_INSN_SIZE > PAGE_SIZE);
+	memcpy(vaddr_new + vaddr, &opcode, UPROBES_BKPT_INSN_SIZE);
 
 	kunmap_atomic(vaddr_new);
 	kunmap_atomic(vaddr_old);
@@ -293,7 +293,7 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
 	lock_page(page);
 	vaddr_new = kmap_atomic(page);
 	vaddr &= ~PAGE_MASK;
-	memcpy(opcode, vaddr_new + vaddr, uprobe_opcode_sz);
+	memcpy(opcode, vaddr_new + vaddr, UPROBES_BKPT_INSN_SIZE);
 	kunmap_atomic(vaddr_new);
 	unlock_page(page);
 

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [tip:perf/uprobes] uprobes/core: Move insn to arch specific structure
  2012-02-22  9:16 ` [PATCH 3/3] uprobes/core: move insn to arch specific structure Srikar Dronamraju
@ 2012-02-22 16:06   ` tip-bot for Srikar Dronamraju
  0 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Srikar Dronamraju @ 2012-02-22 16:06 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, torvalds, peterz, jolsa, anton, jkenisto, rostedt, tglx,
	oleg, hpa, linux-kernel, hch, ananth, jistone,
	masami.hiramatsu.pt, srikar, mingo

Commit-ID:  3ff54efdfaace9e9b2b7c1959a865be6b91de96c
Gitweb:     http://git.kernel.org/tip/3ff54efdfaace9e9b2b7c1959a865be6b91de96c
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Wed, 22 Feb 2012 14:46:02 +0530
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 22 Feb 2012 11:26:09 +0100

uprobes/core: Move insn to arch specific structure

Few cleanups suggested by Ingo Molnar.

- Rename struct uprobe_arch_info to struct arch_uprobe.
- Move insn from struct uprobe to struct arch_uprobe.
- Make arch specific uprobe functions to accept struct arch_uprobe
  instead of  struct uprobe.
- Move struct uprobe to kernel/uprobes.c from include/linux/uprobes.h

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Josh Stone <jistone@redhat.com>
Link: http://lkml.kernel.org/r/20120222091602.15880.40249.sendpatchset@srdronam.in.ibm.com
[ Made various small improvements ]
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/uprobes.h |    6 ++--
 arch/x86/kernel/uprobes.c      |   60 ++++++++++++++++++++--------------------
 include/linux/uprobes.h        |   23 +--------------
 kernel/events/uprobes.c        |   38 +++++++++++++++++--------
 4 files changed, 61 insertions(+), 66 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 072df39..f7ce310 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -31,13 +31,13 @@ typedef u8 uprobe_opcode_t;
 #define UPROBES_BKPT_INSN		0xcc
 #define UPROBES_BKPT_INSN_SIZE		   1
 
-struct uprobe_arch_info {
+struct arch_uprobe {
 	u16				fixups;
+	u8				insn[MAX_UINSN_BYTES];
 #ifdef CONFIG_X86_64
 	unsigned long			rip_rela_target_address;
 #endif
 };
 
-struct uprobe;
-extern int arch_uprobes_analyze_insn(struct mm_struct *mm, struct uprobe *uprobe);
+extern int arch_uprobes_analyze_insn(struct mm_struct *mm, struct arch_uprobe *arch_uprobe);
 #endif	/* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 13d616d..04dfcef 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -200,9 +200,9 @@ static bool is_prefix_bad(struct insn *insn)
 	return false;
 }
 
-static int validate_insn_32bits(struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_32bits(struct arch_uprobe *auprobe, struct insn *insn)
 {
-	insn_init(insn, uprobe->insn, false);
+	insn_init(insn, auprobe->insn, false);
 
 	/* Skip good instruction prefixes; reject "bad" ones. */
 	insn_get_opcode(insn);
@@ -222,11 +222,11 @@ static int validate_insn_32bits(struct uprobe *uprobe, struct insn *insn)
 
 /*
  * Figure out which fixups post_xol() will need to perform, and annotate
- * uprobe->arch_info.fixups accordingly.  To start with,
- * uprobe->arch_info.fixups is either zero or it reflects rip-related
+ * arch_uprobe->fixups accordingly.  To start with,
+ * arch_uprobe->fixups is either zero or it reflects rip-related
  * fixups.
  */
-static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
+static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
 {
 	bool fix_ip = true, fix_call = false;	/* defaults */
 	int reg;
@@ -269,17 +269,17 @@ static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
 		break;
 	}
 	if (fix_ip)
-		uprobe->arch_info.fixups |= UPROBES_FIX_IP;
+		auprobe->fixups |= UPROBES_FIX_IP;
 	if (fix_call)
-		uprobe->arch_info.fixups |= UPROBES_FIX_CALL;
+		auprobe->fixups |= UPROBES_FIX_CALL;
 }
 
 #ifdef CONFIG_X86_64
 /*
- * If uprobe->insn doesn't use rip-relative addressing, return
+ * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
  * its memory operand indirectly through a scratch register.  Set
- * uprobe->arch_info.fixups and uprobe->arch_info.rip_rela_target_address
+ * arch_uprobe->fixups and arch_uprobe->rip_rela_target_address
  * accordingly.  (The contents of the scratch register will be saved
  * before we single-step the modified instruction, and restored
  * afterward.)
@@ -297,7 +297,7 @@ static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
  *  - There's never a SIB byte.
  *  - The displacement is always 4 bytes.
  */
-static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static void handle_riprel_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
 	u8 *cursor;
 	u8 reg;
@@ -305,7 +305,7 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	if (mm->context.ia32_compat)
 		return;
 
-	uprobe->arch_info.rip_rela_target_address = 0x0;
+	auprobe->rip_rela_target_address = 0x0;
 	if (!insn_rip_relative(insn))
 		return;
 
@@ -315,7 +315,7 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	 * we want to encode rax/rcx, not r8/r9.
 	 */
 	if (insn->rex_prefix.nbytes) {
-		cursor = uprobe->insn + insn_offset_rex_prefix(insn);
+		cursor = auprobe->insn + insn_offset_rex_prefix(insn);
 		*cursor &= 0xfe;	/* Clearing REX.B bit */
 	}
 
@@ -324,7 +324,7 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	 * displacement.  Beyond the displacement, for some instructions,
 	 * is the immediate operand.
 	 */
-	cursor = uprobe->insn + insn_offset_modrm(insn);
+	cursor = auprobe->insn + insn_offset_modrm(insn);
 	insn_get_length(insn);
 
 	/*
@@ -341,18 +341,18 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 		 * is NOT the register operand, so we use %rcx (register
 		 * #1) for the scratch register.
 		 */
-		uprobe->arch_info.fixups = UPROBES_FIX_RIP_CX;
+		auprobe->fixups = UPROBES_FIX_RIP_CX;
 		/* Change modrm from 00 000 101 to 00 000 001. */
 		*cursor = 0x1;
 	} else {
 		/* Use %rax (register #0) for the scratch register. */
-		uprobe->arch_info.fixups = UPROBES_FIX_RIP_AX;
+		auprobe->fixups = UPROBES_FIX_RIP_AX;
 		/* Change modrm from 00 xxx 101 to 00 xxx 000 */
 		*cursor = (reg << 3);
 	}
 
 	/* Target address = address of next instruction + (signed) offset */
-	uprobe->arch_info.rip_rela_target_address = (long)insn->length + insn->displacement.value;
+	auprobe->rip_rela_target_address = (long)insn->length + insn->displacement.value;
 
 	/* Displacement field is gone; slide immediate field (if any) over. */
 	if (insn->immediate.nbytes) {
@@ -362,9 +362,9 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, stru
 	return;
 }
 
-static int validate_insn_64bits(struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_64bits(struct arch_uprobe *auprobe, struct insn *insn)
 {
-	insn_init(insn, uprobe->insn, true);
+	insn_init(insn, auprobe->insn, true);
 
 	/* Skip good instruction prefixes; reject "bad" ones. */
 	insn_get_opcode(insn);
@@ -381,42 +381,42 @@ static int validate_insn_64bits(struct uprobe *uprobe, struct insn *insn)
 	return -ENOTSUPP;
 }
 
-static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_bits(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
 	if (mm->context.ia32_compat)
-		return validate_insn_32bits(uprobe, insn);
-	return validate_insn_64bits(uprobe, insn);
+		return validate_insn_32bits(auprobe, insn);
+	return validate_insn_64bits(auprobe, insn);
 }
 #else /* 32-bit: */
-static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static void handle_riprel_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
 	/* No RIP-relative addressing on 32-bit */
 }
 
-static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
+static int validate_insn_bits(struct mm_struct *mm, struct arch_uprobe *auprobe, struct insn *insn)
 {
-	return validate_insn_32bits(uprobe, insn);
+	return validate_insn_32bits(auprobe, insn);
 }
 #endif /* CONFIG_X86_64 */
 
 /**
  * arch_uprobes_analyze_insn - instruction analysis including validity and fixups.
  * @mm: the probed address space.
- * @uprobe: the probepoint information.
+ * @arch_uprobe: the probepoint information.
  * Return 0 on success or a -ve number on error.
  */
-int arch_uprobes_analyze_insn(struct mm_struct *mm, struct uprobe *uprobe)
+int arch_uprobes_analyze_insn(struct mm_struct *mm, struct arch_uprobe *auprobe)
 {
 	int ret;
 	struct insn insn;
 
-	uprobe->arch_info.fixups = 0;
-	ret = validate_insn_bits(mm, uprobe, &insn);
+	auprobe->fixups = 0;
+	ret = validate_insn_bits(mm, auprobe, &insn);
 	if (ret != 0)
 		return ret;
 
-	handle_riprel_insn(mm, uprobe, &insn);
-	prepare_fixups(uprobe, &insn);
+	handle_riprel_insn(mm, auprobe, &insn);
+	prepare_fixups(auprobe, &insn);
 
 	return 0;
 }
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index fd45b70..9c6be62 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -29,12 +29,6 @@
 struct vm_area_struct;
 #ifdef CONFIG_ARCH_SUPPORTS_UPROBES
 #include <asm/uprobes.h>
-#else
-
-typedef u8 uprobe_opcode_t;
-struct uprobe_arch_info {};
-
-#define MAX_UINSN_BYTES 4
 #endif
 
 /* flags that denote/change uprobes behaviour */
@@ -56,22 +50,9 @@ struct uprobe_consumer {
 	struct uprobe_consumer *next;
 };
 
-struct uprobe {
-	struct rb_node		rb_node;	/* node in the rb tree */
-	atomic_t		ref;
-	struct rw_semaphore	consumer_rwsem;
-	struct list_head	pending_list;
-	struct uprobe_arch_info arch_info;
-	struct uprobe_consumer	*consumers;
-	struct inode		*inode;		/* Also hold a ref to inode */
-	loff_t			offset;
-	int			flags;
-	u8			insn[MAX_UINSN_BYTES];
-};
-
 #ifdef CONFIG_UPROBES
-extern int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr);
-extern int __weak set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, bool verify);
+extern int __weak set_bkpt(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr);
+extern int __weak set_orig_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr, bool verify);
 extern bool __weak is_bkpt_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ee496ad..13f1b59 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -65,6 +65,18 @@ struct vma_info {
 	loff_t			vaddr;
 };
 
+struct uprobe {
+	struct rb_node		rb_node;	/* node in the rb tree */
+	atomic_t		ref;
+	struct rw_semaphore	consumer_rwsem;
+	struct list_head	pending_list;
+	struct uprobe_consumer	*consumers;
+	struct inode		*inode;		/* Also hold a ref to inode */
+	loff_t			offset;
+	int			flags;
+	struct arch_uprobe	arch;
+};
+
 /*
  * valid_vma: Verify if the specified vma is an executable vma
  * Relax restrictions while unregistering: vm_flags might have
@@ -180,7 +192,7 @@ bool __weak is_bkpt_insn(uprobe_opcode_t *insn)
 /*
  * write_opcode - write the opcode at a given virtual address.
  * @mm: the probed process address space.
- * @uprobe: the breakpointing information.
+ * @arch_uprobe: the breakpointing information.
  * @vaddr: the virtual address to store the opcode.
  * @opcode: opcode to be written at @vaddr.
  *
@@ -190,13 +202,14 @@ bool __weak is_bkpt_insn(uprobe_opcode_t *insn)
  * For mm @mm, write the opcode at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
+static int write_opcode(struct mm_struct *mm, struct arch_uprobe *auprobe,
 			unsigned long vaddr, uprobe_opcode_t opcode)
 {
 	struct page *old_page, *new_page;
 	struct address_space *mapping;
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
+	struct uprobe *uprobe;
 	loff_t addr;
 	int ret;
 
@@ -216,6 +229,7 @@ static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
 	if (!valid_vma(vma, is_bkpt_insn(&opcode)))
 		goto put_out;
 
+	uprobe = container_of(auprobe, struct uprobe, arch);
 	mapping = uprobe->inode->i_mapping;
 	if (mapping != vma->vm_file->f_mapping)
 		goto put_out;
@@ -326,7 +340,7 @@ static int is_bkpt_at_addr(struct mm_struct *mm, unsigned long vaddr)
  * For mm @mm, store the breakpoint instruction at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr)
+int __weak set_bkpt(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr)
 {
 	int result;
 
@@ -337,7 +351,7 @@ int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long v
 	if (result)
 		return result;
 
-	return write_opcode(mm, uprobe, vaddr, UPROBES_BKPT_INSN);
+	return write_opcode(mm, auprobe, vaddr, UPROBES_BKPT_INSN);
 }
 
 /**
@@ -351,7 +365,7 @@ int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long v
  * Return 0 (success) or a negative errno.
  */
 int __weak
-set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, bool verify)
+set_orig_insn(struct mm_struct *mm, struct arch_uprobe *auprobe, unsigned long vaddr, bool verify)
 {
 	if (verify) {
 		int result;
@@ -363,7 +377,7 @@ set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr,
 		if (result != 1)
 			return result;
 	}
-	return write_opcode(mm, uprobe, vaddr, *(uprobe_opcode_t *)uprobe->insn);
+	return write_opcode(mm, auprobe, vaddr, *(uprobe_opcode_t *)auprobe->insn);
 }
 
 static int match_uprobe(struct uprobe *l, struct uprobe *r)
@@ -593,13 +607,13 @@ static int copy_insn(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned
 
 	/* Instruction at the page-boundary; copy bytes in second page */
 	if (nbytes < bytes) {
-		if (__copy_insn(mapping, vma, uprobe->insn + nbytes,
+		if (__copy_insn(mapping, vma, uprobe->arch.insn + nbytes,
 				bytes - nbytes, uprobe->offset + nbytes))
 			return -ENOMEM;
 
 		bytes = nbytes;
 	}
-	return __copy_insn(mapping, vma, uprobe->insn, bytes, uprobe->offset);
+	return __copy_insn(mapping, vma, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
 static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
@@ -625,23 +639,23 @@ static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
 		if (ret)
 			return ret;
 
-		if (is_bkpt_insn((uprobe_opcode_t *)uprobe->insn))
+		if (is_bkpt_insn((uprobe_opcode_t *)uprobe->arch.insn))
 			return -EEXIST;
 
-		ret = arch_uprobes_analyze_insn(mm, uprobe);
+		ret = arch_uprobes_analyze_insn(mm, &uprobe->arch);
 		if (ret)
 			return ret;
 
 		uprobe->flags |= UPROBES_COPY_INSN;
 	}
-	ret = set_bkpt(mm, uprobe, addr);
+	ret = set_bkpt(mm, &uprobe->arch, addr);
 
 	return ret;
 }
 
 static void remove_breakpoint(struct mm_struct *mm, struct uprobe *uprobe, loff_t vaddr)
 {
-	set_orig_insn(mm, uprobe, (unsigned long)vaddr, true);
+	set_orig_insn(mm, &uprobe->arch, (unsigned long)vaddr, true);
 }
 
 static void delete_uprobe(struct uprobe *uprobe)

^ permalink raw reply related	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-02-22 16:08 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-22  9:15 [PATCH 1/3] uprobes/core: Make instruction tables volatile Srikar Dronamraju
2012-02-22  9:15 ` [PATCH 2/3] uprobes/core: remove uprobe_opcode_sz Srikar Dronamraju
2012-02-22 16:05   ` [tip:perf/uprobes] uprobes/core: Remove uprobe_opcode_sz tip-bot for Srikar Dronamraju
2012-02-22  9:16 ` [PATCH 3/3] uprobes/core: move insn to arch specific structure Srikar Dronamraju
2012-02-22 16:06   ` [tip:perf/uprobes] uprobes/core: Move " tip-bot for Srikar Dronamraju
2012-02-22  9:47 ` [PATCH 1/3] uprobes/core: Make instruction tables volatile Ingo Molnar
2012-02-22 10:04   ` Srikar Dronamraju
2012-02-22 16:04 ` [tip:perf/uprobes] " tip-bot for Srikar Dronamraju

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.