[PATCH] x86: push old stack address on irqstack for unwinder

[PATCH] x86: push old stack address on irqstack for unwinder

[Martin Hicks]

Hi,

KDB was using this information.  Could this be pushed towards 2.6.29 please?

This re-adds the old stack pointer to the top of the irqstack to help
with unwinding.  It was removed in commit d99015b1abbad743aa049b439c1e1dede6d0fa49
as part of the save_args out-of-line work.

Signed-off-by: Martin Hicks <mort@sgi.com>
---
 arch/x86/kernel/entry_64.S |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index e28c7a9..a134621 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -346,6 +346,7 @@ ENTRY(save_args)
 	popq_cfi %rax			/* move return address... */
 	mov %gs:pda_irqstackptr,%rsp
 	EMPTY_FRAME 0
+	pushq_cfi %rbp			/* backlink for unwinder */
 	pushq_cfi %rax			/* ... to the new stack */
 	/*
 	 * We entered an interrupt context - irqs are off:

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[H. Peter Anvin]

Martin Hicks wrote:
> Hi,
> 
> KDB was using this information.  Could this be pushed towards 2.6.29 please?
> 
> This re-adds the old stack pointer to the top of the irqstack to help
> with unwinding.  It was removed in commit d99015b1abbad743aa049b439c1e1dede6d0fa49
> as part of the save_args out-of-line work.
> 

This bothers me... why should we add even a single instruction to what 
is arguably the single hottest path in the kernel to support an 
out-of-tree debugger, especially if kgdb (which is in-tree) doesn't need it?

What does kgdb do differently (or is kgdb broken too)?

	-hpa

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[H. Peter Anvin]

H. Peter Anvin wrote:
> Martin Hicks wrote:
>> Hi,
>>
>> KDB was using this information.  Could this be pushed towards 2.6.29 
>> please?
>>
>> This re-adds the old stack pointer to the top of the irqstack to help
>> with unwinding.  It was removed in commit 
>> d99015b1abbad743aa049b439c1e1dede6d0fa49
>> as part of the save_args out-of-line work.
>>
> 
> This bothers me... why should we add even a single instruction to what 
> is arguably the single hottest path in the kernel to support an 
> out-of-tree debugger, especially if kgdb (which is in-tree) doesn't need 
> it?
> 
> What does kgdb do differently (or is kgdb broken too)?
> 

Thinking about it some more, I think this makes sense under

#ifdef CONFIG_FRAME_POINTER

... since if we're not building with frame pointers, this is pretty 
pointless, and if we are, we're adding these all over the place anyway.

Does this work for you?  Let me know and I'll get it in if so.

	-hpa

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> H. Peter Anvin wrote:
>> Martin Hicks wrote:
>>> Hi,
>>>
>>> KDB was using this information.  Could this be pushed towards 2.6.29  
>>> please?
>>>
>>> This re-adds the old stack pointer to the top of the irqstack to help
>>> with unwinding.  It was removed in commit  
>>> d99015b1abbad743aa049b439c1e1dede6d0fa49
>>> as part of the save_args out-of-line work.
>>>
>>
>> This bothers me... why should we add even a single instruction to what  
>> is arguably the single hottest path in the kernel to support an  
>> out-of-tree debugger, especially if kgdb (which is in-tree) doesn't 
>> need it?
>>
>> What does kgdb do differently (or is kgdb broken too)?
>>
>
> Thinking about it some more, I think this makes sense under
>
> #ifdef CONFIG_FRAME_POINTER
>
> ... since if we're not building with frame pointers, this is pretty  
> pointless, and if we are, we're adding these all over the place anyway.
>
> Does this work for you?  Let me know and I'll get it in if so.

Would be nice to have an #ifdef-less primitive for this - something like:

	pushq_frame %rbp

and a matching:

	popq_frame %rbp

for those cases that need it (this one doesnt as we dont pop out of the 
stack).

	Ingo

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> Would be nice to have an #ifdef-less primitive for this - something like:
> 
> 	pushq_frame %rbp
> 
> and a matching:
> 
> 	popq_frame %rbp
> 
> for those cases that need it (this one doesnt as we dont pop out of the 
> stack).
> 

It certainly would if this isn't a singleton, which I think it could 
possibly be?

Otherwise it really should be a part of an entry/exit macro; this is 
somewhat special in that it sets up a frame pointer as something other 
than a normal entry/exit sequence.

	-hpa

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Ingo Molnar wrote:
>>
>> Would be nice to have an #ifdef-less primitive for this - something like:
>>
>> 	pushq_frame %rbp
>>
>> and a matching:
>>
>> 	popq_frame %rbp
>>
>> for those cases that need it (this one doesnt as we dont pop out of the 
>> stack).
>>
>
> It certainly would if this isn't a singleton, which I think it could 
> possibly be?

yeah. This is pretty much the only non-restored frame we construct so 
indeed it would be a singleton. Perhaps the IST ones are such ones too.

> Otherwise it really should be a part of an entry/exit macro; this is 
> somewhat special in that it sets up a frame pointer as something other 
> than a normal entry/exit sequence.

Sure - your call really.

	Ingo

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[Martin Hicks]

On Sat, Jan 31, 2009 at 01:39:21AM +0100, Ingo Molnar wrote:
> 
> * H. Peter Anvin <hpa@zytor.com> wrote:
> 
> > H. Peter Anvin wrote:
> >> Martin Hicks wrote:
> >>> Hi,
> >>>
> >>> KDB was using this information.  Could this be pushed towards 2.6.29  
> >>> please?
> >>>
> >>> This re-adds the old stack pointer to the top of the irqstack to help
> >>> with unwinding.  It was removed in commit  
> >>> d99015b1abbad743aa049b439c1e1dede6d0fa49
> >>> as part of the save_args out-of-line work.
> >>>
> >>
> >> This bothers me... why should we add even a single instruction to what  
> >> is arguably the single hottest path in the kernel to support an  
> >> out-of-tree debugger, especially if kgdb (which is in-tree) doesn't 
> >> need it?
> >>
> >> What does kgdb do differently (or is kgdb broken too)?
> >>

I was searching around, trying to find out if there was another way for
kdb to do this, and I think removing the backlink is breaking other
stuff also.  dump_trace() in dumpstack_64.S is using the same trick as
KDB to trace out of the interrupt stack:

            /*
             * We link to the next stack (which would be
             * the process stack normally) the last
             * pointer (index -1 to end) in the IRQ stack:
             */
            stack = (unsigned long *) (irqstack_end[-1]);
            irqstack_end = NULL;
            ops->stack(data, "EOI");
            continue;


mh

Re: [PATCH] x86: push old stack address on irqstack for unwinder

[H. Peter Anvin]

Martin Hicks wrote:
> 
> I was searching around, trying to find out if there was another way for
> kdb to do this, and I think removing the backlink is breaking other
> stuff also.  dump_trace() in dumpstack_64.S is using the same trick as
> KDB to trace out of the interrupt stack:
> 

OK, that makes it a no-brainer.  Applied to tip:x86/urgent, thanks!

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.