Patchwork [2/5] powerpc: ftrace, convert to new dynamic ftrace arch API

login
register
mail settings
Submitter Steven Rostedt
Date Nov. 20, 2008, 7:09 p.m.
Message ID <20081120191149.285359258@goodmis.org>
Download mbox | patch
Permalink /patch/9882/
State Not Applicable
Headers show

Comments

Steven Rostedt - Nov. 20, 2008, 7:09 p.m.
From: Steven Rostedt <srostedt@redhat.com>

Impact: update to PowerPC ftrace arch API

This patch converts PowerPC to use the new dynamic ftrace arch API.

Thanks to Paul Mackennas for pointing out the mistakes of my original
test_24bit_addr function.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/include/asm/ftrace.h |   14 +++++++-
 arch/powerpc/kernel/ftrace.c      |   67 ++++++++++++++++++++++++++++++++++---
 2 files changed, 75 insertions(+), 6 deletions(-)
Paul Mackerras - Nov. 24, 2008, 1:43 a.m.
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.
Michael Ellerman - Nov. 24, 2008, 2:49 a.m.
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
Steven Rostedt - Nov. 24, 2008, 6 p.m.
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

Patch

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;
 }