[PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once

[PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once

[Michael Ellerman]

Currently we generate the module stub for ftrace_caller() at the bottom
of apply_relocate_add(). However apply_relocate_add() is potentially
called more than once per module, which means we will try to generate
the ftrace_caller() stub multiple times.

Although the current code deals with that correctly, ie. it only
generates a stub the first time, it would be clearer to only try to
generate the stub once.

Note also on first reading it may appear that we generate a different
stub for each section that requires relocation, but that is not the
case. The code in stub_for_addr() that searches for an existing stub
uses sechdrs[me->arch.stubs_section], ie. the single stub section for
this module.

A cleaner approach is to only generate the ftrace_caller() stub once,
from module_finalize(). An additional benefit is we can clean the ifdefs
up a little.

Finally we must propagate the const'ness of some of the pointers passed
to module_finalize(), but that is also an improvement.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/module.h |  9 +++++++++
 arch/powerpc/kernel/module.c      |  5 +++++
 arch/powerpc/kernel/module_32.c   | 15 ++++++++++-----
 arch/powerpc/kernel/module_64.c   | 22 ++++++++++++++--------
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index dcfcad139bcc..74d25a749018 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -82,6 +82,15 @@ bool is_module_trampoline(u32 *insns);
 int module_trampoline_target(struct module *mod, u32 *trampoline,
 			     unsigned long *target);
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs);
+#else
+static inline int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
+{
+	return 0;
+}
+#endif
+
 struct exception_table_entry;
 void sort_ex_table(struct exception_table_entry *start,
 		   struct exception_table_entry *finish);
diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index 9547381b631a..d1f1b35bf0c7 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -47,6 +47,11 @@ int module_finalize(const Elf_Ehdr *hdr,
 		const Elf_Shdr *sechdrs, struct module *me)
 {
 	const Elf_Shdr *sect;
+	int rc;
+
+	rc = module_finalize_ftrace(me, sechdrs);
+	if (rc)
+		return rc;
 
 	/* Apply feature fixups */
 	sect = find_section(hdr, sechdrs, "__ftr_fixup");
diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index 2c01665eb410..98f698f10956 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -294,11 +294,16 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
 			return -ENOEXEC;
 		}
 	}
+
+	return 0;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
-	module->arch.tramp =
-		do_plt_call(module->core_layout.base,
-			    (unsigned long)ftrace_caller,
-			    sechdrs, module);
-#endif
+int module_finalize_ftrace(struct module *module, const Elf_Shdr *sechdrs)
+{
+	module->arch.tramp = do_plt_call(module->core_layout.base,
+					 (unsigned long)ftrace_caller,
+					 sechdrs, module);
 	return 0;
 }
+#endif
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ac64ffdb52c8..599c753c7960 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -413,7 +413,7 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
 /* r2 is the TOC pointer: it actually points 0x8000 into the TOC (this
    gives the value maximum span in an instruction which uses a signed
    offset) */
-static inline unsigned long my_r2(Elf64_Shdr *sechdrs, struct module *me)
+static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module *me)
 {
 	return sechdrs[me->arch.toc_section].sh_addr + 0x8000;
 }
@@ -426,7 +426,7 @@ static inline unsigned long my_r2(Elf64_Shdr *sechdrs, struct module *me)
 #define PPC_HA(v) PPC_HI ((v) + 0x8000)
 
 /* Patch stub to reference function and correct r2 value. */
-static inline int create_stub(Elf64_Shdr *sechdrs,
+static inline int create_stub(const Elf64_Shdr *sechdrs,
 			      struct ppc64_stub_entry *entry,
 			      unsigned long addr,
 			      struct module *me)
@@ -452,7 +452,7 @@ static inline int create_stub(Elf64_Shdr *sechdrs,
 
 /* Create stub to jump to function described in this OPD/ptr: we need the
    stub to set up the TOC ptr (r2) for the function. */
-static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
+static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
 				   unsigned long addr,
 				   struct module *me)
 {
@@ -693,12 +693,18 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 		}
 	}
 
+	return 0;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
-	me->arch.toc = my_r2(sechdrs, me);
-	me->arch.tramp = stub_for_addr(sechdrs,
-				       (unsigned long)ftrace_caller,
-				       me);
-#endif
+int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
+{
+	mod->arch.toc = my_r2(sechdrs, mod);
+	mod->arch.tramp = stub_for_addr(sechdrs, (unsigned long)ftrace_caller, mod);
+
+	if (!mod->arch.tramp)
+		return -ENOENT;
 
 	return 0;
 }
+#endif
-- 
2.5.0

[PATCH 02/12] powerpc/module: Mark module stubs with a magic value

[Michael Ellerman]

When a module is loaded, calls out to the kernel go via a stub which is
generated at runtime. One of these stubs is used to call _mcount(),
which is the default target of tracing calls generated by the compiler
with -pg.

If dynamic ftrace is enabled (which it typicall is), another stub is
used to call ftrace_caller(), which is the target of tracing calls when
ftrace is actually active.

ftrace then wants to disable the calls to _mcount() at module startup,
and enable/disable the calls to ftrace_caller() when enabling/disablig
tracing - all of these it does by patching the code.

As part of that code patching, the ftrace code wants to confirm that the
branch it is about to modify, is in fact a call to a module stub which
calls _mcount() or ftrace_caller().

Currently it does that by inspecting the instructions and confirming
they are what it expects. Although that works, the code to do it is
pretty intricate because it requires lots of knowledge about the exact
format of the stub.

We can make that process easier by marking the generated stubs with a
magic value, and then looking for that magic value. Altough this is not
as rigorous as the current method, I believe it is sufficient in
practice.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/module.h |  3 +-
 arch/powerpc/kernel/ftrace.c      | 14 ++-----
 arch/powerpc/kernel/module_64.c   | 78 +++++++++++++--------------------------
 3 files changed, 31 insertions(+), 64 deletions(-)

diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 74d25a749018..5b6b5a427b54 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -78,8 +78,7 @@ struct mod_arch_specific {
 #    endif	/* MODULE */
 #endif
 
-bool is_module_trampoline(u32 *insns);
-int module_trampoline_target(struct module *mod, u32 *trampoline,
+int module_trampoline_target(struct module *mod, unsigned long trampoline,
 			     unsigned long *target);
 
 #ifdef CONFIG_DYNAMIC_FTRACE
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 44d4d8eb3c85..4505cbfd0e13 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -106,10 +106,9 @@ static int
 __ftrace_make_nop(struct module *mod,
 		  struct dyn_ftrace *rec, unsigned long addr)
 {
-	unsigned int op;
-	unsigned long entry, ptr;
+	unsigned long entry, ptr, tramp;
 	unsigned long ip = rec->ip;
-	void *tramp;
+	unsigned int op;
 
 	/* read where this goes */
 	if (probe_kernel_read(&op, (void *)ip, sizeof(int)))
@@ -122,14 +121,9 @@ __ftrace_make_nop(struct module *mod,
 	}
 
 	/* lets find where the pointer goes */
-	tramp = (void *)find_bl_target(ip, op);
-
-	pr_devel("ip:%lx jumps to %p", ip, tramp);
+	tramp = find_bl_target(ip, op);
 
-	if (!is_module_trampoline(tramp)) {
-		pr_err("Not a trampoline\n");
-		return -EINVAL;
-	}
+	pr_devel("ip:%lx jumps to %lx", ip, tramp);
 
 	if (module_trampoline_target(mod, tramp, &ptr)) {
 		pr_err("Failed to get trampoline target\n");
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 599c753c7960..9629966e614b 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -96,6 +96,8 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
 }
 #endif
 
+#define STUB_MAGIC 0x73747562 /* stub */
+
 /* Like PPC32, we need little trampolines to do > 24-bit jumps (into
    the kernel itself).  But on PPC64, these need to be used for every
    jump, actually, to reset r2 (TOC+0x8000). */
@@ -105,7 +107,8 @@ struct ppc64_stub_entry
 	 * need 6 instructions on ABIv2 but we always allocate 7 so
 	 * so we don't have to modify the trampoline load instruction. */
 	u32 jump[7];
-	u32 unused;
+	/* Used by ftrace to identify stubs */
+	u32 magic;
 	/* Data for the above code */
 	func_desc_t funcdata;
 };
@@ -139,70 +142,39 @@ static u32 ppc64_stub_insns[] = {
 };
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-
-static u32 ppc64_stub_mask[] = {
-	0xffff0000,
-	0xffff0000,
-	0xffffffff,
-	0xffffffff,
-#if !defined(_CALL_ELF) || _CALL_ELF != 2
-	0xffffffff,
-#endif
-	0xffffffff,
-	0xffffffff
-};
-
-bool is_module_trampoline(u32 *p)
+int module_trampoline_target(struct module *mod, unsigned long addr,
+			     unsigned long *target)
 {
-	unsigned int i;
-	u32 insns[ARRAY_SIZE(ppc64_stub_insns)];
-
-	BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask));
+	struct ppc64_stub_entry *stub;
+	func_desc_t funcdata;
+	u32 magic;
 
-	if (probe_kernel_read(insns, p, sizeof(insns)))
+	if (!within_module_core(addr, mod)) {
+		pr_err("%s: stub %lx not in module %s\n", __func__, addr, mod->name);
 		return -EFAULT;
-
-	for (i = 0; i < ARRAY_SIZE(ppc64_stub_insns); i++) {
-		u32 insna = insns[i];
-		u32 insnb = ppc64_stub_insns[i];
-		u32 mask = ppc64_stub_mask[i];
-
-		if ((insna & mask) != (insnb & mask))
-			return false;
 	}
 
-	return true;
-}
+	stub = (struct ppc64_stub_entry *)addr;
 
-int module_trampoline_target(struct module *mod, u32 *trampoline,
-			     unsigned long *target)
-{
-	u32 buf[2];
-	u16 upper, lower;
-	long offset;
-	void *toc_entry;
-
-	if (probe_kernel_read(buf, trampoline, sizeof(buf)))
+	if (probe_kernel_read(&magic, &stub->magic, sizeof(magic))) {
+		pr_err("%s: fault reading magic for stub %lx for %s\n", __func__, addr, mod->name);
 		return -EFAULT;
+	}
 
-	upper = buf[0] & 0xffff;
-	lower = buf[1] & 0xffff;
-
-	/* perform the addis/addi, both signed */
-	offset = ((short)upper << 16) + (short)lower;
+	if (magic != STUB_MAGIC) {
+		pr_err("%s: bad magic for stub %lx for %s\n", __func__, addr, mod->name);
+		return -EFAULT;
+	}
 
-	/*
-	 * Now get the address this trampoline jumps to. This
-	 * is always 32 bytes into our trampoline stub.
-	 */
-	toc_entry = (void *)mod->arch.toc + offset + 32;
+	if (probe_kernel_read(&funcdata, &stub->funcdata, sizeof(funcdata))) {
+		pr_err("%s: fault reading funcdata for stub %lx for %s\n", __func__, addr, mod->name);
+                return -EFAULT;
+	}
 
-	if (probe_kernel_read(target, toc_entry, sizeof(*target)))
-		return -EFAULT;
+	*target = stub_func_addr(funcdata);
 
 	return 0;
 }
-
 #endif
 
 /* Count how many different 24-bit relocations (different symbol,
@@ -447,6 +419,8 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
 	entry->jump[0] |= PPC_HA(reladdr);
 	entry->jump[1] |= PPC_LO(reladdr);
 	entry->funcdata = func_desc(addr);
+	entry->magic = STUB_MAGIC;
+
 	return 1;
 }
 
-- 
2.5.0

Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic value

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> When a module is loaded, calls out to the kernel go via a stub which is
> generated at runtime. One of these stubs is used to call _mcount(),
> which is the default target of tracing calls generated by the compiler
> with -pg.
>
> If dynamic ftrace is enabled (which it typicall is), another stub is
> used to call ftrace_caller(), which is the target of tracing calls when
> ftrace is actually active.
>
> ftrace then wants to disable the calls to _mcount() at module startup,
> and enable/disable the calls to ftrace_caller() when enabling/disablig
> tracing - all of these it does by patching the code.
>
> As part of that code patching, the ftrace code wants to confirm that the
> branch it is about to modify, is in fact a call to a module stub which
> calls _mcount() or ftrace_caller().
>
> Currently it does that by inspecting the instructions and confirming
> they are what it expects. Although that works, the code to do it is
> pretty intricate because it requires lots of knowledge about the exact
> format of the stub.
>
> We can make that process easier by marking the generated stubs with a
> magic value, and then looking for that magic value. Altough this is not
> as rigorous as the current method, I believe it is sufficient in
> practice.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/module.h |  3 +-
>  arch/powerpc/kernel/ftrace.c      | 14 ++-----
>  arch/powerpc/kernel/module_64.c   | 78 +++++++++++++--------------------------
>  3 files changed, 31 insertions(+), 64 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index 74d25a749018..5b6b5a427b54 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -78,8 +78,7 @@ struct mod_arch_specific {
>  #    endif	/* MODULE */
>  #endif
>  
> -bool is_module_trampoline(u32 *insns);
> -int module_trampoline_target(struct module *mod, u32 *trampoline,
> +int module_trampoline_target(struct module *mod, unsigned long trampoline,
>  			     unsigned long *target);
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 44d4d8eb3c85..4505cbfd0e13 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -106,10 +106,9 @@ static int
>  __ftrace_make_nop(struct module *mod,
>  		  struct dyn_ftrace *rec, unsigned long addr)
>  {
> -	unsigned int op;
> -	unsigned long entry, ptr;
> +	unsigned long entry, ptr, tramp;
>  	unsigned long ip = rec->ip;
> -	void *tramp;
> +	unsigned int op;
>  
>  	/* read where this goes */
>  	if (probe_kernel_read(&op, (void *)ip, sizeof(int)))
> @@ -122,14 +121,9 @@ __ftrace_make_nop(struct module *mod,
>  	}
>  
>  	/* lets find where the pointer goes */
> -	tramp = (void *)find_bl_target(ip, op);
> -
> -	pr_devel("ip:%lx jumps to %p", ip, tramp);
> +	tramp = find_bl_target(ip, op);
>  
> -	if (!is_module_trampoline(tramp)) {
> -		pr_err("Not a trampoline\n");
> -		return -EINVAL;
> -	}
> +	pr_devel("ip:%lx jumps to %lx", ip, tramp);
>  
>  	if (module_trampoline_target(mod, tramp, &ptr)) {
>  		pr_err("Failed to get trampoline target\n");
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 599c753c7960..9629966e614b 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -96,6 +96,8 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
>  }
>  #endif
>  
> +#define STUB_MAGIC 0x73747562 /* stub */
> +
>  /* Like PPC32, we need little trampolines to do > 24-bit jumps (into
>     the kernel itself).  But on PPC64, these need to be used for every
>     jump, actually, to reset r2 (TOC+0x8000). */
> @@ -105,7 +107,8 @@ struct ppc64_stub_entry
>  	 * need 6 instructions on ABIv2 but we always allocate 7 so
>  	 * so we don't have to modify the trampoline load instruction. */
>  	u32 jump[7];
> -	u32 unused;
> +	/* Used by ftrace to identify stubs */
> +	u32 magic;

>  	/* Data for the above code */
>  	func_desc_t funcdata;
>  };
> @@ -139,70 +142,39 @@ static u32 ppc64_stub_insns[] = {
>  };
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> -
> -static u32 ppc64_stub_mask[] = {
> -	0xffff0000,
> -	0xffff0000,
> -	0xffffffff,
> -	0xffffffff,
> -#if !defined(_CALL_ELF) || _CALL_ELF != 2
> -	0xffffffff,
> -#endif
> -	0xffffffff,
> -	0xffffffff
> -};
> -
> -bool is_module_trampoline(u32 *p)
> +int module_trampoline_target(struct module *mod, unsigned long addr,
> +			     unsigned long *target)
>  {
> -	unsigned int i;
> -	u32 insns[ARRAY_SIZE(ppc64_stub_insns)];
> -
> -	BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask));
> +	struct ppc64_stub_entry *stub;
> +	func_desc_t funcdata;
> +	u32 magic;
>  
> -	if (probe_kernel_read(insns, p, sizeof(insns)))
> +	if (!within_module_core(addr, mod)) {
> +		pr_err("%s: stub %lx not in module %s\n", __func__, addr, mod->name);
>  		return -EFAULT;
-EFAULT or -EINVAL? I wonder if we can recover from a bad trampoline address.

Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic value

[Michael Ellerman]

On Thu, 2016-02-25 at 11:04 +1100, Balbir Singh wrote:
>
> On 25/02/16 01:28, Michael Ellerman wrote:
> > -bool is_module_trampoline(u32 *p)
> > +int module_trampoline_target(struct module *mod, unsigned long addr,
> > +			     unsigned long *target)
> >  {
> > -	unsigned int i;
> > -	u32 insns[ARRAY_SIZE(ppc64_stub_insns)];
> > -
> > -	BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask));
> > +	struct ppc64_stub_entry *stub;
> > +	func_desc_t funcdata;
> > +	u32 magic;
> >
> > -	if (probe_kernel_read(insns, p, sizeof(insns)))
> > +	if (!within_module_core(addr, mod)) {
> > +		pr_err("%s: stub %lx not in module %s\n", __func__, addr, mod->name);
> >  		return -EFAULT;

> -EFAULT or -EINVAL?

I think we want EFAULT. Otherwise ftrace_bug() will try and print the actual
instruction, which would then fault. (though I haven't confirmed that by
testing)

> I wonder if we can recover from a bad trampoline address.

We can't recover at the moment. Do you mean is there some way we could recover?

cheers

Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic value

[Torsten Duwe]

On Thu, Feb 25, 2016 at 01:28:25AM +1100, Michael Ellerman wrote:
> 
> We can make that process easier by marking the generated stubs with a
> magic value, and then looking for that magic value. Altough this is not
> as rigorous as the current method, I believe it is sufficient in
> practice.

The actual magic value is sort of debatable; it should be "improbable"
enough. But this can be changed easily, for each kernel compile, even.

> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Torsten Duwe <duwe@suse.de>

[for reference:]
>  
> +#define STUB_MAGIC 0x73747562 /* stub */
> +

	Torsten

Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic value

[Michael Ellerman]

On Thu, 2016-02-25 at 14:17 +0100, Torsten Duwe wrote:
> On Thu, Feb 25, 2016 at 01:28:25AM +1100, Michael Ellerman wrote:
> > 
> > We can make that process easier by marking the generated stubs with a
> > magic value, and then looking for that magic value. Altough this is not
> > as rigorous as the current method, I believe it is sufficient in
> > practice.
> 
> The actual magic value is sort of debatable; it should be "improbable"
> enough. But this can be changed easily, for each kernel compile, even.

Yeah. Given the locations we're trying to patch are computed in the first place
from the mcount call sites, I feel like we don't need to be super paranoid
here. The only time I've heard of this code (the current version) tripping up
is when folks are hacking on ftrace.

> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Torsten Duwe <duwe@suse.de>
> 
> [for reference:]

> > +#define STUB_MAGIC 0x73747562 /* stub */

Which is one of:

ori     r21,r19,29811
andi.   r20,r27,30050

Both of which are pretty improbable. They don't appear in any kernel I have
around here.

I have more plans for this code, which would hopefully mean we can get rid of
the magic checking entirely. But I think this is OK for now.

cheers

[PATCH 03/12] powerpc/module: Create a special stub for ftrace_caller()

[Michael Ellerman]

In order to support the new -mprofile-kernel ABI, we need to be able to
call from the module back to ftrace_caller() (in the kernel) without
using the module's r2. That is because the function in this module which
is calling ftrace_caller() may not have setup r2, if it doesn't
otherwise need it (ie. it accesses no globals).

To make that work we add a new stub which is used for calling
ftrace_caller(), which uses the kernel toc instead of the module toc.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/module_64.c | 48 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9629966e614b..e711d40a3b8f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -671,10 +671,56 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+
+#define PACATOC offsetof(struct paca_struct, kernel_toc)
+
+static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
+{
+	struct ppc64_stub_entry *entry;
+	unsigned int i, num_stubs;
+	static u32 stub_insns[] = {
+		0xe98d0000 | PACATOC, 	/* ld      r12,PACATOC(r13)	*/
+		0x3d8c0000,		/* addis   r12,r12,<high>	*/
+		0x398c0000, 		/* addi    r12,r12,<low>	*/
+		0x7d8903a6, 		/* mtctr   r12			*/
+		0x4e800420, 		/* bctr				*/
+	};
+	long reladdr;
+
+	num_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*entry);
+
+	/* Find the next available stub entry */
+	entry = (void *)sechdrs[me->arch.stubs_section].sh_addr;
+	for (i = 0; i < num_stubs && stub_func_addr(entry->funcdata); i++, entry++);
+
+	if (i >= num_stubs) {
+		pr_err("%s: Unable to find a free slot for ftrace stub.\n", me->name);
+		return 0;
+	}
+
+	memcpy(entry->jump, stub_insns, sizeof(stub_insns));
+
+	/* Stub uses address relative to kernel_toc */
+	reladdr = (unsigned long)ftrace_caller - get_paca()->kernel_toc;
+	if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
+		pr_err("%s: Address of ftrace_caller out of range of kernel_toc.\n", me->name);
+		return 0;
+	}
+
+	entry->jump[1] |= PPC_HA(reladdr);
+	entry->jump[2] |= PPC_LO(reladdr);
+
+	/* Eventhough we don't use funcdata in the stub, it's needed elsewhere. */
+	entry->funcdata = func_desc((unsigned long)ftrace_caller);
+	entry->magic = STUB_MAGIC;
+
+	return (unsigned long)entry;
+}
+
 int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
 {
 	mod->arch.toc = my_r2(sechdrs, mod);
-	mod->arch.tramp = stub_for_addr(sechdrs, (unsigned long)ftrace_caller, mod);
+	mod->arch.tramp = create_ftrace_stub(sechdrs, mod);
 
 	if (!mod->arch.tramp)
 		return -ENOENT;
-- 
2.5.0

Re: [PATCH 03/12] powerpc/module: Create a special stub for ftrace_caller()

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> In order to support the new -mprofile-kernel ABI, we need to be able to
> call from the module back to ftrace_caller() (in the kernel) without
> using the module's r2. That is because the function in this module which
> is calling ftrace_caller() may not have setup r2, if it doesn't
> otherwise need it (ie. it accesses no globals).
>
> To make that work we add a new stub which is used for calling
> ftrace_caller(), which uses the kernel toc instead of the module toc.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/module_64.c | 48 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 9629966e614b..e711d40a3b8f 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -671,10 +671,56 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>  }
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> +
> +#define PACATOC offsetof(struct paca_struct, kernel_toc)
> +
> +static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
> +{
> +	struct ppc64_stub_entry *entry;
> +	unsigned int i, num_stubs;
How about some comments on r2
r2 is still pointing to the module's toc, will be saved by ftrace_caller and restored by the instruction following bl ftrace_caller (after patching _mcount/nop)

> +	static u32 stub_insns[] = {
> +		0xe98d0000 | PACATOC, 	/* ld      r12,PACATOC(r13)	*/
> +		0x3d8c0000,		/* addis   r12,r12,<high>	*/
> +		0x398c0000, 		/* addi    r12,r12,<low>	*/
> +		0x7d8903a6, 		/* mtctr   r12			*/
> +		0x4e800420, 		/* bctr				*/
> +	};
> +	long reladdr;
> +
> +	num_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*entry);
> +
> +	/* Find the next available stub entry */
> +	entry = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> +	for (i = 0; i < num_stubs && stub_func_addr(entry->funcdata); i++, entry++);
> +
> +	if (i >= num_stubs) {
> +		pr_err("%s: Unable to find a free slot for ftrace stub.\n", me->name);
> +		return 0;
> +	}
> +
> +	memcpy(entry->jump, stub_insns, sizeof(stub_insns));
> +
> +	/* Stub uses address relative to kernel_toc */
> +	reladdr = (unsigned long)ftrace_caller - get_paca()->kernel_toc;
> +	if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> +		pr_err("%s: Address of ftrace_caller out of range of kernel_toc.\n", me->name);
> +		return 0;
> +	}
> +
> +	entry->jump[1] |= PPC_HA(reladdr);
> +	entry->jump[2] |= PPC_LO(reladdr);
> +
> +	/* Eventhough we don't use funcdata in the stub, it's needed elsewhere. */
> +	entry->funcdata = func_desc((unsigned long)ftrace_caller);
> +	entry->magic = STUB_MAGIC;
> +
> +	return (unsigned long)entry;
> +}
> +
>  int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
>  {
>  	mod->arch.toc = my_r2(sechdrs, mod);
> -	mod->arch.tramp = stub_for_addr(sechdrs, (unsigned long)ftrace_caller, mod);
> +	mod->arch.tramp = create_ftrace_stub(sechdrs, mod);
>  
>  	if (!mod->arch.tramp)
>  		return -ENOENT;
Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 03/12] powerpc/module: Create a special stub for ftrace_caller()

[Michael Ellerman]

On Thu, 2016-02-25 at 11:08 +1100, Balbir Singh wrote:
> 
> On 25/02/16 01:28, Michael Ellerman wrote:
> > In order to support the new -mprofile-kernel ABI, we need to be able to
> > call from the module back to ftrace_caller() (in the kernel) without
> > using the module's r2. That is because the function in this module which
> > is calling ftrace_caller() may not have setup r2, if it doesn't
> > otherwise need it (ie. it accesses no globals).
> > 
> > To make that work we add a new stub which is used for calling
> > ftrace_caller(), which uses the kernel toc instead of the module toc.
> > 
> > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> > index 9629966e614b..e711d40a3b8f 100644
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -671,10 +671,56 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> >  }
> >  
> >  #ifdef CONFIG_DYNAMIC_FTRACE
> > +
> > +#define PACATOC offsetof(struct paca_struct, kernel_toc)
> > +
> > +static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
> > +{
> > +	struct ppc64_stub_entry *entry;
> > +	unsigned int i, num_stubs;

> How about some comments on r2
> r2 is still pointing to the module's toc, will be saved by ftrace_caller and
> restored by the instruction following bl ftrace_caller (after patching
> _mcount/nop)

Yeah I'll add some commentary.

I think the change log describes it fairly well but a comment is also good.

cheers

Re: [PATCH 03/12] powerpc/module: Create a special stub for ftrace_caller()

[Torsten Duwe]

On Thu, Feb 25, 2016 at 11:08:54AM +1100, Balbir Singh wrote:
> How about some comments on r2
> r2 is still pointing to the module's toc, will be saved by ftrace_caller and restored by the instruction following bl ftrace_caller (after patching _mcount/nop)

To be precise: ftrace_caller needs to save _and_ restore r2 in case of -mprofile-kernel.

> > +	/* Stub uses address relative to kernel_toc */
> > +	reladdr = (unsigned long)ftrace_caller - get_paca()->kernel_toc;

kernel_toc is a compile time constant; do you really want to look it up in
memory at runtime each time? It's a bit tricky to get the +- 0x8000 right
OTOH...

I wrote:
extern unsigned long __toc_start;
    reladdr = addr - ((unsigned long)(&__toc_start) + 0x8000UL);

looks a bit odd, but evaluates to a constant for ftrace_caller.

Either way is fine with me:

Signed-off-by: Torsten Duwe <duwe@suse.de>
Reviewed-by: Torsten Duwe <duwe@suse.de>

> Reviewed-by: Balbir Singh <bsingharora@gmail.com>

	Torsten

Re: [PATCH 03/12] powerpc/module: Create a special stub for ftrace_caller()

[Michael Ellerman]

On Thu, 2016-02-25 at 14:31 +0100, Torsten Duwe wrote:

> On Thu, Feb 25, 2016 at 11:08:54AM +1100, Balbir Singh wrote:

> > How about some comments on r2
> > r2 is still pointing to the module's toc, will be saved by ftrace_caller and restored by the instruction following bl ftrace_caller (after patching _mcount/nop)
> 
> To be precise: ftrace_caller needs to save _and_ restore r2 in case of -mprofile-kernel.

> > > +	/* Stub uses address relative to kernel_toc */
> > > +	reladdr = (unsigned long)ftrace_caller - get_paca()->kernel_toc;

Yeah true. I originally wrote it with the address of ftrace_caller passed as an
argument, so it had to be computed at runtime, and getting it from the paca is
~= to getting it out of the GOT.

> kernel_toc is a compile time constant; do you really want to look it up in
> memory at runtime each time? It's a bit tricky to get the +- 0x8000 right
> OTOH...
> 
> I wrote:
> extern unsigned long __toc_start;
>     reladdr = addr - ((unsigned long)(&__toc_start) + 0x8000UL);
> 
> looks a bit odd, but evaluates to a constant for ftrace_caller.

Yeah that makes sense. I'll add a helper to do the + 32k.

cheers

[PATCH 04/12] powerpc/ftrace: Prepare for -mprofile-kernel

[Michael Ellerman]

From: Torsten Duwe <duwe@lst.de>

The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.

  * arch/powerpc/kernel/entry_64.S:
    - modify the default _mcount to be prepared for such call sites
    - have the ftrace_graph_caller save function arguments before
      calling its C helper prepare_ftrace_return
  * arch/powerpc/include/asm/code-patching.h:
    - define some common macros to make things readable.
    - pull the R2 stack location definition from
      arch/powerpc/kernel/module_64.c
  * arch/powerpc/kernel/module_64.c:
    - enhance binary code examination to handle the new patterns.

Signed-off-by: Torsten Duwe <duwe@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/code-patching.h | 24 ++++++++++++++++
 arch/powerpc/kernel/entry_64.S           | 48 +++++++++++++++++++++++++++++++-
 arch/powerpc/kernel/ftrace.c             | 44 ++++++++++++++++++++++-------
 arch/powerpc/kernel/module_64.c          | 31 +++++++++++++++++++--
 4 files changed, 133 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 840a5509b3f1..7820b32515de 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -99,4 +99,28 @@ static inline unsigned long ppc_global_function_entry(void *func)
 #endif
 }
 
+#ifdef CONFIG_PPC64
+/* Some instruction encodings commonly used in dynamic ftracing
+ * and function live patching:
+ */
+
+/* This must match the definition of STK_GOT in <asm/ppc_asm.h> */
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+#define R2_STACK_OFFSET         24
+#else
+#define R2_STACK_OFFSET         40
+#endif
+
+/* load / store the TOC from / into the stack frame */
+#define PPC_INST_LD_TOC		(PPC_INST_LD  | ___PPC_RT(__REG_R2) | \
+				 ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
+#define PPC_INST_STD_TOC	(PPC_INST_STD | ___PPC_RS(__REG_R2) | \
+				 ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
+
+/* usually preceded by a mflr r0 */
+#define PPC_INST_STD_LR		(PPC_INST_STD | ___PPC_RS(__REG_R0) | \
+				 ___PPC_RA(__REG_R1) | PPC_LR_STKOFF)
+
+#endif /* CONFIG_PPC64 */
+
 #endif /* _ASM_POWERPC_CODE_PATCHING_H */
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 0d525ce3717f..2a7313cfbc7d 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1143,7 +1143,10 @@ _GLOBAL(enter_prom)
 #ifdef CONFIG_DYNAMIC_FTRACE
 _GLOBAL(mcount)
 _GLOBAL(_mcount)
-	blr
+	mflr	r12
+	mtctr	r12
+	mtlr	r0
+	bctr
 
 _GLOBAL_TOC(ftrace_caller)
 	/* Taken from output of objdump from lib64/glibc */
@@ -1198,6 +1201,7 @@ _GLOBAL(ftrace_stub)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#ifndef CC_USING_MPROFILE_KERNEL
 _GLOBAL(ftrace_graph_caller)
 	/* load r4 with local address */
 	ld	r4, 128(r1)
@@ -1222,6 +1226,48 @@ _GLOBAL(ftrace_graph_caller)
 	addi	r1, r1, 112
 	blr
 
+#else /* CC_USING_MPROFILE_KERNEL */
+_GLOBAL(ftrace_graph_caller)
+	/* with -mprofile-kernel, parameter regs are still alive at _mcount */
+	std	r10, 104(r1)
+	std	r9, 96(r1)
+	std	r8, 88(r1)
+	std	r7, 80(r1)
+	std	r6, 72(r1)
+	std	r5, 64(r1)
+	std	r4, 56(r1)
+	std	r3, 48(r1)
+	mfctr	r4		/* ftrace_caller has moved local addr here */
+	std	r4, 40(r1)
+	mflr	r3		/* ftrace_caller has restored LR from stack */
+	subi	r4, r4, MCOUNT_INSN_SIZE
+
+	bl	prepare_ftrace_return
+	nop
+
+	/*
+	 * prepare_ftrace_return gives us the address we divert to.
+	 * Change the LR to this.
+	 */
+	mtlr	r3
+
+	ld	r0, 40(r1)
+	mtctr	r0
+	ld	r10, 104(r1)
+	ld	r9, 96(r1)
+	ld	r8, 88(r1)
+	ld	r7, 80(r1)
+	ld	r6, 72(r1)
+	ld	r5, 64(r1)
+	ld	r4, 56(r1)
+	ld	r3, 48(r1)
+
+	addi	r1, r1, 112
+	mflr	r0
+	std	r0, LRSAVE(r1)
+	bctr
+#endif /* CC_USING_MPROFILE_KERNEL */
+
 _GLOBAL(return_to_handler)
 	/* need to save return values */
 	std	r4,  -32(r1)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 4505cbfd0e13..a1d95f20b017 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -281,16 +281,14 @@ int ftrace_make_nop(struct module *mod,
 
 #ifdef CONFIG_MODULES
 #ifdef CONFIG_PPC64
+/* Examine the existing instructions for __ftrace_make_call.
+ * They should effectively be a NOP, and follow formal constraints,
+ * depending on the ABI. Return false if they don't.
+ */
+#ifndef CC_USING_MPROFILE_KERNEL
 static int
-__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
 {
-	unsigned int op[2];
-	void *ip = (void *)rec->ip;
-
-	/* read where this goes */
-	if (probe_kernel_read(op, ip, sizeof(op)))
-		return -EFAULT;
-
 	/*
 	 * We expect to see:
 	 *
@@ -300,8 +298,34 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	 * The load offset is different depending on the ABI. For simplicity
 	 * just mask it out when doing the compare.
 	 */
-	if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
-		pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
+	if ((op0 != 0x48000008) || ((op1 & 0xffff0000) != 0xe8410000))
+		return 0;
+	return 1;
+}
+#else
+static int
+expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
+{
+	/* look for patched "NOP" on ppc64 with -mprofile-kernel */
+	if (op0 != PPC_INST_NOP)
+		return 0;
+	return 1;
+}
+#endif
+
+static int
+__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned int op[2];
+	void *ip = (void *)rec->ip;
+
+	/* read where this goes */
+	if (probe_kernel_read(op, ip, sizeof(op)))
+		return -EFAULT;
+
+	if (!expected_nop_sequence(ip, op[0], op[1])) {
+		pr_err("Unexpected call sequence at %p: %x %x\n",
+		ip, op[0], op[1]);
 		return -EINVAL;
 	}
 
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index e711d40a3b8f..32c10e0d2aa5 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -41,7 +41,6 @@
    --RR.  */
 
 #if defined(_CALL_ELF) && _CALL_ELF == 2
-#define R2_STACK_OFFSET 24
 
 /* An address is simply the address of the function. */
 typedef unsigned long func_desc_t;
@@ -73,7 +72,6 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
 	return PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
 }
 #else
-#define R2_STACK_OFFSET 40
 
 /* An address is address of the OPD entry, which contains address of fn. */
 typedef struct ppc64_opd_entry func_desc_t;
@@ -450,17 +448,44 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
 	return (unsigned long)&stubs[i];
 }
 
+#ifdef CC_USING_MPROFILE_KERNEL
+static int is_early_mcount_callsite(u32 *instruction)
+{
+	/* -mprofile-kernel sequence starting with
+	 * mflr r0 and maybe std r0, LRSAVE(r1).
+	 */
+	if ((instruction[-3] == PPC_INST_MFLR &&
+	     instruction[-2] == PPC_INST_STD_LR) ||
+	    instruction[-2] == PPC_INST_MFLR) {
+		/* Nothing to be done here, it's an _mcount
+		 * call location and r2 will have to be
+		 * restored in the _mcount function.
+		 */
+		return 1;
+	}
+	return 0;
+}
+#else
+/* without -mprofile-kernel, mcount calls are never early */
+static int is_early_mcount_callsite(u32 *instruction)
+{
+	return 0;
+}
+#endif
+
 /* We expect a noop next: if it is, replace it with instruction to
    restore r2. */
 static int restore_r2(u32 *instruction, struct module *me)
 {
 	if (*instruction != PPC_INST_NOP) {
+		if (is_early_mcount_callsite(instruction))
+			return 1;
 		pr_err("%s: Expect noop after relocate, got %08x\n",
 		       me->name, *instruction);
 		return 0;
 	}
 	/* ld r2,R2_STACK_OFFSET(r1) */
-	*instruction = 0xe8410000 | R2_STACK_OFFSET;
+	*instruction = PPC_INST_LD_TOC;
 	return 1;
 }
 
-- 
2.5.0

Re: [PATCH 04/12] powerpc/ftrace: Prepare for -mprofile-kernel

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> From: Torsten Duwe <duwe@lst.de>
>
> The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
> allows to call _mcount very early in the function, which low-level
> ASM code and code patching functions need to consider.
> Especially the link register and the parameter registers are still
> alive and not yet saved into a new stack frame.
>
>   * arch/powerpc/kernel/entry_64.S:
>     - modify the default _mcount to be prepared for such call sites
>     - have the ftrace_graph_caller save function arguments before
>       calling its C helper prepare_ftrace_return
>   * arch/powerpc/include/asm/code-patching.h:
>     - define some common macros to make things readable.
>     - pull the R2 stack location definition from
>       arch/powerpc/kernel/module_64.c
>   * arch/powerpc/kernel/module_64.c:
>     - enhance binary code examination to handle the new patterns.
>
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/code-patching.h | 24 ++++++++++++++++
>  arch/powerpc/kernel/entry_64.S           | 48 +++++++++++++++++++++++++++++++-
>  arch/powerpc/kernel/ftrace.c             | 44 ++++++++++++++++++++++-------
>  arch/powerpc/kernel/module_64.c          | 31 +++++++++++++++++++--
>  4 files changed, 133 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 840a5509b3f1..7820b32515de 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -99,4 +99,28 @@ static inline unsigned long ppc_global_function_entry(void *func)
>  #endif
>  }
>  
> +#ifdef CONFIG_PPC64
> +/* Some instruction encodings commonly used in dynamic ftracing
> + * and function live patching:
> + */
> +
> +/* This must match the definition of STK_GOT in <asm/ppc_asm.h> */
> +#if defined(_CALL_ELF) && _CALL_ELF == 2
> +#define R2_STACK_OFFSET         24
> +#else
> +#define R2_STACK_OFFSET         40
> +#endif
> +
> +/* load / store the TOC from / into the stack frame */
> +#define PPC_INST_LD_TOC		(PPC_INST_LD  | ___PPC_RT(__REG_R2) | \
> +				 ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
> +#define PPC_INST_STD_TOC	(PPC_INST_STD | ___PPC_RS(__REG_R2) | \
> +				 ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
> +
> +/* usually preceded by a mflr r0 */
> +#define PPC_INST_STD_LR		(PPC_INST_STD | ___PPC_RS(__REG_R0) | \
> +				 ___PPC_RA(__REG_R1) | PPC_LR_STKOFF)
> +
> +#endif /* CONFIG_PPC64 */
> +
>  #endif /* _ASM_POWERPC_CODE_PATCHING_H */
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 0d525ce3717f..2a7313cfbc7d 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1143,7 +1143,10 @@ _GLOBAL(enter_prom)
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  _GLOBAL(mcount)
>  _GLOBAL(_mcount)
> -	blr
> +	mflr	r12
> +	mtctr	r12
> +	mtlr	r0
> +	bctr
>  
>  _GLOBAL_TOC(ftrace_caller)
>  	/* Taken from output of objdump from lib64/glibc */
> @@ -1198,6 +1201,7 @@ _GLOBAL(ftrace_stub)
>  #endif /* CONFIG_DYNAMIC_FTRACE */
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +#ifndef CC_USING_MPROFILE_KERNEL
>  _GLOBAL(ftrace_graph_caller)
>  	/* load r4 with local address */
>  	ld	r4, 128(r1)
> @@ -1222,6 +1226,48 @@ _GLOBAL(ftrace_graph_caller)
>  	addi	r1, r1, 112
>  	blr
>  
> +#else /* CC_USING_MPROFILE_KERNEL */
> +_GLOBAL(ftrace_graph_caller)
> +	/* with -mprofile-kernel, parameter regs are still alive at _mcount */
> +	std	r10, 104(r1)
> +	std	r9, 96(r1)
> +	std	r8, 88(r1)
> +	std	r7, 80(r1)
> +	std	r6, 72(r1)
> +	std	r5, 64(r1)
> +	std	r4, 56(r1)
> +	std	r3, 48(r1)
> +	mfctr	r4		/* ftrace_caller has moved local addr here */
> +	std	r4, 40(r1)
> +	mflr	r3		/* ftrace_caller has restored LR from stack */
> +	subi	r4, r4, MCOUNT_INSN_SIZE
> +
> +	bl	prepare_ftrace_return
> +	nop
> +
> +	/*
> +	 * prepare_ftrace_return gives us the address we divert to.
> +	 * Change the LR to this.
> +	 */
> +	mtlr	r3
> +
> +	ld	r0, 40(r1)
> +	mtctr	r0
> +	ld	r10, 104(r1)
> +	ld	r9, 96(r1)
> +	ld	r8, 88(r1)
> +	ld	r7, 80(r1)
> +	ld	r6, 72(r1)
> +	ld	r5, 64(r1)
> +	ld	r4, 56(r1)
> +	ld	r3, 48(r1)
> +
> +	addi	r1, r1, 112
> +	mflr	r0
> +	std	r0, LRSAVE(r1)
> +	bctr
> +#endif /* CC_USING_MPROFILE_KERNEL */
> +
>  _GLOBAL(return_to_handler)
>  	/* need to save return values */
>  	std	r4,  -32(r1)
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 4505cbfd0e13..a1d95f20b017 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -281,16 +281,14 @@ int ftrace_make_nop(struct module *mod,
>  
>  #ifdef CONFIG_MODULES
>  #ifdef CONFIG_PPC64
> +/* Examine the existing instructions for __ftrace_make_call.
> + * They should effectively be a NOP, and follow formal constraints,
> + * depending on the ABI. Return false if they don't.
> + */
> +#ifndef CC_USING_MPROFILE_KERNEL
>  static int
> -__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> +expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
>  {
> -	unsigned int op[2];
> -	void *ip = (void *)rec->ip;
> -
> -	/* read where this goes */
> -	if (probe_kernel_read(op, ip, sizeof(op)))
> -		return -EFAULT;
> -
>  	/*
>  	 * We expect to see:
>  	 *
> @@ -300,8 +298,34 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  	 * The load offset is different depending on the ABI. For simplicity
>  	 * just mask it out when doing the compare.
>  	 */
> -	if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
> -		pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
> +	if ((op0 != 0x48000008) || ((op1 & 0xffff0000) != 0xe8410000))
> +		return 0;
> +	return 1;
> +}
> +#else
> +static int
> +expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
> +{
> +	/* look for patched "NOP" on ppc64 with -mprofile-kernel */
> +	if (op0 != PPC_INST_NOP)
> +		return 0;
> +	return 1;
With the magic changes, do we care for this? I think it's a bit of an overkill
> +}
> +#endif
> +
> +static int
> +__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> +{
> +	unsigned int op[2];
> +	void *ip = (void *)rec->ip;
> +
> +	/* read where this goes */
> +	if (probe_kernel_read(op, ip, sizeof(op)))
> +		return -EFAULT;
> +
> +	if (!expected_nop_sequence(ip, op[0], op[1])) {
> +		pr_err("Unexpected call sequence at %p: %x %x\n",
> +		ip, op[0], op[1]);
>  		return -EINVAL;
>  	}
>  
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index e711d40a3b8f..32c10e0d2aa5 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -41,7 +41,6 @@
>     --RR.  */
>  
>  #if defined(_CALL_ELF) && _CALL_ELF == 2
> -#define R2_STACK_OFFSET 24
>  
>  /* An address is simply the address of the function. */
>  typedef unsigned long func_desc_t;
> @@ -73,7 +72,6 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
>  	return PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
>  }
>  #else
> -#define R2_STACK_OFFSET 40
>  
>  /* An address is address of the OPD entry, which contains address of fn. */
>  typedef struct ppc64_opd_entry func_desc_t;
> @@ -450,17 +448,44 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
>  	return (unsigned long)&stubs[i];
>  }
>  
> +#ifdef CC_USING_MPROFILE_KERNEL
> +static int is_early_mcount_callsite(u32 *instruction)
> +{
> +	/* -mprofile-kernel sequence starting with
> +	 * mflr r0 and maybe std r0, LRSAVE(r1).
> +	 */
> +	if ((instruction[-3] == PPC_INST_MFLR &&
> +	     instruction[-2] == PPC_INST_STD_LR) ||
> +	    instruction[-2] == PPC_INST_MFLR) {
> +		/* Nothing to be done here, it's an _mcount
> +		 * call location and r2 will have to be
> +		 * restored in the _mcount function.
> +		 */
> +		return 1;
> +	}
> +	return 0;
> +}
> +#else
> +/* without -mprofile-kernel, mcount calls are never early */
> +static int is_early_mcount_callsite(u32 *instruction)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /* We expect a noop next: if it is, replace it with instruction to
>     restore r2. */
>  static int restore_r2(u32 *instruction, struct module *me)
>  {
>  	if (*instruction != PPC_INST_NOP) {
> +		if (is_early_mcount_callsite(instruction))
> +			return 1;
I don't think we need this either, since mcount callsites use a different stub now for ftrace
>  		pr_err("%s: Expect noop after relocate, got %08x\n",
>  		       me->name, *instruction);
>  		return 0;
>  	}
>  	/* ld r2,R2_STACK_OFFSET(r1) */
> -	*instruction = 0xe8410000 | R2_STACK_OFFSET;
> +	*instruction = PPC_INST_LD_TOC;
>  	return 1;
>  }
>  

Warm Regards,
Balbir Singh.

Re: [PATCH 04/12] powerpc/ftrace: Prepare for -mprofile-kernel

[Michael Ellerman]

On Thu, 2016-02-25 at 11:28 +1100, Balbir Singh wrote:
> On 25/02/16 01:28, Michael Ellerman wrote:
> > @@ -300,8 +298,34 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> >  	 * The load offset is different depending on the ABI. For simplicity
> >  	 * just mask it out when doing the compare.
> >  	 */
> > -	if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
> > -		pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
> > +	if ((op0 != 0x48000008) || ((op1 & 0xffff0000) != 0xe8410000))
> > +		return 0;
> > +	return 1;
> > +}
> > +#else
> > +static int
> > +expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
> > +{
> > +	/* look for patched "NOP" on ppc64 with -mprofile-kernel */
> > +	if (op0 != PPC_INST_NOP)
> > +		return 0;
> > +	return 1;

> With the magic changes, do we care for this? I think it's a bit of an overkill

I don't particularly like it either. However this code doesn't actually use the
magic, it's the reverse case of turning a nop into a call to the stub. So the
magic in the stub doesn't actually make that any safer.

I think we do at least want to check there's a nop there. But without
mprofile-kernel it's not a nop, so we need some check and it does need to be
different between the profiling ABIs. So I think for now this is the
conservative approach.

cheers

Re: [PATCH 04/12] powerpc/ftrace: Prepare for -mprofile-kernel

[Torsten Duwe]

On Thu, Feb 25, 2016 at 01:28:27AM +1100, Michael Ellerman wrote:
> @@ -450,17 +448,44 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
>  	return (unsigned long)&stubs[i];
>  }
>  
> +#ifdef CC_USING_MPROFILE_KERNEL
> +static int is_early_mcount_callsite(u32 *instruction)
> +{
> +	/* -mprofile-kernel sequence starting with
> +	 * mflr r0 and maybe std r0, LRSAVE(r1).
> +	 */
> +	if ((instruction[-3] == PPC_INST_MFLR &&
> +	     instruction[-2] == PPC_INST_STD_LR) ||
> +	    instruction[-2] == PPC_INST_MFLR) {
> +		/* Nothing to be done here, it's an _mcount
> +		 * call location and r2 will have to be
> +		 * restored in the _mcount function.
> +		 */
> +		return 1;
> +	}
> +	return 0;
> +}
> +#else

*You* said this might page fault :)

Did we agree yet whether we insist on a streamlined compiler?
(GCC commit e95d0248dace required)?

If not:
if (instruction[-2] == PPC_INST_STD_LR)
  {
    if (instruction[-3] == PPC_INST_MFLR)
      return 1;
  }
else if (instruction[-2] == PPC_INST_MFLR)
    return 1;
return 0;

leaves less freedom for the compiler to "optimise".

Signed-off-by: Torsten Duwe <duwe@suse.de>

	Torsten

Re: [PATCH 04/12] powerpc/ftrace: Prepare for -mprofile-kernel

[Michael Ellerman]

On Thu, 2016-02-25 at 14:52 +0100, Torsten Duwe wrote:
> On Thu, Feb 25, 2016 at 01:28:27AM +1100, Michael Ellerman wrote:
> > @@ -450,17 +448,44 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> >  	return (unsigned long)&stubs[i];
> >  }
> >  
> > +#ifdef CC_USING_MPROFILE_KERNEL
> > +static int is_early_mcount_callsite(u32 *instruction)
> > +{
> > +	/* -mprofile-kernel sequence starting with
> > +	 * mflr r0 and maybe std r0, LRSAVE(r1).
> > +	 */
> > +	if ((instruction[-3] == PPC_INST_MFLR &&
> > +	     instruction[-2] == PPC_INST_STD_LR) ||
> > +	    instruction[-2] == PPC_INST_MFLR) {
> > +		/* Nothing to be done here, it's an _mcount
> > +		 * call location and r2 will have to be
> > +		 * restored in the _mcount function.
> > +		 */
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +#else
> 
> *You* said this might page fault :)

It does :) - I fixed it up in patch 6, sorry I realise that's hard to review.

> Did we agree yet whether we insist on a streamlined compiler?
> (GCC commit e95d0248dace required)?

No we didn't really. I think you're right that it's not /too/ hard to support
both sequences. But we may change our mind in future :)

cheers

[PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to save/restore toc

[Michael Ellerman]

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/entry_64.S | 8 ++++++++
 1 file changed, 8 insertions(+)

Squash.

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 2a7313cfbc7d..9e77a2c8f218 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1237,6 +1237,11 @@ _GLOBAL(ftrace_graph_caller)
 	std	r5, 64(r1)
 	std	r4, 56(r1)
 	std	r3, 48(r1)
+
+	/* Save callee's TOC in the ABI compliant location */
+	std	r2, 24(r1)
+	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
+
 	mfctr	r4		/* ftrace_caller has moved local addr here */
 	std	r4, 40(r1)
 	mflr	r3		/* ftrace_caller has restored LR from stack */
@@ -1262,6 +1267,9 @@ _GLOBAL(ftrace_graph_caller)
 	ld	r4, 56(r1)
 	ld	r3, 48(r1)
 
+	/* Restore callee's TOC */
+	ld	r2, 24(r1)
+
 	addi	r1, r1, 112
 	mflr	r0
 	std	r0, LRSAVE(r1)
-- 
2.5.0

Re: [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to save/restore toc

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/entry_64.S | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> Squash.
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 2a7313cfbc7d..9e77a2c8f218 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1237,6 +1237,11 @@ _GLOBAL(ftrace_graph_caller)
>  	std	r5, 64(r1)
>  	std	r4, 56(r1)
>  	std	r3, 48(r1)
> +
> +	/* Save callee's TOC in the ABI compliant location */
> +	std	r2, 24(r1)
R2_STACK_OFFSET for readability?
> +	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
> +
>  	mfctr	r4		/* ftrace_caller has moved local addr here */
>  	std	r4, 40(r1)
>  	mflr	r3		/* ftrace_caller has restored LR from stack */
> @@ -1262,6 +1267,9 @@ _GLOBAL(ftrace_graph_caller)
>  	ld	r4, 56(r1)
>  	ld	r3, 48(r1)
>  
> +	/* Restore callee's TOC */
> +	ld	r2, 24(r1)
> +
>  	addi	r1, r1, 112
>  	mflr	r0
>  	std	r0, LRSAVE(r1)

Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Balbir Singh.

Re: [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to save/restore toc

[Michael Ellerman]

On Thu, 2016-02-25 at 11:30 +1100, Balbir Singh wrote:
> 
> On 25/02/16 01:28, Michael Ellerman wrote:
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > 
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 2a7313cfbc7d..9e77a2c8f218 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1237,6 +1237,11 @@ _GLOBAL(ftrace_graph_caller)
> >  	std	r5, 64(r1)
> >  	std	r4, 56(r1)
> >  	std	r3, 48(r1)
> > +
> > +	/* Save callee's TOC in the ABI compliant location */
> > +	std	r2, 24(r1)

> R2_STACK_OFFSET for readability?

Hmm, maybe. Personally when I see "24(r1)" what my brain reads is "stack TOC
save slot", but maybe I've been spending too much time with powerpc assembly.

R2_STACK_OFFSET is actually new, pulled out from the module code by Torsten.
Other code uses STK_GOT to mean the same thing. I don't really like either
name, so I'll probably leave do a clean up once this is in.

cheers

Re: [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to save/restore toc

[Torsten Duwe]

On Thu, Feb 25, 2016 at 11:30:38AM +1100, Balbir Singh wrote:
> On 25/02/16 01:28, Michael Ellerman wrote:
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> >  arch/powerpc/kernel/entry_64.S | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >

Ah, -mprofile-kernel, DYNAMIC_FTRACE but without REGS.
Hadn't considered that, thanks!

> >
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 2a7313cfbc7d..9e77a2c8f218 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1237,6 +1237,11 @@ _GLOBAL(ftrace_graph_caller)
> >  	std	r5, 64(r1)
> >  	std	r4, 56(r1)
> >  	std	r3, 48(r1)
> > +
> > +	/* Save callee's TOC in the ABI compliant location */
> > +	std	r2, 24(r1)
> R2_STACK_OFFSET for readability?

I have encountered LRSAVE vs. PPC_LR_STKOFF and
STK_GOT vs. R2_STACK_OFFSET, some usable in assembler source, some in C.

> > +	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
> > +
> >  	mfctr	r4		/* ftrace_caller has moved local addr here */
> >  	std	r4, 40(r1)
> >  	mflr	r3		/* ftrace_caller has restored LR from stack */
> > @@ -1262,6 +1267,9 @@ _GLOBAL(ftrace_graph_caller)
> >  	ld	r4, 56(r1)
> >  	ld	r3, 48(r1)
> >  
> > +	/* Restore callee's TOC */
> > +	ld	r2, 24(r1)
> > +
> >  	addi	r1, r1, 112
> >  	mflr	r0
> >  	std	r0, LRSAVE(r1)
> 
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Reviewed-by: Torsten Duwe <duwe@suse.de>

	Torsten

[PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite()

[Michael Ellerman]

is_early_mcount_callsite() needs to detect either the two instruction or
the three instruction versions of the _mcount() sequence.

But if we're running a kernel with the two instruction sequence, we need
to be careful not to read instruction - 2, otherwise we might fall off
the front of a page and cause an oops.

While we're here convert to bool to make the return semantics clear.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/module_64.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

Squash.

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 32c10e0d2aa5..495df4340623 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -449,27 +449,25 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
 }
 
 #ifdef CC_USING_MPROFILE_KERNEL
-static int is_early_mcount_callsite(u32 *instruction)
+static bool is_early_mcount_callsite(u32 *instruction)
 {
-	/* -mprofile-kernel sequence starting with
-	 * mflr r0 and maybe std r0, LRSAVE(r1).
+	/*
+	 * Check if this is one of the -mprofile-kernel sequences.
 	 */
-	if ((instruction[-3] == PPC_INST_MFLR &&
-	     instruction[-2] == PPC_INST_STD_LR) ||
-	    instruction[-2] == PPC_INST_MFLR) {
-		/* Nothing to be done here, it's an _mcount
-		 * call location and r2 will have to be
-		 * restored in the _mcount function.
-		 */
-		return 1;
-	}
-	return 0;
+	if (instruction[-1] == PPC_INST_STD_LR &&
+	    instruction[-2] == PPC_INST_MFLR)
+		return true;
+
+	if (instruction[-1] == PPC_INST_MFLR)
+		return true;
+
+	return false;
 }
 #else
 /* without -mprofile-kernel, mcount calls are never early */
-static int is_early_mcount_callsite(u32 *instruction)
+static bool is_early_mcount_callsite(u32 *instruction)
 {
-	return 0;
+	return false;
 }
 #endif
 
@@ -478,7 +476,7 @@ static int is_early_mcount_callsite(u32 *instruction)
 static int restore_r2(u32 *instruction, struct module *me)
 {
 	if (*instruction != PPC_INST_NOP) {
-		if (is_early_mcount_callsite(instruction))
+		if (is_early_mcount_callsite(instruction - 1))
 			return 1;
 		pr_err("%s: Expect noop after relocate, got %08x\n",
 		       me->name, *instruction);
-- 
2.5.0

Re: [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite()

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> is_early_mcount_callsite() needs to detect either the two instruction or
> the three instruction versions of the _mcount() sequence.
>
> But if we're running a kernel with the two instruction sequence, we need
> to be careful not to read instruction - 2, otherwise we might fall off
> the front of a page and cause an oops.
>
> While we're here convert to bool to make the return semantics clear.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/module_64.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
>
> Squash.
>
>
Do we even need to do this anymore?

Balbir Singh.

Re: [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite()

[Michael Ellerman]

On Thu, 2016-02-25 at 10:39 +1100, Balbir Singh wrote:
> On 25/02/16 01:28, Michael Ellerman wrote:
> > is_early_mcount_callsite() needs to detect either the two instruction or
> > the three instruction versions of the _mcount() sequence.
> > 
> > But if we're running a kernel with the two instruction sequence, we need
> > to be careful not to read instruction - 2, otherwise we might fall off
> > the front of a page and cause an oops.
> > 
> > While we're here convert to bool to make the return semantics clear.
> > 
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > 
> Do we even need to do this anymore?

Yes. Otherwise the code in apply_relocate_add() will see a far call with no nop
slot after it to do the toc restore, and it considers that a bug (which it
usually is, except mcount is special).

As we discussed today I'm hoping we can clean this code up a bit more in the
medium term, but this works for now.

cheers

Re: [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite()

[Torsten Duwe]

On Thu, Feb 25, 2016 at 09:28:32PM +1100, Michael Ellerman wrote:
> On Thu, 2016-02-25 at 10:39 +1100, Balbir Singh wrote:
> > On 25/02/16 01:28, Michael Ellerman wrote:
> > > is_early_mcount_callsite() needs to detect either the two instruction or
> > > the three instruction versions of the _mcount() sequence.
> > > 
> > > But if we're running a kernel with the two instruction sequence, we need
> > > to be careful not to read instruction - 2, otherwise we might fall off
> > > the front of a page and cause an oops.
> > > 
> > > While we're here convert to bool to make the return semantics clear.
> > > 
> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

I wouldn't mind if you had folded this into the previous patch, see comments there.

> > > 
> > Do we even need to do this anymore?
> 
> Yes. Otherwise the code in apply_relocate_add() will see a far call with no nop
> slot after it to do the toc restore, and it considers that a bug (which it
> usually is, except mcount is special).
> 
> As we discussed today I'm hoping we can clean this code up a bit more in the
> medium term, but this works for now.

Agreed.
Reviewed-by: Torsten Duwe <duwe@suse.de>

	Torsten

[PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation for ppc64le

[Michael Ellerman]

From: Torsten Duwe <duwe@lst.de>

Implement FTRACE_WITH_REGS for powerpc64, on ELF ABI v2.
Initial work started by Vojtech Pavlik, used with permission.

  * arch/powerpc/kernel/entry_64.S:
    - Implement an effective ftrace_caller that works from
      within the kernel binary as well as from modules.
  * arch/powerpc/kernel/ftrace.c:
    - be prepared to deal with ppc64 ELF ABI v2, especially
      calls to _mcount that result from gcc -mprofile-kernel
    - a little more error verbosity
  * arch/powerpc/kernel/module_64.c:
    - do not save the TOC pointer on the trampoline when the
      destination is ftrace_caller. This trampoline jump happens from
      a function prologue before a new stack frame is set up, so bad
      things may happen otherwise...
    - relax is_module_trampoline() to recognise the modified
      trampoline.

Signed-off-by: Torsten Duwe <duwe@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/ftrace.h |  5 +++
 arch/powerpc/kernel/entry_64.S    | 78 +++++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/ftrace.c      | 66 ++++++++++++++++++++++++++++++---
 arch/powerpc/kernel/module_64.c   | 16 ++++++++
 4 files changed, 159 insertions(+), 6 deletions(-)

Probably squash.

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index ef89b1465573..50ca7585abe2 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -46,6 +46,8 @@
 extern void _mcount(void);
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+# define FTRACE_ADDR ((unsigned long)ftrace_caller)
+# define FTRACE_REGS_ADDR FTRACE_ADDR
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
        /* reloction of mcount call site is the same as the address */
@@ -58,6 +60,9 @@ struct dyn_arch_ftrace {
 #endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
 #endif
 
 #if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_PPC64) && !defined(__ASSEMBLY__)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 9e77a2c8f218..149b659a25d9 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1148,6 +1148,7 @@ _GLOBAL(_mcount)
 	mtlr	r0
 	bctr
 
+#ifndef CC_USING_MPROFILE_KERNEL
 _GLOBAL_TOC(ftrace_caller)
 	/* Taken from output of objdump from lib64/glibc */
 	mflr	r3
@@ -1169,6 +1170,83 @@ _GLOBAL(ftrace_graph_stub)
 	ld	r0, 128(r1)
 	mtlr	r0
 	addi	r1, r1, 112
+#else
+_GLOBAL(ftrace_caller)
+	std	r0,LRSAVE(r1)
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+	mflr	r0
+	bl	2f
+2:	mflr	r12
+	mtlr	r0
+	mr      r0,r2   /* save callee's TOC */
+	addis	r2,r12,(.TOC.-ftrace_caller-12)@ha
+	addi    r2,r2,(.TOC.-ftrace_caller-12)@l
+#else
+	mr	r0,r2
+#endif
+	ld	r12,LRSAVE(r1)	/* get caller's address */
+
+	stdu	r1,-SWITCH_FRAME_SIZE(r1)
+
+	std     r12, _LINK(r1)
+	SAVE_8GPRS(0,r1)
+	std	r0, 24(r1)	/* save TOC */
+	SAVE_8GPRS(8,r1)
+	SAVE_8GPRS(16,r1)
+	SAVE_8GPRS(24,r1)
+
+	addis	r3,r2,function_trace_op@toc@ha
+	addi	r3,r3,function_trace_op@toc@l
+	ld	r5,0(r3)
+
+	mflr    r3
+	std     r3, _NIP(r1)
+	std	r3, 16(r1)
+	subi    r3, r3, MCOUNT_INSN_SIZE
+	mfmsr   r4
+	std     r4, _MSR(r1)
+	mfctr   r4
+	std     r4, _CTR(r1)
+	mfxer   r4
+	std     r4, _XER(r1)
+	mr	r4, r12
+	addi    r6, r1 ,STACK_FRAME_OVERHEAD
+
+.globl ftrace_call
+ftrace_call:
+	bl	ftrace_stub
+	nop
+
+	ld	r3, _NIP(r1)
+	mtlr	r3
+
+	REST_8GPRS(0,r1)
+	REST_8GPRS(8,r1)
+	REST_8GPRS(16,r1)
+	REST_8GPRS(24,r1)
+
+	addi r1, r1, SWITCH_FRAME_SIZE
+
+	ld	r12, LRSAVE(r1)  /* get caller's address */
+	mtlr	r12
+	mr	r2,r0		/* restore callee's TOC */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	stdu	r1, -112(r1)
+.globl ftrace_graph_call
+ftrace_graph_call:
+	b	ftrace_graph_stub
+_GLOBAL(ftrace_graph_stub)
+	addi	r1, r1, 112
+#endif
+
+	mflr	r0		/* move this LR to CTR */
+	mtctr	r0
+
+	ld	r0,LRSAVE(r1)	/* restore callee's lr at _mcount site */
+	mtlr	r0
+	bctr			/* jump after _mcount site */
+#endif /* CC_USING_MPROFILE_KERNEL */
 _GLOBAL(ftrace_stub)
 	blr
 #else
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index a1d95f20b017..c6408a399ac6 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -61,8 +61,11 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
 		return -EFAULT;
 
 	/* Make sure it is what we expect it to be */
-	if (replaced != old)
+	if (replaced != old) {
+		pr_err("%p: replaced (%#x) != old (%#x)",
+		(void *)ip, replaced, old);
 		return -EINVAL;
+	}
 
 	/* replace the text with the new text */
 	if (patch_instruction((unsigned int *)ip, new))
@@ -108,11 +111,13 @@ __ftrace_make_nop(struct module *mod,
 {
 	unsigned long entry, ptr, tramp;
 	unsigned long ip = rec->ip;
-	unsigned int op;
+	unsigned int op, pop;
 
 	/* read where this goes */
-	if (probe_kernel_read(&op, (void *)ip, sizeof(int)))
+	if (probe_kernel_read(&op, (void *)ip, sizeof(int))) {
+		pr_err("Fetching opcode failed.\n");
 		return -EFAULT;
+	}
 
 	/* Make sure that that this is still a 24bit jump */
 	if (!is_bl_op(op)) {
@@ -152,10 +157,51 @@ __ftrace_make_nop(struct module *mod,
 	 *
 	 * Use a b +8 to jump over the load.
 	 */
-	op = 0x48000008;	/* b +8 */
 
-	if (patch_instruction((unsigned int *)ip, op))
+	pop = PPC_INST_BRANCH | 8;	/* b +8 */
+
+	/*
+	 * Check what is in the next instruction. We can see ld r2,40(r1), but
+	 * on first pass after boot we will see mflr r0.
+	 */
+	if (probe_kernel_read(&op, (void *)(ip+4), MCOUNT_INSN_SIZE)) {
+		pr_err("Fetching op failed.\n");
+		return -EFAULT;
+	}
+
+	if (op != PPC_INST_LD_TOC)
+	{
+		unsigned int op0, op1;
+
+		if (probe_kernel_read(&op0, (void *)(ip-8), MCOUNT_INSN_SIZE)) {
+			pr_err("Fetching op0 failed.\n");
+			return -EFAULT;
+		}
+
+		if (probe_kernel_read(&op1, (void *)(ip-4), MCOUNT_INSN_SIZE)) {
+			pr_err("Fetching op1 failed.\n");
+			return -EFAULT;
+		}
+
+		/* mflr r0 ; [ std r0,LRSAVE(r1) ]? */
+		if ( (op0 != PPC_INST_MFLR ||
+		      op1 != PPC_INST_STD_LR)
+		     && op1 != PPC_INST_MFLR )
+		{
+			pr_err("Unexpected instructions around bl _mcount\n"
+			       "when enabling dynamic ftrace!\t"
+			       "(%08x,%08x,bl,%08x)\n", op0, op1, op);
+			return -EINVAL;
+		}
+
+		/* When using -mkernel_profile there is no load to jump over */
+		pop = PPC_INST_NOP;
+	}
+
+	if (patch_instruction((unsigned int *)ip, pop)) {
+		pr_err("Patching NOP failed.\n");
 		return -EPERM;
+	}
 
 	return 0;
 }
@@ -281,6 +327,14 @@ int ftrace_make_nop(struct module *mod,
 
 #ifdef CONFIG_MODULES
 #ifdef CONFIG_PPC64
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+			unsigned long addr)
+{
+	return ftrace_make_call(rec, addr);
+}
+#endif
+
 /* Examine the existing instructions for __ftrace_make_call.
  * They should effectively be a NOP, and follow formal constraints,
  * depending on the ABI. Return false if they don't.
@@ -348,7 +402,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 	return 0;
 }
-#else
+#else  /* !CONFIG_PPC64: */
 static int
 __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 495df4340623..a3a13fcfc99c 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -139,6 +139,19 @@ static u32 ppc64_stub_insns[] = {
 	0x4e800420			/* bctr */
 };
 
+#ifdef CC_USING_MPROFILE_KERNEL
+/* In case of _mcount calls or dynamic ftracing, Do not save the
+ * current callee's TOC (in R2) again into the original caller's stack
+ * frame during this trampoline hop. The stack frame already holds
+ * that of the original caller.  _mcount and ftrace_caller will take
+ * care of this TOC value themselves.
+ */
+#define SQUASH_TOC_SAVE_INSN(trampoline_addr) \
+	(((struct ppc64_stub_entry *)(trampoline_addr))->jump[2] = PPC_INST_NOP)
+#else
+#define SQUASH_TOC_SAVE_INSN(trampoline_addr)
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 int module_trampoline_target(struct module *mod, unsigned long addr,
 			     unsigned long *target)
@@ -608,6 +621,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 					return -ENOENT;
 				if (!restore_r2((u32 *)location + 1, me))
 					return -ENOEXEC;
+				/* Squash the TOC saver for profiler calls */
+				if (!strcmp("_mcount", strtab+sym->st_name))
+					SQUASH_TOC_SAVE_INSN(value);
 			} else
 				value += local_entry_offset(sym);
 
-- 
2.5.0

Re: [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation for ppc64le

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> From: Torsten Duwe <duwe@lst.de>
>
> Implement FTRACE_WITH_REGS for powerpc64, on ELF ABI v2.
> Initial work started by Vojtech Pavlik, used with permission.
>
>   * arch/powerpc/kernel/entry_64.S:
>     - Implement an effective ftrace_caller that works from
>       within the kernel binary as well as from modules.
>   * arch/powerpc/kernel/ftrace.c:
>     - be prepared to deal with ppc64 ELF ABI v2, especially
>       calls to _mcount that result from gcc -mprofile-kernel
>     - a little more error verbosity
>   * arch/powerpc/kernel/module_64.c:
>     - do not save the TOC pointer on the trampoline when the
>       destination is ftrace_caller. This trampoline jump happens from
>       a function prologue before a new stack frame is set up, so bad
>       things may happen otherwise...
>     - relax is_module_trampoline() to recognise the modified
>       trampoline.
>
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/ftrace.h |  5 +++
>  arch/powerpc/kernel/entry_64.S    | 78 +++++++++++++++++++++++++++++++++++++++
>  arch/powerpc/kernel/ftrace.c      | 66 ++++++++++++++++++++++++++++++---
>  arch/powerpc/kernel/module_64.c   | 16 ++++++++
>  4 files changed, 159 insertions(+), 6 deletions(-)
>
> Probably squash.
>
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index ef89b1465573..50ca7585abe2 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -46,6 +46,8 @@
>  extern void _mcount(void);
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> +# define FTRACE_ADDR ((unsigned long)ftrace_caller)
> +# define FTRACE_REGS_ADDR FTRACE_ADDR
>  static inline unsigned long ftrace_call_adjust(unsigned long addr)
>  {
>         /* reloction of mcount call site is the same as the address */
> @@ -58,6 +60,9 @@ struct dyn_arch_ftrace {
>  #endif /*  CONFIG_DYNAMIC_FTRACE */
>  #endif /* __ASSEMBLY__ */
>  
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#define ARCH_SUPPORTS_FTRACE_OPS 1
> +#endif
>  #endif
>  
>  #if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_PPC64) && !defined(__ASSEMBLY__)
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 9e77a2c8f218..149b659a25d9 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1148,6 +1148,7 @@ _GLOBAL(_mcount)
>  	mtlr	r0
>  	bctr
>  
> +#ifndef CC_USING_MPROFILE_KERNEL
>  _GLOBAL_TOC(ftrace_caller)
>  	/* Taken from output of objdump from lib64/glibc */
>  	mflr	r3
> @@ -1169,6 +1170,83 @@ _GLOBAL(ftrace_graph_stub)
>  	ld	r0, 128(r1)
>  	mtlr	r0
>  	addi	r1, r1, 112
> +#else
> +_GLOBAL(ftrace_caller)
> +	std	r0,LRSAVE(r1)
> +#if defined(_CALL_ELF) && _CALL_ELF == 2
> +	mflr	r0
> +	bl	2f
> +2:	mflr	r12
> +	mtlr	r0
> +	mr      r0,r2   /* save callee's TOC */
> +	addis	r2,r12,(.TOC.-ftrace_caller-12)@ha
> +	addi    r2,r2,(.TOC.-ftrace_caller-12)@l
> +#else
> +	mr	r0,r2
> +#endif
> +	ld	r12,LRSAVE(r1)	/* get caller's address */
> +
> +	stdu	r1,-SWITCH_FRAME_SIZE(r1)
> +
> +	std     r12, _LINK(r1)
> +	SAVE_8GPRS(0,r1)
> +	std	r0, 24(r1)	/* save TOC */
> +	SAVE_8GPRS(8,r1)
> +	SAVE_8GPRS(16,r1)
> +	SAVE_8GPRS(24,r1)
> +
> +	addis	r3,r2,function_trace_op@toc@ha
> +	addi	r3,r3,function_trace_op@toc@l
> +	ld	r5,0(r3)
> +
> +	mflr    r3
> +	std     r3, _NIP(r1)
> +	std	r3, 16(r1)
> +	subi    r3, r3, MCOUNT_INSN_SIZE
> +	mfmsr   r4
> +	std     r4, _MSR(r1)
> +	mfctr   r4
> +	std     r4, _CTR(r1)
> +	mfxer   r4
> +	std     r4, _XER(r1)
> +	mr	r4, r12
> +	addi    r6, r1 ,STACK_FRAME_OVERHEAD
> +
> +.globl ftrace_call
> +ftrace_call:
> +	bl	ftrace_stub
> +	nop
> +
> +	ld	r3, _NIP(r1)
> +	mtlr	r3
> +
> +	REST_8GPRS(0,r1)
> +	REST_8GPRS(8,r1)
> +	REST_8GPRS(16,r1)
> +	REST_8GPRS(24,r1)
> +
> +	addi r1, r1, SWITCH_FRAME_SIZE
> +
> +	ld	r12, LRSAVE(r1)  /* get caller's address */
> +	mtlr	r12
> +	mr	r2,r0		/* restore callee's TOC */
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	stdu	r1, -112(r1)
> +.globl ftrace_graph_call
> +ftrace_graph_call:
> +	b	ftrace_graph_stub
> +_GLOBAL(ftrace_graph_stub)
> +	addi	r1, r1, 112
> +#endif
> +
> +	mflr	r0		/* move this LR to CTR */
> +	mtctr	r0
> +
> +	ld	r0,LRSAVE(r1)	/* restore callee's lr at _mcount site */
> +	mtlr	r0
> +	bctr			/* jump after _mcount site */
> +#endif /* CC_USING_MPROFILE_KERNEL */
>  _GLOBAL(ftrace_stub)
>  	blr
>  #else
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index a1d95f20b017..c6408a399ac6 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -61,8 +61,11 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
>  		return -EFAULT;
>  
>  	/* Make sure it is what we expect it to be */
> -	if (replaced != old)
> +	if (replaced != old) {
> +		pr_err("%p: replaced (%#x) != old (%#x)",
> +		(void *)ip, replaced, old);
>  		return -EINVAL;
> +	}
>  
>  	/* replace the text with the new text */
>  	if (patch_instruction((unsigned int *)ip, new))
> @@ -108,11 +111,13 @@ __ftrace_make_nop(struct module *mod,
>  {
>  	unsigned long entry, ptr, tramp;
>  	unsigned long ip = rec->ip;
> -	unsigned int op;
> +	unsigned int op, pop;
>  
>  	/* read where this goes */
> -	if (probe_kernel_read(&op, (void *)ip, sizeof(int)))
> +	if (probe_kernel_read(&op, (void *)ip, sizeof(int))) {
> +		pr_err("Fetching opcode failed.\n");
>  		return -EFAULT;
> +	}
>  
>  	/* Make sure that that this is still a 24bit jump */
>  	if (!is_bl_op(op)) {
> @@ -152,10 +157,51 @@ __ftrace_make_nop(struct module *mod,
>  	 *
>  	 * Use a b +8 to jump over the load.
>  	 */
> -	op = 0x48000008;	/* b +8 */
>  
> -	if (patch_instruction((unsigned int *)ip, op))
> +	pop = PPC_INST_BRANCH | 8;	/* b +8 */
> +
Do we really need the bits below for safety? I would put then under DEBUG or DEBUG_FTRACE
> +	/*
> +	 * Check what is in the next instruction. We can see ld r2,40(r1), but
> +	 * on first pass after boot we will see mflr r0.
> +	 */
> +	if (probe_kernel_read(&op, (void *)(ip+4), MCOUNT_INSN_SIZE)) {
> +		pr_err("Fetching op failed.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (op != PPC_INST_LD_TOC)
> +	{
> +		unsigned int op0, op1;
> +
> +		if (probe_kernel_read(&op0, (void *)(ip-8), MCOUNT_INSN_SIZE)) {
> +			pr_err("Fetching op0 failed.\n");
> +			return -EFAULT;
> +		}
> +
> +		if (probe_kernel_read(&op1, (void *)(ip-4), MCOUNT_INSN_SIZE)) {
> +			pr_err("Fetching op1 failed.\n");
> +			return -EFAULT;
> +		}
> +
> +		/* mflr r0 ; [ std r0,LRSAVE(r1) ]? */
> +		if ( (op0 != PPC_INST_MFLR ||
> +		      op1 != PPC_INST_STD_LR)
> +		     && op1 != PPC_INST_MFLR )
> +		{
> +			pr_err("Unexpected instructions around bl _mcount\n"
> +			       "when enabling dynamic ftrace!\t"
> +			       "(%08x,%08x,bl,%08x)\n", op0, op1, op);
> +			return -EINVAL;
> +		}
> +
> +		/* When using -mkernel_profile there is no load to jump over */
> +		pop = PPC_INST_NOP;
> +	}
> +

The bits till here
> +	if (patch_instruction((unsigned int *)ip, pop)) {
> +		pr_err("Patching NOP failed.\n");
>  		return -EPERM;
> +	}
>  
>  	return 0;
>  }
> @@ -281,6 +327,14 @@ int ftrace_make_nop(struct module *mod,
>  
>  #ifdef CONFIG_MODULES
>  #ifdef CONFIG_PPC64
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> +			unsigned long addr)
> +{
> +	return ftrace_make_call(rec, addr);
> +}
> +#endif
> +
>  /* Examine the existing instructions for __ftrace_make_call.
>   * They should effectively be a NOP, and follow formal constraints,
>   * depending on the ABI. Return false if they don't.
> @@ -348,7 +402,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  
>  	return 0;
>  }
> -#else
> +#else  /* !CONFIG_PPC64: */
>  static int
>  __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  {
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 495df4340623..a3a13fcfc99c 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -139,6 +139,19 @@ static u32 ppc64_stub_insns[] = {
>  	0x4e800420			/* bctr */
>  };
>  
> +#ifdef CC_USING_MPROFILE_KERNEL
> +/* In case of _mcount calls or dynamic ftracing, Do not save the
> + * current callee's TOC (in R2) again into the original caller's stack
> + * frame during this trampoline hop. The stack frame already holds
> + * that of the original caller.  _mcount and ftrace_caller will take
> + * care of this TOC value themselves.
> + */
> +#define SQUASH_TOC_SAVE_INSN(trampoline_addr) \
> +	(((struct ppc64_stub_entry *)(trampoline_addr))->jump[2] = PPC_INST_NOP)
> +#else
> +#define SQUASH_TOC_SAVE_INSN(trampoline_addr)
> +#endif
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  int module_trampoline_target(struct module *mod, unsigned long addr,
>  			     unsigned long *target)
> @@ -608,6 +621,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>  					return -ENOENT;
>  				if (!restore_r2((u32 *)location + 1, me))
>  					return -ENOEXEC;
> +				/* Squash the TOC saver for profiler calls */
> +				if (!strcmp("_mcount", strtab+sym->st_name))
> +					SQUASH_TOC_SAVE_INSN(value);
I don't think we need this anymore, do we?
>  			} else
>  				value += local_entry_offset(sym);
>  

Re: [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation for ppc64le

[Torsten Duwe]

On Thu, Feb 25, 2016 at 11:48:59AM +1100, Balbir Singh wrote:
> > @@ -608,6 +621,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> >  					return -ENOENT;
> >  				if (!restore_r2((u32 *)location + 1, me))
> >  					return -ENOEXEC;
> > +				/* Squash the TOC saver for profiler calls */
> > +				if (!strcmp("_mcount", strtab+sym->st_name))
> > +					SQUASH_TOC_SAVE_INSN(value);
> I don't think we need this anymore, do we?

I'm not sure. Once a module is loaded, are all the "bl _mcount"s NOPed out
before any of its functions are run? If not, the _mcount trampoline will
be used, and it must not save R2!

	Torsten

Re: [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation for ppc64le

[Michael Ellerman]

On Thu, 2016-02-25 at 16:11 +0100, Torsten Duwe wrote:
> On Thu, Feb 25, 2016 at 11:48:59AM +1100, Balbir Singh wrote:

> > > @@ -608,6 +621,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > >  					return -ENOENT;
> > >  				if (!restore_r2((u32 *)location + 1, me))
> > >  					return -ENOEXEC;
> > > +				/* Squash the TOC saver for profiler calls */
> > > +				if (!strcmp("_mcount", strtab+sym->st_name))
> > > +					SQUASH_TOC_SAVE_INSN(value);
> > I don't think we need this anymore, do we?
>
> I'm not sure. Once a module is loaded, are all the "bl _mcount"s NOPed out
> before any of its functions are run? If not, the _mcount trampoline will
> be used, and it must not save R2!

With dynamic ftrace, yes they are all nop'ed out before the module runs. See
ftrace_module_init() called from load_module().

But with static ftrace they are just left as-is.

As this series is currently written you can't enable mprofile-kernel with
static ftrace. But that's a bit fragile, someone could easily send a patch to
enable it for static ftrace and we'd probably merge it without thinking about
this code. So I'll leave this as is for now, and we will clean it up once the
series is in.

cheers

[PATCH 08/12] powerpc/ftrace: Rework ftrace_caller()

[Michael Ellerman]

The main change is to just use paca->kernel_toc, rather than a branch to
+4 and mflr etc. That makes the code simpler and should also perform
better.

There was also a sequence after ftrace_call() where we load from
pt_regs->nip, move to LR, then a few instructions later load from LRSAVE
and move to LR. Instead I think we want to put pt_regs->nip into CTR and
branch to it later.

We also rework some of the SPR loads to hopefully speed them up a bit.
Also comment the asm much more, to hopefully make it clearer.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/entry_64.S | 94 ++++++++++++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 32 deletions(-)

Squash.

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 149b659a25d9..f347f50024b8 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1171,65 +1171,98 @@ _GLOBAL(ftrace_graph_stub)
 	mtlr	r0
 	addi	r1, r1, 112
 #else
+/*
+ *
+ * ftrace_caller() is the function that replaces _mcount() when ftrace is
+ * active.
+ *
+ * We arrive here after a function A calls function B, and we are the trace
+ * function for B. When we enter r1 points to A's stack frame, B has not yet
+ * had a chance to allocate one yet.
+ *
+ * Additionally r2 may point either to the TOC for A, or B, depending on
+ * whether B did a TOC setup sequence before calling us.
+ *
+ * On entry the LR points back to the _mcount() call site, and r0 holds the
+ * saved LR as it was on entry to B, ie. the original return address at the
+ * call site in A.
+ *
+ * Our job is to save the register state into a struct pt_regs (on the stack)
+ * and then arrange for the ftrace function to be called.
+ */
 _GLOBAL(ftrace_caller)
+	/* Save the original return address in A's stack frame */
 	std	r0,LRSAVE(r1)
-#if defined(_CALL_ELF) && _CALL_ELF == 2
-	mflr	r0
-	bl	2f
-2:	mflr	r12
-	mtlr	r0
-	mr      r0,r2   /* save callee's TOC */
-	addis	r2,r12,(.TOC.-ftrace_caller-12)@ha
-	addi    r2,r2,(.TOC.-ftrace_caller-12)@l
-#else
-	mr	r0,r2
-#endif
-	ld	r12,LRSAVE(r1)	/* get caller's address */
 
+	/* Create our stack frame + pt_regs */
 	stdu	r1,-SWITCH_FRAME_SIZE(r1)
 
-	std     r12, _LINK(r1)
+	/* Save all gprs to pt_regs */
 	SAVE_8GPRS(0,r1)
-	std	r0, 24(r1)	/* save TOC */
 	SAVE_8GPRS(8,r1)
 	SAVE_8GPRS(16,r1)
 	SAVE_8GPRS(24,r1)
 
+	/* Load special regs for save below */
+	mfmsr   r8
+	mfctr   r9
+	mfxer   r10
+	mfcr	r11
+
+	/* Get the _mcount() call site out of LR */
+	mflr	r7
+	/* Save it as pt_regs->nip & pt_regs->link */
+	std     r7, _NIP(r1)
+	std     r7, _LINK(r1)
+
+	/* Save callee's TOC in the ABI compliant location */
+	std	r2, 24(r1)
+	ld	r2,PACATOC(r13)	/* get kernel TOC in r2 */
+
 	addis	r3,r2,function_trace_op@toc@ha
 	addi	r3,r3,function_trace_op@toc@l
 	ld	r5,0(r3)
 
-	mflr    r3
-	std     r3, _NIP(r1)
-	std	r3, 16(r1)
-	subi    r3, r3, MCOUNT_INSN_SIZE
-	mfmsr   r4
-	std     r4, _MSR(r1)
-	mfctr   r4
-	std     r4, _CTR(r1)
-	mfxer   r4
-	std     r4, _XER(r1)
-	mr	r4, r12
+	/* Calculate ip from nip-4 into r3 for call below */
+	subi    r3, r7, MCOUNT_INSN_SIZE
+
+	/* Put the original return address in r4 as parent_ip */
+	mr	r4, r0
+
+	/* Save special regs */
+	std     r8, _MSR(r1)
+	std     r9, _CTR(r1)
+	std     r10, _XER(r1)
+	std     r11, _CCR(r1)
+
+	/* Load &pt_regs in r6 for call below */
 	addi    r6, r1 ,STACK_FRAME_OVERHEAD
 
+	/* ftrace_call(r3, r4, r5, r6) */
 .globl ftrace_call
 ftrace_call:
 	bl	ftrace_stub
 	nop
 
+	/* Load ctr with the possibly modified NIP */
 	ld	r3, _NIP(r1)
-	mtlr	r3
+	mtctr	r3
 
+	/* Restore gprs */
 	REST_8GPRS(0,r1)
 	REST_8GPRS(8,r1)
 	REST_8GPRS(16,r1)
 	REST_8GPRS(24,r1)
 
+	/* Restore callee's TOC */
+	ld	r2, 24(r1)
+
+	/* Pop our stack frame */
 	addi r1, r1, SWITCH_FRAME_SIZE
 
-	ld	r12, LRSAVE(r1)  /* get caller's address */
-	mtlr	r12
-	mr	r2,r0		/* restore callee's TOC */
+	/* Restore original LR for return to B */
+	ld	r0, LRSAVE(r1)
+	mtlr	r0
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	stdu	r1, -112(r1)
@@ -1240,9 +1273,6 @@ _GLOBAL(ftrace_graph_stub)
 	addi	r1, r1, 112
 #endif
 
-	mflr	r0		/* move this LR to CTR */
-	mtctr	r0
-
 	ld	r0,LRSAVE(r1)	/* restore callee's lr at _mcount site */
 	mtlr	r0
 	bctr			/* jump after _mcount site */
-- 
2.5.0

Re: [PATCH 08/12] powerpc/ftrace: Rework ftrace_caller()

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> The main change is to just use paca->kernel_toc, rather than a branch to
> +4 and mflr etc. That makes the code simpler and should also perform
> better.
>
> There was also a sequence after ftrace_call() where we load from
> pt_regs->nip, move to LR, then a few instructions later load from LRSAVE
> and move to LR. Instead I think we want to put pt_regs->nip into CTR and
> branch to it later.
>
> We also rework some of the SPR loads to hopefully speed them up a bit.
> Also comment the asm much more, to hopefully make it clearer.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/entry_64.S | 94 ++++++++++++++++++++++++++++--------------
>  1 file changed, 62 insertions(+), 32 deletions(-)
>
> Squash.
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 149b659a25d9..f347f50024b8 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1171,65 +1171,98 @@ _GLOBAL(ftrace_graph_stub)
>  	mtlr	r0
>  	addi	r1, r1, 112
>  #else
> +/*
> + *
> + * ftrace_caller() is the function that replaces _mcount() when ftrace is
> + * active.
> + *
> + * We arrive here after a function A calls function B, and we are the trace
> + * function for B. When we enter r1 points to A's stack frame, B has not yet
> + * had a chance to allocate one yet.
> + *
> + * Additionally r2 may point either to the TOC for A, or B, depending on
> + * whether B did a TOC setup sequence before calling us.
> + *
> + * On entry the LR points back to the _mcount() call site, and r0 holds the
> + * saved LR as it was on entry to B, ie. the original return address at the
> + * call site in A.
> + *
> + * Our job is to save the register state into a struct pt_regs (on the stack)
> + * and then arrange for the ftrace function to be called.
> + */
>  _GLOBAL(ftrace_caller)
> +	/* Save the original return address in A's stack frame */
>  	std	r0,LRSAVE(r1)
> -#if defined(_CALL_ELF) && _CALL_ELF == 2
> -	mflr	r0
> -	bl	2f
> -2:	mflr	r12
> -	mtlr	r0
> -	mr      r0,r2   /* save callee's TOC */
> -	addis	r2,r12,(.TOC.-ftrace_caller-12)@ha
> -	addi    r2,r2,(.TOC.-ftrace_caller-12)@l
> -#else
> -	mr	r0,r2
> -#endif
> -	ld	r12,LRSAVE(r1)	/* get caller's address */
>  
> +	/* Create our stack frame + pt_regs */
>  	stdu	r1,-SWITCH_FRAME_SIZE(r1)
>  
> -	std     r12, _LINK(r1)
> +	/* Save all gprs to pt_regs */
>  	SAVE_8GPRS(0,r1)
> -	std	r0, 24(r1)	/* save TOC */
>  	SAVE_8GPRS(8,r1)
>  	SAVE_8GPRS(16,r1)
>  	SAVE_8GPRS(24,r1)
>  
> +	/* Load special regs for save below */
> +	mfmsr   r8
> +	mfctr   r9
> +	mfxer   r10
> +	mfcr	r11
> +
> +	/* Get the _mcount() call site out of LR */
> +	mflr	r7
> +	/* Save it as pt_regs->nip & pt_regs->link */
> +	std     r7, _NIP(r1)
> +	std     r7, _LINK(r1)
> +
> +	/* Save callee's TOC in the ABI compliant location */
> +	std	r2, 24(r1)
> +	ld	r2,PACATOC(r13)	/* get kernel TOC in r2 */
> +
>  	addis	r3,r2,function_trace_op@toc@ha
>  	addi	r3,r3,function_trace_op@toc@l
>  	ld	r5,0(r3)
>  
> -	mflr    r3
> -	std     r3, _NIP(r1)
> -	std	r3, 16(r1)
> -	subi    r3, r3, MCOUNT_INSN_SIZE
> -	mfmsr   r4
> -	std     r4, _MSR(r1)
> -	mfctr   r4
> -	std     r4, _CTR(r1)
> -	mfxer   r4
> -	std     r4, _XER(r1)
> -	mr	r4, r12
> +	/* Calculate ip from nip-4 into r3 for call below */
> +	subi    r3, r7, MCOUNT_INSN_SIZE
> +
> +	/* Put the original return address in r4 as parent_ip */
> +	mr	r4, r0
> +
> +	/* Save special regs */
> +	std     r8, _MSR(r1)
> +	std     r9, _CTR(r1)
> +	std     r10, _XER(r1)
> +	std     r11, _CCR(r1)
> +
> +	/* Load &pt_regs in r6 for call below */
>  	addi    r6, r1 ,STACK_FRAME_OVERHEAD
>  
> +	/* ftrace_call(r3, r4, r5, r6) */
>  .globl ftrace_call
>  ftrace_call:
>  	bl	ftrace_stub
>  	nop
>  
> +	/* Load ctr with the possibly modified NIP */
>  	ld	r3, _NIP(r1)
> -	mtlr	r3
> +	mtctr	r3
>  
> +	/* Restore gprs */
>  	REST_8GPRS(0,r1)
>  	REST_8GPRS(8,r1)
>  	REST_8GPRS(16,r1)
>  	REST_8GPRS(24,r1)
>  
> +	/* Restore callee's TOC */
> +	ld	r2, 24(r1)
> +
> +	/* Pop our stack frame */
>  	addi r1, r1, SWITCH_FRAME_SIZE
>  
> -	ld	r12, LRSAVE(r1)  /* get caller's address */
> -	mtlr	r12
> -	mr	r2,r0		/* restore callee's TOC */
> +	/* Restore original LR for return to B */
> +	ld	r0, LRSAVE(r1)
> +	mtlr	r0
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	stdu	r1, -112(r1)
> @@ -1240,9 +1273,6 @@ _GLOBAL(ftrace_graph_stub)
>  	addi	r1, r1, 112
>  #endif
>  
> -	mflr	r0		/* move this LR to CTR */
> -	mtctr	r0
> -
>  	ld	r0,LRSAVE(r1)	/* restore callee's lr at _mcount site */
>  	mtlr	r0
>  	bctr			/* jump after _mcount site */

Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 08/12] powerpc/ftrace: Rework ftrace_caller()

[Torsten Duwe]

On Thu, Feb 25, 2016 at 01:28:31AM +1100, Michael Ellerman wrote:
> The main change is to just use paca->kernel_toc, rather than a branch to
> +4 and mflr etc. That makes the code simpler and should also perform
> better.

Indeed.

> There was also a sequence after ftrace_call() where we load from
> pt_regs->nip, move to LR, then a few instructions later load from LRSAVE
> and move to LR. Instead I think we want to put pt_regs->nip into CTR and
> branch to it later.

Yes, I did some of this cleanup in the livepatch implementation.

> We also rework some of the SPR loads to hopefully speed them up a bit.
> Also comment the asm much more, to hopefully make it clearer.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Reviewed-by: Torsten Duwe <duwe@suse.de>

	Torsten

[PATCH 09/12] powerpc/ftrace: Use generic ftrace_modify_all_code()

[Michael Ellerman]

From: Torsten Duwe <duwe@lst.de>

Convert powerpc's arch_ftrace_update_code() from its own version to use
the generic default functionality (without stop_machine -- our
instructions are properly aligned and the replacements atomic).

With this we gain error checking and the much-needed function_trace_op
handling.

Signed-off-by: Torsten Duwe <duwe@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/ftrace.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index c6408a399ac6..56e5bd53c323 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -527,20 +527,13 @@ void ftrace_replace_code(int enable)
 	}
 }
 
+/*
+ * Use the default ftrace_modify_all_code, but without
+ * stop_machine().
+ */
 void arch_ftrace_update_code(int command)
 {
-	if (command & FTRACE_UPDATE_CALLS)
-		ftrace_replace_code(1);
-	else if (command & FTRACE_DISABLE_CALLS)
-		ftrace_replace_code(0);
-
-	if (command & FTRACE_UPDATE_TRACE_FUNC)
-		ftrace_update_ftrace_func(ftrace_trace_function);
-
-	if (command & FTRACE_START_FUNC_RET)
-		ftrace_enable_ftrace_graph_caller();
-	else if (command & FTRACE_STOP_FUNC_RET)
-		ftrace_disable_ftrace_graph_caller();
+	ftrace_modify_all_code(command);
 }
 
 int __init ftrace_dyn_arch_init(void)
-- 
2.5.0

Re: [PATCH 09/12] powerpc/ftrace: Use generic ftrace_modify_all_code()

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> From: Torsten Duwe <duwe@lst.de>
>
> Convert powerpc's arch_ftrace_update_code() from its own version to use
> the generic default functionality (without stop_machine -- our
> instructions are properly aligned and the replacements atomic).
>
> With this we gain error checking and the much-needed function_trace_op
> handling.
>
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/ftrace.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index c6408a399ac6..56e5bd53c323 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -527,20 +527,13 @@ void ftrace_replace_code(int enable)
>  	}
>  }
>  
> +/*
> + * Use the default ftrace_modify_all_code, but without
> + * stop_machine().
> + */
>  void arch_ftrace_update_code(int command)
>  {
> -	if (command & FTRACE_UPDATE_CALLS)
> -		ftrace_replace_code(1);
> -	else if (command & FTRACE_DISABLE_CALLS)
> -		ftrace_replace_code(0);
> -
> -	if (command & FTRACE_UPDATE_TRACE_FUNC)
> -		ftrace_update_ftrace_func(ftrace_trace_function);
> -
> -	if (command & FTRACE_START_FUNC_RET)
> -		ftrace_enable_ftrace_graph_caller();
> -	else if (command & FTRACE_STOP_FUNC_RET)
> -		ftrace_disable_ftrace_graph_caller();
> +	ftrace_modify_all_code(command);
>  }
>  
>  int __init ftrace_dyn_arch_init(void)
Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Balbir

[PATCH 10/12] powerpc/ftrace: FTRACE_WITH_REGS configuration variables

[Michael Ellerman]

From: Torsten Duwe <duwe@lst.de>

* arch/powerpc/Makefile:
    - globally use -mprofile-kernel in case it's configured,
      available and bug-free.
  * arch/powerpc/gcc-mprofile-kernel-notrace.sh:
    - make sure -mprofile-kernel works and has none of the
      known bugs.
  * arch/powerpc/kernel/ftrace.c:
    - error out on compile with HAVE_DYNAMIC_FTRACE_WITH_REGS
      and a buggy compiler.
  * arch/powerpc/Kconfig / kernel/trace/Kconfig:
    - declare that ppc64le HAVE_MPROFILE_KERNEL and
      HAVE_DYNAMIC_FTRACE_WITH_REGS, and use it.

Signed-off-by: Torsten Duwe <duwe@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/Kconfig                        |  2 ++
 arch/powerpc/Makefile                       | 17 +++++++++++++++
 arch/powerpc/gcc-mprofile-kernel-notrace.sh | 33 +++++++++++++++++++++++++++++
 arch/powerpc/kernel/ftrace.c                |  5 +++++
 kernel/trace/Kconfig                        |  5 +++++
 5 files changed, 62 insertions(+)
 create mode 100755 arch/powerpc/gcc-mprofile-kernel-notrace.sh

FIXME, needs more work. We can't break the build for someone with an
unsupported toolchain.

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e4824fd04bb7..e5f288ca7e12 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -94,8 +94,10 @@ config PPC
 	select OF_RESERVED_MEM
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if PPC64 && CPU_LITTLE_ENDIAN
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
+	select HAVE_MPROFILE_KERNEL if PPC64 && CPU_LITTLE_ENDIAN
 	select SYSCTL_EXCEPTION_TRACE
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select VIRT_TO_BUS if !PPC64
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 96efd8213c1c..08a39523e17a 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -133,6 +133,23 @@ else
 CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
 endif
 
+ifeq ($(CONFIG_PPC64),y)
+ifdef CONFIG_HAVE_MPROFILE_KERNEL
+
+ifdef CONFIG_DYNAMIC_FTRACE
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/arch/powerpc/gcc-mprofile-kernel-notrace.sh $(CC) -I$(srctree)/include -D__KERNEL__), y)
+CC_USING_MPROFILE_KERNEL := -mprofile-kernel
+endif
+endif
+
+ifdef CC_USING_MPROFILE_KERNEL
+CC_FLAGS_FTRACE	:= -pg $(CC_USING_MPROFILE_KERNEL)
+KBUILD_CPPFLAGS	+= -DCC_USING_MPROFILE_KERNEL
+endif
+
+endif
+endif
+
 CFLAGS-$(CONFIG_CELL_CPU) += $(call cc-option,-mcpu=cell)
 CFLAGS-$(CONFIG_POWER4_CPU) += $(call cc-option,-mcpu=power4)
 CFLAGS-$(CONFIG_POWER5_CPU) += $(call cc-option,-mcpu=power5)
diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
new file mode 100755
index 000000000000..68d6482d56ab
--- /dev/null
+++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+# Test whether the compile option -mprofile-kernel
+# generates profiling code ( = a call to mcount), and
+# whether a function without any global references sets
+# the TOC pointer properly at the beginning, and
+# whether the "notrace" function attribute successfully
+# suppresses the _mcount call.
+
+echo "int func() { return 0; }" | \
+    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+    grep -q "mcount"
+
+trace_result=$?
+
+echo "int func() { return 0; }" | \
+    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
+
+leaf_toc_result=$?
+
+/bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \
+    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+    grep -q "mcount"
+
+notrace_result=$?
+
+if [ "$trace_result" -eq "0" -a \
+	"$leaf_toc_result" -eq "0" -a \
+	"$notrace_result" -eq "1" ]; then
+	echo y
+else
+	echo n
+fi
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 56e5bd53c323..f190528e3781 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -28,6 +28,11 @@
 
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+#if defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && defined(CONFIG_PPC64) && \
+  !defined(CC_USING_MPROFILE_KERNEL)
+#error "DYNAMIC_FTRACE_WITH_REGS requires working -mprofile-kernel"
+#endif
+
 static unsigned int
 ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
 {
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index e45db6b0d878..a138f6d866ae 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -52,6 +52,11 @@ config HAVE_FENTRY
 	help
 	  Arch supports the gcc options -pg with -mfentry
 
+config HAVE_MPROFILE_KERNEL
+	bool
+	help
+	  Arch supports the gcc options -pg with -mprofile-kernel
+
 config HAVE_C_RECORDMCOUNT
 	bool
 	help
-- 
2.5.0

Re: [PATCH 10/12] powerpc/ftrace: FTRACE_WITH_REGS configuration variables

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> From: Torsten Duwe <duwe@lst.de>
>
> * arch/powerpc/Makefile:
>     - globally use -mprofile-kernel in case it's configured,
>       available and bug-free.
>   * arch/powerpc/gcc-mprofile-kernel-notrace.sh:
>     - make sure -mprofile-kernel works and has none of the
>       known bugs.
>   * arch/powerpc/kernel/ftrace.c:
>     - error out on compile with HAVE_DYNAMIC_FTRACE_WITH_REGS
>       and a buggy compiler.
>   * arch/powerpc/Kconfig / kernel/trace/Kconfig:
>     - declare that ppc64le HAVE_MPROFILE_KERNEL and
>       HAVE_DYNAMIC_FTRACE_WITH_REGS, and use it.
>
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/Kconfig                        |  2 ++
>  arch/powerpc/Makefile                       | 17 +++++++++++++++
>  arch/powerpc/gcc-mprofile-kernel-notrace.sh | 33 +++++++++++++++++++++++++++++
>  arch/powerpc/kernel/ftrace.c                |  5 +++++
>  kernel/trace/Kconfig                        |  5 +++++
>  5 files changed, 62 insertions(+)
>  create mode 100755 arch/powerpc/gcc-mprofile-kernel-notrace.sh
>
> FIXME, needs more work. We can't break the build for someone with an
> unsupported toolchain.
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index e4824fd04bb7..e5f288ca7e12 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -94,8 +94,10 @@ config PPC
>  	select OF_RESERVED_MEM
>  	select HAVE_FTRACE_MCOUNT_RECORD
>  	select HAVE_DYNAMIC_FTRACE
> +	select HAVE_DYNAMIC_FTRACE_WITH_REGS if PPC64 && CPU_LITTLE_ENDIAN
>  	select HAVE_FUNCTION_TRACER
>  	select HAVE_FUNCTION_GRAPH_TRACER
> +	select HAVE_MPROFILE_KERNEL if PPC64 && CPU_LITTLE_ENDIAN
>  	select SYSCTL_EXCEPTION_TRACE
>  	select ARCH_WANT_OPTIONAL_GPIOLIB
>  	select VIRT_TO_BUS if !PPC64
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 96efd8213c1c..08a39523e17a 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -133,6 +133,23 @@ else
>  CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
>  endif
>  
> +ifeq ($(CONFIG_PPC64),y)
> +ifdef CONFIG_HAVE_MPROFILE_KERNEL
> +
> +ifdef CONFIG_DYNAMIC_FTRACE
> +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/arch/powerpc/gcc-mprofile-kernel-notrace.sh $(CC) -I$(srctree)/include -D__KERNEL__), y)
> +CC_USING_MPROFILE_KERNEL := -mprofile-kernel
> +endif
> +endif
> +
> +ifdef CC_USING_MPROFILE_KERNEL
> +CC_FLAGS_FTRACE	:= -pg $(CC_USING_MPROFILE_KERNEL)
> +KBUILD_CPPFLAGS	+= -DCC_USING_MPROFILE_KERNEL
> +endif
> +
> +endif
> +endif
> +
>  CFLAGS-$(CONFIG_CELL_CPU) += $(call cc-option,-mcpu=cell)
>  CFLAGS-$(CONFIG_POWER4_CPU) += $(call cc-option,-mcpu=power4)
>  CFLAGS-$(CONFIG_POWER5_CPU) += $(call cc-option,-mcpu=power5)
> diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> new file mode 100755
> index 000000000000..68d6482d56ab
> --- /dev/null
> +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +# Test whether the compile option -mprofile-kernel
> +# generates profiling code ( = a call to mcount), and
> +# whether a function without any global references sets
> +# the TOC pointer properly at the beginning, and
> +# whether the "notrace" function attribute successfully
> +# suppresses the _mcount call.
> +
> +echo "int func() { return 0; }" | \
> +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> +    grep -q "mcount"
> +
> +trace_result=$?
> +
> +echo "int func() { return 0; }" | \
> +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> +
> +leaf_toc_result=$?
> +
We should remove this bit, we don't need a TOC for leaf procedures anymore
> +/bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \
> +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> +    grep -q "mcount"
> +
> +notrace_result=$?
> +
> +if [ "$trace_result" -eq "0" -a \
> +	"$leaf_toc_result" -eq "0" -a \
> +	"$notrace_result" -eq "1" ]; then
> +	echo y
> +else
> +	echo n
> +fi
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 56e5bd53c323..f190528e3781 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -28,6 +28,11 @@
>  
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> +#if defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && defined(CONFIG_PPC64) && \
> +  !defined(CC_USING_MPROFILE_KERNEL)
> +#error "DYNAMIC_FTRACE_WITH_REGS requires working -mprofile-kernel"
> +#endif
> +
>  static unsigned int
>  ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
>  {
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index e45db6b0d878..a138f6d866ae 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -52,6 +52,11 @@ config HAVE_FENTRY
>  	help
>  	  Arch supports the gcc options -pg with -mfentry
>  
> +config HAVE_MPROFILE_KERNEL
> +	bool
> +	help
> +	  Arch supports the gcc options -pg with -mprofile-kernel
> +
>  config HAVE_C_RECORDMCOUNT
>  	bool
>  	help

Re: [PATCH 10/12] powerpc/ftrace: FTRACE_WITH_REGS configuration variables

[Torsten Duwe]

On Thu, Feb 25, 2016 at 12:11:33PM +1100, Balbir Singh wrote:
> On 25/02/16 01:28, Michael Ellerman wrote:
> >
> > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > new file mode 100755
> > index 000000000000..68d6482d56ab
> > --- /dev/null
> > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > @@ -0,0 +1,33 @@
> > +#!/bin/sh
> > +# Test whether the compile option -mprofile-kernel
> > +# generates profiling code ( = a call to mcount), and
> > +# whether a function without any global references sets
> > +# the TOC pointer properly at the beginning, and

Remove the above two lines, for completeness,

> > +# whether the "notrace" function attribute successfully
> > +# suppresses the _mcount call.
> > +
> > +echo "int func() { return 0; }" | \
> > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > +    grep -q "mcount"
> > +
> > +trace_result=$?
> > +
> > +echo "int func() { return 0; }" | \
> > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > +
> > +leaf_toc_result=$?
> > +
> We should remove this bit, we don't need a TOC for leaf procedures anymore

Exactly. I thought it was a bug when I wrote this test, Michael insisted
it was a feature :-)

> > +/bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \
> > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > +    grep -q "mcount"
> > +
> > +notrace_result=$?
> > +
> > +if [ "$trace_result" -eq "0" -a \
> > +	"$leaf_toc_result" -eq "0" -a \

In particular, remove this ^ line.

> > +	"$notrace_result" -eq "1" ]; then
> > +	echo y
> > +else
> > +	echo n
> > +fi

That version would have made it into my v9.

Signed-off-by: Torsten Duwe <duwe@suse.de>

	Torsten

[PATCH 11/12] powerpc/ftrace: Rework __ftrace_make_nop()

[Michael Ellerman]

__ftrace_make_nop() needs to detect either the two instruction or
the three instruction versions of the _mcount() sequence.

But if we're running a kernel with the two instruction sequence, we need
to be careful not to read from ip - 8, or we'll fault and (possibly)
incorrectly declare the sequence doesn't match.

To keep the code simpler just look at ip - 4, and if it is either of the
expected instructions declare it good. We've already passed a lot of
other checks.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/ftrace.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

Squash.

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index f190528e3781..fe7486f9849e 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -174,28 +174,19 @@ __ftrace_make_nop(struct module *mod,
 		return -EFAULT;
 	}
 
-	if (op != PPC_INST_LD_TOC)
-	{
-		unsigned int op0, op1;
+	if (op != PPC_INST_LD_TOC) {
+		unsigned int inst;
 
-		if (probe_kernel_read(&op0, (void *)(ip-8), MCOUNT_INSN_SIZE)) {
-			pr_err("Fetching op0 failed.\n");
+		if (probe_kernel_read(&inst, (void *)(ip - 4), 4)) {
+			pr_err("Fetching instruction at %lx failed.\n", ip - 4);
 			return -EFAULT;
 		}
 
-		if (probe_kernel_read(&op1, (void *)(ip-4), MCOUNT_INSN_SIZE)) {
-			pr_err("Fetching op1 failed.\n");
-			return -EFAULT;
-		}
-
-		/* mflr r0 ; [ std r0,LRSAVE(r1) ]? */
-		if ( (op0 != PPC_INST_MFLR ||
-		      op1 != PPC_INST_STD_LR)
-		     && op1 != PPC_INST_MFLR )
-		{
+		/* We expect either a mlfr r0, or a std r0, LRSAVE(r1) */
+		if (inst != PPC_INST_MFLR && inst != PPC_INST_STD_LR) {
 			pr_err("Unexpected instructions around bl _mcount\n"
 			       "when enabling dynamic ftrace!\t"
-			       "(%08x,%08x,bl,%08x)\n", op0, op1, op);
+			       "(%08x,bl,%08x)\n", inst, op);
 			return -EINVAL;
 		}
 
-- 
2.5.0

[PATCH 12/12] powerpc/ftrace: Disable profiling for some files

[Michael Ellerman]

From: Torsten Duwe <duwe@lst.de>

Using -mprofile-kernel on early boot code not only confuses the
checker but is also useless, as the infrastructure is not yet in
place. Proceed like with -pg (remove it from CFLAGS), equally with
time.o, ftrace and its helper files.

  * arch/powerpc/kernel/Makefile,
    arch/powerpc/lib/Makefile:
    - remove -mprofile-kernel from low level, boot code and
      code-patching objects' CFLAGS.

Signed-off-by: Torsten Duwe <duwe@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/Makefile | 12 ++++++------
 arch/powerpc/lib/Makefile    |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 794f22adf99d..44667fde7ae4 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -16,14 +16,14 @@ endif
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
-CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog -mprofile-kernel
 # do not trace tracer code
-CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog -mprofile-kernel
 # timers used by tracing
-CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog -mprofile-kernel
 endif
 
 obj-y				:= cputable.o ptrace.o syscalls.o \
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index a47e14277fd8..98e22b2d7bec 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -6,8 +6,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
 
 ccflags-$(CONFIG_PPC64)	:= $(NO_MINIMAL_TOC)
 
-CFLAGS_REMOVE_code-patching.o = -pg
-CFLAGS_REMOVE_feature-fixups.o = -pg
+CFLAGS_REMOVE_code-patching.o = -pg -mprofile-kernel
+CFLAGS_REMOVE_feature-fixups.o = -pg -mprofile-kernel
 
 obj-y += string.o alloc.o crtsavres.o ppc_ksyms.o code-patching.o \
 	 feature-fixups.o
-- 
2.5.0

Re: [PATCH 12/12] powerpc/ftrace: Disable profiling for some files

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> From: Torsten Duwe <duwe@lst.de>
>
> Using -mprofile-kernel on early boot code not only confuses the
> checker but is also useless, as the infrastructure is not yet in
> place. Proceed like with -pg (remove it from CFLAGS), equally with
> time.o, ftrace and its helper files.
>
>   * arch/powerpc/kernel/Makefile,
>     arch/powerpc/lib/Makefile:
>     - remove -mprofile-kernel from low level, boot code and
>       code-patching objects' CFLAGS.
>
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/Makefile | 12 ++++++------
>  arch/powerpc/lib/Makefile    |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 794f22adf99d..44667fde7ae4 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -16,14 +16,14 @@ endif
>  
>  ifdef CONFIG_FUNCTION_TRACER
>  # Do not trace early boot code
> -CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
> -CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
> -CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
> -CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog -mprofile-kernel
> +CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog -mprofile-kernel
> +CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog -mprofile-kernel
> +CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog -mprofile-kernel
>  # do not trace tracer code
> -CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog -mprofile-kernel
>  # timers used by tracing
> -CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
> +CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog -mprofile-kernel
>  endif
>  
>  obj-y				:= cputable.o ptrace.o syscalls.o \
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index a47e14277fd8..98e22b2d7bec 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -6,8 +6,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
>  
>  ccflags-$(CONFIG_PPC64)	:= $(NO_MINIMAL_TOC)
>  
> -CFLAGS_REMOVE_code-patching.o = -pg
> -CFLAGS_REMOVE_feature-fixups.o = -pg
> +CFLAGS_REMOVE_code-patching.o = -pg -mprofile-kernel
> +CFLAGS_REMOVE_feature-fixups.o = -pg -mprofile-kernel
>  
>  obj-y += string.o alloc.o crtsavres.o ppc_ksyms.o code-patching.o \
>  	 feature-fixups.o

Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once

[Balbir Singh]

On 25/02/16 01:28, Michael Ellerman wrote:
> Currently we generate the module stub for ftrace_caller() at the bottom
> of apply_relocate_add(). However apply_relocate_add() is potentially
> called more than once per module, which means we will try to generate
> the ftrace_caller() stub multiple times.
This makes sense
Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once

[Balbir Singh]

On 25/02/16 10:55, Balbir Singh wrote:
>
> On 25/02/16 01:28, Michael Ellerman wrote:
>> Currently we generate the module stub for ftrace_caller() at the bottom
>> of apply_relocate_add(). However apply_relocate_add() is potentially
>> called more than once per module, which means we will try to generate
>> the ftrace_caller() stub multiple times.
> This makes sense
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>
For the entire series also

Tested-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once

[Torsten Duwe]

On Thu, Feb 25, 2016 at 01:28:24AM +1100, Michael Ellerman wrote:
> Currently we generate the module stub for ftrace_caller() at the bottom
> of apply_relocate_add(). However apply_relocate_add() is potentially
> called more than once per module, which means we will try to generate
> the ftrace_caller() stub multiple times.
> 
> Although the current code deals with that correctly, ie. it only
> generates a stub the first time, it would be clearer to only try to
> generate the stub once.
> 
> Note also on first reading it may appear that we generate a different
> stub for each section that requires relocation, but that is not the
> case. The code in stub_for_addr() that searches for an existing stub
> uses sechdrs[me->arch.stubs_section], ie. the single stub section for
> this module.
> 
> A cleaner approach is to only generate the ftrace_caller() stub once,
> from module_finalize(). An additional benefit is we can clean the ifdefs
> up a little.
> 
> Finally we must propagate the const'ness of some of the pointers passed
> to module_finalize(), but that is also an improvement.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Reviewed-by: Torsten Duwe <duwe@suse.de>

	Torsten

Re: [PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once

[Kamalesh Babulal]

* Michael Ellerman <mpe@ellerman.id.au> [2016-02-25 01:28:24]:

> Currently we generate the module stub for ftrace_caller() at the bottom
> of apply_relocate_add(). However apply_relocate_add() is potentially
> called more than once per module, which means we will try to generate
> the ftrace_caller() stub multiple times.
> 
> Although the current code deals with that correctly, ie. it only
> generates a stub the first time, it would be clearer to only try to
> generate the stub once.
> 
> Note also on first reading it may appear that we generate a different
> stub for each section that requires relocation, but that is not the
> case. The code in stub_for_addr() that searches for an existing stub
> uses sechdrs[me->arch.stubs_section], ie. the single stub section for
> this module.
> 
> A cleaner approach is to only generate the ftrace_caller() stub once,
> from module_finalize(). An additional benefit is we can clean the ifdefs
> up a little.
> 
> Finally we must propagate the const'ness of some of the pointers passed
> to module_finalize(), but that is also an improvement.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

For all of the patches in the series.

Tested-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>


Regards,
Kamalesh.