So far, powerpc Book3S code has been written with an assumption of only
one watchpoint. But future power architecture is introducing second
watchpoint register (DAWR). Even though this patchset does not enable
2nd DAWR, it make the infrastructure ready so that enabling 2nd DAWR
should just be a matter of changing count.
Existing functionality works fine with the patchset. I've tested it with
perf, ptrace(gdb), xmon. All hw-breakpoint selftests are passing as well.
And I've build tested for 8xx and 'AMCC 44x, 46x or 47x'.
Note: kvm or PowerVM guest is not enabled yet.
v1:
https://lore.kernel.org/linuxppc-dev/[email protected]
v1->v2:
- No major functionality changes. Changed few minor things as suggested
by Christophe
- Fixed build failure reported by 'kbuild test robot <[email protected]>'
for patch #9.
- Rebased to powerpc/next plus Christophe's ptrace series[1][2]
[1]: https://lore.kernel.org/linuxppc-dev/[email protected]
[2]: https://lore.kernel.org/linuxppc-dev/5558d8c22ff0ed03cb5392798564dd203bd68501.1584787012.git.christophe.leroy@c-s.fr
Ravi Bangoria (16):
powerpc/watchpoint: Rename current DAWR macros
powerpc/watchpoint: Add SPRN macros for second DAWR
powerpc/watchpoint: Introduce function to get nr watchpoints
dynamically
powerpc/watchpoint/ptrace: Return actual num of available watchpoints
powerpc/watchpoint: Provide DAWR number to set_dawr
powerpc/watchpoint: Provide DAWR number to __set_breakpoint
powerpc/watchpoint: Get watchpoint count dynamically while disabling
them
powerpc/watchpoint: Disable all available watchpoints when
!dawr_force_enable
powerpc/watchpoint: Convert thread_struct->hw_brk to an array
powerpc/watchpoint: Use loop for thread_struct->ptrace_bps
powerpc/watchpoint: Introduce is_ptrace_bp() function
powerpc/watchpoint: Use builtin ALIGN*() macros
powerpc/watchpoint: Prepare handler to handle more than one
watcnhpoint
powerpc/watchpoint: Don't allow concurrent perf and ptrace events
powerpc/watchpoint/xmon: Don't allow breakpoint overwriting
powerpc/watchpoint/xmon: Support 2nd dawr
arch/powerpc/include/asm/cputable.h | 6 +-
arch/powerpc/include/asm/debug.h | 2 +-
arch/powerpc/include/asm/hw_breakpoint.h | 31 +-
arch/powerpc/include/asm/processor.h | 6 +-
arch/powerpc/include/asm/reg.h | 6 +-
arch/powerpc/include/asm/sstep.h | 2 +
arch/powerpc/kernel/dawr.c | 23 +-
arch/powerpc/kernel/hw_breakpoint.c | 636 ++++++++++++++++++----
arch/powerpc/kernel/process.c | 82 +--
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 72 ++-
arch/powerpc/kernel/ptrace/ptrace32.c | 4 +-
arch/powerpc/kernel/signal.c | 9 +-
arch/powerpc/kvm/book3s_hv.c | 12 +-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 18 +-
arch/powerpc/xmon/xmon.c | 99 +++-
kernel/events/hw_breakpoint.c | 16 +
16 files changed, 806 insertions(+), 218 deletions(-)
--
2.21.1
Introduce is_ptrace_bp() function and move the check inside the
function. We will utilize it more in later set of patches.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/kernel/hw_breakpoint.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 6daa95b8bb88..8d3f7d87d790 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -90,6 +90,11 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
hw_breakpoint_disable();
}
+static bool is_ptrace_bp(struct perf_event *bp)
+{
+ return bp->overflow_handler == ptrace_triggered;
+}
+
/*
* Perform cleanup of arch-specific counters during unregistration
* of the perf-event
@@ -324,7 +329,7 @@ int hw_breakpoint_handler(struct die_args *args)
* one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
* generated in do_dabr().
*/
- if (bp->overflow_handler == ptrace_triggered) {
+ if (is_ptrace_bp(bp)) {
perf_bp_event(bp, regs);
rc = NOTIFY_DONE;
goto out;
--
2.21.1
Currently we calculate hw aligned start and end addresses manually.
Replace them with builtin ALIGN_DOWN() and ALIGN() macros.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/include/asm/hw_breakpoint.h | 5 +++--
arch/powerpc/kernel/hw_breakpoint.c | 6 +++---
arch/powerpc/kernel/process.c | 4 ++--
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 +-
4 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index fae33c729ba9..abc4603c0efe 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 8d3f7d87d790..71274fbbac38 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -145,7 +145,7 @@ 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.
@@ -156,8 +156,8 @@ 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;
+ start_addr = ALIGN_DOWN(hw->address, HW_BREAKPOINT_SIZE);
+ end_addr = ALIGN(hw->address + hw->len, HW_BREAKPOINT_SIZE) - 1;
hw_len = end_addr - start_addr + 1;
if (dawr_enabled()) {
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 73c0800f0bcf..cbd6e9b79401 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -794,8 +794,8 @@ 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) - 1;
if (start_addr == 0)
lctrl2 |= LCTRL2_LW0LA_F;
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)
--
2.21.1
With Book3s DAWR, ptrace and perf watchpoints on powerpc behaves
differently. Ptrace watchpoint works in one-shot mode and generates
signal before executing instruction. It's ptrace user's job to
single-step the instruction and re-enable the watchpoint. OTOH, in
case of perf watchpoint, kernel emulates/single-steps the instruction
and then generates event. If perf and ptrace creates two events with
same or overlapping address ranges, it's ambiguous to decide who
should single-step the instruction. Because of this issue, don't
allow perf and ptrace watchpoint at the same time if their address
range overlaps.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/include/asm/hw_breakpoint.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 222 +++++++++++++++++++++++
kernel/events/hw_breakpoint.c | 16 ++
3 files changed, 240 insertions(+)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index abc4603c0efe..9d3bd1169591 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -70,6 +70,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
unsigned long val, void *data);
int arch_install_hw_breakpoint(struct perf_event *bp);
void arch_uninstall_hw_breakpoint(struct perf_event *bp);
+int arch_reserve_bp_slot(struct perf_event *bp);
+void arch_release_bp_slot(struct perf_event *bp);
void arch_unregister_hw_breakpoint(struct perf_event *bp);
void hw_breakpoint_pmu_read(struct perf_event *bp);
extern void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 07a6cdea84ed..f813acb0d9f0 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -123,6 +123,228 @@ static bool is_ptrace_bp(struct perf_event *bp)
return bp->overflow_handler == ptrace_triggered;
}
+struct breakpoint {
+ struct list_head list;
+ struct perf_event *bp;
+ bool ptrace_bp;
+};
+
+static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
+static LIST_HEAD(task_bps);
+
+static struct breakpoint *alloc_breakpoint(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return ERR_PTR(-ENOMEM);
+ tmp->bp = bp;
+ tmp->ptrace_bp = is_ptrace_bp(bp);
+ return tmp;
+}
+
+static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
+{
+ __u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
+
+ bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
+ bp1_eaddr = ALIGN(bp1->attr.bp_addr + bp1->attr.bp_len, HW_BREAKPOINT_SIZE) - 1;
+ bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
+ bp2_eaddr = ALIGN(bp2->attr.bp_addr + bp2->attr.bp_len, HW_BREAKPOINT_SIZE) - 1;
+
+ return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);
+}
+
+static bool alternate_infra_bp(struct breakpoint *b, struct perf_event *bp)
+{
+ return is_ptrace_bp(bp) ? !b->ptrace_bp : b->ptrace_bp;
+}
+
+static bool can_co_exist(struct breakpoint *b, struct perf_event *bp)
+{
+ return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
+}
+
+static int task_bps_add(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ tmp = alloc_breakpoint(bp);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ list_add(&tmp->list, &task_bps);
+ return 0;
+}
+
+static void task_bps_remove(struct perf_event *bp)
+{
+ struct list_head *pos, *q;
+ struct breakpoint *tmp;
+
+ list_for_each_safe(pos, q, &task_bps) {
+ tmp = list_entry(pos, struct breakpoint, list);
+
+ if (tmp->bp == bp) {
+ list_del(&tmp->list);
+ kfree(tmp);
+ break;
+ }
+ }
+}
+
+/*
+ * If any task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool all_task_bps_check(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ list_for_each_entry(tmp, &task_bps, list) {
+ if (!can_co_exist(tmp, bp))
+ return true;
+ }
+ return false;
+}
+
+/*
+ * If same task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool same_task_bps_check(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ list_for_each_entry(tmp, &task_bps, list) {
+ if (tmp->bp->hw.target == bp->hw.target &&
+ !can_co_exist(tmp, bp))
+ return true;
+ }
+ return false;
+}
+
+static int cpu_bps_add(struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ struct breakpoint *tmp;
+ int i = 0;
+
+ tmp = alloc_breakpoint(bp);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!cpu_bp[i]) {
+ cpu_bp[i] = tmp;
+ break;
+ }
+ }
+ return 0;
+}
+
+static void cpu_bps_remove(struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ int i = 0;
+
+ cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!cpu_bp[i])
+ continue;
+
+ if (cpu_bp[i]->bp == bp) {
+ kfree(cpu_bp[i]);
+ cpu_bp[i] = NULL;
+ break;
+ }
+ }
+}
+
+static bool cpu_bps_check(int cpu, struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ int i;
+
+ cpu_bp = per_cpu_ptr(cpu_bps, cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp))
+ return true;
+ }
+ return false;
+}
+
+static bool all_cpu_bps_check(struct perf_event *bp)
+{
+ int cpu;
+
+ for_each_online_cpu(cpu) {
+ if (cpu_bps_check(cpu, bp))
+ return true;
+ }
+ return false;
+}
+
+/*
+ * We don't use any locks to serialize accesses to cpu_bps or task_bps
+ * because are already inside nr_bp_mutex.
+ */
+int arch_reserve_bp_slot(struct perf_event *bp)
+{
+ int ret;
+
+ /* ptrace breakpoint */
+ if (is_ptrace_bp(bp)) {
+ if (all_cpu_bps_check(bp))
+ return -ENOSPC;
+
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ return task_bps_add(bp);
+ }
+
+ /* perf breakpoint */
+ if (is_kernel_addr(bp->attr.bp_addr))
+ return 0;
+
+ if (bp->hw.target && bp->cpu == -1) {
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ return task_bps_add(bp);
+ } else if (!bp->hw.target && bp->cpu != -1) {
+ if (all_task_bps_check(bp))
+ return -ENOSPC;
+
+ return cpu_bps_add(bp);
+ }
+
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ ret = cpu_bps_add(bp);
+ if (ret)
+ return ret;
+ ret = task_bps_add(bp);
+ if (ret)
+ cpu_bps_remove(bp);
+
+ return ret;
+}
+
+void arch_release_bp_slot(struct perf_event *bp)
+{
+ if (!is_kernel_addr(bp->attr.bp_addr)) {
+ if (bp->hw.target)
+ task_bps_remove(bp);
+ if (bp->cpu != -1)
+ cpu_bps_remove(bp);
+ }
+}
+
/*
* Perform cleanup of arch-specific counters during unregistration
* of the perf-event
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3cc8416ec844..b48d7039a015 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -213,6 +213,15 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
list_del(&bp->hw.bp_list);
}
+__weak int arch_reserve_bp_slot(struct perf_event *bp)
+{
+ return 0;
+}
+
+__weak void arch_release_bp_slot(struct perf_event *bp)
+{
+}
+
/*
* Function to perform processor-specific cleanup during unregistration
*/
@@ -270,6 +279,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
struct bp_busy_slots slots = {0};
enum bp_type_idx type;
int weight;
+ int ret;
/* We couldn't initialize breakpoint constraints on boot */
if (!constraints_initialized)
@@ -294,6 +304,10 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
if (slots.pinned + (!!slots.flexible) > nr_slots[type])
return -ENOSPC;
+ ret = arch_reserve_bp_slot(bp);
+ if (ret)
+ return ret;
+
toggle_bp_slot(bp, true, type, weight);
return 0;
@@ -317,6 +331,8 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
enum bp_type_idx type;
int weight;
+ arch_release_bp_slot(bp);
+
type = find_slot_idx(bp_type);
weight = hw_breakpoint_weight(bp);
toggle_bp_slot(bp, false, type, weight);
--
2.21.1
Instead of disabling only first watchpoint, disable all available
watchpoints while clearing dawr_force_enable.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/kernel/dawr.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
index 311e51ee09f4..5c882f07ac7d 100644
--- a/arch/powerpc/kernel/dawr.c
+++ b/arch/powerpc/kernel/dawr.c
@@ -50,9 +50,13 @@ int set_dawr(struct arch_hw_breakpoint *brk, int nr)
return 0;
}
-static void set_dawr_cb(void *info)
+static void disable_dawrs(void *info)
{
- set_dawr(info, 0);
+ struct arch_hw_breakpoint null_brk = {0};
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++)
+ set_dawr(&null_brk, i);
}
static ssize_t dawr_write_file_bool(struct file *file,
@@ -74,7 +78,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
/* If we are clearing, make sure all CPUs have the DAWR cleared */
if (!dawr_force_enable)
- smp_call_function(set_dawr_cb, &null_brk, 0);
+ smp_call_function(disable_dawrs, NULL, 0);
return rc;
}
--
2.21.1
Add support for 2nd DAWR in xmon. With this, we can have two
simultaneous breakpoints from xmon.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/xmon/xmon.c | 101 ++++++++++++++++++++++++++-------------
1 file changed, 69 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a1e36501af25..4f2c8d4cbc38 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -111,7 +111,7 @@ struct bpt {
#define NBPTS 256
static struct bpt bpts[NBPTS];
-static struct bpt dabr;
+static struct bpt dabr[HBP_NUM_MAX];
static struct bpt *iabr;
static unsigned bpinstr = 0x7fe00008; /* trap */
@@ -787,10 +787,17 @@ static int xmon_sstep(struct pt_regs *regs)
static int xmon_break_match(struct pt_regs *regs)
{
+ int i;
+
if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) != (MSR_IR|MSR_64BIT))
return 0;
- if (dabr.enabled == 0)
- return 0;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (dabr[i].enabled)
+ goto found;
+ }
+ return 0;
+
+found:
xmon_core(regs, 0);
return 1;
}
@@ -929,13 +936,16 @@ static void insert_bpts(void)
static void insert_cpu_bpts(void)
{
+ int i;
struct arch_hw_breakpoint brk;
- if (dabr.enabled) {
- brk.address = dabr.address;
- brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
- brk.len = DABR_MAX_LEN;
- __set_breakpoint(&brk, 0);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (dabr[i].enabled) {
+ brk.address = dabr[i].address;
+ brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
+ brk.len = 8;
+ __set_breakpoint(&brk, i);
+ }
}
if (iabr)
@@ -1349,6 +1359,35 @@ static long check_bp_loc(unsigned long addr)
return 1;
}
+static int find_free_data_bpt(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!dabr[i].enabled)
+ return i;
+ }
+ printf("Couldn't find free breakpoint register\n");
+ return -1;
+}
+
+static void print_data_bpts(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!dabr[i].enabled)
+ continue;
+
+ printf(" data "REG" [", dabr[i].address);
+ if (dabr[i].enabled & 1)
+ printf("r");
+ if (dabr[i].enabled & 2)
+ printf("w");
+ printf("]\n");
+ }
+}
+
static char *breakpoint_help_string =
"Breakpoint command usage:\n"
"b show breakpoints\n"
@@ -1382,10 +1421,9 @@ bpt_cmds(void)
printf("Hardware data breakpoint not supported on this cpu\n");
break;
}
- if (dabr.enabled) {
- printf("Couldn't find free breakpoint register\n");
+ i = find_free_data_bpt();
+ if (i < 0)
break;
- }
mode = 7;
cmd = inchar();
if (cmd == 'r')
@@ -1394,15 +1432,15 @@ bpt_cmds(void)
mode = 6;
else
termch = cmd;
- dabr.address = 0;
- dabr.enabled = 0;
- if (scanhex(&dabr.address)) {
- if (!is_kernel_addr(dabr.address)) {
+ dabr[i].address = 0;
+ dabr[i].enabled = 0;
+ if (scanhex(&dabr[i].address)) {
+ if (!is_kernel_addr(dabr[i].address)) {
printf(badaddr);
break;
}
- dabr.address &= ~HW_BRK_TYPE_DABR;
- dabr.enabled = mode | BP_DABR;
+ dabr[i].address &= ~HW_BRK_TYPE_DABR;
+ dabr[i].enabled = mode | BP_DABR;
}
force_enable_xmon();
@@ -1441,7 +1479,9 @@ bpt_cmds(void)
for (i = 0; i < NBPTS; ++i)
bpts[i].enabled = 0;
iabr = NULL;
- dabr.enabled = 0;
+ for (i = 0; i < nr_wp_slots(); i++)
+ dabr[i].enabled = 0;
+
printf("All breakpoints cleared\n");
break;
}
@@ -1475,14 +1515,7 @@ bpt_cmds(void)
if (xmon_is_ro || !scanhex(&a)) {
/* print all breakpoints */
printf(" type address\n");
- if (dabr.enabled) {
- printf(" data "REG" [", dabr.address);
- if (dabr.enabled & 1)
- printf("r");
- if (dabr.enabled & 2)
- printf("w");
- printf("]\n");
- }
+ print_data_bpts();
for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
if (!bp->enabled)
continue;
@@ -1942,8 +1975,13 @@ static void dump_207_sprs(void)
printf("hfscr = %.16lx dhdes = %.16lx rpr = %.16lx\n",
mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
- printf("dawr = %.16lx dawrx = %.16lx ciabr = %.16lx\n",
- mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
+ printf("dawr0 = %.16lx dawrx0 = %.16lx\n",
+ mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0));
+ if (nr_wp_slots() > 1) {
+ printf("dawr1 = %.16lx dawrx1 = %.16lx\n",
+ mfspr(SPRN_DAWR1), mfspr(SPRN_DAWRX1));
+ }
+ printf("ciabr = %.16lx\n", mfspr(SPRN_CIABR));
#endif
}
@@ -3868,10 +3906,9 @@ static void clear_all_bpt(void)
bpts[i].enabled = 0;
/* Clear any data or iabr breakpoints */
- if (iabr || dabr.enabled) {
- iabr = NULL;
- dabr.enabled = 0;
- }
+ iabr = NULL;
+ for (i = 0; i < nr_wp_slots(); i++)
+ dabr[i].enabled = 0;
}
#ifdef CONFIG_DEBUG_FS
--
2.21.1
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 <[email protected]>
---
arch/powerpc/include/asm/processor.h | 2 +-
arch/powerpc/include/asm/sstep.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 396 +++++++++++++++++++++------
arch/powerpc/kernel/process.c | 3 -
4 files changed, 313 insertions(+), 90 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 71274fbbac38..07a6cdea84ed 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(info, 0);
+ if (!single_step_pending())
+ __set_breakpoint(info, i);
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"))
+ return;
+
+ __set_breakpoint(&null_brk, i);
}
static bool is_ptrace_bp(struct perf_event *bp)
@@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
*/
void arch_unregister_hw_breakpoint(struct perf_event *bp)
{
+ int i;
+
/*
* 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)) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (bp->ctx->task->thread.last_hit_ubp[i] == bp)
+ bp->ctx->task->thread.last_hit_ubp[i] = NULL;
+ }
+ }
}
/*
@@ -220,90 +254,215 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
{
struct arch_hw_breakpoint *info;
+ int i;
- if (likely(!tsk->thread.last_hit_ubp))
- return;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (unlikely(tsk->thread.last_hit_ubp[i]))
+ goto reset;
+ }
+ return;
- info = counter_arch_bp(tsk->thread.last_hit_ubp);
+reset:
regs->msr &= ~MSR_SE;
- __set_breakpoint(info, 0);
- tsk->thread.last_hit_ubp = NULL;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ info = counter_arch_bp(__this_cpu_read(bp_per_reg[i]));
+ __set_breakpoint(info, i);
+ tsk->thread.last_hit_ubp[i] = NULL;
+ }
}
-static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
+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
-dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
+static bool dar_user_range_overlaps(unsigned long dar, int size,
+ struct arch_hw_breakpoint *info)
{
return ((dar <= info->address + info->len - 1) &&
(dar + size - 1 >= 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) - 1;
+
+ return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
+}
+
+static bool dar_hw_range_overlaps(unsigned long dar, int size,
+ 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) - 1;
+
+ return ((dar <= hw_end_addr) && (dar + size - 1 >= hw_start_addr));
+}
+
/*
- * Handle debug exception notifications.
+ * If hw has multiple DAWR registers, we also need to check all
+ * dawrx constraint bits to confirm this is _really_ a valid event.
*/
-static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
- struct arch_hw_breakpoint *info)
+static bool check_dawrx_constraints(struct pt_regs *regs, int type,
+ struct arch_hw_breakpoint *info)
{
- unsigned int instr = 0;
- int ret, type, size;
- struct instruction_op op;
- unsigned long addr = info->address;
+ if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
+ return false;
- if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
- goto fail;
+ if (OP_IS_STORE(type) && !(info->type & HW_BRK_TYPE_WRITE))
+ return false;
- ret = analyse_instr(&op, regs, instr);
- type = GETTYPE(op.type);
- size = GETSIZE(op.type);
+ if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
+ return false;
- if (!ret && (type == LARX || type == STCX)) {
- printk_ratelimited("Breakpoint hit on instruction that can't be emulated."
- " Breakpoint at 0x%lx will be disabled.\n", addr);
- goto disable;
- }
+ if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
+ return false;
+
+ return true;
+}
+
+/*
+ * Returns true if the event is valid wrt dawr configuration,
+ * including extraneous exception. Otherwise return false.
+ */
+static bool check_constraints(struct pt_regs *regs, unsigned int instr,
+ int type, int size,
+ struct arch_hw_breakpoint *info)
+{
+ bool in_user_range = dar_in_user_range(regs->dar, info);
+ bool dawrx_constraints;
/*
- * If it's extraneous event, we still need to emulate/single-
- * step the instruction, but we don't generate an event.
+ * 8xx supports only one breakpoint and thus we can
+ * unconditionally return true.
*/
- if (size && !dar_range_overlaps(regs->dar, size, info))
- info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ if (IS_ENABLED(CONFIG_PPC_8xx)) {
+ if (!in_user_range)
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
- /* Do not emulate user-space instructions, instead single-step them */
- if (user_mode(regs)) {
- current->thread.last_hit_ubp = bp;
- regs->msr |= MSR_SE;
+ if (unlikely(instr == -1)) {
+ if (in_user_range)
+ return true;
+
+ if (dar_in_hw_range(regs->dar, info)) {
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
return false;
}
- if (!emulate_step(regs, instr))
- goto fail;
+ dawrx_constraints = check_dawrx_constraints(regs, type, info);
- return true;
+ if (dar_user_range_overlaps(regs->dar, size, info))
+ return dawrx_constraints;
+
+ if (dar_hw_range_overlaps(regs->dar, size, info)) {
+ if (dawrx_constraints) {
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
+ }
+ return false;
+}
+
+static int get_instr_detail(struct pt_regs *regs, int *type, int *size,
+ bool *larx_stcx)
+{
+ unsigned int instr = 0;
+ struct instruction_op op;
+
+ if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
+ return -1;
+
+ analyse_instr(&op, regs, instr);
-fail:
/*
- * We've failed in reliably handling the hw-breakpoint. Unregister
- * it and throw a warning message to let the user know about it.
+ * Set size = 8 if analyse_instr() fails. If it's a userspace
+ * watchpoint(valid or extraneous), we can notify user about it.
+ * If it's a kernel watchpoint, instruction emulation will fail
+ * in stepping_handler() and watchpoint will be disabled.
*/
- WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
- "0x%lx will be disabled.", addr);
+ *type = GETTYPE(op.type);
+ *size = !(*type == UNKNOWN) ? GETSIZE(op.type) : 8;
+ *larx_stcx = (*type == LARX || *type == STCX);
-disable:
+ return instr;
+}
+
+/*
+ * We've failed in reliably handling the hw-breakpoint. Unregister
+ * it and throw a warning message to let the user know about it.
+ */
+static void handler_error(struct perf_event *bp, struct arch_hw_breakpoint *info)
+{
+ WARN(1, "Unable to handle hardware breakpoint."
+ "Breakpoint at 0x%lx will be disabled.",
+ info->address);
+ perf_event_disable_inatomic(bp);
+}
+
+static void larx_stcx_err(struct perf_event *bp, struct arch_hw_breakpoint *info)
+{
+ printk_ratelimited("Breakpoint hit on instruction that can't "
+ "be emulated. Breakpoint at 0x%lx will be "
+ "disabled.\n", info->address);
perf_event_disable_inatomic(bp);
- return false;
+}
+
+static bool stepping_handler(struct pt_regs *regs, struct perf_event **bp,
+ struct arch_hw_breakpoint **info, int *hit,
+ unsigned int instr)
+{
+ int i;
+ int stepped;
+
+ /* Do not emulate user-space instructions, instead single-step them */
+ if (user_mode(regs)) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ current->thread.last_hit_ubp[i] = bp[i];
+ info[i] = NULL;
+ }
+ regs->msr |= MSR_SE;
+ return false;
+ }
+
+ stepped = emulate_step(regs, instr);
+ if (!stepped) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ handler_error(bp[i], info[i]);
+ info[i] = NULL;
+ }
+ return false;
+ }
+ return true;
}
int hw_breakpoint_handler(struct die_args *args)
{
+ bool err = false;
int rc = NOTIFY_STOP;
- struct perf_event *bp;
+ struct perf_event *bp[HBP_NUM_MAX] = {0};
struct pt_regs *regs = args->regs;
- struct arch_hw_breakpoint *info;
+ struct arch_hw_breakpoint *info[HBP_NUM_MAX] = {0};
+ int i;
+ int hit[HBP_NUM_MAX] = {0};
+ int nr_hit = 0;
+ bool ptrace_bp = false;
+ unsigned int instr = 0;
+ int type = 0;
+ int size = 0;
+ bool larx_stcx = false;
/* Disable breakpoints during exception handling */
hw_breakpoint_disable();
@@ -316,12 +475,39 @@ int hw_breakpoint_handler(struct die_args *args)
*/
rcu_read_lock();
- bp = __this_cpu_read(bp_per_reg);
- if (!bp) {
+ if (!IS_ENABLED(CONFIG_PPC_8xx))
+ instr = get_instr_detail(regs, &type, &size, &larx_stcx);
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ bp[i] = __this_cpu_read(bp_per_reg[i]);
+ if (!bp[i])
+ continue;
+
+ info[i] = counter_arch_bp(bp[i]);
+ info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
+
+ if (check_constraints(regs, instr, type, size, info[i])) {
+ if (!IS_ENABLED(CONFIG_PPC_8xx) && instr == -1) {
+ handler_error(bp[i], info[i]);
+ info[i] = NULL;
+ err = 1;
+ continue;
+ }
+
+ if (is_ptrace_bp(bp[i]))
+ ptrace_bp = true;
+ hit[i] = 1;
+ nr_hit++;
+ }
+ }
+
+ if (err)
+ goto reset;
+
+ if (!nr_hit) {
rc = NOTIFY_DONE;
goto out;
}
- info = counter_arch_bp(bp);
/*
* Return early after invoking user-callback function without restoring
@@ -329,29 +515,50 @@ int hw_breakpoint_handler(struct die_args *args)
* one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
* generated in do_dabr().
*/
- if (is_ptrace_bp(bp)) {
- perf_bp_event(bp, regs);
+ if (ptrace_bp) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ perf_bp_event(bp[i], regs);
+ info[i] = NULL;
+ }
rc = NOTIFY_DONE;
- goto out;
+ goto reset;
}
- info->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
- if (IS_ENABLED(CONFIG_PPC_8xx)) {
- if (!dar_within_range(regs->dar, info))
- info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
- } else {
- if (!stepping_handler(regs, bp, info))
- goto out;
+ if (!IS_ENABLED(CONFIG_PPC_8xx)) {
+ if (larx_stcx) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ larx_stcx_err(bp[i], info[i]);
+ info[i] = NULL;
+ }
+ goto reset;
+ }
+
+ if (!stepping_handler(regs, bp, info, hit, instr))
+ goto reset;
}
/*
* As a policy, the callback is invoked in a 'trigger-after-execute'
* fashion
*/
- if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
- perf_bp_event(bp, regs);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ if (!(info[i]->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
+ perf_bp_event(bp[i], regs);
+ }
+
+reset:
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!info[i])
+ continue;
+ __set_breakpoint(info[i], i);
+ }
- __set_breakpoint(info, 0);
out:
rcu_read_unlock();
return rc;
@@ -366,26 +573,43 @@ static int single_step_dabr_instruction(struct die_args *args)
struct pt_regs *regs = args->regs;
struct perf_event *bp = NULL;
struct arch_hw_breakpoint *info;
+ int i;
+ bool found = false;
- bp = current->thread.last_hit_ubp;
/*
* Check if we are single-stepping as a result of a
* previous HW Breakpoint exception
*/
- if (!bp)
- return NOTIFY_DONE;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ bp = current->thread.last_hit_ubp[i];
+
+ if (!bp)
+ continue;
+
+ found = true;
+ info = counter_arch_bp(bp);
+
+ /*
+ * We shall invoke the user-defined callback function in the
+ * single stepping handler to confirm to 'trigger-after-execute'
+ * semantics
+ */
+ if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
+ perf_bp_event(bp, regs);
+ current->thread.last_hit_ubp[i] = NULL;
+ }
- info = counter_arch_bp(bp);
+ if (!found)
+ return NOTIFY_DONE;
- /*
- * We shall invoke the user-defined callback function in the single
- * stepping handler to confirm to 'trigger-after-execute' semantics
- */
- if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
- perf_bp_event(bp, regs);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ bp = __this_cpu_read(bp_per_reg[i]);
+ if (!bp)
+ continue;
- __set_breakpoint(info, 0);
- current->thread.last_hit_ubp = NULL;
+ info = counter_arch_bp(bp);
+ __set_breakpoint(info, i);
+ }
/*
* If the process was being single-stepped by ptrace, let the
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index cbd6e9b79401..7c64299b001a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -622,9 +622,6 @@ void do_break (struct pt_regs *regs, unsigned long address,
if (debugger_break_match(regs))
return;
- /* Clear the breakpoint */
- hw_breakpoint_disable();
-
/* Deliver the signal to userspace */
force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
}
--
2.21.1
Xmon allows overwriting breakpoints because it's supported by only
one dawr. But with multiple dawrs, overwriting becomes ambiguous
or unnecessary complicated. So let's not allow it.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/xmon/xmon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 508d353e7f06..a1e36501af25 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1382,6 +1382,10 @@ bpt_cmds(void)
printf("Hardware data breakpoint not supported on this cpu\n");
break;
}
+ if (dabr.enabled) {
+ printf("Couldn't find free breakpoint register\n");
+ break;
+ }
mode = 7;
cmd = inchar();
if (cmd == 'r')
--
2.21.1
Le 01/04/2020 à 08:13, Ravi Bangoria a écrit :
> Instead of disabling only first watchpoint, disable all available
> watchpoints while clearing dawr_force_enable.
Can you also explain why you change the function name ?
>
> Signed-off-by: Ravi Bangoria <[email protected]>
> ---
> arch/powerpc/kernel/dawr.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
> index 311e51ee09f4..5c882f07ac7d 100644
> --- a/arch/powerpc/kernel/dawr.c
> +++ b/arch/powerpc/kernel/dawr.c
> @@ -50,9 +50,13 @@ int set_dawr(struct arch_hw_breakpoint *brk, int nr)
> return 0;
> }
>
> -static void set_dawr_cb(void *info)
> +static void disable_dawrs(void *info)
Wouldn't it be better to keep _cb at the end of the function ?
> {
> - set_dawr(info, 0);
> + struct arch_hw_breakpoint null_brk = {0};
> + int i;
> +
> + for (i = 0; i < nr_wp_slots(); i++)
> + set_dawr(&null_brk, i);
> }
>
> static ssize_t dawr_write_file_bool(struct file *file,
> @@ -74,7 +78,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
>
> /* If we are clearing, make sure all CPUs have the DAWR cleared */
> if (!dawr_force_enable)
> - smp_call_function(set_dawr_cb, &null_brk, 0);
> + smp_call_function(disable_dawrs, NULL, 0);
>
> return rc;
> }
>
Christophe
Le 01/04/2020 à 08:13, 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 <[email protected]>
> ---
> arch/powerpc/include/asm/processor.h | 2 +-
> arch/powerpc/include/asm/sstep.h | 2 +
> arch/powerpc/kernel/hw_breakpoint.c | 396 +++++++++++++++++++++------
> arch/powerpc/kernel/process.c | 3 -
> 4 files changed, 313 insertions(+), 90 deletions(-)
>
[...]
> -static bool
> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
> +static bool dar_user_range_overlaps(unsigned long dar, int size,
> + struct arch_hw_breakpoint *info)
> {
> return ((dar <= info->address + info->len - 1) &&
> (dar + size - 1 >= info->address));
> }
Here and several other places, I think it would be more clear if you
could avoid the - 1 :
return ((dar < info->address + info->len) &&
(dar + 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) - 1;
> +
> + return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
> +}
hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
return ((hw_start_addr <= dar) && (hw_end_addr > dar));
Christophe
> +
> +static bool dar_hw_range_overlaps(unsigned long dar, int size,
> + 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) - 1;
> +
> + return ((dar <= hw_end_addr) && (dar + size - 1 >= hw_start_addr));
> +}
Same
Christophe
Le 01/04/2020 à 08:13, Ravi Bangoria a écrit :
> With Book3s DAWR, ptrace and perf watchpoints on powerpc behaves
> differently. Ptrace watchpoint works in one-shot mode and generates
> signal before executing instruction. It's ptrace user's job to
> single-step the instruction and re-enable the watchpoint. OTOH, in
> case of perf watchpoint, kernel emulates/single-steps the instruction
> and then generates event. If perf and ptrace creates two events with
> same or overlapping address ranges, it's ambiguous to decide who
> should single-step the instruction. Because of this issue, don't
> allow perf and ptrace watchpoint at the same time if their address
> range overlaps.
>
> Signed-off-by: Ravi Bangoria <[email protected]>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 2 +
> arch/powerpc/kernel/hw_breakpoint.c | 222 +++++++++++++++++++++++
> kernel/events/hw_breakpoint.c | 16 ++
> 3 files changed, 240 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index abc4603c0efe..9d3bd1169591 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -70,6 +70,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
> unsigned long val, void *data);
> int arch_install_hw_breakpoint(struct perf_event *bp);
> void arch_uninstall_hw_breakpoint(struct perf_event *bp);
> +int arch_reserve_bp_slot(struct perf_event *bp);
> +void arch_release_bp_slot(struct perf_event *bp);
> void arch_unregister_hw_breakpoint(struct perf_event *bp);
> void hw_breakpoint_pmu_read(struct perf_event *bp);
> extern void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 07a6cdea84ed..f813acb0d9f0 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -123,6 +123,228 @@ static bool is_ptrace_bp(struct perf_event *bp)
> return bp->overflow_handler == ptrace_triggered;
> }
>
> +struct breakpoint {
> + struct list_head list;
> + struct perf_event *bp;
> + bool ptrace_bp;
> +};
> +
> +static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
> +static LIST_HEAD(task_bps);
> +
> +static struct breakpoint *alloc_breakpoint(struct perf_event *bp)
> +{
> + struct breakpoint *tmp;
> +
> + tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return ERR_PTR(-ENOMEM);
> + tmp->bp = bp;
> + tmp->ptrace_bp = is_ptrace_bp(bp);
> + return tmp;
> +}
> +
> +static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
> +{
> + __u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
> +
> + bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
> + bp1_eaddr = ALIGN(bp1->attr.bp_addr + bp1->attr.bp_len, HW_BREAKPOINT_SIZE) - 1;
> + bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
> + bp2_eaddr = ALIGN(bp2->attr.bp_addr + bp2->attr.bp_len, HW_BREAKPOINT_SIZE) - 1;
> +
> + return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);
Could avoid the - 1 on bp1_eaddr and bp2_eaddr by doing:
return (bp1_saddr < bp2_eaddr && bp1_eaddr > bp2_saddr);
Christophe
On 4/1/20 12:03 PM, Christophe Leroy wrote:
>
>
> Le 01/04/2020 à 08:13, Ravi Bangoria a écrit :
>> Instead of disabling only first watchpoint, disable all available
>> watchpoints while clearing dawr_force_enable.
>
> Can you also explain why you change the function name ?
Right. I should have. Will add it in next version.
>
>>
>> Signed-off-by: Ravi Bangoria <[email protected]>
>> ---
>> arch/powerpc/kernel/dawr.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
>> index 311e51ee09f4..5c882f07ac7d 100644
>> --- a/arch/powerpc/kernel/dawr.c
>> +++ b/arch/powerpc/kernel/dawr.c
>> @@ -50,9 +50,13 @@ int set_dawr(struct arch_hw_breakpoint *brk, int nr)
>> return 0;
>> }
>> -static void set_dawr_cb(void *info)
>> +static void disable_dawrs(void *info)
>
> Wouldn't it be better to keep _cb at the end of the function ?
Sure.
Ravi
On 4/1/20 12:20 PM, Christophe Leroy wrote:
>
>
> Le 01/04/2020 à 08:13, 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 <[email protected]>
>> ---
>> arch/powerpc/include/asm/processor.h | 2 +-
>> arch/powerpc/include/asm/sstep.h | 2 +
>> arch/powerpc/kernel/hw_breakpoint.c | 396 +++++++++++++++++++++------
>> arch/powerpc/kernel/process.c | 3 -
>> 4 files changed, 313 insertions(+), 90 deletions(-)
>>
>
> [...]
>
>> -static bool
>> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>> + struct arch_hw_breakpoint *info)
>> {
>> return ((dar <= info->address + info->len - 1) &&
>> (dar + size - 1 >= info->address));
>> }
>
> Here and several other places, I think it would be more clear if you could avoid the - 1 :
>
> return ((dar < info->address + info->len) &&
> (dar + size > info->address));
Ok. see below...
>
>
>> +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) - 1;
>> +
>> + return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
>> +}
>
> hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
>
> return ((hw_start_addr <= dar) && (hw_end_addr > dar));
I'm using -1 while calculating end address is to make it
inclusive. If I don't use -1, the end address points to a
location outside of actual range, i.e. it's not really an
end address.
Ravi
Le 01/04/2020 à 11:13, Ravi Bangoria a écrit :
>
>
> On 4/1/20 12:20 PM, Christophe Leroy wrote:
>>
>>
>> Le 01/04/2020 à 08:13, 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 <[email protected]>
>>> ---
>>> arch/powerpc/include/asm/processor.h | 2 +-
>>> arch/powerpc/include/asm/sstep.h | 2 +
>>> arch/powerpc/kernel/hw_breakpoint.c | 396 +++++++++++++++++++++------
>>> arch/powerpc/kernel/process.c | 3 -
>>> 4 files changed, 313 insertions(+), 90 deletions(-)
>>>
>>
>> [...]
>>
>>> -static bool
>>> -dar_range_overlaps(unsigned long dar, int size, struct
>>> arch_hw_breakpoint *info)
>>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>>> + struct arch_hw_breakpoint *info)
>>> {
>>> return ((dar <= info->address + info->len - 1) &&
>>> (dar + size - 1 >= info->address));
>>> }
>>
>> Here and several other places, I think it would be more clear if you
>> could avoid the - 1 :
>>
>> return ((dar < info->address + info->len) &&
>> (dar + size > info->address));
>
> Ok. see below...
>
>>
>>
>>> +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) - 1;
>>> +
>>> + return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
>>> +}
>>
>> hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
>>
>> return ((hw_start_addr <= dar) && (hw_end_addr > dar));
>
> I'm using -1 while calculating end address is to make it
> inclusive. If I don't use -1, the end address points to a
> location outside of actual range, i.e. it's not really an
> end address.
But that's what is done is several places, for instance:
https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/mm/dma-noncoherent.c#L22
https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/include/asm/book3s/32/kup.h#L92
In several places like this, end is outside of the range. My feeling is
that is helps with readability.
Christophe
On 4/1/20 2:50 PM, Christophe Leroy wrote:
>
>
> Le 01/04/2020 à 11:13, Ravi Bangoria a écrit :
>>
>>
>> On 4/1/20 12:20 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 01/04/2020 à 08:13, 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 <[email protected]>
>>>> ---
>>>> arch/powerpc/include/asm/processor.h | 2 +-
>>>> arch/powerpc/include/asm/sstep.h | 2 +
>>>> arch/powerpc/kernel/hw_breakpoint.c | 396 +++++++++++++++++++++------
>>>> arch/powerpc/kernel/process.c | 3 -
>>>> 4 files changed, 313 insertions(+), 90 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> -static bool
>>>> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
>>>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>>>> + struct arch_hw_breakpoint *info)
>>>> {
>>>> return ((dar <= info->address + info->len - 1) &&
>>>> (dar + size - 1 >= info->address));
>>>> }
>>>
>>> Here and several other places, I think it would be more clear if you could avoid the - 1 :
>>>
>>> return ((dar < info->address + info->len) &&
>>> (dar + size > info->address));
>>
>> Ok. see below...
>>
>>>
>>>
>>>> +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) - 1;
>>>> +
>>>> + return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
>>>> +}
>>>
>>> hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
>>>
>>> return ((hw_start_addr <= dar) && (hw_end_addr > dar));
>>
>> I'm using -1 while calculating end address is to make it
>> inclusive. If I don't use -1, the end address points to a
>> location outside of actual range, i.e. it's not really an
>> end address.
>
> But that's what is done is several places, for instance:
>
> https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/mm/dma-noncoherent.c#L22
>
> https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/include/asm/book3s/32/kup.h#L92
>
> In several places like this, end is outside of the range. My feeling is that is helps with readability.
Agreed, it helps with readability. Will change it.
Thanks for the links.
Ravi