| Message ID | 20081120191149.285359258@goodmis.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Steven Rostedt writes: > Thanks to Paul Mackennas for pointing out the mistakes of my original Mackerras > +static int test_24bit_addr(unsigned long ip, unsigned long addr) > +{ > + long diff; > + > + /* > + * Can we get to addr from ip in 24 bits? > + * (26 really, since we mulitply by 4 for 4 byte alignment) > + */ > + diff = addr - ip; > + > + /* > + * Return true if diff is less than 1 << 25 > + * and greater than -1 << 26. > + */ > + return (diff < (1 << 25)) && (diff > (-1 << 26)); I think this still isn't right, and the comment is one of those ones that is only useful to people who can't read C, as it's just a transliteration of the code. The comment should say something like "Return true if diff can be represented as a 26-bit twos-complement binary number" and the second part of the test should be (diff >= (-1 << 25)). However, since you define a test_offset() function in patch 4/5 that does the same test but using only one comparison instead of two, why don't you just say: return !test_offset(diff); (having first moved test_offset() before test_24bit_addr)? Paul.
On Mon, 2008-11-24 at 12:43 +1100, Paul Mackerras wrote: > Steven Rostedt writes: > > > Thanks to Paul Mackennas for pointing out the mistakes of my original > > Mackerras > > > +static int test_24bit_addr(unsigned long ip, unsigned long addr) > > +{ > > + long diff; > > + > > + /* > > + * Can we get to addr from ip in 24 bits? > > + * (26 really, since we mulitply by 4 for 4 byte alignment) > > + */ > > + diff = addr - ip; > > + > > + /* > > + * Return true if diff is less than 1 << 25 > > + * and greater than -1 << 26. > > + */ > > + return (diff < (1 << 25)) && (diff > (-1 << 26)); > > I think this still isn't right, and the comment is one of those ones > that is only useful to people who can't read C, as it's just a > transliteration of the code. > > The comment should say something like "Return true if diff can be > represented as a 26-bit twos-complement binary number" and the second > part of the test should be (diff >= (-1 << 25)). However, since you > define a test_offset() function in patch 4/5 that does the same test > but using only one comparison instead of two, why don't you just say: > > return !test_offset(diff); > > (having first moved test_offset() before test_24bit_addr)? Or better still, split out and use the code from create_branch() in arch/powerpc/lib/code-patching.c .. which is hopefully correct :) cheers
On Mon, 24 Nov 2008, Paul Mackerras wrote: > Steven Rostedt writes: > > > Thanks to Paul Mackennas for pointing out the mistakes of my original > > Mackerras Heh, I have two reasons for that typo. 1) I reference Paul McKenney a lot, and my fingers are programmed. 2) I type with the dvorak layout, and the 'r' is just above the 'n'. qwerty 'o' == dvorak 'r' qwerty 'l' == dvorak 'n' ;-) > > > +static int test_24bit_addr(unsigned long ip, unsigned long addr) > > +{ > > + long diff; > > + > > + /* > > + * Can we get to addr from ip in 24 bits? > > + * (26 really, since we mulitply by 4 for 4 byte alignment) > > + */ > > + diff = addr - ip; > > + > > + /* > > + * Return true if diff is less than 1 << 25 > > + * and greater than -1 << 26. > > + */ > > + return (diff < (1 << 25)) && (diff > (-1 << 26)); > > I think this still isn't right, and the comment is one of those ones > that is only useful to people who can't read C, as it's just a > transliteration of the code. Argh!!! That must be a misfold. I fixed that code. The final version ad the same shift for both. > > The comment should say something like "Return true if diff can be > represented as a 26-bit twos-complement binary number" and the second > part of the test should be (diff >= (-1 << 25)). However, since you > define a test_offset() function in patch 4/5 that does the same test > but using only one comparison instead of two, why don't you just say: > > return !test_offset(diff); > > (having first moved test_offset() before test_24bit_addr)? OK, will fix. Thanks. Hmm, maybe I ment to do that, and miss the patch :-/ -- 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..24c023a 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,62 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code, return faulted; } +static int test_24bit_addr(unsigned long ip, unsigned long addr) +{ + long diff; + + /* + * Can we get to addr from ip in 24 bits? + * (26 really, since we mulitply by 4 for 4 byte alignment) + */ + diff = addr - ip; + + /* + * Return true if diff is less than 1 << 25 + * and greater than -1 << 26. + */ + return (diff < (1 << 25)) && (diff > (-1 << 26)); +} + +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 +184,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; }