* Re: [PATCH] ftrace: mcount record based dynamic tracing for ARM
[not found] ` <20081121035509.GA5266@blackbean.org>
@ 2008-11-21 14:27 ` Steven Rostedt
2008-11-21 15:38 ` Russell King - ARM Linux
0 siblings, 1 reply; 4+ messages in thread
From: Steven Rostedt @ 2008-11-21 14:27 UTC (permalink / raw)
To: Jim Radford
Cc: Russell King, Ingo Molnar, Thomas Gleixner, Sam Ravnborg,
linux-arm-kernel, LKML
On Thu, 20 Nov 2008, Jim Radford wrote:
> commit 3b17e9f39304e266edf8ef05c4a01e31c6ecbf5c
> Author: Jim Radford <radford@galvanix.com>
> Date: Thu Nov 20 19:48:39 2008 -0800
>
> ftrace: enable dynamic ftrace for arm
>
> Update to the latest api and syncing functions with the x86 versions.
Again, Signed-off-by: is missing.
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 9722f8b..1b4162c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -16,7 +16,9 @@ config ARM
> select HAVE_ARCH_KGDB
> select HAVE_KPROBES if (!XIP_KERNEL)
> select HAVE_KRETPROBES if (HAVE_KPROBES)
> - select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> + select HAVE_FTRACE_MCOUNT_RECORD
> + select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)
Russell mentioned something about the code not being compatible with
Thumb2, is the above if statement enough?
> + select HAVE_FUNCTION_TRACER
> select HAVE_GENERIC_DMA_COHERENT
> help
> The ARM series is a line of low-power-consumption RISC chip designs
> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> index 39c8bc1..877c0d4 100644
> --- a/arch/arm/include/asm/ftrace.h
> +++ b/arch/arm/include/asm/ftrace.h
> @@ -7,6 +7,19 @@
>
> #ifndef __ASSEMBLY__
> extern void mcount(void);
> +
> +static inline unsigned long ftrace_call_adjust(unsigned long addr)
> +{
> + return addr;
> +}
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE
> +
> +struct dyn_arch_ftrace {
> + /* No extra data needed for x86 */
Do you mean: /* No extra data needed for ARM */
;-)
> +};
> +
> +#endif /* CONFIG_DYNAMIC_FTRACE */
> #endif
>
> #endif
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 06269ea..06216af 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -104,14 +104,7 @@ ENDPROC(ret_from_fork)
> #ifdef CONFIG_FUNCTION_TRACER
> #ifdef CONFIG_DYNAMIC_FTRACE
> ENTRY(mcount)
> - stmdb sp!, {r0-r3, lr}
> - mov r0, lr
> - sub r0, r0, #MCOUNT_INSN_SIZE
> -
> - .globl mcount_call
> -mcount_call:
> - bl ftrace_stub
> - ldmia sp!, {r0-r3, pc}
> + mov pc, lr
This looks OK. The new mcount stub for dynamic ftrace is a simple return.
It has been a while since I've programmed ARM (7 years ago), but I'm
assuming that loading the program counter with the link register is
a simple return.
>
> ENTRY(ftrace_caller)
> stmdb sp!, {r0-r3, lr}
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index 6c90479..d63b5a6 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -23,13 +23,13 @@
> static unsigned long bl_insn;
> static const unsigned long NOP = 0xe1a00000; /* mov r0, r0 */
>
> -unsigned char *ftrace_nop_replace(void)
> +static unsigned char *ftrace_nop_replace(void)
> {
> return (char *)&NOP;
> }
>
> /* construct a branch (BL) instruction to addr */
> -unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
> +static unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
> {
> long offset;
>
> @@ -46,7 +46,7 @@ unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
> return (unsigned char *)&bl_insn;
> }
>
> -int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
> +static int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
> unsigned char *new_code)
> {
The ftrace_modify_code could use a bit of clean up. Unless there is some
strange reason that it can not use the probe_kernel_ functions, it needs
to be updated. That approach is the most robust. Here's an example of
what it should look like:
{
unsigned char replaced[MCOUNT_INSN_SIZE];
/*
* Note: Due to modules and __init, code can
* disappear and change, we need to protect against faulting
* as well as code changing. We do this by using the
* probe_kernel_* functions.
*
* No real locking needed, this code is run through
* kstop_machine, or before SMP starts.
*/
/* read the text we want to modify */
if (probe_kernel_read(replaced, (void *)pc, MCOUNT_INSN_SIZE))
return -EFAULT;
/* Make sure it is what we expect it to be */
if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
return -EINVAL;
/* replace the text with the new text */
if (probe_kernel_write((void *)pc, new_code, MCOUNT_INSN_SIZE))
return -EPERM;
flush_icache_range(pc, pc + MCOUNT_INSN_SIZE);
return 0;
}
And the new design expects special return values:
-EFAULT: error reading the address
-EINVAL: what was at the address was not what we expected
-EPERM: could not write to the address
-- Steve
> unsigned long err = 0, replaced = 0, old, new;
> @@ -82,22 +82,46 @@ int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
> return err;
> }
>
> +int ftrace_make_nop(struct module *mod,
> + struct dyn_ftrace *rec, unsigned long addr)
> +{
> + unsigned char *new, *old;
> + unsigned long ip = rec->ip;
> +
> + old = ftrace_call_replace(ip, addr);
> + new = ftrace_nop_replace();
> +
> + return ftrace_modify_code(rec->ip, old, new);
> +}
> +
> +int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> +{
> + unsigned char *new, *old;
> + unsigned long ip = rec->ip;
> +
> + old = ftrace_nop_replace();
> + new = ftrace_call_replace(ip, addr);
> +
> + return ftrace_modify_code(rec->ip, old, new);
> +}
> +
> int ftrace_update_ftrace_func(ftrace_func_t func)
> {
> + unsigned long ip = (unsigned long)(&ftrace_call);
> + unsigned char old[MCOUNT_INSN_SIZE], *new;
> int ret;
> - unsigned long pc, old;
> - unsigned char *new;
>
> - pc = (unsigned long)&ftrace_call;
> - memcpy(&old, &ftrace_call, MCOUNT_INSN_SIZE);
> - new = ftrace_call_replace(pc, (unsigned long)func);
> - ret = ftrace_modify_code(pc, (unsigned char *)&old, new);
> + memcpy(old, &ftrace_call, sizeof(old));
> + new = ftrace_call_replace(ip, (unsigned long)func);
> + ret = ftrace_modify_code(ip, (unsigned char *)&old, new);
> +
> return ret;
> }
>
> /* run from kstop_machine */
> int __init ftrace_dyn_arch_init(void *data)
> {
> - ftrace_mcount_set(data);
> + /* The return code is retured via data */
> + *(unsigned long *)data = 0;
> return 0;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread