[PATCH] riscv/ftrace: Add basic support

[PATCH] 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>
---
 arch/riscv/Kconfig              |   8 +++
 arch/riscv/include/asm/Kbuild   |   1 -
 arch/riscv/include/asm/ftrace.h |  23 +++++++
 arch/riscv/kernel/Makefile      |   7 ++
 arch/riscv/kernel/ftrace.c      |  56 ++++++++++++++++
 arch/riscv/kernel/mcount.S      | 139 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 233 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..07c3df0919b7 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -60,6 +60,12 @@ config PAGE_OFFSET
 config STACKTRACE_SUPPORT
 	def_bool y
 
+config TRACE_IRQFLAGS_SUPPORT
+	def_bool y
+
+config LOCKDEP_SUPPORT
+	def_bool y
+
 config RWSEM_GENERIC_SPINLOCK
 	def_bool y
 
@@ -112,6 +118,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..38beadb07ad5
--- /dev/null
+++ b/arch/riscv/include/asm/ftrace.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2017 Andes Technology Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * 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..e425c8f3f1cd
--- /dev/null
+++ b/arch/riscv/kernel/ftrace.c
@@ -0,0 +1,56 @@
+/*
+ * arch/riscv/kernel/ftrace.c
+ *
+ * Copyright (C) 2013 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ * Copyright (C) 2017 Andes Technology Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/ftrace.h>
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+/*
+ * 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;
+}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
new file mode 100644
index 000000000000..5307358219bf
--- /dev/null
+++ b/arch/riscv/kernel/mcount.S
@@ -0,0 +1,139 @@
+/*
+ * Copyright (C) 2017 Andes Technology Corporation
+ * Author: Alan Kao <alankao@andestech.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#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.0

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

[Philippe Ombredanne]

On Thu, Nov 30, 2017 at 9:53 AM, Alan Kao <nonerkao@gmail.com> wrote:
[]
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> new file mode 100644
> index 000000000000..38beadb07ad5
> --- /dev/null
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright (C) 2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */

You may want to use the new SPDX ids here instead?
e.g.
> +// SPDX-License-Identifer: GPL-2.0
> +// Copyright (C) 2017 Andes Technology Corporation

Neater and simpler?
See Linus posts on the topic of comment styles too BTW

-- 
Cordially
Philippe Ombredanne

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

[Palmer Dabbelt]

On Thu, 30 Nov 2017 03:31:53 PST (-0800), pombredanne@nexb.com wrote:
> On Thu, Nov 30, 2017 at 9:53 AM, Alan Kao <nonerkao@gmail.com> wrote:
> []
>> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
>> new file mode 100644
>> index 000000000000..38beadb07ad5
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/ftrace.h
>> @@ -0,0 +1,23 @@
>> +/*
>> + * Copyright (C) 2017 Andes Technology Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>
> You may want to use the new SPDX ids here instead?
> e.g.
>> +// SPDX-License-Identifer: GPL-2.0
>> +// Copyright (C) 2017 Andes Technology Corporation
>
> Neater and simpler?
> See Linus posts on the topic of comment styles too BTW

I believe we need to convert all the RISC-V stuff to SPDX headers, that change 
happened between when we got the port reviewed and made it into Linus' tree.  
Do you happen to know if there's some script I can run to do this 
automatically?

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

[Steven Rostedt]

On Thu, 30 Nov 2017 16:53:35 +0800
Alan Kao <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.

Hmm, that document is somewhat out of date. Hopefully it was good
enough. I'm going to need some time to update it.

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

I'm curious to what is in setup.c that ftrace uses.

> 
> Signed-off-by: Alan Kao <alankao@andestech.com>
> ---
>  arch/riscv/Kconfig              |   8 +++
>  arch/riscv/include/asm/Kbuild   |   1 -
>  arch/riscv/include/asm/ftrace.h |  23 +++++++
>  arch/riscv/kernel/Makefile      |   7 ++
>  arch/riscv/kernel/ftrace.c      |  56 ++++++++++++++++
>  arch/riscv/kernel/mcount.S      | 139 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 233 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..07c3df0919b7 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -60,6 +60,12 @@ config PAGE_OFFSET
>  config STACKTRACE_SUPPORT
>  	def_bool y
>  
> +config TRACE_IRQFLAGS_SUPPORT
> +	def_bool y
> +
> +config LOCKDEP_SUPPORT
> +	def_bool y

Hmm, not sure the above is needed for function tracing.

> +
>  config RWSEM_GENERIC_SPINLOCK
>  	def_bool y
>  
> @@ -112,6 +118,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..38beadb07ad5
> --- /dev/null
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright (C) 2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * 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..e425c8f3f1cd
> --- /dev/null
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -0,0 +1,56 @@
> +/*
> + * arch/riscv/kernel/ftrace.c
> + *
> + * Copyright (C) 2013 Linaro Limited
> + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> + * Copyright (C) 2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/ftrace.h>
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER

This file only gets compiled when that config is set. This #ifdef is
somewhat redundant.

> +/*
> + * 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;
> +}
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> new file mode 100644
> index 000000000000..5307358219bf
> --- /dev/null
> +++ b/arch/riscv/kernel/mcount.S
> @@ -0,0 +1,139 @@
> +/*
> + * Copyright (C) 2017 Andes Technology Corporation
> + * Author: Alan Kao <alankao@andestech.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#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

If function graph is enabled, you jump straight to the graph tracer,
but never return back to here?

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

I'm guessing you don't support function tracer and function graph
tracer running at the same time?

-- Steve

> +	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)

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

[Alan Kao]

On Thu, Nov 30, 2017 at 12:31:53PM +0100, Philippe Ombredanne wrote:
> On Thu, Nov 30, 2017 at 9:53 AM, Alan Kao <nonerkao@gmail.com> wrote:
> []
> > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > new file mode 100644
> > index 000000000000..38beadb07ad5
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/ftrace.h
> > @@ -0,0 +1,23 @@
> > +/*
> > + * Copyright (C) 2017 Andes Technology Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> 
> You may want to use the new SPDX ids here instead?
> e.g.
> > +// SPDX-License-Identifer: GPL-2.0
> > +// Copyright (C) 2017 Andes Technology Corporation
> 
> Neater and simpler?
> See Linus posts on the topic of comment styles too BTW

Thanks for the tips.
I'll fix this in the next version.

Alan Kao

> 
> -- 
> Cordially
> Philippe Ombredanne

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

[Alan Kao]

On Thu, Nov 30, 2017 at 03:24:21PM -0500, Steven Rostedt wrote:
> On Thu, 30 Nov 2017 16:53:35 +0800
> Alan Kao <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.
> 
> Hmm, that document is somewhat out of date. Hopefully it was good
> enough. I'm going to need some time to update it.
> 
> > 
> > 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.
> 
> I'm curious to what is in setup.c that ftrace uses.

In the scenario for some embedded systems, the __init prefix does not give 
us the notrace feature without the MODULE config.  Therefore, all functions 
would have been hooked with the _mcount trampoline if the -pg flag was not 
specifically disabled.

And a terrible result would have happened after function setup_vm called
_mcount.  As _mcount compared the value of ftrace_trace_function and 
the position of ftrace_stub, it crashed the kernel because one of them 
was a physical address while the other was a virtual address but
actually they pointed to the same.

Adding notrace to setup_vm can solve the described issue, but it might be 
redundant once the MODULE config becomes stable and default on most 
platforms. To be honest, nobody really needs those init procedures to be
ftrace-able.

> 
> > 
> > Signed-off-by: Alan Kao <alankao@andestech.com>
> > ---
> >  arch/riscv/Kconfig              |   8 +++
> >  arch/riscv/include/asm/Kbuild   |   1 -
> >  arch/riscv/include/asm/ftrace.h |  23 +++++++
> >  arch/riscv/kernel/Makefile      |   7 ++
> >  arch/riscv/kernel/ftrace.c      |  56 ++++++++++++++++
> >  arch/riscv/kernel/mcount.S      | 139 ++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 233 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..07c3df0919b7 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -60,6 +60,12 @@ config PAGE_OFFSET
> >  config STACKTRACE_SUPPORT
> >  	def_bool y
> >  
> > +config TRACE_IRQFLAGS_SUPPORT
> > +	def_bool y
> > +
> > +config LOCKDEP_SUPPORT
> > +	def_bool y
> 
> Hmm, not sure the above is needed for function tracing.
>

FTRACE depends on TRACING_SUPPORT, and TRACING_SUPPORT depends on
TRACE_IRQFLAGS_SUPPORT. But LOCKDEP_SUPPORT is not actually needed
for any of the ftrace features implemented in this patch.

The LOCKDEP_SUPPORT will be removed in the next version.

> > +
> >  config RWSEM_GENERIC_SPINLOCK
> >  	def_bool y
> >  
> > @@ -112,6 +118,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..38beadb07ad5
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/ftrace.h
> > @@ -0,0 +1,23 @@
> > +/*
> > + * Copyright (C) 2017 Andes Technology Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/*
> > + * 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..e425c8f3f1cd
> > --- /dev/null
> > +++ b/arch/riscv/kernel/ftrace.c
> > @@ -0,0 +1,56 @@
> > +/*
> > + * arch/riscv/kernel/ftrace.c
> > + *
> > + * Copyright (C) 2013 Linaro Limited
> > + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > + * Copyright (C) 2017 Andes Technology Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/ftrace.h>
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> 
> This file only gets compiled when that config is set. This #ifdef is
> somewhat redundant.
>

OK.

> > +/*
> > + * 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;
> > +}
> > +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> > diff --git a/arch/riscv/kernel/mcount.S b/arch/riscv/kernel/mcount.S
> > new file mode 100644
> > index 000000000000..5307358219bf
> > --- /dev/null
> > +++ b/arch/riscv/kernel/mcount.S
> > @@ -0,0 +1,139 @@
> > +/*
> > + * Copyright (C) 2017 Andes Technology Corporation
> > + * Author: Alan Kao <alankao@andestech.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#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
> 
> If function graph is enabled, you jump straight to the graph tracer,
> but never return back to here?
>

Because prepare_ftrace_return function can return to the caller of
_mcount directly without messing up the stack.

> > +
> > +	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
> 
> I'm guessing you don't support function tracer and function graph
> tracer running at the same time?
> 
> -- Steve
> 

This code section implements similar logic as those for arm, arm64,
blackfin, and others.  Also, according to Documentation/trace/ftrace.txt,
the current_tracer is introduced as singular.

Is it necessary to support simultaneous tracers?

> > +	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)
> 

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

[Steven Rostedt]

On Mon, 4 Dec 2017 13:52:30 +0800
Alan Kao <nonerkao@gmail.com> wrote:

> > > 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.  
> > 
> > I'm curious to what is in setup.c that ftrace uses.  
> 
> In the scenario for some embedded systems, the __init prefix does not give 
> us the notrace feature without the MODULE config.  Therefore, all functions 
> would have been hooked with the _mcount trampoline if the -pg flag was not 
> specifically disabled.

But is there functions you may want to trace. There's an effort going
on to allow function tracing to start in early boot up.

> 
> And a terrible result would have happened after function setup_vm called
> _mcount.  As _mcount compared the value of ftrace_trace_function and 
> the position of ftrace_stub, it crashed the kernel because one of them 
> was a physical address while the other was a virtual address but
> actually they pointed to the same.
> 
> Adding notrace to setup_vm can solve the described issue, but it might be 
> redundant once the MODULE config becomes stable and default on most 
> platforms. To be honest, nobody really needs those init procedures to be
> ftrace-able.

Um no, because MODULE init code can now be traced. It use to be that we
didn't trace any __init, but I worked on having both inits be traced.
The module code was a little bit trickier because it can be loaded
multiple times and we needed to figure out the best way to handle init
functions in the buffer that went stale and is replaced by other module
init functions.

 
> > > +config TRACE_IRQFLAGS_SUPPORT
> > > +	def_bool y
> > > +
> > > +config LOCKDEP_SUPPORT
> > > +	def_bool y  
> > 
> > Hmm, not sure the above is needed for function tracing.
> >  
> 
> FTRACE depends on TRACING_SUPPORT, and TRACING_SUPPORT depends on
> TRACE_IRQFLAGS_SUPPORT. But LOCKDEP_SUPPORT is not actually needed
> for any of the ftrace features implemented in this patch.

Hmm, I think that's stale. Thanks for bringing that to my attention,
and don't believe that dependency still exists.

> 
> The LOCKDEP_SUPPORT will be removed in the next version.
>

I should have also asked, is lockdep really supported on this arch, and
is IRQSFLAGS really supported too? I vaguely remember making ftrace
depend on IRQFLAGS because we wanted archs to support TRACE_IRQFLAGS
before they supported ftrace. Maybe I'll keep that dependency.

> > > +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  
> > 
> > If function graph is enabled, you jump straight to the graph tracer,
> > but never return back to here?
> >  
> 
> Because prepare_ftrace_return function can return to the caller of
> _mcount directly without messing up the stack.

Yes, is that required?

> 
> > > +
> > > +	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  
> > 
> > I'm guessing you don't support function tracer and function graph
> > tracer running at the same time?
> > 
> > -- Steve
> >   
> 
> This code section implements similar logic as those for arm, arm64,
> blackfin, and others.  Also, according to Documentation/trace/ftrace.txt,
> the current_tracer is introduced as singular.
> 
> Is it necessary to support simultaneous tracers?

Well, you can do things like have multiple buffers today (different
tracers recording in different buffers). We can have function tracing
happening at the same time as the graph tracer.

Is this a requirement? No. Just letting you know.

While you only support static ftrace, and not dynamic (code modifying)
ftrace, this isn't yet an issue. I'm just trying to let you know of
some of the current features that are supported in other archs, in case
you extend this.

-- Steve

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

[Alan Kao]

On Mon, Dec 04, 2017 at 03:05:09AM -0500, Steven Rostedt wrote:
> On Mon, 4 Dec 2017 13:52:30 +0800
> Alan Kao <nonerkao@gmail.com> wrote:
> 
> > > > 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.  
> > > 
> > > I'm curious to what is in setup.c that ftrace uses.  
> > 
> > In the scenario for some embedded systems, the __init prefix does not give 
> > us the notrace feature without the MODULE config.  Therefore, all functions 
> > would have been hooked with the _mcount trampoline if the -pg flag was not 
> > specifically disabled.
> 
> But is there functions you may want to trace. There's an effort going
> on to allow function tracing to start in early boot up.
> 

Fair enough, but no. Those (in setup.c) are very early stage functions. 
Unless Palmer has different opinion on this, making all of them notrace
should be ok.

> > 
> > And a terrible result would have happened after function setup_vm called
> > _mcount.  As _mcount compared the value of ftrace_trace_function and 
> > the position of ftrace_stub, it crashed the kernel because one of them 
> > was a physical address while the other was a virtual address but
> > actually they pointed to the same.
> > 
> > Adding notrace to setup_vm can solve the described issue, but it might be 
> > redundant once the MODULE config becomes stable and default on most 
> > platforms. To be honest, nobody really needs those init procedures to be
> > ftrace-able.
> 
> Um no, because MODULE init code can now be traced. It use to be that we
> didn't trace any __init, but I worked on having both inits be traced.
> The module code was a little bit trickier because it can be loaded
> multiple times and we needed to figure out the best way to handle init
> functions in the buffer that went stale and is replaced by other module
> init functions.
> 

Thanks for the explanation. But sorry for being unclear, I didn't mean
all the init procedures, but only those in setup.c.

>  
> > > > +config TRACE_IRQFLAGS_SUPPORT
> > > > +	def_bool y
> > > > +
> > > > +config LOCKDEP_SUPPORT
> > > > +	def_bool y  
> > > 
> > > Hmm, not sure the above is needed for function tracing.
> > >  
> > 
> > FTRACE depends on TRACING_SUPPORT, and TRACING_SUPPORT depends on
> > TRACE_IRQFLAGS_SUPPORT. But LOCKDEP_SUPPORT is not actually needed
> > for any of the ftrace features implemented in this patch.
> 
> Hmm, I think that's stale. Thanks for bringing that to my attention,
> and don't believe that dependency still exists.
> 
> > 
> > The LOCKDEP_SUPPORT will be removed in the next version.
> >
> 
> I should have also asked, is lockdep really supported on this arch, and
> is IRQSFLAGS really supported too? I vaguely remember making ftrace
> depend on IRQFLAGS because we wanted archs to support TRACE_IRQFLAGS
> before they supported ftrace. Maybe I'll keep that dependency.

We do have the implementation of IRQFLAGS in
arch/riscv/include/asm/irqflags.  But I'm not sure about LOCKDEP.
 
> > > > +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  
> > > 
> > > If function graph is enabled, you jump straight to the graph tracer,
> > > but never return back to here?
> > >  
> > 
> > Because prepare_ftrace_return function can return to the caller of
> > _mcount directly without messing up the stack.
> 
> Yes, is that required?
>

I don't get your point here. Are you suggesting that a call is better than 
a jump here, for future extension towards dynamic tracing support? 

> > 
> > > > +
> > > > +	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  
> > > 
> > > I'm guessing you don't support function tracer and function graph
> > > tracer running at the same time?
> > > 
> > > -- Steve
> > >   
> > 
> > This code section implements similar logic as those for arm, arm64,
> > blackfin, and others.  Also, according to Documentation/trace/ftrace.txt,
> > the current_tracer is introduced as singular.
> > 
> > Is it necessary to support simultaneous tracers?
> 
> Well, you can do things like have multiple buffers today (different
> tracers recording in different buffers). We can have function tracing
> happening at the same time as the graph tracer.
> 
> Is this a requirement? No. Just letting you know.
> 
> While you only support static ftrace, and not dynamic (code modifying)
> ftrace, this isn't yet an issue. I'm just trying to let you know of
> some of the current features that are supported in other archs, in case
> you extend this.
> 
> -- Steve
> 
Thanks for the information. I am now working on the dynamic part.
Knowing that the current documents are outdated, I will reference the
implementations of other architectures much more carefully.

If no other comments on this patch, the v2 of this will be ready soon.

Many thanks,
Alan