Skip to contentSkip to conversation

breakpoint: Rework arch validation v2

PATCH In review Frederic Weisbecker 4 participants 24 messages May 19, 2018 – Jun 5, 2018

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

Frederic Weisbecker

author
#0 lore raw

cover breakpoint: Rework arch validation v2

Changes since v1:

- Avoid arch code duplication between check and commit. Use a temporary struct arch_hw_breakpoint to fill up arch data on the fly (Mark Rutland)

- Start with a default weak version of hw_breakpoint_arch_parse() that architectures override one by one (Peter Zijlstra, Andy Lutomirski)

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

HEAD: a350174ed8f7240b91ae46933bfe2274d3d03806

--- Summary ---

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..51320c2 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,
+			     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,
+			       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
reviewer
#2 lore raw

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

Quoted 14 lines · Frederic Weisbecker
kernel/events/hw_breakpoint.c +2 -3
index 6e28d28..51320c2 100644
@@ -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)

kernel/events/hw_breakpoint.c static int validate_hw_breakpoint(struct perf_event *bp) · line 453

Quoted 10 lines
kernel/events/hw_breakpoint.c +4 -5
-		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);

Is there a good reason we pass bp and bp->attr? (I assume so)

That added to the confusion in the existing code I think.

cheers

author
#3 lore raw

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

Quoted 29 lines · Michael Ellerman
Quoted 14 lines · Frederic Weisbecker
kernel/events/hw_breakpoint.c +2 -3
index 6e28d28..51320c2 100644
@@ -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)

kernel/events/hw_breakpoint.c static int validate_hw_breakpoint(struct perf_event *bp) · line 453

Quoted 10 lines
kernel/events/hw_breakpoint.c +4 -5
-		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);

Is there a good reason we pass bp and bp->attr? (I assume so)

Quoted 1 line

That added to the confusion in the existing code I think.

Yes, on breakpoint creation (which is the above function) it's not needed but breakpoint modification wants it as we need to pass the attr that are to be validated, and those are not yet copied to the breakpoint at this stage. This happens in the end of the series.

Thanks.

reviewer
#4 lore raw

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

Quoted 30 lines · Frederic Weisbecker
Quoted 29 lines · Michael Ellerman
Quoted 14 lines · Frederic Weisbecker
kernel/events/hw_breakpoint.c +2 -3
index 6e28d28..51320c2 100644
@@ -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)

kernel/events/hw_breakpoint.c static int validate_hw_breakpoint(struct perf_event *bp) · line 453

Quoted 10 lines
kernel/events/hw_breakpoint.c +4 -5
-		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);

Is there a good reason we pass bp and bp->attr? (I assume so)

Quoted 6 lines
Quoted 1 line

That added to the confusion in the existing code I think.

Yes, on breakpoint creation (which is the above function) it's not needed but breakpoint modification wants it as we need to pass the attr that are to be validated, and those are not yet copied to the breakpoint at this stage. This happens in the end of the series.

OK thanks.

cheers

author
#5 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 51320c2..9ac03ab 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
#6 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..8cc161f 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,
+				    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
#7 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

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..8f647e6 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,
+				    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
reviewer Acked-by on v1 4/12
#8 lore raw

Re: [PATCH 04/12] powerpc: Implement hw_breakpoint_arch_parse()

arch/powerpc/kernel/hw_breakpoint.c int arch_bp_generic_fields(int type, int *gen_bp_type) · line 144

Quoted 12 lines · Frederic Weisbecker
arch/powerpc/kernel/hw_breakpoint.c +3 -1
index 348cac9..fba6527 100644
@@ -139,30 +139,31 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 /*
  * 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,
+			     struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)

I think the semantics here are that we are reading from bp/attr and writing to hw?

If so would some sprinkling of const on the first two parameters help make that clearer?

Quoted 27 lines
arch/powerpc/kernel/hw_breakpoint.c +6 -13
 {
 	int ret = -EINVAL, length_max;
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 
 	if (!bp)
 		return ret;
 
-	info->type = HW_BRK_TYPE_TRANSLATE;
-	if (bp->attr.bp_type & HW_BREAKPOINT_R)
-		info->type |= HW_BRK_TYPE_READ;
-	if (bp->attr.bp_type & HW_BREAKPOINT_W)
-		info->type |= HW_BRK_TYPE_WRITE;
-	if (info->type == HW_BRK_TYPE_TRANSLATE)
+	hw->type = HW_BRK_TYPE_TRANSLATE;
+	if (attr->bp_type & HW_BREAKPOINT_R)
+		hw->type |= HW_BRK_TYPE_READ;
+	if (attr->bp_type & HW_BREAKPOINT_W)
+		hw->type |= HW_BRK_TYPE_WRITE;
+	if (hw->type == HW_BRK_TYPE_TRANSLATE)
 		/* must set alteast read or write */
 		return ret;
-	if (!(bp->attr.exclude_user))
-		info->type |= HW_BRK_TYPE_USER;
-	if (!(bp->attr.exclude_kernel))
-		info->type |= HW_BRK_TYPE_KERNEL;
-	if (!(bp->attr.exclude_hv))
-		info->type |= HW_BRK_TYPE_HYP;

arch/powerpc/kernel/hw_breakpoint.c int arch_bp_generic_fields(int type, int *gen_bp_type) · line 166

Quoted 10 lines
arch/powerpc/kernel/hw_breakpoint.c +8 -2
-	info->address = bp->attr.bp_addr;
-	info->len = bp->attr.bp_len;
+	if (!attr->exclude_user)
+		hw->type |= HW_BRK_TYPE_USER;
+	if (!attr->exclude_kernel)
+		hw->type |= HW_BRK_TYPE_KERNEL;
+	if (!attr->exclude_hv)
+		hw->type |= HW_BRK_TYPE_HYP;
+	hw->address = attr->bp_addr;
+	hw->len = attr->bp_len;

All looks right to me.

arch/powerpc/kernel/hw_breakpoint.c int arch_validate_hwbkpt_settings(struct perf_event *bp) · line 182

Quoted 11 lines
arch/powerpc/kernel/hw_breakpoint.c +2 -2
 	/*
 	 * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)

@@ -176,12 +177,12 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	if (cpu_has_feature(CPU_FTR_DAWR)) {
 		length_max = 512 ; /* 64 doublewords */
 		/* DAWR region can't cross 512 boundary */
-		if ((bp->attr.bp_addr >> 10) != 
-		    ((bp->attr.bp_addr + bp->attr.bp_len - 1) >> 10))
+		if ((attr->bp_addr >> 10) !=
+		    ((attr->bp_addr + attr->bp_len - 1) >> 10))
 			return -EINVAL;

This will conflict with my next branch, but it should be easy enough to resolve.

arch/powerpc/kernel/hw_breakpoint.c int arch_validate_hwbkpt_settings(struct perf_event *bp) · line 188

Quoted 8 lines
arch/powerpc/kernel/hw_breakpoint.c +2 -2
 	}
-	if (info->len >
-	    (length_max - (info->address & HW_BREAKPOINT_ALIGN)))
+	if (hw->len >
+	    (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
 		return -EINVAL;
 	return 0;
 }
Acked-by Michael Ellerman <[email protected]>

cheers

author
#9 lore raw

Re: [PATCH 04/12] powerpc: Implement hw_breakpoint_arch_parse()

Quoted 18 lines · Michael Ellerman

arch/powerpc/kernel/hw_breakpoint.c int arch_bp_generic_fields(int type, int *gen_bp_type) · line 144

Quoted 12 lines · Frederic Weisbecker
arch/powerpc/kernel/hw_breakpoint.c +3 -1
index 348cac9..fba6527 100644
@@ -139,30 +139,31 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 /*
  * 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,
+			     struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)

I think the semantics here are that we are reading from bp/attr and writing to hw?

Quoted 2 lines

If so would some sprinkling of const on the first two parameters help make that clearer?

I seem to remember there was an issue with that due to the various functions we call that need to be converted to take const as well. I thought I would do it in a seperate series but actually it should be no big deal to do it on this one.

Let me try that and respin.

Quoted 6 lines
Quoted 6 lines
arch/powerpc/kernel/hw_breakpoint.c +0 -0
 	/*
 	 * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)

@@ -176,12 +177,12 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	if (cpu_has_feature(CPU_FTR_DAWR)) {
 		length_max = 512 ; /* 64 doublewords */
 		/* DAWR region can't cross 512 boundary */
Quoted 8 lines

arch/powerpc/kernel/hw_breakpoint.c int arch_validate_hwbkpt_settings(struct perf_event *bp) · line 182

Quoted 5 lines
arch/powerpc/kernel/hw_breakpoint.c +2 -2
-		if ((bp->attr.bp_addr >> 10) != 
-		    ((bp->attr.bp_addr + bp->attr.bp_len - 1) >> 10))
+		if ((attr->bp_addr >> 10) !=
+		    ((attr->bp_addr + attr->bp_len - 1) >> 10))
 			return -EINVAL;

This will conflict with my next branch, but it should be easy enough to resolve.

Ok.

Quoted 11 lines

arch/powerpc/kernel/hw_breakpoint.c int arch_validate_hwbkpt_settings(struct perf_event *bp) · line 188

Quoted 9 lines
arch/powerpc/kernel/hw_breakpoint.c +2 -2
 	}
-	if (info->len >
-	    (length_max - (info->address & HW_BREAKPOINT_ALIGN)))
+	if (hw->len >
+	    (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
 		return -EINVAL;
 	return 0;
 }
Acked-by Michael Ellerman <[email protected]>

Thanks!

reviewer
#10 lore raw

Re: [PATCH 04/12] powerpc: Implement hw_breakpoint_arch_parse()

Quoted 19 lines · Frederic Weisbecker
Quoted 18 lines · Michael Ellerman

arch/powerpc/kernel/hw_breakpoint.c int arch_bp_generic_fields(int type, int *gen_bp_type) · line 144

Quoted 12 lines · Frederic Weisbecker
arch/powerpc/kernel/hw_breakpoint.c +3 -1
index 348cac9..fba6527 100644
@@ -139,30 +139,31 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 /*
  * 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,
+			     struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)

I think the semantics here are that we are reading from bp/attr and writing to hw?

Quoted 7 lines
Quoted 2 lines

If so would some sprinkling of const on the first two parameters help make that clearer?

I seem to remember there was an issue with that due to the various functions we call that need to be converted to take const as well. I thought I would do it in a seperate series but actually it should be no big deal to do it on this one.

Yeah, that does sometimes snowball out of control.

Quoted 1 line

Let me try that and respin.

Cool. It would be nice to have, but obviously not crucial.

cheers

author
#11 lore raw

Re: [PATCH 04/12] powerpc: Implement hw_breakpoint_arch_parse()

Quoted 29 lines · Michael Ellerman
Quoted 19 lines · Frederic Weisbecker
Quoted 18 lines · Michael Ellerman

arch/powerpc/kernel/hw_breakpoint.c int arch_bp_generic_fields(int type, int *gen_bp_type) · line 144

Quoted 12 lines · Frederic Weisbecker
arch/powerpc/kernel/hw_breakpoint.c +3 -1
index 348cac9..fba6527 100644
@@ -139,30 +139,31 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 /*
  * 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,
+			     struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)

I think the semantics here are that we are reading from bp/attr and writing to hw?

Quoted 7 lines
Quoted 2 lines

If so would some sprinkling of const on the first two parameters help make that clearer?

I seem to remember there was an issue with that due to the various functions we call that need to be converted to take const as well. I thought I would do it in a seperate series but actually it should be no big deal to do it on this one.

Quoted 5 lines

Yeah, that does sometimes snowball out of control.

Quoted 1 line

Let me try that and respin.

Cool. It would be nice to have, but obviously not crucial.

So I managed to constify the perf_event_attr parameter but not the struct perf_event *bp because the task target is fetched from it on is_compat_bp() on ARM64. I could constify it all the way up to test_ti_thread_flag() but the thread info can only be retrieved through a call to task_thread_info() and that's where the qualifier control ends. The const can not be passed there and we can't afford to constify the function either, I fear, as it is used everywhere for any purpose, including thread_info modifications.

reviewer
#12 lore raw

Re: [PATCH 04/12] powerpc: Implement hw_breakpoint_arch_parse()

Quoted 34 lines · Frederic Weisbecker
Quoted 33 lines · Michael Ellerman
Quoted 19 lines · Frederic Weisbecker
Quoted 18 lines · Michael Ellerman

arch/powerpc/kernel/hw_breakpoint.c int arch_bp_generic_fields(int type, int *gen_bp_type) · line 144

Quoted 12 lines · Frederic Weisbecker
arch/powerpc/kernel/hw_breakpoint.c +3 -1
index 348cac9..fba6527 100644
@@ -139,30 +139,31 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 /*
  * 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,
+			     struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)

I think the semantics here are that we are reading from bp/attr and writing to hw?

Quoted 7 lines
Quoted 2 lines

If so would some sprinkling of const on the first two parameters help make that clearer?

I seem to remember there was an issue with that due to the various functions we call that need to be converted to take const as well. I thought I would do it in a seperate series but actually it should be no big deal to do it on this one.

Yeah, that does sometimes snowball out of control.

Quoted 1 line

Let me try that and respin.

Quoted 8 lines
Quoted 1 line

Cool. It would be nice to have, but obviously not crucial.

So I managed to constify the perf_event_attr parameter but not the struct perf_event *bp because the task target is fetched from it on is_compat_bp() on ARM64. I could constify it all the way up to test_ti_thread_flag() but the thread info can only be retrieved through a call to task_thread_info() and that's where the qualifier control ends. The const can not be passed there and we can't afford to constify the function either, I fear, as it is used everywhere for any purpose, including thread_info modifications.

Thanks for trying. 1 out of 2 const parameters is better than none :)

cheers

author
#13 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.

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..451544d 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,
+				    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
maintainer Acked-by on v1 5/12
#14 lore raw

Re: [PATCH 05/12] arm: Implement hw_breakpoint_arch_parse()

Quoted 24 lines · 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]>
Quoted 4 lines
---
 arch/arm/include/asm/hw_breakpoint.h |  6 ++-
 arch/arm/kernel/hw_breakpoint.c      | 71 ++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 36 deletions(-)

Looks fine to me.

Acked-by Mark Rutland <[email protected]>

Mark.

author
#15 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.

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..3b43319 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,
+				    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
Acked-by on v1 6/12
#16 lore raw

Re: [PATCH 06/12] arm64: Implement hw_breakpoint_arch_parse()

Quoted 24 lines · 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]>
Quoted 4 lines
---
 arch/arm64/include/asm/hw_breakpoint.h |  6 ++-
 arch/arm64/kernel/hw_breakpoint.c      | 79 +++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 40 deletions(-)

Looks fairly mechanical:

Acked-by Will Deacon <[email protected]>

Will

maintainer Acked-by on v1 6/12
#17 lore raw

Re: [PATCH 06/12] arm64: Implement hw_breakpoint_arch_parse()

In reply to Frederic Weisbecker
Quoted 24 lines · 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]>
Quoted 4 lines
---
 arch/arm64/include/asm/hw_breakpoint.h |  6 ++-
 arch/arm64/kernel/hw_breakpoint.c      | 79 +++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 40 deletions(-)

Looks fine to me.

Acked-by Mark Rutland <[email protected]>

Mark.

author
#18 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
#19 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..ed421dc 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,
+				    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..78a87b1 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,
+			     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,
+			     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
#20 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..58632d3 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,
+			     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..a0def44 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,
+			     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
#21 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 451544d..3533fbe 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,
 				    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 3b43319..d1a9c73 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,
 				    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 8f647e6..22226fe 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,
 				    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 ed421dc..4910990 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,
 				    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 8cc161f..525f8c7 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,
 				    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 58632d3..f3b6036 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,
 			     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 9ac03ab..d7ed438 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,
-			     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,
 			       struct perf_event_attr *attr,
 			       struct arch_hw_breakpoint *hw)
Signature · 1 line
2.7.4
author
#22 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 d7ed438..7840746 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
#23 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 7840746..1af761a 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