[PATCH v2] riscv/ftrace: Add basic support

[PATCH v2] riscv/ftrace: Add basic support

[Alan Kao]

This patch contains basic ftrace support for RV64I platform.
Specifically, function tracer (HAVE_FUNCTION_TRACER), function graph
tracer (HAVE_FUNCTION_GRAPH_TRACER), and a frame pointer test
(HAVE_FUNCTION_GRAPH_FP_TEST) are implemented following the
instructions in Documentation/trace/ftrace-design.txt.

Note that the functions in both ftrace.c and setup.c should not be
hooked with the compiler's -pg option: to prevent infinite self-
referencing for the former, and to ignore early setup stuff for the
latter.

Signed-off-by: Alan Kao <alankao@andestech.com>
---
Changes in v2:
  - Add SPDX license identifier
  - Remove unnecessary LOCKDEP_SUPPORT option
  - Remove unnecessary #ifdef in the newly added kernel/ftrace.c

 arch/riscv/Kconfig              |   5 ++
 arch/riscv/include/asm/Kbuild   |   1 -
 arch/riscv/include/asm/ftrace.h |  10 ++++
 arch/riscv/kernel/Makefile      |   7 +++
 arch/riscv/kernel/ftrace.c      |  41 +++++++++++++
 arch/riscv/kernel/mcount.S      | 126 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 189 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/include/asm/ftrace.h
 create mode 100644 arch/riscv/kernel/ftrace.c
 create mode 100644 arch/riscv/kernel/mcount.S

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 2e15e85c8f7e..40a67fc12328 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -60,6 +60,9 @@ config PAGE_OFFSET
 config STACKTRACE_SUPPORT
 	def_bool y
 
+config TRACE_IRQFLAGS_SUPPORT
+	def_bool y
+
 config RWSEM_GENERIC_SPINLOCK
 	def_bool y
 
@@ -112,6 +115,8 @@ config ARCH_RV64I
 	bool "RV64I"
 	select CPU_SUPPORTS_64BIT_KERNEL
 	select 64BIT
+	select HAVE_FUNCTION_TRACER
+	select HAVE_FUNCTION_GRAPH_TRACER
 
 endchoice
 
diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index 18158be62a2b..680301bfbc4b 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -12,7 +12,6 @@ generic-y += errno.h
 generic-y += exec.h
 generic-y += fb.h
 generic-y += fcntl.h
-generic-y += ftrace.h
 generic-y += futex.h
 generic-y += hardirq.h
 generic-y += hash.h
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
new file mode 100644
index 000000000000..66d4175eb13e
--- /dev/null
+++ b/arch/riscv/include/asm/ftrace.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2017 Andes Technology Corporation */
+
+/*
+ * The graph frame test is not possible if CONFIG_FRAME_POINTER is not enabled.
+ * Check arch/riscv/kernel/mcount.S for detail.
+ */
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && defined(CONFIG_FRAME_POINTER)
+#define HAVE_FUNCTION_GRAPH_FP_TEST
+#endif
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index ab8baf7bd142..15941f3b8363 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -2,6 +2,11 @@
 # Makefile for the RISC-V Linux kernel
 #
 
+#ifdef CONFIG_FTRACE
+CFLAGS_REMOVE_ftrace.o = -pg
+CFLAGS_REMOVE_setup.o = -pg
+#endif
+
 extra-y += head.o
 extra-y += vmlinux.lds
 
@@ -29,5 +34,7 @@ CFLAGS_setup.o := -mcmodel=medany
 obj-$(CONFIG_SMP)		+= smpboot.o
 obj-$(CONFIG_SMP)		+= smp.o
 obj-$(CONFIG_MODULES)		+= module.o
+obj-$(CONFIG_FUNCTION_TRACER)	+= mcount.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
 
 clean:
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
new file mode 100644
index 000000000000..d0de68d144cb
--- /dev/null
+++ b/arch/riscv/kernel/ftrace.c
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2013 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ * Copyright (C) 2017 Andes Technology Corporation
+ */
+
+#include <linux/ftrace.h>
+
+/*
+ * Most of this file is copied from arm64.
+ */
+void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
+			   unsigned long frame_pointer)
+{
+	unsigned long return_hooker = (unsigned long)&return_to_handler;
+	unsigned long old;
+	struct ftrace_graph_ent trace;
+	int err;
+
+	if (unlikely(atomic_read(&current->tracing_graph_pause)))
+		return;
+
+	/*
+	 * We don't suffer access faults, so no extra fault-recovery assembly
+	 * is needed here.
+	 */
+	old = *parent;
+
+	trace.func = self_addr;
+	trace.depth = current->curr_ret_stack + 1;
+
+	if (!ftrace_graph_entry(&trace))
+		return;
+
+	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
+				       frame_pointer, NULL);
+	if (err == -EBUSY)
+		return;
+	*parent = return_hooker;
+}
diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
new file mode 100644
index 000000000000..32f2432202e8
--- /dev/null
+++ b/arch/riscv/kernel/mcount.S
@@ -0,0 +1,126 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2017 Andes Technology Corporation */
+
+#include <linux/init.h>
+#include <linux/linkage.h>
+#include <asm/asm.h>
+#include <asm/csr.h>
+#include <asm/unistd.h>
+#include <asm/thread_info.h>
+#include <asm/asm-offsets.h>
+#include <asm-generic/export.h>
+#include <asm/ftrace.h>
+
+	.text
+
+	.macro SAVE_ABI_STATE
+	addi	sp, sp, -16
+	sd	s0, 0(sp)
+	sd	ra, 8(sp)
+	addi	s0, sp, 16
+	.endm
+
+	/*
+	 * The call to ftrace_return_to_handler would overwrite the return
+	 * register if a0 was not saved.
+	 */
+	.macro SAVE_RET_ABI_STATE
+	addi	sp, sp, -32
+	sd	s0, 16(sp)
+	sd	ra, 24(sp)
+	sd	a0, 8(sp)
+	addi	s0, sp, 32
+	.endm
+
+	.macro STORE_ABI_STATE
+	ld	ra, 8(sp)
+	ld	s0, 0(sp)
+	addi	sp, sp, 16
+	.endm
+
+	.macro STORE_RET_ABI_STATE
+	ld	ra, 24(sp)
+	ld	s0, 16(sp)
+	ld	a0, 8(sp)
+	addi	sp, sp, 32
+	.endm
+
+ENTRY(ftrace_stub)
+	ret
+ENDPROC(ftrace_stub)
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(return_to_handler)
+/*
+ * On implementing the frame point test, the ideal way is to compare the
+ * s0 (frame pointer, if enabled) on entry and the sp (stack pointer) on return.
+ * However, the psABI of variable-length-argument functions does not allow this.
+ *
+ * So alternatively we check the *old* frame pointer position, that is, the
+ * value stored in -16(s0) on entry, and the s0 on return.
+ */
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
+	mv	t6, s0
+#endif
+	SAVE_RET_ABI_STATE
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
+	mv	a0, t6
+#endif
+	la	t0, ftrace_return_to_handler
+	jalr	t0
+	mv	a1, a0
+	STORE_RET_ABI_STATE
+	jr	a1
+ENDPROC(return_to_handler)
+EXPORT_SYMBOL(return_to_handler)
+#endif
+
+ENTRY(_mcount)
+	la	t4, ftrace_stub
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	la	t0, ftrace_graph_return
+	ld	t1, 0(t0)
+	bne	t1, t4, do_ftrace_graph_caller
+
+	la	t3, ftrace_graph_entry
+	ld	t2, 0(t3)
+	la	t6, ftrace_graph_entry_stub
+	bne	t2, t6, do_ftrace_graph_caller
+#endif
+	la	t3, ftrace_trace_function
+	ld	t5, 0(t3)
+	bne	t5, t4, do_trace
+	ret
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+/*
+ * A pseudo representation for the function graph tracer:
+ * prepare_to_return(&ra_to_caller_of_caller, ra_to_caller)
+ */
+do_ftrace_graph_caller:
+	addi	a0, s0, -8
+	mv	a1, ra
+#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
+	ld	a2, -16(s0)
+#endif
+	SAVE_ABI_STATE
+	la	t0, prepare_ftrace_return
+	jalr	t0
+	STORE_ABI_STATE
+	ret
+#endif
+
+/*
+ * A pseudo representation for the function tracer:
+ * (*ftrace_trace_function)(ra_to_caller, ra_to_caller_of_caller)
+ */
+do_trace:
+	ld	a1, -8(s0)
+	mv	a0, ra
+
+	SAVE_ABI_STATE
+	jalr	t5
+	STORE_ABI_STATE
+	ret
+ENDPROC(_mcount)
+EXPORT_SYMBOL(_mcount)
-- 
2.15.1

Re: [patches] [PATCH v2] riscv/ftrace: Add basic support

[Palmer Dabbelt]

On Wed, 06 Dec 2017 18:31:10 PST (-0800), nonerkao@gmail.com wrote:
> This patch contains basic ftrace support for RV64I platform.
> Specifically, function tracer (HAVE_FUNCTION_TRACER), function graph
> tracer (HAVE_FUNCTION_GRAPH_TRACER), and a frame pointer test
> (HAVE_FUNCTION_GRAPH_FP_TEST) are implemented following the
> instructions in Documentation/trace/ftrace-design.txt.
>
> Note that the functions in both ftrace.c and setup.c should not be
> hooked with the compiler's -pg option: to prevent infinite self-
> referencing for the former, and to ignore early setup stuff for the
> latter.
>
> Signed-off-by: Alan Kao <alankao@andestech.com>
> ---
> Changes in v2:
>   - Add SPDX license identifier
>   - Remove unnecessary LOCKDEP_SUPPORT option
>   - Remove unnecessary #ifdef in the newly added kernel/ftrace.c
>
>  arch/riscv/Kconfig              |   5 ++
>  arch/riscv/include/asm/Kbuild   |   1 -
>  arch/riscv/include/asm/ftrace.h |  10 ++++
>  arch/riscv/kernel/Makefile      |   7 +++
>  arch/riscv/kernel/ftrace.c      |  41 +++++++++++++
>  arch/riscv/kernel/mcount.S      | 126 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 189 insertions(+), 1 deletion(-)
>  create mode 100644 arch/riscv/include/asm/ftrace.h
>  create mode 100644 arch/riscv/kernel/ftrace.c
>  create mode 100644 arch/riscv/kernel/mcount.S
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 2e15e85c8f7e..40a67fc12328 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -60,6 +60,9 @@ config PAGE_OFFSET
>  config STACKTRACE_SUPPORT
>  	def_bool y
>
> +config TRACE_IRQFLAGS_SUPPORT
> +	def_bool y
> +
>  config RWSEM_GENERIC_SPINLOCK
>  	def_bool y
>
> @@ -112,6 +115,8 @@ config ARCH_RV64I
>  	bool "RV64I"
>  	select CPU_SUPPORTS_64BIT_KERNEL
>  	select 64BIT
> +	select HAVE_FUNCTION_TRACER
> +	select HAVE_FUNCTION_GRAPH_TRACER
>
>  endchoice
>
> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> index 18158be62a2b..680301bfbc4b 100644
> --- a/arch/riscv/include/asm/Kbuild
> +++ b/arch/riscv/include/asm/Kbuild
> @@ -12,7 +12,6 @@ generic-y += errno.h
>  generic-y += exec.h
>  generic-y += fb.h
>  generic-y += fcntl.h
> -generic-y += ftrace.h
>  generic-y += futex.h
>  generic-y += hardirq.h
>  generic-y += hash.h
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> new file mode 100644
> index 000000000000..66d4175eb13e
> --- /dev/null
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2017 Andes Technology Corporation */
> +
> +/*
> + * The graph frame test is not possible if CONFIG_FRAME_POINTER is not enabled.
> + * Check arch/riscv/kernel/mcount.S for detail.
> + */
> +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && defined(CONFIG_FRAME_POINTER)
> +#define HAVE_FUNCTION_GRAPH_FP_TEST
> +#endif
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index ab8baf7bd142..15941f3b8363 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -2,6 +2,11 @@
>  # Makefile for the RISC-V Linux kernel
>  #
>
> +#ifdef CONFIG_FTRACE
> +CFLAGS_REMOVE_ftrace.o = -pg
> +CFLAGS_REMOVE_setup.o = -pg
> +#endif
> +

You want ifeq, not #ifdef, in Makefiles.

Also: I thought we were only unable to trace setup_vm()?  Either way, I don't 
think it's a big deal to avoid tracing anything in setup.c: the stuff in here 
is called once pet hart before userspace is set up, so I doubt anyone is going 
to want to trace it anyway.

>  extra-y += head.o
>  extra-y += vmlinux.lds
>
> @@ -29,5 +34,7 @@ CFLAGS_setup.o := -mcmodel=medany
>  obj-$(CONFIG_SMP)		+= smpboot.o
>  obj-$(CONFIG_SMP)		+= smp.o
>  obj-$(CONFIG_MODULES)		+= module.o
> +obj-$(CONFIG_FUNCTION_TRACER)	+= mcount.o
> +obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
>
>  clean:
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> new file mode 100644
> index 000000000000..d0de68d144cb
> --- /dev/null
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2013 Linaro Limited
> + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> + * Copyright (C) 2017 Andes Technology Corporation
> + */
> +
> +#include <linux/ftrace.h>
> +
> +/*
> + * Most of this file is copied from arm64.
> + */
> +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
> +			   unsigned long frame_pointer)
> +{
> +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> +	unsigned long old;
> +	struct ftrace_graph_ent trace;
> +	int err;
> +
> +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +		return;
> +
> +	/*
> +	 * We don't suffer access faults, so no extra fault-recovery assembly
> +	 * is needed here.
> +	 */
> +	old = *parent;
> +
> +	trace.func = self_addr;
> +	trace.depth = current->curr_ret_stack + 1;
> +
> +	if (!ftrace_graph_entry(&trace))
> +		return;
> +
> +	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
> +				       frame_pointer, NULL);
> +	if (err == -EBUSY)
> +		return;
> +	*parent = return_hooker;
> +}

This looks almost exactly like the arm64 version, except they're setting 
"*parent = old" a few more times.  I can't actually figure out why: they also 
set "old = *parent", and they never seem to modify it.  Can you explain the 
discrepancy?

> diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> new file mode 100644
> index 000000000000..32f2432202e8
> --- /dev/null
> +++ b/arch/riscv/kernel/mcount.S
> @@ -0,0 +1,126 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2017 Andes Technology Corporation */
> +
> +#include <linux/init.h>
> +#include <linux/linkage.h>
> +#include <asm/asm.h>
> +#include <asm/csr.h>
> +#include <asm/unistd.h>
> +#include <asm/thread_info.h>
> +#include <asm/asm-offsets.h>
> +#include <asm-generic/export.h>
> +#include <asm/ftrace.h>
> +
> +	.text
> +
> +	.macro SAVE_ABI_STATE
> +	addi	sp, sp, -16
> +	sd	s0, 0(sp)
> +	sd	ra, 8(sp)
> +	addi	s0, sp, 16
> +	.endm
> +
> +	/*
> +	 * The call to ftrace_return_to_handler would overwrite the return
> +	 * register if a0 was not saved.
> +	 */
> +	.macro SAVE_RET_ABI_STATE
> +	addi	sp, sp, -32
> +	sd	s0, 16(sp)
> +	sd	ra, 24(sp)

Is there any reason these happen out of order?

> +	sd	a0, 8(sp)
> +	addi	s0, sp, 32
> +	.endm
> +
> +	.macro STORE_ABI_STATE
> +	ld	ra, 8(sp)
> +	ld	s0, 0(sp)
> +	addi	sp, sp, 16
> +	.endm
> +
> +	.macro STORE_RET_ABI_STATE
> +	ld	ra, 24(sp)
> +	ld	s0, 16(sp)
> +	ld	a0, 8(sp)
> +	addi	sp, sp, 32
> +	.endm
> +
> +ENTRY(ftrace_stub)
> +	ret
> +ENDPROC(ftrace_stub)
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +ENTRY(return_to_handler)
> +/*
> + * On implementing the frame point test, the ideal way is to compare the
> + * s0 (frame pointer, if enabled) on entry and the sp (stack pointer) on return.
> + * However, the psABI of variable-length-argument functions does not allow this.
> + *
> + * So alternatively we check the *old* frame pointer position, that is, the
> + * value stored in -16(s0) on entry, and the s0 on return.
> + */
> +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> +	mv	t6, s0
> +#endif
> +	SAVE_RET_ABI_STATE
> +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> +	mv	a0, t6
> +#endif
> +	la	t0, ftrace_return_to_handler
> +	jalr	t0
> +	mv	a1, a0
> +	STORE_RET_ABI_STATE
> +	jr	a1
> +ENDPROC(return_to_handler)
> +EXPORT_SYMBOL(return_to_handler)
> +#endif
> +
> +ENTRY(_mcount)
> +	la	t4, ftrace_stub
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	la	t0, ftrace_graph_return
> +	ld	t1, 0(t0)
> +	bne	t1, t4, do_ftrace_graph_caller
> +
> +	la	t3, ftrace_graph_entry
> +	ld	t2, 0(t3)
> +	la	t6, ftrace_graph_entry_stub
> +	bne	t2, t6, do_ftrace_graph_caller
> +#endif
> +	la	t3, ftrace_trace_function
> +	ld	t5, 0(t3)
> +	bne	t5, t4, do_trace
> +	ret

Since this is a performance-critical function, it'd be good to have it 
optimized.  I notice two things:

* There's no early out.
* You can save an instruction when addressing by using somethingl like "ld t1, 
  ftrace_graph_return" instead of "la t0, ftrace_graph_return; ld t1 0(t0)".

It's not a big deal, though -- we can fix these later.  The more interesting 
thing here is that this code means our `-pg` stuff is now part of the GCC ABI, 
which is something I'd never though of before.  I've added Jim, our GCC guy.

Jim: do you mind checking to make sure the GCC profiling support is sane?  
Specifically, I'm thinking:

* Are there any profiling features we don't support that would require an ABI 
 break?
* Is there a way to add future ISA extensions without breaking the ABI?
* Should we document this as part of the ELF psABI specification?

Even though this isn't user-visible as far an Linux is concerned, it'd be a bit 
of a pain to have to break this ABI because we did something brain-dead.  Since 
there's a bit of time before 7.3.0, I think it'd be OK to consider breaking the 
profiling ABI if there's a good reason.

> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +/*
> + * A pseudo representation for the function graph tracer:
> + * prepare_to_return(&ra_to_caller_of_caller, ra_to_caller)
> + */
> +do_ftrace_graph_caller:
> +	addi	a0, s0, -8
> +	mv	a1, ra
> +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> +	ld	a2, -16(s0)
> +#endif
> +	SAVE_ABI_STATE
> +	la	t0, prepare_ftrace_return
> +	jalr	t0
> +	STORE_ABI_STATE
> +	ret
> +#endif
> +
> +/*
> + * A pseudo representation for the function tracer:
> + * (*ftrace_trace_function)(ra_to_caller, ra_to_caller_of_caller)
> + */
> +do_trace:
> +	ld	a1, -8(s0)
> +	mv	a0, ra
> +
> +	SAVE_ABI_STATE
> +	jalr	t5
> +	STORE_ABI_STATE
> +	ret
> +ENDPROC(_mcount)
> +EXPORT_SYMBOL(_mcount)

Thanks!

I don't know much about ftrace, but I'm OK taking this into our 
"for-linux-next" tree so this gets more visibility.  As we get userspace a bit 
saner then I'll give it another look.

I've put this here for now:

  https://github.com/riscv/riscv-linux/pull/117

Re: [PATCH v2] riscv/ftrace: Add basic support

[Alan Kao]

On Mon, Dec 11, 2017 at 10:15:58AM -0800, Palmer Dabbelt wrote:
> On Wed, 06 Dec 2017 18:31:10 PST (-0800), nonerkao@gmail.com wrote:
> > This patch contains basic ftrace support for RV64I platform.
> > Specifically, function tracer (HAVE_FUNCTION_TRACER), function graph
> > tracer (HAVE_FUNCTION_GRAPH_TRACER), and a frame pointer test
> > (HAVE_FUNCTION_GRAPH_FP_TEST) are implemented following the
> > instructions in Documentation/trace/ftrace-design.txt.
> > 
> > Note that the functions in both ftrace.c and setup.c should not be
> > hooked with the compiler's -pg option: to prevent infinite self-
> > referencing for the former, and to ignore early setup stuff for the
> > latter.
> > 
> > Signed-off-by: Alan Kao <alankao@andestech.com>
> > ---
> > Changes in v2:
> >   - Add SPDX license identifier
> >   - Remove unnecessary LOCKDEP_SUPPORT option
> >   - Remove unnecessary #ifdef in the newly added kernel/ftrace.c
> > 
> >  arch/riscv/Kconfig              |   5 ++
> >  arch/riscv/include/asm/Kbuild   |   1 -
> >  arch/riscv/include/asm/ftrace.h |  10 ++++
> >  arch/riscv/kernel/Makefile      |   7 +++
> >  arch/riscv/kernel/ftrace.c      |  41 +++++++++++++
> >  arch/riscv/kernel/mcount.S      | 126 ++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 189 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/riscv/include/asm/ftrace.h
> >  create mode 100644 arch/riscv/kernel/ftrace.c
> >  create mode 100644 arch/riscv/kernel/mcount.S
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 2e15e85c8f7e..40a67fc12328 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -60,6 +60,9 @@ config PAGE_OFFSET
> >  config STACKTRACE_SUPPORT
> >  	def_bool y
> > 
> > +config TRACE_IRQFLAGS_SUPPORT
> > +	def_bool y
> > +
> >  config RWSEM_GENERIC_SPINLOCK
> >  	def_bool y
> > 
> > @@ -112,6 +115,8 @@ config ARCH_RV64I
> >  	bool "RV64I"
> >  	select CPU_SUPPORTS_64BIT_KERNEL
> >  	select 64BIT
> > +	select HAVE_FUNCTION_TRACER
> > +	select HAVE_FUNCTION_GRAPH_TRACER
> > 
> >  endchoice
> > 
> > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > index 18158be62a2b..680301bfbc4b 100644
> > --- a/arch/riscv/include/asm/Kbuild
> > +++ b/arch/riscv/include/asm/Kbuild
> > @@ -12,7 +12,6 @@ generic-y += errno.h
> >  generic-y += exec.h
> >  generic-y += fb.h
> >  generic-y += fcntl.h
> > -generic-y += ftrace.h
> >  generic-y += futex.h
> >  generic-y += hardirq.h
> >  generic-y += hash.h
> > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > new file mode 100644
> > index 000000000000..66d4175eb13e
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/ftrace.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2017 Andes Technology Corporation */
> > +
> > +/*
> > + * The graph frame test is not possible if CONFIG_FRAME_POINTER is not enabled.
> > + * Check arch/riscv/kernel/mcount.S for detail.
> > + */
> > +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && defined(CONFIG_FRAME_POINTER)
> > +#define HAVE_FUNCTION_GRAPH_FP_TEST
> > +#endif
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index ab8baf7bd142..15941f3b8363 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -2,6 +2,11 @@
> >  # Makefile for the RISC-V Linux kernel
> >  #
> > 
> > +#ifdef CONFIG_FTRACE
> > +CFLAGS_REMOVE_ftrace.o = -pg
> > +CFLAGS_REMOVE_setup.o = -pg
> > +#endif
> > +
> 
> You want ifeq, not #ifdef, in Makefiles.
>

Thanks for pointing out this. It will be fixed in v3.

> Also: I thought we were only unable to trace setup_vm()?  Either way, I
> don't think it's a big deal to avoid tracing anything in setup.c: the stuff
> in here is called once pet hart before userspace is set up, so I doubt
> anyone is going to want to trace it anyway.
>

Sure. setup_vm() is the only trouble that causes panic.

> >  extra-y += head.o
> >  extra-y += vmlinux.lds
> > 
> > @@ -29,5 +34,7 @@ CFLAGS_setup.o := -mcmodel=medany
> >  obj-$(CONFIG_SMP)		+= smpboot.o
> >  obj-$(CONFIG_SMP)		+= smp.o
> >  obj-$(CONFIG_MODULES)		+= module.o
> > +obj-$(CONFIG_FUNCTION_TRACER)	+= mcount.o
> > +obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
> > 
> >  clean:
> > diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> > new file mode 100644
> > index 000000000000..d0de68d144cb
> > --- /dev/null
> > +++ b/arch/riscv/kernel/ftrace.c
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2013 Linaro Limited
> > + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > + * Copyright (C) 2017 Andes Technology Corporation
> > + */
> > +
> > +#include <linux/ftrace.h>
> > +
> > +/*
> > + * Most of this file is copied from arm64.
> > + */
> > +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
> > +			   unsigned long frame_pointer)
> > +{
> > +	unsigned long return_hooker = (unsigned long)&return_to_handler;
> > +	unsigned long old;
> > +	struct ftrace_graph_ent trace;
> > +	int err;
> > +
> > +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> > +		return;
> > +
> > +	/*
> > +	 * We don't suffer access faults, so no extra fault-recovery assembly
> > +	 * is needed here.
> > +	 */
> > +	old = *parent;
> > +
> > +	trace.func = self_addr;
> > +	trace.depth = current->curr_ret_stack + 1;
> > +
> > +	if (!ftrace_graph_entry(&trace))
> > +		return;
> > +
> > +	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
> > +				       frame_pointer, NULL);
> > +	if (err == -EBUSY)
> > +		return;
> > +	*parent = return_hooker;
> > +}
> 
> This looks almost exactly like the arm64 version, except they're setting
> "*parent = old" a few more times.  I can't actually figure out why: they
> also set "old = *parent", and they never seem to modify it.  Can you explain
> the discrepancy?
>

I don't see what you say here. The only difference between arm64's and this is 
that they have a *else* case for the error check of -EBUSY. I remove that else
keyword because an else after return is not necessary.

> > diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> > new file mode 100644
> > index 000000000000..32f2432202e8
> > --- /dev/null
> > +++ b/arch/riscv/kernel/mcount.S
> > @@ -0,0 +1,126 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2017 Andes Technology Corporation */
> > +
> > +#include <linux/init.h>
> > +#include <linux/linkage.h>
> > +#include <asm/asm.h>
> > +#include <asm/csr.h>
> > +#include <asm/unistd.h>
> > +#include <asm/thread_info.h>
> > +#include <asm/asm-offsets.h>
> > +#include <asm-generic/export.h>
> > +#include <asm/ftrace.h>
> > +
> > +	.text
> > +
> > +	.macro SAVE_ABI_STATE
> > +	addi	sp, sp, -16
> > +	sd	s0, 0(sp)
> > +	sd	ra, 8(sp)
> > +	addi	s0, sp, 16
> > +	.endm
> > +
> > +	/*
> > +	 * The call to ftrace_return_to_handler would overwrite the return
> > +	 * register if a0 was not saved.
> > +	 */
> > +	.macro SAVE_RET_ABI_STATE
> > +	addi	sp, sp, -32
> > +	sd	s0, 16(sp)
> > +	sd	ra, 24(sp)
> 
> Is there any reason these happen out of order?
>

I once de-assembled the whole vmlinux as a reference, and the
store-s0-before-ra sequence is quite normal everywhere.

Just for reference, I develop this and ongoing ftrace patches based on:
* riscv-gcc		b4dae89
* riscv-glibc		bff4ec0
* riscv-binutils	9790f45

> > +	sd	a0, 8(sp)
> > +	addi	s0, sp, 32
> > +	.endm
> > +
> > +	.macro STORE_ABI_STATE
> > +	ld	ra, 8(sp)
> > +	ld	s0, 0(sp)
> > +	addi	sp, sp, 16
> > +	.endm
> > +
> > +	.macro STORE_RET_ABI_STATE
> > +	ld	ra, 24(sp)
> > +	ld	s0, 16(sp)
> > +	ld	a0, 8(sp)
> > +	addi	sp, sp, 32
> > +	.endm
> > +
> > +ENTRY(ftrace_stub)
> > +	ret
> > +ENDPROC(ftrace_stub)
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +ENTRY(return_to_handler)
> > +/*
> > + * On implementing the frame point test, the ideal way is to compare the
> > + * s0 (frame pointer, if enabled) on entry and the sp (stack pointer) on return.
> > + * However, the psABI of variable-length-argument functions does not allow this.
> > + *
> > + * So alternatively we check the *old* frame pointer position, that is, the
> > + * value stored in -16(s0) on entry, and the s0 on return.
> > + */
> > +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> > +	mv	t6, s0
> > +#endif
> > +	SAVE_RET_ABI_STATE
> > +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> > +	mv	a0, t6
> > +#endif
> > +	la	t0, ftrace_return_to_handler
> > +	jalr	t0
> > +	mv	a1, a0
> > +	STORE_RET_ABI_STATE
> > +	jr	a1
> > +ENDPROC(return_to_handler)
> > +EXPORT_SYMBOL(return_to_handler)
> > +#endif
> > +
> > +ENTRY(_mcount)
> > +	la	t4, ftrace_stub
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	la	t0, ftrace_graph_return
> > +	ld	t1, 0(t0)
> > +	bne	t1, t4, do_ftrace_graph_caller
> > +
> > +	la	t3, ftrace_graph_entry
> > +	ld	t2, 0(t3)
> > +	la	t6, ftrace_graph_entry_stub
> > +	bne	t2, t6, do_ftrace_graph_caller
> > +#endif
> > +	la	t3, ftrace_trace_function
> > +	ld	t5, 0(t3)
> > +	bne	t5, t4, do_trace
> > +	ret
> 
> Since this is a performance-critical function, it'd be good to have it
> optimized.  I notice two things:
>
> * There's no early out.

I don't understand what the "early out" means here.  Do you mean
something like the short circuit evaluation?  That's because, there may
be cases that a system supports CONFIG_FUNCTION_GRAPH_TRACER but enables
the function tracer only, so after the evaluation at line 295, the flow
goes to the function tracer part instead of returning.

> * You can save an instruction when addressing by using somethingl like "ld
> t1,  ftrace_graph_return" instead of "la t0, ftrace_graph_return; ld t1
> 0(t0)".
>

Thanks. I will take care of this.

> It's not a big deal, though -- we can fix these later.  The more interesting
> thing here is that this code means our `-pg` stuff is now part of the GCC
> ABI, which is something I'd never though of before.  I've added Jim, our GCC
> guy.
> 
> Jim: do you mind checking to make sure the GCC profiling support is sane?
> Specifically, I'm thinking:
> 
> * Are there any profiling features we don't support that would require an
> ABI break?
> * Is there a way to add future ISA extensions without breaking the ABI?
> * Should we document this as part of the ELF psABI specification?
> 
> Even though this isn't user-visible as far an Linux is concerned, it'd be a
> bit of a pain to have to break this ABI because we did something brain-dead.
> Since there's a bit of time before 7.3.0, I think it'd be OK to consider
> breaking the profiling ABI if there's a good reason.
> 

As far as I can tell, the `-pg` flag only inserts the _mcount call after every 
valid function prologue and seems breaking no existing ABI. But indeed
it would be good if compiler guys can take a look at the gcc profiling
features.

> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +/*
> > + * A pseudo representation for the function graph tracer:
> > + * prepare_to_return(&ra_to_caller_of_caller, ra_to_caller)
> > + */
> > +do_ftrace_graph_caller:
> > +	addi	a0, s0, -8
> > +	mv	a1, ra
> > +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> > +	ld	a2, -16(s0)
> > +#endif
> > +	SAVE_ABI_STATE
> > +	la	t0, prepare_ftrace_return
> > +	jalr	t0
> > +	STORE_ABI_STATE
> > +	ret
> > +#endif
> > +
> > +/*
> > + * A pseudo representation for the function tracer:
> > + * (*ftrace_trace_function)(ra_to_caller, ra_to_caller_of_caller)
> > + */
> > +do_trace:
> > +	ld	a1, -8(s0)
> > +	mv	a0, ra
> > +
> > +	SAVE_ABI_STATE
> > +	jalr	t5
> > +	STORE_ABI_STATE
> > +	ret
> > +ENDPROC(_mcount)
> > +EXPORT_SYMBOL(_mcount)
> 
> Thanks!
> 
> I don't know much about ftrace, but I'm OK taking this into our
> "for-linux-next" tree so this gets more visibility.  As we get userspace a
> bit saner then I'll give it another look.
> 
> I've put this here for now:
> 
>  https://github.com/riscv/riscv-linux/pull/117

Thanks for the feedback!

Just a side note, the dynamic ftrace part requires a lot more work than this 
static version, and will be ready soon as a patchset. 

Alan

Re: [PATCH v2] riscv/ftrace: Add basic support

[Jim Wilson]

On Mon, Dec 11, 2017 at 11:08 PM, Alan Kao <nonerkao@gmail.com> wrote:
> On Mon, Dec 11, 2017 at 10:15:58AM -0800, Palmer Dabbelt wrote:
>> It's not a big deal, though -- we can fix these later.  The more interesting
>> thing here is that this code means our `-pg` stuff is now part of the GCC
>> ABI, which is something I'd never though of before.  I've added Jim, our GCC
>> guy.
>>
>> Jim: do you mind checking to make sure the GCC profiling support is sane?
>> Specifically, I'm thinking:
>>
>> * Are there any profiling features we don't support that would require an
>> ABI break?
>> * Is there a way to add future ISA extensions without breaking the ABI?
>> * Should we document this as part of the ELF psABI specification?
>>
>> Even though this isn't user-visible as far an Linux is concerned, it'd be a
>> bit of a pain to have to break this ABI because we did something brain-dead.
>> Since there's a bit of time before 7.3.0, I think it'd be OK to consider
>> breaking the profiling ABI if there's a good reason.

It looks sane to me.  I don't have a proper linux environment to test
in, but simple statically linked binaries are working on the spike
simulator, and doing what I expect.  The call is after the prologue,
so no need to worry about mcount overwriting registers that the
prologue needs.

As Alan mentioned, all gcc does is call mcount with two args, parent
pc and self pc, same as most other linux targets.  Most of the
interesting features of prof/gprof profiling happen inside glibc, with
the special start files provided by glibc, and some special functions
like profil(3).  I think this is more of a glibc API issue than a gcc
ABI issue.  If the glibc API changes, then the kernel support will
have to change too.  I checked half a dozen different processor ABIs,
and I didn't find that one documents how mcount works.

Jim

Re: [PATCH v2] riscv/ftrace: Add basic support

[Steven Rostedt]

On Tue, 12 Dec 2017 15:08:00 +0800
Alan Kao <nonerkao@gmail.com> wrote:

> > It's not a big deal, though -- we can fix these later.  The more interesting
> > thing here is that this code means our `-pg` stuff is now part of the GCC
> > ABI, which is something I'd never though of before.  I've added Jim, our GCC
> > guy.
> > 
> > Jim: do you mind checking to make sure the GCC profiling support is sane?
> > Specifically, I'm thinking:
> > 
> > * Are there any profiling features we don't support that would require an
> > ABI break?
> > * Is there a way to add future ISA extensions without breaking the ABI?
> > * Should we document this as part of the ELF psABI specification?
> > 
> > Even though this isn't user-visible as far an Linux is concerned, it'd be a
> > bit of a pain to have to break this ABI because we did something brain-dead.
> > Since there's a bit of time before 7.3.0, I think it'd be OK to consider
> > breaking the profiling ABI if there's a good reason.
> >   
> 
> As far as I can tell, the `-pg` flag only inserts the _mcount call after every 
> valid function prologue and seems breaking no existing ABI. But indeed
> it would be good if compiler guys can take a look at the gcc profiling
> features.

This is an interesting discussion, although I'm a bit confused. What
ABI are you worried about breaking?

-- Steve

Re: [PATCH v2] riscv/ftrace: Add basic support

[Steven Rostedt]

On Tue, 12 Dec 2017 09:47:03 -0800
Jim Wilson <jimw@sifive.com> wrote:


> As Alan mentioned, all gcc does is call mcount with two args, parent
> pc and self pc, same as most other linux targets.  Most of the
> interesting features of prof/gprof profiling happen inside glibc, with
> the special start files provided by glibc, and some special functions
> like profil(3).  I think this is more of a glibc API issue than a gcc
> ABI issue.  If the glibc API changes, then the kernel support will
> have to change too.  I checked half a dozen different processor ABIs,
> and I didn't find that one documents how mcount works.

mcount appears to be totally undocumented in all archs. My way of
figuring out what it did was simply reading the glibc code and how it
handled it.

Note, gcc on some archs supports the -mfentry option added with -pg,
which will will not use "mcount" but instead "__fentry__" which comes
before the prologue. This allows for ftrace to hijack the function, and
this is how live kernel patching is implemented.

-- Steve

Re: [patches] [PATCH v2] riscv/ftrace: Add basic support

[Alan Kao]

On Mon, Dec 11, 2017 at 10:15:58AM -0800, Palmer Dabbelt wrote:
> On Wed, 06 Dec 2017 18:31:10 PST (-0800), nonerkao@gmail.com wrote:

Hi Palmer, 

I forgot to explain this section in the previous reply:

> > +ENTRY(_mcount)
> > +	la	t4, ftrace_stub
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	la	t0, ftrace_graph_return
> > +	ld	t1, 0(t0)
> > +	bne	t1, t4, do_ftrace_graph_caller
> > +
> > +	la	t3, ftrace_graph_entry
> > +	ld	t2, 0(t3)
> > +	la	t6, ftrace_graph_entry_stub
> > +	bne	t2, t6, do_ftrace_graph_caller
> > +#endif
> > +	la	t3, ftrace_trace_function
> > +	ld	t5, 0(t3)
> > +	bne	t5, t4, do_trace
> > +	ret
> 
> * You can save an instruction when addressing by using somethingl like "ld
> t1,  ftrace_graph_return" instead of "la t0, ftrace_graph_return; ld t1
> 0(t0)".
> 

There are three "la-ld" instruction pairs for loading ftrace_graph_return,
ftrace_graph_entry, and ftrace_trace_function.  All of them are function
pointers in C.  The problem here is that, if we applied an "ld" inst. to a
function pointer, we would have loaded the content, which would be the 
first 8 bytes in the function, rather than the address of the target function
that the function pointer stored before.

In brief, the logic of the "la-ld" pairs should be fine.

Alan