On x86_64, rbp isn't saved on entering interrupt handler even when
CONFIG_FRAME_POINTER is turned on. This breaks profile_pc()
(resulting in oops) which uses regs->rbp to track back to the original
stack. Save full stack when CONFIG_FRAME_POINTER is specified.
--
tejun
On Sun, 12 Sep 2004 18:16:28 +0900
Tejun Heo <[email protected]> wrote:
> On x86_64, rbp isn't saved on entering interrupt handler even when
> CONFIG_FRAME_POINTER is turned on. This breaks profile_pc()
> (resulting in oops) which uses regs->rbp to track back to the original
> stack. Save full stack when CONFIG_FRAME_POINTER is specified.
I don't think your patch is correct, you don't restore rbp ever and it gets corrupted.
I think the correct change is to fix profile_pc() to not reference rbp, but just hardcode
the rsp offset for the FP and non FP cases (8 and 0)
-Andi
Hi,
On Sun, Sep 12, 2004 at 01:24:54PM +0200, Andi Kleen wrote:
> I don't think your patch is correct, you don't restore rbp ever and
> it gets corrupted.
R15 R14 R13 R12 RBP RBX are callee-saved and they aren't
saved/restored during kernel entry unless they are explicitly needed
(ptrace, etc..). interrupt macro which I modified is used only for
IRQ handling and apic interrupts, both of which aren't supposed to
modify any of above registers.
So, in the original code, when CONFIG_DEBUG is off only caller-saved
registers are saved and restored, and when the option is on, SAVE_ALL
is used but it's saved only to link back to the original stack such
that the debugger can track back into the previous frame. The current
code contains a line to restore rbp in ret_from_interrupt (other
calle-saved registers are not restored), but I don't think that line
is necessary. Unless somebody explicitly changes regs->rbp, the value
would be always the same, and if somebody is allowed to modify the
contents of regs when CONFIG_DEBUG is turned on, we should be
restoring all the callee-saved registers not just rbp.
> I think the correct change is to fix profile_pc() to not reference
> rbp,
I thought about that, but CONFIG_FRAME_POINTER is a debug feature
anyway, and it seemed reasonable to allow frame linking on interrupts
when the option turned on (x86 also supports back linking when
CONFIG_FRAME_POINTER is turned on).
> but just hardcode the rsp offset for the FP and non FP cases (8
> and 0)
You lost me here. regs->rbp is used in profile_pc() if the interrupt
occurs while the kernel is running spinlock codes. To attribute ticks
spent during spinlock operations to the locking/unlocking function,
profile_pc() reads the return value of the frame which was running
before the interrupt occurs. Are you saying that we can track back
into the previous frame without saving rbp? We can read the next
frame's(do_IRQ's) rbp storage area, but that doesn't seem to be a very
good idea to me.
--
tejun
On Sun, 12 Sep 2004, Andi Kleen wrote:
> On Sun, 12 Sep 2004 18:16:28 +0900
> Tejun Heo <[email protected]> wrote:
>
> > On x86_64, rbp isn't saved on entering interrupt handler even when
> > CONFIG_FRAME_POINTER is turned on. This breaks profile_pc()
> > (resulting in oops) which uses regs->rbp to track back to the original
> > stack. Save full stack when CONFIG_FRAME_POINTER is specified.
>
>
> I don't think your patch is correct, you don't restore rbp ever and it gets corrupted.
>
> I think the correct change is to fix profile_pc() to not reference rbp, but just hardcode
> the rsp offset for the FP and non FP cases (8 and 0)
Yep, i botched up the patch, after looking at the disassembly on
x86_64 without CONFIG_FRAME_POINTER again it's definitely incorrect. In
fact there are still a few users such as _spin_lock_irqsave which push
flags onto the stack and the stack pointer isn't consistent across all
functions in that text section. I'm going to have to try Andi's previous
suggestions.
Thanks,
Zwane
On Sun, Sep 12, 2004 at 01:10:26PM -0400, Zwane Mwaikambo wrote:
> On Sun, 12 Sep 2004, Andi Kleen wrote:
> > On Sun, 12 Sep 2004 18:16:28 +0900
> > Tejun Heo <[email protected]> wrote:
> >
> > > On x86_64, rbp isn't saved on entering interrupt handler even when
> > > CONFIG_FRAME_POINTER is turned on. This breaks profile_pc()
> > > (resulting in oops) which uses regs->rbp to track back to the original
> > > stack. Save full stack when CONFIG_FRAME_POINTER is specified.
> >
> >
> > I don't think your patch is correct, you don't restore rbp ever and it gets corrupted.
> >
> > I think the correct change is to fix profile_pc() to not reference rbp, but just hardcode
> > the rsp offset for the FP and non FP cases (8 and 0)
>
> Yep, i botched up the patch, after looking at the disassembly on
> x86_64 without CONFIG_FRAME_POINTER again it's definitely incorrect. In
> fact there are still a few users such as _spin_lock_irqsave which push
> flags onto the stack and the stack pointer isn't consistent across all
> functions in that text section. I'm going to have to try Andi's previous
> suggestions.
I'm sorry but I guess I'm slow today. :-( Can you please be kind
enough to lighten me up on how things get corrupted? I've read the
assembly source and disassembly of the output but I don't really see
how it'll get corrupted.
--
tejun