Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and
 ptrace events
To:     Christophe Leroy <christophe.leroy@c-s.fr>
Cc:     mpe@ellerman.id.au, mikey@neuling.org, apopple@linux.ibm.com,
        paulus@samba.org, npiggin@gmail.com,
        naveen.n.rao@linux.vnet.ibm.com, peterz@infradead.org,
        jolsa@kernel.org, oleg@redhat.com, fweisbec@gmail.com,
        mingo@kernel.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org,
        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
References: <20200309085806.155823-1-ravi.bangoria@linux.ibm.com>
 <20200309085806.155823-14-ravi.bangoria@linux.ibm.com>
 <673cf087-1422-84b3-e3bc-13c4dd491414@c-s.fr>
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Date:   Wed, 18 Mar 2020 18:05:44 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <673cf087-1422-84b3-e3bc-13c4dd491414@c-s.fr>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Message-Id: <ee92b133-c5f8-0191-a4b0-858794ebe69a@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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