diff mbox series

[v3,12/16] powerpc/watchpoint: Use builtin ALIGN*() macros

Message ID 20200414031659.58875-13-ravi.bangoria@linux.ibm.com (mailing list archive)
State Superseded
Headers show
Series powerpc/watchpoint: Preparation for more than one watchpoint | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (a9aa21d05c33c556e48c5062b6632a9b94906570)
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 74 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Ravi Bangoria April 14, 2020, 3:16 a.m. UTC 
Currently we calculate hw aligned start and end addresses manually.
Replace them with builtin ALIGN_DOWN() and ALIGN() macros.

Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h  |  5 +++--
 arch/powerpc/kernel/hw_breakpoint.c       | 12 ++++++------
 arch/powerpc/kernel/process.c             |  8 ++++----
 arch/powerpc/kernel/ptrace/ptrace-noadv.c |  2 +-
 4 files changed, 14 insertions(+), 13 deletions(-)

Comments

Christophe Leroy April 14, 2020, 5:48 a.m. UTC | #1 
Le 14/04/2020 à 05:16, Ravi Bangoria a écrit :
> Currently we calculate hw aligned start and end addresses manually.
> Replace them with builtin ALIGN_DOWN() and ALIGN() macros.
> 
> Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/hw_breakpoint.h  |  5 +++--
>   arch/powerpc/kernel/hw_breakpoint.c       | 12 ++++++------
>   arch/powerpc/kernel/process.c             |  8 ++++----
>   arch/powerpc/kernel/ptrace/ptrace-noadv.c |  2 +-
>   4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index d472b2eb757e..add5aa076919 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -34,10 +34,11 @@ struct arch_hw_breakpoint {
>   #define HW_BRK_TYPE_PRIV_ALL	(HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
>   				 HW_BRK_TYPE_HYP)
>   
> +/* Minimum granularity */
>   #ifdef CONFIG_PPC_8xx
> -#define HW_BREAKPOINT_ALIGN 0x3
> +#define HW_BREAKPOINT_SIZE  0x4
>   #else
> -#define HW_BREAKPOINT_ALIGN 0x7
> +#define HW_BREAKPOINT_SIZE  0x8
>   #endif
>   
>   #define DABR_MAX_LEN	8
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 319a761b7412..02ffd14f4519 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -145,10 +145,10 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
>    *    <---8 bytes--->
>    *
>    * In this case, we should configure hw as:
> - *   start_addr = address & ~HW_BREAKPOINT_ALIGN
> + *   start_addr = address & ~(HW_BREAKPOINT_SIZE - 1)
>    *   len = 16 bytes
>    *
> - * @start_addr and @end_addr are inclusive.
> + * @start_addr is inclusive but @end_addr is exclusive.
>    */
>   static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
>   {
> @@ -156,14 +156,14 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
>   	u16 hw_len;
>   	unsigned long start_addr, end_addr;
>   
> -	start_addr = hw->address & ~HW_BREAKPOINT_ALIGN;
> -	end_addr = (hw->address + hw->len - 1) | HW_BREAKPOINT_ALIGN;
> -	hw_len = end_addr - start_addr + 1;
> +	start_addr = ALIGN_DOWN(hw->address, HW_BREAKPOINT_SIZE);
> +	end_addr = ALIGN(hw->address + hw->len, HW_BREAKPOINT_SIZE);
> +	hw_len = end_addr - start_addr;
>   
>   	if (dawr_enabled()) {
>   		max_len = DAWR_MAX_LEN;
>   		/* DAWR region can't cross 512 bytes boundary */
> -		if ((start_addr >> 9) != (end_addr >> 9))
> +		if ((start_addr >> 9) != ((end_addr - 1) >> 9))

What about:
	if (ALIGN(start_addr, SZ_512M) != ALIGN(end - 1, SZ_512M))

>   			return -EINVAL;
>   	} else if (IS_ENABLED(CONFIG_PPC_8xx)) {
>   		/* 8xx can setup a range without limitation */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index aab82ab80dfa..06679adac447 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -800,12 +800,12 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
>   	unsigned long lctrl1 = LCTRL1_CTE_GT | LCTRL1_CTF_LT | LCTRL1_CRWE_RW |
>   			       LCTRL1_CRWF_RW;
>   	unsigned long lctrl2 = LCTRL2_LW0EN | LCTRL2_LW0LADC | LCTRL2_SLW0EN;
> -	unsigned long start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
> -	unsigned long end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
> +	unsigned long start_addr = ALIGN_DOWN(brk->address, HW_BREAKPOINT_SIZE);
> +	unsigned long end_addr = ALIGN(brk->address + brk->len, HW_BREAKPOINT_SIZE);
>   
>   	if (start_addr == 0)
>   		lctrl2 |= LCTRL2_LW0LA_F;
> -	else if (end_addr == ~0U)
> +	else if (end_addr - 1 == ~0U)

What about:
	else if (end_addr == 0)

>   		lctrl2 |= LCTRL2_LW0LA_E;
>   	else
>   		lctrl2 |= LCTRL2_LW0LA_EandF;
> @@ -821,7 +821,7 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
>   		lctrl1 |= LCTRL1_CRWE_WO | LCTRL1_CRWF_WO;
>   
>   	mtspr(SPRN_CMPE, start_addr - 1);
> -	mtspr(SPRN_CMPF, end_addr + 1);
> +	mtspr(SPRN_CMPF, end_addr);
>   	mtspr(SPRN_LCTRL1, lctrl1);
>   	mtspr(SPRN_LCTRL2, lctrl2);
>   
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 08cb8c1b504c..697c7e4b5877 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -216,7 +216,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
>   	if ((unsigned long)bp_info->addr >= TASK_SIZE)
>   		return -EIO;
>   
> -	brk.address = bp_info->addr & ~HW_BREAKPOINT_ALIGN;
> +	brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
>   	brk.type = HW_BRK_TYPE_TRANSLATE;
>   	brk.len = DABR_MAX_LEN;
>   	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> 

Christophe
Ravi Bangoria April 24, 2020, 3:24 a.m. UTC | #2 
Hi Christophe,

>>           max_len = DAWR_MAX_LEN;
>>           /* DAWR region can't cross 512 bytes boundary */
>> -        if ((start_addr >> 9) != (end_addr >> 9))
>> +        if ((start_addr >> 9) != ((end_addr - 1) >> 9))
> 
> What about:
>      if (ALIGN(start_addr, SZ_512M) != ALIGN(end - 1, SZ_512M))

ok.

> 
>>               return -EINVAL;
>>       } else if (IS_ENABLED(CONFIG_PPC_8xx)) {
>>           /* 8xx can setup a range without limitation */
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index aab82ab80dfa..06679adac447 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -800,12 +800,12 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
>>       unsigned long lctrl1 = LCTRL1_CTE_GT | LCTRL1_CTF_LT | LCTRL1_CRWE_RW |
>>                      LCTRL1_CRWF_RW;
>>       unsigned long lctrl2 = LCTRL2_LW0EN | LCTRL2_LW0LADC | LCTRL2_SLW0EN;
>> -    unsigned long start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
>> -    unsigned long end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
>> +    unsigned long start_addr = ALIGN_DOWN(brk->address, HW_BREAKPOINT_SIZE);
>> +    unsigned long end_addr = ALIGN(brk->address + brk->len, HW_BREAKPOINT_SIZE);
>>       if (start_addr == 0)
>>           lctrl2 |= LCTRL2_LW0LA_F;
>> -    else if (end_addr == ~0U)
>> +    else if (end_addr - 1 == ~0U)
> 
> What about:
>      else if (end_addr == 0)

That's better.

Thanks,
Ravi
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index d472b2eb757e..add5aa076919 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -34,10 +34,11 @@  struct arch_hw_breakpoint {
 #define HW_BRK_TYPE_PRIV_ALL	(HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
 				 HW_BRK_TYPE_HYP)
 
+/* Minimum granularity */
 #ifdef CONFIG_PPC_8xx
-#define HW_BREAKPOINT_ALIGN 0x3
+#define HW_BREAKPOINT_SIZE  0x4
 #else
-#define HW_BREAKPOINT_ALIGN 0x7
+#define HW_BREAKPOINT_SIZE  0x8
 #endif
 
 #define DABR_MAX_LEN	8
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 319a761b7412..02ffd14f4519 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -145,10 +145,10 @@  int arch_bp_generic_fields(int type, int *gen_bp_type)
  *    <---8 bytes--->
  *
  * In this case, we should configure hw as:
- *   start_addr = address & ~HW_BREAKPOINT_ALIGN
+ *   start_addr = address & ~(HW_BREAKPOINT_SIZE - 1)
  *   len = 16 bytes
  *
- * @start_addr and @end_addr are inclusive.
+ * @start_addr is inclusive but @end_addr is exclusive.
  */
 static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
 {
@@ -156,14 +156,14 @@  static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
 	u16 hw_len;
 	unsigned long start_addr, end_addr;
 
-	start_addr = hw->address & ~HW_BREAKPOINT_ALIGN;
-	end_addr = (hw->address + hw->len - 1) | HW_BREAKPOINT_ALIGN;
-	hw_len = end_addr - start_addr + 1;
+	start_addr = ALIGN_DOWN(hw->address, HW_BREAKPOINT_SIZE);
+	end_addr = ALIGN(hw->address + hw->len, HW_BREAKPOINT_SIZE);
+	hw_len = end_addr - start_addr;
 
 	if (dawr_enabled()) {
 		max_len = DAWR_MAX_LEN;
 		/* DAWR region can't cross 512 bytes boundary */
-		if ((start_addr >> 9) != (end_addr >> 9))
+		if ((start_addr >> 9) != ((end_addr - 1) >> 9))
 			return -EINVAL;
 	} else if (IS_ENABLED(CONFIG_PPC_8xx)) {
 		/* 8xx can setup a range without limitation */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index aab82ab80dfa..06679adac447 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -800,12 +800,12 @@  static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
 	unsigned long lctrl1 = LCTRL1_CTE_GT | LCTRL1_CTF_LT | LCTRL1_CRWE_RW |
 			       LCTRL1_CRWF_RW;
 	unsigned long lctrl2 = LCTRL2_LW0EN | LCTRL2_LW0LADC | LCTRL2_SLW0EN;
-	unsigned long start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
-	unsigned long end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
+	unsigned long start_addr = ALIGN_DOWN(brk->address, HW_BREAKPOINT_SIZE);
+	unsigned long end_addr = ALIGN(brk->address + brk->len, HW_BREAKPOINT_SIZE);
 
 	if (start_addr == 0)
 		lctrl2 |= LCTRL2_LW0LA_F;
-	else if (end_addr == ~0U)
+	else if (end_addr - 1 == ~0U)
 		lctrl2 |= LCTRL2_LW0LA_E;
 	else
 		lctrl2 |= LCTRL2_LW0LA_EandF;
@@ -821,7 +821,7 @@  static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
 		lctrl1 |= LCTRL1_CRWE_WO | LCTRL1_CRWF_WO;
 
 	mtspr(SPRN_CMPE, start_addr - 1);
-	mtspr(SPRN_CMPF, end_addr + 1);
+	mtspr(SPRN_CMPF, end_addr);
 	mtspr(SPRN_LCTRL1, lctrl1);
 	mtspr(SPRN_LCTRL2, lctrl2);
 
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 08cb8c1b504c..697c7e4b5877 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -216,7 +216,7 @@  long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
 	if ((unsigned long)bp_info->addr >= TASK_SIZE)
 		return -EIO;
 
-	brk.address = bp_info->addr & ~HW_BREAKPOINT_ALIGN;
+	brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
 	brk.type = HW_BRK_TYPE_TRANSLATE;
 	brk.len = DABR_MAX_LEN;
 	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)