Re: [PATCH 3/5][RFC] ftrace: Return pt_regs to function trace callback (x86_64 only so

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2011/08/11 1:22), Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> Return as the 4th paramater to the function tracer callback the pt_regs.
>
> So far this is only supported by x86_64. The ftrace_ops flag
> FTRACE_OPS_FL_SAVE_REGS is added to tell the arch to save all regs
> to the pt_regs, otherwise a minimum is just passed back (the same
> regs that is saved by mcount itself).

I guess it will be a bit hard to port this on x86-32, because
on x86-32, the top of stack address in pt_regs is the address
of sp member (e.g. &(pt_regs->sp)). I mean that when mcount-entry
calls ftrace_caller, it pushes an address of the next instruction
of mcount-entry on the top of stack.
In that case, &(pt_regs->sp) points the entry which stores the
address, instead of the return address of probed function.

e.g. with kprobes (on x86-32):

[  <bx>  ] <- pt_regs
[  ...   ]
[  <cs>  ]
[<flags> ]
[ret-addr] <- &(pt_regs.sp)
[  arg1  ]
[  arg2  ]

with this method:

[  <bx>  ] <- pt_regs
[  ...   ]
[  <cs>  ]
[<flags> ]
[mcount-ret] <- &(pt_regs.sp)
[ret-addr]
[  arg1  ]
[  arg2  ]

I think this is hard to solve without a tricky hack.

For example, on x86-32, MCOUNT_FRAME_SAVE saves
flags on the entry which will be <cs> and it saves
mcount-ret to local stack and moves flags to next entry.

<save-frame>
pushf			# save flags on CS(%esp)
subl $12, %esp		# skip ip, orig_ax and gs
pushl %fs
pushl %es
...
pushl %ebx
movl 56(%esp), %ebx	# load mcount-ret address
movl 52(%esp), %ebp	# load flags
movl %ebp, 56(%esp)	# store flags

call function (ebx is callee save)

<restore-frame>
movl 56(%esp), %ebp	# load flags
movl %ebp, 52(%esp)	# store flags
movl %ebx, 56(%esp)	# load mcount-ret address
...
popf
ret

Hmm?

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  arch/x86/include/asm/ftrace.h     |   38 ++++++++++++++++++++----------------
>  arch/x86/kernel/entry_64.S        |   23 +++++++++++++++++++++-
>  include/linux/ftrace.h            |   15 ++++++++++++-
>  kernel/trace/ftrace.c             |   29 ++++++++++++++++++---------
>  kernel/trace/trace_events.c       |    2 +-
>  kernel/trace/trace_functions.c    |    7 +++--
>  kernel/trace/trace_irqsoff.c      |    2 +-
>  kernel/trace/trace_sched_wakeup.c |    3 +-
>  kernel/trace/trace_selftest.c     |   15 +++++++++----
>  kernel/trace/trace_stack.c        |    3 +-
>  10 files changed, 95 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index b3fcf16..0750c2a 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -4,26 +4,29 @@
>  #ifdef __ASSEMBLY__
>
>  	.macro MCOUNT_SAVE_FRAME
> -	/* taken from glibc */
> -	subq $0x38, %rsp
> -	movq %rax, (%rsp)
> -	movq %rcx, 8(%rsp)
> -	movq %rdx, 16(%rsp)
> -	movq %rsi, 24(%rsp)
> -	movq %rdi, 32(%rsp)
> -	movq %r8, 40(%rsp)
> -	movq %r9, 48(%rsp)
> +	 /*
> +	  * We add enough stack to save all regs,
> +	  * and we what we need in the location of pt_regs.
> +	  */
> +	subq $ORIG_RAX, %rsp
> +	movq %rax, RAX(%rsp)
> +	movq %rcx, RCX(%rsp)
> +	movq %rdx, RDX(%rsp)
> +	movq %rsi, RSI(%rsp)
> +	movq %rdi, RDI(%rsp)
> +	movq %r8, R8(%rsp)
> +	movq %r9, R9(%rsp)
>  	.endm
>
>  	.macro MCOUNT_RESTORE_FRAME
> -	movq 48(%rsp), %r9
> -	movq 40(%rsp), %r8
> -	movq 32(%rsp), %rdi
> -	movq 24(%rsp), %rsi
> -	movq 16(%rsp), %rdx
> -	movq 8(%rsp), %rcx
> -	movq (%rsp), %rax
> -	addq $0x38, %rsp
> +	movq R9(%rsp), %r9
> +	movq R8(%rsp), %r8
> +	movq RDI(%rsp), %rdi
> +	movq RSI(%rsp), %rsi
> +	movq RDX(%rsp), %rdx
> +	movq RCX(%rsp), %rcx
> +	movq RAX(%rsp), %rax
> +	addq $ORIG_RAX, %rsp
>  	.endm
>
>  #endif
> @@ -34,6 +37,7 @@
>
>  #if defined(CONFIG_DYNAMIC_FTRACE) && defined(CONFIG_X86_64)
>  #define ARCH_SUPPORTS_FTRACE_OPS 1
> +#define ARCH_SUPPORTS_FTRACE_SAVE_REGS 1
>  #endif
>
>  #ifndef __ASSEMBLY__
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 27adc2b..b77f297 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -78,7 +78,16 @@ ENTRY(ftrace_caller)

I can see below code before save frame.

	cmpl $0, function_trace_stop
	jne  ftrace_stub

Please pushf before comparing it. :)
Sometimes, the value of eflags is worth to watch.
I know that SF/ZF will be never used between
function call, so it is OK if the eflags is saved
in MCOUNT_SAVE_FRAME.

>  	MCOUNT_SAVE_FRAME
>
>  	leaq function_trace_op, %rdx
> -	movq 0x38(%rsp), %rdi
> +
> +	cmpl $0, ftrace_save_regs
> +	jne  save_all_regs
> +
> +call_func:
> +
> +	/* regs go into 4th parameter */
> +	leaq (%rsp), %rcx
> +
> +	movq ORIG_RAX(%rsp), %rdi
>  	movq 8(%rbp), %rsi
>  	subq $MCOUNT_INSN_SIZE, %rdi
>
> @@ -96,6 +105,18 @@ GLOBAL(ftrace_stub)
>  	retq
>  END(ftrace_caller)
>
> +save_all_regs:
> +	/* Save the rest of pt_regs */
> +	movq %r15, R15(%rsp)
> +	movq %r14, R14(%rsp)
> +	movq %r13, R13(%rsp)
> +	movq %r12, R12(%rsp)
> +	movq %r10, R10(%rsp)
> +	movq %rbp, RBP(%rsp)
> +	movq %rbx, RBX(%rsp)
> +	jmp call_func

At least, pt_regs.sp must be saved for accessing
vars on stack.

> +
> +
>  #else /* ! CONFIG_DYNAMIC_FTRACE */
>  ENTRY(mcount)
>  	cmpl $0, function_trace_stop

You also need to restore the rest of pt_regs if
ftrace_save_regs is true.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com