Re: [RFC][PATCH 2/4 v4] ftrace/x86: Add save_regs for i386 function calls

[Steven Rostedt <rostedt@goodmis.org>]

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 '&regs->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)(&regs->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