[PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor

[PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor

[Ravi Bangoria]

patch 1-3: Code refactor
patch 4: Speedup disabling breakpoint
patch 5: Fix length calculation for unaligned targets

Ravi Bangoria (5):
  Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
  Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()
  Powerpc/hw-breakpoint: Refactor set_dawr()
  Powerpc/hw-breakpoint: Optimize disable path
  Powerpc/Watchpoint: Fix length calculation for unaligned target

 arch/powerpc/include/asm/hw_breakpoint.h | 10 ++--
 arch/powerpc/kernel/hw_breakpoint.c      | 56 ++++++++++++----------
 arch/powerpc/kernel/process.c            | 61 +++++++++++++++++++-----
 arch/powerpc/kernel/ptrace.c             |  2 +-
 4 files changed, 86 insertions(+), 43 deletions(-)

-- 
2.20.1

[PATCH 1/5] Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()

[Ravi Bangoria]

do_dabr() was renamed with do_break() long ago. But I still see
some comments mentioning do_dabr(). Replace it.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/hw_breakpoint.c | 2 +-
 arch/powerpc/kernel/ptrace.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index a293a53b4365..1908e4fcc132 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -232,7 +232,7 @@ int hw_breakpoint_handler(struct die_args *args)
 	 * Return early after invoking user-callback function without restoring
 	 * DABR if the breakpoint is from ptrace which always operates in
 	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
-	 * generated in do_dabr().
+	 * generated in do_break().
 	 */
 	if (bp->overflow_handler == ptrace_triggered) {
 		perf_bp_event(bp, regs);
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 684b0b315c32..44b823e5e8c8 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2373,7 +2373,7 @@ void ptrace_triggered(struct perf_event *bp,
 	/*
 	 * Disable the breakpoint request here since ptrace has defined a
 	 * one-shot behaviour for breakpoint exceptions in PPC64.
-	 * The SIGTRAP signal is generated automatically for us in do_dabr().
+	 * The SIGTRAP signal is generated automatically for us in do_break().
 	 * We don't have to do anything about that here
 	 */
 	attr = bp->attr;
-- 
2.20.1

[PATCH 2/5] Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()

[Ravi Bangoria]

Move feature availability check at the start of the function.
Rearrange comment to it's associated code. Use hw->address and
hw->len in the 512 bytes boundary check(to write if statement
in a single line). Add spacing between code blocks.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/hw_breakpoint.c | 34 +++++++++++++++--------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 1908e4fcc132..36bcf705df65 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -133,10 +133,13 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 			     const struct perf_event_attr *attr,
 			     struct arch_hw_breakpoint *hw)
 {
-	int ret = -EINVAL, length_max;
+	int length_max;
+
+	if (!ppc_breakpoint_available())
+		return -ENODEV;
 
 	if (!bp)
-		return ret;
+		return -EINVAL;
 
 	hw->type = HW_BRK_TYPE_TRANSLATE;
 	if (attr->bp_type & HW_BREAKPOINT_R)
@@ -145,34 +148,33 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 		hw->type |= HW_BRK_TYPE_WRITE;
 	if (hw->type == HW_BRK_TYPE_TRANSLATE)
 		/* must set alteast read or write */
-		return ret;
+		return -EINVAL;
+
 	if (!attr->exclude_user)
 		hw->type |= HW_BRK_TYPE_USER;
 	if (!attr->exclude_kernel)
 		hw->type |= HW_BRK_TYPE_KERNEL;
 	if (!attr->exclude_hv)
 		hw->type |= HW_BRK_TYPE_HYP;
+
 	hw->address = attr->bp_addr;
 	hw->len = attr->bp_len;
 
-	/*
-	 * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)
-	 * and breakpoint addresses are aligned to nearest double-word
-	 * 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 (dawr_enabled()) {
 		length_max = 512 ; /* 64 doublewords */
-		/* DAWR region can't cross 512 boundary */
-		if ((attr->bp_addr >> 9) !=
-		    ((attr->bp_addr + attr->bp_len - 1) >> 9))
+		/* DAWR region can't cross 512 bytes boundary */
+		if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
 			return -EINVAL;
 	}
-	if (hw->len >
-	    (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
+
+	/*
+	 * Since breakpoint length can be a maximum of length_max and
+	 * breakpoint addresses are aligned to nearest double-word
+	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
+	 * the 'symbolsize' should satisfy the check below.
+	 */
+	if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
 		return -EINVAL;
 	return 0;
 }
-- 
2.20.1

[PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr()

[Ravi Bangoria]

Remove unnecessary comments. Code itself is self explanatory.
And, ISA already talks about MRD field. I Don't think we need
to re-describe it.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/process.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f0fbbf6a6a1f..f002d2ffff86 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -799,18 +799,11 @@ int set_dawr(struct arch_hw_breakpoint *brk)
 
 	dawr = brk->address;
 
-	dawrx  = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE)) \
-		                   << (63 - 58); //* read/write bits */
-	dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) \
-		                   << (63 - 59); //* translate */
-	dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) \
-		                   >> 3; //* PRIM bits */
-	/* dawr length is stored in field MDR bits 48:53.  Matches range in
-	   doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
-	   0b111111=64DW.
-	   brk->len is in bytes.
-	   This aligns up to double word size, shifts and does the bias.
-	*/
+	dawrx  = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
+	dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
+	dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
+
+	/* brk->len is in bytes. */
 	mrd = ((brk->len + 7) >> 3) - 1;
 	dawrx |= (mrd & 0x3f) << (63 - 53);
 
-- 
2.20.1

[PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path

[Ravi Bangoria]

Directly setting dawr and dawrx with 0 should be enough to
disable watchpoint. No need to reset individual bits in
variable and then set in hw.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h |  3 ++-
 arch/powerpc/kernel/process.c            | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 78202d5fb13a..8acbbdd4a2d5 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -19,6 +19,7 @@ struct arch_hw_breakpoint {
 /* Note: Don't change the the first 6 bits below as they are in the same order
  * as the dabr and dabrx.
  */
+#define HW_BRK_TYPE_DISABLE		0x00
 #define HW_BRK_TYPE_READ		0x01
 #define HW_BRK_TYPE_WRITE		0x02
 #define HW_BRK_TYPE_TRANSLATE		0x04
@@ -68,7 +69,7 @@ static inline void hw_breakpoint_disable(void)
 	struct arch_hw_breakpoint brk;
 
 	brk.address = 0;
-	brk.type = 0;
+	brk.type = HW_BRK_TYPE_DISABLE;
 	brk.len = 0;
 	if (ppc_breakpoint_available())
 		__set_breakpoint(&brk);
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f002d2ffff86..265fac9fb3a4 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -793,10 +793,22 @@ static inline int set_dabr(struct arch_hw_breakpoint *brk)
 	return __set_dabr(dabr, dabrx);
 }
 
+static int disable_dawr(void)
+{
+	if (ppc_md.set_dawr)
+		return ppc_md.set_dawr(0, 0);
+
+	mtspr(SPRN_DAWRX, 0);
+	return 0;
+}
+
 int set_dawr(struct arch_hw_breakpoint *brk)
 {
 	unsigned long dawr, dawrx, mrd;
 
+	if (brk->type == HW_BRK_TYPE_DISABLE)
+		return disable_dawr();
+
 	dawr = brk->address;
 
 	dawrx  = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
-- 
2.20.1

[PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target

[Ravi Bangoria]

Watchpoint match range is always doubleword(8 bytes) aligned on
powerpc. If the given range is crossing doubleword boundary, we
need to increase the length such that next doubleword also get
covered. Ex,

          address   len = 6 bytes
                |=========.
   |------------v--|------v--------|
   | | | | | | | | | | | | | | | | |
   |---------------|---------------|
    <---8 bytes--->

In such case, current code configures hw as:
  start_addr = address & ~HW_BREAKPOINT_ALIGN
  len = 8 bytes

And thus read/write in last 4 bytes of the given range is ignored.
Fix this by including next doubleword in the length. Watchpoint
exception handler already ignores extraneous exceptions, so no
changes required for that.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h |  7 ++--
 arch/powerpc/kernel/hw_breakpoint.c      | 44 +++++++++++++-----------
 arch/powerpc/kernel/process.c            | 34 ++++++++++++++++--
 3 files changed, 60 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 8acbbdd4a2d5..749a357164d5 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -34,6 +34,8 @@ struct arch_hw_breakpoint {
 #define HW_BRK_TYPE_PRIV_ALL	(HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
 				 HW_BRK_TYPE_HYP)
 
+#define HW_BREAKPOINT_ALIGN 0x7
+
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 #include <linux/kdebug.h>
 #include <asm/reg.h>
@@ -45,8 +47,6 @@ struct pmu;
 struct perf_sample_data;
 struct task_struct;
 
-#define HW_BREAKPOINT_ALIGN 0x7
-
 extern int hw_breakpoint_slots(int type);
 extern int arch_bp_generic_fields(int type, int *gen_bp_type);
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
@@ -76,7 +76,8 @@ static inline void hw_breakpoint_disable(void)
 }
 extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
 int hw_breakpoint_handler(struct die_args *args);
-
+extern u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
+		unsigned long *start_addr, unsigned long *end_addr);
 extern int set_dawr(struct arch_hw_breakpoint *brk);
 extern bool dawr_force_enable;
 static inline bool dawr_enabled(void)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 36bcf705df65..c122fd55aa44 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -126,6 +126,28 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
 	return 0;
 }
 
+/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */
+static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
+{
+	u16 length_max = 8;
+	u16 final_len;
+	unsigned long start_addr, end_addr;
+
+	final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr);
+
+	if (dawr_enabled()) {
+		length_max = 512;
+		/* DAWR region can't cross 512 bytes boundary */
+		if ((start_addr >> 9) != (end_addr >> 9))
+			return -EINVAL;
+	}
+
+	if (final_len > length_max)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
@@ -133,12 +155,10 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 			     const struct perf_event_attr *attr,
 			     struct arch_hw_breakpoint *hw)
 {
-	int length_max;
-
 	if (!ppc_breakpoint_available())
 		return -ENODEV;
 
-	if (!bp)
+	if (!bp || !attr->bp_len)
 		return -EINVAL;
 
 	hw->type = HW_BRK_TYPE_TRANSLATE;
@@ -160,23 +180,7 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 	hw->address = attr->bp_addr;
 	hw->len = attr->bp_len;
 
-	length_max = 8; /* DABR */
-	if (dawr_enabled()) {
-		length_max = 512 ; /* 64 doublewords */
-		/* DAWR region can't cross 512 bytes boundary */
-		if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
-			return -EINVAL;
-	}
-
-	/*
-	 * Since breakpoint length can be a maximum of length_max and
-	 * breakpoint addresses are aligned to nearest double-word
-	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
-	 * the 'symbolsize' should satisfy the check below.
-	 */
-	if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
-		return -EINVAL;
-	return 0;
+	return hw_breakpoint_validate_len(hw);
 }
 
 /*
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 265fac9fb3a4..159aaa70de46 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -802,9 +802,39 @@ static int disable_dawr(void)
 	return 0;
 }
 
+/*
+ * Watchpoint match range is always doubleword(8 bytes) aligned on
+ * powerpc. If the given range is crossing doubleword boundary, we
+ * need to increase the length such that next doubleword also get
+ * covered. Ex,
+ *
+ *          address   len = 6 bytes
+ *                |=========.
+ *   |------------v--|------v--------|
+ *   | | | | | | | | | | | | | | | | |
+ *   |---------------|---------------|
+ *    <---8 bytes--->
+ *
+ * In this case, we should configure hw as:
+ *   start_addr = address & ~HW_BREAKPOINT_ALIGN
+ *   len = 16 bytes
+ *
+ * @start_addr and @end_addr are inclusive.
+ */
+u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
+				unsigned long *start_addr,
+				unsigned long *end_addr)
+{
+	*start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
+	*end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
+	return *end_addr - *start_addr + 1;
+}
+
 int set_dawr(struct arch_hw_breakpoint *brk)
 {
 	unsigned long dawr, dawrx, mrd;
+	unsigned long start_addr, end_addr;
+	u16 final_len;
 
 	if (brk->type == HW_BRK_TYPE_DISABLE)
 		return disable_dawr();
@@ -815,8 +845,8 @@ int set_dawr(struct arch_hw_breakpoint *brk)
 	dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
 	dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
 
-	/* brk->len is in bytes. */
-	mrd = ((brk->len + 7) >> 3) - 1;
+	final_len = hw_breakpoint_get_final_len(brk, &start_addr, &end_addr);
+	mrd = ((final_len + 7) >> 3) - 1;
 	dawrx |= (mrd & 0x3f) << (63 - 53);
 
 	if (ppc_md.set_dawr)
-- 
2.20.1

Re: [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor

[Christophe Leroy]

Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> patch 1-3: Code refactor
> patch 4: Speedup disabling breakpoint
> patch 5: Fix length calculation for unaligned targets

While you are playing with hw breakpoints, did you have a look at 
https://github.com/linuxppc/issues/issues/38 ?

Christophe

> 
> Ravi Bangoria (5):
>    Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()
>    Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()
>    Powerpc/hw-breakpoint: Refactor set_dawr()
>    Powerpc/hw-breakpoint: Optimize disable path
>    Powerpc/Watchpoint: Fix length calculation for unaligned target
> 
>   arch/powerpc/include/asm/hw_breakpoint.h | 10 ++--
>   arch/powerpc/kernel/hw_breakpoint.c      | 56 ++++++++++++----------
>   arch/powerpc/kernel/process.c            | 61 +++++++++++++++++++-----
>   arch/powerpc/kernel/ptrace.c             |  2 +-
>   4 files changed, 86 insertions(+), 43 deletions(-)
> 

Re: [PATCH 1/5] Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()

[Michael Neuling]

> Subject: Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()

Can you add the word "comment" to this subject. Currently it implies there are
code changes here.

Mikey


On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
> do_dabr() was renamed with do_break() long ago. But I still see
> some comments mentioning do_dabr(). Replace it.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  arch/powerpc/kernel/hw_breakpoint.c | 2 +-
>  arch/powerpc/kernel/ptrace.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c
> b/arch/powerpc/kernel/hw_breakpoint.c
> index a293a53b4365..1908e4fcc132 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -232,7 +232,7 @@ int hw_breakpoint_handler(struct die_args *args)
>  	 * Return early after invoking user-callback function without restoring
>  	 * DABR if the breakpoint is from ptrace which always operates in
>  	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
> -	 * generated in do_dabr().
> +	 * generated in do_break().
>  	 */
>  	if (bp->overflow_handler == ptrace_triggered) {
>  		perf_bp_event(bp, regs);
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 684b0b315c32..44b823e5e8c8 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -2373,7 +2373,7 @@ void ptrace_triggered(struct perf_event *bp,
>  	/*
>  	 * Disable the breakpoint request here since ptrace has defined a
>  	 * one-shot behaviour for breakpoint exceptions in PPC64.
> -	 * The SIGTRAP signal is generated automatically for us in do_dabr().
> +	 * The SIGTRAP signal is generated automatically for us in do_break().
>  	 * We don't have to do anything about that here
>  	 */
>  	attr = bp->attr;

Re: [PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr()

[Michael Neuling]

This is going to collide with this patch 
https://patchwork.ozlabs.org/patch/1109594/

Mikey


On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
> Remove unnecessary comments. Code itself is self explanatory.
> And, ISA already talks about MRD field. I Don't think we need
> to re-describe it.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  arch/powerpc/kernel/process.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f0fbbf6a6a1f..f002d2ffff86 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -799,18 +799,11 @@ int set_dawr(struct arch_hw_breakpoint *brk)
>  
>  	dawr = brk->address;
>  
> -	dawrx  = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE)) \
> -		                   << (63 - 58); //* read/write bits */
> -	dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) \
> -		                   << (63 - 59); //* translate */
> -	dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) \
> -		                   >> 3; //* PRIM bits */
> -	/* dawr length is stored in field MDR bits 48:53.  Matches range in
> -	   doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
> -	   0b111111=64DW.
> -	   brk->len is in bytes.
> -	   This aligns up to double word size, shifts and does the bias.
> -	*/
> +	dawrx  = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
> +	dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
> +	dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
> +
> +	/* brk->len is in bytes. */
>  	mrd = ((brk->len + 7) >> 3) - 1;
>  	dawrx |= (mrd & 0x3f) << (63 - 53);
>  

Re: [PATCH 1/5] Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break()

[Christophe Leroy]

The subject text should mention you are changing comments. Here it 
suggests you are changing code text.

Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> do_dabr() was renamed with do_break() long ago. But I still see
> some comments mentioning do_dabr(). Replace it.

s/Replace it/Replace them/

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/hw_breakpoint.c | 2 +-
>   arch/powerpc/kernel/ptrace.c        | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index a293a53b4365..1908e4fcc132 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -232,7 +232,7 @@ int hw_breakpoint_handler(struct die_args *args)
>   	 * Return early after invoking user-callback function without restoring
>   	 * DABR if the breakpoint is from ptrace which always operates in
>   	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
> -	 * generated in do_dabr().
> +	 * generated in do_break().
>   	 */
>   	if (bp->overflow_handler == ptrace_triggered) {
>   		perf_bp_event(bp, regs);
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 684b0b315c32..44b823e5e8c8 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -2373,7 +2373,7 @@ void ptrace_triggered(struct perf_event *bp,
>   	/*
>   	 * Disable the breakpoint request here since ptrace has defined a
>   	 * one-shot behaviour for breakpoint exceptions in PPC64.
> -	 * The SIGTRAP signal is generated automatically for us in do_dabr().
> +	 * The SIGTRAP signal is generated automatically for us in do_break().
>   	 * We don't have to do anything about that here
>   	 */
>   	attr = bp->attr;
> 

Re: [PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path

[Michael Neuling]

On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
> Directly setting dawr and dawrx with 0 should be enough to
> disable watchpoint. No need to reset individual bits in
> variable and then set in hw.

This seems like a pointless optimisation to me. 

I'm all for adding more code/complexity if it buys us some performance, but I
can't imagine this is a fast path (nor have you stated any performance
benefits). 

Mikey

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hw_breakpoint.h |  3 ++-
>  arch/powerpc/kernel/process.c            | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h
> b/arch/powerpc/include/asm/hw_breakpoint.h
> index 78202d5fb13a..8acbbdd4a2d5 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -19,6 +19,7 @@ struct arch_hw_breakpoint {
>  /* Note: Don't change the the first 6 bits below as they are in the same
> order
>   * as the dabr and dabrx.
>   */
> +#define HW_BRK_TYPE_DISABLE		0x00
>  #define HW_BRK_TYPE_READ		0x01
>  #define HW_BRK_TYPE_WRITE		0x02
>  #define HW_BRK_TYPE_TRANSLATE		0x04
> @@ -68,7 +69,7 @@ static inline void hw_breakpoint_disable(void)
>  	struct arch_hw_breakpoint brk;
>  
>  	brk.address = 0;
> -	brk.type = 0;
> +	brk.type = HW_BRK_TYPE_DISABLE;
>  	brk.len = 0;
>  	if (ppc_breakpoint_available())
>  		__set_breakpoint(&brk);
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f002d2ffff86..265fac9fb3a4 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -793,10 +793,22 @@ static inline int set_dabr(struct arch_hw_breakpoint
> *brk)
>  	return __set_dabr(dabr, dabrx);
>  }
>  
> +static int disable_dawr(void)
> +{
> +	if (ppc_md.set_dawr)
> +		return ppc_md.set_dawr(0, 0);
> +
> +	mtspr(SPRN_DAWRX, 0);
> +	return 0;
> +}
> +
>  int set_dawr(struct arch_hw_breakpoint *brk)
>  {
>  	unsigned long dawr, dawrx, mrd;
>  
> +	if (brk->type == HW_BRK_TYPE_DISABLE)
> +		return disable_dawr();
> +
>  	dawr = brk->address;
>  
>  	dawrx  = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);

Re: [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor

[Michael Neuling]

On Tue, 2019-06-18 at 08:01 +0200, Christophe Leroy wrote:
> 
> Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> > patch 1-3: Code refactor
> > patch 4: Speedup disabling breakpoint
> > patch 5: Fix length calculation for unaligned targets
> 
> While you are playing with hw breakpoints, did you have a look at 
> https://github.com/linuxppc/issues/issues/38 ?

Agreed and also: 

https://github.com/linuxppc/issues/issues/170

https://github.com/linuxppc/issues/issues/128 

Mikey

Re: [PATCH 2/5] Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()

[Christophe Leroy]

Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> Move feature availability check at the start of the function.
> Rearrange comment to it's associated code. Use hw->address and
> hw->len in the 512 bytes boundary check(to write if statement
> in a single line). Add spacing between code blocks.

Are those cosmetic changes in the boundary check worth it since they 
disappear in the final patch ?

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/hw_breakpoint.c | 34 +++++++++++++++--------------
>   1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 1908e4fcc132..36bcf705df65 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -133,10 +133,13 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   			     const struct perf_event_attr *attr,
>   			     struct arch_hw_breakpoint *hw)
>   {
> -	int ret = -EINVAL, length_max;
> +	int length_max;
> +
> +	if (!ppc_breakpoint_available())
> +		return -ENODEV;
>   
>   	if (!bp)
> -		return ret;
> +		return -EINVAL;
>   
>   	hw->type = HW_BRK_TYPE_TRANSLATE;
>   	if (attr->bp_type & HW_BREAKPOINT_R)
> @@ -145,34 +148,33 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   		hw->type |= HW_BRK_TYPE_WRITE;
>   	if (hw->type == HW_BRK_TYPE_TRANSLATE)
>   		/* must set alteast read or write */
> -		return ret;
> +		return -EINVAL;
> +
>   	if (!attr->exclude_user)
>   		hw->type |= HW_BRK_TYPE_USER;
>   	if (!attr->exclude_kernel)
>   		hw->type |= HW_BRK_TYPE_KERNEL;
>   	if (!attr->exclude_hv)
>   		hw->type |= HW_BRK_TYPE_HYP;
> +
>   	hw->address = attr->bp_addr;
>   	hw->len = attr->bp_len;
>   
> -	/*
> -	 * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)
> -	 * and breakpoint addresses are aligned to nearest double-word
> -	 * 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 (dawr_enabled()) {
>   		length_max = 512 ; /* 64 doublewords */
> -		/* DAWR region can't cross 512 boundary */
> -		if ((attr->bp_addr >> 9) !=
> -		    ((attr->bp_addr + attr->bp_len - 1) >> 9))
> +		/* DAWR region can't cross 512 bytes boundary */
> +		if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
>   			return -EINVAL;
>   	}
> -	if (hw->len >
> -	    (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
> +
> +	/*
> +	 * Since breakpoint length can be a maximum of length_max and
> +	 * breakpoint addresses are aligned to nearest double-word
> +	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
> +	 * the 'symbolsize' should satisfy the check below.
> +	 */
> +	if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
>   		return -EINVAL;
>   	return 0;
>   }
> 

Re: [PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr()

[Christophe Leroy]

Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> Remove unnecessary comments. Code itself is self explanatory.
> And, ISA already talks about MRD field. I Don't think we need
> to re-describe it.

In an RFC patch you may "don't think".
But in the final patch you need to make a decision and write it as such.

Ie, you should write: "We don't need to re-describe it."


> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/process.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f0fbbf6a6a1f..f002d2ffff86 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -799,18 +799,11 @@ int set_dawr(struct arch_hw_breakpoint *brk)
>   
>   	dawr = brk->address;
>   
> -	dawrx  = (brk->type & (HW_BRK_TYPE_READ | HW_BRK_TYPE_WRITE)) \
> -		                   << (63 - 58); //* read/write bits */
> -	dawrx |= ((brk->type & (HW_BRK_TYPE_TRANSLATE)) >> 2) \
> -		                   << (63 - 59); //* translate */
> -	dawrx |= (brk->type & (HW_BRK_TYPE_PRIV_ALL)) \
> -		                   >> 3; //* PRIM bits */
> -	/* dawr length is stored in field MDR bits 48:53.  Matches range in
> -	   doublewords (64 bits) baised by -1 eg. 0b000000=1DW and
> -	   0b111111=64DW.
> -	   brk->len is in bytes.
> -	   This aligns up to double word size, shifts and does the bias.
> -	*/
> +	dawrx  = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
> +	dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
> +	dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
> +
> +	/* brk->len is in bytes. */
>   	mrd = ((brk->len + 7) >> 3) - 1;
>   	dawrx |= (mrd & 0x3f) << (63 - 53);
>   
> 

Re: [PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path

[Christophe Leroy]

Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> Directly setting dawr and dawrx with 0 should be enough to
> disable watchpoint. No need to reset individual bits in
> variable and then set in hw.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/hw_breakpoint.h |  3 ++-
>   arch/powerpc/kernel/process.c            | 12 ++++++++++++
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 78202d5fb13a..8acbbdd4a2d5 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -19,6 +19,7 @@ struct arch_hw_breakpoint {
>   /* Note: Don't change the the first 6 bits below as they are in the same order
>    * as the dabr and dabrx.
>    */
> +#define HW_BRK_TYPE_DISABLE		0x00

I'd rather call it HW_BRK_TYPE_NONE

>   #define HW_BRK_TYPE_READ		0x01
>   #define HW_BRK_TYPE_WRITE		0x02
>   #define HW_BRK_TYPE_TRANSLATE		0x04
> @@ -68,7 +69,7 @@ static inline void hw_breakpoint_disable(void)
>   	struct arch_hw_breakpoint brk;
>   
>   	brk.address = 0;
> -	brk.type = 0;
> +	brk.type = HW_BRK_TYPE_DISABLE;
>   	brk.len = 0;
>   	if (ppc_breakpoint_available())
>   		__set_breakpoint(&brk);
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f002d2ffff86..265fac9fb3a4 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -793,10 +793,22 @@ static inline int set_dabr(struct arch_hw_breakpoint *brk)
>   	return __set_dabr(dabr, dabrx);
>   }
>   
> +static int disable_dawr(void)
> +{
> +	if (ppc_md.set_dawr)
> +		return ppc_md.set_dawr(0, 0);
> +
> +	mtspr(SPRN_DAWRX, 0);

And SPRN_DAWR ?

The above code looks pretty similar to the one at the end of set_dawr(). 
You should factorise it, for instance

static int __set_dawr(int dawr, int dawrx)
{
	if (ppc_md.set_dawr)
		return ppc_md.set_dawr(dawr, dawrx);
	mtspr(SPRN_DAWR, dawr);
	mtspr(SPRN_DAWRX, dawrx);
	return 0;
}

> +	return 0;
> +}
> +
>   int set_dawr(struct arch_hw_breakpoint *brk)
>   {
>   	unsigned long dawr, dawrx, mrd;
>   
> +	if (brk->type == HW_BRK_TYPE_DISABLE)
> +		return disable_dawr();

Then replace by __set_dawr(0, 0);

> +
>   	dawr = brk->address;
>   
>   	dawrx  = (brk->type & HW_BRK_TYPE_RDWR) << (63 - 58);
> 

And use the new helper at the end of the function too.

Christophe

Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target

[Christophe Leroy]

Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> Watchpoint match range is always doubleword(8 bytes) aligned on
> powerpc. If the given range is crossing doubleword boundary, we
> need to increase the length such that next doubleword also get
> covered. Ex,
> 
>            address   len = 6 bytes
>                  |=========.
>     |------------v--|------v--------|
>     | | | | | | | | | | | | | | | | |
>     |---------------|---------------|
>      <---8 bytes--->
> 
> In such case, current code configures hw as:
>    start_addr = address & ~HW_BREAKPOINT_ALIGN
>    len = 8 bytes
> 
> And thus read/write in last 4 bytes of the given range is ignored.
> Fix this by including next doubleword in the length. Watchpoint
> exception handler already ignores extraneous exceptions, so no
> changes required for that.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/hw_breakpoint.h |  7 ++--
>   arch/powerpc/kernel/hw_breakpoint.c      | 44 +++++++++++++-----------
>   arch/powerpc/kernel/process.c            | 34 ++++++++++++++++--
>   3 files changed, 60 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 8acbbdd4a2d5..749a357164d5 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -34,6 +34,8 @@ struct arch_hw_breakpoint {
>   #define HW_BRK_TYPE_PRIV_ALL	(HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
>   				 HW_BRK_TYPE_HYP)
>   
> +#define HW_BREAKPOINT_ALIGN 0x7
> +
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
>   #include <linux/kdebug.h>
>   #include <asm/reg.h>
> @@ -45,8 +47,6 @@ struct pmu;
>   struct perf_sample_data;
>   struct task_struct;
>   
> -#define HW_BREAKPOINT_ALIGN 0x7
> -
>   extern int hw_breakpoint_slots(int type);
>   extern int arch_bp_generic_fields(int type, int *gen_bp_type);
>   extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
> @@ -76,7 +76,8 @@ static inline void hw_breakpoint_disable(void)
>   }
>   extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
>   int hw_breakpoint_handler(struct die_args *args);
> -
> +extern u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
> +		unsigned long *start_addr, unsigned long *end_addr);
>   extern int set_dawr(struct arch_hw_breakpoint *brk);
>   extern bool dawr_force_enable;
>   static inline bool dawr_enabled(void)
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 36bcf705df65..c122fd55aa44 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -126,6 +126,28 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
>   	return 0;
>   }
>   
> +/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */
> +static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
> +{
> +	u16 length_max = 8;
> +	u16 final_len;

You should be more consistent in naming. If one is called final_len, the 
other one should be called max_len.

> +	unsigned long start_addr, end_addr;
> +
> +	final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr);
> +
> +	if (dawr_enabled()) {
> +		length_max = 512;
> +		/* DAWR region can't cross 512 bytes boundary */
> +		if ((start_addr >> 9) != (end_addr >> 9))
> +			return -EINVAL;
> +	}
> +
> +	if (final_len > length_max)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +

Is many places, we have those numeric 512 and 9 shift. Could we replace 
them by some symbol, for instance DAWR_SIZE and DAWR_SHIFT ?

>   /*
>    * Validate the arch-specific HW Breakpoint register settings
>    */
> @@ -133,12 +155,10 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   			     const struct perf_event_attr *attr,
>   			     struct arch_hw_breakpoint *hw)
>   {
> -	int length_max;
> -
>   	if (!ppc_breakpoint_available())
>   		return -ENODEV;
>   
> -	if (!bp)
> +	if (!bp || !attr->bp_len)
>   		return -EINVAL;
>   
>   	hw->type = HW_BRK_TYPE_TRANSLATE;
> @@ -160,23 +180,7 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   	hw->address = attr->bp_addr;
>   	hw->len = attr->bp_len;
>   
> -	length_max = 8; /* DABR */
> -	if (dawr_enabled()) {
> -		length_max = 512 ; /* 64 doublewords */
> -		/* DAWR region can't cross 512 bytes boundary */
> -		if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
> -			return -EINVAL;
> -	}
> -
> -	/*
> -	 * Since breakpoint length can be a maximum of length_max and
> -	 * breakpoint addresses are aligned to nearest double-word
> -	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
> -	 * the 'symbolsize' should satisfy the check below.
> -	 */
> -	if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
> -		return -EINVAL;
> -	return 0;
> +	return hw_breakpoint_validate_len(hw);
>   }
>   
>   /*
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 265fac9fb3a4..159aaa70de46 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -802,9 +802,39 @@ static int disable_dawr(void)
>   	return 0;
>   }
>   
> +/*
> + * Watchpoint match range is always doubleword(8 bytes) aligned on
> + * powerpc. If the given range is crossing doubleword boundary, we
> + * need to increase the length such that next doubleword also get
> + * covered. Ex,
> + *
> + *          address   len = 6 bytes
> + *                |=========.
> + *   |------------v--|------v--------|
> + *   | | | | | | | | | | | | | | | | |
> + *   |---------------|---------------|
> + *    <---8 bytes--->
> + *
> + * In this case, we should configure hw as:
> + *   start_addr = address & ~HW_BREAKPOINT_ALIGN
> + *   len = 16 bytes
> + *
> + * @start_addr and @end_addr are inclusive.
> + */
> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
> +				unsigned long *start_addr,
> +				unsigned long *end_addr)
> +{
> +	*start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
> +	*end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
> +	return *end_addr - *start_addr + 1;
> +}

This function gives horrible code (a couple of unneeded store/re-read 
and read/re-read).

000006bc <hw_breakpoint_get_final_len>:
      6bc:	81 23 00 00 	lwz     r9,0(r3)
      6c0:	55 29 00 38 	rlwinm  r9,r9,0,0,28
      6c4:	91 24 00 00 	stw     r9,0(r4)
      6c8:	81 43 00 00 	lwz     r10,0(r3)
      6cc:	a1 23 00 06 	lhz     r9,6(r3)
      6d0:	38 6a ff ff 	addi    r3,r10,-1
      6d4:	7c 63 4a 14 	add     r3,r3,r9
      6d8:	60 63 00 07 	ori     r3,r3,7
      6dc:	90 65 00 00 	stw     r3,0(r5)
      6e0:	38 63 00 01 	addi    r3,r3,1
      6e4:	81 24 00 00 	lwz     r9,0(r4)
      6e8:	7c 69 18 50 	subf    r3,r9,r3
      6ec:	54 63 04 3e 	clrlwi  r3,r3,16
      6f0:	4e 80 00 20 	blr

Below code gives something better:

u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
				unsigned long *start_addr,
				unsigned long *end_addr)
{
	unsigned long address = brk->address;
	unsigned long len = brk->len;
	unsigned long start = address & ~HW_BREAKPOINT_ALIGN;
	unsigned long end = (address + len - 1) | HW_BREAKPOINT_ALIGN;

	*start_addr = start;
	*end_addr = end;
	return end - start + 1;
}

000006bc <hw_breakpoint_get_final_len>:
      6bc:	81 43 00 00 	lwz     r10,0(r3)
      6c0:	a1 03 00 06 	lhz     r8,6(r3)
      6c4:	39 2a ff ff 	addi    r9,r10,-1
      6c8:	7d 28 4a 14 	add     r9,r8,r9
      6cc:	55 4a 00 38 	rlwinm  r10,r10,0,0,28
      6d0:	61 29 00 07 	ori     r9,r9,7
      6d4:	91 44 00 00 	stw     r10,0(r4)
      6d8:	20 6a 00 01 	subfic  r3,r10,1
      6dc:	91 25 00 00 	stw     r9,0(r5)
      6e0:	7c 63 4a 14 	add     r3,r3,r9
      6e4:	54 63 04 3e 	clrlwi  r3,r3,16
      6e8:	4e 80 00 20 	blr


And regardless, that's a pitty to have this function using pointers 
which are from local variables in the callers, as we loose the benefit 
of registers. Couldn't this function go in the .h as a static inline ? 
I'm sure the result would be worth it.

Christophe

> +
>   int set_dawr(struct arch_hw_breakpoint *brk)
>   {
>   	unsigned long dawr, dawrx, mrd;
> +	unsigned long start_addr, end_addr;
> +	u16 final_len;
>   
>   	if (brk->type == HW_BRK_TYPE_DISABLE)
>   		return disable_dawr();
> @@ -815,8 +845,8 @@ int set_dawr(struct arch_hw_breakpoint *brk)
>   	dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
>   	dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
>   
> -	/* brk->len is in bytes. */
> -	mrd = ((brk->len + 7) >> 3) - 1;
> +	final_len = hw_breakpoint_get_final_len(brk, &start_addr, &end_addr);
> +	mrd = ((final_len + 7) >> 3) - 1;
>   	dawrx |= (mrd & 0x3f) << (63 - 53);
>   
>   	if (ppc_md.set_dawr)
> 

Re: [PATCH 2/5] Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()

[Ravi Bangoria]

On 6/18/19 11:51 AM, Christophe Leroy wrote:
> 
> 
> Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
>> Move feature availability check at the start of the function.
>> Rearrange comment to it's associated code. Use hw->address and
>> hw->len in the 512 bytes boundary check(to write if statement
>> in a single line). Add spacing between code blocks.
> 
> Are those cosmetic changes in the boundary check worth it since they disappear in the final patch ?

Nope.. not necessary. I was just going bit more patch by patch.
I don't mind keeping the code as it is and then change it in
the final patch.

Re: [PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr()

[Ravi Bangoria]

On 6/18/19 11:41 AM, Michael Neuling wrote:
> This is going to collide with this patch 
> https://patchwork.ozlabs.org/patch/1109594/

Yeah, I'm aware of the patch. I just developed this on powerpc/next.
I'll rebase my patches accordingly once mpe picks up that patche.

Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target

[Michael Neuling]

On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
> Watchpoint match range is always doubleword(8 bytes) aligned on
> powerpc. If the given range is crossing doubleword boundary, we
> need to increase the length such that next doubleword also get
> covered. Ex,
> 
>           address   len = 6 bytes
>                 |=========.
>    |------------v--|------v--------|
>    | | | | | | | | | | | | | | | | |
>    |---------------|---------------|
>     <---8 bytes--->
> 
> In such case, current code configures hw as:
>   start_addr = address & ~HW_BREAKPOINT_ALIGN
>   len = 8 bytes
> 
> And thus read/write in last 4 bytes of the given range is ignored.
> Fix this by including next doubleword in the length. Watchpoint
> exception handler already ignores extraneous exceptions, so no
> changes required for that.

Nice catch. Thanks.

I assume this has been broken forever? Should we be CCing stable? If so, it
would be nice to have this self contained (separate from the refactor) so we can
more easily backport it.

Also, can you update 
tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c to catch this issue?

A couple more comments below.

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hw_breakpoint.h |  7 ++--
>  arch/powerpc/kernel/hw_breakpoint.c      | 44 +++++++++++++-----------
>  arch/powerpc/kernel/process.c            | 34 ++++++++++++++++--
>  3 files changed, 60 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h
> b/arch/powerpc/include/asm/hw_breakpoint.h
> index 8acbbdd4a2d5..749a357164d5 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -34,6 +34,8 @@ struct arch_hw_breakpoint {
>  #define HW_BRK_TYPE_PRIV_ALL	(HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
>  				 HW_BRK_TYPE_HYP)
>  
> +#define HW_BREAKPOINT_ALIGN 0x7
> +
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  #include <linux/kdebug.h>
>  #include <asm/reg.h>
> @@ -45,8 +47,6 @@ struct pmu;
>  struct perf_sample_data;
>  struct task_struct;
>  
> -#define HW_BREAKPOINT_ALIGN 0x7
> -
>  extern int hw_breakpoint_slots(int type);
>  extern int arch_bp_generic_fields(int type, int *gen_bp_type);
>  extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
> @@ -76,7 +76,8 @@ static inline void hw_breakpoint_disable(void)
>  }
>  extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
>  int hw_breakpoint_handler(struct die_args *args);
> -
> +extern u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
> +		unsigned long *start_addr, unsigned long *end_addr);
>  extern int set_dawr(struct arch_hw_breakpoint *brk);
>  extern bool dawr_force_enable;
>  static inline bool dawr_enabled(void)
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c
> b/arch/powerpc/kernel/hw_breakpoint.c
> index 36bcf705df65..c122fd55aa44 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -126,6 +126,28 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
>  	return 0;
>  }
>  
> +/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */
> +static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
> +{
> +	u16 length_max = 8;
> +	u16 final_len;
> +	unsigned long start_addr, end_addr;
> +
> +	final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr);
> +
> +	if (dawr_enabled()) {
> +		length_max = 512;
> +		/* DAWR region can't cross 512 bytes boundary */
> +		if ((start_addr >> 9) != (end_addr >> 9))
> +			return -EINVAL;
> +	}
> +
> +	if (final_len > length_max)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /*
>   * Validate the arch-specific HW Breakpoint register settings
>   */
> @@ -133,12 +155,10 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>  			     const struct perf_event_attr *attr,
>  			     struct arch_hw_breakpoint *hw)
>  {
> -	int length_max;
> -
>  	if (!ppc_breakpoint_available())
>  		return -ENODEV;
>  
> -	if (!bp)
> +	if (!bp || !attr->bp_len)
>  		return -EINVAL;
>  
>  	hw->type = HW_BRK_TYPE_TRANSLATE;
> @@ -160,23 +180,7 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>  	hw->address = attr->bp_addr;
>  	hw->len = attr->bp_len;
>  
> -	length_max = 8; /* DABR */
> -	if (dawr_enabled()) {
> -		length_max = 512 ; /* 64 doublewords */
> -		/* DAWR region can't cross 512 bytes boundary */
> -		if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
> -			return -EINVAL;
> -	}
> -
> -	/*
> -	 * Since breakpoint length can be a maximum of length_max and
> -	 * breakpoint addresses are aligned to nearest double-word
> -	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
> -	 * the 'symbolsize' should satisfy the check below.
> -	 */
> -	if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
> -		return -EINVAL;
> -	return 0;
> +	return hw_breakpoint_validate_len(hw);
>  }
>  
>  /*
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 265fac9fb3a4..159aaa70de46 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -802,9 +802,39 @@ static int disable_dawr(void)
>  	return 0;
>  }
>  
> +/*
> + * Watchpoint match range is always doubleword(8 bytes) aligned on
> + * powerpc. If the given range is crossing doubleword boundary, we
> + * need to increase the length such that next doubleword also get
> + * covered. Ex,
> + *
> + *          address   len = 6 bytes
> + *                |=========.
> + *   |------------v--|------v--------|
> + *   | | | | | | | | | | | | | | | | |
> + *   |---------------|---------------|
> + *    <---8 bytes--->
> + *
> + * In this case, we should configure hw as:
> + *   start_addr = address & ~HW_BREAKPOINT_ALIGN
> + *   len = 16 bytes
> + *
> + * @start_addr and @end_addr are inclusive.
> + */
> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
> +				unsigned long *start_addr,
> +				unsigned long *end_addr)

I don't really like this.  "final" is not a good name. Something like hardware
would be better.

Also, can you put the start_addr and end addr in the arch_hw_breakpoint rather
than doing what you have above.  Call them hw_start_addr, hw_end_addr.

We could even set these two new addresses where we set the set of
arch_hw_breakpoint rather than having this late call.

> +{
> +	*start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
> +	*end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
> +	return *end_addr - *start_addr + 1;
> +}
> +
>  int set_dawr(struct arch_hw_breakpoint *brk)
>  {
>  	unsigned long dawr, dawrx, mrd;
> +	unsigned long start_addr, end_addr;
> +	u16 final_len;
>  
>  	if (brk->type == HW_BRK_TYPE_DISABLE)
>  		return disable_dawr();
> @@ -815,8 +845,8 @@ int set_dawr(struct arch_hw_breakpoint *brk)
>  	dawrx |= ((brk->type & HW_BRK_TYPE_TRANSLATE) >> 2) << (63 - 59);
>  	dawrx |= (brk->type & HW_BRK_TYPE_PRIV_ALL) >> 3;
>  
> -	/* brk->len is in bytes. */
> -	mrd = ((brk->len + 7) >> 3) - 1;
> +	final_len = hw_breakpoint_get_final_len(brk, &start_addr, &end_addr);

Again, hardware length, or something other than "final"

> +	mrd = ((final_len + 7) >> 3) - 1;
>  	dawrx |= (mrd & 0x3f) << (63 - 53);
>  
>  	if (ppc_md.set_dawr)

Re: [PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path

[Ravi Bangoria]

On 6/18/19 11:45 AM, Michael Neuling wrote:
> On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
>> Directly setting dawr and dawrx with 0 should be enough to
>> disable watchpoint. No need to reset individual bits in
>> variable and then set in hw.
> 
> This seems like a pointless optimisation to me. 
> 
> I'm all for adding more code/complexity if it buys us some performance, but I
> can't imagine this is a fast path (nor have you stated any performance
> benefits). 

This gets called from sched_switch. I expected the improvement when
we switch from monitored process to non-monitored process. With such
scenario, I tried to measure the difference in execution time of
set_dawr but I don't see any improvement. So I'll drop the patch.

Re: [PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path

[Ravi Bangoria]

On 6/18/19 12:01 PM, Christophe Leroy wrote:
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index f002d2ffff86..265fac9fb3a4 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -793,10 +793,22 @@ static inline int set_dabr(struct arch_hw_breakpoint *brk)
>>       return __set_dabr(dabr, dabrx);
>>   }
>>   +static int disable_dawr(void)
>> +{
>> +    if (ppc_md.set_dawr)
>> +        return ppc_md.set_dawr(0, 0);
>> +
>> +    mtspr(SPRN_DAWRX, 0);
> 
> And SPRN_DAWR ?

Setting DAWRx with 0 should be enough to disable the breakpoint.

Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target

[Ravi Bangoria]

On 6/18/19 12:16 PM, Christophe Leroy wrote:
>>   +/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */
>> +static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
>> +{
>> +    u16 length_max = 8;
>> +    u16 final_len;
> 
> You should be more consistent in naming. If one is called final_len, the other one should be called max_len.

Copy/paste :). Will change it.

> 
>> +    unsigned long start_addr, end_addr;
>> +
>> +    final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr);
>> +
>> +    if (dawr_enabled()) {
>> +        length_max = 512;
>> +        /* DAWR region can't cross 512 bytes boundary */
>> +        if ((start_addr >> 9) != (end_addr >> 9))
>> +            return -EINVAL;
>> +    }
>> +
>> +    if (final_len > length_max)
>> +        return -EINVAL;
>> +
>> +    return 0;
>> +}
>> +
> 
> Is many places, we have those numeric 512 and 9 shift. Could we replace them by some symbol, for instance DAWR_SIZE and DAWR_SHIFT ?

I don't see any other place where we check for boundary limit.

[...]

> 
>> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
>> +                unsigned long *start_addr,
>> +                unsigned long *end_addr)
>> +{
>> +    *start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
>> +    *end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
>> +    return *end_addr - *start_addr + 1;
>> +}
> 
> This function gives horrible code (a couple of unneeded store/re-read and read/re-read).
> 
> 000006bc <hw_breakpoint_get_final_len>:
>      6bc:    81 23 00 00     lwz     r9,0(r3)
>      6c0:    55 29 00 38     rlwinm  r9,r9,0,0,28
>      6c4:    91 24 00 00     stw     r9,0(r4)
>      6c8:    81 43 00 00     lwz     r10,0(r3)
>      6cc:    a1 23 00 06     lhz     r9,6(r3)
>      6d0:    38 6a ff ff     addi    r3,r10,-1
>      6d4:    7c 63 4a 14     add     r3,r3,r9
>      6d8:    60 63 00 07     ori     r3,r3,7
>      6dc:    90 65 00 00     stw     r3,0(r5)
>      6e0:    38 63 00 01     addi    r3,r3,1
>      6e4:    81 24 00 00     lwz     r9,0(r4)
>      6e8:    7c 69 18 50     subf    r3,r9,r3
>      6ec:    54 63 04 3e     clrlwi  r3,r3,16
>      6f0:    4e 80 00 20     blr
> 
> Below code gives something better:
> 
> u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
>                 unsigned long *start_addr,
>                 unsigned long *end_addr)
> {
>     unsigned long address = brk->address;
>     unsigned long len = brk->len;
>     unsigned long start = address & ~HW_BREAKPOINT_ALIGN;
>     unsigned long end = (address + len - 1) | HW_BREAKPOINT_ALIGN;
> 
>     *start_addr = start;
>     *end_addr = end;
>     return end - start + 1;
> }
> 
> 000006bc <hw_breakpoint_get_final_len>:
>      6bc:    81 43 00 00     lwz     r10,0(r3)
>      6c0:    a1 03 00 06     lhz     r8,6(r3)
>      6c4:    39 2a ff ff     addi    r9,r10,-1
>      6c8:    7d 28 4a 14     add     r9,r8,r9
>      6cc:    55 4a 00 38     rlwinm  r10,r10,0,0,28
>      6d0:    61 29 00 07     ori     r9,r9,7
>      6d4:    91 44 00 00     stw     r10,0(r4)
>      6d8:    20 6a 00 01     subfic  r3,r10,1
>      6dc:    91 25 00 00     stw     r9,0(r5)
>      6e0:    7c 63 4a 14     add     r3,r3,r9
>      6e4:    54 63 04 3e     clrlwi  r3,r3,16
>      6e8:    4e 80 00 20     blr
> 
> 
> And regardless, that's a pitty to have this function using pointers which are from local variables in the callers, as we loose the benefit of registers. Couldn't this function go in the .h as a static inline ? I'm sure the result would be worth it.

This is obviously a bit of optimization, but I like Mikey's idea of
storing start_addr and end_addr in the arch_hw_breakpoint. That way
we don't have to recalculate length every time in set_dawr.

Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target

[Ravi Bangoria]

On 6/18/19 7:02 PM, Michael Neuling wrote:
> On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
>> Watchpoint match range is always doubleword(8 bytes) aligned on
>> powerpc. If the given range is crossing doubleword boundary, we
>> need to increase the length such that next doubleword also get
>> covered. Ex,
>>
>>           address   len = 6 bytes
>>                 |=========.
>>    |------------v--|------v--------|
>>    | | | | | | | | | | | | | | | | |
>>    |---------------|---------------|
>>     <---8 bytes--->
>>
>> In such case, current code configures hw as:
>>   start_addr = address & ~HW_BREAKPOINT_ALIGN
>>   len = 8 bytes
>>
>> And thus read/write in last 4 bytes of the given range is ignored.
>> Fix this by including next doubleword in the length. Watchpoint
>> exception handler already ignores extraneous exceptions, so no
>> changes required for that.
> 
> Nice catch. Thanks.
> 
> I assume this has been broken forever? Should we be CCing stable? If so, it
> would be nice to have this self contained (separate from the refactor) so we can
> more easily backport it.

Yes this has been broken forever. I'll add Fixes: tag and cc stable.

> 
> Also, can you update 
> tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c to catch this issue?

Sure, will add the test case.

[...]

>> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
>> +				unsigned long *start_addr,
>> +				unsigned long *end_addr)
> 
> I don't really like this.  "final" is not a good name. Something like hardware
> would be better.
> 
> Also, can you put the start_addr and end addr in the arch_hw_breakpoint rather
> than doing what you have above.  Call them hw_start_addr, hw_end_addr.
> 
> We could even set these two new addresses where we set the set of
> arch_hw_breakpoint rather than having this late call.

Sure, will use 'hw_' prefix for them.

Re: [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor

[Ravi Bangoria]

On 6/18/19 11:47 AM, Michael Neuling wrote:
> On Tue, 2019-06-18 at 08:01 +0200, Christophe Leroy wrote:
>>
>> Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
>>> patch 1-3: Code refactor
>>> patch 4: Speedup disabling breakpoint
>>> patch 5: Fix length calculation for unaligned targets
>>
>> While you are playing with hw breakpoints, did you have a look at 
>> https://github.com/linuxppc/issues/issues/38 ?
> 
> Agreed and also: 
> 
> https://github.com/linuxppc/issues/issues/170
> 
> https://github.com/linuxppc/issues/issues/128 
> 

Yes, I'm aware of those. Will have a look at them.