Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag to pass DIE_DEBUG notification

[Frederic Weisbecker <fweisbec@gmail.com>]

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 <dongdong.deng@windriver.com>
> Reviewed-by: Bruce Ashfield <bruce.ashfield@windriver.com>
> ---
>  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.