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 |
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 |
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
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 --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)
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(-)