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;
Subject: Re: [PATCH v3 2/3] Powerpc64/Watchpoint: Don't ignore extraneous
 exceptions
To:     Ravi Bangoria <ravi.bangoria@linux.ibm.com>, mpe@ellerman.id.au,
        mikey@neuling.org
Cc:     benh@kernel.crashing.org, paulus@samba.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        npiggin@gmail.com, naveen.n.rao@linux.vnet.ibm.com
References: <20190710045445.31037-1-ravi.bangoria@linux.ibm.com>
 <20190710045445.31037-3-ravi.bangoria@linux.ibm.com>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <1f3fd425-3d2f-8d18-eff1-01ca5b605ba0@c-s.fr>
Date:   Wed, 10 Jul 2019 08:27:32 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20190710045445.31037-3-ravi.bangoria@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@linux.ibm.com>
> ---
>   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 */
>