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 5/5] Powerpc/Watchpoint: Fix length calculation for
 unaligned target
To:     Michael Neuling <mikey@neuling.org>
Cc:     mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        npiggin@gmail.com, christophe.leroy@c-s.fr,
        naveen.n.rao@linux.vnet.ibm.com,
        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
References: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
 <20190618042732.5582-6-ravi.bangoria@linux.ibm.com>
 <707bc0b664b8ebbb843a1541155fed219c216035.camel@neuling.org>
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Date:   Wed, 19 Jun 2019 13:15:53 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <707bc0b664b8ebbb843a1541155fed219c216035.camel@neuling.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <8a8a17b6-bdd6-4efb-7937-b1af105e08e0@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 6/18/19 7:02 PM, Michael Neuling wrote:
> On Tue, 2019-06-18 at 09:57 +0530, Ravi Bangoria wrote:
>> Watchpoint match range is always doubleword(8 bytes) aligned on
>> powerpc. If the given range is crossing doubleword boundary, we
>> need to increase the length such that next doubleword also get
>> covered. Ex,
>>
>>           address   len = 6 bytes
>>                 |=========.
>>    |------------v--|------v--------|
>>    | | | | | | | | | | | | | | | | |
>>    |---------------|---------------|
>>     <---8 bytes--->
>>
>> In such case, current code configures hw as:
>>   start_addr = address & ~HW_BREAKPOINT_ALIGN
>>   len = 8 bytes
>>
>> And thus read/write in last 4 bytes of the given range is ignored.
>> Fix this by including next doubleword in the length. Watchpoint
>> exception handler already ignores extraneous exceptions, so no
>> changes required for that.
> 
> Nice catch. Thanks.
> 
> I assume this has been broken forever? Should we be CCing stable? If so, it
> would be nice to have this self contained (separate from the refactor) so we can
> more easily backport it.

Yes this has been broken forever. I'll add Fixes: tag and cc stable.

> 
> Also, can you update 
> tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c to catch this issue?

Sure, will add the test case.

[...]

>> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
>> +				unsigned long *start_addr,
>> +				unsigned long *end_addr)
> 
> I don't really like this.  "final" is not a good name. Something like hardware
> would be better.
> 
> Also, can you put the start_addr and end addr in the arch_hw_breakpoint rather
> than doing what you have above.  Call them hw_start_addr, hw_end_addr.
> 
> We could even set these two new addresses where we set the set of
> arch_hw_breakpoint rather than having this late call.

Sure, will use 'hw_' prefix for them.