| Message ID | 20081116212515.331278109@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Steven Rostedt writes: > +static int test_24bit_addr(unsigned long ip, unsigned long addr) > +{ > + unsigned long diff; > + > + /* can we get to addr from ip in 24 bits? */ > + diff = ip > addr ? ip - addr : addr - ip; > + > + return !(diff & ((unsigned long)-1 << 24)); Since a branch instruction can reach +/- 32MB, the 24 needs to be 25. It's 25 not 26 since you have effectively accounted for one bit by taking the absolute value of the offset. Also, this will say that an offset of -0x2000000 is out of range whereas it is just in range. That probably doesn't matter unless you're relying on this to give exactly the same answer in all cases as tests done elsewhere (e.g. in module_64.c). Note that the address difference being too large isn't the only reason for a procedure call to go via a trampoline on ppc64. A trampoline is also needed when the called function uses a different TOC from the caller. The most obvious example of this is when the caller is in a module and the callee is in the main kernel or another module. Currently all functions in the main kernel use the same TOC, but the linker is capable of dividing up a large executable into groups of functions and assigning a different TOC to each group, in order to avoid any of the TOCs getting too large (a TOC is limited to 64kB). If the linker does that, it automatically inserts trampolines to handle the TOC change. Paul.
On Mon, 17 Nov 2008, Paul Mackerras wrote: > Steven Rostedt writes: > > > +static int test_24bit_addr(unsigned long ip, unsigned long addr) > > +{ > > + unsigned long diff; > > + > > + /* can we get to addr from ip in 24 bits? */ > > + diff = ip > addr ? ip - addr : addr - ip; > > + > > + return !(diff & ((unsigned long)-1 << 24)); > > Since a branch instruction can reach +/- 32MB, the 24 needs to be 25. > It's 25 not 26 since you have effectively accounted for one bit by > taking the absolute value of the offset. Also, this will say that an > offset of -0x2000000 is out of range whereas it is just in range. > That probably doesn't matter unless you're relying on this to give > exactly the same answer in all cases as tests done elsewhere (e.g. in > module_64.c). I'll update this to be correct. > > Note that the address difference being too large isn't the only reason > for a procedure call to go via a trampoline on ppc64. A trampoline is > also needed when the called function uses a different TOC from the > caller. The most obvious example of this is when the caller is in a > module and the callee is in the main kernel or another module. > > Currently all functions in the main kernel use the same TOC, but the > linker is capable of dividing up a large executable into groups of > functions and assigning a different TOC to each group, in order to > avoid any of the TOCs getting too large (a TOC is limited to 64kB). > If the linker does that, it automatically inserts trampolines to > handle the TOC change. OK, I'll deal with that at a another time. If this does happen in core kernel code, it will produce a nice warning and nicely shutdown ftrace, without causing any harm. Then I can deal with the bug report. -- Steve
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h index b298f7a..17efecc 100644 --- a/arch/powerpc/include/asm/ftrace.h +++ b/arch/powerpc/include/asm/ftrace.h @@ -7,7 +7,19 @@ #ifndef __ASSEMBLY__ extern void _mcount(void); -#endif + +#ifdef CONFIG_DYNAMIC_FTRACE +static inline unsigned long ftrace_call_adjust(unsigned long addr) +{ + /* reloction of mcount call site is the same as the address */ + return addr; +} + +struct dyn_arch_ftrace { + /* nothing yet */ +}; +#endif /* CONFIG_DYNAMIC_FTRACE */ +#endif /* __ASSEMBLY__ */ #endif diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c index f4b006e..aa5d0b2 100644 --- a/arch/powerpc/kernel/ftrace.c +++ b/arch/powerpc/kernel/ftrace.c @@ -33,12 +33,12 @@ static unsigned int ftrace_calc_offset(long ip, long addr) return (int)(addr - ip); } -unsigned char *ftrace_nop_replace(void) +static unsigned char *ftrace_nop_replace(void) { return (char *)&ftrace_nop; } -unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr) +static unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr) { static unsigned int op; @@ -68,7 +68,7 @@ unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr) # define _ASM_PTR " .long " #endif -int +static int ftrace_modify_code(unsigned long ip, unsigned char *old_code, unsigned char *new_code) { @@ -113,6 +113,55 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code, return faulted; } +static int test_24bit_addr(unsigned long ip, unsigned long addr) +{ + unsigned long diff; + + /* can we get to addr from ip in 24 bits? */ + diff = ip > addr ? ip - addr : addr - ip; + + return !(diff & ((unsigned long)-1 << 24)); +} + +int ftrace_make_nop(struct module *mod, + struct dyn_ftrace *rec, unsigned long addr) +{ + unsigned char *old, *new; + + /* + * If the calling address is more that 24 bits away, + * then we had to use a trampoline to make the call. + * Otherwise just update the call site. + */ + if (test_24bit_addr(rec->ip, addr)) { + /* within range */ + old = ftrace_call_replace(rec->ip, addr); + new = ftrace_nop_replace(); + return ftrace_modify_code(rec->ip, old, new); + } + + return 0; +} + +int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) +{ + unsigned char *old, *new; + + /* + * If the calling address is more that 24 bits away, + * then we had to use a trampoline to make the call. + * Otherwise just update the call site. + */ + if (test_24bit_addr(rec->ip, addr)) { + /* within range */ + old = ftrace_nop_replace(); + new = ftrace_call_replace(rec->ip, addr); + return ftrace_modify_code(rec->ip, old, new); + } + + return 0; +} + int ftrace_update_ftrace_func(ftrace_func_t func) { unsigned long ip = (unsigned long)(&ftrace_call); @@ -128,9 +177,10 @@ int ftrace_update_ftrace_func(ftrace_func_t func) int __init ftrace_dyn_arch_init(void *data) { - /* This is running in kstop_machine */ + /* caller expects data to be zero */ + unsigned long *p = data; - ftrace_mcount_set(data); + *p = 0; return 0; }
Impact: change PPC to use new ftrace arch API This patch converts PPC to use the new dynamic ftrace arch API. Signed-off-by: Steven Rostedt <srostedt@redhat.com> --- arch/powerpc/include/asm/ftrace.h | 14 ++++++++- arch/powerpc/kernel/ftrace.c | 60 +++++++++++++++++++++++++++++++++--- 2 files changed, 68 insertions(+), 6 deletions(-)