[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1f3fd425-3d2f-8d18-eff1-01ca5b605ba0@c-s.fr>
Date: Wed, 10 Jul 2019 08:27:32 +0200
From: Christophe Leroy <christophe.leroy@....fr>
To: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>, mpe@...erman.id.au,
mikey@...ling.org
Cc: benh@...nel.crashing.org, paulus@...ba.org,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
npiggin@...il.com, naveen.n.rao@...ux.vnet.ibm.com
Subject: Re: [PATCH v3 2/3] Powerpc64/Watchpoint: Don't ignore extraneous
exceptions
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@...ux.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 */
>
Powered by blists - more mailing lists