Re: [PATCH v5 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
Hi Ravi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.9-rc2 next-20200826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Ravi-Bangoria/powerpc-watchpoint-Bug-fixes-plus-new-feature-flag/20200825-123831
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r024-20200826 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
arch/powerpc/kernel/hw_breakpoint_constraints.c: In function
'ea_hw_range_overlaps':
>> arch/powerpc/kernel/hw_breakpoint_constraints.c:42:16: error:
>> 'HW_BREAKPOINT_SIZE_QUADWORD' undeclared (first use in this function); did
>> you mean 'HW_BREAKPOINT_SIZE'?
42 | align_size = HW_BREAKPOINT_SIZE_QUADWORD;
|^~~
|HW_BREAKPOINT_SIZE
arch/powerpc/kernel/hw_breakpoint_constraints.c:42:16: note: each undeclared
identifier is reported only once for each function it appears in
#
https://github.com/0day-ci/linux/commit/4899293e6a722214368fd6b5df8ecda43600ccfb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Ravi-Bangoria/powerpc-watchpoint-Bug-fixes-plus-new-feature-flag/20200825-123831
git checkout 4899293e6a722214368fd6b5df8ecda43600ccfb
vim +42 arch/powerpc/kernel/hw_breakpoint_constraints.c
30
31 static bool ea_hw_range_overlaps(unsigned long ea, int size,
32 struct arch_hw_breakpoint *info)
33 {
34 unsigned long hw_start_addr, hw_end_addr;
35 unsigned long align_size = HW_BREAKPOINT_SIZE;
36
37 /*
38 * On p10 predecessors, quadword is handle differently then
39 * other instructions.
40 */
41 if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> 42 align_size = HW_BREAKPOINT_SIZE_QUADWORD;
43
44 hw_start_addr = ALIGN_DOWN(info->address, align_size);
45 hw_end_addr = ALIGN(info->address + info->len, align_size);
46
47 return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
48 }
49
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v5 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
Le 25/08/2020 à 13:08, Ravi Bangoria a écrit :
Hi Christophe,
+static int cache_op_size(void)
+{
+#ifdef __powerpc64__
+ return ppc64_caches.l1d.block_size;
+#else
+ return L1_CACHE_BYTES;
+#endif
+}
You've got l1_dcache_bytes() in arch/powerpc/include/asm/cache.h to do
that.
+
+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(, regs, *instr);
+ *type = GETTYPE(op.type);
+ *ea = op.ea;
+#ifdef __powerpc64__
+ if (!(regs->msr & MSR_64BIT))
+ *ea &= 0xUL;
+#endif
This #ifdef is unneeded, it should build fine on a 32 bits too.
This patch is just a code movement from one file to another.
I don't really change the logic. Would you mind if I do a
separate patch for these changes (not a part of this series)?
Sure, do it in a separate patch.
Christophe
Re: [PATCH v5 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
Hi Christophe,
+static int cache_op_size(void)
+{
+#ifdef __powerpc64__
+ return ppc64_caches.l1d.block_size;
+#else
+ return L1_CACHE_BYTES;
+#endif
+}
You've got l1_dcache_bytes() in arch/powerpc/include/asm/cache.h to do that.
+
+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(, regs, *instr);
+ *type = GETTYPE(op.type);
+ *ea = op.ea;
+#ifdef __powerpc64__
+ if (!(regs->msr & MSR_64BIT))
+ *ea &= 0xUL;
+#endif
This #ifdef is unneeded, it should build fine on a 32 bits too.
This patch is just a code movement from one file to another.
I don't really change the logic. Would you mind if I do a
separate patch for these changes (not a part of this series)?
Thanks for review,
Ravi
Re: [PATCH v5 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
Le 25/08/2020 à 06:36, Ravi Bangoria a écrit :
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
---
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/kernel/hw_breakpoint_constraints.c
b/arch/powerpc/kernel/hw_breakpoint_constraints.c
new file mode 100644
index ..867ee4aa026a
--- /dev/null
+++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
@@ -0,0 +1,162 @@
[...]
+
+static int cache_op_size(void)
+{
+#ifdef __powerpc64__
+ return ppc64_caches.l1d.block_size;
+#else
+ return L1_CACHE_BYTES;
+#endif
+}
You've got l1_dcache_bytes() in arch/powerpc/include/asm/cache.h to do that.
+
+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(, regs, *instr);
+ *type = GETTYPE(op.type);
+ *ea = op.ea;
+#ifdef __powerpc64__
+ if (!(regs->msr & MSR_64BIT))
+ *ea &= 0xUL;
+#endif
This #ifdef is unneeded, it should build fine on a 32 bits too.
+
+ *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);
+ }
+}
Christophe
[PATCH v5 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
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
---
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 da38e05e04d9..2eca3dd54b55 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
+#include
#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
#include
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 &
