Re: [PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Hi Ravi,
Le 24/04/2020 à 05:32, Ravi Bangoria a écrit :
Hi Christophe,
@@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
*/
void arch_unregister_hw_breakpoint(struct perf_event *bp)
{
+ int i;
+
This declaration should be in the block using it.
/*
* If the breakpoint is unregistered between a
hw_breakpoint_handler()
* and the single_step_dabr_instruction(), then cleanup the
breakpoint
* restoration variables to prevent dangling pointers.
* FIXME, this should not be using bp->ctx at all! Sayeth peterz.
*/
- if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
- bp->ctx->task->thread.last_hit_ubp = NULL;
+ if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
Add declaration of 'int i' here.
How will that help? Keeping declaration at the start of function is also
common practice and I don't see any recommendation to move them inside
conditional block.
Reducing the scope of local variables increases readability, you don't
have to scroll all up to the top of the function to see the declaration
of the variable.
common practice ? Are you sure ? Just have a look at fs/io_uring.c or
many other files in the kernel to see how uncommon your practice is.
Christophe
Re: [PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Hi Christophe,
@@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
*/
void arch_unregister_hw_breakpoint(struct perf_event *bp)
{
+ int i;
+
This declaration should be in the block using it.
/*
* If the breakpoint is unregistered between a hw_breakpoint_handler()
* and the single_step_dabr_instruction(), then cleanup the breakpoint
* restoration variables to prevent dangling pointers.
* FIXME, this should not be using bp->ctx at all! Sayeth peterz.
*/
- if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
- bp->ctx->task->thread.last_hit_ubp = NULL;
+ if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
Add declaration of 'int i' here.
How will that help? Keeping declaration at the start of function is also
common practice and I don't see any recommendation to move them inside
conditional block.
Thanks,
Ravi
Re: [PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Le 14/04/2020 à 05:16, Ravi Bangoria a écrit :
Currently we assume that we have only one watchpoint supported by hw.
Get rid of that assumption and use dynamic loop instead. This should
make supporting more watchpoints very easy.
With more than one watchpoint, exception handler need to know which
DAWR caused the exception, and hw currently does not provide it. So
we need sw logic for the same. To figure out which DAWR caused the
exception, check all different combinations of user specified range,
dawr address range, actual access range and dawrx constrains. For ex,
if user specified range and actual access range overlaps but dawrx is
configured for readonly watchpoint and the instruction is store, this
DAWR must not have caused exception.
Signed-off-by: Ravi Bangoria
---
arch/powerpc/include/asm/processor.h | 2 +-
arch/powerpc/include/asm/sstep.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 400 +--
arch/powerpc/kernel/process.c| 3 -
4 files changed, 315 insertions(+), 92 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h
b/arch/powerpc/include/asm/processor.h
index 65b03162cd67..4bc4e4a99166 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -185,7 +185,7 @@ struct thread_struct {
* Helps identify source of single-step exception and subsequent
* hw-breakpoint enablement
*/
- struct perf_event *last_hit_ubp;
+ struct perf_event *last_hit_ubp[HBP_NUM_MAX];
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint
info */
unsigned long trap_nr;/* last trap # on this thread */
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 769f055509c9..38919b27a6fa 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -48,6 +48,8 @@ enum instruction_type {
#define INSTR_TYPE_MASK 0x1f
+#define OP_IS_LOAD(type) ((LOAD <= (type) && (type) <= LOAD_VSX) || (type) == LARX)
+#define OP_IS_STORE(type) ((STORE <= (type) && (type) <= STORE_VSX) ||
(type) == STCX)
#define OP_IS_LOAD_STORE(type)(LOAD <= (type) && (type) <= STCX)
/* Compute flags, ORed in with type */
diff --git a/arch/powerpc/kernel/hw_breakpoint.c
b/arch/powerpc/kernel/hw_breakpoint.c
index 02ffd14f4519..9b5812bca892 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -30,7 +30,7 @@
* Stores the breakpoints currently in use on each breakpoint address
* register for every cpu
*/
-static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
+static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
/*
* Returns total number of data or instruction breakpoints available.
@@ -42,6 +42,17 @@ int hw_breakpoint_slots(int type)
return 0; /* no instruction breakpoints available */
}
+static bool single_step_pending(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (current->thread.last_hit_ubp[i])
+ return true;
+ }
+ return false;
+}
+
/*
* Install a perf counter breakpoint.
*
@@ -54,16 +65,26 @@ int hw_breakpoint_slots(int type)
int arch_install_hw_breakpoint(struct perf_event *bp)
{
struct arch_hw_breakpoint *info = counter_arch_bp(bp);
- struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+ struct perf_event **slot;
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ slot = this_cpu_ptr(&bp_per_reg[i]);
+ if (!*slot) {
+ *slot = bp;
+ break;
+ }
+ }
- *slot = bp;
+ if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
+ return -EBUSY;
/*
* Do not install DABR values if the instruction must be single-stepped.
* If so, DABR will be populated in single_step_dabr_instruction().
*/
- if (current->thread.last_hit_ubp != bp)
- __set_breakpoint(0, info);
+ if (!single_step_pending())
+ __set_breakpoint(i, info);
return 0;
}
@@ -79,15 +100,22 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
*/
void arch_uninstall_hw_breakpoint(struct perf_event *bp)
{
- struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+ struct arch_hw_breakpoint null_brk = {0};
+ struct perf_event **slot;
+ int i;
- if (*slot != bp) {
- WARN_ONCE(1, "Can't find the breakpoint");
- return;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ slot = this_cpu_ptr(&bp_per_reg[i]);
+ if (*slot == bp) {
+ *slot = NULL;
+ break;
+ }
}
- *slot = NULL;
- hw_breakpoint_disable();
+ if (WARN
[PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Currently we assume that we have only one watchpoint supported by hw.
Get rid of that assumption and use dynamic loop instead. This should
make supporting more watchpoints very easy.
With more than one watchpoint, exception handler need to know which
DAWR caused the exception, and hw currently does not provide it. So
we need sw logic for the same. To figure out which DAWR caused the
exception, check all different combinations of user specified range,
dawr address range, actual access range and dawrx constrains. For ex,
if user specified range and actual access range overlaps but dawrx is
configured for readonly watchpoint and the instruction is store, this
DAWR must not have caused exception.
Signed-off-by: Ravi Bangoria
---
arch/powerpc/include/asm/processor.h | 2 +-
arch/powerpc/include/asm/sstep.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 400 +--
arch/powerpc/kernel/process.c| 3 -
4 files changed, 315 insertions(+), 92 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h
b/arch/powerpc/include/asm/processor.h
index 65b03162cd67..4bc4e4a99166 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -185,7 +185,7 @@ struct thread_struct {
* Helps identify source of single-step exception and subsequent
* hw-breakpoint enablement
*/
- struct perf_event *last_hit_ubp;
+ struct perf_event *last_hit_ubp[HBP_NUM_MAX];
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint
info */
unsigned long trap_nr;/* last trap # on this thread */
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 769f055509c9..38919b27a6fa 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -48,6 +48,8 @@ enum instruction_type {
#define INSTR_TYPE_MASK0x1f
+#define OP_IS_LOAD(type) ((LOAD <= (type) && (type) <= LOAD_VSX) ||
(type) == LARX)
+#define OP_IS_STORE(type) ((STORE <= (type) && (type) <= STORE_VSX) ||
(type) == STCX)
#define OP_IS_LOAD_STORE(type) (LOAD <= (type) && (type) <= STCX)
/* Compute flags, ORed in with type */
diff --git a/arch/powerpc/kernel/hw_breakpoint.c
b/arch/powerpc/kernel/hw_breakpoint.c
index 02ffd14f4519..9b5812bca892 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -30,7 +30,7 @@
* Stores the breakpoints currently in use on each breakpoint address
* register for every cpu
*/
-static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
+static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
/*
* Returns total number of data or instruction breakpoints available.
@@ -42,6 +42,17 @@ int hw_breakpoint_slots(int type)
return 0; /* no instruction breakpoints available */
}
+static bool single_step_pending(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (current->thread.last_hit_ubp[i])
+ return true;
+ }
+ return false;
+}
+
/*
* Install a perf counter breakpoint.
*
@@ -54,16 +65,26 @@ int hw_breakpoint_slots(int type)
int arch_install_hw_breakpoint(struct perf_event *bp)
{
struct arch_hw_breakpoint *info = counter_arch_bp(bp);
- struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+ struct perf_event **slot;
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ slot = this_cpu_ptr(&bp_per_reg[i]);
+ if (!*slot) {
+ *slot = bp;
+ break;
+ }
+ }
- *slot = bp;
+ if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
+ return -EBUSY;
/*
* Do not install DABR values if the instruction must be single-stepped.
* If so, DABR will be populated in single_step_dabr_instruction().
*/
- if (current->thread.last_hit_ubp != bp)
- __set_breakpoint(0, info);
+ if (!single_step_pending())
+ __set_breakpoint(i, info);
return 0;
}
@@ -79,15 +100,22 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
*/
void arch_uninstall_hw_breakpoint(struct perf_event *bp)
{
- struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+ struct arch_hw_breakpoint null_brk = {0};
+ struct perf_event **slot;
+ int i;
- if (*slot != bp) {
- WARN_ONCE(1, "Can't find the breakpoint");
- return;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ slot = this_cpu_ptr(&bp_per_reg[i]);
+ if (*slot == bp) {
+ *slot = NULL;
+ break;
+ }
}
- *slot = NULL;
- hw_breakpoint_disable();
+ if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
