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.
