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

[Ravi Bangoria]

On 6/18/19 12:16 PM, Christophe Leroy wrote:
>> Â +/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */
>> +static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
>> +{
>> +ÂÂÂ u16 length_max = 8;
>> +ÂÂÂ u16 final_len;
> 
> You should be more consistent in naming. If one is called final_len, the other one should be called max_len.

Copy/paste :). Will change it.

> 
>> +ÂÂÂ unsigned long start_addr, end_addr;
>> +
>> +ÂÂÂ final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr);
>> +
>> +ÂÂÂ if (dawr_enabled()) {
>> +ÂÂÂÂÂÂÂ length_max = 512;
>> +ÂÂÂÂÂÂÂ /* DAWR region can't cross 512 bytes boundary */
>> +ÂÂÂÂÂÂÂ if ((start_addr >> 9) != (end_addr >> 9))
>> +ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ if (final_len > length_max)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ return 0;
>> +}
>> +
> 
> Is many places, we have those numeric 512 and 9 shift. Could we replace them by some symbol, for instance DAWR_SIZE and DAWR_SHIFT ?

I don't see any other place where we check for boundary limit.

[...]

> 
>> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *start_addr,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *end_addr)
>> +{
>> +ÂÂÂ *start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
>> +ÂÂÂ *end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
>> +ÂÂÂ return *end_addr - *start_addr + 1;
>> +}
> 
> This function gives horrible code (a couple of unneeded store/re-read and read/re-read).
> 
> 000006bc <hw_breakpoint_get_final_len>:
> ÂÂÂÂ 6bc:ÂÂÂ 81 23 00 00ÂÂÂÂ lwzÂÂÂÂ r9,0(r3)
>  6c0: 55 29 00 38 rlwinm r9,r9,0,0,28
> ÂÂÂÂ 6c4:ÂÂÂ 91 24 00 00ÂÂÂÂ stwÂÂÂÂ r9,0(r4)
> ÂÂÂÂ 6c8:ÂÂÂ 81 43 00 00ÂÂÂÂ lwzÂÂÂÂ r10,0(r3)
> ÂÂÂÂ 6cc:ÂÂÂ a1 23 00 06ÂÂÂÂ lhzÂÂÂÂ r9,6(r3)
> ÂÂÂÂ 6d0:ÂÂÂ 38 6a ff ffÂÂÂÂ addiÂÂÂ r3,r10,-1
> ÂÂÂÂ 6d4:ÂÂÂ 7c 63 4a 14ÂÂÂÂ addÂÂÂÂ r3,r3,r9
> ÂÂÂÂ 6d8:ÂÂÂ 60 63 00 07ÂÂÂÂ oriÂÂÂÂ r3,r3,7
> ÂÂÂÂ 6dc:ÂÂÂ 90 65 00 00ÂÂÂÂ stwÂÂÂÂ r3,0(r5)
> ÂÂÂÂ 6e0:ÂÂÂ 38 63 00 01ÂÂÂÂ addiÂÂÂ r3,r3,1
> ÂÂÂÂ 6e4:ÂÂÂ 81 24 00 00ÂÂÂÂ lwzÂÂÂÂ r9,0(r4)
> ÂÂÂÂ 6e8:ÂÂÂ 7c 69 18 50ÂÂÂÂ subfÂÂÂ r3,r9,r3
>  6ec: 54 63 04 3e clrlwi r3,r3,16
> ÂÂÂÂ 6f0:ÂÂÂ 4e 80 00 20ÂÂÂÂ blr
> 
> Below code gives something better:
> 
> u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *start_addr,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *end_addr)
> {
> ÂÂÂÂunsigned long address = brk->address;
> ÂÂÂÂunsigned long len = brk->len;
> ÂÂÂÂunsigned long start = address & ~HW_BREAKPOINT_ALIGN;
> ÂÂÂÂunsigned long end = (address + len - 1) | HW_BREAKPOINT_ALIGN;
> 
> ÂÂÂÂ*start_addr = start;
> ÂÂÂÂ*end_addr = end;
> ÂÂÂÂreturn end - start + 1;
> }
> 
> 000006bc <hw_breakpoint_get_final_len>:
> ÂÂÂÂ 6bc:ÂÂÂ 81 43 00 00ÂÂÂÂ lwzÂÂÂÂ r10,0(r3)
> ÂÂÂÂ 6c0:ÂÂÂ a1 03 00 06ÂÂÂÂ lhzÂÂÂÂ r8,6(r3)
> ÂÂÂÂ 6c4:ÂÂÂ 39 2a ff ffÂÂÂÂ addiÂÂÂ r9,r10,-1
> ÂÂÂÂ 6c8:ÂÂÂ 7d 28 4a 14ÂÂÂÂ addÂÂÂÂ r9,r8,r9
>  6cc: 55 4a 00 38 rlwinm r10,r10,0,0,28
> ÂÂÂÂ 6d0:ÂÂÂ 61 29 00 07ÂÂÂÂ oriÂÂÂÂ r9,r9,7
> ÂÂÂÂ 6d4:ÂÂÂ 91 44 00 00ÂÂÂÂ stwÂÂÂÂ r10,0(r4)
>  6d8: 20 6a 00 01 subfic r3,r10,1
> ÂÂÂÂ 6dc:ÂÂÂ 91 25 00 00ÂÂÂÂ stwÂÂÂÂ r9,0(r5)
> ÂÂÂÂ 6e0:ÂÂÂ 7c 63 4a 14ÂÂÂÂ addÂÂÂÂ r3,r3,r9
>  6e4: 54 63 04 3e clrlwi r3,r3,16
> ÂÂÂÂ 6e8:ÂÂÂ 4e 80 00 20ÂÂÂÂ blr
> 
> 
> And regardless, that's a pitty to have this function using pointers which are from local variables in the callers, as we loose the benefit of registers. Couldn't this function go in the .h as a static inline ? I'm sure the result would be worth it.

This is obviously a bit of optimization, but I like Mikey's idea of
storing start_addr and end_addr in the arch_hw_breakpoint. That way
we don't have to recalculate length every time in set_dawr.