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;
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Subject: Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for
 unaligned target
To:     Christophe Leroy <christophe.leroy@c-s.fr>
Cc:     mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org,
        mikey@neuling.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org, npiggin@gmail.com,
        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>
 <3390c3c2-8290-da55-a183-872c593c7b1e@c-s.fr>
Date:   Wed, 19 Jun 2019 12:21:37 +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: <3390c3c2-8290-da55-a183-872c593c7b1e@c-s.fr>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Message-Id: <1f3873b7-d924-61ad-2f0e-f6cc12c012ea@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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.