[RFC][PATCH 0/5] ftrace/x86_32: Ftrace cleanup and add support for -mfentry

[RFC][PATCH 0/5] ftrace/x86_32: Ftrace cleanup and add support for -mfentry

[Steven Rostedt]

With the issues of gcc screwing around with the mcount stack frame causing
function graph tracer to panic on x86_32, and with Linus saying that we
should start deprecating mcount (at least on x86), I figured that x86_32
needs to support fentry.

First, I renamed mcount_64.S to ftrace_64.S (hmm, my subject says ftrace.S,
oh well, this is still RFC, I'll fix that before posting). As we want to
get away from mcount, having the ftrace code in a file called mcount
seems rather backwards.

Next I moved the ftrace code out of entry_32.S. It's not in entry_64.S
and it does not belong in entry_32.S.

I noticed that the x86_32 code has the same issue as the x86_64 did
in the past with respect to a stack frame. I fixed that just for the main
ftrace_caller. The ftrace_regs_caller is rather special, and so is
function graph tracing.

I realized the ftrace_regs_caller code was complex due to me aggressively
saving flags, even though I could still do push, lea and mov without
changing them. That made the logic a little nicer.

Finally I added the fentry code.

I tested this (and I'm currently testing it) with an old compiler
(for mcount) with and without FRAME_POINTER set. I also did it with
a new compiler (with fentry), with and without FRAME_POINTER. I tested
function tracing, stack tracing, function_graph tracing, and kprobes
(as that uses the ftrace_regs_caller). So far this works. I'm still testing
but I'm confident enough with the current code to post an RFC.

Thoughts?

-- Steve


Steven Rostedt (VMware) (5):
      x86/ftrace: Rename mcount_64.S to ftrace.S
      ftrace/x86-32: Move the ftrace specific code out of entry_32.S
      ftrace/x86_32: Add stack frame pointer to ftrace_caller
      ftrace/x86_32: Clean up ftrace_regs_caller
      ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

----
 Makefile                                     |  12 +-
 arch/x86/Kconfig                             |   2 +-
 arch/x86/entry/entry_32.S                    | 168 -----------------
 arch/x86/kernel/Makefile                     |   5 +-
 arch/x86/kernel/ftrace_32.S                  | 257 +++++++++++++++++++++++++++
 arch/x86/kernel/{mcount_64.S => ftrace_64.S} |   0
 6 files changed, 267 insertions(+), 177 deletions(-)
 create mode 100644 arch/x86/kernel/ftrace_32.S
 rename arch/x86/kernel/{mcount_64.S => ftrace_64.S} (100%)

[RFC][PATCH 1/5] x86/ftrace: Rename mcount_64.S to ftrace.S

[Steven Rostedt]

[-- Attachment #1: 0001-x86-ftrace-Rename-mcount_64.S-to-ftrace.S.patch --]
[-- Type: text/plain, Size: 1670 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

With the advent of -mfentry that uses the new "fentry" hook over mcount, the
mcount name is obsolete. Having the code file that ftrace hooks into called
"mcount*.S" is rather misleading. Rename it to ftrace.S

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/Makefile                     | 4 ++--
 arch/x86/kernel/{mcount_64.S => ftrace_64.S} | 0
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename arch/x86/kernel/{mcount_64.S => ftrace_64.S} (100%)

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 84c00592d359..d3743a37e990 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -27,7 +27,7 @@ KASAN_SANITIZE_stacktrace.o := n
 
 OBJECT_FILES_NON_STANDARD_head_$(BITS).o		:= y
 OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o	:= y
-OBJECT_FILES_NON_STANDARD_mcount_$(BITS).o		:= y
+OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o		:= y
 OBJECT_FILES_NON_STANDARD_test_nx.o			:= y
 
 # If instrumentation of this dir is enabled, boot hangs during first second.
@@ -46,7 +46,7 @@ obj-$(CONFIG_MODIFY_LDT_SYSCALL)	+= ldt.o
 obj-y			+= setup.o x86_init.o i8259.o irqinit.o jump_label.o
 obj-$(CONFIG_IRQ_WORK)  += irq_work.o
 obj-y			+= probe_roms.o
-obj-$(CONFIG_X86_64)	+= sys_x86_64.o mcount_64.o
+obj-$(CONFIG_X86_64)	+= sys_x86_64.o ftrace_64.o
 obj-$(CONFIG_X86_ESPFIX64)	+= espfix_64.o
 obj-$(CONFIG_SYSFS)	+= ksysfs.o
 obj-y			+= bootflag.o e820.o
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/ftrace_64.S
similarity index 100%
rename from arch/x86/kernel/mcount_64.S
rename to arch/x86/kernel/ftrace_64.S
-- 
2.10.2

[RFC][PATCH 2/5] ftrace/x86-32: Move the ftrace specific code out of entry_32.S

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-x86-32-Move-the-ftrace-specific-code-out-of-e.patch --]
[-- Type: text/plain, Size: 10692 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The function tracing hook code for ftrace is not an entry point from
userspace and does not belong in the entry_*.S files. It has already been
moved out of entry_64.S. This moves it out of entry_32.S

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 Makefile                    |  12 +--
 arch/x86/entry/entry_32.S   | 168 ------------------------------------------
 arch/x86/kernel/Makefile    |   1 +
 arch/x86/kernel/ftrace_32.S | 176 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 183 insertions(+), 174 deletions(-)
 create mode 100644 arch/x86/kernel/ftrace_32.S

diff --git a/Makefile b/Makefile
index 165cf9783a5d..7e3aadeea0c1 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,12 @@ KBUILD_CFLAGS += $(call cc-ifversion, -lt, 0409, \
 # Tell gcc to never replace conditional load with a non-conditional one
 KBUILD_CFLAGS	+= $(call cc-option,--param=allow-store-data-races=0)
 
+# check for 'asm goto'
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
+	KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
+	KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
+endif
+
 include scripts/Makefile.gcc-plugins
 
 ifdef CONFIG_READABLE_ASM
@@ -798,12 +804,6 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=incompatible-pointer-types)
 # use the deterministic mode of AR if available
 KBUILD_ARFLAGS := $(call ar-option,D)
 
-# check for 'asm goto'
-ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
-	KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
-	KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
-endif
-
 include scripts/Makefile.kasan
 include scripts/Makefile.extrawarn
 include scripts/Makefile.ubsan
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 57f7ec35216e..c576352361a7 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -38,13 +38,11 @@
 #include <asm/page_types.h>
 #include <asm/percpu.h>
 #include <asm/processor-flags.h>
-#include <asm/ftrace.h>
 #include <asm/irq_vectors.h>
 #include <asm/cpufeatures.h>
 #include <asm/alternative-asm.h>
 #include <asm/asm.h>
 #include <asm/smap.h>
-#include <asm/export.h>
 #include <asm/frame.h>
 
 	.section .entry.text, "ax"
@@ -886,172 +884,6 @@ BUILD_INTERRUPT3(hyperv_callback_vector, HYPERVISOR_CALLBACK_VECTOR,
 
 #endif /* CONFIG_HYPERV */
 
-#ifdef CONFIG_FUNCTION_TRACER
-#ifdef CONFIG_DYNAMIC_FTRACE
-
-ENTRY(mcount)
-	ret
-END(mcount)
-
-ENTRY(ftrace_caller)
-	pushl	%eax
-	pushl	%ecx
-	pushl	%edx
-	pushl	$0				/* Pass NULL as regs pointer */
-	movl	4*4(%esp), %eax
-	movl	0x4(%ebp), %edx
-	movl	function_trace_op, %ecx
-	subl	$MCOUNT_INSN_SIZE, %eax
-
-.globl ftrace_call
-ftrace_call:
-	call	ftrace_stub
-
-	addl	$4, %esp			/* skip NULL pointer */
-	popl	%edx
-	popl	%ecx
-	popl	%eax
-.Lftrace_ret:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
-	jmp	ftrace_stub
-#endif
-
-/* This is weak to keep gas from relaxing the jumps */
-WEAK(ftrace_stub)
-	ret
-END(ftrace_caller)
-
-ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
-	/*
-	 * 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 */
-
-	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) */
-	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
-	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
-	movl	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 */
-	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	.Lftrace_ret
-
-	popf
-	jmp	ftrace_stub
-#else /* ! CONFIG_DYNAMIC_FTRACE */
-
-ENTRY(mcount)
-	cmpl	$__PAGE_OFFSET, %esp
-	jb	ftrace_stub			/* Paging not enabled yet? */
-
-	cmpl	$ftrace_stub, ftrace_trace_function
-	jnz	.Ltrace
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	cmpl	$ftrace_stub, ftrace_graph_return
-	jnz	ftrace_graph_caller
-
-	cmpl	$ftrace_graph_entry_stub, ftrace_graph_entry
-	jnz	ftrace_graph_caller
-#endif
-.globl ftrace_stub
-ftrace_stub:
-	ret
-
-	/* taken from glibc */
-.Ltrace:
-	pushl	%eax
-	pushl	%ecx
-	pushl	%edx
-	movl	0xc(%esp), %eax
-	movl	0x4(%ebp), %edx
-	subl	$MCOUNT_INSN_SIZE, %eax
-
-	call	*ftrace_trace_function
-
-	popl	%edx
-	popl	%ecx
-	popl	%eax
-	jmp	ftrace_stub
-END(mcount)
-#endif /* CONFIG_DYNAMIC_FTRACE */
-EXPORT_SYMBOL(mcount)
-#endif /* CONFIG_FUNCTION_TRACER */
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-ENTRY(ftrace_graph_caller)
-	pushl	%eax
-	pushl	%ecx
-	pushl	%edx
-	movl	0xc(%esp), %eax
-	lea	0x4(%ebp), %edx
-	movl	(%ebp), %ecx
-	subl	$MCOUNT_INSN_SIZE, %eax
-	call	prepare_ftrace_return
-	popl	%edx
-	popl	%ecx
-	popl	%eax
-	ret
-END(ftrace_graph_caller)
-
-.globl return_to_handler
-return_to_handler:
-	pushl	%eax
-	pushl	%edx
-	movl	%ebp, %eax
-	call	ftrace_return_to_handler
-	movl	%eax, %ecx
-	popl	%edx
-	popl	%eax
-	jmp	*%ecx
-#endif
-
 #ifdef CONFIG_TRACING
 ENTRY(trace_page_fault)
 	ASM_CLAC
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index d3743a37e990..55e8902c461f 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -47,6 +47,7 @@ obj-y			+= setup.o x86_init.o i8259.o irqinit.o jump_label.o
 obj-$(CONFIG_IRQ_WORK)  += irq_work.o
 obj-y			+= probe_roms.o
 obj-$(CONFIG_X86_64)	+= sys_x86_64.o ftrace_64.o
+obj-$(CONFIG_X86_32)	+= ftrace_32.o
 obj-$(CONFIG_X86_ESPFIX64)	+= espfix_64.o
 obj-$(CONFIG_SYSFS)	+= ksysfs.o
 obj-y			+= bootflag.o e820.o
diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
new file mode 100644
index 000000000000..4ff30f31f0a5
--- /dev/null
+++ b/arch/x86/kernel/ftrace_32.S
@@ -0,0 +1,176 @@
+/*
+ *  linux/arch/x86_64/mcount_64.S
+ *
+ *  Copyright (C) 2017  Steven Rostedt, VMware Inc.
+ */
+
+#include <linux/linkage.h>
+#include <asm/segment.h>
+#include <asm/export.h>
+#include <asm/ftrace.h>
+
+#ifdef CONFIG_FUNCTION_TRACER
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+ENTRY(mcount)
+	ret
+END(mcount)
+
+ENTRY(ftrace_caller)
+	pushl	%eax
+	pushl	%ecx
+	pushl	%edx
+	pushl	$0				/* Pass NULL as regs pointer */
+	movl	4*4(%esp), %eax
+	movl	0x4(%ebp), %edx
+	movl	function_trace_op, %ecx
+	subl	$MCOUNT_INSN_SIZE, %eax
+
+.globl ftrace_call
+ftrace_call:
+	call	ftrace_stub
+
+	addl	$4, %esp			/* skip NULL pointer */
+	popl	%edx
+	popl	%ecx
+	popl	%eax
+.Lftrace_ret:
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+.globl ftrace_graph_call
+ftrace_graph_call:
+	jmp	ftrace_stub
+#endif
+
+/* This is weak to keep gas from relaxing the jumps */
+WEAK(ftrace_stub)
+	ret
+END(ftrace_caller)
+
+ENTRY(ftrace_regs_caller)
+	pushf	/* push flags before compare (in cs location) */
+
+	/*
+	 * 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 */
+
+	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) */
+	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
+	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
+	movl	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 */
+	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	.Lftrace_ret
+
+	popf
+	jmp	ftrace_stub
+#else /* ! CONFIG_DYNAMIC_FTRACE */
+
+ENTRY(mcount)
+	cmpl	$__PAGE_OFFSET, %esp
+	jb	ftrace_stub			/* Paging not enabled yet? */
+
+	cmpl	$ftrace_stub, ftrace_trace_function
+	jnz	.Ltrace
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	cmpl	$ftrace_stub, ftrace_graph_return
+	jnz	ftrace_graph_caller
+
+	cmpl	$ftrace_graph_entry_stub, ftrace_graph_entry
+	jnz	ftrace_graph_caller
+#endif
+.globl ftrace_stub
+ftrace_stub:
+	ret
+
+	/* taken from glibc */
+.Ltrace:
+	pushl	%eax
+	pushl	%ecx
+	pushl	%edx
+	movl	0xc(%esp), %eax
+	movl	0x4(%ebp), %edx
+	subl	$MCOUNT_INSN_SIZE, %eax
+
+	call	*ftrace_trace_function
+
+	popl	%edx
+	popl	%ecx
+	popl	%eax
+	jmp	ftrace_stub
+END(mcount)
+#endif /* CONFIG_DYNAMIC_FTRACE */
+EXPORT_SYMBOL(mcount)
+#endif /* CONFIG_FUNCTION_TRACER */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(ftrace_graph_caller)
+	pushl	%eax
+	pushl	%ecx
+	pushl	%edx
+	movl	0xc(%esp), %eax
+	lea	0x4(%ebp), %edx
+	movl	(%ebp), %ecx
+	subl	$MCOUNT_INSN_SIZE, %eax
+	call	prepare_ftrace_return
+	popl	%edx
+	popl	%ecx
+	popl	%eax
+	ret
+END(ftrace_graph_caller)
+
+.globl return_to_handler
+return_to_handler:
+	pushl	%eax
+	pushl	%edx
+	movl	%ebp, %eax
+	call	ftrace_return_to_handler
+	movl	%eax, %ecx
+	popl	%edx
+	popl	%eax
+	jmp	*%ecx
+#endif
-- 
2.10.2

[RFC][PATCH 3/5] ftrace/x86_32: Add stack frame pointer to ftrace_caller

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-x86_32-Add-stack-frame-pointer-to-ftrace_call.patch --]
[-- Type: text/plain, Size: 2078 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The function hook ftrace_caller does not create its own stack frame, and
this causes the ftrace stack trace to miss the first function when doing
stack traces.

 # echo schedule:stacktrace > /sys/kernel/tracing/set_ftrace_filter

Before:
         <idle>-0     [002] .N..    29.865807: <stack trace>
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [001] ....    29.866509: <stack trace>
 => kthread
 => ret_from_fork
           <...>-1     [000] ....    29.865377: <stack trace>
 => poll_schedule_timeout
 => do_select
 => core_sys_select
 => SyS_select
 => do_fast_syscall_32
 => entry_SYSENTER_32

After:
          <idle>-0     [002] .N..    31.234853: <stack trace>
 => do_idle
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [003] ....    31.235140: <stack trace>
 => rcu_gp_kthread
 => kthread
 => ret_from_fork
           <...>-1819  [000] ....    31.264172: <stack trace>
 => schedule_hrtimeout_range
 => poll_schedule_timeout
 => do_sys_poll
 => SyS_ppoll
 => do_fast_syscall_32
 => entry_SYSENTER_32

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace_32.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 4ff30f31f0a5..73d61a62649d 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -17,12 +17,19 @@ ENTRY(mcount)
 END(mcount)
 
 ENTRY(ftrace_caller)
+
+	pushl %ebp
+	movl %esp, %ebp
+
 	pushl	%eax
 	pushl	%ecx
 	pushl	%edx
 	pushl	$0				/* Pass NULL as regs pointer */
-	movl	4*4(%esp), %eax
-	movl	0x4(%ebp), %edx
+	movl	5*4(%esp), %eax
+	/* Copy original ebp into %edx */
+	movl	4*4(%esp), %edx
+	/* Get the parent ip */
+	movl	0x4(%edx), %edx
 	movl	function_trace_op, %ecx
 	subl	$MCOUNT_INSN_SIZE, %eax
 
@@ -34,6 +41,7 @@ ftrace_call:
 	popl	%edx
 	popl	%ecx
 	popl	%eax
+	popl	%ebp
 .Lftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
-- 
2.10.2

Re: [RFC][PATCH 3/5] ftrace/x86_32: Add stack frame pointer to ftrace_caller

[Josh Poimboeuf]

On Wed, Mar 15, 2017 at 03:55:30PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The function hook ftrace_caller does not create its own stack frame, and
> this causes the ftrace stack trace to miss the first function when doing
> stack traces.
> 
>  # echo schedule:stacktrace > /sys/kernel/tracing/set_ftrace_filter
> 
> Before:
>          <idle>-0     [002] .N..    29.865807: <stack trace>
>  => cpu_startup_entry
>  => start_secondary
>  => startup_32_smp
>            <...>-7     [001] ....    29.866509: <stack trace>
>  => kthread
>  => ret_from_fork
>            <...>-1     [000] ....    29.865377: <stack trace>
>  => poll_schedule_timeout
>  => do_select
>  => core_sys_select
>  => SyS_select
>  => do_fast_syscall_32
>  => entry_SYSENTER_32
> 
> After:
>           <idle>-0     [002] .N..    31.234853: <stack trace>
>  => do_idle
>  => cpu_startup_entry
>  => start_secondary
>  => startup_32_smp
>            <...>-7     [003] ....    31.235140: <stack trace>
>  => rcu_gp_kthread
>  => kthread
>  => ret_from_fork
>            <...>-1819  [000] ....    31.264172: <stack trace>
>  => schedule_hrtimeout_range
>  => poll_schedule_timeout
>  => do_sys_poll
>  => SyS_ppoll
>  => do_fast_syscall_32
>  => entry_SYSENTER_32
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/ftrace_32.S | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> index 4ff30f31f0a5..73d61a62649d 100644
> --- a/arch/x86/kernel/ftrace_32.S
> +++ b/arch/x86/kernel/ftrace_32.S
> @@ -17,12 +17,19 @@ ENTRY(mcount)
>  END(mcount)
>  
>  ENTRY(ftrace_caller)
> +
> +	pushl %ebp
> +	movl %esp, %ebp
> +

The operands for the above two instructions should be vertically aligned
like the rest of the code.  Otherwise:

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[RFC][PATCH 4/5] ftrace/x86_32: Clean up ftrace_regs_caller

[Steven Rostedt]

[-- Attachment #1: 0004-ftrace-x86_32-Clean-up-ftrace_regs_caller.patch --]
[-- Type: text/plain, Size: 3372 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

When ftrace_regs_caller was created, it was designed to preserve flags as
much as possible as it needed to act just like a breakpoint triggered on the
same location. But the design is over complicated as it treated all
operations as modifying flags. But push, mov and lea do not modify flags.
This means the code can become more simplified by allowing flags to be
stored further down.

Making ftrace_regs_caller simpler will also be useful in implementing fentry
logic.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 73d61a62649d..8ca33d9806ac 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -55,23 +55,30 @@ WEAK(ftrace_stub)
 END(ftrace_caller)
 
 ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
 	/*
 	 * 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 */
+	 * regs->ip location, and move flags into the return ip location.
+         */
+	pushl	$__KERNEL_CS
+	pushl	4(%esp)				/* Save the return ip */
+
+	/* temporarily save flags in the orig_ax location */
+	pushf
 
-	pushl	$0				/* Load 0 into orig_ax */
 	pushl	%gs
 	pushl	%fs
 	pushl	%es
 	pushl	%ds
 	pushl	%eax
+
+	/* move flags into the location of where the return ip was */
+	movl	5*4(%esp), %eax
+	movl	$0, (%esp)			/* Load 0 into orig_ax */
+	movl	%eax, 8*4(%esp)
+
 	pushl	%ebp
 	pushl	%edi
 	pushl	%esi
@@ -79,11 +86,6 @@ ENTRY(ftrace_regs_caller)
 	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) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
@@ -94,10 +96,14 @@ 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 */
-	movl	%eax, 14*4(%esp)		/* Put return ip back for ret */
+
+	/* Since we don't care about cs, move flags there to simplify return */
+	movl	14*4(%esp), %eax
+	movl	%eax, 13*4(%esp)
+
+	/* Move ip back to its original location */
+	movl	12*4(%esp), %eax
+	movl	%eax, 14*4(%esp)
 
 	popl	%ebx
 	popl	%ecx
@@ -110,12 +116,11 @@ 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) */
-	jmp	.Lftrace_ret
+	addl	$8, %esp			/* Skip orig_ax and old ip */
 
-	popf
-	jmp	ftrace_stub
+	popf					/* flags is in the cs location */
+
+	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)
-- 
2.10.2

Re: [RFC][PATCH 4/5] ftrace/x86_32: Clean up ftrace_regs_caller

[Josh Poimboeuf]

On Wed, Mar 15, 2017 at 03:55:31PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> When ftrace_regs_caller was created, it was designed to preserve flags as
> much as possible as it needed to act just like a breakpoint triggered on the
> same location. But the design is over complicated as it treated all
> operations as modifying flags. But push, mov and lea do not modify flags.
> This means the code can become more simplified by allowing flags to be
> stored further down.
> 
> Making ftrace_regs_caller simpler will also be useful in implementing fentry
> logic.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> index 73d61a62649d..8ca33d9806ac 100644
> --- a/arch/x86/kernel/ftrace_32.S
> +++ b/arch/x86/kernel/ftrace_32.S
> @@ -55,23 +55,30 @@ WEAK(ftrace_stub)
>  END(ftrace_caller)
>  
>  ENTRY(ftrace_regs_caller)
> -	pushf	/* push flags before compare (in cs location) */
> -
>  	/*
>  	 * 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 */
> +	 * regs->ip location, and move flags into the return ip location.
> +         */
> +	pushl	$__KERNEL_CS
> +	pushl	4(%esp)				/* Save the return ip */
> +
> +	/* temporarily save flags in the orig_ax location */
> +	pushf
>  
> -	pushl	$0				/* Load 0 into orig_ax */
>  	pushl	%gs
>  	pushl	%fs
>  	pushl	%es
>  	pushl	%ds
>  	pushl	%eax
> +
> +	/* move flags into the location of where the return ip was */
> +	movl	5*4(%esp), %eax
> +	movl	$0, (%esp)			/* Load 0 into orig_ax */

This writes a 0 into regs->ax instead of regs->orig_ax.

-- 
Josh

Re: [RFC][PATCH 4/5] ftrace/x86_32: Clean up ftrace_regs_caller

[Steven Rostedt]

On Wed, 15 Mar 2017 16:13:43 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 15, 2017 at 03:55:31PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > When ftrace_regs_caller was created, it was designed to preserve flags as
> > much as possible as it needed to act just like a breakpoint triggered on the
> > same location. But the design is over complicated as it treated all
> > operations as modifying flags. But push, mov and lea do not modify flags.
> > This means the code can become more simplified by allowing flags to be
> > stored further down.
> > 
> > Making ftrace_regs_caller simpler will also be useful in implementing fentry
> > logic.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> > index 73d61a62649d..8ca33d9806ac 100644
> > --- a/arch/x86/kernel/ftrace_32.S
> > +++ b/arch/x86/kernel/ftrace_32.S
> > @@ -55,23 +55,30 @@ WEAK(ftrace_stub)
> >  END(ftrace_caller)
> >  
> >  ENTRY(ftrace_regs_caller)
> > -	pushf	/* push flags before compare (in cs location) */
> > -
> >  	/*
> >  	 * 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 */
> > +	 * regs->ip location, and move flags into the return ip location.
> > +         */
> > +	pushl	$__KERNEL_CS
> > +	pushl	4(%esp)				/* Save the return ip */
> > +
> > +	/* temporarily save flags in the orig_ax location */
> > +	pushf
> >  
> > -	pushl	$0				/* Load 0 into orig_ax */
> >  	pushl	%gs
> >  	pushl	%fs
> >  	pushl	%es
> >  	pushl	%ds
> >  	pushl	%eax
> > +
> > +	/* move flags into the location of where the return ip was */
> > +	movl	5*4(%esp), %eax
> > +	movl	$0, (%esp)			/* Load 0 into orig_ax */  
> 
> This writes a 0 into regs->ax instead of regs->orig_ax.
> 

Ouch, I'm surprised this didn't crash my box. Thanks for catching it.

-- Steve

Re: [RFC][PATCH 4/5] ftrace/x86_32: Clean up ftrace_regs_caller

[Masami Hiramatsu]

On Wed, 15 Mar 2017 19:11:24 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 15 Mar 2017 16:13:43 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Wed, Mar 15, 2017 at 03:55:31PM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > 
> > > When ftrace_regs_caller was created, it was designed to preserve flags as
> > > much as possible as it needed to act just like a breakpoint triggered on the
> > > same location. But the design is over complicated as it treated all
> > > operations as modifying flags. But push, mov and lea do not modify flags.
> > > This means the code can become more simplified by allowing flags to be
> > > stored further down.
> > > 
> > > Making ftrace_regs_caller simpler will also be useful in implementing fentry
> > > logic.
> > > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > ---
> > >  arch/x86/kernel/ftrace_32.S | 45 +++++++++++++++++++++++++--------------------
> > >  1 file changed, 25 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> > > index 73d61a62649d..8ca33d9806ac 100644
> > > --- a/arch/x86/kernel/ftrace_32.S
> > > +++ b/arch/x86/kernel/ftrace_32.S
> > > @@ -55,23 +55,30 @@ WEAK(ftrace_stub)
> > >  END(ftrace_caller)
> > >  
> > >  ENTRY(ftrace_regs_caller)
> > > -	pushf	/* push flags before compare (in cs location) */
> > > -
> > >  	/*
> > >  	 * 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 */
> > > +	 * regs->ip location, and move flags into the return ip location.
> > > +         */
> > > +	pushl	$__KERNEL_CS
> > > +	pushl	4(%esp)				/* Save the return ip */
> > > +
> > > +	/* temporarily save flags in the orig_ax location */
> > > +	pushf
> > >  
> > > -	pushl	$0				/* Load 0 into orig_ax */
> > >  	pushl	%gs
> > >  	pushl	%fs
> > >  	pushl	%es
> > >  	pushl	%ds
> > >  	pushl	%eax
> > > +
> > > +	/* move flags into the location of where the return ip was */
> > > +	movl	5*4(%esp), %eax
> > > +	movl	$0, (%esp)			/* Load 0 into orig_ax */  
> > 
> > This writes a 0 into regs->ax instead of regs->orig_ax.
> > 
> 
> Ouch, I'm surprised this didn't crash my box. Thanks for catching it.

Since mcount/fentry is just "called", %eax (caller-saved register)
will be ignored :) Of course for kprobes and livepatch, this should be
critical.

Except for the above point, it seems good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[RFC][PATCH 5/5] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

[Steven Rostedt]

[-- Attachment #1: 0005-ftrace-x86-32-Add-mfentry-support-to-x86_32-with-DYN.patch --]
[-- Type: text/plain, Size: 5323 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

x86_64 has had fentry support for some time. I did not add support to x86_32
as I was unsure if it will be used much in the future. It is still very much
used, and there's issues with function graph tracing with gcc playing around
with the mcount frames, causing function graph to panic. The fentry code
does not have this issue, and is able to cope as there is no frame to mess
up.

Note, this only add support for fentry when DYNAMIC_FTRACE is set. There's
really no reason to not have that set, because the performance of the
machine drops significantly when it's not enabled. I only keep
!DYNAMIC_FTRACE around to test it off, as there's still some archs that have
FTRACE but not DYNAMIC_FTRACE.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/Kconfig            |  2 +-
 arch/x86/kernel/ftrace_32.S | 88 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 79 insertions(+), 11 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index cc98d5a294ee..8c17146427ca 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -127,7 +127,7 @@ config X86
 	select HAVE_EBPF_JIT			if X86_64
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_EXIT_THREAD
-	select HAVE_FENTRY			if X86_64
+	select HAVE_FENTRY			if X86_64 || DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACER
diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 8ca33d9806ac..4bf8223555cd 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -9,27 +9,75 @@
 #include <asm/export.h>
 #include <asm/ftrace.h>
 
+
+#ifdef CC_USING_FENTRY
+# define function_hook	__fentry__
+EXPORT_SYMBOL(__fentry__)
+#else
+# define function_hook	mcount
+EXPORT_SYMBOL(mcount)
+#endif
+
+/* mcount uses a frame pointer even if CONFIG_FRAME_POINTER is not set */
+#if !defined(CC_USING_FENTRY) || defined(CONFIG_FRAME_POINTER)
+# define USING_FRAME_POINTER
+#endif
+
+#ifdef USING_FRAME_POINTER
+# ifdef CC_USING_FENTRY
+#  define MCOUNT_FRAME_SIZE		(4*4)	/* bp,ip and parent's  */
+# else
+#  define MCOUNT_FRAME_SIZE		4	/* just the bp         */
+# endif
+# define MCOUNT_FRAME			1	/* using frame = true  */
+#else
+# define MCOUNT_FRAME_SIZE		0	/* no stack frame      */
+# define MCOUNT_FRAME			0	/* using frame = false */
+#endif
+
 #ifdef CONFIG_FUNCTION_TRACER
 #ifdef CONFIG_DYNAMIC_FTRACE
 
-ENTRY(mcount)
+ENTRY(function_hook)
 	ret
-END(mcount)
+END(function_hook)
 
 ENTRY(ftrace_caller)
 
+#ifdef USING_FRAME_POINTER
+# ifdef CC_USING_FENTRY
+	/*
+	 * Frame pointers are of ip followed by bp.
+	 * Since fentry is an immediate jump, we are left with
+	 * parent-ip, function-ip. We need to add a frame with
+	 * parent-ip followed by ebp.
+	 */
+	pushl 4(%esp)				/* parent ip */
 	pushl %ebp
 	movl %esp, %ebp
-
+	pushl 2*4(%esp)				/* function ip */
+# endif
+	/* For mcount, the function ip is directly above */
+	pushl %ebp
+	movl %esp, %ebp
+#endif
 	pushl	%eax
 	pushl	%ecx
 	pushl	%edx
 	pushl	$0				/* Pass NULL as regs pointer */
-	movl	5*4(%esp), %eax
-	/* Copy original ebp into %edx */
+
+#ifdef USING_FRAME_POINTER
+	/* Load parent ebp into edx */
 	movl	4*4(%esp), %edx
+#else
+	/* There's no frame pointer, load the appropriate stack addr instead */
+	lea	4*4(%esp), %edx
+#endif
+
+	movl	(MCOUNT_FRAME+4)*4(%esp), %eax	/* load the rip */
 	/* Get the parent ip */
-	movl	0x4(%edx), %edx
+	movl	4(%edx), %edx			/* edx has ebp */
+
 	movl	function_trace_op, %ecx
 	subl	$MCOUNT_INSN_SIZE, %eax
 
@@ -41,7 +89,14 @@ ftrace_call:
 	popl	%edx
 	popl	%ecx
 	popl	%eax
+#ifdef USING_FRAME_POINTER
 	popl	%ebp
+# ifdef CC_USING_FENTRY
+	addl	$4,%esp				/* skip function ip */
+	popl	%ebp				/* this is the orig bp */
+	addl	$4, %esp			/* skip parent ip */
+# endif
+#endif
 .Lftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
@@ -85,6 +140,10 @@ ENTRY(ftrace_regs_caller)
 	pushl	%edx
 	pushl	%ecx
 	pushl	%ebx
+#ifndef USING_FRAME_POINTER
+	/* Load 4 off of the parent ip addr into ebp */
+	lea	14*4(%esp), %ebp
+#endif
 
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
@@ -123,7 +182,7 @@ GLOBAL(ftrace_regs_call)
 	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
-ENTRY(mcount)
+ENTRY(function_hook)
 	cmpl	$__PAGE_OFFSET, %esp
 	jb	ftrace_stub			/* Paging not enabled yet? */
 
@@ -155,9 +214,8 @@ ftrace_stub:
 	popl	%ecx
 	popl	%eax
 	jmp	ftrace_stub
-END(mcount)
+END(function_hook)
 #endif /* CONFIG_DYNAMIC_FTRACE */
-EXPORT_SYMBOL(mcount)
 #endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
@@ -165,9 +223,15 @@ ENTRY(ftrace_graph_caller)
 	pushl	%eax
 	pushl	%ecx
 	pushl	%edx
-	movl	0xc(%esp), %eax
+	movl	3*4(%esp), %eax
+	/* Even with frame pointers, fentry doesn't have one here */
+#ifdef CC_USING_FENTRY
+	lea	4*4(%esp), %edx
+	movl	$0, %ecx
+#else
 	lea	0x4(%ebp), %edx
 	movl	(%ebp), %ecx
+#endif
 	subl	$MCOUNT_INSN_SIZE, %eax
 	call	prepare_ftrace_return
 	popl	%edx
@@ -180,7 +244,11 @@ END(ftrace_graph_caller)
 return_to_handler:
 	pushl	%eax
 	pushl	%edx
+#ifdef CC_USING_FENTRY
+	movl	$0, %eax
+#else
 	movl	%ebp, %eax
+#endif
 	call	ftrace_return_to_handler
 	movl	%eax, %ecx
 	popl	%edx
-- 
2.10.2

Re: [RFC][PATCH 5/5] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

[Josh Poimboeuf]

On Wed, Mar 15, 2017 at 03:55:32PM -0400, Steven Rostedt wrote:
> +#ifdef USING_FRAME_POINTER
> +# ifdef CC_USING_FENTRY
> +	/*
> +	 * Frame pointers are of ip followed by bp.
> +	 * Since fentry is an immediate jump, we are left with
> +	 * parent-ip, function-ip. We need to add a frame with
> +	 * parent-ip followed by ebp.
> +	 */
> +	pushl 4(%esp)				/* parent ip */
>  	pushl %ebp
>  	movl %esp, %ebp
> -
> +	pushl 2*4(%esp)				/* function ip */
> +# endif
> +	/* For mcount, the function ip is directly above */
> +	pushl %ebp
> +	movl %esp, %ebp
> +#endif

More vertical operand alignment issues.

> @@ -85,6 +140,10 @@ ENTRY(ftrace_regs_caller)
>  	pushl	%edx
>  	pushl	%ecx
>  	pushl	%ebx
> +#ifndef USING_FRAME_POINTER
> +	/* Load 4 off of the parent ip addr into ebp */
> +	lea	14*4(%esp), %ebp
> +#endif

Instead of:

	#ifndef USING_FRAME_POINTER
	
Shouldn't it be:

	#ifdef CC_USING_FENTRY

?

Otherwise, if I'm reading it right, with fentry and
CONFIG_FRAME_POINTER=y, ebp will lead to the grandparent's ip instead of
the parent's ip.

-- 
Josh

Re: [RFC][PATCH 5/5] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

[Steven Rostedt]

On Wed, 15 Mar 2017 16:55:45 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 15, 2017 at 03:55:32PM -0400, Steven Rostedt wrote:
> > +#ifdef USING_FRAME_POINTER
> > +# ifdef CC_USING_FENTRY
> > +	/*
> > +	 * Frame pointers are of ip followed by bp.
> > +	 * Since fentry is an immediate jump, we are left with
> > +	 * parent-ip, function-ip. We need to add a frame with
> > +	 * parent-ip followed by ebp.
> > +	 */
> > +	pushl 4(%esp)				/* parent ip */
> >  	pushl %ebp
> >  	movl %esp, %ebp
> > -
> > +	pushl 2*4(%esp)				/* function ip */
> > +# endif
> > +	/* For mcount, the function ip is directly above */
> > +	pushl %ebp
> > +	movl %esp, %ebp
> > +#endif  
> 
> More vertical operand alignment issues.
> 
> > @@ -85,6 +140,10 @@ ENTRY(ftrace_regs_caller)
> >  	pushl	%edx
> >  	pushl	%ecx
> >  	pushl	%ebx
> > +#ifndef USING_FRAME_POINTER
> > +	/* Load 4 off of the parent ip addr into ebp */
> > +	lea	14*4(%esp), %ebp
> > +#endif  
> 
> Instead of:
> 
> 	#ifndef USING_FRAME_POINTER
> 	
> Shouldn't it be:
> 
> 	#ifdef CC_USING_FENTRY
> 
> ?
> 
> Otherwise, if I'm reading it right, with fentry and
> CONFIG_FRAME_POINTER=y, ebp will lead to the grandparent's ip instead of
> the parent's ip.

Yep I think you're right. I original added frame pointer setup for the
parent on CC_USING_FENTRY and FRAME_POINTER but that got ugly, this may
be leftover from that.

Thanks, will update.

-- Steve

Re: [RFC][PATCH 5/5] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

[Masami Hiramatsu]

On Wed, 15 Mar 2017 15:55:32 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> x86_64 has had fentry support for some time. I did not add support to x86_32
> as I was unsure if it will be used much in the future. It is still very much
> used, and there's issues with function graph tracing with gcc playing around
> with the mcount frames, causing function graph to panic. The fentry code
> does not have this issue, and is able to cope as there is no frame to mess
> up.
> 
> Note, this only add support for fentry when DYNAMIC_FTRACE is set. There's
> really no reason to not have that set, because the performance of the
> machine drops significantly when it's not enabled. I only keep
> !DYNAMIC_FTRACE around to test it off, as there's still some archs that have
> FTRACE but not DYNAMIC_FTRACE.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/x86/Kconfig            |  2 +-
>  arch/x86/kernel/ftrace_32.S | 88 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cc98d5a294ee..8c17146427ca 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -127,7 +127,7 @@ config X86
>  	select HAVE_EBPF_JIT			if X86_64
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
>  	select HAVE_EXIT_THREAD
> -	select HAVE_FENTRY			if X86_64
> +	select HAVE_FENTRY			if X86_64 || DYNAMIC_FTRACE
>  	select HAVE_FTRACE_MCOUNT_RECORD
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_FUNCTION_TRACER
> diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> index 8ca33d9806ac..4bf8223555cd 100644
> --- a/arch/x86/kernel/ftrace_32.S
> +++ b/arch/x86/kernel/ftrace_32.S
> @@ -9,27 +9,75 @@
>  #include <asm/export.h>
>  #include <asm/ftrace.h>
>  
> +
> +#ifdef CC_USING_FENTRY
> +# define function_hook	__fentry__
> +EXPORT_SYMBOL(__fentry__)
> +#else
> +# define function_hook	mcount
> +EXPORT_SYMBOL(mcount)
> +#endif
> +
> +/* mcount uses a frame pointer even if CONFIG_FRAME_POINTER is not set */
> +#if !defined(CC_USING_FENTRY) || defined(CONFIG_FRAME_POINTER)
> +# define USING_FRAME_POINTER
> +#endif
> +
> +#ifdef USING_FRAME_POINTER
> +# ifdef CC_USING_FENTRY
> +#  define MCOUNT_FRAME_SIZE		(4*4)	/* bp,ip and parent's  */
> +# else
> +#  define MCOUNT_FRAME_SIZE		4	/* just the bp         */
> +# endif
> +# define MCOUNT_FRAME			1	/* using frame = true  */
> +#else
> +# define MCOUNT_FRAME_SIZE		0	/* no stack frame      */
> +# define MCOUNT_FRAME			0	/* using frame = false */
> +#endif

It seems that there is no use of MCOUNT_FRAME_SIZE below. Do we really need it?

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC][PATCH 5/5] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

[Steven Rostedt]

On Thu, 16 Mar 2017 18:24:12 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > +/* mcount uses a frame pointer even if CONFIG_FRAME_POINTER is not set */
> > +#if !defined(CC_USING_FENTRY) || defined(CONFIG_FRAME_POINTER)
> > +# define USING_FRAME_POINTER
> > +#endif
> > +
> > +#ifdef USING_FRAME_POINTER
> > +# ifdef CC_USING_FENTRY
> > +#  define MCOUNT_FRAME_SIZE		(4*4)	/* bp,ip and parent's  */
> > +# else
> > +#  define MCOUNT_FRAME_SIZE		4	/* just the bp         */
> > +# endif
> > +# define MCOUNT_FRAME			1	/* using frame = true  */
> > +#else
> > +# define MCOUNT_FRAME_SIZE		0	/* no stack frame      */
> > +# define MCOUNT_FRAME			0	/* using frame = false */
> > +#endif  
> 
> It seems that there is no use of MCOUNT_FRAME_SIZE below. Do we really need it?

Hmm, one of the previous versions of my patch required it. I think I
was able to tinker with the code to remove all the use cases of it. But
I never removed the definition. Thanks, I'll fix this too.

-- Steve