[PATCH 0/9] breakpoint: Rework arch validation

[PATCH 0/9] breakpoint: Rework arch validation

[Frederic Weisbecker]

This is the first round of breakpoint code rework and cleanup. Here we
split up architecture validation and commit so that we don't mess up
with architecture internals in case attributes are rejected or slot
can't be reserved. It also unconfuse the code in general.

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

HEAD: 0a788eff09988e58708c4a3ce7b8441ce3e7df95

Thanks,
	Frederic
---

Frederic Weisbecker (9):
      x86/breakpoint: Split validation into "check" and "commit"
      sh: Remove "struct arch_hw_breakpoint::name" unused field
      sh: Split breakpoint validation into "check" and "commit"
      arm: Split breakpoint validation into "check" and "commit"
      xtensa: Split breakpoint validation into "check" and "commit"
      arm64: Split breakpoint validation into "check" and "commit"
      powerpc: Split breakpoint validation into "check" and "commit"
      perf/breakpoint: Split breakpoint "check" and "commit"
      perf/breakpoint: Only commit breakpoint to arch upon slot reservation success


 arch/arm/include/asm/hw_breakpoint.h     |   5 +-
 arch/arm/kernel/hw_breakpoint.c          | 160 +++++++++++++++-------------
 arch/arm64/include/asm/hw_breakpoint.h   |   5 +-
 arch/arm64/kernel/hw_breakpoint.c        | 173 +++++++++++++++++++------------
 arch/powerpc/include/asm/hw_breakpoint.h |   5 +-
 arch/powerpc/kernel/hw_breakpoint.c      |  66 ++++++------
 arch/sh/include/asm/hw_breakpoint.h      |   6 +-
 arch/sh/kernel/hw_breakpoint.c           | 102 ++++++++----------
 arch/x86/include/asm/hw_breakpoint.h     |   5 +-
 arch/x86/kernel/hw_breakpoint.c          | 140 ++++++++++++-------------
 arch/xtensa/include/asm/hw_breakpoint.h  |   5 +-
 arch/xtensa/kernel/hw_breakpoint.c       |  54 ++++++----
 kernel/events/hw_breakpoint.c            |  78 ++++++++------
 13 files changed, 443 insertions(+), 361 deletions(-)

[PATCH 1/9] x86/breakpoint: Split validation into "check" and "commit"

[Frederic Weisbecker]

The breakpoint code 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 struct.

Prepare fox fixing this misdesign and separate both logics.

Original-patch-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 137 +++++++++++++++++++++-------------------
 1 file changed, 72 insertions(+), 65 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 8771766..6960800 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -233,20 +233,15 @@ int arch_bp_generic_fields(int x86_len, int x86_type,
 	return 0;
 }
 
-
-static int arch_build_bp_info(struct perf_event *bp)
+static int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	unsigned int align;
 
-	info->address = bp->attr.bp_addr;
-
-	/* Type */
-	switch (bp->attr.bp_type) {
+	/* Check type */
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_W:
-		info->type = X86_BREAKPOINT_WRITE;
-		break;
 	case HW_BREAKPOINT_W | HW_BREAKPOINT_R:
-		info->type = X86_BREAKPOINT_RW;
 		break;
 	case HW_BREAKPOINT_X:
 		/*
@@ -254,33 +249,84 @@ static int arch_build_bp_info(struct perf_event *bp)
 		 * acceptable for kprobes.  On non-kprobes kernels, we don't
 		 * allow kernel breakpoints at all.
 		 */
-		if (bp->attr.bp_addr >= TASK_SIZE_MAX) {
+		if (attr->bp_addr >= TASK_SIZE_MAX) {
 #ifdef CONFIG_KPROBES
-			if (within_kprobe_blacklist(bp->attr.bp_addr))
+			if (within_kprobe_blacklist(attr->bp_addr))
 				return -EINVAL;
 #else
 			return -EINVAL;
 #endif
 		}
 
+		if (attr->bp_len == sizeof(long))
+			return 0;
+	default:
+		return -EINVAL;
+	}
+
+	/* Check len */
+	switch (attr->bp_len) {
+	case HW_BREAKPOINT_LEN_1:
+	case HW_BREAKPOINT_LEN_2:
+	case HW_BREAKPOINT_LEN_4:
+		break;
+#ifdef CONFIG_X86_64
+	case HW_BREAKPOINT_LEN_8:
+		break;
+	default:
+		/* AMD range breakpoint */
+		if (!is_power_of_2(attr->bp_len))
+			return -EINVAL;
+
+		if (!boot_cpu_has(X86_FEATURE_BPEXT))
+			return -EOPNOTSUPP;
+#endif
+	}
+
+	align = attr->bp_len - 1;
+
+	/*
+	 * Check that the low-order bits of the address are appropriate
+	 * for the alignment implied by len.
+	 */
+	if (attr->bp_addr & align)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void hw_breakpoint_arch_commit(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event_attr *attr = &bp->attr;
+
+	info->address = attr->bp_addr;
+
+	/* Set type */
+	switch (attr->bp_type) {
+	case HW_BREAKPOINT_W:
+		info->type = X86_BREAKPOINT_WRITE;
+		break;
+	case HW_BREAKPOINT_W | HW_BREAKPOINT_R:
+		info->type = X86_BREAKPOINT_RW;
+		break;
+	case HW_BREAKPOINT_X:
 		info->type = X86_BREAKPOINT_EXECUTE;
 		/*
 		 * x86 inst breakpoints need to have a specific undefined len.
 		 * But we still need to check userspace is not trying to setup
 		 * an unsupported length, to get a range breakpoint for example.
 		 */
-		if (bp->attr.bp_len == sizeof(long)) {
-			info->len = X86_BREAKPOINT_LEN_X;
-			return 0;
-		}
+		info->len = X86_BREAKPOINT_LEN_X;
+		return;
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
 	}
 
-	/* Len */
+	/* Set len */
 	info->mask = 0;
 
-	switch (bp->attr.bp_len) {
+	switch (attr->bp_len) {
 	case HW_BREAKPOINT_LEN_1:
 		info->len = X86_BREAKPOINT_LEN_1;
 		break;
@@ -296,15 +342,6 @@ static int arch_build_bp_info(struct perf_event *bp)
 		break;
 #endif
 	default:
-		/* AMD range breakpoint */
-		if (!is_power_of_2(bp->attr.bp_len))
-			return -EINVAL;
-		if (bp->attr.bp_addr & (bp->attr.bp_len - 1))
-			return -EINVAL;
-
-		if (!boot_cpu_has(X86_FEATURE_BPEXT))
-			return -EOPNOTSUPP;
-
 		/*
 		 * It's impossible to use a range breakpoint to fake out
 		 * user vs kernel detection because bp_len - 1 can't
@@ -312,54 +349,24 @@ static int arch_build_bp_info(struct perf_event *bp)
 		 * breakpoints, then we'll have to check for kprobe-blacklisted
 		 * addresses anywhere in the range.
 		 */
-		info->mask = bp->attr.bp_len - 1;
+		info->mask = attr->bp_len - 1;
 		info->len = X86_BREAKPOINT_LEN_1;
 	}
-
-	return 0;
 }
 
+
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
 int arch_validate_hwbkpt_settings(struct perf_event *bp)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	unsigned int align;
-	int ret;
+	int err;
 
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
 
-	ret = arch_build_bp_info(bp);
-	if (ret)
-		return ret;
-
-	switch (info->len) {
-	case X86_BREAKPOINT_LEN_1:
-		align = 0;
-		if (info->mask)
-			align = info->mask;
-		break;
-	case X86_BREAKPOINT_LEN_2:
-		align = 1;
-		break;
-	case X86_BREAKPOINT_LEN_4:
-		align = 3;
-		break;
-#ifdef CONFIG_X86_64
-	case X86_BREAKPOINT_LEN_8:
-		align = 7;
-		break;
-#endif
-	default:
-		WARN_ON_ONCE(1);
-	}
-
-	/*
-	 * Check that the low-order bits of the address are appropriate
-	 * for the alignment implied by len.
-	 */
-	if (info->address & align)
-		return -EINVAL;
+	hw_breakpoint_arch_commit(bp);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 2/9] sh: Remove "struct arch_hw_breakpoint::name" unused field

[Frederic Weisbecker]

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

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/sh/include/asm/hw_breakpoint.h | 1 -
 arch/sh/kernel/hw_breakpoint.c      | 7 -------
 2 files changed, 8 deletions(-)

diff --git a/arch/sh/include/asm/hw_breakpoint.h b/arch/sh/include/asm/hw_breakpoint.h
index 7431c17..68384e6 100644
--- a/arch/sh/include/asm/hw_breakpoint.h
+++ b/arch/sh/include/asm/hw_breakpoint.h
@@ -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;
diff --git a/arch/sh/kernel/hw_breakpoint.c b/arch/sh/kernel/hw_breakpoint.c
index afe9657..286c3ab 100644
--- a/arch/sh/kernel/hw_breakpoint.c
+++ b/arch/sh/kernel/hw_breakpoint.c
@@ -249,13 +249,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.
 	 */
-- 
2.7.4

[PATCH 3/9] sh: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

The breakpoint code 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 struct.

Prepare fox fixing this misdesign and separate both logics.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/sh/kernel/hw_breakpoint.c | 89 ++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/arch/sh/kernel/hw_breakpoint.c b/arch/sh/kernel/hw_breakpoint.c
index 286c3ab..c89152f 100644
--- a/arch/sh/kernel/hw_breakpoint.c
+++ b/arch/sh/kernel/hw_breakpoint.c
@@ -174,14 +174,53 @@ 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 hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr)
+{
+	unsigned int align;
+
+	/* Check type */
+	switch (attr->bp_type) {
+	case HW_BREAKPOINT_W:
+	case HW_BREAKPOINT_R:
+	case HW_BREAKPOINT_W | HW_BREAKPOINT_R:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Check len */
+	switch (attr->bp_len) {
+	case HW_BREAKPOINT_LEN_1:
+	case HW_BREAKPOINT_LEN_2:
+	case HW_BREAKPOINT_LEN_4:
+	case HW_BREAKPOINT_LEN_8:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	align = attr->bp_len - 1;
+
+	/*
+	 * Check that the low-order bits of the address are appropriate
+	 * for the alignment implied by len.
+	 */
+	if (attr->bp_addr & align)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event_attr *attr = &bp->attr;
 
-	info->address = bp->attr.bp_addr;
+	info->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;
 		break;
@@ -195,11 +234,11 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->len = SH_BREAKPOINT_LEN_8;
 		break;
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
 	}
 
 	/* Type */
-	switch (bp->attr.bp_type) {
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_R:
 		info->type = SH_BREAKPOINT_READ;
 		break;
@@ -210,10 +249,8 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->type = SH_BREAKPOINT_RW;
 		break;
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
 	}
-
-	return 0;
 }
 
 /*
@@ -221,39 +258,13 @@ static int arch_build_bp_info(struct perf_event *bp)
  */
 int arch_validate_hwbkpt_settings(struct perf_event *bp)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	unsigned int align;
-	int ret;
+	int err;
 
-	ret = arch_build_bp_info(bp);
-	if (ret)
-		return ret;
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
 
-	ret = -EINVAL;
-
-	switch (info->len) {
-	case SH_BREAKPOINT_LEN_1:
-		align = 0;
-		break;
-	case SH_BREAKPOINT_LEN_2:
-		align = 1;
-		break;
-	case SH_BREAKPOINT_LEN_4:
-		align = 3;
-		break;
-	case SH_BREAKPOINT_LEN_8:
-		align = 7;
-		break;
-	default:
-		return ret;
-	}
-
-	/*
-	 * Check that the low-order bits of the address are appropriate
-	 * for the alignment implied by len.
-	 */
-	if (info->address & align)
-		return -EINVAL;
+	hw_breakpoint_arch_commit(bp);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

The breakpoint code 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 struct.

Prepare fox fixing this misdesign and separate both logics.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/arm/kernel/hw_breakpoint.c | 176 +++++++++++++++++++++++-----------------
 1 file changed, 103 insertions(+), 73 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 629e251..1769d3a 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -515,45 +515,33 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 	return 0;
 }
 
-/*
- * Construct an arch_hw_breakpoint from a perf_event.
- */
-static int arch_build_bp_info(struct perf_event *bp)
+static int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr)
 {
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	u32 offset, alignment_mask = 0x3;
+
+	/* Ensure that we are in monitor debug mode. */
+	if (!monitor_mode_enabled())
+		return -ENODEV;
 
 	/* Type */
-	switch (bp->attr.bp_type) {
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_X:
-		info->ctrl.type = ARM_BREAKPOINT_EXECUTE;
-		break;
 	case HW_BREAKPOINT_R:
-		info->ctrl.type = ARM_BREAKPOINT_LOAD;
-		break;
 	case HW_BREAKPOINT_W:
-		info->ctrl.type = ARM_BREAKPOINT_STORE;
-		break;
 	case HW_BREAKPOINT_RW:
-		info->ctrl.type = ARM_BREAKPOINT_LOAD | ARM_BREAKPOINT_STORE;
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	/* Len */
-	switch (bp->attr.bp_len) {
+	switch (attr->bp_len) {
 	case HW_BREAKPOINT_LEN_1:
-		info->ctrl.len = ARM_BREAKPOINT_LEN_1;
-		break;
 	case HW_BREAKPOINT_LEN_2:
-		info->ctrl.len = ARM_BREAKPOINT_LEN_2;
-		break;
 	case HW_BREAKPOINT_LEN_4:
-		info->ctrl.len = ARM_BREAKPOINT_LEN_4;
-		break;
 	case HW_BREAKPOINT_LEN_8:
-		info->ctrl.len = ARM_BREAKPOINT_LEN_8;
-		if ((info->ctrl.type != ARM_BREAKPOINT_EXECUTE)
+		if ((attr->bp_type != HW_BREAKPOINT_X)
 			&& max_watchpoint_len >= 8)
 			break;
 	default:
@@ -566,50 +554,17 @@ static int arch_build_bp_info(struct perf_event *bp)
 	 * by the hardware and must be aligned to the appropriate number of
 	 * bytes.
 	 */
-	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE &&
-	    info->ctrl.len != ARM_BREAKPOINT_LEN_2 &&
-	    info->ctrl.len != ARM_BREAKPOINT_LEN_4)
+	if (attr->bp_type == HW_BREAKPOINT_X &&
+	    attr->bp_len != HW_BREAKPOINT_LEN_2 &&
+	    attr->bp_len != HW_BREAKPOINT_LEN_4)
 		return -EINVAL;
 
-	/* Address */
-	info->address = bp->attr.bp_addr;
-
-	/* Privilege */
-	info->ctrl.privilege = ARM_BREAKPOINT_USER;
-	if (arch_check_bp_in_kernelspace(bp))
-		info->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
-
-	/* Enabled? */
-	info->ctrl.enabled = !bp->attr.disabled;
-
-	/* Mismatch */
-	info->ctrl.mismatch = 0;
-
-	return 0;
-}
-
-/*
- * Validate the arch-specific HW Breakpoint register settings.
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	int ret = 0;
-	u32 offset, alignment_mask = 0x3;
-
-	/* Ensure that we are in monitor debug mode. */
-	if (!monitor_mode_enabled())
-		return -ENODEV;
-
-	/* Build the arch_hw_breakpoint. */
-	ret = arch_build_bp_info(bp);
-	if (ret)
-		goto out;
-
 	/* Check address alignment. */
-	if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
+	if (attr->bp_len == HW_BREAKPOINT_LEN_8)
 		alignment_mask = 0x7;
-	offset = info->address & alignment_mask;
+
+	offset = attr->bp_addr & alignment_mask;
+
 	switch (offset) {
 	case 0:
 		/* Aligned */
@@ -617,20 +572,16 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	case 1:
 	case 2:
 		/* Allow halfword watchpoints and breakpoints. */
-		if (info->ctrl.len == ARM_BREAKPOINT_LEN_2)
+		if (attr->bp_len == HW_BREAKPOINT_LEN_2)
 			break;
 	case 3:
 		/* Allow single byte watchpoint. */
-		if (info->ctrl.len == ARM_BREAKPOINT_LEN_1)
+		if (attr->bp_len == HW_BREAKPOINT_LEN_1)
 			break;
 	default:
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
-	info->address &= ~alignment_mask;
-	info->ctrl.len <<= offset;
-
 	if (is_default_overflow_handler(bp)) {
 		/*
 		 * Mismatch breakpoints are required for single-stepping
@@ -655,13 +606,92 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		 * reports them.
 		 */
 		if (!debug_exception_updates_fsr() &&
-		    (info->ctrl.type == ARM_BREAKPOINT_LOAD ||
-		     info->ctrl.type == ARM_BREAKPOINT_STORE))
+		    (attr->bp_type == HW_BREAKPOINT_R ||
+		     attr->bp_type == HW_BREAKPOINT_W))
 			return -EINVAL;
 	}
 
-out:
-	return ret;
+	return 0;
+}
+
+static void hw_breakpoint_arch_commit(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event_attr *attr = &bp->attr;
+	u32 offset, alignment_mask = 0x3;
+
+	/* Type */
+	switch (attr->bp_type) {
+	case HW_BREAKPOINT_X:
+		info->ctrl.type = ARM_BREAKPOINT_EXECUTE;
+		break;
+	case HW_BREAKPOINT_R:
+		info->ctrl.type = ARM_BREAKPOINT_LOAD;
+		break;
+	case HW_BREAKPOINT_W:
+		info->ctrl.type = ARM_BREAKPOINT_STORE;
+		break;
+	case HW_BREAKPOINT_RW:
+		info->ctrl.type = ARM_BREAKPOINT_LOAD | ARM_BREAKPOINT_STORE;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+	}
+
+	/* Len */
+	switch (attr->bp_len) {
+	case HW_BREAKPOINT_LEN_1:
+		info->ctrl.len = ARM_BREAKPOINT_LEN_1;
+		break;
+	case HW_BREAKPOINT_LEN_2:
+		info->ctrl.len = ARM_BREAKPOINT_LEN_2;
+		break;
+	case HW_BREAKPOINT_LEN_4:
+		info->ctrl.len = ARM_BREAKPOINT_LEN_4;
+		break;
+	case HW_BREAKPOINT_LEN_8:
+		info->ctrl.len = ARM_BREAKPOINT_LEN_8;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+	}
+
+	/* Address */
+	info->address = attr->bp_addr;
+
+	/* Privilege */
+	info->ctrl.privilege = ARM_BREAKPOINT_USER;
+	if (arch_check_bp_in_kernelspace(bp))
+		info->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
+
+	/* Enabled? */
+	info->ctrl.enabled = !attr->disabled;
+
+	/* Mismatch */
+	info->ctrl.mismatch = 0;
+
+	if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
+		alignment_mask = 0x7;
+
+	offset = info->address & alignment_mask;
+	info->address &= ~alignment_mask;
+	info->ctrl.len <<= offset;
+}
+
+/*
+ * Validate the arch-specific HW Breakpoint register settings.
+ */
+int arch_validate_hwbkpt_settings(struct perf_event *bp)
+{
+	int err;
+
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
+
+	hw_breakpoint_arch_commit(bp);
+
+	return 0;
 }
 
 /*
-- 
2.7.4

Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Mark Rutland]

Hi Frederick,

On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> The breakpoint code 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 struct.
> 
> Prepare fox fixing this misdesign and separate both logics.

Could you elaborate on what the problem is? I would have expected that
when arch_build_bp_info() returns an error code, we wouldn't
subsequently use the arch_hw_breakpoint information. Where does that
happen?

I understand that there was a problem on x86 -- I'm just having
difficulty figuring it out.

I also see that the check and commit hooks have to duplicate a
reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
instead refactor the existing arch_build_bp_info() hooks to use a
temporary arch_hw_breakpoint, and then struct assign it after all the
error cases, e.g.

static int arch_build_bp_info(struct perf_event *bp)
{
	struct arch_hw_breakpoint hbp;
	
	if (some_condition(bp))
		hbp->field = 0xf00;

	switch (bp->attr.type) {
	case FOO:
		return -EINVAL;
	case BAR:
		hbp->other_field = 7;
		break;
	};

	if (failure_case(foo))
		return err;
	
	*counter_arch_bp(bp) = hbp;
}

... or is that also problematic?

Thanks,
Mark.

> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Rich Felker <dalias@libc.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> ---
>  arch/arm/kernel/hw_breakpoint.c | 176 +++++++++++++++++++++++-----------------
>  1 file changed, 103 insertions(+), 73 deletions(-)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index 629e251..1769d3a 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -515,45 +515,33 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>  	return 0;
>  }
>  
> -/*
> - * Construct an arch_hw_breakpoint from a perf_event.
> - */
> -static int arch_build_bp_info(struct perf_event *bp)
> +static int hw_breakpoint_arch_check(struct perf_event *bp,
> +				    const struct perf_event_attr *attr)
>  {
> -	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +	u32 offset, alignment_mask = 0x3;
> +
> +	/* Ensure that we are in monitor debug mode. */
> +	if (!monitor_mode_enabled())
> +		return -ENODEV;
>  
>  	/* Type */
> -	switch (bp->attr.bp_type) {
> +	switch (attr->bp_type) {
>  	case HW_BREAKPOINT_X:
> -		info->ctrl.type = ARM_BREAKPOINT_EXECUTE;
> -		break;
>  	case HW_BREAKPOINT_R:
> -		info->ctrl.type = ARM_BREAKPOINT_LOAD;
> -		break;
>  	case HW_BREAKPOINT_W:
> -		info->ctrl.type = ARM_BREAKPOINT_STORE;
> -		break;
>  	case HW_BREAKPOINT_RW:
> -		info->ctrl.type = ARM_BREAKPOINT_LOAD | ARM_BREAKPOINT_STORE;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
>  	/* Len */
> -	switch (bp->attr.bp_len) {
> +	switch (attr->bp_len) {
>  	case HW_BREAKPOINT_LEN_1:
> -		info->ctrl.len = ARM_BREAKPOINT_LEN_1;
> -		break;
>  	case HW_BREAKPOINT_LEN_2:
> -		info->ctrl.len = ARM_BREAKPOINT_LEN_2;
> -		break;
>  	case HW_BREAKPOINT_LEN_4:
> -		info->ctrl.len = ARM_BREAKPOINT_LEN_4;
> -		break;
>  	case HW_BREAKPOINT_LEN_8:
> -		info->ctrl.len = ARM_BREAKPOINT_LEN_8;
> -		if ((info->ctrl.type != ARM_BREAKPOINT_EXECUTE)
> +		if ((attr->bp_type != HW_BREAKPOINT_X)
>  			&& max_watchpoint_len >= 8)
>  			break;
>  	default:
> @@ -566,50 +554,17 @@ static int arch_build_bp_info(struct perf_event *bp)
>  	 * by the hardware and must be aligned to the appropriate number of
>  	 * bytes.
>  	 */
> -	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE &&
> -	    info->ctrl.len != ARM_BREAKPOINT_LEN_2 &&
> -	    info->ctrl.len != ARM_BREAKPOINT_LEN_4)
> +	if (attr->bp_type == HW_BREAKPOINT_X &&
> +	    attr->bp_len != HW_BREAKPOINT_LEN_2 &&
> +	    attr->bp_len != HW_BREAKPOINT_LEN_4)
>  		return -EINVAL;
>  
> -	/* Address */
> -	info->address = bp->attr.bp_addr;
> -
> -	/* Privilege */
> -	info->ctrl.privilege = ARM_BREAKPOINT_USER;
> -	if (arch_check_bp_in_kernelspace(bp))
> -		info->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
> -
> -	/* Enabled? */
> -	info->ctrl.enabled = !bp->attr.disabled;
> -
> -	/* Mismatch */
> -	info->ctrl.mismatch = 0;
> -
> -	return 0;
> -}
> -
> -/*
> - * Validate the arch-specific HW Breakpoint register settings.
> - */
> -int arch_validate_hwbkpt_settings(struct perf_event *bp)
> -{
> -	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> -	int ret = 0;
> -	u32 offset, alignment_mask = 0x3;
> -
> -	/* Ensure that we are in monitor debug mode. */
> -	if (!monitor_mode_enabled())
> -		return -ENODEV;
> -
> -	/* Build the arch_hw_breakpoint. */
> -	ret = arch_build_bp_info(bp);
> -	if (ret)
> -		goto out;
> -
>  	/* Check address alignment. */
> -	if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
> +	if (attr->bp_len == HW_BREAKPOINT_LEN_8)
>  		alignment_mask = 0x7;
> -	offset = info->address & alignment_mask;
> +
> +	offset = attr->bp_addr & alignment_mask;
> +
>  	switch (offset) {
>  	case 0:
>  		/* Aligned */
> @@ -617,20 +572,16 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>  	case 1:
>  	case 2:
>  		/* Allow halfword watchpoints and breakpoints. */
> -		if (info->ctrl.len == ARM_BREAKPOINT_LEN_2)
> +		if (attr->bp_len == HW_BREAKPOINT_LEN_2)
>  			break;
>  	case 3:
>  		/* Allow single byte watchpoint. */
> -		if (info->ctrl.len == ARM_BREAKPOINT_LEN_1)
> +		if (attr->bp_len == HW_BREAKPOINT_LEN_1)
>  			break;
>  	default:
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>  	}
>  
> -	info->address &= ~alignment_mask;
> -	info->ctrl.len <<= offset;
> -
>  	if (is_default_overflow_handler(bp)) {
>  		/*
>  		 * Mismatch breakpoints are required for single-stepping
> @@ -655,13 +606,92 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>  		 * reports them.
>  		 */
>  		if (!debug_exception_updates_fsr() &&
> -		    (info->ctrl.type == ARM_BREAKPOINT_LOAD ||
> -		     info->ctrl.type == ARM_BREAKPOINT_STORE))
> +		    (attr->bp_type == HW_BREAKPOINT_R ||
> +		     attr->bp_type == HW_BREAKPOINT_W))
>  			return -EINVAL;
>  	}
>  
> -out:
> -	return ret;
> +	return 0;
> +}
> +
> +static void hw_breakpoint_arch_commit(struct perf_event *bp)
> +{
> +	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +	struct perf_event_attr *attr = &bp->attr;
> +	u32 offset, alignment_mask = 0x3;
> +
> +	/* Type */
> +	switch (attr->bp_type) {
> +	case HW_BREAKPOINT_X:
> +		info->ctrl.type = ARM_BREAKPOINT_EXECUTE;
> +		break;
> +	case HW_BREAKPOINT_R:
> +		info->ctrl.type = ARM_BREAKPOINT_LOAD;
> +		break;
> +	case HW_BREAKPOINT_W:
> +		info->ctrl.type = ARM_BREAKPOINT_STORE;
> +		break;
> +	case HW_BREAKPOINT_RW:
> +		info->ctrl.type = ARM_BREAKPOINT_LOAD | ARM_BREAKPOINT_STORE;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +	}
> +
> +	/* Len */
> +	switch (attr->bp_len) {
> +	case HW_BREAKPOINT_LEN_1:
> +		info->ctrl.len = ARM_BREAKPOINT_LEN_1;
> +		break;
> +	case HW_BREAKPOINT_LEN_2:
> +		info->ctrl.len = ARM_BREAKPOINT_LEN_2;
> +		break;
> +	case HW_BREAKPOINT_LEN_4:
> +		info->ctrl.len = ARM_BREAKPOINT_LEN_4;
> +		break;
> +	case HW_BREAKPOINT_LEN_8:
> +		info->ctrl.len = ARM_BREAKPOINT_LEN_8;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +	}
> +
> +	/* Address */
> +	info->address = attr->bp_addr;
> +
> +	/* Privilege */
> +	info->ctrl.privilege = ARM_BREAKPOINT_USER;
> +	if (arch_check_bp_in_kernelspace(bp))
> +		info->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
> +
> +	/* Enabled? */
> +	info->ctrl.enabled = !attr->disabled;
> +
> +	/* Mismatch */
> +	info->ctrl.mismatch = 0;
> +
> +	if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
> +		alignment_mask = 0x7;
> +
> +	offset = info->address & alignment_mask;
> +	info->address &= ~alignment_mask;
> +	info->ctrl.len <<= offset;
> +}
> +
> +/*
> + * Validate the arch-specific HW Breakpoint register settings.
> + */
> +int arch_validate_hwbkpt_settings(struct perf_event *bp)
> +{
> +	int err;
> +
> +	err = hw_breakpoint_arch_check(bp, &bp->attr);
> +	if (err)
> +		return err;
> +
> +	hw_breakpoint_arch_commit(bp);
> +
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.7.4
> 

Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Mark Rutland]

On Tue, May 08, 2018 at 12:13:23PM +0100, Mark Rutland wrote:
> Hi Frederick,

Argh, sorry for the typo -- I realise that K should not be there.

Mark.

> On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> > The breakpoint code 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 struct.
> > 
> > Prepare fox fixing this misdesign and separate both logics.
> 
> Could you elaborate on what the problem is? I would have expected that
> when arch_build_bp_info() returns an error code, we wouldn't
> subsequently use the arch_hw_breakpoint information. Where does that
> happen?
> 
> I understand that there was a problem on x86 -- I'm just having
> difficulty figuring it out.
> 
> I also see that the check and commit hooks have to duplicate a
> reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
> instead refactor the existing arch_build_bp_info() hooks to use a
> temporary arch_hw_breakpoint, and then struct assign it after all the
> error cases, e.g.
> 
> static int arch_build_bp_info(struct perf_event *bp)
> {
> 	struct arch_hw_breakpoint hbp;
> 	
> 	if (some_condition(bp))
> 		hbp->field = 0xf00;
> 
> 	switch (bp->attr.type) {
> 	case FOO:
> 		return -EINVAL;
> 	case BAR:
> 		hbp->other_field = 7;
> 		break;
> 	};
> 
> 	if (failure_case(foo))
> 		return err;
> 	
> 	*counter_arch_bp(bp) = hbp;
> }
> 
> ... or is that also problematic?
> 
> Thanks,
> Mark.

Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Mark Rutland]

On Tue, May 08, 2018 at 12:13:23PM +0100, Mark Rutland wrote:
> Hi Frederick,
> 
> On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> > The breakpoint code 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 struct.
> > 
> > Prepare fox fixing this misdesign and separate both logics.
> 
> Could you elaborate on what the problem is? I would have expected that
> when arch_build_bp_info() returns an error code, we wouldn't
> subsequently use the arch_hw_breakpoint information. Where does that
> happen?

>From digging, I now see that this is a problem when
modify_user_hw_breakpoint() is called on an existing breakpoint. It
would be nice to mention that in the commit message.

> I also see that the check and commit hooks have to duplicate a
> reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
> instead refactor the existing arch_build_bp_info() hooks to use a
> temporary arch_hw_breakpoint, and then struct assign it after all the
> error cases, > e.g.
> 
> static int arch_build_bp_info(struct perf_event *bp)
> {
> 	struct arch_hw_breakpoint hbp;
> 	
> 	if (some_condition(bp))
> 		hbp->field = 0xf00;
> 
> 	switch (bp->attr.type) {
> 	case FOO:
> 		return -EINVAL;
> 	case BAR:
> 		hbp->other_field = 7;
> 		break;
> 	};
> 
> 	if (failure_case(foo))
> 		return err;
> 	
> 	*counter_arch_bp(bp) = hbp;
> }
> 
> ... or is that also problematic?

IIUC, this *would* work, but it is a little opaque.

Perhaps we could explicitly pass the temporary arch_hw_breakpoint in,
and have the core code struct-assign it after checking for errors?

Thanks,
Mark.

Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Andy Lutomirski]

On Wed, May 9, 2018 at 4:33 AM Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, May 08, 2018 at 12:13:23PM +0100, Mark Rutland wrote:
> > Hi Frederick,
> >
> > On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> > > The breakpoint code 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 struct.
> > >
> > > Prepare fox fixing this misdesign and separate both logics.
> >
> > Could you elaborate on what the problem is? I would have expected that
> > when arch_build_bp_info() returns an error code, we wouldn't
> > subsequently use the arch_hw_breakpoint information. Where does that
> > happen?

>  From digging, I now see that this is a problem when
> modify_user_hw_breakpoint() is called on an existing breakpoint. It
> would be nice to mention that in the commit message.

> > I also see that the check and commit hooks have to duplicate a
> > reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
> > instead refactor the existing arch_build_bp_info() hooks to use a
> > temporary arch_hw_breakpoint, and then struct assign it after all the
> > error cases, > e.g.
> >
> > static int arch_build_bp_info(struct perf_event *bp)
> > {
> >       struct arch_hw_breakpoint hbp;
> >
> >       if (some_condition(bp))
> >               hbp->field = 0xf00;
> >
> >       switch (bp->attr.type) {
> >       case FOO:
> >               return -EINVAL;
> >       case BAR:
> >               hbp->other_field = 7;
> >               break;
> >       };
> >
> >       if (failure_case(foo))
> >               return err;
> >
> >       *counter_arch_bp(bp) = hbp;
> > }
> >
> > ... or is that also problematic?

> IIUC, this *would* work, but it is a little opaque.

> Perhaps we could explicitly pass the temporary arch_hw_breakpoint in,
> and have the core code struct-assign it after checking for errors?

Hmm, maybe.  OTOH, I'm not really convinced that arch_hw_breakpoint is even
needed.  x86 at least could probably just regenerate the DRn and DR7 bits
on the fly as needed rather than caching them with basically no loss in
performance.  I completely agree that reducing duplication would be nice.

Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

On Wed, May 09, 2018 at 07:51:28PM +0000, Andy Lutomirski wrote:
> On Wed, May 9, 2018 at 4:33 AM Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Tue, May 08, 2018 at 12:13:23PM +0100, Mark Rutland wrote:
> > > Hi Frederick,
> > >
> > > On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> > > > The breakpoint code 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 struct.
> > > >
> > > > Prepare fox fixing this misdesign and separate both logics.
> > >
> > > Could you elaborate on what the problem is? I would have expected that
> > > when arch_build_bp_info() returns an error code, we wouldn't
> > > subsequently use the arch_hw_breakpoint information. Where does that
> > > happen?
> 
> >  From digging, I now see that this is a problem when
> > modify_user_hw_breakpoint() is called on an existing breakpoint. It
> > would be nice to mention that in the commit message.
> 
> > > I also see that the check and commit hooks have to duplicate a
> > > reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
> > > instead refactor the existing arch_build_bp_info() hooks to use a
> > > temporary arch_hw_breakpoint, and then struct assign it after all the
> > > error cases, > e.g.
> > >
> > > static int arch_build_bp_info(struct perf_event *bp)
> > > {
> > >       struct arch_hw_breakpoint hbp;
> > >
> > >       if (some_condition(bp))
> > >               hbp->field = 0xf00;
> > >
> > >       switch (bp->attr.type) {
> > >       case FOO:
> > >               return -EINVAL;
> > >       case BAR:
> > >               hbp->other_field = 7;
> > >               break;
> > >       };
> > >
> > >       if (failure_case(foo))
> > >               return err;
> > >
> > >       *counter_arch_bp(bp) = hbp;
> > > }
> > >
> > > ... or is that also problematic?
> 
> > IIUC, this *would* work, but it is a little opaque.
> 
> > Perhaps we could explicitly pass the temporary arch_hw_breakpoint in,
> > and have the core code struct-assign it after checking for errors?
> 
> Hmm, maybe.  OTOH, I'm not really convinced that arch_hw_breakpoint is even
> needed.  x86 at least could probably just regenerate the DRn and DR7 bits
> on the fly as needed rather than caching them with basically no loss in
> performance.

I'm not sure, we would need to translate the length and types everytime we
schedule in/out a perf breakpoint event. Maybe it's not too much a big deal
but perf event sched in/out is something I would consider a fast path and
there is quite a few switch/case involved there.

Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

On Wed, May 09, 2018 at 12:32:57PM +0100, Mark Rutland wrote:
> On Tue, May 08, 2018 at 12:13:23PM +0100, Mark Rutland wrote:
> > Hi Frederick,
> > 
> > On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> > > The breakpoint code 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 struct.
> > > 
> > > Prepare fox fixing this misdesign and separate both logics.
> > 
> > Could you elaborate on what the problem is? I would have expected that
> > when arch_build_bp_info() returns an error code, we wouldn't
> > subsequently use the arch_hw_breakpoint information. Where does that
> > happen?
> 
> From digging, I now see that this is a problem when
> modify_user_hw_breakpoint() is called on an existing breakpoint. It
> would be nice to mention that in the commit message.

Right, I'll improve the changelog.

> 
> > I also see that the check and commit hooks have to duplicate a
> > reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
> > instead refactor the existing arch_build_bp_info() hooks to use a
> > temporary arch_hw_breakpoint, and then struct assign it after all the
> > error cases, > e.g.
> > 
> > static int arch_build_bp_info(struct perf_event *bp)
> > {
> > 	struct arch_hw_breakpoint hbp;
> > 	
> > 	if (some_condition(bp))
> > 		hbp->field = 0xf00;
> > 
> > 	switch (bp->attr.type) {
> > 	case FOO:
> > 		return -EINVAL;
> > 	case BAR:
> > 		hbp->other_field = 7;
> > 		break;
> > 	};
> > 
> > 	if (failure_case(foo))
> > 		return err;
> > 	
> > 	*counter_arch_bp(bp) = hbp;
> > }
> > 
> > ... or is that also problematic?
> 
> IIUC, this *would* work, but it is a little opaque.
> 
> Perhaps we could explicitly pass the temporary arch_hw_breakpoint in,
> and have the core code struct-assign it after checking for errors?

Exactly, that looks like a good idea, I'm trying that.

Thanks.

[PATCH 5/9] xtensa: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

The breakpoint code 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 struct.

Prepare fox fixing this misdesign and separate both logics.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/xtensa/kernel/hw_breakpoint.c | 60 ++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/arch/xtensa/kernel/hw_breakpoint.c b/arch/xtensa/kernel/hw_breakpoint.c
index b35656a..aae055d 100644
--- a/arch/xtensa/kernel/hw_breakpoint.c
+++ b/arch/xtensa/kernel/hw_breakpoint.c
@@ -45,15 +45,38 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }
 
-/*
- * Construct an arch_hw_breakpoint from a perf_event.
- */
-static int arch_build_bp_info(struct perf_event *bp)
+static int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr)
+{
+	/* Type */
+	switch (attr->bp_type) {
+	case HW_BREAKPOINT_X:
+	case HW_BREAKPOINT_R:
+	case HW_BREAKPOINT_W:
+	case HW_BREAKPOINT_RW:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Len */
+	if (attr->bp_len < 1 || attr->bp_len > 64 || !is_power_of_2(attr->bp_len))
+		return -EINVAL;
+
+	/* Address */
+	if (attr->bp_addr & (attr->bp_len - 1))
+		return -EINVAL;
+
+	return 0;
+}
+
+static void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event_attr *attr = &bp->attr;
 
 	/* Type */
-	switch (bp->attr.bp_type) {
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_X:
 		info->type = XTENSA_BREAKPOINT_EXECUTE;
 		break;
@@ -67,29 +90,30 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->type = XTENSA_BREAKPOINT_LOAD | XTENSA_BREAKPOINT_STORE;
 		break;
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
 	}
 
 	/* Len */
-	info->len = bp->attr.bp_len;
-	if (info->len < 1 || info->len > 64 || !is_power_of_2(info->len))
-		return -EINVAL;
+	info->len = attr->bp_len;
 
 	/* Address */
-	info->address = bp->attr.bp_addr;
-	if (info->address & (info->len - 1))
-		return -EINVAL;
-
-	return 0;
+	info->address = attr->bp_addr;
 }
 
+/*
+ * Validate the arch-specific HW Breakpoint register settings
+ */
 int arch_validate_hwbkpt_settings(struct perf_event *bp)
 {
-	int ret;
+	int err;
 
-	/* Build the arch_hw_breakpoint. */
-	ret = arch_build_bp_info(bp);
-	return ret;
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
+
+	hw_breakpoint_arch_commit(bp);
+
+	return 0;
 }
 
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
-- 
2.7.4

[PATCH 6/9] arm64: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

The breakpoint code 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 struct.

Prepare fox fixing this misdesign and separate both logics.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/arm64/kernel/hw_breakpoint.c | 183 +++++++++++++++++++++++++-------------
 1 file changed, 122 insertions(+), 61 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 74bb56f..fa02995 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -419,15 +419,114 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 	return 0;
 }
 
+static int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr)
+{
+	u64 addr = attr->bp_addr, len = attr->bp_len;
+	u32 type = attr->bp_type;
+
+	/* Type */
+	switch (type) {
+	case HW_BREAKPOINT_X:
+	case HW_BREAKPOINT_R:
+	case HW_BREAKPOINT_W:
+	case HW_BREAKPOINT_RW:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Len */
+	switch (len) {
+	case HW_BREAKPOINT_LEN_1:
+	case HW_BREAKPOINT_LEN_2:
+	case HW_BREAKPOINT_LEN_3:
+	case HW_BREAKPOINT_LEN_4:
+	case HW_BREAKPOINT_LEN_5:
+	case HW_BREAKPOINT_LEN_6:
+	case HW_BREAKPOINT_LEN_7:
+	case HW_BREAKPOINT_LEN_8:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * On AArch64, we only permit breakpoints of length 4, whereas
+	 * AArch32 also requires breakpoints of length 2 for Thumb.
+	 * Watchpoints can be of length 1, 2, 4 or 8 bytes.
+	 */
+	if (type == HW_BREAKPOINT_X) {
+		if (is_compat_bp(bp)) {
+			if (len != HW_BREAKPOINT_LEN_2 &&
+			    len != HW_BREAKPOINT_LEN_4)
+				return -EINVAL;
+		} else if (len != HW_BREAKPOINT_LEN_4) {
+			/*
+			 * FIXME: Some tools (I'm looking at you perf) assume
+			 *	  that breakpoints should be sizeof(long). This
+			 *	  is nonsense. For now, we fix up the parameter
+			 *	  but we should probably return -EINVAL instead.
+			 */
+			len = HW_BREAKPOINT_LEN_4;
+		}
+	}
+
+	/*
+	 * Check address alignment.
+	 * We don't do any clever alignment correction for watchpoints
+	 * because using 64-bit unaligned addresses is deprecated for
+	 * AArch64.
+	 *
+	 * AArch32 tasks expect some simple alignment fixups, so emulate
+	 * that here.
+	 */
+	if (is_compat_bp(bp)) {
+		u64 alignment_mask, offset;
+
+		if (len == HW_BREAKPOINT_LEN_8)
+			alignment_mask = 0x7;
+		else
+			alignment_mask = 0x3;
+		offset = addr & alignment_mask;
+		switch (offset) {
+		case 0:
+			/* Aligned */
+			break;
+		case 1:
+			/* Allow single byte watchpoint. */
+			if (len == HW_BREAKPOINT_LEN_1)
+				break;
+		case 2:
+			/* Allow halfword watchpoints and breakpoints. */
+			if (len == HW_BREAKPOINT_LEN_2)
+				break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	/*
+	 * Disallow per-task kernel breakpoints since these would
+	 * complicate the stepping code.
+	 */
+	if (arch_check_bp_in_kernelspace(bp) && bp->hw.target)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Construct an arch_hw_breakpoint from a perf_event.
  */
-static int arch_build_bp_info(struct perf_event *bp)
+static void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event_attr *attr = &bp->attr;
+	u64 alignment_mask, offset;
 
 	/* Type */
-	switch (bp->attr.bp_type) {
+	switch (attr->bp_type) {
 	case HW_BREAKPOINT_X:
 		info->ctrl.type = ARM_BREAKPOINT_EXECUTE;
 		break;
@@ -441,11 +540,11 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->ctrl.type = ARM_BREAKPOINT_LOAD | ARM_BREAKPOINT_STORE;
 		break;
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
 	}
 
 	/* Len */
-	switch (bp->attr.bp_len) {
+	switch (attr->bp_len) {
 	case HW_BREAKPOINT_LEN_1:
 		info->ctrl.len = ARM_BREAKPOINT_LEN_1;
 		break;
@@ -471,7 +570,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->ctrl.len = ARM_BREAKPOINT_LEN_8;
 		break;
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
 	}
 
 	/*
@@ -480,11 +579,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 	 * Watchpoints can be of length 1, 2, 4 or 8 bytes.
 	 */
 	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE) {
-		if (is_compat_bp(bp)) {
-			if (info->ctrl.len != ARM_BREAKPOINT_LEN_2 &&
-			    info->ctrl.len != ARM_BREAKPOINT_LEN_4)
-				return -EINVAL;
-		} else if (info->ctrl.len != ARM_BREAKPOINT_LEN_4) {
+		if (!is_compat_bp(bp) && info->ctrl.len != ARM_BREAKPOINT_LEN_4) {
 			/*
 			 * FIXME: Some tools (I'm looking at you perf) assume
 			 *	  that breakpoints should be sizeof(long). This
@@ -496,7 +591,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 	}
 
 	/* Address */
-	info->address = bp->attr.bp_addr;
+	info->address = attr->bp_addr;
 
 	/*
 	 * Privilege
@@ -509,72 +604,38 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->ctrl.privilege = AARCH64_BREAKPOINT_EL0;
 
 	/* Enabled? */
-	info->ctrl.enabled = !bp->attr.disabled;
+	info->ctrl.enabled = !attr->disabled;
 
-	return 0;
-}
-
-/*
- * Validate the arch-specific HW Breakpoint register settings.
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	int ret;
-	u64 alignment_mask, offset;
-
-	/* Build the arch_hw_breakpoint. */
-	ret = arch_build_bp_info(bp);
-	if (ret)
-		return ret;
-
-	/*
-	 * Check address alignment.
-	 * We don't do any clever alignment correction for watchpoints
-	 * because using 64-bit unaligned addresses is deprecated for
-	 * AArch64.
-	 *
-	 * AArch32 tasks expect some simple alignment fixups, so emulate
-	 * that here.
-	 */
 	if (is_compat_bp(bp)) {
 		if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
 			alignment_mask = 0x7;
 		else
 			alignment_mask = 0x3;
-		offset = info->address & alignment_mask;
-		switch (offset) {
-		case 0:
-			/* Aligned */
-			break;
-		case 1:
-			/* Allow single byte watchpoint. */
-			if (info->ctrl.len == ARM_BREAKPOINT_LEN_1)
-				break;
-		case 2:
-			/* Allow halfword watchpoints and breakpoints. */
-			if (info->ctrl.len == ARM_BREAKPOINT_LEN_2)
-				break;
-		default:
-			return -EINVAL;
-		}
 	} else {
 		if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE)
 			alignment_mask = 0x3;
 		else
 			alignment_mask = 0x7;
-		offset = info->address & alignment_mask;
 	}
 
+	offset = info->address & alignment_mask;
+
 	info->address &= ~alignment_mask;
 	info->ctrl.len <<= offset;
+}
 
-	/*
-	 * Disallow per-task kernel breakpoints since these would
-	 * complicate the stepping code.
-	 */
-	if (info->ctrl.privilege == AARCH64_BREAKPOINT_EL1 && bp->hw.target)
-		return -EINVAL;
+/*
+ * Validate the arch-specific HW Breakpoint register settings
+ */
+int arch_validate_hwbkpt_settings(struct perf_event *bp)
+{
+	int err;
+
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
+
+	hw_breakpoint_arch_commit(bp);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 7/9] powerpc: Split breakpoint validation into "check" and "commit"

[Frederic Weisbecker]

The breakpoint code 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 struct.

Prepare fox fixing this misdesign and separate both logics.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/powerpc/kernel/hw_breakpoint.c | 82 ++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 4c1012b..c39fc86 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -138,33 +138,20 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 	return 0;
 }
 
-/*
- * Validate the arch-specific HW Breakpoint register settings
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
+static int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr)
 {
-	int ret = -EINVAL, length_max;
-	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	int length_max;
 
 	if (!bp)
-		return ret;
+		return -EINVAL;
 
-	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)
-		/* 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;
-	info->address = bp->attr.bp_addr;
-	info->len = bp->attr.bp_len;
+	/* Must set at least read or write */
+	if (!(attr->bp_type & (HW_BREAKPOINT_R | HW_BREAKPOINT_W)))
+		return -EINVAL;
+
+	if (!ppc_breakpoint_available())
+		return -ENODEV;
 
 	/*
 	 * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)
@@ -172,19 +159,56 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address, the
 	 * 'symbolsize' should satisfy the check below.
 	 */
-	if (!ppc_breakpoint_available())
-		return -ENODEV;
 	length_max = 8; /* DABR */
 	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;
 	}
-	if (info->len >
-	    (length_max - (info->address & HW_BREAKPOINT_ALIGN)))
+	if (attr->bp_len >
+	    (length_max - (attr->bp_addr & HW_BREAKPOINT_ALIGN)))
 		return -EINVAL;
+
+	return 0;
+}
+
+static void hw_breakpoint_arch_commit(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event_attr *attr = &bp->attr;
+
+	info->type = HW_BRK_TYPE_TRANSLATE;
+	if (attr->bp_type & HW_BREAKPOINT_R)
+		info->type |= HW_BRK_TYPE_READ;
+	if (attr->bp_type & HW_BREAKPOINT_W)
+		info->type |= HW_BRK_TYPE_WRITE;
+
+	if (!attr->exclude_user)
+		info->type |= HW_BRK_TYPE_USER;
+	if (!attr->exclude_kernel)
+		info->type |= HW_BRK_TYPE_KERNEL;
+	if (!attr->exclude_hv)
+		info->type |= HW_BRK_TYPE_HYP;
+
+	info->address = attr->bp_addr;
+	info->len = attr->bp_len;
+}
+
+/*
+ * Validate the arch-specific HW Breakpoint register settings
+ */
+int arch_validate_hwbkpt_settings(struct perf_event *bp)
+{
+	int err;
+
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
+
+	hw_breakpoint_arch_commit(bp);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Frederic Weisbecker]

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

Now that we have split its logic on all archs, we can remove this
misdesigned function and call directly the arch check and commit
functions instead. This allows us to later avoid commiting
a breakpoint to architecture when its slot couldn't be allocated.

Original-patch-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
 arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
 arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
 arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
 arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
 arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
 arch/sh/include/asm/hw_breakpoint.h      |  5 ++++-
 arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
 arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
 arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
 arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
 arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------
 kernel/events/hw_breakpoint.c            | 11 ++++++-----
 13 files changed, 48 insertions(+), 126 deletions(-)

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index e46e4e7..a42d3b9 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -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,9 @@ 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_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr);
+extern void hw_breakpoint_arch_commit(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
 
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 1769d3a..e14b32d 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -515,8 +515,8 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 	return 0;
 }
 
-static int hw_breakpoint_arch_check(struct perf_event *bp,
-				    const struct perf_event_attr *attr)
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr)
 {
 	u32 offset, alignment_mask = 0x3;
 
@@ -614,7 +614,7 @@ static int hw_breakpoint_arch_check(struct perf_event *bp,
 	return 0;
 }
 
-static void hw_breakpoint_arch_commit(struct perf_event *bp)
+void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event_attr *attr = &bp->attr;
@@ -679,22 +679,6 @@ static void hw_breakpoint_arch_commit(struct perf_event *bp)
 }
 
 /*
- * Validate the arch-specific HW Breakpoint register settings.
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int err;
-
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
-	if (err)
-		return err;
-
-	hw_breakpoint_arch_commit(bp);
-
-	return 0;
-}
-
-/*
  * Enable/disable single-stepping over the breakpoint bp at address addr.
  */
 static void enable_single_step(struct perf_event *bp, u32 addr)
diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 4177076..916980d 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -117,6 +117,7 @@ static inline void decode_ctrl_reg(u32 reg,
 	write_sysreg(VAL, dbg##REG##N##_el1);\
 } while (0)
 
+struct perf_event_attr;
 struct task_struct;
 struct notifier_block;
 struct perf_event;
@@ -125,7 +126,9 @@ 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_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr);
+extern void hw_breakpoint_arch_commit(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
 
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index fa02995..eb254c1 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -419,8 +419,8 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 	return 0;
 }
 
-static int hw_breakpoint_arch_check(struct perf_event *bp,
-				    const struct perf_event_attr *attr)
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr)
 {
 	u64 addr = attr->bp_addr, len = attr->bp_len;
 	u32 type = attr->bp_type;
@@ -519,7 +519,7 @@ static int hw_breakpoint_arch_check(struct perf_event *bp,
 /*
  * Construct an arch_hw_breakpoint from a perf_event.
  */
-static void hw_breakpoint_arch_commit(struct perf_event *bp)
+void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event_attr *attr = &bp->attr;
@@ -625,22 +625,6 @@ static void hw_breakpoint_arch_commit(struct perf_event *bp)
 }
 
 /*
- * Validate the arch-specific HW Breakpoint register settings
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int err;
-
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
-	if (err)
-		return err;
-
-	hw_breakpoint_arch_commit(bp);
-
-	return 0;
-}
-
-/*
  * Enable/disable all of the breakpoints active at the specified
  * exception level at the register level.
  * This is used when single-stepping after a breakpoint exception.
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 8e7b097..90d0379 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -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,9 @@ 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_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr);
+extern void hw_breakpoint_arch_commit(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 						unsigned long val, void *data);
 int arch_install_hw_breakpoint(struct perf_event *bp);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index c39fc86..38366ed 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -138,8 +138,8 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 	return 0;
 }
 
-static int hw_breakpoint_arch_check(struct perf_event *bp,
-				    const struct perf_event_attr *attr)
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr)
 {
 	int length_max;
 
@@ -174,7 +174,7 @@ static int hw_breakpoint_arch_check(struct perf_event *bp,
 	return 0;
 }
 
-static void hw_breakpoint_arch_commit(struct perf_event *bp)
+void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event_attr *attr = &bp->attr;
@@ -197,22 +197,6 @@ static void hw_breakpoint_arch_commit(struct perf_event *bp)
 }
 
 /*
- * Validate the arch-specific HW Breakpoint register settings
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int err;
-
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
-	if (err)
-		return err;
-
-	hw_breakpoint_arch_commit(bp);
-
-	return 0;
-}
-
-/*
  * Restores the breakpoint on the debug registers.
  * Invoke this function if it is known that the execution context is
  * about to change to cause loss of MSR_SE settings.
diff --git a/arch/sh/include/asm/hw_breakpoint.h b/arch/sh/include/asm/hw_breakpoint.h
index 68384e6..d4d4c34 100644
--- a/arch/sh/include/asm/hw_breakpoint.h
+++ b/arch/sh/include/asm/hw_breakpoint.h
@@ -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,9 @@ 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_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr);
+extern void hw_breakpoint_arch_commit(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
 
diff --git a/arch/sh/kernel/hw_breakpoint.c b/arch/sh/kernel/hw_breakpoint.c
index c89152f..85cbf71 100644
--- a/arch/sh/kernel/hw_breakpoint.c
+++ b/arch/sh/kernel/hw_breakpoint.c
@@ -174,8 +174,8 @@ int arch_bp_generic_fields(int sh_len, int sh_type,
 	return 0;
 }
 
-static int hw_breakpoint_arch_check(struct perf_event *bp,
-				    const struct perf_event_attr *attr)
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr)
 {
 	unsigned int align;
 
@@ -212,7 +212,7 @@ static int hw_breakpoint_arch_check(struct perf_event *bp,
 	return 0;
 }
 
-static void hw_breakpoint_arch_commit(struct perf_event *bp)
+void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event_attr *attr = &bp->attr;
@@ -254,22 +254,6 @@ static void hw_breakpoint_arch_commit(struct perf_event *bp)
 }
 
 /*
- * Validate the arch-specific HW Breakpoint register settings
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int err;
-
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
-	if (err)
-		return err;
-
-	hw_breakpoint_arch_commit(bp);
-
-	return 0;
-}
-
-/*
  * Release the user breakpoints used by ptrace
  */
 void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
index f59c398..9b7dc36 100644
--- a/arch/x86/include/asm/hw_breakpoint.h
+++ b/arch/x86/include/asm/hw_breakpoint.h
@@ -49,11 +49,14 @@ 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 perf_event *bp);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_arch_check(struct perf_event *bp,
+				    const struct perf_event_attr *attr);
+extern void hw_breakpoint_arch_commit(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 					   unsigned long val, void *data);
 
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 6960800..833aed9 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -233,8 +233,8 @@ int arch_bp_generic_fields(int x86_len, int x86_type,
 	return 0;
 }
 
-static int hw_breakpoint_arch_check(struct perf_event *bp,
-				    const struct perf_event_attr *attr)
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr)
 {
 	unsigned int align;
 
@@ -295,7 +295,7 @@ static int hw_breakpoint_arch_check(struct perf_event *bp,
 	return 0;
 }
 
-static void hw_breakpoint_arch_commit(struct perf_event *bp)
+void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event_attr *attr = &bp->attr;
@@ -354,23 +354,6 @@ static void hw_breakpoint_arch_commit(struct perf_event *bp)
 	}
 }
 
-
-/*
- * Validate the arch-specific HW Breakpoint register settings
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int err;
-
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
-	if (err)
-		return err;
-
-	hw_breakpoint_arch_commit(bp);
-
-	return 0;
-}
-
 /*
  * Dump the debug register contents to the user.
  * We can't dump our per cpu values because it
diff --git a/arch/xtensa/include/asm/hw_breakpoint.h b/arch/xtensa/include/asm/hw_breakpoint.h
index dbe3053b..6761cea 100644
--- a/arch/xtensa/include/asm/hw_breakpoint.h
+++ b/arch/xtensa/include/asm/hw_breakpoint.h
@@ -30,13 +30,16 @@ 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 perf_event *bp);
-int arch_validate_hwbkpt_settings(struct perf_event *bp);
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr);
+void hw_breakpoint_arch_commit(struct perf_event *bp);
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data);
 
diff --git a/arch/xtensa/kernel/hw_breakpoint.c b/arch/xtensa/kernel/hw_breakpoint.c
index aae055d..015ffdd 100644
--- a/arch/xtensa/kernel/hw_breakpoint.c
+++ b/arch/xtensa/kernel/hw_breakpoint.c
@@ -45,8 +45,8 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }
 
-static int hw_breakpoint_arch_check(struct perf_event *bp,
-				    const struct perf_event_attr *attr)
+int hw_breakpoint_arch_check(struct perf_event *bp,
+			     const struct perf_event_attr *attr)
 {
 	/* Type */
 	switch (attr->bp_type) {
@@ -70,7 +70,7 @@ static int hw_breakpoint_arch_check(struct perf_event *bp,
 	return 0;
 }
 
-static void hw_breakpoint_arch_commit(struct perf_event *bp)
+void hw_breakpoint_arch_commit(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event_attr *attr = &bp->attr;
@@ -100,22 +100,6 @@ static void hw_breakpoint_arch_commit(struct perf_event *bp)
 	info->address = attr->bp_addr;
 }
 
-/*
- * Validate the arch-specific HW Breakpoint register settings
- */
-int arch_validate_hwbkpt_settings(struct perf_event *bp)
-{
-	int err;
-
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
-	if (err)
-		return err;
-
-	hw_breakpoint_arch_commit(bp);
-
-	return 0;
-}
-
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data)
 {
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..6896ceeb 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)
 
 static int validate_hw_breakpoint(struct perf_event *bp)
 {
-	int ret;
+	int err;
 
-	ret = arch_validate_hwbkpt_settings(bp);
-	if (ret)
-		return ret;
+	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	if (err)
+		return err;
+	hw_breakpoint_arch_commit(bp);
 
 	if (arch_check_bp_in_kernelspace(bp)) {
 		if (bp->attr.exclude_kernel)
@@ -432,7 +433,7 @@ int register_perf_hw_breakpoint(struct perf_event *bp)
 
 	ret = validate_hw_breakpoint(bp);
 
-	/* if arch_validate_hwbkpt_settings() fails then release bp slot */
+	/* if hw_breakpoint_arch_check() fails then release bp slot */
 	if (ret)
 		release_bp_slot(bp);
 
-- 
2.7.4

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Joel Fernandes]

On Sun, May 6, 2018 at 12:22 PM Frederic Weisbecker <frederic@kernel.org>
wrote:

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

> Now that we have split its logic on all archs, we can remove this
> misdesigned function and call directly the arch check and commit
> functions instead. This allows us to later avoid commiting
> a breakpoint to architecture when its slot couldn't be allocated.

[...]
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 6e28d28..6896ceeb 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)

>   static int validate_hw_breakpoint(struct perf_event *bp)
>   {
> -       int ret;
> +       int err;

> -       ret = arch_validate_hwbkpt_settings(bp);
> -       if (ret)
> -               return ret;
> +       err = hw_breakpoint_arch_check(bp, &bp->attr);
> +       if (err)
> +               return err;
> +       hw_breakpoint_arch_commit(bp);

minor nit:
To preserve bisectability, shouldn't this be the following in this and
earlier patches?

        err = hw_breakpoint_arch_check(bp, &bp->attr);
        hw_breakpoint_arch_commit(bp);
        if (err)
                return err;

And then in patch 9/9 you can fix it the right way?

thanks,

- Joel

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Frederic Weisbecker]

On Mon, May 07, 2018 at 12:46:06AM +0000, Joel Fernandes wrote:
> On Sun, May 6, 2018 at 12:22 PM Frederic Weisbecker <frederic@kernel.org>
> wrote:
> 
> > 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 struct.
> 
> > Now that we have split its logic on all archs, we can remove this
> > misdesigned function and call directly the arch check and commit
> > functions instead. This allows us to later avoid commiting
> > a breakpoint to architecture when its slot couldn't be allocated.
> 
> [...]
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 6e28d28..6896ceeb 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)
> 
> >   static int validate_hw_breakpoint(struct perf_event *bp)
> >   {
> > -       int ret;
> > +       int err;
> 
> > -       ret = arch_validate_hwbkpt_settings(bp);
> > -       if (ret)
> > -               return ret;
> > +       err = hw_breakpoint_arch_check(bp, &bp->attr);
> > +       if (err)
> > +               return err;
> > +       hw_breakpoint_arch_commit(bp);
> 
> minor nit:
> To preserve bisectability, shouldn't this be the following in this and
> earlier patches?
> 
>         err = hw_breakpoint_arch_check(bp, &bp->attr);
>         hw_breakpoint_arch_commit(bp);
>         if (err)
>                 return err;
> 
> And then in patch 9/9 you can fix it the right way?

I don't see how it was breaking bisectability.
Anyway I'm rewriting it entirely to use:

    struct arch_hw_breakpoint hw;
    int err;

    err = hw_breakpoint_arch_parse(bp, attr, &hw);
    if (err)
        return err;

    .....

    bp->hw.info = hw;


Thanks.

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Joel Fernandes]

On Tue, May 15, 2018 at 03:53:00PM +0200, Frederic Weisbecker wrote:
> On Mon, May 07, 2018 at 12:46:06AM +0000, Joel Fernandes wrote:
> > On Sun, May 6, 2018 at 12:22 PM Frederic Weisbecker <frederic@kernel.org>
> > wrote:
> > 
> > > 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 struct.
> > 
> > > Now that we have split its logic on all archs, we can remove this
> > > misdesigned function and call directly the arch check and commit
> > > functions instead. This allows us to later avoid commiting
> > > a breakpoint to architecture when its slot couldn't be allocated.
> > 
> > [...]
> > > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > > index 6e28d28..6896ceeb 100644
> > > --- a/kernel/events/hw_breakpoint.c
> > > +++ b/kernel/events/hw_breakpoint.c
> > > @@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)
> > 
> > >   static int validate_hw_breakpoint(struct perf_event *bp)
> > >   {
> > > -       int ret;
> > > +       int err;
> > 
> > > -       ret = arch_validate_hwbkpt_settings(bp);
> > > -       if (ret)
> > > -               return ret;
> > > +       err = hw_breakpoint_arch_check(bp, &bp->attr);
> > > +       if (err)
> > > +               return err;
> > > +       hw_breakpoint_arch_commit(bp);
> > 
> > minor nit:
> > To preserve bisectability, shouldn't this be the following in this and
> > earlier patches?
> > 
> >         err = hw_breakpoint_arch_check(bp, &bp->attr);
> >         hw_breakpoint_arch_commit(bp);
> >         if (err)
> >                 return err;
> > 
> > And then in patch 9/9 you can fix it the right way?
> 
> I don't see how it was breaking bisectability.

Sorry may be I used wrong words, I meant keeping the restructure patch as
just for restvructuring and then changing the logic later. But I'm Ok with
what you have, and also like Peter's __weak thing suggestion.

> Anyway I'm rewriting it entirely to use:
> 
>     struct arch_hw_breakpoint hw;
>     int err;
> 
>     err = hw_breakpoint_arch_parse(bp, attr, &hw);
>     if (err)
>         return err;

Cool, if you don't mind do CC me on patches, Thanks.

Just to update you about my email address change, the new one is: joel@joelfernandes.org

thanks,

- Joel

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Peter Zijlstra]

On Sun, May 06, 2018 at 09:19:54PM +0200, Frederic Weisbecker wrote:
>  arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
>  arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
>  arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
>  arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
>  arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
>  arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
>  arch/sh/include/asm/hw_breakpoint.h      |  5 ++++-
>  arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
>  arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
>  arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
>  arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
>  arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------

Because of those ^,

>  kernel/events/hw_breakpoint.c            | 11 ++++++-----

would it not make sense to have a prelimenary patch doing something
like:

__weak int hw_breakpoint_arch_check(struct perf_event *bp)
{
	return arch_validate_hwbkpt_settings(bp);
}

__weak void hw_breakpoint_arch_commit(struct perf_event *bp)
{
}

combined with this bit:

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 6e28d28..6896ceeb 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)
>  
>  static int validate_hw_breakpoint(struct perf_event *bp)
>  {
> -	int ret;
> +	int err;
>  
> -	ret = arch_validate_hwbkpt_settings(bp);
> -	if (ret)
> -		return ret;
> +	err = hw_breakpoint_arch_check(bp, &bp->attr);
> +	if (err)
> +		return err;
> +	hw_breakpoint_arch_commit(bp);
>  
>  	if (arch_check_bp_in_kernelspace(bp)) {
>  		if (bp->attr.exclude_kernel)

And then convert the archs over one by one, and at the end remove the
weak thingies entirely?

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Sun, May 06, 2018 at 09:19:54PM +0200, Frederic Weisbecker wrote:
> >  arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
> >  arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
> >  arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
> >  arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
> >  arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
> >  arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
> >  arch/sh/include/asm/hw_breakpoint.h      |  5 ++++-
> >  arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
> >  arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
> >  arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
> >  arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
> >  arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------
> 
> Because of those ^,
> 
> >  kernel/events/hw_breakpoint.c            | 11 ++++++-----
> 
> would it not make sense to have a prelimenary patch doing something
> like:
> 
> __weak int hw_breakpoint_arch_check(struct perf_event *bp)
> {
> 	return arch_validate_hwbkpt_settings(bp);
> }
> 
> __weak void hw_breakpoint_arch_commit(struct perf_event *bp)
> {
> }
> 
> combined with this bit:
> 
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 6e28d28..6896ceeb 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)
> >  
> >  static int validate_hw_breakpoint(struct perf_event *bp)
> >  {
> > -	int ret;
> > +	int err;
> >  
> > -	ret = arch_validate_hwbkpt_settings(bp);
> > -	if (ret)
> > -		return ret;
> > +	err = hw_breakpoint_arch_check(bp, &bp->attr);
> > +	if (err)
> > +		return err;
> > +	hw_breakpoint_arch_commit(bp);
> >  
> >  	if (arch_check_bp_in_kernelspace(bp)) {
> >  		if (bp->attr.exclude_kernel)
> 
> And then convert the archs over one by one, and at the end remove the
> weak thingies entirely?

Makes sense.

The rest looks good to me - Frederic, once you implement Peter's uggestion I 
suspect this series can be applied.

Thanks,

	Ingo

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Frederic Weisbecker]

On Tue, May 15, 2018 at 08:57:47AM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Sun, May 06, 2018 at 09:19:54PM +0200, Frederic Weisbecker wrote:
> > >  arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
> > >  arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
> > >  arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
> > >  arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
> > >  arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
> > >  arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
> > >  arch/sh/include/asm/hw_breakpoint.h      |  5 ++++-
> > >  arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
> > >  arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
> > >  arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
> > >  arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
> > >  arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------
> > 
> > Because of those ^,
> > 
> > >  kernel/events/hw_breakpoint.c            | 11 ++++++-----
> > 
> > would it not make sense to have a prelimenary patch doing something
> > like:
> > 
> > __weak int hw_breakpoint_arch_check(struct perf_event *bp)
> > {
> > 	return arch_validate_hwbkpt_settings(bp);
> > }
> > 
> > __weak void hw_breakpoint_arch_commit(struct perf_event *bp)
> > {
> > }
> > 
> > combined with this bit:
> > 
> > > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > > index 6e28d28..6896ceeb 100644
> > > --- a/kernel/events/hw_breakpoint.c
> > > +++ b/kernel/events/hw_breakpoint.c
> > > @@ -402,11 +402,12 @@ int dbg_release_bp_slot(struct perf_event *bp)
> > >  
> > >  static int validate_hw_breakpoint(struct perf_event *bp)
> > >  {
> > > -	int ret;
> > > +	int err;
> > >  
> > > -	ret = arch_validate_hwbkpt_settings(bp);
> > > -	if (ret)
> > > -		return ret;
> > > +	err = hw_breakpoint_arch_check(bp, &bp->attr);
> > > +	if (err)
> > > +		return err;
> > > +	hw_breakpoint_arch_commit(bp);
> > >  
> > >  	if (arch_check_bp_in_kernelspace(bp)) {
> > >  		if (bp->attr.exclude_kernel)
> > 
> > And then convert the archs over one by one, and at the end remove the
> > weak thingies entirely?
> 
> Makes sense.
> 
> The rest looks good to me - Frederic, once you implement Peter's uggestion I 
> suspect this series can be applied.

Right, I'll try to do a smoother transition as in Peterz suggestion.
I'm just going to pass around the struct arch_hw_breakpoint to avoid code
duplication in check and commit. The end result may look like:

    struct arch_hw_breakpoint hw;
    int err;

    err = hw_breakpoint_arch_parse(bp, attr, &hw);
    if (err)
        return err;

    .....

    bp->hw.info = hw;

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Frederic Weisbecker]

On Wed, May 09, 2018 at 11:17:03AM +0200, Peter Zijlstra wrote:
> On Sun, May 06, 2018 at 09:19:54PM +0200, Frederic Weisbecker wrote:
> >  arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
> >  arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
> >  arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
> >  arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
> >  arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
> >  arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
> >  arch/sh/include/asm/hw_breakpoint.h      |  5 ++++-
> >  arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
> >  arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
> >  arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
> >  arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
> >  arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------
> 
> Because of those ^,
> 
> >  kernel/events/hw_breakpoint.c            | 11 ++++++-----
> 
> would it not make sense to have a prelimenary patch doing something
> like:
> 
> __weak int hw_breakpoint_arch_check(struct perf_event *bp)
> {
> 	return arch_validate_hwbkpt_settings(bp);
> }

So eventually I fear I can't do that, due to linking order.

Say I convert x86 to implement hw_breakpoint_arch_check(), so I
remove arch_validate_hwbkpt_settings(). On build time, the weak version
is still compiled and can't find a declaration for arch_validate_hwbkpt_settings().

I tried to keep the declaration while the definition has been removed but
it seems the weak version is linked first before it gets later replaced by
the overriden arch version. So I get a build error.

I could keep arch_validate_hwbkpt_settings() around on all archs and remove it in
the end with the weak version but that would defeat the purpose of removing
the mid-state in the current patch.

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Andy Lutomirski]

> On May 15, 2018, at 8:11 PM, Frederic Weisbecker <frederic@kernel.org> wrote:
> 
>> On Wed, May 09, 2018 at 11:17:03AM +0200, Peter Zijlstra wrote:
>>> On Sun, May 06, 2018 at 09:19:54PM +0200, Frederic Weisbecker wrote:
>>> arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
>>> arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
>>> arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
>>> arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
>>> arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
>>> arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
>>> arch/sh/include/asm/hw_breakpoint.h |  5 ++++-
>>> arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
>>> arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
>>> arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
>>> arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
>>> arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------
>> 
>> Because of those ^,
>> 
>>> kernel/events/hw_breakpoint.c            | 11 ++++++-----
>> 
>> would it not make sense to have a prelimenary patch doing something
>> like:
>> 
>> __weak int hw_breakpoint_arch_check(struct perf_event *bp)
>> {
>>    return arch_validate_hwbkpt_settings(bp);
>> }
> 
> So eventually I fear I can't do that, due to linking order.
> 
> Say I convert x86 to implement hw_breakpoint_arch_check(), so I
> remove arch_validate_hwbkpt_settings(). On build time, the weak version
> is still compiled and can't find a declaration for arch_validate_hwbkpt_settings().
> 
> I tried to keep the declaration while the definition has been removed but
> it seems the weak version is linked first before it gets later replaced by
> the overriden arch version. So I get a build error.
> 
> I could keep arch_validate_hwbkpt_settings() around on all archs and remove it in
> the end with the weak version but that would defeat the purpose of removing
> the mid-state in the current patch.

How about just not using weak functions?  Weak functions have annoying issues like this, and they have trouble generating good code. I much prefer the pattern:

in arch header:
extern void arch_func(whatever);
#define arch_func arch_func

in generic header:
#ifndef arch_func
static inline void arch_func(whatever) ...
#endif

Re: [PATCH 8/9] perf/breakpoint: Split breakpoint "check" and "commit"

[Frederic Weisbecker]

On Tue, May 15, 2018 at 09:58:03PM -0700, Andy Lutomirski wrote:
> 
> 
> > On May 15, 2018, at 8:11 PM, Frederic Weisbecker <frederic@kernel.org> wrote:
> > 
> >> On Wed, May 09, 2018 at 11:17:03AM +0200, Peter Zijlstra wrote:
> >>> On Sun, May 06, 2018 at 09:19:54PM +0200, Frederic Weisbecker wrote:
> >>> arch/arm/include/asm/hw_breakpoint.h     |  5 ++++-
> >>> arch/arm/kernel/hw_breakpoint.c          | 22 +++-------------------
> >>> arch/arm64/include/asm/hw_breakpoint.h   |  5 ++++-
> >>> arch/arm64/kernel/hw_breakpoint.c        | 22 +++-------------------
> >>> arch/powerpc/include/asm/hw_breakpoint.h |  5 ++++-
> >>> arch/powerpc/kernel/hw_breakpoint.c      | 22 +++-------------------
> >>> arch/sh/include/asm/hw_breakpoint.h |  5 ++++-
> >>> arch/sh/kernel/hw_breakpoint.c           | 22 +++-------------------
> >>> arch/x86/include/asm/hw_breakpoint.h     |  5 ++++-
> >>> arch/x86/kernel/hw_breakpoint.c          | 23 +++--------------------
> >>> arch/xtensa/include/asm/hw_breakpoint.h  |  5 ++++-
> >>> arch/xtensa/kernel/hw_breakpoint.c       | 22 +++-------------------
> >> 
> >> Because of those ^,
> >> 
> >>> kernel/events/hw_breakpoint.c            | 11 ++++++-----
> >> 
> >> would it not make sense to have a prelimenary patch doing something
> >> like:
> >> 
> >> __weak int hw_breakpoint_arch_check(struct perf_event *bp)
> >> {
> >>    return arch_validate_hwbkpt_settings(bp);
> >> }
> > 
> > So eventually I fear I can't do that, due to linking order.
> > 
> > Say I convert x86 to implement hw_breakpoint_arch_check(), so I
> > remove arch_validate_hwbkpt_settings(). On build time, the weak version
> > is still compiled and can't find a declaration for arch_validate_hwbkpt_settings().
> > 
> > I tried to keep the declaration while the definition has been removed but
> > it seems the weak version is linked first before it gets later replaced by
> > the overriden arch version. So I get a build error.
> > 
> > I could keep arch_validate_hwbkpt_settings() around on all archs and remove it in
> > the end with the weak version but that would defeat the purpose of removing
> > the mid-state in the current patch.
> 
> How about just not using weak functions?  Weak functions have annoying issues like this, and they have trouble generating good code. I much prefer the pattern:
> 
> in arch header:
> extern void arch_func(whatever);
> #define arch_func arch_func
> 
> in generic header:
> #ifndef arch_func
> static inline void arch_func(whatever) ...
> #endif

Thanks, that works well!

[PATCH 9/9] perf/breakpoint: Only commit breakpoint to arch upon slot reservation success

[Frederic Weisbecker]

When slot reservation fails on breakpoint modification, we restore the
breakpoint previous state but only halfway as the architecture structure
isn't cleaned up. It should be harmless as the breakpoint has to be
deactivated at this point. But it's a terrible misdesign.

Now that we have split the attribute check and commit code, we can avoid
commiting a breakpoint to the architecture until its slot reservation
has been accepted and completed.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Original-patch-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 kernel/events/hw_breakpoint.c | 77 ++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 33 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6896ceeb..f5fbc76 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,14 +400,14 @@ int dbg_release_bp_slot(struct perf_event *bp)
 	return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+static int hw_breakpoint_check(struct perf_event *bp,
+			       const struct perf_event_attr *attr)
 {
 	int err;
 
-	err = hw_breakpoint_arch_check(bp, &bp->attr);
+	err = hw_breakpoint_arch_check(bp, attr);
 	if (err)
 		return err;
-	hw_breakpoint_arch_commit(bp);
 
 	if (arch_check_bp_in_kernelspace(bp)) {
 		if (bp->attr.exclude_kernel)
@@ -425,19 +425,21 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-	int ret;
+	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 hw_breakpoint_arch_check() fails then release bp slot */
-	if (ret)
+	err = hw_breakpoint_check(bp, &bp->attr);
+	if (err) {
 		release_bp_slot(bp);
+		return err;
+	}
 
-	return ret;
+	hw_breakpoint_arch_commit(bp);
+
+	return 0;
 }
 
 /**
@@ -457,35 +459,44 @@ 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;
-	int err = 0;
+	int err;
 
-	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;
-
-	err = validate_hw_breakpoint(bp);
-	if (!err && modify)
-		err = modify_bp_slot(bp, old_type);
-
-	if (err) {
-		bp->attr.bp_addr = old_addr;
-		bp->attr.bp_type = old_type;
-		bp->attr.bp_len  = old_len;
+	err = hw_breakpoint_check(bp, attr);
+	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;
 	}
 
-	bp->attr.disabled = attr->disabled;
+	if (bp->attr.bp_type != attr->bp_type) {
+		err = modify_bp_slot(bp, bp->attr.bp_type);
+		if (err)
+			return err;
+	}
+
+	hw_breakpoint_copy_attr(&bp->attr, attr);
+	hw_breakpoint_arch_commit(bp);
+
 	return 0;
 }
 
-- 
2.7.4