[GIT PULL] kgdb regression fixes for 2.6.35-rc5

[GIT PULL] kgdb regression fixes for 2.6.35-rc5

[Jason Wessel]

Linus, please pull the for_linus branch for 2.6.35-rc6.  

git://git.kernel.org/pub/scm/linux/kernel/git/jwessel/linux-2.6-kgdb.git for_linus

The kgdb validation tests found a regression where HW breakpoints
stopped working with kdb / kgdb.  The problem was bisected and after
talking with Frederic Weisbecker a patch was generated to repair the
regression.

Thanks,
Jason.


---
Jason Wessel (1):
      x86,kgdb: Fix hw breakpoint regression

 arch/x86/kernel/kgdb.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

[PATCH] x86,kgdb: Fix hw breakpoint regression

[Jason Wessel]

HW breakpoints events stopped working correctly with kgdb
as a result of commit: 018cbffe6819f6f8db20a0a3acd9bab9bfd667e4
(Merge commit 'v2.6.33' into perf/core).

The regression occurred because the behavior changed for setting
NOTIFY_STOP as the return value to the die notifier if the breakpoint
was known to the HW breakpoint API.  Because kgdb is using the HW
breakpoint API to register HW breakpoints slots, it must also now
implement the overflow_handler call back else kgdb does not get to see
the events from the die notifier.

The kgdb_ll_trap function will be changed to be general purpose code
which can allow an easy way to implement the hw_breakpoint API
overflow call back.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
Acked-by: Dongdong Deng <dongdong.deng@windriver.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/x86/kernel/kgdb.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 4f4af75..4030ce5 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -572,10 +572,10 @@ static int __kgdb_notify(struct die_args *args, unsigned long cmd)
 	return NOTIFY_STOP;
 }
 
-#ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
 int kgdb_ll_trap(int cmd, const char *str,
 		 struct pt_regs *regs, long err, int trap, int sig)
 {
+	unsigned long flags;
 	struct die_args args = {
 		.regs	= regs,
 		.str	= str,
@@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
 	if (!kgdb_io_module_registered)
 		return NOTIFY_DONE;
 
+	local_irq_save(flags);
 	return __kgdb_notify(&args, cmd);
+	local_irq_restore(flags);
 }
-#endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
 
 static int
 kgdb_notify(struct notifier_block *self, unsigned long cmd, void *ptr)
@@ -625,6 +626,12 @@ int kgdb_arch_init(void)
 	return register_die_notifier(&kgdb_notifier);
 }
 
+static void kgdb_hw_overflow_handler(struct perf_event *event, int nmi,
+		struct perf_sample_data *data, struct pt_regs *regs)
+{
+	kgdb_ll_trap(DIE_DEBUG, "debug", regs, 0, 0, SIGTRAP);
+}
+
 void kgdb_arch_late(void)
 {
 	int i, cpu;
@@ -655,6 +662,7 @@ void kgdb_arch_late(void)
 		for_each_online_cpu(cpu) {
 			pevent = per_cpu_ptr(breakinfo[i].pev, cpu);
 			pevent[0]->hw.sample_period = 1;
+			pevent[0]->overflow_handler = kgdb_hw_overflow_handler;
 			if (pevent[0]->destroy != NULL) {
 				pevent[0]->destroy = NULL;
 				release_bp_slot(*pevent);
-- 
1.6.4.rc1

Re: [GIT PULL] kgdb regression fixes for 2.6.35-rc5

[Frederic Weisbecker]

On Wed, Jul 28, 2010 at 03:39:49PM -0500, Jason Wessel wrote:
> Linus, please pull the for_linus branch for 2.6.35-rc6.  
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/jwessel/linux-2.6-kgdb.git for_linus
> 
> The kgdb validation tests found a regression where HW breakpoints
> stopped working with kdb / kgdb.  The problem was bisected and after
> talking with Frederic Weisbecker a patch was generated to repair the
> regression.
> 
> Thanks,
> Jason.
> 
> 
> ---
> Jason Wessel (1):
>       x86,kgdb: Fix hw breakpoint regression
> 
>  arch/x86/kernel/kgdb.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)



I'm sorry I spot it a bit late.
There is a little issue in this patch, irqs won't
be restored:

@@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
        if (!kgdb_io_module_registered)
                return NOTIFY_DONE;
 
+       local_irq_save(flags);
        return __kgdb_notify(&args, cmd);
+       local_irq_restore(flags);
 }

Re: [GIT PULL] kgdb regression fixes for 2.6.35-rc5

[Linus Torvalds]

On Wed, Jul 28, 2010 at 2:17 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
> I'm sorry I spot it a bit late.
> There is a little issue in this patch, irqs won't
> be restored:
>
> @@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
>        if (!kgdb_io_module_registered)
>                return NOTIFY_DONE;
>
> +       local_irq_save(flags);
>        return __kgdb_notify(&args, cmd);
> +       local_irq_restore(flags);
>  }

Yeah, that's obviously crap. I also wonder why __kgdb_notify messes up
the flags to begin with.

             Linus

Re: [GIT PULL] kgdb regression fixes for 2.6.35-rc5

[Jason Wessel]

On 07/28/2010 04:26 PM, Linus Torvalds wrote:
> On Wed, Jul 28, 2010 at 2:17 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> I'm sorry I spot it a bit late.
>> There is a little issue in this patch, irqs won't
>> be restored:
>>
>> @@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
>>        if (!kgdb_io_module_registered)
>>                return NOTIFY_DONE;
>>
>> +       local_irq_save(flags);
>>        return __kgdb_notify(&args, cmd);
>> +       local_irq_restore(flags);
>>  }
> 
> Yeah, that's obviously crap. I also wonder why __kgdb_notify messes up
> the flags to begin with.
> 

This should not be needed as the irqs should already be off while in the overflow handler.

Sorry for the noise on the one.   I updated the for_linus branch with the final version.


 1 files changed, 7 insertions(+), 2 deletions(-)


--
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -572,7 +572,6 @@ static int __kgdb_notify(struct die_args *args, unsigned long cmd)
 	return NOTIFY_STOP;
 }
 
-#ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
 int kgdb_ll_trap(int cmd, const char *str,
 		 struct pt_regs *regs, long err, int trap, int sig)
 {
@@ -590,7 +589,6 @@ int kgdb_ll_trap(int cmd, const char *str,
 
 	return __kgdb_notify(&args, cmd);
 }
-#endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
 
 static int
 kgdb_notify(struct notifier_block *self, unsigned long cmd, void *ptr)
@@ -625,6 +623,12 @@ int kgdb_arch_init(void)
 	return register_die_notifier(&kgdb_notifier);
 }
 
+static void kgdb_hw_overflow_handler(struct perf_event *event, int nmi,
+		struct perf_sample_data *data, struct pt_regs *regs)
+{
+	kgdb_ll_trap(DIE_DEBUG, "debug", regs, 0, 0, SIGTRAP);
+}
+
 void kgdb_arch_late(void)
 {
 	int i, cpu;
@@ -655,6 +659,7 @@ void kgdb_arch_late(void)
 		for_each_online_cpu(cpu) {
 			pevent = per_cpu_ptr(breakinfo[i].pev, cpu);
 			pevent[0]->hw.sample_period = 1;
+			pevent[0]->overflow_handler = kgdb_hw_overflow_handler;
 			if (pevent[0]->destroy != NULL) {
 				pevent[0]->destroy = NULL;
 				release_bp_slot(*pevent);

Re: [GIT PULL] kgdb regression fixes for 2.6.35-rc5

[Frederic Weisbecker]

On Wed, Jul 28, 2010 at 07:14:42PM -0500, Jason Wessel wrote:
> On 07/28/2010 04:26 PM, Linus Torvalds wrote:
> > On Wed, Jul 28, 2010 at 2:17 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> I'm sorry I spot it a bit late.
> >> There is a little issue in this patch, irqs won't
> >> be restored:
> >>
> >> @@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
> >>        if (!kgdb_io_module_registered)
> >>                return NOTIFY_DONE;
> >>
> >> +       local_irq_save(flags);
> >>        return __kgdb_notify(&args, cmd);
> >> +       local_irq_restore(flags);
> >>  }
> > 
> > Yeah, that's obviously crap. I also wonder why __kgdb_notify messes up
> > the flags to begin with.
> > 
> 
> This should not be needed as the irqs should already be off while in the overflow handler.



Interrupts are not disabled in the debug exception handler.

I'm not sure what would be the state of dr6 if a nested debug exception
would happen while do_debug() is interrupted.

Perhaps the previous dr6 value is dropped. Intel manuals don't seem to say anything
about that.

Anyway that's a separate issue. The current patch fixes a regression and
it looks good.

Thanks.

Re: [Kgdb-bugreport] [GIT PULL] kgdb regression fixes for 2.6.35-rc5

[DDD]

Frederic Weisbecker wrote:
> On Wed, Jul 28, 2010 at 07:14:42PM -0500, Jason Wessel wrote:
>> On 07/28/2010 04:26 PM, Linus Torvalds wrote:
>>> On Wed, Jul 28, 2010 at 2:17 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>>> I'm sorry I spot it a bit late.
>>>> There is a little issue in this patch, irqs won't
>>>> be restored:
>>>>
>>>> @@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
>>>>        if (!kgdb_io_module_registered)
>>>>                return NOTIFY_DONE;
>>>>
>>>> +       local_irq_save(flags);
>>>>        return __kgdb_notify(&args, cmd);
>>>> +       local_irq_restore(flags);
>>>>  }
>>> Yeah, that's obviously crap. I also wonder why __kgdb_notify messes up
>>> the flags to begin with.
>>>
>> This should not be needed as the irqs should already be off while in the overflow handler.
> 
> 
> 
> Interrupts are not disabled in the debug exception handler.

Hi Frederic,

The debug was initialized as an intr gate, thus the interrupts have been 
disabled in the debug exception handler. :-)

arch/x86/kernel/traps.c:
820 /* Set of traps needed for early debugging. */
821 void __init early_trap_init(void)
822 {
823     set_intr_gate_ist(1, &debug, DEBUG_STACK);
824     /* int3 can be called from all */
825     set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
826     set_intr_gate(14, &page_fault);
827     load_idt(&idt_descr);
828 }

Thanks,
Dongdong

> 
> I'm not sure what would be the state of dr6 if a nested debug exception
> would happen while do_debug() is interrupted.
> 
> Perhaps the previous dr6 value is dropped. Intel manuals don't seem to say anything
> about that.
> 
> Anyway that's a separate issue. The current patch fixes a regression and
> it looks good.
> 
> Thanks.
> 
> 
> ------------------------------------------------------------------------------
> 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
> Kgdb-bugreport@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
> 

Re: [Kgdb-bugreport] [GIT PULL] kgdb regression fixes for 2.6.35-rc5

[Frederic Weisbecker]

On Thu, Jul 29, 2010 at 10:49:50AM +0800, DDD wrote:
> Frederic Weisbecker wrote:
>> On Wed, Jul 28, 2010 at 07:14:42PM -0500, Jason Wessel wrote:
>>> On 07/28/2010 04:26 PM, Linus Torvalds wrote:
>>>> On Wed, Jul 28, 2010 at 2:17 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>>>> I'm sorry I spot it a bit late.
>>>>> There is a little issue in this patch, irqs won't
>>>>> be restored:
>>>>>
>>>>> @@ -588,9 +588,10 @@ int kgdb_ll_trap(int cmd, const char *str,
>>>>>        if (!kgdb_io_module_registered)
>>>>>                return NOTIFY_DONE;
>>>>>
>>>>> +       local_irq_save(flags);
>>>>>        return __kgdb_notify(&args, cmd);
>>>>> +       local_irq_restore(flags);
>>>>>  }
>>>> Yeah, that's obviously crap. I also wonder why __kgdb_notify messes up
>>>> the flags to begin with.
>>>>
>>> This should not be needed as the irqs should already be off while in the overflow handler.
>>
>>
>>
>> Interrupts are not disabled in the debug exception handler.
>
> Hi Frederic,
>
> The debug was initialized as an intr gate, thus the interrupts have been  
> disabled in the debug exception handler. :-)



Ah ok. Thanks for the correction :)




> arch/x86/kernel/traps.c:
> 820 /* Set of traps needed for early debugging. */
> 821 void __init early_trap_init(void)
> 822 {
> 823     set_intr_gate_ist(1, &debug, DEBUG_STACK);
> 824     /* int3 can be called from all */
> 825     set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
> 826     set_intr_gate(14, &page_fault);
> 827     load_idt(&idt_descr);
> 828 }
>
> Thanks,
> Dongdong