Re: [PATCH v3 2/3] Powerpc64/Watchpoint: Don't ignore extraneous exceptions

[Christophe Leroy]

Le 10/07/2019 Ã 06:54, Ravi Bangoria a ÃcritÂ:
> On Powerpc64, watchpoint match range is double-word granular. On
> a watchpoint hit, DAR is set to the first byte of overlap between
> actual access and watched range. And thus it's quite possible that
> DAR does not point inside user specified range. Ex, say user creates
> a watchpoint with address range 0x1004 to 0x1007. So hw would be
> configured to watch from 0x1000 to 0x1007. If there is a 4 byte
> access from 0x1002 to 0x1005, DAR will point to 0x1002 and thus
> interrupt handler considers it as extraneous, but it's actually not,
> because part of the access belongs to what user has asked. So, let
> kernel pass it on to user and let user decide what to do with it
> instead of silently ignoring it. The drawback is, it can generate
> false positive events.

Why adding some #ifdefs based on CONFIG_8xx ?

I see your commit log mentions 'Powerpc64'. What about BOOK3S/32 ?

Christophe

> Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx>
> ---
>   arch/powerpc/kernel/hw_breakpoint.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 5c876e986c18..c457d52778e3 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -204,9 +204,10 @@ int hw_breakpoint_handler(struct die_args *args)
>   #ifndef CONFIG_PPC_8xx
>   	int stepped = 1;
>   	unsigned int instr;
> +#else
> +	unsigned long dar = regs->dar;
>   #endif
>   	struct arch_hw_breakpoint *info;
> -	unsigned long dar = regs->dar;
>   
>   	/* Disable breakpoints during exception handling */
>   	hw_breakpoint_disable();
> @@ -240,14 +241,14 @@ int hw_breakpoint_handler(struct die_args *args)
>   
>   	/*
>   	 * Verify if dar lies within the address range occupied by the symbol
> -	 * being watched to filter extraneous exceptions.  If it doesn't,
> -	 * we still need to single-step the instruction, but we don't
> -	 * generate an event.
> +	 * being watched to filter extraneous exceptions.
>   	 */
>   	info->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +#ifdef CONFIG_PPC_8xx
>   	if (!((bp->attr.bp_addr <= dar) &&
>   	      (dar - bp->attr.bp_addr < bp->attr.bp_len)))
>   		info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +#endif
>   
>   #ifndef CONFIG_PPC_8xx
>   	/* Do not emulate user-space instructions, instead single-step them */