Re: [patches] [PATCH v2] riscv/ftrace: Add basic support
On Mon, Dec 11, 2017 at 10:15:58AM -0800, Palmer Dabbelt wrote: > On Wed, 06 Dec 2017 18:31:10 PST (-0800), noner...@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
Re: [PATCH v2] riscv/ftrace: Add basic support
On Tue, 12 Dec 2017 09:47:03 -0800 Jim Wilson 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: [PATCH v2] riscv/ftrace: Add basic support
On Tue, 12 Dec 2017 15:08:00 +0800 Alan Kao 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
On Mon, Dec 11, 2017 at 11:08 PM, Alan Kao 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
On Mon, Dec 11, 2017 at 10:15:58AM -0800, Palmer Dabbelt wrote: > On Wed, 06 Dec 2017 18:31:10 PST (-0800), noner...@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 > > --- > > 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 ..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 ..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 > > + * Copyright (C) 2017 Andes Technology Corporation > > + */ > > + > > +#include > > + > > +/* > > + * 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 = (unsigne
Re: [patches] [PATCH v2] riscv/ftrace: Add basic support
On Wed, 06 Dec 2017 18:31:10 PST (-0800), noner...@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 --- 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 ..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 ..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 + * Copyright (C) 2017 Andes Technology Corporation + */ + +#include + +/* + * 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(¤t->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) +
[PATCH v2] riscv/ftrace: Add basic support
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 --- 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 ..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 ..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 + * Copyright (C) 2017 Andes Technology Corporation + */ + +#include + +/* + * 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(¤t->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 ..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 +#include +#include +#inclu