Messages in this thread | | | Subject | Re: [PATCH v6 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c | From | Rogerio Alves <> | Date | Thu, 17 Sep 2020 10:25:47 -0300 |
| |
On 9/2/20 1:29 AM, Ravi Bangoria wrote: > Power10 hw has multiple DAWRs but hw doesn't tell which DAWR caused > the exception. So we have a sw logic to detect that in hw_breakpoint.c. > But hw_breakpoint.c gets compiled only with CONFIG_HAVE_HW_BREAKPOINT=Y. > Move DAWR detection logic outside of hw_breakpoint.c so that it can be > reused when CONFIG_HAVE_HW_BREAKPOINT is not set. > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> Tested-by: Rogerio Alves <rcardoso@linux.ibm.com> > --- > arch/powerpc/include/asm/hw_breakpoint.h | 8 + > arch/powerpc/kernel/Makefile | 3 +- > arch/powerpc/kernel/hw_breakpoint.c | 159 +---------------- > .../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++ > 4 files changed, 174 insertions(+), 158 deletions(-) > create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c > > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > index 9b68eafebf43..81872c420476 100644 > --- a/arch/powerpc/include/asm/hw_breakpoint.h > +++ b/arch/powerpc/include/asm/hw_breakpoint.h > @@ -10,6 +10,7 @@ > #define _PPC_BOOK3S_64_HW_BREAKPOINT_H > > #include <asm/cpu_has_feature.h> > +#include <asm/inst.h> > > #ifdef __KERNEL__ > struct arch_hw_breakpoint { > @@ -52,6 +53,13 @@ static inline int nr_wp_slots(void) > return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1; > } > > +bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr, > + unsigned long ea, int type, int size, > + struct arch_hw_breakpoint *info); > + > +void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr, > + int *type, int *size, unsigned long *ea); > + > #ifdef CONFIG_HAVE_HW_BREAKPOINT > #include <linux/kdebug.h> > #include <asm/reg.h> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile > index cbf41fb4ee89..a5550c2b24c4 100644 > --- a/arch/powerpc/kernel/Makefile > +++ b/arch/powerpc/kernel/Makefile > @@ -45,7 +45,8 @@ obj-y := cputable.o syscalls.o \ > signal.o sysfs.o cacheinfo.o time.o \ > prom.o traps.o setup-common.o \ > udbg.o misc.o io.o misc_$(BITS).o \ > - of_platform.o prom_parse.o firmware.o > + of_platform.o prom_parse.o firmware.o \ > + hw_breakpoint_constraints.o > obj-y += ptrace/ > obj-$(CONFIG_PPC64) += setup_64.o \ > paca.o nvram_64.o note.o syscall_64.o > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c > index f6b24838ca3c..f4e8f21046f5 100644 > --- a/arch/powerpc/kernel/hw_breakpoint.c > +++ b/arch/powerpc/kernel/hw_breakpoint.c > @@ -494,161 +494,6 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs) > } > } > > -static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info) > -{ > - return ((info->address <= dar) && (dar - info->address < info->len)); > -} > - > -static bool ea_user_range_overlaps(unsigned long ea, int size, > - struct arch_hw_breakpoint *info) > -{ > - return ((ea < info->address + info->len) && > - (ea + size > info->address)); > -} > - > -static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info) > -{ > - unsigned long hw_start_addr, hw_end_addr; > - > - hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE); > - hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE); > - > - return ((hw_start_addr <= dar) && (hw_end_addr > dar)); > -} > - > -static bool ea_hw_range_overlaps(unsigned long ea, int size, > - struct arch_hw_breakpoint *info) > -{ > - unsigned long hw_start_addr, hw_end_addr; > - unsigned long align_size = HW_BREAKPOINT_SIZE; > - > - /* > - * On p10 predecessors, quadword is handle differently then > - * other instructions. > - */ > - if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16) > - align_size = HW_BREAKPOINT_SIZE_QUADWORD; > - > - hw_start_addr = ALIGN_DOWN(info->address, align_size); > - hw_end_addr = ALIGN(info->address + info->len, align_size); > - > - return ((ea < hw_end_addr) && (ea + size > hw_start_addr)); > -} > - > -/* > - * If hw has multiple DAWR registers, we also need to check all > - * dawrx constraint bits to confirm this is _really_ a valid event. > - * If type is UNKNOWN, but privilege level matches, consider it as > - * a positive match. > - */ > -static bool check_dawrx_constraints(struct pt_regs *regs, int type, > - struct arch_hw_breakpoint *info) > -{ > - if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ)) > - return false; > - > - /* > - * The Cache Management instructions other than dcbz never > - * cause a match. i.e. if type is CACHEOP, the instruction > - * is dcbz, and dcbz is treated as Store. > - */ > - if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE)) > - return false; > - > - if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL)) > - return false; > - > - if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER)) > - return false; > - > - return true; > -} > - > -/* > - * Return true if the event is valid wrt dawr configuration, > - * including extraneous exception. Otherwise return false. > - */ > -static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr, > - unsigned long ea, int type, int size, > - struct arch_hw_breakpoint *info) > -{ > - bool in_user_range = dar_in_user_range(regs->dar, info); > - bool dawrx_constraints; > - > - /* > - * 8xx supports only one breakpoint and thus we can > - * unconditionally return true. > - */ > - if (IS_ENABLED(CONFIG_PPC_8xx)) { > - if (!in_user_range) > - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ; > - return true; > - } > - > - if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) { > - if (cpu_has_feature(CPU_FTR_ARCH_31) && > - !dar_in_hw_range(regs->dar, info)) > - return false; > - > - return true; > - } > - > - dawrx_constraints = check_dawrx_constraints(regs, type, info); > - > - if (type == UNKNOWN) { > - if (cpu_has_feature(CPU_FTR_ARCH_31) && > - !dar_in_hw_range(regs->dar, info)) > - return false; > - > - return dawrx_constraints; > - } > - > - if (ea_user_range_overlaps(ea, size, info)) > - return dawrx_constraints; > - > - if (ea_hw_range_overlaps(ea, size, info)) { > - if (dawrx_constraints) { > - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ; > - return true; > - } > - } > - return false; > -} > - > -static int cache_op_size(void) > -{ > -#ifdef __powerpc64__ > - return ppc64_caches.l1d.block_size; > -#else > - return L1_CACHE_BYTES; > -#endif > -} > - > -static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr, > - int *type, int *size, unsigned long *ea) > -{ > - struct instruction_op op; > - > - if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip)) > - return; > - > - analyse_instr(&op, regs, *instr); > - *type = GETTYPE(op.type); > - *ea = op.ea; > -#ifdef __powerpc64__ > - if (!(regs->msr & MSR_64BIT)) > - *ea &= 0xffffffffUL; > -#endif > - > - *size = GETSIZE(op.type); > - if (*type == CACHEOP) { > - *size = cache_op_size(); > - *ea &= ~(*size - 1); > - } else if (*type == LOAD_VMX || *type == STORE_VMX) { > - *ea &= ~(*size - 1); > - } > -} > - > static bool is_larx_stcx_instr(int type) > { > return type == LARX || type == STCX; > @@ -732,7 +577,7 @@ int hw_breakpoint_handler(struct die_args *args) > rcu_read_lock(); > > if (!IS_ENABLED(CONFIG_PPC_8xx)) > - get_instr_detail(regs, &instr, &type, &size, &ea); > + wp_get_instr_detail(regs, &instr, &type, &size, &ea); > > for (i = 0; i < nr_wp_slots(); i++) { > bp[i] = __this_cpu_read(bp_per_reg[i]); > @@ -742,7 +587,7 @@ int hw_breakpoint_handler(struct die_args *args) > info[i] = counter_arch_bp(bp[i]); > info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ; > > - if (check_constraints(regs, instr, ea, type, size, info[i])) { > + if (wp_check_constraints(regs, instr, ea, type, size, info[i])) { > if (!IS_ENABLED(CONFIG_PPC_8xx) && > ppc_inst_equal(instr, ppc_inst(0))) { > handler_error(bp[i], info[i]); > diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c > new file mode 100644 > index 000000000000..867ee4aa026a > --- /dev/null > +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c > @@ -0,0 +1,162 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +#include <linux/kernel.h> > +#include <linux/uaccess.h> > +#include <linux/sched.h> > +#include <asm/hw_breakpoint.h> > +#include <asm/sstep.h> > +#include <asm/cache.h> > + > +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info) > +{ > + return ((info->address <= dar) && (dar - info->address < info->len)); > +} > + > +static bool ea_user_range_overlaps(unsigned long ea, int size, > + struct arch_hw_breakpoint *info) > +{ > + return ((ea < info->address + info->len) && > + (ea + size > info->address)); > +} > + > +static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info) > +{ > + unsigned long hw_start_addr, hw_end_addr; > + > + hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE); > + hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE); > + > + return ((hw_start_addr <= dar) && (hw_end_addr > dar)); > +} > + > +static bool ea_hw_range_overlaps(unsigned long ea, int size, > + struct arch_hw_breakpoint *info) > +{ > + unsigned long hw_start_addr, hw_end_addr; > + unsigned long align_size = HW_BREAKPOINT_SIZE; > + > + /* > + * On p10 predecessors, quadword is handle differently then > + * other instructions. > + */ > + if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16) > + align_size = HW_BREAKPOINT_SIZE_QUADWORD; > + > + hw_start_addr = ALIGN_DOWN(info->address, align_size); > + hw_end_addr = ALIGN(info->address + info->len, align_size); > + > + return ((ea < hw_end_addr) && (ea + size > hw_start_addr)); > +} > + > +/* > + * If hw has multiple DAWR registers, we also need to check all > + * dawrx constraint bits to confirm this is _really_ a valid event. > + * If type is UNKNOWN, but privilege level matches, consider it as > + * a positive match. > + */ > +static bool check_dawrx_constraints(struct pt_regs *regs, int type, > + struct arch_hw_breakpoint *info) > +{ > + if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ)) > + return false; > + > + /* > + * The Cache Management instructions other than dcbz never > + * cause a match. i.e. if type is CACHEOP, the instruction > + * is dcbz, and dcbz is treated as Store. > + */ > + if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE)) > + return false; > + > + if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL)) > + return false; > + > + if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER)) > + return false; > + > + return true; > +} > + > +/* > + * Return true if the event is valid wrt dawr configuration, > + * including extraneous exception. Otherwise return false. > + */ > +bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr, > + unsigned long ea, int type, int size, > + struct arch_hw_breakpoint *info) > +{ > + bool in_user_range = dar_in_user_range(regs->dar, info); > + bool dawrx_constraints; > + > + /* > + * 8xx supports only one breakpoint and thus we can > + * unconditionally return true. > + */ > + if (IS_ENABLED(CONFIG_PPC_8xx)) { > + if (!in_user_range) > + info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ; > + return true; > + } > + > + if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) { > + if (cpu_has_feature(CPU_FTR_ARCH_31) && > + !dar_in_hw_range(regs->dar, info)) > + return false; > + > + return true; > + } > + > + dawrx_constraints = check_dawrx_constraints(regs, type, info); > + > + if (type == UNKNOWN) { > + if (cpu_has_feature(CPU_FTR_ARCH_31) && > + !dar_in_hw_range(regs->dar, info)) > + return false; > + > + return dawrx_constraints; > + } > + > + if (ea_user_range_overlaps(ea, size, info)) > + return dawrx_constraints; > + > + if (ea_hw_range_overlaps(ea, size, info)) { > + if (dawrx_constraints) { > + info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ; > + return true; > + } > + } > + return false; > +} > + > +static int cache_op_size(void) > +{ > +#ifdef __powerpc64__ > + return ppc64_caches.l1d.block_size; > +#else > + return L1_CACHE_BYTES; > +#endif > +} > + > +void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr, > + int *type, int *size, unsigned long *ea) > +{ > + struct instruction_op op; > + > + if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip)) > + return; > + > + analyse_instr(&op, regs, *instr); > + *type = GETTYPE(op.type); > + *ea = op.ea; > +#ifdef __powerpc64__ > + if (!(regs->msr & MSR_64BIT)) > + *ea &= 0xffffffffUL; > +#endif > + > + *size = GETSIZE(op.type); > + if (*type == CACHEOP) { > + *size = cache_op_size(); > + *ea &= ~(*size - 1); > + } else if (*type == LOAD_VMX || *type == STORE_VMX) { > + *ea &= ~(*size - 1); > + } > +} >
| |