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

On 6/18/19 12:16 PM, Christophe Leroy wrote:

quoted

  +/* 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.

quoted

+    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.

[...]

quoted

+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.