Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1342069 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 01/12] powerpc/module: Only try to generate the ftrace_caller() stub once Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:30 +0100
    [PATCH 02/12] powerpc/module: Mark module stubs with a magic value Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic  value Balbir Singh <bsingharora@gmail.com> - 2016-02-25 01:10 +0100
        Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic  value Michael Ellerman <mpe@ellerman.id.au> - 2016-02-25 07:50 +0100
      Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic  value Torsten Duwe <duwe@lst.de> - 2016-02-25 14:20 +0100
        Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic  value Michael Ellerman <mpe@ellerman.id.au> - 2016-02-26 11:40 +0100
    [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite() Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite() Balbir Singh <bsingharora@gmail.com> - 2016-02-25 00:40 +0100
        Re: [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite() Michael Ellerman <mpe@ellerman.id.au> - 2016-02-25 11:30 +0100
          Re: [PATCH 06/12] powerpc/module: Rework is_early_mcount_callsite() Torsten Duwe <duwe@lst.de> - 2016-02-25 15:10 +0100
    [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to save/restore toc Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to  save/restore toc Balbir Singh <bsingharora@gmail.com> - 2016-02-25 01:40 +0100
        Re: [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to  save/restore toc Michael Ellerman <mpe@ellerman.id.au> - 2016-02-25 11:50 +0100
        Re: [PATCH 05/12] powerpc/ftrace: ftrace_graph_caller() needs to  save/restore toc Torsten Duwe <duwe@lst.de> - 2016-02-25 15:10 +0100
    [PATCH 10/12] powerpc/ftrace: FTRACE_WITH_REGS configuration variables Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 10/12] powerpc/ftrace: FTRACE_WITH_REGS configuration  variables Balbir Singh <bsingharora@gmail.com> - 2016-02-25 02:20 +0100
        Re: [PATCH 10/12] powerpc/ftrace: FTRACE_WITH_REGS configuration  variables Torsten Duwe <duwe@lst.de> - 2016-02-25 15:40 +0100
    [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation for ppc64le Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation for  ppc64le Balbir Singh <bsingharora@gmail.com> - 2016-02-25 01:50 +0100
        Re: [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation  for ppc64le Torsten Duwe <duwe@lst.de> - 2016-02-25 16:20 +0100
          Re: [PATCH 07/12] powerpc/ftrace: FTRACE_WITH_REGS implementation  for ppc64le Michael Ellerman <mpe@ellerman.id.au> - 2016-02-26 11:20 +0100
    [PATCH 09/12] powerpc/ftrace: Use generic ftrace_modify_all_code() Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 09/12] powerpc/ftrace: Use generic  ftrace_modify_all_code() Balbir Singh <bsingharora@gmail.com> - 2016-02-25 02:20 +0100
    [PATCH 03/12] powerpc/module: Create a special stub for ftrace_caller() Michael Ellerman <mpe@ellerman.id.au> - 2016-02-24 15:40 +0100
      Re: [PATCH 03/12] powerpc/module: Create a special stub for  ftrace_caller() Balbir Singh <bsingharora@gmail.com> - 2016-02-25 01:10 +0100
        Re: [PATCH 03/12] powerpc/module: Create a special stub for  ftrace_caller() Michael Ellerman <mpe@ellerman.id.au> - 2016-02-25 11:50 +0100
        Re: [PATCH 03/12] powerpc/module: Create a special stub for  ftrace_caller() Torsten Duwe <duwe@lst.de> - 2016-02-25 14:40 +0100
          Re: [PATCH 03/12] powerpc/module: Create a special stub for  ftrace_caller() Michael Ellerman <mpe@ellerman.id.au> - 2016-02-26 11:40 +0100
    Re: [PATCH 01/12] powerpc/module: Only try to generate the  ftrace_caller() stub once Balbir Singh <bsingharora@gmail.com> - 2016-02-25 01:00 +0100
      Re: [PATCH 01/12] powerpc/module: Only try to generate the  ftrace_caller() stub once Balbir Singh <bsingharora@gmail.com> - 2016-02-25 05:40 +0100
    Re: [PATCH 01/12] powerpc/module: Only try to generate the  ftrace_caller() stub once Torsten Duwe <duwe@lst.de> - 2016-02-25 14:10 +0100
    Re: [PATCH 01/12] powerpc/module: Only try to generate the  ftrace_caller() stub once Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> - 2016-02-25 15:40 +0100

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web