I'm slowly getting this patch set into working order ;-)
On Thu, 2012-07-12 at 21:39 +0900, Masami Hiramatsu wrote:
>
> > +ENTRY(ftrace_regs_caller)
> > + pushf /* push flags before compare (in ss location) */
> > + cmpl $0, function_trace_stop
> > + jne ftrace_restore_flags
> > +
> > + pushl %esp /* Save stack in sp location */
> > + subl $4, (%esp) /* Adjust saved stack to skip saved flags */
> > + pushl 4(%esp) /* Save flags in correct position */
> > + movl $__KERNEL_DS, 8(%esp) /* Save ss */
> > + pushl $__KERNEL_CS
> > + pushl 4*4(%esp) /* Save the ip */
> > + subl $MCOUNT_INSN_SIZE, (%esp) /* Adjust ip */
> > + pushl $0 /* Load 0 into orig_ax */
>
> Oops, you might forget that the i386's interrupt stack layout is a bit
> different from x86-64.
>
> On x86-64, regs->sp directly points the top of stack.
> On the other hand (i386), regs->sp IS the top of stack. You can see
> below code in arch/x86/include/asm/ptrace.h
> ---
> /*
> * X86_32 CPUs don't save ss and esp if the CPU is already in kernel mode
> * when it traps. The previous stack will be directly underneath the saved
> * registers, and 'sp/ss' won't even have been saved. Thus the '®s->sp'.
> *
> * This is valid only for kernel mode traps.
> */
> static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
> {
> #ifdef CONFIG_X86_32
> return (unsigned long)(®s->sp);
> #else
> return regs->sp;
> #endif
Yuck yuck yuck!
> }
> ---
>
> This means that you need a trick here.
>
> sp-> [retaddr]
> (*)-> [orig_stack]
>
> Here is the stack layout when the ftrace_regs_caller is called.
> (*) points the original stack pointer. this means that regs->sp has
> placed at (*). After doing pushf, it changed as below.
>
> (what user expects)
> sp-> [flags] <- regs.cs
> [retaddr] <- regs.flags
> (*)-> [orig_stack] <- regs.sp
>
> So we have to change this stack layout as the user expected. That is
> what I did it in my previous series;
Yeah, I saw that you did this, but didn't fully understand why. I
completely forgot about that hack in x86_32 :-(
This is why I'm insisting to get your Reviewed-by, as you seem to be
more up-to-date on the subtleties between 32 and 64 than I am.
>
> https://lkml.org/lkml/2012/6/5/119
>
> In this patch, I clobbered the return address on the stack and
> stores it in the local stack because of that reason.
>
> + movl 14*4(%esp), %eax /* Load return address */
> + pushl %eax /* Save return address (+4) */
> + subl $MCOUNT_INSN_SIZE, %eax
> + movl %eax, 12*4+4(%esp) /* Store IP */
> + movl 13*4+4(%esp), %edx /* Load flags */
> + movl %edx, 14*4+4(%esp) /* Store flags */
> + movl $__KERNEL_CS, %edx
> + movl %edx, 13*4+4(%esp)
Well the change log does say that my patch set was influenced by your
code. I started to veer from that. I shouldn't have.
>
> Thank you,
>
No, thank you!
/me goes to work on v5.
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
