Ingo,
Please pull the perf/urgent branch that can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
perf/urgent
Thanks,
Frederic
---
Frederic Weisbecker (1):
x86: Ignore trap bits on single step exceptions
Jason Wessel (1):
perf,hw_breakpoint: Initialize hardware api earlier
arch/x86/kernel/hw_breakpoint.c | 4 ++++
include/linux/hw_breakpoint.h | 4 ++++
kernel/hw_breakpoint.c | 3 +--
kernel/perf_event.c | 6 ++++++
4 files changed, 15 insertions(+), 2 deletions(-)
When a single step exception fires, the trap bits, used to
signal hardware breakpoints, are in a random state.
These trap bits might be set if another exception will follow,
like a breakpoint in the next instruction, or a watchpoint in the
previous one. Or there can be any junk there.
So if we handle these trap bits during the single step exception,
we are going to handle an exception twice, or we are going to
handle junk.
Just ignore them in this case.
This fixes https://bugzilla.kernel.org/show_bug.cgi?id=21332
Reported-by: Michael Stefaniuc <[email protected]>
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Maciej Rutecki <[email protected]>
Cc: Alexandre Julliard <[email protected]>
Cc: Jason Wessel <[email protected]>
Cc: All since 2.6.33.x <[email protected]>
---
arch/x86/kernel/hw_breakpoint.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index ff15c9d..42c5942 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -433,6 +433,10 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args)
dr6_p = (unsigned long *)ERR_PTR(args->err);
dr6 = *dr6_p;
+ /* If it's a single step, TRAP bits are random */
+ if (dr6 & DR_STEP)
+ return NOTIFY_DONE;
+
/* Do an early return if no trap bits are set in DR6 */
if ((dr6 & DR_TRAP_BITS) == 0)
return NOTIFY_DONE;
--
1.6.2.3
From: Jason Wessel <[email protected]>
When using early debugging, the kernel does not initialize the
hw_breakpoint API early enough and causes the late initialization of
the kernel debugger to fail. The boot arguments are:
earlyprintk=vga ekgdboc=kbd kgdbwait
Then simply type "go" at the kdb prompt and boot. The kernel will
later emit the message:
kgdb: Could not allocate hwbreakpoints
And at that point the kernel debugger will cease to work correctly.
The solution is to initialize the hw_breakpoint at the same time that
all the other perf call backs are initialized instead of using a
core_initcall() initialization which happens well after the kernel
debugger can make use of hardware breakpoints.
Signed-off-by: Jason Wessel <[email protected]>
CC: Frederic Weisbecker <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Frederic Weisbecker <[email protected]>
---
include/linux/hw_breakpoint.h | 4 ++++
kernel/hw_breakpoint.c | 3 +--
kernel/perf_event.c | 6 ++++++
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index a2d6ea4..d1e55fe 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -33,6 +33,8 @@ enum bp_type_idx {
#ifdef CONFIG_HAVE_HW_BREAKPOINT
+extern int __init init_hw_breakpoint(void);
+
static inline void hw_breakpoint_init(struct perf_event_attr *attr)
{
memset(attr, 0, sizeof(*attr));
@@ -108,6 +110,8 @@ static inline struct arch_hw_breakpoint *counter_arch_bp(struct perf_event *bp)
#else /* !CONFIG_HAVE_HW_BREAKPOINT */
+static inline int __init init_hw_breakpoint(void) { return 0; }
+
static inline struct perf_event *
register_user_hw_breakpoint(struct perf_event_attr *attr,
perf_overflow_handler_t triggered,
diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index 2c9120f..e532582 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -620,7 +620,7 @@ static struct pmu perf_breakpoint = {
.read = hw_breakpoint_pmu_read,
};
-static int __init init_hw_breakpoint(void)
+int __init init_hw_breakpoint(void)
{
unsigned int **task_bp_pinned;
int cpu, err_cpu;
@@ -655,6 +655,5 @@ static int __init init_hw_breakpoint(void)
return -ENOMEM;
}
-core_initcall(init_hw_breakpoint);
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 517d827..05b7d8c 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -31,6 +31,7 @@
#include <linux/kernel_stat.h>
#include <linux/perf_event.h>
#include <linux/ftrace_event.h>
+#include <linux/hw_breakpoint.h>
#include <asm/irq_regs.h>
@@ -6295,6 +6296,8 @@ perf_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
void __init perf_event_init(void)
{
+ int ret;
+
perf_event_init_all_cpus();
init_srcu_struct(&pmus_srcu);
perf_pmu_register(&perf_swevent);
@@ -6302,4 +6305,7 @@ void __init perf_event_init(void)
perf_pmu_register(&perf_task_clock);
perf_tp_register();
perf_cpu_notifier(perf_cpu_notify);
+
+ ret = init_hw_breakpoint();
+ WARN(ret, "hw_breakpoint initialization failed with: %d", ret);
}
--
1.6.2.3
On Sat, 2010-11-13 at 22:37 +0100, Frederic Weisbecker wrote:
> From: Jason Wessel <[email protected]>
>
> When using early debugging, the kernel does not initialize the
> hw_breakpoint API early enough and causes the late initialization of
> the kernel debugger to fail. The boot arguments are:
>
> earlyprintk=vga ekgdboc=kbd kgdbwait
>
> Then simply type "go" at the kdb prompt and boot. The kernel will
> later emit the message:
>
> kgdb: Could not allocate hwbreakpoints
>
> And at that point the kernel debugger will cease to work correctly.
>
> The solution is to initialize the hw_breakpoint at the same time that
> all the other perf call backs are initialized instead of using a
> core_initcall() initialization which happens well after the kernel
> debugger can make use of hardware breakpoints.
How early is it needed? I've got a patch converting the x86 (still need
to do the other hardware pmus) to early_initcall() instead of random
places in the arch bringup (notably before the perf core init).
On 11/13/2010 03:46 PM, Peter Zijlstra wrote:
> On Sat, 2010-11-13 at 22:37 +0100, Frederic Weisbecker wrote:
>
>> From: Jason Wessel <[email protected]>
>>
>> When using early debugging, the kernel does not initialize the
>> hw_breakpoint API early enough and causes the late initialization of
>> the kernel debugger to fail. The boot arguments are:
>>
>> earlyprintk=vga ekgdboc=kbd kgdbwait
>>
>> Then simply type "go" at the kdb prompt and boot. The kernel will
>> later emit the message:
>>
>> kgdb: Could not allocate hwbreakpoints
>>
>> And at that point the kernel debugger will cease to work correctly.
>>
>> The solution is to initialize the hw_breakpoint at the same time that
>> all the other perf call backs are initialized instead of using a
>> core_initcall() initialization which happens well after the kernel
>> debugger can make use of hardware breakpoints.
>>
>
> How early is it needed?
The HW breakpoint callback needs to be registered at the same time the
rest of the perf callbacks are registered. More specifically, the
infrastructure setup needs to be completed before the debugger calls
"register_wide_hw_breakpoint(&attr, NULL)" in arch/x86/kernel/kgdb.c.
After the debugger calls the registration function, from that point on
perf owns the reservation system for all hw break point registers.
> I've got a patch converting the x86 (still need
> to do the other hardware pmus) to early_initcall() instead of random
> places in the arch bringup (notably before the perf core init).
>
>
This sounds to me like it would be early enough. I could certainly run
the simple test case in the patch to make sure it still works, if you
point me to your patch(es). I imagine I should also test the hand off
procedure where the debugger uses the registers directly up until the
point that perf is capable of handling reservations for the hw
breakpoint slots.
Thanks,
Jason.
On Sun, 2010-11-14 at 07:33 -0600, Jason Wessel wrote:
>
> This sounds to me like it would be early enough. I could certainly run
> the simple test case in the patch to make sure it still works, if you
> point me to your patch(es). I imagine I should also test the hand off
> procedure where the debugger uses the registers directly up until the
> point that perf is capable of handling reservations for the hw
> breakpoint slots.
I had to actually write them -- I had some hacky things in my sysfs RFC.
Find attached.
* Frederic Weisbecker <[email protected]> wrote:
> Ingo,
>
> Please pull the perf/urgent branch that can be found at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> perf/urgent
>
> Thanks,
> Frederic
> ---
>
> Frederic Weisbecker (1):
> x86: Ignore trap bits on single step exceptions
>
> Jason Wessel (1):
> perf,hw_breakpoint: Initialize hardware api earlier
>
>
> arch/x86/kernel/hw_breakpoint.c | 4 ++++
> include/linux/hw_breakpoint.h | 4 ++++
> kernel/hw_breakpoint.c | 3 +--
> kernel/perf_event.c | 6 ++++++
> 4 files changed, 15 insertions(+), 2 deletions(-)
Pulled, thanks a lot Frederic!
Ingo
On 11/16/2010 03:24 PM, Peter Zijlstra wrote:
> On Sun, 2010-11-14 at 07:33 -0600, Jason Wessel wrote:
>> This sounds to me like it would be early enough. I could certainly run
>> the simple test case in the patch to make sure it still works, if you
>> point me to your patch(es). I imagine I should also test the hand off
>> procedure where the debugger uses the registers directly up until the
>> point that perf is capable of handling reservations for the hw
>> breakpoint slots.
>
> I had to actually write them -- I had some hacky things in my sysfs RFC.
> Find attached.
It is worth summarizing the out of band discussion for anyone following
this thread.
The patch set did not address the problem of initializing the
hw_breakpoint api early enough to allow the breakpoint register hand
off to work correctly.
> Index: linux-2.6/kernel/hw_breakpoint.c
> ===========================================================
> --- linux-2.6.orig/kernel/hw_breakpoint.c
> +++ linux-2.6/kernel/hw_breakpoint.c
> @@ -655,6 +655,6 @@ static int __init init_hw_breakpoint(voi
>
> return -ENOMEM;
> }
> -core_initcall(init_hw_breakpoint);
> +early_initcall(init_hw_breakpoint);
An early_initcall is still well after smp_init() and not early enough
for the kgdb arch setup to succeed when transitioning from early
debugging to late debugging.
There is a patch pending in the tip tree which moves the
hw_breakpoint_api registration to the same time the other perf
callbacks register. We will go with that for now.
In the long term picture, at some point we should have a hand off
function that tells an early user of the hw breakpoint registers to
stop using them. Presently there is a small window of overlap where
perf and kgdb both might write to the hw break point registers up
until the late kgdb initialization completes. Presently there are no
regression tests for this condition and it might be the case that no
one ever has to debug this area with a hardware breakpoint, so we'll
leave it as a TODO item for the time being.
Thanks,
Jason.
Hello Frederic,
On 11/13/2010 10:37 PM, Frederic Weisbecker wrote:
> When a single step exception fires, the trap bits, used to
> signal hardware breakpoints, are in a random state.
>
> These trap bits might be set if another exception will follow,
> like a breakpoint in the next instruction, or a watchpoint in the
> previous one. Or there can be any junk there.
>
> So if we handle these trap bits during the single step exception,
> we are going to handle an exception twice, or we are going to
> handle junk.
>
> Just ignore them in this case.
>
> This fixes https://bugzilla.kernel.org/show_bug.cgi?id=21332
sorry for the delay in testing this.
I have cherry-picked this patch on top of v2.6.37-rc3-102-gea49b16 and
the ntdll/exception tests pass now.
Many thanks
bye
michael
>
> Reported-by: Michael Stefaniuc<[email protected]>
> Signed-off-by: Frederic Weisbecker<[email protected]>
> Cc: Rafael J. Wysocki<[email protected]>
> Cc: Maciej Rutecki<[email protected]>
> Cc: Alexandre Julliard<[email protected]>
> Cc: Jason Wessel<[email protected]>
> Cc: All since 2.6.33.x<[email protected]>
> ---
> arch/x86/kernel/hw_breakpoint.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index ff15c9d..42c5942 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -433,6 +433,10 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args)
> dr6_p = (unsigned long *)ERR_PTR(args->err);
> dr6 = *dr6_p;
>
> + /* If it's a single step, TRAP bits are random */
> + if (dr6& DR_STEP)
> + return NOTIFY_DONE;
> +
> /* Do an early return if no trap bits are set in DR6 */
> if ((dr6& DR_TRAP_BITS) == 0)
> return NOTIFY_DONE;
--
Michael Stefaniuc Tel.: +49-711-96437-199
Consulting Communications Engineer Fax.: +49-711-96437-111
--------------------------------------------------------------------
Reg. Adresse: Red Hat GmbH, Otto-Hahn-Strasse 20, 85609 Dornach
Handelsregister: Amtsgericht Muenchen HRB 153243
Geschäftsführer: Brendan Lane, Charlie Peters, Michael Cunningham,
Charles Cachera
On Wed, Nov 24, 2010 at 09:32:02PM +0100, Michael Stefaniuc wrote:
> Hello Frederic,
>
> On 11/13/2010 10:37 PM, Frederic Weisbecker wrote:
> >When a single step exception fires, the trap bits, used to
> >signal hardware breakpoints, are in a random state.
> >
> >These trap bits might be set if another exception will follow,
> >like a breakpoint in the next instruction, or a watchpoint in the
> >previous one. Or there can be any junk there.
> >
> >So if we handle these trap bits during the single step exception,
> >we are going to handle an exception twice, or we are going to
> >handle junk.
> >
> >Just ignore them in this case.
> >
> >This fixes https://bugzilla.kernel.org/show_bug.cgi?id=21332
> sorry for the delay in testing this.
>
> I have cherry-picked this patch on top of v2.6.37-rc3-102-gea49b16
> and the ntdll/exception tests pass now.
>
> Many thanks
> bye
> michael
Thanks for testing!