Messages in this thread | | | Subject | Re: [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events | From | Ravi Bangoria <> | Date | Wed, 18 Mar 2020 18:05:44 +0530 |
| |
On 3/17/20 4:38 PM, Christophe Leroy wrote: > > > Le 09/03/2020 à 09:58, Ravi Bangoria a écrit : >> ptrace and perf watchpoints on powerpc behaves differently. Ptrace > > On the 8xx, ptrace generates signal after executing the instruction.
8xx logic is unchanged. I should have mentioned "Book3s DAWR".
> >> 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 ptrace and perf event can't coexist when the address range >> overlaps. > > Ok, and then ? What's the purpose of this (big) patch ?
Don't allow perf and ptrace watchpoint at the same time if their address range overlaps.
...
>> +struct breakpoint { >> + struct list_head list; >> + struct perf_event *bp; >> + bool ptrace_bp; >> +}; > > Don't we have an equivalent struct already ?
No. Using this we track percpu and perthread watchpoints for both perf and ptrace. This problems is powerpc(DAWR) specific and thus we need to hook arch specific logic in watchopint installation/uninstallation path.
...
>> +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 = bp1->attr.bp_addr & ~HW_BREAKPOINT_ALIGN; >> + bp1_eaddr = (bp1->attr.bp_addr + bp1->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN; >> + bp2_saddr = bp2->attr.bp_addr & ~HW_BREAKPOINT_ALIGN; >> + bp2_eaddr = (bp2->attr.bp_addr + bp2->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN; >> + >> + return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr); > > Would be better with something like (HW_BREAKPOINT_SIZE needs to be defined). > > bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE); > bp1_eaddr = ALIGN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE); > bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE); > bp2_eaddr = ALIGN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE); > > return (bp1_saddr < bp2_eaddr && bp1_eaddr > bp2_saddr);
Ok.
Thanks, Ravi
| |