Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target

[Ravi Bangoria]

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.