Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Ravi Bangoria <ravi.bangoria@linux.ibm.com>]

On 3/17/20 4:29 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:58, 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.
> 
> I think using 'we' is to be avoided in commit message.
> 
> Could be something like:
> 
> "Currently the handler assumes there is only one watchpoint supported by hw"
> 
>>
>> So far, with only one watchpoint, the handler was simple. But with
>> multiple watchpoints, we need a mechanism to detect which watchpoint
>> caused the exception. HW doesn't provide that information and thus
>> we need software logic for this. This makes exception handling bit
>> more complex.
> 
> Same here, the 'we' should be avoided.
> 
> 
> This patch is pretty big. I think you should explain a bit more what is done. Otherwise it is difficult to review.

I understand, and the diff is also messy. But splitting this into granular
patches looks difficult to me.

The patch enables core hw-breakpoint subsystem to install/uninstall more than one
watchpoint, basically converting direct accesses of data structures in a loop.
Similarly changes in thread_chage_pc(), single_step_dabr_instruction() etc.

Now, with more than one watchpoints, 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, we 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. This logic is
implemented by check_constraints() function.

...

>> -static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
>> +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
> 
> Is this name change directly related to the patch ?

Yes. now we have two functions: dar_in_user_range() and dar_in_hw_range().
More detail below...

> 
>>   {
>>       return ((info->address <= dar) && (dar - info->address < info->len));
>>   }
>> -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)
> 
> Same question.

For single DAWR, hw_breakpoint_handler() logic is:

	if (user range overlaps with actual access range)
		valid event;
	else
		valid but extraneous;

With multiple breakpoints, this logic can't work when hw is not telling us which
DAWR caused exception. So the logic is like:

	for (i = 0; i < nr_wps; i++) {
		if (user range overlaps with actual access range)
			valid event;
		else if (hw range overlaps with actual access range)
			valid but extraneous;
		else
			exception cause by some other dawr;
	}

So we have two functions: dar_user_range_overlaps() and dar_hw_range_overlaps().
If reading instruction fails, we can't get actual access range an in that case
we use: dar_in_user_range() and dar_in_hw_range().

Thanks,
Ravi