Re: [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events

[Ravi Bangoria]

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