| Message ID | 20190710045445.31037-3-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Powerpc64/Watchpoint: Few important fixes | expand |
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 */ >
On 7/10/19 11:57 AM, Christophe Leroy wrote: > > > 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 don't know how 8xx behaves so I'm keeping the current behavior(ignore extraneous exception) for 8xx. > > I see your commit log mentions 'Powerpc64'. What about BOOK3S/32 ? Hmm, I should not have mention 64 there. Yes, the change should cover both Books3S/64 and Book3S/32.
Ravi Bangoria wrote: > 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. I think you should do the additional validation here, instead of generating false positives. You should be able to read the instruction, run it through analyse_instr(), and then use OP_IS_LOAD_STORE() and GETSIZE() to understand the access range. This can be used to then perform a better match against what the user asked for. - Naveen
On 9/4/19 8:12 PM, Naveen N. Rao wrote: > Ravi Bangoria wrote: >> 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. > > I think you should do the additional validation here, instead of generating false positives. You should be able to read the instruction, run it through analyse_instr(), and then use OP_IS_LOAD_STORE() and GETSIZE() to understand the access range. This can be used to then perform a better match against what the user asked for. Ok. Let me see how feasible that is. But patch 1 and 3 are independent of this and can still go in. mpe? -Ravi
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 */
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. Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> --- arch/powerpc/kernel/hw_breakpoint.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)