Skip to contentSkip to conversation

breakpoint: Rework arch validation v3

GIT PULL New Frederic Weisbecker 2 participants 14 messages Jun 1, 2018 – Jun 21, 2018

Subsystems (8)
Arm (32-bit) x86 PowerPC perf events +4 more subsystems
lore
Share & tools
lore

Frederic Weisbecker

author
#0 lore raw

Ingo,

Please pull the perf/breakpoint-v3 branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
perf/breakpoint-v3

HEAD: 3c0681cc01b39e7fac6eb810e82380165c9b56f1

Changes since v2:

* Constify the struct perf_event_attr passed in hw_breakpoint_parse()
* Add acks

---
When we modify a hardware breakpoint, the architecture code fills up the architecture data as the validation of generic attributes progresses. If something goes wrong in the middle, the architecture data changes aren't rolled back and we are left with a halfway fiddled breakpoint.

This set fixes the various misdesigns that back this bad behaviour.

Thanks,
Frederic
---

Frederic Weisbecker (12):
perf/breakpoint: Split attribute parse and commit perf/breakpoint: Pass arch breakpoint struct to arch_check_bp_in_kernelspace()
x86: Implement hw_breakpoint_arch_parse()
powerpc: Implement hw_breakpoint_arch_parse()
arm: Implement hw_breakpoint_arch_parse()
arm64: Implement hw_breakpoint_arch_parse()
sh: Remove "struct arch_hw_breakpoint::name" unused field
sh: Implement hw_breakpoint_arch_parse()
xtensa: Implement hw_breakpoint_arch_parse()
perf/breakpoint: Remove default hw_breakpoint_arch_parse() perf/breakpoint: Pass new breakpoint type to modify_breakpoint_slot() perf/breakpoint: Clean up and consolidate modify_user_hw_breakpoint_check()

 arch/arm/include/asm/hw_breakpoint.h     |   7 +-
 arch/arm/kernel/hw_breakpoint.c          |  78 +++++++++---------
 arch/arm64/include/asm/hw_breakpoint.h   |   7 +-
 arch/arm64/kernel/hw_breakpoint.c        |  86 ++++++++++----------
 arch/powerpc/include/asm/hw_breakpoint.h |   7 +-
 arch/powerpc/kernel/hw_breakpoint.c      |  47 ++++++-----
 arch/sh/include/asm/hw_breakpoint.h      |   8 +-
 arch/sh/kernel/hw_breakpoint.c           |  53 ++++++-------
 arch/x86/include/asm/hw_breakpoint.h     |   7 +-
 arch/x86/kernel/hw_breakpoint.c          | 131 ++++++++++++++++---------------
 arch/xtensa/include/asm/hw_breakpoint.h  |   7 +-
 arch/xtensa/kernel/hw_breakpoint.c       |  40 ++++------
 kernel/events/hw_breakpoint.c            |  92 +++++++++++++---------
 13 files changed, 294 insertions(+), 276 deletions(-)
author
#1 lore raw

PATCH 1/12 perf/breakpoint: Split attribute parse and commit

arch_validate_hwbkpt_settings() mixes up attribute check and commit into a single code entity. Therefore the validation may return an error due to incorrect atributes while still leaving halfway modified architecture breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is passed a "struct arch_hw_breakpoint" to fill on top of the new attr and the core takes care about copying the backend data once it's fully validated. The architectures then need to implement the new API.

Reported-by Linus Torvalds <[email protected]>
Original-patch-by Andy Lutomirski <[email protected]>
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 kernel/events/hw_breakpoint.c | 57 +++++++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 16 deletions(-)
kernel/events/hw_breakpoint.c +41 -16
index 6e28d28..314e2a9 100644
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
 	return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+			     const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)
 {
-	int ret;
+	int err;
 
-	ret = arch_validate_hwbkpt_settings(bp);
-	if (ret)
-		return ret;
+	err = arch_validate_hwbkpt_settings(bp);
+	if (err)
+		return err;
+
+	*hw = bp->hw.info;
+
+	return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+			       const struct perf_event_attr *attr,
+			       struct arch_hw_breakpoint *hw)
+{
+	int err;
+
+	err = hw_breakpoint_arch_parse(bp, attr, hw);
+	if (err)
+		return err;
 
 	if (arch_check_bp_in_kernelspace(bp)) {
-		if (bp->attr.exclude_kernel)
+		if (attr->exclude_kernel)
 			return -EINVAL;
 		/*
 		 * Don't let unprivileged users set a breakpoint in the trap

@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-	int ret;
+	struct arch_hw_breakpoint hw;
+	int err;
 
-	ret = reserve_bp_slot(bp);
-	if (ret)
-		return ret;
+	err = reserve_bp_slot(bp);
+	if (err)
+		return err;
 
-	ret = validate_hw_breakpoint(bp);
-
-	/* if arch_validate_hwbkpt_settings() fails then release bp slot */
-	if (ret)
+	err = hw_breakpoint_parse(bp, &bp->attr, &hw);
+	if (err) {
 		release_bp_slot(bp);
+		return err;
+	}
 
-	return ret;
+	bp->hw.info = hw;
+
+	return 0;
 }
 
 /**

@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *a
 	u64 old_len  = bp->attr.bp_len;
 	int old_type = bp->attr.bp_type;
 	bool modify  = attr->bp_type != old_type;
+	struct arch_hw_breakpoint hw;
 	int err = 0;
 
 	bp->attr.bp_addr = attr->bp_addr;

@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *a
 	if (check && memcmp(&bp->attr, attr, sizeof(*attr)))
 		return -EINVAL;
 
-	err = validate_hw_breakpoint(bp);
+	err = hw_breakpoint_parse(bp, attr, &hw);
 	if (!err && modify)
 		err = modify_bp_slot(bp, old_type);
 

@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *a
 		return err;
 	}
 
+	bp->hw.info = hw;
 	bp->attr.disabled = attr->disabled;
+
 	return 0;
 }
Signature · 1 line
2.7.4
author
#2 lore raw

PATCH 2/12 perf/breakpoint: Pass arch breakpoint struct to arch_check_bp_in_kernelspace()

In reply to Frederic Weisbecker

We can't pass the breakpoint directly on arch_check_bp_in_kernelspace() anymore because its architecture internal datas (struct arch_hw_breakpoint) are not yet filled by the time we call the function, and most implementation need this backend to be up to date. So arrange the function to take the probing struct instead.

Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/arm/include/asm/hw_breakpoint.h     |  2 +-
 arch/arm/kernel/hw_breakpoint.c          | 11 +++--
 arch/arm64/include/asm/hw_breakpoint.h   |  2 +-
 arch/arm64/kernel/hw_breakpoint.c        |  9 ++--
 arch/powerpc/include/asm/hw_breakpoint.h |  2 +-
 arch/powerpc/kernel/hw_breakpoint.c      |  6 +--
 arch/sh/include/asm/hw_breakpoint.h      |  2 +-
 arch/sh/kernel/hw_breakpoint.c           |  9 ++--
 arch/x86/include/asm/hw_breakpoint.h     |  2 +-
 arch/x86/kernel/hw_breakpoint.c          | 71 +++++++++++++++++---------------
 arch/xtensa/include/asm/hw_breakpoint.h  |  2 +-
 arch/xtensa/kernel/hw_breakpoint.c       |  7 ++--
 kernel/events/hw_breakpoint.c            |  2 +-
 13 files changed, 63 insertions(+), 64 deletions(-)
arch/arm/include/asm/hw_breakpoint.h +1 -1
index e46e4e7..d5a0f52 100644
@@ -117,7 +117,7 @@ struct pmu;
 
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 				  int *gen_len, int *gen_type);
-extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
+extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/arm/kernel/hw_breakpoint.c +5 -6
index 629e251..385dcf4 100644
@@ -456,14 +456,13 @@ static int get_hbp_len(u8 hbp_len)
 /*
  * Check whether bp virtual address is in kernel space.
  */
-int arch_check_bp_in_kernelspace(struct perf_event *bp)
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
 {
 	unsigned int len;
 	unsigned long va;
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 
-	va = info->address;
-	len = get_hbp_len(info->ctrl.len);
+	va = hw->address;
+	len = get_hbp_len(hw->ctrl.len);
 
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }

@@ -576,7 +575,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 
 	/* Privilege */
 	info->ctrl.privilege = ARM_BREAKPOINT_USER;
-	if (arch_check_bp_in_kernelspace(bp))
+	if (arch_check_bp_in_kernelspace(info))
 		info->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
 
 	/* Enabled? */

@@ -640,7 +639,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 			return -EINVAL;
 
 		/* We don't allow mismatch breakpoints in kernel space. */
-		if (arch_check_bp_in_kernelspace(bp))
+		if (arch_check_bp_in_kernelspace(info))
 			return -EPERM;
 
 		/*
arch/arm64/include/asm/hw_breakpoint.h +1 -1
index 4177076..9f4a3d4 100644
@@ -124,7 +124,7 @@ struct pmu;
 
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 				  int *gen_len, int *gen_type, int *offset);
-extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
+extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/arm64/kernel/hw_breakpoint.c +4 -5
index 74bb56f..92c6973 100644
@@ -344,14 +344,13 @@ static int get_hbp_len(u8 hbp_len)
 /*
  * Check whether bp virtual address is in kernel space.
  */
-int arch_check_bp_in_kernelspace(struct perf_event *bp)
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
 {
 	unsigned int len;
 	unsigned long va;
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 
-	va = info->address;
-	len = get_hbp_len(info->ctrl.len);
+	va = hw->address;
+	len = get_hbp_len(hw->ctrl.len);
 
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }

@@ -503,7 +502,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 	 * Note that we disallow combined EL0/EL1 breakpoints because
 	 * that would complicate the stepping code.
 	 */
-	if (arch_check_bp_in_kernelspace(bp))
+	if (arch_check_bp_in_kernelspace(info))
 		info->ctrl.privilege = AARCH64_BREAKPOINT_EL1;
 	else
 		info->ctrl.privilege = AARCH64_BREAKPOINT_EL0;
arch/powerpc/include/asm/hw_breakpoint.h +1 -1
index 8e7b097..4d0b1bf 100644
@@ -60,7 +60,7 @@ struct perf_sample_data;
 
 extern int hw_breakpoint_slots(int type);
 extern int arch_bp_generic_fields(int type, int *gen_bp_type);
-extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
+extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 						unsigned long val, void *data);
arch/powerpc/kernel/hw_breakpoint.c +2 -4
index 4c1012b..348cac9 100644
@@ -119,11 +119,9 @@ void arch_unregister_hw_breakpoint(struct perf_event *bp)
 /*
  * Check for virtual address in kernel space.
  */
-int arch_check_bp_in_kernelspace(struct perf_event *bp)
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-
-	return is_kernel_addr(info->address);
+	return is_kernel_addr(hw->address);
 }
 
 int arch_bp_generic_fields(int type, int *gen_bp_type)
arch/sh/include/asm/hw_breakpoint.h +1 -1
index 7431c17..8a88ed0 100644
@@ -54,7 +54,7 @@ static inline int hw_breakpoint_slots(int type)
 }
 
 /* arch/sh/kernel/hw_breakpoint.c */
-extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
+extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/sh/kernel/hw_breakpoint.c +4 -5
index afe9657..713699b 100644
@@ -124,14 +124,13 @@ static int get_hbp_len(u16 hbp_len)
 /*
  * Check for virtual address in kernel space.
  */
-int arch_check_bp_in_kernelspace(struct perf_event *bp)
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
 {
 	unsigned int len;
 	unsigned long va;
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 
-	va = info->address;
-	len = get_hbp_len(info->len);
+	va = hw->address;
+	len = get_hbp_len(hw->len);
 
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }

@@ -346,7 +345,7 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args)
 		perf_bp_event(bp, args->regs);
 
 		/* Deliver the signal to userspace */
-		if (!arch_check_bp_in_kernelspace(bp)) {
+		if (!arch_check_bp_in_kernelspace(&bp->hw.info)) {
 			siginfo_t info;
 
 			info.si_signo = args->signr;
arch/x86/include/asm/hw_breakpoint.h +1 -1
index f59c398..7892459 100644
@@ -52,7 +52,7 @@ static inline int hw_breakpoint_slots(int type)
 struct perf_event;
 struct pmu;
 
-extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
+extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/x86/kernel/hw_breakpoint.c +38 -33

94 lines not on this page · raw message

arch/xtensa/include/asm/hw_breakpoint.h +1 -1
index dbe3053b..2525bf6 100644
@@ -35,7 +35,7 @@ struct pt_regs;
 struct task_struct;
 
 int hw_breakpoint_slots(int type);
-int arch_check_bp_in_kernelspace(struct perf_event *bp);
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 int arch_validate_hwbkpt_settings(struct perf_event *bp);
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data);
arch/xtensa/kernel/hw_breakpoint.c +3 -4
index b35656a..6e34c38 100644
@@ -33,14 +33,13 @@ int hw_breakpoint_slots(int type)
 	}
 }
 
-int arch_check_bp_in_kernelspace(struct perf_event *bp)
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
 {
 	unsigned int len;
 	unsigned long va;
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 
-	va = info->address;
-	len = bp->attr.bp_len;
+	va = hw->address;
+	len = hw->len;
 
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }
kernel/events/hw_breakpoint.c +1 -1
index 314e2a9..89fe94c 100644
@@ -427,7 +427,7 @@ static int hw_breakpoint_parse(struct perf_event *bp,
 	if (err)
 		return err;
 
-	if (arch_check_bp_in_kernelspace(bp)) {
+	if (arch_check_bp_in_kernelspace(hw)) {
 		if (attr->exclude_kernel)
 			return -EINVAL;
 		/*
Signature · 1 line
2.7.4
author
#3 lore raw

PATCH 3/12 x86: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

Migrate to the new API in order to remove arch_validate_hwbkpt_settings() that clumsily mixes up architecture validation and commit.

Original-patch-by Andy Lutomirski <[email protected]>
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/x86/include/asm/hw_breakpoint.h |  6 +++-
 arch/x86/kernel/hw_breakpoint.c      | 60 ++++++++++++++++++------------------
 2 files changed, 35 insertions(+), 31 deletions(-)
arch/x86/include/asm/hw_breakpoint.h +5 -1
index 7892459..6c88e8e2 100644
@@ -49,11 +49,15 @@ static inline int hw_breakpoint_slots(int type)
 	return HBP_NUM;
 }
 
+struct perf_event_attr;
 struct perf_event;
 struct pmu;
 
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_parse(struct perf_event *bp,
+				    const struct perf_event_attr *attr,
+				    struct arch_hw_breakpoint *hw);
+#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/x86/kernel/hw_breakpoint.c +30 -30

137 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#4 lore raw

PATCH 4/12 powerpc: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

Migrate to the new API in order to remove arch_validate_hwbkpt_settings() that clumsily mixes up architecture validation and commit

Acked-by Michael Ellerman <[email protected]>
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/powerpc/include/asm/hw_breakpoint.h |  6 ++++-
 arch/powerpc/kernel/hw_breakpoint.c      | 41 ++++++++++++++++----------------
 2 files changed, 26 insertions(+), 21 deletions(-)
arch/powerpc/include/asm/hw_breakpoint.h +5 -1
index 4d0b1bf..38ae180 100644
@@ -52,6 +52,7 @@ struct arch_hw_breakpoint {
 #include <asm/reg.h>
 #include <asm/debug.h>
 
+struct perf_event_attr;
 struct perf_event;
 struct pmu;
 struct perf_sample_data;

@@ -61,7 +62,10 @@ struct perf_sample_data;
 extern int hw_breakpoint_slots(int type);
 extern int arch_bp_generic_fields(int type, int *gen_bp_type);
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_parse(struct perf_event *bp,
+				    const struct perf_event_attr *attr,
+				    struct arch_hw_breakpoint *hw);
+#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 						unsigned long val, void *data);
 int arch_install_hw_breakpoint(struct perf_event *bp);
arch/powerpc/kernel/hw_breakpoint.c +21 -20

63 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#5 lore raw

PATCH 5/12 arm: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

Migrate to the new API in order to remove arch_validate_hwbkpt_settings() that clumsily mixes up architecture validation and commit.

Acked-by Mark Rutland <[email protected]>
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/arm/include/asm/hw_breakpoint.h |  6 ++-
 arch/arm/kernel/hw_breakpoint.c      | 71 ++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 36 deletions(-)
arch/arm/include/asm/hw_breakpoint.h +5 -1
index d5a0f52..1e02925 100644
@@ -111,6 +111,7 @@ static inline void decode_ctrl_reg(u32 reg,
 	asm volatile("mcr p14, 0, %0, " #N "," #M ", " #OP2 : : "r" (VAL));\
 } while (0)
 
+struct perf_event_attr;
 struct notifier_block;
 struct perf_event;
 struct pmu;

@@ -118,7 +119,10 @@ struct pmu;
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 				  int *gen_len, int *gen_type);
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_parse(struct perf_event *bp,
+				    const struct perf_event_attr *attr,
+				    struct arch_hw_breakpoint *hw);
+#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/arm/kernel/hw_breakpoint.c +36 -35

159 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#6 lore raw

PATCH 6/12 arm64: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

Migrate to the new API in order to remove arch_validate_hwbkpt_settings() that clumsily mixes up architecture validation and commit.

Acked-by Will Deacon <[email protected]>
Acked-by Mark Rutland <[email protected]>
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/arm64/include/asm/hw_breakpoint.h |  6 ++-
 arch/arm64/kernel/hw_breakpoint.c      | 79 +++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 40 deletions(-)
arch/arm64/include/asm/hw_breakpoint.h +5 -1
index 9f4a3d4..bf9c305 100644
@@ -119,13 +119,17 @@ static inline void decode_ctrl_reg(u32 reg,
 
 struct task_struct;
 struct notifier_block;
+struct perf_event_attr;
 struct perf_event;
 struct pmu;
 
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 				  int *gen_len, int *gen_type, int *offset);
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_parse(struct perf_event *bp,
+				    const struct perf_event_attr *attr,
+				    struct arch_hw_breakpoint *hw);
+#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/arm64/kernel/hw_breakpoint.c +40 -39

186 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#7 lore raw

PATCH 7/12 sh: Remove "struct arch_hw_breakpoint::name" unused field

In reply to Frederic Weisbecker

This field seem to be unused, perhaps a leftover from old code...

Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/sh/include/asm/hw_breakpoint.h | 1 -
 arch/sh/kernel/hw_breakpoint.c      | 7 -------
 2 files changed, 8 deletions(-)
arch/sh/include/asm/hw_breakpoint.h +0 -1
index 8a88ed0..dae622d 100644
@@ -10,7 +10,6 @@
 #include <linux/types.h>
 
 struct arch_hw_breakpoint {
-	char		*name; /* Contains name of the symbol to set bkpt */
 	unsigned long	address;
 	u16		len;
 	u16		type;
arch/sh/kernel/hw_breakpoint.c +0 -7
index 713699b..3594773 100644
@@ -248,13 +248,6 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	}
 
 	/*
-	 * For kernel-addresses, either the address or symbol name can be
-	 * specified.
-	 */
-	if (info->name)
-		info->address = (unsigned long)kallsyms_lookup_name(info->name);
-
-	/*
 	 * Check that the low-order bits of the address are appropriate
 	 * for the alignment implied by len.
 	 */
Signature · 1 line
2.7.4
author
#8 lore raw

PATCH 8/12 sh: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

Migrate to the new API in order to remove arch_validate_hwbkpt_settings() that clumsily mixes up architecture validation and commit

Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/sh/include/asm/hw_breakpoint.h |  6 +++++-
 arch/sh/kernel/hw_breakpoint.c      | 37 +++++++++++++++++++------------------
 2 files changed, 24 insertions(+), 19 deletions(-)
arch/sh/include/asm/hw_breakpoint.h +5 -1
index dae622d..867edcc 100644
@@ -40,6 +40,7 @@ struct sh_ubc {
 	struct clk	*clk;	/* optional interface clock / MSTP bit */
 };
 
+struct perf_event_attr;
 struct perf_event;
 struct task_struct;
 struct pmu;

@@ -54,7 +55,10 @@ static inline int hw_breakpoint_slots(int type)
 
 /* arch/sh/kernel/hw_breakpoint.c */
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_parse(struct perf_event *bp,
+				    const struct perf_event_attr *attr,
+				    struct arch_hw_breakpoint *hw);
+#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/sh/kernel/hw_breakpoint.c +19 -18
index 3594773..099416d 100644
@@ -173,40 +173,40 @@ int arch_bp_generic_fields(int sh_len, int sh_type,
 	return 0;
 }
 
-static int arch_build_bp_info(struct perf_event *bp)
+static int arch_build_bp_info(struct perf_event *bp,
+			      const struct perf_event_attr *attr,
+			      struct arch_hw_breakpoint *hw)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-
-	info->address = bp->attr.bp_addr;
+	hw->address = attr->bp_addr;
 
 	/* Len */
-	switch (bp->attr.bp_len) {
+	switch (attr->bp_len) {
 	case HW_BREAKPOINT_LEN_1:
-		info->len = SH_BREAKPOINT_LEN_1;
+		hw->len = SH_BREAKPOINT_LEN_1;
 		break;
 	case HW_BREAKPOINT_LEN_2:
-		info->len = SH_BREAKPOINT_LEN_2;
+		hw->len = SH_BREAKPOINT_LEN_2;
 		break;
 	case HW_BREAKPOINT_LEN_4:
-		info->len = SH_BREAKPOINT_LEN_4;
+		hw->len = SH_BREAKPOINT_LEN_4;
 		break;
 	case HW_BREAKPOINT_LEN_8:
-		info->len = SH_BREAKPOINT_LEN_8;
+		hw->len = SH_BREAKPOINT_LEN_8;
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	/* Type */
-	switch (bp->attr.bp_type) {
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_R:
-		info->type = SH_BREAKPOINT_READ;
+		hw->type = SH_BREAKPOINT_READ;
 		break;
 	case HW_BREAKPOINT_W:
-		info->type = SH_BREAKPOINT_WRITE;
+		hw->type = SH_BREAKPOINT_WRITE;
 		break;
 	case HW_BREAKPOINT_W | HW_BREAKPOINT_R:
-		info->type = SH_BREAKPOINT_RW;
+		hw->type = SH_BREAKPOINT_RW;
 		break;
 	default:
 		return -EINVAL;

@@ -218,19 +218,20 @@ static int arch_build_bp_info(struct perf_event *bp)
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+			     const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	unsigned int align;
 	int ret;
 
-	ret = arch_build_bp_info(bp);
+	ret = arch_build_bp_info(bp, attr, hw);
 	if (ret)
 		return ret;
 
 	ret = -EINVAL;
 
-	switch (info->len) {
+	switch (hw->len) {
 	case SH_BREAKPOINT_LEN_1:
 		align = 0;
 		break;

@@ -251,7 +252,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	 * Check that the low-order bits of the address are appropriate
 	 * for the alignment implied by len.
 	 */
-	if (info->address & align)
+	if (hw->address & align)
 		return -EINVAL;
 
 	return 0;
Signature · 1 line
2.7.4
author
#9 lore raw

PATCH 9/12 xtensa: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

Migrate to the new API in order to remove arch_validate_hwbkpt_settings() that clumsily mixes up architecture validation and commit

Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/xtensa/include/asm/hw_breakpoint.h |  6 +++++-
 arch/xtensa/kernel/hw_breakpoint.c      | 33 ++++++++++++---------------------
 2 files changed, 17 insertions(+), 22 deletions(-)
arch/xtensa/include/asm/hw_breakpoint.h +5 -1
index 2525bf6..f347c21 100644
@@ -30,13 +30,17 @@ struct arch_hw_breakpoint {
 	u16 type;
 };
 
+struct perf_event_attr;
 struct perf_event;
 struct pt_regs;
 struct task_struct;
 
 int hw_breakpoint_slots(int type);
 int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
-int arch_validate_hwbkpt_settings(struct perf_event *bp);
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+			     const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw);
+#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data);
arch/xtensa/kernel/hw_breakpoint.c +12 -21
index 6e34c38..c2e387c 100644
@@ -47,50 +47,41 @@ int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
 /*
  * Construct an arch_hw_breakpoint from a perf_event.
  */
-static int arch_build_bp_info(struct perf_event *bp)
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+			     const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-
 	/* Type */
-	switch (bp->attr.bp_type) {
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_X:
-		info->type = XTENSA_BREAKPOINT_EXECUTE;
+		hw->type = XTENSA_BREAKPOINT_EXECUTE;
 		break;
 	case HW_BREAKPOINT_R:
-		info->type = XTENSA_BREAKPOINT_LOAD;
+		hw->type = XTENSA_BREAKPOINT_LOAD;
 		break;
 	case HW_BREAKPOINT_W:
-		info->type = XTENSA_BREAKPOINT_STORE;
+		hw->type = XTENSA_BREAKPOINT_STORE;
 		break;
 	case HW_BREAKPOINT_RW:
-		info->type = XTENSA_BREAKPOINT_LOAD | XTENSA_BREAKPOINT_STORE;
+		hw->type = XTENSA_BREAKPOINT_LOAD | XTENSA_BREAKPOINT_STORE;
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	/* Len */
-	info->len = bp->attr.bp_len;
-	if (info->len < 1 || info->len > 64 || !is_power_of_2(info->len))
+	hw->len = attr->bp_len;
+	if (hw->len < 1 || hw->len > 64 || !is_power_of_2(hw->len))
 		return -EINVAL;
 
 	/* Address */
-	info->address = bp->attr.bp_addr;
-	if (info->address & (info->len - 1))
+	hw->address = attr->bp_addr;
+	if (hw->address & (hw->len - 1))
 		return -EINVAL;
 
 	return 0;
 }
 
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int ret;
-
-	/* Build the arch_hw_breakpoint. */
-	ret = arch_build_bp_info(bp);
-	return ret;
-}
-
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data)
 {
Signature · 1 line
2.7.4
author
#10 lore raw

PATCH 10/12 perf/breakpoint: Remove default hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker

All architectures have implemented it, we can now remove the poor weak version.

Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 arch/arm/include/asm/hw_breakpoint.h     |  1 -
 arch/arm64/include/asm/hw_breakpoint.h   |  1 -
 arch/powerpc/include/asm/hw_breakpoint.h |  1 -
 arch/sh/include/asm/hw_breakpoint.h      |  1 -
 arch/x86/include/asm/hw_breakpoint.h     |  1 -
 arch/xtensa/include/asm/hw_breakpoint.h  |  1 -
 kernel/events/hw_breakpoint.c            | 17 -----------------
 7 files changed, 23 deletions(-)
arch/arm/include/asm/hw_breakpoint.h +0 -1
index 1e02925..ac54c06 100644
@@ -122,7 +122,6 @@ extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int hw_breakpoint_arch_parse(struct perf_event *bp,
 				    const struct perf_event_attr *attr,
 				    struct arch_hw_breakpoint *hw);
-#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/arm64/include/asm/hw_breakpoint.h +0 -1
index bf9c305..6a53e59 100644
@@ -129,7 +129,6 @@ extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int hw_breakpoint_arch_parse(struct perf_event *bp,
 				    const struct perf_event_attr *attr,
 				    struct arch_hw_breakpoint *hw);
-#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/powerpc/include/asm/hw_breakpoint.h +0 -1
index 38ae180..27d6e3c 100644
@@ -65,7 +65,6 @@ extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int hw_breakpoint_arch_parse(struct perf_event *bp,
 				    const struct perf_event_attr *attr,
 				    struct arch_hw_breakpoint *hw);
-#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 						unsigned long val, void *data);
 int arch_install_hw_breakpoint(struct perf_event *bp);
arch/sh/include/asm/hw_breakpoint.h +0 -1
index 867edcc..199d17b 100644
@@ -58,7 +58,6 @@ extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int hw_breakpoint_arch_parse(struct perf_event *bp,
 				    const struct perf_event_attr *attr,
 				    struct arch_hw_breakpoint *hw);
-#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/x86/include/asm/hw_breakpoint.h +0 -1
index 6c88e8e2..a1f0e90 100644
@@ -57,7 +57,6 @@ extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int hw_breakpoint_arch_parse(struct perf_event *bp,
 				    const struct perf_event_attr *attr,
 				    struct arch_hw_breakpoint *hw);
-#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
arch/xtensa/include/asm/hw_breakpoint.h +0 -1
index f347c21..9f119c1 100644
@@ -40,7 +40,6 @@ int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 int hw_breakpoint_arch_parse(struct perf_event *bp,
 			     const struct perf_event_attr *attr,
 			     struct arch_hw_breakpoint *hw);
-#define hw_breakpoint_arch_parse hw_breakpoint_arch_parse
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data);
kernel/events/hw_breakpoint.c +0 -17
index 89fe94c..e7bc8d0 100644
@@ -400,23 +400,6 @@ int dbg_release_bp_slot(struct perf_event *bp)
 	return 0;
 }
 
-#ifndef hw_breakpoint_arch_parse
-int hw_breakpoint_arch_parse(struct perf_event *bp,
-			     const struct perf_event_attr *attr,
-			     struct arch_hw_breakpoint *hw)
-{
-	int err;
-
-	err = arch_validate_hwbkpt_settings(bp);
-	if (err)
-		return err;
-
-	*hw = bp->hw.info;
-
-	return 0;
-}
-#endif
-
 static int hw_breakpoint_parse(struct perf_event *bp,
 			       const struct perf_event_attr *attr,
 			       struct arch_hw_breakpoint *hw)
Signature · 1 line
2.7.4
author
#11 lore raw

PATCH 11/12 perf/breakpoint: Pass new breakpoint type to modify_breakpoint_slot()

In reply to Frederic Weisbecker

We soon won't be able to rely on bp->attr anymore to get the new type of the modifying breakpoint because the new attributes are going to be copied only once we successfully modified the breakpoint slot.

This will fix the current misdesigned layout where the new attr are copied to the modifying breakpoint before we actually know if the modification will be validated.

In order to prepare for that, allow modify_breakpoint_slot() to take the new breakpoint type.

Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 kernel/events/hw_breakpoint.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
kernel/events/hw_breakpoint.c +5 -5
index e7bc8d0..7138770 100644
@@ -345,13 +345,13 @@ void release_bp_slot(struct perf_event *bp)
 	mutex_unlock(&nr_bp_mutex);
 }
 
-static int __modify_bp_slot(struct perf_event *bp, u64 old_type)
+static int __modify_bp_slot(struct perf_event *bp, u64 old_type, u64 new_type)
 {
 	int err;
 
 	__release_bp_slot(bp, old_type);
 
-	err = __reserve_bp_slot(bp, bp->attr.bp_type);
+	err = __reserve_bp_slot(bp, new_type);
 	if (err) {
 		/*
 		 * Reserve the old_type slot back in case

@@ -367,12 +367,12 @@ static int __modify_bp_slot(struct perf_event *bp, u64 old_type)
 	return err;
 }
 
-static int modify_bp_slot(struct perf_event *bp, u64 old_type)
+static int modify_bp_slot(struct perf_event *bp, u64 old_type, u64 new_type)
 {
 	int ret;
 
 	mutex_lock(&nr_bp_mutex);
-	ret = __modify_bp_slot(bp, old_type);
+	ret = __modify_bp_slot(bp, old_type, new_type);
 	mutex_unlock(&nr_bp_mutex);
 	return ret;
 }

@@ -481,7 +481,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *a
 
 	err = hw_breakpoint_parse(bp, attr, &hw);
 	if (!err && modify)
-		err = modify_bp_slot(bp, old_type);
+		err = modify_bp_slot(bp, old_type, bp->attr.bp_type);
 
 	if (err) {
 		bp->attr.bp_addr = old_addr;
Signature · 1 line
2.7.4
author
#12 lore raw

PATCH 12/12 perf/breakpoint: Clean up and consolidate modify_user_hw_breakpoint_check()

In reply to Frederic Weisbecker

Remove the dance around old and new attributes. Just don't modify the previous breakpoint at all until we have verified everything.

Reported-by Linus Torvalds <[email protected]>
Original-patch-by Andy Lutomirski <[email protected]>
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Max Filippov <[email protected]>
Cc Chris Zankel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Joel Fernandes <[email protected]>
---
 kernel/events/hw_breakpoint.c | 46 ++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 20 deletions(-)
kernel/events/hw_breakpoint.c +26 -20
index 7138770..b3814fc 100644
@@ -461,37 +461,43 @@ register_user_hw_breakpoint(struct perf_event_attr *attr,
 }
 EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
 
+static void hw_breakpoint_copy_attr(struct perf_event_attr *to,
+				    struct perf_event_attr *from)
+{
+	to->bp_addr = from->bp_addr;
+	to->bp_type = from->bp_type;
+	to->bp_len  = from->bp_len;
+	to->disabled = from->disabled;
+}
+
 int
 modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *attr,
 			        bool check)
 {
-	u64 old_addr = bp->attr.bp_addr;
-	u64 old_len  = bp->attr.bp_len;
-	int old_type = bp->attr.bp_type;
-	bool modify  = attr->bp_type != old_type;
 	struct arch_hw_breakpoint hw;
-	int err = 0;
-
-	bp->attr.bp_addr = attr->bp_addr;
-	bp->attr.bp_type = attr->bp_type;
-	bp->attr.bp_len  = attr->bp_len;
-
-	if (check && memcmp(&bp->attr, attr, sizeof(*attr)))
-		return -EINVAL;
+	int err;
 
 	err = hw_breakpoint_parse(bp, attr, &hw);
-	if (!err && modify)
-		err = modify_bp_slot(bp, old_type, bp->attr.bp_type);
-
-	if (err) {
-		bp->attr.bp_addr = old_addr;
-		bp->attr.bp_type = old_type;
-		bp->attr.bp_len  = old_len;
+	if (err)
 		return err;
+
+	if (check) {
+		struct perf_event_attr old_attr;
+
+		old_attr = bp->attr;
+		hw_breakpoint_copy_attr(&old_attr, attr);
+		if (memcmp(&old_attr, attr, sizeof(*attr)))
+			return -EINVAL;
+	}
+
+	if (bp->attr.bp_type != attr->bp_type) {
+		err = modify_bp_slot(bp, bp->attr.bp_type, attr->bp_type);
+		if (err)
+			return err;
 	}
 
+	hw_breakpoint_copy_attr(&bp->attr, attr);
 	bp->hw.info = hw;
-	bp->attr.disabled = attr->disabled;
 
 	return 0;
 }
Signature · 1 line
2.7.4
#13 lore raw
In reply to Frederic Weisbecker
Quoted 41 lines · Frederic Weisbecker

Ingo,

Please pull the perf/breakpoint-v3 branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
perf/breakpoint-v3

HEAD: 3c0681cc01b39e7fac6eb810e82380165c9b56f1

Changes since v2:

* Constify the struct perf_event_attr passed in hw_breakpoint_parse()
* Add acks

---
When we modify a hardware breakpoint, the architecture code fills up the architecture data as the validation of generic attributes progresses. If something goes wrong in the middle, the architecture data changes aren't rolled back and we are left with a halfway fiddled breakpoint.

This set fixes the various misdesigns that back this bad behaviour.

Thanks,
Frederic
---

Frederic Weisbecker (12):
perf/breakpoint: Split attribute parse and commit perf/breakpoint: Pass arch breakpoint struct to arch_check_bp_in_kernelspace()
x86: Implement hw_breakpoint_arch_parse()
powerpc: Implement hw_breakpoint_arch_parse()
arm: Implement hw_breakpoint_arch_parse()
arm64: Implement hw_breakpoint_arch_parse()
sh: Remove "struct arch_hw_breakpoint::name" unused field
sh: Implement hw_breakpoint_arch_parse()
xtensa: Implement hw_breakpoint_arch_parse()
perf/breakpoint: Remove default hw_breakpoint_arch_parse() perf/breakpoint: Pass new breakpoint type to modify_breakpoint_slot() perf/breakpoint: Clean up and consolidate modify_user_hw_breakpoint_check()

Quoted 14 lines
 arch/arm/include/asm/hw_breakpoint.h     |   7 +-
 arch/arm/kernel/hw_breakpoint.c          |  78 +++++++++---------
 arch/arm64/include/asm/hw_breakpoint.h   |   7 +-
 arch/arm64/kernel/hw_breakpoint.c        |  86 ++++++++++----------
 arch/powerpc/include/asm/hw_breakpoint.h |   7 +-
 arch/powerpc/kernel/hw_breakpoint.c      |  47 ++++++-----
 arch/sh/include/asm/hw_breakpoint.h      |   8 +-
 arch/sh/kernel/hw_breakpoint.c           |  53 ++++++-------
 arch/x86/include/asm/hw_breakpoint.h     |   7 +-
 arch/x86/kernel/hw_breakpoint.c          | 131 ++++++++++++++++---------------
 arch/xtensa/include/asm/hw_breakpoint.h  |   7 +-
 arch/xtensa/kernel/hw_breakpoint.c       |  40 ++++------
 kernel/events/hw_breakpoint.c            |  92 +++++++++++++---------
 13 files changed, 294 insertions(+), 276 deletions(-)

Ok, this looks pretty good.

I was trying to apply this series today, but got a few conflicts due to new changes in the merge window, in:

arch/sh/kernel/hw_breakpoint.c
arch/powerpc/kernel/hw_breakpoint.c

Could you please resolve those conflicts and resend the series?

Thanks!

Ingo