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 <[email protected]>
---
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:
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
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
* H. Peter Anvin <[email protected]> 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
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
* H. Peter Anvin <[email protected]> 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
On Sat, Jan 31, 2009 at 01:39:21AM +0100, Ingo Molnar wrote:
>
> * H. Peter Anvin <[email protected]> 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