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 | 53 +++++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/ftrace.c | 4 ----
3 files changed, 53 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 71eeef6..fd34c43 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -39,10 +39,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);
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 4f9895f..c54d5db 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -1116,6 +1116,7 @@ ftrace_call:
popl %edx
popl %ecx
popl %eax
+ftrace_ret:
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
.globl ftrace_graph_call
ftrace_graph_call:
@@ -1127,6 +1128,58 @@ ftrace_stub:
ret
END(ftrace_caller)
+ENTRY(ftrace_regs_caller)
+ pushf /* push flags before compare */
+ cmpl $0, function_trace_stop
+ jne ftrace_exit
+
+ subl $8, %esp /* skip ip and 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 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) /* Store CS */
+
+ movl 0x4(%ebp), %edx
+ lea 4(%esp), %ecx
+ pushl %ecx /* Save pt_regs as 4th parameter */
+ leal function_trace_op, %ecx
+
+GLOBAL(ftrace_regs_call)
+ call ftrace_stub
+
+ addl $4,%esp /* Skip pt_regs */
+ popl %eax
+ movl %eax, 14*4(%esp) /* Restore return address */
+ 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
+ftrace_exit:
+ addl $4, %esp /* Skip eflags */
+ jmp ftrace_ret
#else /* ! CONFIG_DYNAMIC_FTRACE */
ENTRY(mcount)
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 54cfc66..cf4de56 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -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 */
int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
unsigned long addr)
@@ -220,7 +219,6 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
return ftrace_modify_code(rec->ip, old, new);
}
-#endif
int ftrace_update_ftrace_func(ftrace_func_t func)
{
@@ -236,7 +234,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 version */
if (!ret) {
ip = (unsigned long)(&ftrace_regs_call);
@@ -244,7 +241,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);
--
1.7.10
On Tue, 2012-06-05 at 23:51 -0400, Steven Rostedt wrote:
> +ENTRY(ftrace_regs_caller)
> + pushf /* push flags before compare */
> + cmpl $0, function_trace_stop
> + jne ftrace_exit
> +
> +
Masami,
Do we really need to push before the compare? As the compare flags are
really meaningless with calling functions, and here we are only trying
to hide what the cmpl did. If something else was tracing without regs,
and we put a probe just after the nop, then it would include the cmpl
changes. My version of the patch doesn't restore the flags, so two
probes would have different values. But again, do we care? What would
need to know the value of cmp flags when calling into a function when
they are not going to be restored anyway.
-- Steve
(2012/06/06 23:37), Steven Rostedt wrote:
> On Tue, 2012-06-05 at 23:51 -0400, Steven Rostedt wrote:
>
>> +ENTRY(ftrace_regs_caller)
>> + pushf /* push flags before compare */
>> + cmpl $0, function_trace_stop
>> + jne ftrace_exit
>> +
>> +
>
> Masami,
>
> Do we really need to push before the compare? As the compare flags are
> really meaningless with calling functions, and here we are only trying
> to hide what the cmpl did. If something else was tracing without regs,
> and we put a probe just after the nop, then it would include the cmpl
> changes. My version of the patch doesn't restore the flags, so two
> probes would have different values. But again, do we care? What would
> need to know the value of cmp flags when calling into a function when
> they are not going to be restored anyway.
Yes, it needs to be saved and restored too, for transparency.
If we don't guarantee it, users must check whether a probe
can do what they are doing, before they put the probe. And
if not, they must find another appropriate place. It's not
compatible with previous kprobes.
Actually, I think we can push flags after compare if we
add a note on kprobes document, so that user can expect
compare flags will be not correct if KPROBE_FLAG_FTRACE
is set. (Of course, it is better to provide an API
(ftrace_location is enough?) for giving him a hint how he
can get a correct flags with kprobes)
But if you'd like to introduce -mfentry, I hope ftrace to
restore flags, which will be useful for debugging/investigation
by tweaking flags while executing a function.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]