Re: [PATCH v2 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

From: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
To: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: mpe@ellerman.id.au, mikey@neuling.org, apopple@linux.ibm.com,
	paulus@samba.org, npiggin@gmail.com,
	naveen.n.rao@linux.vnet.ibm.com, peterz@infradead.org,
	jolsa@kernel.org, oleg@redhat.com, fweisbec@gmail.com,
	mingo@kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org,
	Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Subject: Re: [PATCH v2 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Date: Wed, 1 Apr 2020 14:53:45 +0530	[thread overview]
Message-ID: <955bf95b-0cda-dfa4-57ca-3f0f4415b5ea@linux.ibm.com> (raw)
In-Reply-To: <78559ff4-c2c3-e652-a906-8f40673b53d6@c-s.fr>

On 4/1/20 2:50 PM, Christophe Leroy wrote:
> 
> 
> Le 01/04/2020 à 11:13, Ravi Bangoria a écrit :
>>
>>
>> On 4/1/20 12:20 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 01/04/2020 à 08:13, Ravi Bangoria a écrit :
>>>> Currently we assume that we have only one watchpoint supported by hw.
>>>> Get rid of that assumption and use dynamic loop instead. This should
>>>> make supporting more watchpoints very easy.
>>>>
>>>> With more than one watchpoint, exception handler need to know which
>>>> DAWR caused the exception, and hw currently does not provide it. So
>>>> we need sw logic for the same. To figure out which DAWR caused the
>>>> exception, check all different combinations of user specified range,
>>>> dawr address range, actual access range and dawrx constrains. For ex,
>>>> if user specified range and actual access range overlaps but dawrx is
>>>> configured for readonly watchpoint and the instruction is store, this
>>>> DAWR must not have caused exception.
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>>> ---
>>>>   arch/powerpc/include/asm/processor.h |   2 +-
>>>>   arch/powerpc/include/asm/sstep.h     |   2 +
>>>>   arch/powerpc/kernel/hw_breakpoint.c  | 396 +++++++++++++++++++++------
>>>>   arch/powerpc/kernel/process.c        |   3 -
>>>>   4 files changed, 313 insertions(+), 90 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> -static bool
>>>> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
>>>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>>>> +                    struct arch_hw_breakpoint *info)
>>>>   {
>>>>       return ((dar <= info->address + info->len - 1) &&
>>>>           (dar + size - 1 >= info->address));
>>>>   }
>>>
>>> Here and several other places, I think it would be more clear if you could avoid the - 1 :
>>>
>>>      return ((dar < info->address + info->len) &&
>>>          (dar + size > info->address));
>>
>> Ok. see below...
>>
>>>
>>>
>>>> +static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
>>>> +{
>>>> +    unsigned long hw_start_addr, hw_end_addr;
>>>> +
>>>> +    hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
>>>> +    hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE) - 1;
>>>> +
>>>> +    return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
>>>> +}
>>>
>>>      hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
>>>
>>>      return ((hw_start_addr <= dar) && (hw_end_addr > dar));
>>
>> I'm using -1 while calculating end address is to make it
>> inclusive. If I don't use -1, the end address points to a
>> location outside of actual range, i.e. it's not really an
>> end address.
> 
> But that's what is done is several places, for instance:
> 
> https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/mm/dma-noncoherent.c#L22
> 
> https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/include/asm/book3s/32/kup.h#L92
> 
> In several places like this, end is outside of the range. My feeling is that is helps with readability.

Agreed, it helps with readability. Will change it.
Thanks for the links.

Ravi