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

2020-04-23 Thread Ravi Bangoria

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


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

2020-04-13 Thread Christophe Leroy




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 
Signed-off-by: Ravi Bangoria 
---
  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