On 07/23/2010 08:04 AM, Frederic Weisbecker wrote:
> On Fri, Jul 23, 2010 at 10:16:01AM +0800, Dongdong Deng wrote:
>
>> The hw_breakpoint subsystem consumes all the hardware
>> breakpoint exceptions since it hooks the notify_die
>> handlers first, this means that kgdb doesn't get the
>> opportunity to handle hw breakpoint exceptions generated
>> by kgdb itself.
>>
>> This patch adds an extend flag to perf_event_attr for
>> hw_breakpoint_handler() to decide to pass or stop the
>> DIE_DEBUG notification.
>>
>> As KGDB set that flag, hw_breakpoint_handler() will pass
>> the DIE_DEBUG notification, thus kgdb have the chance
>> to take DIE_DEBUG notification.
>>
>> Signed-off-by: Dongdong Deng <[email protected]>
>> Reviewed-by: Bruce Ashfield <[email protected]>
>> ---
>> arch/x86/kernel/hw_breakpoint.c | 14 ++++++++++++++
>> arch/x86/kernel/kgdb.c | 2 ++
>> include/linux/perf_event.h | 9 +++++++++
>> 3 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kernel/hw_breakpoint.c
>> b/arch/x86/kernel/hw_breakpoint.c
>> index a8f1b80..b38f786 100644
>> --- a/arch/x86/kernel/hw_breakpoint.c
>> +++ b/arch/x86/kernel/hw_breakpoint.c
>> @@ -406,6 +406,8 @@ EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
>> * ii) When there are more bits than trap<n> set in DR6 register (such
>> * as BD, BS or BT) indicating that more than one debug condition is
>> * met and requires some more action in do_debug().
>> + * iii) The source of hw breakpoint event want to handle the event
>> + * by itself, currently just KGDB have this notion.
>> *
>> * NOTIFY_STOP returned for all other cases
>> *
>> @@ -464,6 +466,18 @@ static int __kprobes hw_breakpoint_handler(struct
>> die_args *args)
>> break;
>> }
>>
>> + if (bp->attr.flag == SKIP_HWBP_EVENT_PERF_FLAG) {
>> + /*
>> + * when attr.flag is set to SKIP_HWBP_EVENT_PERF_FLAG
>> + * it indicates currently hw breakpoint event
>> + * source want to handle this event by itself.
>> + * thus return NOTIFY_DONE here.
>> + */
>> + rc = NOTIFY_DONE;
>> + rcu_read_unlock();
>> + break;
>> + }
>> +
>>
>
>
>
> No. We really shouldn't make a user ABI change (adding attr.flag) just
> to solve an in-kernel-only problem.
>
> And moreover we probably don't need flags at all. Why not just turning kgdb
> handler
> into a higher priority?
>
> I don't even remember why kgdb has its own handler instead of using the
> struct perf_event:overflow_handler. May be that's because of the early
> breakpoints.
>
>
The patch may or may not be the right way to solve the problem. It is
worth noting that early breakpoints are handled separately with a direct
writes to the debug registers so this API does not apply.
This patch effectively causes the events to get passed to the normal
handlers.
The source of the original problem (which was merged in 2.6.35) is
commit: 018cbffe6819f6f8db20a0a3acd9bab9bfd667e4 - Merge commit
'v2.6.33' into perf/core
Specifically this line right here:
@@@ -502,6 -486,8 +486,6 @@@ static int __kprobes hw_breakpoint_hand
rcu_read_lock();
bp = per_cpu(bp_per_reg[i], cpu);
- if (bp)
- rc = NOTIFY_DONE;
Because the NOTIFY_DONE is never set, a default value of NOTIFY_STOP is
passed at the end and kgdb never gets to see the break point even that
was never intended for the perf handler in the first place.
It is not as easy of course to just revert this patch because it changed
other logic.
Jason.
------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share
of $1 Million in cash or HP Products. Visit us here for more details:
http://p.sf.net/sfu/dev2dev-palm
_______________________________________________
Kgdb-bugreport mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
