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

From: Christophe Leroy
Date: Wed Apr 01 2020 - 05:20:30 EST




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@xxxxxxxxxxxxx>
---
 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.

Christophe