ftrace/kprobes: Setting up ftrace for kprobes

From: Steven Rostedt <[email protected]>

Add a way to have different functions calling different trampolines. If a ftrace_ops wants regs saved on the return, then have only the functions with ops registered to save regs. Functions registered by other ops would not be affected, unless the functions overlap.

If one ftrace_ops registered functions A, B and C and another ops registered fucntions to save regs on A, and D, then only functions A and D would be saving regs. Function B and C would work as normal. Although A is registered by both ops: normal and saves regs; this is fine as saving the regs is needed to satisfy one of the ops that calls it but the regs are ignored by the other ops function.

x86_64 implements the full regs saving, and i386 just passes a NULL for regs to satisfy the ftrace_ops passing. Where an arch must supply both regs and ftrace_ops parameters, even if regs is just NULL.

It is OK for an arch to pass NULL regs. All function trace users that require regs passing must add the flag FTRACE_OPS_FL_SAVE_REGS when registering the ftrace_ops. If the arch does not support saving regs then the ftrace_ops will fail to register. The flag FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED may be set that will prevent the ftrace_ops from failing to register. In this case, the handler may either check if regs is not NULL or check if ARCH_SUPPORTS_FTRACE_SAVE_REGS. If the arch supports passing regs it will set this macro and pass regs for ops that request them. All other archs will just pass NULL.

Cc Alexander van Heukelum <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---

From: Steven Rostedt <[email protected]>

Add saving full regs for function tracing on i386. The saving of regs was influenced by patches sent out by Masami Hiramatsu.

Cc Masami Hiramatsu <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    2 --
 arch/x86/kernel/entry_32.S    |   58 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/ftrace.c      |    4 ---
 3 files changed, 58 insertions(+), 6 deletions(-)

arch/x86/include/asm/ftrace.h +0 -2

index a847501..a6cae0c 100644
@@ -40,10 +40,8 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#ifdef CONFIG_X86_64
 #define ARCH_SUPPORTS_FTRACE_SAVE_REGS
 #endif
-#endif
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);

arch/x86/kernel/entry_32.S +4 -0

index 5da11d1..8ef138f 100644
@@ -1123,6 +1123,7 @@ ftrace_call:
 	popl %edx
 	popl %ecx
 	popl %eax
+ftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
 ftrace_graph_call:

@@ -1134,6 +1135,63 @@ ftrace_stub:
 	ret
 END(ftrace_caller)
 
+ENTRY(ftrace_regs_caller)
+	pushf	/* push flags before compare (in ss location) */
+	cmpl $0, function_trace_stop

Quoted 4 lines

arch/x86/kernel/entry_32.S +3 -0

+ENTRY(ftrace_regs_caller)
+	pushf	/* push flags before compare (in ss location) */
+	cmpl $0, function_trace_stop

arch/x86/kernel/entry_32.S ftrace_stub: · line 1150

Quoted 10 lines

arch/x86/kernel/entry_32.S +10 -0

+	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

}
---

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;

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.

Thank you,

/*
* 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
}

---

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;

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.

Quoted 9 lines · Masami Hiramatsu

/*
* 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) {

Quoted 6 lines

#ifdef CONFIG_X86_32
        return (unsigned long)(&regs->sp);
#else
        return regs->sp;

#endif
}

I found that regs_get_register() doesn't honor this either. Thus, kprobes in tracing gets this:

 # echo 'p:ftrace sys_read+4 s=%sp' > /debug/tracing/kprobe_events
 # echo 1 > /debug/tracing/events/kprobes/enable
 # cat trace
            sshd-1345  [000] d...   489.117168: ftrace: (sys_read+0x4/0x70) s=b7e96768
            sshd-1345  [000] d...   489.117191: ftrace: (sys_read+0x4/0x70) s=b7e96768
             cat-1447  [000] d...   489.117392: ftrace: (sys_read+0x4/0x70) s=5a7
             cat-1447  [001] d...   489.118023: ftrace: (sys_read+0x4/0x70) s=b77ad05f
            less-1448  [000] d...   489.118079: ftrace: (sys_read+0x4/0x70) s=b7762e06
            less-1448  [000] d...   489.118117: ftrace: (sys_read+0x4/0x70) s=b7764970

Quoted 13 lines

I found that regs_get_register() doesn't honor this either. Thus, kprobes in tracing gets this:

 # echo 'p:ftrace sys_read+4 s=%sp' > /debug/tracing/kprobe_events
 # echo 1 > /debug/tracing/events/kprobes/enable
 # cat trace
            sshd-1345  [000] d...   489.117168: ftrace: (sys_read+0x4/0x70) s=b7e96768
            sshd-1345  [000] d...   489.117191: ftrace: (sys_read+0x4/0x70) s=b7e96768
             cat-1447  [000] d...   489.117392: ftrace: (sys_read+0x4/0x70) s=5a7
             cat-1447  [001] d...   489.118023: ftrace: (sys_read+0x4/0x70) s=b77ad05f
            less-1448  [000] d...   489.118079: ftrace: (sys_read+0x4/0x70) s=b7762e06
            less-1448  [000] d...   489.118117: ftrace: (sys_read+0x4/0x70) s=b7764970

Yes, that is by design, since I made it so. :) Instead of %sp, kprobe tracer provides $stack special argument for stack address, because "sp" is not always means the stack address on every arch.

Quoted 14 lines · Masami Hiramatsu

Quoted 13 lines

I found that regs_get_register() doesn't honor this either. Thus, kprobes in tracing gets this:

 # echo 'p:ftrace sys_read+4 s=%sp' > /debug/tracing/kprobe_events
 # echo 1 > /debug/tracing/events/kprobes/enable
 # cat trace
            sshd-1345  [000] d...   489.117168: ftrace: (sys_read+0x4/0x70) s=b7e96768
            sshd-1345  [000] d...   489.117191: ftrace: (sys_read+0x4/0x70) s=b7e96768
             cat-1447  [000] d...   489.117392: ftrace: (sys_read+0x4/0x70) s=5a7
             cat-1447  [001] d...   489.118023: ftrace: (sys_read+0x4/0x70) s=b77ad05f
            less-1448  [000] d...   489.118079: ftrace: (sys_read+0x4/0x70) s=b7762e06
            less-1448  [000] d...   489.118117: ftrace: (sys_read+0x4/0x70) s=b7764970

Quoted 4 lines

Yes, that is by design, since I made it so. :) Instead of %sp, kprobe tracer provides $stack special argument for stack address, because "sp" is not always means the stack address on every arch.

But is that useful? Wouldn't the actual stack pointer be more informative?

From: Steven Rostedt <[email protected]>
Date: Tue, 5 Jun 2012 20:00:11 -0400
Subject: [PATCH] ftrace/x86: Add save_regs for i386 function calls

Add saving full regs for function tracing on i386. The saving of regs was influenced by patches sent out by Masami Hiramatsu.

Cc Masami Hiramatsu <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    2 -
 arch/x86/kernel/entry_32.S    |   68 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/ftrace.c      |    4 --
 3 files changed, 68 insertions(+), 6 deletions(-)

arch/x86/include/asm/ftrace.h +0 -2

index a847501..a6cae0c 100644
@@ -40,10 +40,8 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#ifdef CONFIG_X86_64
 #define ARCH_SUPPORTS_FTRACE_SAVE_REGS
 #endif
-#endif
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);

arch/x86/kernel/entry_32.S +68 -0

index 5da11d1..46caa56 100644
@@ -1123,6 +1123,7 @@ ftrace_call:
 	popl %edx
 	popl %ecx
 	popl %eax
+ftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
 ftrace_graph_call:

@@ -1134,6 +1135,73 @@ ftrace_stub:
 	ret
 END(ftrace_caller)
 
+ENTRY(ftrace_regs_caller)
+	pushf	/* push flags before compare (in cs location) */
+	cmpl $0, function_trace_stop
+	jne ftrace_restore_flags
+
+	/*
+	 * i386 does not save SS and ESP when coming from kernel.
+	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
+	 * Unfortunately, that means eflags must be at the same location
+	 * as the current return ip is. We move the return ip into the
+	 * ip location, and move flags into the return ip location.
+	 */
+	pushl 4(%esp)	/* save return ip into ip slot */
+	subl $MCOUNT_INSN_SIZE, (%esp)	/* Adjust ip */
+
+	pushl $0	/* Load 0 into orig_ax */
+	pushl %gs
+	pushl %fs
+	pushl %es
+	pushl %ds
+	pushl %eax
+	pushl %ebp
+	pushl %edi
+	pushl %esi
+	pushl %edx
+	pushl %ecx
+	pushl %ebx
+
+	movl 13*4(%esp), %eax	/* Get the saved flags */
+	movl %eax, 14*4(%esp)	/* Move saved flags into regs->flags location */
+				/* clobbering return ip */
+	movl $__KERNEL_CS,13*4(%esp)
+
+	movl 12*4(%esp), %eax	/* Load ip (1st parameter) */
+	movl 0x4(%ebp), %edx	/* Load parent ip (2cd parameter) */
+	lea  (%esp), %ecx
+	pushl %ecx		/* Save pt_regs as 4th parameter */
+	leal function_trace_op, %ecx /* Save ftrace_pos in 3rd parameter */
+
+GLOBAL(ftrace_regs_call)
+	call ftrace_stub
+
+	addl $4, %esp		/* Skip pt_regs */
+	movl 14*4(%esp), %eax	/* Move flags back into cs */
+	movl %eax, 13*4(%esp)	/* Needed to keep addl from modifying flags */
+	movl 12*4(%esp), %eax	/* Get return ip from regs->ip */
+	addl $MCOUNT_INSN_SIZE, %eax
+	movl %eax, 14*4(%esp)	/* Put return ip back for ret */
+
+	popl %ebx
+	popl %ecx
+	popl %edx
+	popl %esi
+	popl %edi
+	popl %ebp
+	popl %eax
+	popl %ds
+	popl %es
+	popl %fs
+	popl %gs
+	addl $8, %esp		/* Skip orig_ax and ip */
+	popf			/* Pop flags at end (no addl to corrupt flags) */
+	jmp ftrace_ret
+
+ftrace_restore_flags:
+	popf
+	jmp  ftrace_stub
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)

arch/x86/kernel/ftrace.c +0 -4

index b90eb1a..1d41402 100644
@@ -206,7 +206,6 @@ static int
 ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 		   unsigned const char *new_code);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
 /*
  * Should never be called:
  *  As it is only called by __ftrace_replace_code() which is called by

@@ -221,7 +220,6 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 	WARN_ON(1);
 	return -EINVAL;
 }
-#endif
 
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {

@@ -237,7 +235,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	ret = ftrace_modify_code(ip, old, new);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
 	/* Also update the regs callback function */
 	if (!ret) {
 		ip = (unsigned long)(&ftrace_regs_call);

@@ -245,7 +242,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 		new = ftrace_call_replace(ip, (unsigned long)func);
 		ret = ftrace_modify_code(ip, old, new);
 	}
-#endif
 
 	atomic_dec(&modifying_ftrace_code);
 

On Fri, 2012-07-13 at 14:47 -0400, Steven Rostedt wrote:

Masami, can you give your Reviewed-by tag for this version? Or is there something else needing to be fixed?

Thanks!

-- Steve

Quoted 146 lines

From: Steven Rostedt <[email protected]>
Date: Tue, 5 Jun 2012 20:00:11 -0400
Subject: [PATCH] ftrace/x86: Add save_regs for i386 function calls

Add saving full regs for function tracing on i386. The saving of regs was influenced by patches sent out by Masami Hiramatsu.

Cc Masami Hiramatsu <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/include/asm/ftrace.h |    2 -
 arch/x86/kernel/entry_32.S    |   68 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/ftrace.c      |    4 --
 3 files changed, 68 insertions(+), 6 deletions(-)

arch/x86/include/asm/ftrace.h +0 -2

index a847501..a6cae0c 100644
@@ -40,10 +40,8 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#ifdef CONFIG_X86_64
 #define ARCH_SUPPORTS_FTRACE_SAVE_REGS
 #endif
-#endif
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);

arch/x86/kernel/entry_32.S +68 -0

index 5da11d1..46caa56 100644
@@ -1123,6 +1123,7 @@ ftrace_call:
 	popl %edx
 	popl %ecx
 	popl %eax
+ftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
 ftrace_graph_call:

@@ -1134,6 +1135,73 @@ ftrace_stub:
 	ret
 END(ftrace_caller)
 
+ENTRY(ftrace_regs_caller)
+	pushf	/* push flags before compare (in cs location) */
+	cmpl $0, function_trace_stop
+	jne ftrace_restore_flags
+
+	/*
+	 * i386 does not save SS and ESP when coming from kernel.
+	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
+	 * Unfortunately, that means eflags must be at the same location
+	 * as the current return ip is. We move the return ip into the
+	 * ip location, and move flags into the return ip location.
+	 */
+	pushl 4(%esp)	/* save return ip into ip slot */
+	subl $MCOUNT_INSN_SIZE, (%esp)	/* Adjust ip */
+
+	pushl $0	/* Load 0 into orig_ax */
+	pushl %gs
+	pushl %fs
+	pushl %es
+	pushl %ds
+	pushl %eax
+	pushl %ebp
+	pushl %edi
+	pushl %esi
+	pushl %edx
+	pushl %ecx
+	pushl %ebx
+
+	movl 13*4(%esp), %eax	/* Get the saved flags */
+	movl %eax, 14*4(%esp)	/* Move saved flags into regs->flags location */
+				/* clobbering return ip */
+	movl $__KERNEL_CS,13*4(%esp)
+
+	movl 12*4(%esp), %eax	/* Load ip (1st parameter) */
+	movl 0x4(%ebp), %edx	/* Load parent ip (2cd parameter) */
+	lea  (%esp), %ecx
+	pushl %ecx		/* Save pt_regs as 4th parameter */
+	leal function_trace_op, %ecx /* Save ftrace_pos in 3rd parameter */
+
+GLOBAL(ftrace_regs_call)
+	call ftrace_stub
+
+	addl $4, %esp		/* Skip pt_regs */
+	movl 14*4(%esp), %eax	/* Move flags back into cs */
+	movl %eax, 13*4(%esp)	/* Needed to keep addl from modifying flags */
+	movl 12*4(%esp), %eax	/* Get return ip from regs->ip */
+	addl $MCOUNT_INSN_SIZE, %eax
+	movl %eax, 14*4(%esp)	/* Put return ip back for ret */
+
+	popl %ebx
+	popl %ecx
+	popl %edx
+	popl %esi
+	popl %edi
+	popl %ebp
+	popl %eax
+	popl %ds
+	popl %es
+	popl %fs
+	popl %gs
+	addl $8, %esp		/* Skip orig_ax and ip */
+	popf			/* Pop flags at end (no addl to corrupt flags) */
+	jmp ftrace_ret
+
+ftrace_restore_flags:
+	popf
+	jmp  ftrace_stub
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)

arch/x86/kernel/ftrace.c +0 -3

index b90eb1a..1d41402 100644
@@ -206,7 +206,6 @@ static int
 ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 		   unsigned const char *new_code);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
 /*
  * Should never be called:
  *  As it is only called by __ftrace_replace_code() which is called by

@@ -221,7 +220,6 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 	WARN_ON(1);
 	return -EINVAL;
 }
-#endif
 
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {

@@ -237,7 +235,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	ret = ftrace_modify_code(ip, old, new);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
 	/* Also update the regs callback function */
 	if (!ret) {
 		ip = (unsigned long)(&ftrace_regs_call);

arch/x86/kernel/ftrace.c int ftrace_update_ftrace_func(ftrace_func_t func) · line 247

Quoted 7 lines

arch/x86/kernel/ftrace.c +0 -1

@@ -245,7 +242,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 		new = ftrace_call_replace(ip, (unsigned long)func);
 		ret = ftrace_modify_code(ip, old, new);
 	}
-#endif
 
 	atomic_dec(&modifying_ftrace_code);
 

--
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/

Quoted 4 lines · Steven Rostedt

On Fri, 2012-07-13 at 14:47 -0400, Steven Rostedt wrote:

Masami, can you give your Reviewed-by tag for this version? Or is there something else needing to be fixed?

No, that is OK for me. I've just missed that...

Reviewed-by Masami Hiramatsu <[email protected]>

Thank you!

GLOBAL(ftrace_regs_call)

arch/x86/kernel/entry_32.S +2 -2

 	call ftrace_stub

@@ -1195,8 +1194,8 @@ GLOBAL(ftrace_regs_call)
 	popl %es
 	popl %fs
 	popl %gs
-	addl $8, %esp		/* Skip orig_ax and ip */
-	popf			/* Pop flags at end (no addl to corrupt flags) */
+	lea 8(%esp), %esp	/* Skip orig_ax and ip */
+	popf			/* Pop flags at end */
 	jmp ftrace_ret
 
 ftrace_restore_flags:

Because we no longer have that 4 byte offset on the stack when we need to load the 4th parameter, we can just load the current stack pointer into the stack (pushl %esp), without the save to %ecx step.

also, because lea is faster than add (and doesn't even modify flags), I changed the last part to use lea instead of addl.

Can you give your reviewed-by tag for this too? I'd like to push this out today so we can still make 3.6.

Thanks!

-- Steve

here's the full patch:

arch/x86/include/asm/ftrace.h +0 -2

index a847501..a6cae0c 100644
@@ -40,10 +40,8 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#ifdef CONFIG_X86_64
 #define ARCH_SUPPORTS_FTRACE_SAVE_REGS
 #endif
-#endif
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);

arch/x86/kernel/entry_32.S +50 -0

index 5da11d1..ca5a146 100644
@@ -1123,6 +1123,7 @@ ftrace_call:
 	popl %edx
 	popl %ecx
 	popl %eax
+ftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
 ftrace_graph_call:

@@ -1134,6 +1135,72 @@ ftrace_stub:
 	ret
 END(ftrace_caller)
 
+ENTRY(ftrace_regs_caller)
+	pushf	/* push flags before compare (in cs location) */
+	cmpl $0, function_trace_stop
+	jne ftrace_restore_flags
+
+	/*
+	 * i386 does not save SS and ESP when coming from kernel.
+	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
+	 * Unfortunately, that means eflags must be at the same location
+	 * as the current return ip is. We move the return ip into the
+	 * ip location, and move flags into the return ip location.
+	 */
+	pushl 4(%esp)	/* save return ip into ip slot */
+	subl $MCOUNT_INSN_SIZE, (%esp)	/* Adjust ip */
+
+	pushl $0	/* Load 0 into orig_ax */
+	pushl %gs
+	pushl %fs
+	pushl %es
+	pushl %ds
+	pushl %eax
+	pushl %ebp
+	pushl %edi
+	pushl %esi
+	pushl %edx
+	pushl %ecx
+	pushl %ebx
+
+	movl 13*4(%esp), %eax	/* Get the saved flags */
+	movl %eax, 14*4(%esp)	/* Move saved flags into regs->flags location */
+				/* clobbering return ip */
+	movl $__KERNEL_CS,13*4(%esp)
+
+	movl 12*4(%esp), %eax	/* Load ip (1st parameter) */
+	movl 0x4(%ebp), %edx	/* Load parent ip (2nd parameter) */
+	leal function_trace_op, %ecx /* Save ftrace_pos in 3rd parameter */
+	pushl %esp		/* Save pt_regs as 4th parameter */
+
+GLOBAL(ftrace_regs_call)
+	call ftrace_stub
+
+	addl $4, %esp		/* Skip pt_regs */
+	movl 14*4(%esp), %eax	/* Move flags back into cs */
+	movl %eax, 13*4(%esp)	/* Needed to keep addl from modifying flags */
+	movl 12*4(%esp), %eax	/* Get return ip from regs->ip */
+	addl $MCOUNT_INSN_SIZE, %eax
+	movl %eax, 14*4(%esp)	/* Put return ip back for ret */
+
+	popl %ebx

arch/x86/kernel/entry_32.S +5 -0

+	jmp ftrace_ret
+
+ftrace_restore_flags:
+	popf
+	jmp  ftrace_stub
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)

arch/x86/kernel/ftrace.c +0 -4

index b90eb1a..1d41402 100644
@@ -206,7 +206,6 @@ static int
 ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 		   unsigned const char *new_code);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
 /*
  * Should never be called:
  *  As it is only called by __ftrace_replace_code() which is called by

@@ -221,7 +220,6 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 	WARN_ON(1);
 	return -EINVAL;
 }
-#endif
 
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {

@@ -237,7 +235,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	ret = ftrace_modify_code(ip, old, new);
 
-#ifdef ARCH_SUPPORTS_FTRACE_SAVE_REGS
 	/* Also update the regs callback function */
 	if (!ret) {
 		ip = (unsigned long)(&ftrace_regs_call);

@@ -245,7 +242,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 		new = ftrace_call_replace(ip, (unsigned long)func);
 		ret = ftrace_modify_code(ip, old, new);
 	}
-#endif
 
 	atomic_dec(&modifying_ftrace_code);
 

Quoted 3 lines

also, because lea is faster than add (and doesn't even modify flags), I changed the last part to use lea instead of addl.

Now I'm told that this is not always the case (at least not for Atom), so I reverted this part and put back the addl. But can you still give you reviewed by for the first part?

lea is not typically faster than add, but in the case of Atom, it is done in an earlier pipeline stage (AGU instead of ALU) which means lea is faster if its inputs are already available as address expressions and is consumed by address expressions; the goal is to avoid the ALU->AGU forwarding latency.

Quoted 5 lines · H. Peter Anvin

lea is not typically faster than add, but in the case of Atom, it is done in an earlier pipeline stage (AGU instead of ALU) which means lea is faster if its inputs are already available as address expressions and is consumed by address expressions; the goal is to avoid the ALU->AGU forwarding latency.

Well, the question is, which is faster:

	lea 8(%esp), %esp
	addl $8, %esp

Basically, all we want to do is add 8 to the stack pointer. And this is for the x86_32 version of whatever hardware is in use.

Quoted 3 lines · Steven Rostedt

Basically, all we want to do is add 8 to the stack pointer. And this is for the x86_32 version of whatever hardware is in use.

What I'm telling you is that it depends on the context.

Quoted 12 lines · Masami Hiramatsu

Quoted 4 lines · Steven Rostedt

On Fri, 2012-07-13 at 14:47 -0400, Steven Rostedt wrote:

Masami, can you give your Reviewed-by tag for this version? Or is there something else needing to be fixed?

No, that is OK for me. I've just missed that...

Reviewed-by Masami Hiramatsu <[email protected]>

Thank you!

Thanks! Except I have one more version. Someone offlist gave me some ideas.

Here's the diff from the previous patch.

arch/x86/kernel/entry_32.S +0 -0

index 46caa56..ca5a146 100644
@@ -1169,10 +1169,9 @@ ENTRY(ftrace_regs_caller)
 	movl $__KERNEL_CS,13*4(%esp)