All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] arm64: ftrace cleanup + FTRACE_WITH_REGS
@ 2019-10-21 16:34 ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

Hi,

This series is a reworked version of Torsten's v8 FTRACE_WITH_REGS
series [1]. I've tried to rework the existing code in preparatory
patches so that the patchable-function-entry bits slot in with fewer
surprises. This version is based on v5.4-rc3, and can be found in my
arm64/ftrace-with-regs branch [2].

I've added an (optional) ftrace_init_nop(), which the core code uses to
initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
symbol, and more cleanly separates the one-time initialization of the
callsite from dynamic NOP<->CALL modification. Architectures which don't
implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.

I've moved the module PLT initialization to module load time, which
simplifies runtime callsite modification. This also means that we don't
transitently mark the module text RW, and will allow for the removal of
module_disable_ro().

Since the last posting, parisc gained ftrace support using
patchable-function-entry. I've made the handling of module callsite
locations common in kernel/module.c with a new FTRACE_CALLSITE_SECTION
definition, and removed the newly redundant bits from arch/parisc.

Thanks,
Mark.

[1] https://lore.kernel.org/r/20190208150826.44EBC68DD2@newverein.lst.de
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs

Mark Rutland (7):
  ftrace: add ftrace_init_nop()
  module/ftrace: handle patchable-function-entry
  arm64: module: rework special section handling
  arm64: module/ftrace: intialize PLT at load time
  arm64: insn: add encoder for MOV (register)
  arm64: asm-offsets: add S_FP
  arm64: ftrace: minimize ifdeffery

Torsten Duwe (1):
  arm64: implement ftrace with regs

 arch/arm64/Kconfig               |   2 +
 arch/arm64/Makefile              |   5 ++
 arch/arm64/include/asm/ftrace.h  |  23 +++++++
 arch/arm64/include/asm/insn.h    |   3 +
 arch/arm64/include/asm/module.h  |   2 +-
 arch/arm64/kernel/asm-offsets.c  |   1 +
 arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/ftrace.c       | 123 ++++++++++++++++++++--------------
 arch/arm64/kernel/insn.c         |  13 ++++
 arch/arm64/kernel/module-plts.c  |   3 +-
 arch/arm64/kernel/module.c       |  57 +++++++++++++---
 arch/parisc/Makefile             |   1 -
 arch/parisc/kernel/module.c      |  10 ++-
 arch/parisc/kernel/module.lds    |   7 --
 include/linux/ftrace.h           |   5 ++
 kernel/module.c                  |   2 +-
 kernel/trace/ftrace.c            |  13 +++-
 17 files changed, 330 insertions(+), 80 deletions(-)
 delete mode 100644 arch/parisc/kernel/module.lds

-- 
2.11.0


^ permalink raw reply	[flat|nested] 32+ messages in thread
* [PATCH 0/8] arm64: ftrace cleanup + FTRACE_WITH_REGS
@ 2019-10-21 16:34 ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

Hi,

This series is a reworked version of Torsten's v8 FTRACE_WITH_REGS
series [1]. I've tried to rework the existing code in preparatory
patches so that the patchable-function-entry bits slot in with fewer
surprises. This version is based on v5.4-rc3, and can be found in my
arm64/ftrace-with-regs branch [2].

I've added an (optional) ftrace_init_nop(), which the core code uses to
initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
symbol, and more cleanly separates the one-time initialization of the
callsite from dynamic NOP<->CALL modification. Architectures which don't
implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.

I've moved the module PLT initialization to module load time, which
simplifies runtime callsite modification. This also means that we don't
transitently mark the module text RW, and will allow for the removal of
module_disable_ro().

Since the last posting, parisc gained ftrace support using
patchable-function-entry. I've made the handling of module callsite
locations common in kernel/module.c with a new FTRACE_CALLSITE_SECTION
definition, and removed the newly redundant bits from arch/parisc.

Thanks,
Mark.

[1] https://lore.kernel.org/r/20190208150826.44EBC68DD2@newverein.lst.de
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs

Mark Rutland (7):
  ftrace: add ftrace_init_nop()
  module/ftrace: handle patchable-function-entry
  arm64: module: rework special section handling
  arm64: module/ftrace: intialize PLT at load time
  arm64: insn: add encoder for MOV (register)
  arm64: asm-offsets: add S_FP
  arm64: ftrace: minimize ifdeffery

Torsten Duwe (1):
  arm64: implement ftrace with regs

 arch/arm64/Kconfig               |   2 +
 arch/arm64/Makefile              |   5 ++
 arch/arm64/include/asm/ftrace.h  |  23 +++++++
 arch/arm64/include/asm/insn.h    |   3 +
 arch/arm64/include/asm/module.h  |   2 +-
 arch/arm64/kernel/asm-offsets.c  |   1 +
 arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/ftrace.c       | 123 ++++++++++++++++++++--------------
 arch/arm64/kernel/insn.c         |  13 ++++
 arch/arm64/kernel/module-plts.c  |   3 +-
 arch/arm64/kernel/module.c       |  57 +++++++++++++---
 arch/parisc/Makefile             |   1 -
 arch/parisc/kernel/module.c      |  10 ++-
 arch/parisc/kernel/module.lds    |   7 --
 include/linux/ftrace.h           |   5 ++
 kernel/module.c                  |   2 +-
 kernel/trace/ftrace.c            |  13 +++-
 17 files changed, 330 insertions(+), 80 deletions(-)
 delete mode 100644 arch/parisc/kernel/module.lds

-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

Architectures may need to perform special initialization of ftrace
callsites, and today they do so by special-casing ftrace_make_nop() when
the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
patchable-function-entry), we don't have an mcount-like symbol and don't
want a synthetic MCOUNT_ADDR, but we may need to perform some
initialization of callsites.

To make it possible to separate initialization from runtime
modification, and to handle cases without an mcount-like symbol, this
patch adds an optional ftrace_init_nop() function that architectures can
implement, which does not pass a branch address.

Where an architecture does not provide ftrace_init_nop(), we will fall
back to the existing behaviour of calling ftrace_make_nop() with
MCOUNT_ADDR.

At the same time, ftrace_code_disable() is renamed to
ftrace_code_init_disabled() to make it clearer that it is intended to
intialize a callsite into a disabled state, and is not for disabling a
callsite that has been runtime enabled.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Torsten Duwe <duwe@suse.de>
---
 kernel/trace/ftrace.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f296d89be757..afd7e210e595 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
 	return &iter->pg->records[iter->index];
 }
 
+#ifndef ftrace_init_nop
+static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+}
+#endif
+
 static int
-ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
+ftrace_code_init_disabled(struct module *mod, struct dyn_ftrace *rec)
 {
 	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return 0;
 
-	ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+	ret = ftrace_init_nop(mod, rec);
 	if (ret) {
 		ftrace_bug_type = FTRACE_BUG_INIT;
 		ftrace_bug(ret, rec);
@@ -2943,7 +2950,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
 			 * to the NOP instructions.
 			 */
 			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
-			    !ftrace_code_disable(mod, p))
+			    !ftrace_code_init_disabled(mod, p))
 				break;
 
 			update_cnt++;
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

Architectures may need to perform special initialization of ftrace
callsites, and today they do so by special-casing ftrace_make_nop() when
the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
patchable-function-entry), we don't have an mcount-like symbol and don't
want a synthetic MCOUNT_ADDR, but we may need to perform some
initialization of callsites.

To make it possible to separate initialization from runtime
modification, and to handle cases without an mcount-like symbol, this
patch adds an optional ftrace_init_nop() function that architectures can
implement, which does not pass a branch address.

Where an architecture does not provide ftrace_init_nop(), we will fall
back to the existing behaviour of calling ftrace_make_nop() with
MCOUNT_ADDR.

At the same time, ftrace_code_disable() is renamed to
ftrace_code_init_disabled() to make it clearer that it is intended to
intialize a callsite into a disabled state, and is not for disabling a
callsite that has been runtime enabled.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Torsten Duwe <duwe@suse.de>
---
 kernel/trace/ftrace.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f296d89be757..afd7e210e595 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
 	return &iter->pg->records[iter->index];
 }
 
+#ifndef ftrace_init_nop
+static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+}
+#endif
+
 static int
-ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
+ftrace_code_init_disabled(struct module *mod, struct dyn_ftrace *rec)
 {
 	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return 0;
 
-	ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+	ret = ftrace_init_nop(mod, rec);
 	if (ret) {
 		ftrace_bug_type = FTRACE_BUG_INIT;
 		ftrace_bug(ret, rec);
@@ -2943,7 +2950,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
 			 * to the NOP instructions.
 			 */
 			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
-			    !ftrace_code_disable(mod, p))
+			    !ftrace_code_init_disabled(mod, p))
 				break;
 
 			update_cnt++;
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 2/8] module/ftrace: handle patchable-function-entry
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

When using patchable-function-entry, the compiler will record the
callsites into a section named "__patchable_function_entries" rather
than "__mcount_loc". Let's abstract this difference behind a new
FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
explicitly (e.g. with custom module linker scripts).

As parisc currently handles this explicitly, it is fixed up accordingly,
with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
only defined when DYNAMIC_FTRACE is selected, the parisc module loading
code is updated to only use the definition in that case. When
DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
this removes some redundant work in that case.

I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
and verified that the section made it into the .ko files for modules.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@stackframe.org>
---
 arch/parisc/Makefile          |  1 -
 arch/parisc/kernel/module.c   | 10 +++++++---
 arch/parisc/kernel/module.lds |  7 -------
 include/linux/ftrace.h        |  5 +++++
 kernel/module.c               |  2 +-
 5 files changed, 13 insertions(+), 12 deletions(-)
 delete mode 100644 arch/parisc/kernel/module.lds

diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
index 36b834f1c933..dca8f2de8cf5 100644
--- a/arch/parisc/Makefile
+++ b/arch/parisc/Makefile
@@ -60,7 +60,6 @@ KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
 		 -DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
 
 CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
-KBUILD_LDS_MODULE += $(srctree)/arch/parisc/kernel/module.lds
 endif
 
 OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index ac5f34993b53..1c50093e2ebe 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -43,6 +43,7 @@
 #include <linux/elf.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
+#include <linux/ftrace.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
@@ -862,7 +863,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const char *strtab = NULL;
 	const Elf_Shdr *s;
 	char *secstrings;
-	int err, symindex = -1;
+	int symindex = -1;
 	Elf_Sym *newptr, *oldptr;
 	Elf_Shdr *symhdr = NULL;
 #ifdef DEBUG
@@ -946,11 +947,13 @@ int module_finalize(const Elf_Ehdr *hdr,
 			/* patch .altinstructions */
 			apply_alternatives(aseg, aseg + s->sh_size, me->name);
 
+#ifdef CONFIG_DYNAMIC_FTRACE
 		/* For 32 bit kernels we're compiling modules with
 		 * -ffunction-sections so we must relocate the addresses in the
-		 *__mcount_loc section.
+		 *  ftrace callsite section.
 		 */
-		if (symindex != -1 && !strcmp(secname, "__mcount_loc")) {
+		if (symindex != -1 && !strcmp(secname, FTRACE_CALLSITE_SECTION)) {
+			int err;
 			if (s->sh_type == SHT_REL)
 				err = apply_relocate((Elf_Shdr *)sechdrs,
 							strtab, symindex,
@@ -962,6 +965,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 			if (err)
 				return err;
 		}
+#endif
 	}
 	return 0;
 }
diff --git a/arch/parisc/kernel/module.lds b/arch/parisc/kernel/module.lds
deleted file mode 100644
index 1a9a92aca5c8..000000000000
--- a/arch/parisc/kernel/module.lds
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-
-SECTIONS {
-	__mcount_loc : {
-		*(__patchable_function_entries)
-	}
-}
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8a8cb3c401b2..cea0746c8f4c 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -709,6 +709,11 @@ static inline unsigned long get_lock_parent_ip(void)
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 extern void ftrace_init(void);
+#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
+#define FTRACE_CALLSITE_SECTION	"__patchable_function_entries"
+#else
+#define FTRACE_CALLSITE_SECTION	"__mcount_loc"
+#endif
 #else
 static inline void ftrace_init(void) { }
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index ff2d7359a418..acf7962936c4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3222,7 +3222,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 #endif
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 	/* sechdrs[0].sh_size is always zero */
-	mod->ftrace_callsites = section_objs(info, "__mcount_loc",
+	mod->ftrace_callsites = section_objs(info, FTRACE_CALLSITE_SECTION,
 					     sizeof(*mod->ftrace_callsites),
 					     &mod->num_ftrace_callsites);
 #endif
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 2/8] module/ftrace: handle patchable-function-entry
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

When using patchable-function-entry, the compiler will record the
callsites into a section named "__patchable_function_entries" rather
than "__mcount_loc". Let's abstract this difference behind a new
FTRACE_CALLSITE_SECTION, so that architectures don't have to handle this
explicitly (e.g. with custom module linker scripts).

As parisc currently handles this explicitly, it is fixed up accordingly,
with its custom linker script removed. Since FTRACE_CALLSITE_SECTION is
only defined when DYNAMIC_FTRACE is selected, the parisc module loading
code is updated to only use the definition in that case. When
DYNAMIC_FTRACE is not selected, modules shouldn't have this section, so
this removes some redundant work in that case.

I built parisc generic-{32,64}bit_defconfig with DYNAMIC_FTRACE enabled,
and verified that the section made it into the .ko files for modules.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@stackframe.org>
---
 arch/parisc/Makefile          |  1 -
 arch/parisc/kernel/module.c   | 10 +++++++---
 arch/parisc/kernel/module.lds |  7 -------
 include/linux/ftrace.h        |  5 +++++
 kernel/module.c               |  2 +-
 5 files changed, 13 insertions(+), 12 deletions(-)
 delete mode 100644 arch/parisc/kernel/module.lds

diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
index 36b834f1c933..dca8f2de8cf5 100644
--- a/arch/parisc/Makefile
+++ b/arch/parisc/Makefile
@@ -60,7 +60,6 @@ KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
 		 -DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
 
 CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
-KBUILD_LDS_MODULE += $(srctree)/arch/parisc/kernel/module.lds
 endif
 
 OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index ac5f34993b53..1c50093e2ebe 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -43,6 +43,7 @@
 #include <linux/elf.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
+#include <linux/ftrace.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
@@ -862,7 +863,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const char *strtab = NULL;
 	const Elf_Shdr *s;
 	char *secstrings;
-	int err, symindex = -1;
+	int symindex = -1;
 	Elf_Sym *newptr, *oldptr;
 	Elf_Shdr *symhdr = NULL;
 #ifdef DEBUG
@@ -946,11 +947,13 @@ int module_finalize(const Elf_Ehdr *hdr,
 			/* patch .altinstructions */
 			apply_alternatives(aseg, aseg + s->sh_size, me->name);
 
+#ifdef CONFIG_DYNAMIC_FTRACE
 		/* For 32 bit kernels we're compiling modules with
 		 * -ffunction-sections so we must relocate the addresses in the
-		 *__mcount_loc section.
+		 *  ftrace callsite section.
 		 */
-		if (symindex != -1 && !strcmp(secname, "__mcount_loc")) {
+		if (symindex != -1 && !strcmp(secname, FTRACE_CALLSITE_SECTION)) {
+			int err;
 			if (s->sh_type == SHT_REL)
 				err = apply_relocate((Elf_Shdr *)sechdrs,
 							strtab, symindex,
@@ -962,6 +965,7 @@ int module_finalize(const Elf_Ehdr *hdr,
 			if (err)
 				return err;
 		}
+#endif
 	}
 	return 0;
 }
diff --git a/arch/parisc/kernel/module.lds b/arch/parisc/kernel/module.lds
deleted file mode 100644
index 1a9a92aca5c8..000000000000
--- a/arch/parisc/kernel/module.lds
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-
-SECTIONS {
-	__mcount_loc : {
-		*(__patchable_function_entries)
-	}
-}
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8a8cb3c401b2..cea0746c8f4c 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -709,6 +709,11 @@ static inline unsigned long get_lock_parent_ip(void)
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 extern void ftrace_init(void);
+#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
+#define FTRACE_CALLSITE_SECTION	"__patchable_function_entries"
+#else
+#define FTRACE_CALLSITE_SECTION	"__mcount_loc"
+#endif
 #else
 static inline void ftrace_init(void) { }
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index ff2d7359a418..acf7962936c4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3222,7 +3222,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 #endif
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 	/* sechdrs[0].sh_size is always zero */
-	mod->ftrace_callsites = section_objs(info, "__mcount_loc",
+	mod->ftrace_callsites = section_objs(info, FTRACE_CALLSITE_SECTION,
 					     sizeof(*mod->ftrace_callsites),
 					     &mod->num_ftrace_callsites);
 #endif
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 3/8] arm64: module: rework special section handling
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

When we load a module, we have to perform some special work for a couple
of named sections. To do this, we iterate over all of the module's
sections, and perform work for each section we recognize.

To make it easier to handle the unexpected absence of a section, and to
make the section-specific logic easer to read, let's factor the section
search into a helper. Similar is already done in the core module loader,
and other architectures (and ideally we'd unify these in future).

If we expect a module to have an ftrace trampoline section, but it
doesn't have one, we'll now reject loading the module. When
ARM64_MODULE_PLTS is selected, any correctly built module should have
one (and this is assumed by arm64's ftrace PLT code) and the absence of
such a section implies something has gone wrong at build time.

Subsequent patches will make use of the new helper.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Bieshsheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/module.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 03ff15bffbb6..763a86d52fef 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -470,22 +470,39 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	return -ENOEXEC;
 }
 
-int module_finalize(const Elf_Ehdr *hdr,
-		    const Elf_Shdr *sechdrs,
-		    struct module *me)
+static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
+				    const Elf_Shdr *sechdrs,
+				    const char *name)
 {
 	const Elf_Shdr *s, *se;
 	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
 	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
-		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0)
-			apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+		if (strcmp(name, secstrs + s->sh_name) == 0)
+			return s;
+	}
+
+	return NULL;
+}
+
+int module_finalize(const Elf_Ehdr *hdr,
+		    const Elf_Shdr *sechdrs,
+		    struct module *me)
+{
+	const Elf_Shdr *s;
+
+	s = find_section(hdr, sechdrs, ".altinstructions");
+	if (s)
+		apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+
 #ifdef CONFIG_ARM64_MODULE_PLTS
-		if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
-		    !strcmp(".text.ftrace_trampoline", secstrs + s->sh_name))
-			me->arch.ftrace_trampoline = (void *)s->sh_addr;
-#endif
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
+		s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
+		if (!s)
+			return -ENOEXEC;
+		me->arch.ftrace_trampoline = (void *)s->sh_addr;
 	}
+#endif
 
 	return 0;
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 3/8] arm64: module: rework special section handling
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

When we load a module, we have to perform some special work for a couple
of named sections. To do this, we iterate over all of the module's
sections, and perform work for each section we recognize.

To make it easier to handle the unexpected absence of a section, and to
make the section-specific logic easer to read, let's factor the section
search into a helper. Similar is already done in the core module loader,
and other architectures (and ideally we'd unify these in future).

If we expect a module to have an ftrace trampoline section, but it
doesn't have one, we'll now reject loading the module. When
ARM64_MODULE_PLTS is selected, any correctly built module should have
one (and this is assumed by arm64's ftrace PLT code) and the absence of
such a section implies something has gone wrong at build time.

Subsequent patches will make use of the new helper.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Bieshsheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/module.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 03ff15bffbb6..763a86d52fef 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -470,22 +470,39 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	return -ENOEXEC;
 }
 
-int module_finalize(const Elf_Ehdr *hdr,
-		    const Elf_Shdr *sechdrs,
-		    struct module *me)
+static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
+				    const Elf_Shdr *sechdrs,
+				    const char *name)
 {
 	const Elf_Shdr *s, *se;
 	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
 	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
-		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0)
-			apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+		if (strcmp(name, secstrs + s->sh_name) == 0)
+			return s;
+	}
+
+	return NULL;
+}
+
+int module_finalize(const Elf_Ehdr *hdr,
+		    const Elf_Shdr *sechdrs,
+		    struct module *me)
+{
+	const Elf_Shdr *s;
+
+	s = find_section(hdr, sechdrs, ".altinstructions");
+	if (s)
+		apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+
 #ifdef CONFIG_ARM64_MODULE_PLTS
-		if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
-		    !strcmp(".text.ftrace_trampoline", secstrs + s->sh_name))
-			me->arch.ftrace_trampoline = (void *)s->sh_addr;
-#endif
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
+		s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
+		if (!s)
+			return -ENOEXEC;
+		me->arch.ftrace_trampoline = (void *)s->sh_addr;
 	}
+#endif
 
 	return 0;
 }
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 4/8] arm64: module/ftrace: intialize PLT at load time
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

Currently we lazily-initialize a module's ftrace PLT at runtime when we
install the first ftrace call. To do so we have to apply a number of
sanity checks, transiently mark the module text as RW, and perform an
IPI as part of handling Neoverse-N1 erratum #1542419.

We only expect the ftrace trampoline to point at ftrace_caller() (AKA
FTRACE_ADDR), so let's simplify all of this by intializing the PLT at
module load time, before the module loader marks the module RO and
performs the intial I-cache maintenance for the module.

Thus we can rely on the module having been correctly intialized, and can
simplify the runtime work necessary to install an ftrace call in a
module. This will also allow for the removal of module_disable_ro().

Tested by forcing ftrace_make_call() to use the module PLT, and then
loading up a module after setting up ftrace with:

| echo ":mod:<module-name>" > set_ftrace_filter;
| echo function > current_tracer;
| modprobe <module-name>

Since FTRACE_ADDR is only defined when CONFIG_DYNAMIC_FTRACE is
selected, we wrap its use along with most of module_init_ftrace_plt()
with ifdeffery rather than using IS_ENABLED().

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ftrace.c | 55 ++++++++++++----------------------------------
 arch/arm64/kernel/module.c | 32 +++++++++++++++++----------
 2 files changed, 35 insertions(+), 52 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 06e56b470315..822718eafdb4 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -73,10 +73,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 	if (offset < -SZ_128M || offset >= SZ_128M) {
 #ifdef CONFIG_ARM64_MODULE_PLTS
-		struct plt_entry trampoline, *dst;
 		struct module *mod;
 
 		/*
+		 * There is only one ftrace trampoline per module. For now,
+		 * this is not a problem since on arm64, all dynamic ftrace
+		 * invocations are routed via ftrace_caller(). This will need
+		 * to be revisited if support for multiple ftrace entry points
+		 * is added in the future, but for now, the pr_err() below
+		 * deals with a theoretical issue only.
+		 */
+		if (addr != FTRACE_ADDR) {
+			pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
+			return -EINVAL;
+		}
+
+		/*
 		 * On kernels that support module PLTs, the offset between the
 		 * branch instruction and its target may legally exceed the
 		 * range of an ordinary relative 'bl' opcode. In this case, we
@@ -93,46 +105,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		if (WARN_ON(!mod))
 			return -EINVAL;
 
-		/*
-		 * There is only one ftrace trampoline per module. For now,
-		 * this is not a problem since on arm64, all dynamic ftrace
-		 * invocations are routed via ftrace_caller(). This will need
-		 * to be revisited if support for multiple ftrace entry points
-		 * is added in the future, but for now, the pr_err() below
-		 * deals with a theoretical issue only.
-		 *
-		 * Note that PLTs are place relative, and plt_entries_equal()
-		 * checks whether they point to the same target. Here, we need
-		 * to check if the actual opcodes are in fact identical,
-		 * regardless of the offset in memory so use memcmp() instead.
-		 */
-		dst = mod->arch.ftrace_trampoline;
-		trampoline = get_plt_entry(addr, dst);
-		if (memcmp(dst, &trampoline, sizeof(trampoline))) {
-			if (plt_entry_is_initialized(dst)) {
-				pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
-				return -EINVAL;
-			}
-
-			/* point the trampoline to our ftrace entry point */
-			module_disable_ro(mod);
-			*dst = trampoline;
-			module_enable_ro(mod, true);
-
-			/*
-			 * Ensure updated trampoline is visible to instruction
-			 * fetch before we patch in the branch. Although the
-			 * architecture doesn't require an IPI in this case,
-			 * Neoverse-N1 erratum #1542419 does require one
-			 * if the TLB maintenance in module_enable_ro() is
-			 * skipped due to rodata_enabled. It doesn't seem worth
-			 * it to make it conditional given that this is
-			 * certainly not a fast-path.
-			 */
-			flush_icache_range((unsigned long)&dst[0],
-					   (unsigned long)&dst[1]);
-		}
-		addr = (unsigned long)dst;
+		addr = (unsigned long)mod->arch.ftrace_trampoline;
 #else /* CONFIG_ARM64_MODULE_PLTS */
 		return -EINVAL;
 #endif /* CONFIG_ARM64_MODULE_PLTS */
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 763a86d52fef..5f5bc3b94da7 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -9,6 +9,7 @@
 
 #include <linux/bitops.h>
 #include <linux/elf.h>
+#include <linux/ftrace.h>
 #include <linux/gfp.h>
 #include <linux/kasan.h>
 #include <linux/kernel.h>
@@ -485,24 +486,33 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
 	return NULL;
 }
 
+int module_init_ftrace_plt(const Elf_Ehdr *hdr,
+			   const Elf_Shdr *sechdrs,
+			   struct module *mod)
+{
+#if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
+	const Elf_Shdr *s;
+	struct plt_entry *plt;
+
+	s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
+	if (!s)
+		return -ENOEXEC;
+
+	plt = (void *)s->sh_addr;
+	*plt = get_plt_entry(FTRACE_ADDR, plt);
+	mod->arch.ftrace_trampoline = plt;
+#endif
+	return 0;
+}
+
 int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
 	const Elf_Shdr *s;
-
 	s = find_section(hdr, sechdrs, ".altinstructions");
 	if (s)
 		apply_alternatives_module((void *)s->sh_addr, s->sh_size);
 
-#ifdef CONFIG_ARM64_MODULE_PLTS
-	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
-		s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
-		if (!s)
-			return -ENOEXEC;
-		me->arch.ftrace_trampoline = (void *)s->sh_addr;
-	}
-#endif
-
-	return 0;
+	return module_init_ftrace_plt(hdr, sechdrs, me);
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 4/8] arm64: module/ftrace: intialize PLT at load time
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

Currently we lazily-initialize a module's ftrace PLT at runtime when we
install the first ftrace call. To do so we have to apply a number of
sanity checks, transiently mark the module text as RW, and perform an
IPI as part of handling Neoverse-N1 erratum #1542419.

We only expect the ftrace trampoline to point at ftrace_caller() (AKA
FTRACE_ADDR), so let's simplify all of this by intializing the PLT at
module load time, before the module loader marks the module RO and
performs the intial I-cache maintenance for the module.

Thus we can rely on the module having been correctly intialized, and can
simplify the runtime work necessary to install an ftrace call in a
module. This will also allow for the removal of module_disable_ro().

Tested by forcing ftrace_make_call() to use the module PLT, and then
loading up a module after setting up ftrace with:

| echo ":mod:<module-name>" > set_ftrace_filter;
| echo function > current_tracer;
| modprobe <module-name>

Since FTRACE_ADDR is only defined when CONFIG_DYNAMIC_FTRACE is
selected, we wrap its use along with most of module_init_ftrace_plt()
with ifdeffery rather than using IS_ENABLED().

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ftrace.c | 55 ++++++++++++----------------------------------
 arch/arm64/kernel/module.c | 32 +++++++++++++++++----------
 2 files changed, 35 insertions(+), 52 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 06e56b470315..822718eafdb4 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -73,10 +73,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 
 	if (offset < -SZ_128M || offset >= SZ_128M) {
 #ifdef CONFIG_ARM64_MODULE_PLTS
-		struct plt_entry trampoline, *dst;
 		struct module *mod;
 
 		/*
+		 * There is only one ftrace trampoline per module. For now,
+		 * this is not a problem since on arm64, all dynamic ftrace
+		 * invocations are routed via ftrace_caller(). This will need
+		 * to be revisited if support for multiple ftrace entry points
+		 * is added in the future, but for now, the pr_err() below
+		 * deals with a theoretical issue only.
+		 */
+		if (addr != FTRACE_ADDR) {
+			pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
+			return -EINVAL;
+		}
+
+		/*
 		 * On kernels that support module PLTs, the offset between the
 		 * branch instruction and its target may legally exceed the
 		 * range of an ordinary relative 'bl' opcode. In this case, we
@@ -93,46 +105,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		if (WARN_ON(!mod))
 			return -EINVAL;
 
-		/*
-		 * There is only one ftrace trampoline per module. For now,
-		 * this is not a problem since on arm64, all dynamic ftrace
-		 * invocations are routed via ftrace_caller(). This will need
-		 * to be revisited if support for multiple ftrace entry points
-		 * is added in the future, but for now, the pr_err() below
-		 * deals with a theoretical issue only.
-		 *
-		 * Note that PLTs are place relative, and plt_entries_equal()
-		 * checks whether they point to the same target. Here, we need
-		 * to check if the actual opcodes are in fact identical,
-		 * regardless of the offset in memory so use memcmp() instead.
-		 */
-		dst = mod->arch.ftrace_trampoline;
-		trampoline = get_plt_entry(addr, dst);
-		if (memcmp(dst, &trampoline, sizeof(trampoline))) {
-			if (plt_entry_is_initialized(dst)) {
-				pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
-				return -EINVAL;
-			}
-
-			/* point the trampoline to our ftrace entry point */
-			module_disable_ro(mod);
-			*dst = trampoline;
-			module_enable_ro(mod, true);
-
-			/*
-			 * Ensure updated trampoline is visible to instruction
-			 * fetch before we patch in the branch. Although the
-			 * architecture doesn't require an IPI in this case,
-			 * Neoverse-N1 erratum #1542419 does require one
-			 * if the TLB maintenance in module_enable_ro() is
-			 * skipped due to rodata_enabled. It doesn't seem worth
-			 * it to make it conditional given that this is
-			 * certainly not a fast-path.
-			 */
-			flush_icache_range((unsigned long)&dst[0],
-					   (unsigned long)&dst[1]);
-		}
-		addr = (unsigned long)dst;
+		addr = (unsigned long)mod->arch.ftrace_trampoline;
 #else /* CONFIG_ARM64_MODULE_PLTS */
 		return -EINVAL;
 #endif /* CONFIG_ARM64_MODULE_PLTS */
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 763a86d52fef..5f5bc3b94da7 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -9,6 +9,7 @@
 
 #include <linux/bitops.h>
 #include <linux/elf.h>
+#include <linux/ftrace.h>
 #include <linux/gfp.h>
 #include <linux/kasan.h>
 #include <linux/kernel.h>
@@ -485,24 +486,33 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
 	return NULL;
 }
 
+int module_init_ftrace_plt(const Elf_Ehdr *hdr,
+			   const Elf_Shdr *sechdrs,
+			   struct module *mod)
+{
+#if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
+	const Elf_Shdr *s;
+	struct plt_entry *plt;
+
+	s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
+	if (!s)
+		return -ENOEXEC;
+
+	plt = (void *)s->sh_addr;
+	*plt = get_plt_entry(FTRACE_ADDR, plt);
+	mod->arch.ftrace_trampoline = plt;
+#endif
+	return 0;
+}
+
 int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
 	const Elf_Shdr *s;
-
 	s = find_section(hdr, sechdrs, ".altinstructions");
 	if (s)
 		apply_alternatives_module((void *)s->sh_addr, s->sh_size);
 
-#ifdef CONFIG_ARM64_MODULE_PLTS
-	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE)) {
-		s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
-		if (!s)
-			return -ENOEXEC;
-		me->arch.ftrace_trampoline = (void *)s->sh_addr;
-	}
-#endif
-
-	return 0;
+	return module_init_ftrace_plt(hdr, sechdrs, me);
 }
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 5/8] arm64: insn: add encoder for MOV (register)
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

For FTRACE_WITH_REGS, we're going to want to generate a MOV (register)
instruction as part of the callsite intialization. As MOV (register) is
an alias for ORR (shifted register), we can generate this with
aarch64_insn_gen_logical_shifted_reg(), but it's somewhat verbose and
difficult to read in-context.

Add a aarch64_insn_gen_move_reg() wrapper for this case so that we can
write callers in a more straightforward way.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/insn.h |  3 +++
 arch/arm64/kernel/insn.c      | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 39e7780bedd6..bb313dde58a4 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -440,6 +440,9 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 					 int shift,
 					 enum aarch64_insn_variant variant,
 					 enum aarch64_insn_logic_type type);
+u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
+			      enum aarch64_insn_register src,
+			      enum aarch64_insn_variant variant);
 u32 aarch64_insn_gen_logical_immediate(enum aarch64_insn_logic_type type,
 				       enum aarch64_insn_variant variant,
 				       enum aarch64_insn_register Rn,
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index d801a7094076..513b29c3e735 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -1268,6 +1268,19 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_6, insn, shift);
 }
 
+/*
+ * MOV (register) is architecturally an alias of ORR (shifted register) where
+ * MOV <*d>, <*m> is equivalent to ORR <*d>, <*ZR>, <*m>
+ */
+u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
+			      enum aarch64_insn_register src,
+			      enum aarch64_insn_variant variant)
+{
+	return aarch64_insn_gen_logical_shifted_reg(dst, AARCH64_INSN_REG_ZR,
+						    src, 0, variant,
+						    AARCH64_INSN_LOGIC_ORR);
+}
+
 u32 aarch64_insn_gen_adr(unsigned long pc, unsigned long addr,
 			 enum aarch64_insn_register reg,
 			 enum aarch64_insn_adr_type type)
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 5/8] arm64: insn: add encoder for MOV (register)
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

For FTRACE_WITH_REGS, we're going to want to generate a MOV (register)
instruction as part of the callsite intialization. As MOV (register) is
an alias for ORR (shifted register), we can generate this with
aarch64_insn_gen_logical_shifted_reg(), but it's somewhat verbose and
difficult to read in-context.

Add a aarch64_insn_gen_move_reg() wrapper for this case so that we can
write callers in a more straightforward way.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/insn.h |  3 +++
 arch/arm64/kernel/insn.c      | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 39e7780bedd6..bb313dde58a4 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -440,6 +440,9 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 					 int shift,
 					 enum aarch64_insn_variant variant,
 					 enum aarch64_insn_logic_type type);
+u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
+			      enum aarch64_insn_register src,
+			      enum aarch64_insn_variant variant);
 u32 aarch64_insn_gen_logical_immediate(enum aarch64_insn_logic_type type,
 				       enum aarch64_insn_variant variant,
 				       enum aarch64_insn_register Rn,
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index d801a7094076..513b29c3e735 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -1268,6 +1268,19 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_6, insn, shift);
 }
 
+/*
+ * MOV (register) is architecturally an alias of ORR (shifted register) where
+ * MOV <*d>, <*m> is equivalent to ORR <*d>, <*ZR>, <*m>
+ */
+u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
+			      enum aarch64_insn_register src,
+			      enum aarch64_insn_variant variant)
+{
+	return aarch64_insn_gen_logical_shifted_reg(dst, AARCH64_INSN_REG_ZR,
+						    src, 0, variant,
+						    AARCH64_INSN_LOGIC_ORR);
+}
+
 u32 aarch64_insn_gen_adr(unsigned long pc, unsigned long addr,
 			 enum aarch64_insn_register reg,
 			 enum aarch64_insn_adr_type type)
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 6/8] arm64: asm-offsets: add S_FP
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

So that assembly code can more easily manipulate the FP (x29) within a
pt_regs, add an S_FP asm-offsets definition.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/asm-offsets.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 214685760e1c..a5bdce8af65b 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -56,6 +56,7 @@ int main(void)
   DEFINE(S_X24,			offsetof(struct pt_regs, regs[24]));
   DEFINE(S_X26,			offsetof(struct pt_regs, regs[26]));
   DEFINE(S_X28,			offsetof(struct pt_regs, regs[28]));
+  DEFINE(S_FP,			offsetof(struct pt_regs, regs[29]));
   DEFINE(S_LR,			offsetof(struct pt_regs, regs[30]));
   DEFINE(S_SP,			offsetof(struct pt_regs, sp));
   DEFINE(S_PSTATE,		offsetof(struct pt_regs, pstate));
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 6/8] arm64: asm-offsets: add S_FP
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

So that assembly code can more easily manipulate the FP (x29) within a
pt_regs, add an S_FP asm-offsets definition.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/asm-offsets.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 214685760e1c..a5bdce8af65b 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -56,6 +56,7 @@ int main(void)
   DEFINE(S_X24,			offsetof(struct pt_regs, regs[24]));
   DEFINE(S_X26,			offsetof(struct pt_regs, regs[26]));
   DEFINE(S_X28,			offsetof(struct pt_regs, regs[28]));
+  DEFINE(S_FP,			offsetof(struct pt_regs, regs[29]));
   DEFINE(S_LR,			offsetof(struct pt_regs, regs[30]));
   DEFINE(S_SP,			offsetof(struct pt_regs, sp));
   DEFINE(S_PSTATE,		offsetof(struct pt_regs, pstate));
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 7/8] arm64: implement ftrace with regs
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will, Torsten Duwe

From: Torsten Duwe <duwe@lst.de>

This patch implements FTRACE_WITH_REGS for arm64, which allows a traced
function's arguments (and some other registers) to be captured into a
struct pt_regs, allowing these to be inspected and/or modified. This is
a building block for live-patching, where a function's arguments may be
forwarded to another function. This is also necessary to enable ftrace
and in-kernel pointer authentication at the same time, as it allows the
LR value to be captured and adjusted prior to signing.

Using GCC's -fpatchable-function-entry=N option, we can have the
compiler insert a configurable number of NOPs between the function entry
point and the usual prologue. This also ensures functions are AAPCS
compliant (e.g. disabling inter-procedural register allocation).

For example, with -fpatchable-function-entry=2, GCC 8.1.0 compiles the
following:

| unsigned long bar(void);
|
| unsigned long foo(void)
| {
|         return bar() + 1;
| }

... to:

| <foo>:
|         nop
|         nop
|         stp     x29, x30, [sp, #-16]!
|         mov     x29, sp
|         bl      0 <bar>
|         add     x0, x0, #0x1
|         ldp     x29, x30, [sp], #16
|         ret

This patch builds the kernel with -fpatchable-function-entry=2,
prefixing each function with two NOPs. To trace a function, we replace
these NOPs with a sequence that saves the LR into a GPR, then calls an
ftrace entry assembly function which saves this and other relevant
registers:

| mov	x9, x30
| bl	<ftrace-entry>

Since patchable functions are AAPCS compliant (and the kernel does not
use x18 as a platform register), x9-x18 can be safely clobbered in the
patched sequence and the ftrace entry code.

There are now two ftrace entry functions, ftrace_regs_entry (which saves
all GPRs), and ftrace_entry (which saves the bare minimum). A PLT is
allocated for each within modules.

Signed-off-by: Torsten Duwe <duwe@suse.de>
[Mark: rework asm, comments, PLTs, initialization, commit message]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Julien Thierry <jthierry@redhat.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/Kconfig               |   2 +
 arch/arm64/Makefile              |   5 ++
 arch/arm64/include/asm/ftrace.h  |  23 +++++++
 arch/arm64/include/asm/module.h  |   2 +-
 arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/ftrace.c       |  84 +++++++++++++++++++----
 arch/arm64/kernel/module-plts.c  |   3 +-
 arch/arm64/kernel/module.c       |  18 +++--
 8 files changed, 252 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 950a56b71ff0..0ffb8596b8a1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -143,6 +143,8 @@ config ARM64
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
+		if $(cc-option,-fpatchable-function-entry=2)
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_FAST_GUP
 	select HAVE_FTRACE_MCOUNT_RECORD
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 2c0238ce0551..1fbe24d4fdb6 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -95,6 +95,11 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
 KBUILD_LDS_MODULE	+= $(srctree)/arch/arm64/kernel/module.lds
 endif
 
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
+  KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
+  CC_FLAGS_FTRACE := -fpatchable-function-entry=2
+endif
+
 # Default value
 head-y		:= arch/arm64/kernel/head.o
 
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index d48667b04c41..91fa4baa1a93 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -11,9 +11,20 @@
 #include <asm/insn.h>
 
 #define HAVE_FUNCTION_GRAPH_FP_TEST
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#else
 #define MCOUNT_ADDR		((unsigned long)_mcount)
+#endif
+
+/* The BL at the callsite's adjusted rec->ip */
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
+#define FTRACE_PLT_IDX		0
+#define FTRACE_REGS_PLT_IDX	1
+#define NR_FTRACE_PLTS		2
+
 /*
  * Currently, gcc tends to save the link register after the local variables
  * on the stack. This causes the max stack tracer to report the function
@@ -44,12 +55,24 @@ extern void return_to_handler(void);
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*
+	 * Adjust addr to point at the BL in the callsite.
+	 * See ftrace_init_nop() for the callsite sequence.
+	 */
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+		return addr + AARCH64_INSN_SIZE;
+	/*
 	 * addr is the address of the mcount call instruction.
 	 * recordmcount does the necessary offset calculation.
 	 */
 	return addr;
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+struct dyn_ftrace;
+int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
+#define ftrace_init_nop ftrace_init_nop
+#endif
+
 #define ftrace_return_address(n) return_address(n)
 
 /*
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index f80e13cbf8ec..1e93de68c044 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -21,7 +21,7 @@ struct mod_arch_specific {
 	struct mod_plt_sec	init;
 
 	/* for CONFIG_DYNAMIC_FTRACE */
-	struct plt_entry 	*ftrace_trampoline;
+	struct plt_entry	*ftrace_trampolines;
 };
 #endif
 
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 33d003d80121..94720388957f 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -7,10 +7,137 @@
  */
 
 #include <linux/linkage.h>
+#include <asm/asm-offsets.h>
 #include <asm/assembler.h>
 #include <asm/ftrace.h>
 #include <asm/insn.h>
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+/*
+ * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs before
+ * the regular function prologue. For an enabled callsite, ftrace_init_nop() and
+ * ftrace_make_call() have patched those NOPs to:
+ *
+ * 	MOV	X9, LR
+ * 	BL	<entry>
+ *
+ * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
+ *
+ * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
+ * live, and x9-x18 are safe to clobber.
+ *
+ * We save the callsite's context into a pt_regs before invoking and ftrace
+ * callbacks. So that we can get a sensible backtrace, we create a stack record
+ * for the callsite and the ftrace entry assembly. This is not sufficient for
+ * reliable stacktrace: until we create the callsite stack record, its caller
+ * is missing from the LR and existing chain of frame records.
+ */
+	.macro  ftrace_regs_entry, allregs=0
+	/* Make room for pt_regs, plus a callee frame */
+	sub	sp, sp, #(S_FRAME_SIZE + 16)
+
+	/* Save function arguments (and x9 for simplicity) */
+	stp	x0, x1, [sp, #S_X0]
+	stp	x2, x3, [sp, #S_X2]
+	stp	x4, x5, [sp, #S_X4]
+	stp	x6, x7, [sp, #S_X6]
+	stp	x8, x9, [sp, #S_X8]
+
+	/* Optionally save the callee-saved registers, always save the FP */
+	.if \allregs == 1
+	stp	x10, x11, [sp, #S_X10]
+	stp	x12, x13, [sp, #S_X12]
+	stp	x14, x15, [sp, #S_X14]
+	stp	x16, x17, [sp, #S_X16]
+	stp	x18, x19, [sp, #S_X18]
+	stp	x20, x21, [sp, #S_X20]
+	stp	x22, x23, [sp, #S_X22]
+	stp	x24, x25, [sp, #S_X24]
+	stp	x26, x27, [sp, #S_X26]
+	stp	x28, x29, [sp, #S_X28]
+	.else
+	str	x29, [sp, #S_FP]
+	.endif
+
+	/* Save the callsite's SP and LR */
+	add	x10, sp, #(S_FRAME_SIZE + 16)
+	stp	x9, x10, [sp, #S_LR]
+
+	/* Save the PC after the ftrace callsite */
+	str	x30, [sp, #S_PC]
+
+	/* Create a frame record for the callsite above pt_regs */
+	stp	x29, x9, [sp, #S_FRAME_SIZE]
+	add	x29, sp, #S_FRAME_SIZE
+
+	/* Create our frame record within pt_regs. */
+	stp	x29, x30, [sp, #S_STACKFRAME]
+	add	x29, sp, #S_STACKFRAME
+	.endm
+
+ENTRY(ftrace_regs_caller)
+	ftrace_regs_entry	1
+	b	ftrace_common
+ENDPROC(ftrace_regs_caller)
+
+ENTRY(ftrace_caller)
+	ftrace_regs_entry	0
+	b	ftrace_common
+ENDPROC(ftrace_caller)
+
+ENTRY(ftrace_common)
+	sub	x0, x30, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
+	mov	x1, x9				// parent_ip (callsite's LR)
+	ldr_l	x2, function_trace_op		// op
+	mov	x3, sp				// regs
+
+GLOBAL(ftrace_call)
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+GLOBAL(ftrace_graph_call)		// ftrace_graph_caller();
+	nop				// If enabled, this will be replaced
+					// "b ftrace_graph_caller"
+#endif
+
+/*
+ * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
+ * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
+ * to restore x0-x8, x29, and x30.
+ */
+ftrace_common_return:
+	/* Restore function arguments */
+	ldp	x0, x1, [sp]
+	ldp	x2, x3, [sp, #S_X2]
+	ldp	x4, x5, [sp, #S_X4]
+	ldp	x6, x7, [sp, #S_X6]
+	ldr	x8, [sp, #S_X8]
+
+	/* Restore the callsite's FP, LR, PC */
+	ldr	x29, [sp, #S_FP]
+	ldr	x30, [sp, #S_LR]
+	ldr	x9, [sp, #S_PC]
+
+	/* Restore the callsite's SP */
+	add	sp, sp, #S_FRAME_SIZE + 16
+
+	ret	x9
+ENDPROC(ftrace_common)
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(ftrace_graph_caller)
+	ldr	x0, [sp, #S_PC]
+	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
+	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
+	ldr	x2, [sp, #S_FRAME_SIZE]	   	// parent fp (callsite's FP)
+	bl	prepare_ftrace_return
+	b	ftrace_common_return
+ENDPROC(ftrace_graph_caller)
+#else
+#endif
+
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
 /*
  * Gcc with -pg will put the following code in the beginning of each function:
  *      mov x0, x30
@@ -160,11 +287,6 @@ GLOBAL(ftrace_graph_call)		// ftrace_graph_caller();
 
 	mcount_exit
 ENDPROC(ftrace_caller)
-#endif /* CONFIG_DYNAMIC_FTRACE */
-
-ENTRY(ftrace_stub)
-	ret
-ENDPROC(ftrace_stub)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
@@ -184,7 +306,15 @@ ENTRY(ftrace_graph_caller)
 
 	mcount_exit
 ENDPROC(ftrace_graph_caller)
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+#endif /* CONFIG_DYNAMIC_FTRACE */
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
+ENTRY(ftrace_stub)
+	ret
+ENDPROC(ftrace_stub)
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
  * void return_to_handler(void)
  *
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 822718eafdb4..aea652c33a38 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -62,6 +62,19 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(pc, 0, new, false);
 }
 
+#ifdef CONFIG_ARM64_MODULE_PLTS
+static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
+{
+	struct plt_entry *plt = mod->arch.ftrace_trampolines;
+
+	if (addr == FTRACE_ADDR)
+		return &plt[FTRACE_PLT_IDX];
+	if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
+		return &plt[FTRACE_REGS_PLT_IDX];
+	return NULL;
+}
+#endif
+
 /*
  * Turn on the call to ftrace_caller() in instrumented function
  */
@@ -74,19 +87,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	if (offset < -SZ_128M || offset >= SZ_128M) {
 #ifdef CONFIG_ARM64_MODULE_PLTS
 		struct module *mod;
-
-		/*
-		 * There is only one ftrace trampoline per module. For now,
-		 * this is not a problem since on arm64, all dynamic ftrace
-		 * invocations are routed via ftrace_caller(). This will need
-		 * to be revisited if support for multiple ftrace entry points
-		 * is added in the future, but for now, the pr_err() below
-		 * deals with a theoretical issue only.
-		 */
-		if (addr != FTRACE_ADDR) {
-			pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
-			return -EINVAL;
-		}
+		struct plt_entry *plt;
 
 		/*
 		 * On kernels that support module PLTs, the offset between the
@@ -105,7 +106,13 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		if (WARN_ON(!mod))
 			return -EINVAL;
 
-		addr = (unsigned long)mod->arch.ftrace_trampoline;
+		plt = get_ftrace_plt(mod, addr);
+		if (!plt) {
+			pr_err("ftrace: no module PLT for %ps\n", (void *)addr);
+			return -EINVAL;
+		}
+
+		addr = (unsigned long)plt;
 #else /* CONFIG_ARM64_MODULE_PLTS */
 		return -EINVAL;
 #endif /* CONFIG_ARM64_MODULE_PLTS */
@@ -117,6 +124,55 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	return ftrace_modify_code(pc, old, new, true);
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+			unsigned long addr)
+{
+	unsigned long pc = rec->ip;
+	u32 old, new;
+
+	old = aarch64_insn_gen_branch_imm(pc, old_addr,
+					  AARCH64_INSN_BRANCH_LINK);
+	new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
+
+	return ftrace_modify_code(pc, old, new, true);
+}
+
+/*
+ * The compiler has inserted two NOPs before the regular function prologue.
+ * All instrumented functions follow the AAPCS, so x0-x8 and x19-x30 are live,
+ * and x9-x18 are free for our use.
+ *
+ * At runtime we want to be able to swing a single NOP <-> BL to enable or
+ * disable the ftrace call. The BL requires us to save the original LR value,
+ * so here we insert a <MOV X9, LR> over the first NOP so the instructions
+ * before the regular prologue are:
+ *
+ * | Compiled | Disabled   | Enabled    |
+ * +----------+------------+------------+
+ * | NOP      | MOV X9, LR | MOV X9, LR |
+ * | NOP      | NOP        | BL <entry> |
+ *
+ * The LR value will be recovered by ftrace_regs_entry, and restored into LR
+ * before returning to the regular function prologue. When a function is not
+ * being traced, the MOV is not harmful given x9 is not live per the AAPCS.
+ *
+ * Note: ftrace_process_locs() has pre-adjusted rec->ip to be the address of
+ * the BL.
+ */
+int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+	unsigned long pc = rec->ip - AARCH64_INSN_SIZE;
+	u32 old, new;
+
+	old = aarch64_insn_gen_nop();
+	new = aarch64_insn_gen_move_reg(AARCH64_INSN_REG_9,
+					AARCH64_INSN_REG_LR,
+					AARCH64_INSN_VARIANT_64BIT);
+	return ftrace_modify_code(pc, old, new, true);
+}
+#endif
+
 /*
  * Turn off the call to ftrace_caller() in instrumented function
  */
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index b182442b87a3..65b08a74aec6 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/elf.h>
+#include <linux/ftrace.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sort.h>
@@ -330,7 +331,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 		tramp->sh_type = SHT_NOBITS;
 		tramp->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
 		tramp->sh_addralign = __alignof__(struct plt_entry);
-		tramp->sh_size = sizeof(struct plt_entry);
+		tramp->sh_size = NR_FTRACE_PLTS * sizeof(struct plt_entry);
 	}
 
 	return 0;
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 5f5bc3b94da7..00d21a420c60 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -486,21 +486,31 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
 	return NULL;
 }
 
+static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
+{
+	*plt = get_plt_entry(addr, plt);
+}
+
 int module_init_ftrace_plt(const Elf_Ehdr *hdr,
 			   const Elf_Shdr *sechdrs,
 			   struct module *mod)
 {
 #if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
 	const Elf_Shdr *s;
-	struct plt_entry *plt;
+	struct plt_entry *plts;
 
 	s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
 	if (!s)
 		return -ENOEXEC;
 
-	plt = (void *)s->sh_addr;
-	*plt = get_plt_entry(FTRACE_ADDR, plt);
-	mod->arch.ftrace_trampoline = plt;
+	plts = (void *)s->sh_addr;
+
+	__init_plt(&plts[FTRACE_PLT_IDX], FTRACE_ADDR);
+
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+		__init_plt(&plts[FTRACE_REGS_PLT_IDX], FTRACE_REGS_ADDR);
+
+	mod->arch.ftrace_trampolines = plts;
 #endif
 	return 0;
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 7/8] arm64: implement ftrace with regs
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, Torsten Duwe, svens,
	duwe

From: Torsten Duwe <duwe@lst.de>

This patch implements FTRACE_WITH_REGS for arm64, which allows a traced
function's arguments (and some other registers) to be captured into a
struct pt_regs, allowing these to be inspected and/or modified. This is
a building block for live-patching, where a function's arguments may be
forwarded to another function. This is also necessary to enable ftrace
and in-kernel pointer authentication at the same time, as it allows the
LR value to be captured and adjusted prior to signing.

Using GCC's -fpatchable-function-entry=N option, we can have the
compiler insert a configurable number of NOPs between the function entry
point and the usual prologue. This also ensures functions are AAPCS
compliant (e.g. disabling inter-procedural register allocation).

For example, with -fpatchable-function-entry=2, GCC 8.1.0 compiles the
following:

| unsigned long bar(void);
|
| unsigned long foo(void)
| {
|         return bar() + 1;
| }

... to:

| <foo>:
|         nop
|         nop
|         stp     x29, x30, [sp, #-16]!
|         mov     x29, sp
|         bl      0 <bar>
|         add     x0, x0, #0x1
|         ldp     x29, x30, [sp], #16
|         ret

This patch builds the kernel with -fpatchable-function-entry=2,
prefixing each function with two NOPs. To trace a function, we replace
these NOPs with a sequence that saves the LR into a GPR, then calls an
ftrace entry assembly function which saves this and other relevant
registers:

| mov	x9, x30
| bl	<ftrace-entry>

Since patchable functions are AAPCS compliant (and the kernel does not
use x18 as a platform register), x9-x18 can be safely clobbered in the
patched sequence and the ftrace entry code.

There are now two ftrace entry functions, ftrace_regs_entry (which saves
all GPRs), and ftrace_entry (which saves the bare minimum). A PLT is
allocated for each within modules.

Signed-off-by: Torsten Duwe <duwe@suse.de>
[Mark: rework asm, comments, PLTs, initialization, commit message]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Julien Thierry <jthierry@redhat.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/Kconfig               |   2 +
 arch/arm64/Makefile              |   5 ++
 arch/arm64/include/asm/ftrace.h  |  23 +++++++
 arch/arm64/include/asm/module.h  |   2 +-
 arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/ftrace.c       |  84 +++++++++++++++++++----
 arch/arm64/kernel/module-plts.c  |   3 +-
 arch/arm64/kernel/module.c       |  18 +++--
 8 files changed, 252 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 950a56b71ff0..0ffb8596b8a1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -143,6 +143,8 @@ config ARM64
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
+		if $(cc-option,-fpatchable-function-entry=2)
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_FAST_GUP
 	select HAVE_FTRACE_MCOUNT_RECORD
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 2c0238ce0551..1fbe24d4fdb6 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -95,6 +95,11 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
 KBUILD_LDS_MODULE	+= $(srctree)/arch/arm64/kernel/module.lds
 endif
 
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS),y)
+  KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
+  CC_FLAGS_FTRACE := -fpatchable-function-entry=2
+endif
+
 # Default value
 head-y		:= arch/arm64/kernel/head.o
 
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index d48667b04c41..91fa4baa1a93 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -11,9 +11,20 @@
 #include <asm/insn.h>
 
 #define HAVE_FUNCTION_GRAPH_FP_TEST
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#else
 #define MCOUNT_ADDR		((unsigned long)_mcount)
+#endif
+
+/* The BL at the callsite's adjusted rec->ip */
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
+#define FTRACE_PLT_IDX		0
+#define FTRACE_REGS_PLT_IDX	1
+#define NR_FTRACE_PLTS		2
+
 /*
  * Currently, gcc tends to save the link register after the local variables
  * on the stack. This causes the max stack tracer to report the function
@@ -44,12 +55,24 @@ extern void return_to_handler(void);
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*
+	 * Adjust addr to point at the BL in the callsite.
+	 * See ftrace_init_nop() for the callsite sequence.
+	 */
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+		return addr + AARCH64_INSN_SIZE;
+	/*
 	 * addr is the address of the mcount call instruction.
 	 * recordmcount does the necessary offset calculation.
 	 */
 	return addr;
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+struct dyn_ftrace;
+int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
+#define ftrace_init_nop ftrace_init_nop
+#endif
+
 #define ftrace_return_address(n) return_address(n)
 
 /*
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index f80e13cbf8ec..1e93de68c044 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -21,7 +21,7 @@ struct mod_arch_specific {
 	struct mod_plt_sec	init;
 
 	/* for CONFIG_DYNAMIC_FTRACE */
-	struct plt_entry 	*ftrace_trampoline;
+	struct plt_entry	*ftrace_trampolines;
 };
 #endif
 
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 33d003d80121..94720388957f 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -7,10 +7,137 @@
  */
 
 #include <linux/linkage.h>
+#include <asm/asm-offsets.h>
 #include <asm/assembler.h>
 #include <asm/ftrace.h>
 #include <asm/insn.h>
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+/*
+ * Due to -fpatchable-function-entry=2, the compiler has placed two NOPs before
+ * the regular function prologue. For an enabled callsite, ftrace_init_nop() and
+ * ftrace_make_call() have patched those NOPs to:
+ *
+ * 	MOV	X9, LR
+ * 	BL	<entry>
+ *
+ * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
+ *
+ * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
+ * live, and x9-x18 are safe to clobber.
+ *
+ * We save the callsite's context into a pt_regs before invoking and ftrace
+ * callbacks. So that we can get a sensible backtrace, we create a stack record
+ * for the callsite and the ftrace entry assembly. This is not sufficient for
+ * reliable stacktrace: until we create the callsite stack record, its caller
+ * is missing from the LR and existing chain of frame records.
+ */
+	.macro  ftrace_regs_entry, allregs=0
+	/* Make room for pt_regs, plus a callee frame */
+	sub	sp, sp, #(S_FRAME_SIZE + 16)
+
+	/* Save function arguments (and x9 for simplicity) */
+	stp	x0, x1, [sp, #S_X0]
+	stp	x2, x3, [sp, #S_X2]
+	stp	x4, x5, [sp, #S_X4]
+	stp	x6, x7, [sp, #S_X6]
+	stp	x8, x9, [sp, #S_X8]
+
+	/* Optionally save the callee-saved registers, always save the FP */
+	.if \allregs == 1
+	stp	x10, x11, [sp, #S_X10]
+	stp	x12, x13, [sp, #S_X12]
+	stp	x14, x15, [sp, #S_X14]
+	stp	x16, x17, [sp, #S_X16]
+	stp	x18, x19, [sp, #S_X18]
+	stp	x20, x21, [sp, #S_X20]
+	stp	x22, x23, [sp, #S_X22]
+	stp	x24, x25, [sp, #S_X24]
+	stp	x26, x27, [sp, #S_X26]
+	stp	x28, x29, [sp, #S_X28]
+	.else
+	str	x29, [sp, #S_FP]
+	.endif
+
+	/* Save the callsite's SP and LR */
+	add	x10, sp, #(S_FRAME_SIZE + 16)
+	stp	x9, x10, [sp, #S_LR]
+
+	/* Save the PC after the ftrace callsite */
+	str	x30, [sp, #S_PC]
+
+	/* Create a frame record for the callsite above pt_regs */
+	stp	x29, x9, [sp, #S_FRAME_SIZE]
+	add	x29, sp, #S_FRAME_SIZE
+
+	/* Create our frame record within pt_regs. */
+	stp	x29, x30, [sp, #S_STACKFRAME]
+	add	x29, sp, #S_STACKFRAME
+	.endm
+
+ENTRY(ftrace_regs_caller)
+	ftrace_regs_entry	1
+	b	ftrace_common
+ENDPROC(ftrace_regs_caller)
+
+ENTRY(ftrace_caller)
+	ftrace_regs_entry	0
+	b	ftrace_common
+ENDPROC(ftrace_caller)
+
+ENTRY(ftrace_common)
+	sub	x0, x30, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
+	mov	x1, x9				// parent_ip (callsite's LR)
+	ldr_l	x2, function_trace_op		// op
+	mov	x3, sp				// regs
+
+GLOBAL(ftrace_call)
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+GLOBAL(ftrace_graph_call)		// ftrace_graph_caller();
+	nop				// If enabled, this will be replaced
+					// "b ftrace_graph_caller"
+#endif
+
+/*
+ * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
+ * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
+ * to restore x0-x8, x29, and x30.
+ */
+ftrace_common_return:
+	/* Restore function arguments */
+	ldp	x0, x1, [sp]
+	ldp	x2, x3, [sp, #S_X2]
+	ldp	x4, x5, [sp, #S_X4]
+	ldp	x6, x7, [sp, #S_X6]
+	ldr	x8, [sp, #S_X8]
+
+	/* Restore the callsite's FP, LR, PC */
+	ldr	x29, [sp, #S_FP]
+	ldr	x30, [sp, #S_LR]
+	ldr	x9, [sp, #S_PC]
+
+	/* Restore the callsite's SP */
+	add	sp, sp, #S_FRAME_SIZE + 16
+
+	ret	x9
+ENDPROC(ftrace_common)
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(ftrace_graph_caller)
+	ldr	x0, [sp, #S_PC]
+	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
+	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
+	ldr	x2, [sp, #S_FRAME_SIZE]	   	// parent fp (callsite's FP)
+	bl	prepare_ftrace_return
+	b	ftrace_common_return
+ENDPROC(ftrace_graph_caller)
+#else
+#endif
+
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
 /*
  * Gcc with -pg will put the following code in the beginning of each function:
  *      mov x0, x30
@@ -160,11 +287,6 @@ GLOBAL(ftrace_graph_call)		// ftrace_graph_caller();
 
 	mcount_exit
 ENDPROC(ftrace_caller)
-#endif /* CONFIG_DYNAMIC_FTRACE */
-
-ENTRY(ftrace_stub)
-	ret
-ENDPROC(ftrace_stub)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
@@ -184,7 +306,15 @@ ENTRY(ftrace_graph_caller)
 
 	mcount_exit
 ENDPROC(ftrace_graph_caller)
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+#endif /* CONFIG_DYNAMIC_FTRACE */
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+
+ENTRY(ftrace_stub)
+	ret
+ENDPROC(ftrace_stub)
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
  * void return_to_handler(void)
  *
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 822718eafdb4..aea652c33a38 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -62,6 +62,19 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(pc, 0, new, false);
 }
 
+#ifdef CONFIG_ARM64_MODULE_PLTS
+static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
+{
+	struct plt_entry *plt = mod->arch.ftrace_trampolines;
+
+	if (addr == FTRACE_ADDR)
+		return &plt[FTRACE_PLT_IDX];
+	if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
+		return &plt[FTRACE_REGS_PLT_IDX];
+	return NULL;
+}
+#endif
+
 /*
  * Turn on the call to ftrace_caller() in instrumented function
  */
@@ -74,19 +87,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	if (offset < -SZ_128M || offset >= SZ_128M) {
 #ifdef CONFIG_ARM64_MODULE_PLTS
 		struct module *mod;
-
-		/*
-		 * There is only one ftrace trampoline per module. For now,
-		 * this is not a problem since on arm64, all dynamic ftrace
-		 * invocations are routed via ftrace_caller(). This will need
-		 * to be revisited if support for multiple ftrace entry points
-		 * is added in the future, but for now, the pr_err() below
-		 * deals with a theoretical issue only.
-		 */
-		if (addr != FTRACE_ADDR) {
-			pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
-			return -EINVAL;
-		}
+		struct plt_entry *plt;
 
 		/*
 		 * On kernels that support module PLTs, the offset between the
@@ -105,7 +106,13 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		if (WARN_ON(!mod))
 			return -EINVAL;
 
-		addr = (unsigned long)mod->arch.ftrace_trampoline;
+		plt = get_ftrace_plt(mod, addr);
+		if (!plt) {
+			pr_err("ftrace: no module PLT for %ps\n", (void *)addr);
+			return -EINVAL;
+		}
+
+		addr = (unsigned long)plt;
 #else /* CONFIG_ARM64_MODULE_PLTS */
 		return -EINVAL;
 #endif /* CONFIG_ARM64_MODULE_PLTS */
@@ -117,6 +124,55 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	return ftrace_modify_code(pc, old, new, true);
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+			unsigned long addr)
+{
+	unsigned long pc = rec->ip;
+	u32 old, new;
+
+	old = aarch64_insn_gen_branch_imm(pc, old_addr,
+					  AARCH64_INSN_BRANCH_LINK);
+	new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK);
+
+	return ftrace_modify_code(pc, old, new, true);
+}
+
+/*
+ * The compiler has inserted two NOPs before the regular function prologue.
+ * All instrumented functions follow the AAPCS, so x0-x8 and x19-x30 are live,
+ * and x9-x18 are free for our use.
+ *
+ * At runtime we want to be able to swing a single NOP <-> BL to enable or
+ * disable the ftrace call. The BL requires us to save the original LR value,
+ * so here we insert a <MOV X9, LR> over the first NOP so the instructions
+ * before the regular prologue are:
+ *
+ * | Compiled | Disabled   | Enabled    |
+ * +----------+------------+------------+
+ * | NOP      | MOV X9, LR | MOV X9, LR |
+ * | NOP      | NOP        | BL <entry> |
+ *
+ * The LR value will be recovered by ftrace_regs_entry, and restored into LR
+ * before returning to the regular function prologue. When a function is not
+ * being traced, the MOV is not harmful given x9 is not live per the AAPCS.
+ *
+ * Note: ftrace_process_locs() has pre-adjusted rec->ip to be the address of
+ * the BL.
+ */
+int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+	unsigned long pc = rec->ip - AARCH64_INSN_SIZE;
+	u32 old, new;
+
+	old = aarch64_insn_gen_nop();
+	new = aarch64_insn_gen_move_reg(AARCH64_INSN_REG_9,
+					AARCH64_INSN_REG_LR,
+					AARCH64_INSN_VARIANT_64BIT);
+	return ftrace_modify_code(pc, old, new, true);
+}
+#endif
+
 /*
  * Turn off the call to ftrace_caller() in instrumented function
  */
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index b182442b87a3..65b08a74aec6 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/elf.h>
+#include <linux/ftrace.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sort.h>
@@ -330,7 +331,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 		tramp->sh_type = SHT_NOBITS;
 		tramp->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
 		tramp->sh_addralign = __alignof__(struct plt_entry);
-		tramp->sh_size = sizeof(struct plt_entry);
+		tramp->sh_size = NR_FTRACE_PLTS * sizeof(struct plt_entry);
 	}
 
 	return 0;
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 5f5bc3b94da7..00d21a420c60 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -486,21 +486,31 @@ static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
 	return NULL;
 }
 
+static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
+{
+	*plt = get_plt_entry(addr, plt);
+}
+
 int module_init_ftrace_plt(const Elf_Ehdr *hdr,
 			   const Elf_Shdr *sechdrs,
 			   struct module *mod)
 {
 #if defined(CONFIG_ARM64_MODULE_PLTS) && defined(CONFIG_DYNAMIC_FTRACE)
 	const Elf_Shdr *s;
-	struct plt_entry *plt;
+	struct plt_entry *plts;
 
 	s = find_section(hdr, sechdrs, ".text.ftrace_trampoline");
 	if (!s)
 		return -ENOEXEC;
 
-	plt = (void *)s->sh_addr;
-	*plt = get_plt_entry(FTRACE_ADDR, plt);
-	mod->arch.ftrace_trampoline = plt;
+	plts = (void *)s->sh_addr;
+
+	__init_plt(&plts[FTRACE_PLT_IDX], FTRACE_ADDR);
+
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
+		__init_plt(&plts[FTRACE_REGS_PLT_IDX], FTRACE_REGS_ADDR);
+
+	mod->arch.ftrace_trampolines = plts;
 #endif
 	return 0;
 }
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 8/8] arm64: ftrace: minimize ifdeffery
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-21 16:34   ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: amit.kachhap, ard.biesheuvel, catalin.marinas, deller, duwe,
	james.morse, jeyu, jpoimboe, jthierry, mark.rutland, mingo,
	peterz, rostedt, svens, takahiro.akashi, will

Now that we no longer refer to mod->arch.ftrace_trampolines in the body
of ftrace_make_call(), we can use IS_ENABLED() rather than ifdeffery,
and make the code easier to follow. Likewise in ftrace_make_nop().

Let's do so.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ftrace.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index aea652c33a38..8618faa82e6d 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -62,18 +62,18 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(pc, 0, new, false);
 }
 
-#ifdef CONFIG_ARM64_MODULE_PLTS
 static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
 {
+#ifdef CONFIG_ARM64_MODULE_PLTS
 	struct plt_entry *plt = mod->arch.ftrace_trampolines;
 
 	if (addr == FTRACE_ADDR)
 		return &plt[FTRACE_PLT_IDX];
 	if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
 		return &plt[FTRACE_REGS_PLT_IDX];
+#endif
 	return NULL;
 }
-#endif
 
 /*
  * Turn on the call to ftrace_caller() in instrumented function
@@ -85,10 +85,12 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	long offset = (long)pc - (long)addr;
 
 	if (offset < -SZ_128M || offset >= SZ_128M) {
-#ifdef CONFIG_ARM64_MODULE_PLTS
 		struct module *mod;
 		struct plt_entry *plt;
 
+		if (!IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+			return -EINVAL;
+
 		/*
 		 * On kernels that support module PLTs, the offset between the
 		 * branch instruction and its target may legally exceed the
@@ -113,9 +115,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		}
 
 		addr = (unsigned long)plt;
-#else /* CONFIG_ARM64_MODULE_PLTS */
-		return -EINVAL;
-#endif /* CONFIG_ARM64_MODULE_PLTS */
 	}
 
 	old = aarch64_insn_gen_nop();
@@ -185,9 +184,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 	long offset = (long)pc - (long)addr;
 
 	if (offset < -SZ_128M || offset >= SZ_128M) {
-#ifdef CONFIG_ARM64_MODULE_PLTS
 		u32 replaced;
 
+		if (!IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+			return -EINVAL;
+
 		/*
 		 * 'mod' is only set at module load time, but if we end up
 		 * dealing with an out-of-range condition, we can assume it
@@ -218,9 +219,6 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 			return -EINVAL;
 
 		validate = false;
-#else /* CONFIG_ARM64_MODULE_PLTS */
-		return -EINVAL;
-#endif /* CONFIG_ARM64_MODULE_PLTS */
 	} else {
 		old = aarch64_insn_gen_branch_imm(pc, addr,
 						  AARCH64_INSN_BRANCH_LINK);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 8/8] arm64: ftrace: minimize ifdeffery
@ 2019-10-21 16:34   ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-21 16:34 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel
  Cc: mark.rutland, jthierry, will, ard.biesheuvel, peterz,
	catalin.marinas, deller, jpoimboe, rostedt, takahiro.akashi,
	mingo, james.morse, jeyu, amit.kachhap, svens, duwe

Now that we no longer refer to mod->arch.ftrace_trampolines in the body
of ftrace_make_call(), we can use IS_ENABLED() rather than ifdeffery,
and make the code easier to follow. Likewise in ftrace_make_nop().

Let's do so.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ftrace.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index aea652c33a38..8618faa82e6d 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -62,18 +62,18 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ftrace_modify_code(pc, 0, new, false);
 }
 
-#ifdef CONFIG_ARM64_MODULE_PLTS
 static struct plt_entry *get_ftrace_plt(struct module *mod, unsigned long addr)
 {
+#ifdef CONFIG_ARM64_MODULE_PLTS
 	struct plt_entry *plt = mod->arch.ftrace_trampolines;
 
 	if (addr == FTRACE_ADDR)
 		return &plt[FTRACE_PLT_IDX];
 	if (addr == FTRACE_REGS_ADDR && IS_ENABLED(CONFIG_FTRACE_WITH_REGS))
 		return &plt[FTRACE_REGS_PLT_IDX];
+#endif
 	return NULL;
 }
-#endif
 
 /*
  * Turn on the call to ftrace_caller() in instrumented function
@@ -85,10 +85,12 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	long offset = (long)pc - (long)addr;
 
 	if (offset < -SZ_128M || offset >= SZ_128M) {
-#ifdef CONFIG_ARM64_MODULE_PLTS
 		struct module *mod;
 		struct plt_entry *plt;
 
+		if (!IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+			return -EINVAL;
+
 		/*
 		 * On kernels that support module PLTs, the offset between the
 		 * branch instruction and its target may legally exceed the
@@ -113,9 +115,6 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 		}
 
 		addr = (unsigned long)plt;
-#else /* CONFIG_ARM64_MODULE_PLTS */
-		return -EINVAL;
-#endif /* CONFIG_ARM64_MODULE_PLTS */
 	}
 
 	old = aarch64_insn_gen_nop();
@@ -185,9 +184,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 	long offset = (long)pc - (long)addr;
 
 	if (offset < -SZ_128M || offset >= SZ_128M) {
-#ifdef CONFIG_ARM64_MODULE_PLTS
 		u32 replaced;
 
+		if (!IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+			return -EINVAL;
+
 		/*
 		 * 'mod' is only set at module load time, but if we end up
 		 * dealing with an out-of-range condition, we can assume it
@@ -218,9 +219,6 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 			return -EINVAL;
 
 		validate = false;
-#else /* CONFIG_ARM64_MODULE_PLTS */
-		return -EINVAL;
-#endif /* CONFIG_ARM64_MODULE_PLTS */
 	} else {
 		old = aarch64_insn_gen_branch_imm(pc, addr,
 						  AARCH64_INSN_BRANCH_LINK);
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-21 16:34   ` Mark Rutland
@ 2019-10-21 18:07     ` Steven Rostedt
  -1 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2019-10-21 18:07 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, linux-kernel, amit.kachhap, ard.biesheuvel,
	catalin.marinas, deller, duwe, james.morse, jeyu, jpoimboe,
	jthierry, mingo, peterz, svens, takahiro.akashi, will

On Mon, 21 Oct 2019 17:34:19 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> Architectures may need to perform special initialization of ftrace
> callsites, and today they do so by special-casing ftrace_make_nop() when
> the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> patchable-function-entry), we don't have an mcount-like symbol and don't
> want a synthetic MCOUNT_ADDR, but we may need to perform some
> initialization of callsites.
> 
> To make it possible to separate initialization from runtime
> modification, and to handle cases without an mcount-like symbol, this
> patch adds an optional ftrace_init_nop() function that architectures can
> implement, which does not pass a branch address.
> 
> Where an architecture does not provide ftrace_init_nop(), we will fall
> back to the existing behaviour of calling ftrace_make_nop() with
> MCOUNT_ADDR.
> 
> At the same time, ftrace_code_disable() is renamed to
> ftrace_code_init_disabled() to make it clearer that it is intended to
> intialize a callsite into a disabled state, and is not for disabling a
> callsite that has been runtime enabled.

To make the name even better, let's just rename it to:

 ftrace_nop_initialization()

I think that may be the best description for it.

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Torsten Duwe <duwe@suse.de>
> ---
>  kernel/trace/ftrace.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f296d89be757..afd7e210e595 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
>  	return &iter->pg->records[iter->index];
>  }
>  
> +#ifndef ftrace_init_nop
> +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +}
> +#endif

Can you place the above in the ftrace.h header. That's where that would
belong.

#ifndef ftrace_init_nop
struct module;
static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
{
	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
}
#endif

-- Steve

> +
>  static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_code_init_disabled(struct module *mod, struct dyn_ftrace *rec)
>  {
>  	int ret;
>  
>  	if (unlikely(ftrace_disabled))
>  		return 0;
>  
> -	ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +	ret = ftrace_init_nop(mod, rec);
>  	if (ret) {
>  		ftrace_bug_type = FTRACE_BUG_INIT;
>  		ftrace_bug(ret, rec);
> @@ -2943,7 +2950,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
>  			 * to the NOP instructions.
>  			 */
>  			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
> -			    !ftrace_code_disable(mod, p))
> +			    !ftrace_code_init_disabled(mod, p))
>  				break;
>  
>  			update_cnt++;


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-21 18:07     ` Steven Rostedt
  0 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2019-10-21 18:07 UTC (permalink / raw)
  To: Mark Rutland
  Cc: jthierry, will, ard.biesheuvel, peterz, catalin.marinas, deller,
	jpoimboe, linux-kernel, takahiro.akashi, mingo, james.morse,
	jeyu, amit.kachhap, svens, duwe, linux-arm-kernel

On Mon, 21 Oct 2019 17:34:19 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> Architectures may need to perform special initialization of ftrace
> callsites, and today they do so by special-casing ftrace_make_nop() when
> the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> patchable-function-entry), we don't have an mcount-like symbol and don't
> want a synthetic MCOUNT_ADDR, but we may need to perform some
> initialization of callsites.
> 
> To make it possible to separate initialization from runtime
> modification, and to handle cases without an mcount-like symbol, this
> patch adds an optional ftrace_init_nop() function that architectures can
> implement, which does not pass a branch address.
> 
> Where an architecture does not provide ftrace_init_nop(), we will fall
> back to the existing behaviour of calling ftrace_make_nop() with
> MCOUNT_ADDR.
> 
> At the same time, ftrace_code_disable() is renamed to
> ftrace_code_init_disabled() to make it clearer that it is intended to
> intialize a callsite into a disabled state, and is not for disabling a
> callsite that has been runtime enabled.

To make the name even better, let's just rename it to:

 ftrace_nop_initialization()

I think that may be the best description for it.

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Torsten Duwe <duwe@suse.de>
> ---
>  kernel/trace/ftrace.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f296d89be757..afd7e210e595 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
>  	return &iter->pg->records[iter->index];
>  }
>  
> +#ifndef ftrace_init_nop
> +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +}
> +#endif

Can you place the above in the ftrace.h header. That's where that would
belong.

#ifndef ftrace_init_nop
struct module;
static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
{
	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
}
#endif

-- Steve

> +
>  static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_code_init_disabled(struct module *mod, struct dyn_ftrace *rec)
>  {
>  	int ret;
>  
>  	if (unlikely(ftrace_disabled))
>  		return 0;
>  
> -	ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +	ret = ftrace_init_nop(mod, rec);
>  	if (ret) {
>  		ftrace_bug_type = FTRACE_BUG_INIT;
>  		ftrace_bug(ret, rec);
> @@ -2943,7 +2950,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
>  			 * to the NOP instructions.
>  			 */
>  			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
> -			    !ftrace_code_disable(mod, p))
> +			    !ftrace_code_init_disabled(mod, p))
>  				break;
>  
>  			update_cnt++;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-21 18:07     ` Steven Rostedt
@ 2019-10-22 11:28       ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-22 11:28 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-arm-kernel, linux-kernel, amit.kachhap, ard.biesheuvel,
	catalin.marinas, deller, duwe, james.morse, jeyu, jpoimboe,
	jthierry, mingo, peterz, svens, takahiro.akashi, will

On Mon, Oct 21, 2019 at 02:07:56PM -0400, Steven Rostedt wrote:
> On Mon, 21 Oct 2019 17:34:19 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Architectures may need to perform special initialization of ftrace
> > callsites, and today they do so by special-casing ftrace_make_nop() when
> > the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> > patchable-function-entry), we don't have an mcount-like symbol and don't
> > want a synthetic MCOUNT_ADDR, but we may need to perform some
> > initialization of callsites.
> > 
> > To make it possible to separate initialization from runtime
> > modification, and to handle cases without an mcount-like symbol, this
> > patch adds an optional ftrace_init_nop() function that architectures can
> > implement, which does not pass a branch address.
> > 
> > Where an architecture does not provide ftrace_init_nop(), we will fall
> > back to the existing behaviour of calling ftrace_make_nop() with
> > MCOUNT_ADDR.
> > 
> > At the same time, ftrace_code_disable() is renamed to
> > ftrace_code_init_disabled() to make it clearer that it is intended to
> > intialize a callsite into a disabled state, and is not for disabling a
> > callsite that has been runtime enabled.
> 
> To make the name even better, let's just rename it to:
> 
>  ftrace_nop_initialization()
> 
> I think that may be the best description for it.

Perhaps ftrace_nop_initialize(), so that it's not a noun?

I've made it ftrace_nop_initialization() in my branch for now.

> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f296d89be757..afd7e210e595 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> >  	return &iter->pg->records[iter->index];
> >  }
> >  
> > +#ifndef ftrace_init_nop
> > +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > +{
> > +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > +}
> > +#endif
> 
> Can you place the above in the ftrace.h header. That's where that would
> belong.
> 
> #ifndef ftrace_init_nop
> struct module;
> static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> {
> 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> }
> #endif

True.

I've put this immediately after ftrace_make_nop() in the header, and
given it a kerneldoc comment. There's a declaration for struct module at
the top of the header, so I've just relied on that

That looks like:

| /**
|  * ftrace_init_nop - initialize a nop call site
|  * @mod: module structure if called by module load initialization
|  * @rec: the mcount call site record
|  *
|  * This is a very sensitive operation and great care needs
|  * to be taken by the arch.  The operation should carefully
|  * read the location, check to see if what is read is indeed
|  * what we expect it to be, and then on success of the compare,
|  * it should write to the location.
|  *
|  * The code segment at @rec->ip should be as initialized by the
|  * compiler
|  *
|  * Return must be:
|  *  0 on success
|  *  -EFAULT on error reading the location
|  *  -EINVAL on a failed compare of the contents
|  *  -EPERM  on error writing to the location
|  * Any other value will be considered a failure.
|  */
| #ifndef ftrace_init_nop
| static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
| {
| 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
| }
| #endif

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-22 11:28       ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-22 11:28 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: jthierry, will, ard.biesheuvel, peterz, catalin.marinas, deller,
	jpoimboe, linux-kernel, takahiro.akashi, mingo, james.morse,
	jeyu, amit.kachhap, svens, duwe, linux-arm-kernel

On Mon, Oct 21, 2019 at 02:07:56PM -0400, Steven Rostedt wrote:
> On Mon, 21 Oct 2019 17:34:19 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Architectures may need to perform special initialization of ftrace
> > callsites, and today they do so by special-casing ftrace_make_nop() when
> > the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> > patchable-function-entry), we don't have an mcount-like symbol and don't
> > want a synthetic MCOUNT_ADDR, but we may need to perform some
> > initialization of callsites.
> > 
> > To make it possible to separate initialization from runtime
> > modification, and to handle cases without an mcount-like symbol, this
> > patch adds an optional ftrace_init_nop() function that architectures can
> > implement, which does not pass a branch address.
> > 
> > Where an architecture does not provide ftrace_init_nop(), we will fall
> > back to the existing behaviour of calling ftrace_make_nop() with
> > MCOUNT_ADDR.
> > 
> > At the same time, ftrace_code_disable() is renamed to
> > ftrace_code_init_disabled() to make it clearer that it is intended to
> > intialize a callsite into a disabled state, and is not for disabling a
> > callsite that has been runtime enabled.
> 
> To make the name even better, let's just rename it to:
> 
>  ftrace_nop_initialization()
> 
> I think that may be the best description for it.

Perhaps ftrace_nop_initialize(), so that it's not a noun?

I've made it ftrace_nop_initialization() in my branch for now.

> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f296d89be757..afd7e210e595 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> >  	return &iter->pg->records[iter->index];
> >  }
> >  
> > +#ifndef ftrace_init_nop
> > +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > +{
> > +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > +}
> > +#endif
> 
> Can you place the above in the ftrace.h header. That's where that would
> belong.
> 
> #ifndef ftrace_init_nop
> struct module;
> static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> {
> 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> }
> #endif

True.

I've put this immediately after ftrace_make_nop() in the header, and
given it a kerneldoc comment. There's a declaration for struct module at
the top of the header, so I've just relied on that

That looks like:

| /**
|  * ftrace_init_nop - initialize a nop call site
|  * @mod: module structure if called by module load initialization
|  * @rec: the mcount call site record
|  *
|  * This is a very sensitive operation and great care needs
|  * to be taken by the arch.  The operation should carefully
|  * read the location, check to see if what is read is indeed
|  * what we expect it to be, and then on success of the compare,
|  * it should write to the location.
|  *
|  * The code segment at @rec->ip should be as initialized by the
|  * compiler
|  *
|  * Return must be:
|  *  0 on success
|  *  -EFAULT on error reading the location
|  *  -EINVAL on a failed compare of the contents
|  *  -EPERM  on error writing to the location
|  * Any other value will be considered a failure.
|  */
| #ifndef ftrace_init_nop
| static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
| {
| 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
| }
| #endif

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-22 11:28       ` Mark Rutland
@ 2019-10-22 12:54         ` Steven Rostedt
  -1 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2019-10-22 12:54 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, linux-kernel, amit.kachhap, ard.biesheuvel,
	catalin.marinas, deller, duwe, james.morse, jeyu, jpoimboe,
	jthierry, mingo, peterz, svens, takahiro.akashi, will

On Tue, 22 Oct 2019 12:28:11 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> > To make the name even better, let's just rename it to:
> > 
> >  ftrace_nop_initialization()
> > 
> > I think that may be the best description for it.  
> 
> Perhaps ftrace_nop_initialize(), so that it's not a noun?
> 
> I've made it ftrace_nop_initialization() in my branch for now.

I'm fine with ftrace_nop_initialize().

> 
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index f296d89be757..afd7e210e595 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> > >  	return &iter->pg->records[iter->index];
> > >  }
> > >  
> > > +#ifndef ftrace_init_nop
> > > +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > > +{
> > > +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > > +}
> > > +#endif  
> > 
> > Can you place the above in the ftrace.h header. That's where that would
> > belong.
> > 
> > #ifndef ftrace_init_nop
> > struct module;
> > static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > {
> > 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > }
> > #endif  
> 
> True.
> 
> I've put this immediately after ftrace_make_nop() in the header, and
> given it a kerneldoc comment. There's a declaration for struct module at
> the top of the header, so I've just relied on that
> 
> That looks like:
> 
> | /**
> |  * ftrace_init_nop - initialize a nop call site
> |  * @mod: module structure if called by module load initialization
> |  * @rec: the mcount call site record

Perhaps say "mcount/fentry"

> |  *
> |  * This is a very sensitive operation and great care needs
> |  * to be taken by the arch.  The operation should carefully
> |  * read the location, check to see if what is read is indeed
> |  * what we expect it to be, and then on success of the compare,
> |  * it should write to the location.
> |  *
> |  * The code segment at @rec->ip should be as initialized by the

"should be as" is a bit confusing. What about?

 "The code segment at @rec->ip should contain the contents created by
  the compiler".

-- Steve


> |  * compiler
> |  *
> |  * Return must be:
> |  *  0 on success
> |  *  -EFAULT on error reading the location
> |  *  -EINVAL on a failed compare of the contents
> |  *  -EPERM  on error writing to the location
> |  * Any other value will be considered a failure.
> |  */
> | #ifndef ftrace_init_nop
> | static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> | {
> | 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> | }
> | #endif
> 
> Thanks,
> Mark.


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-22 12:54         ` Steven Rostedt
  0 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2019-10-22 12:54 UTC (permalink / raw)
  To: Mark Rutland
  Cc: jthierry, will, ard.biesheuvel, peterz, catalin.marinas, deller,
	jpoimboe, linux-kernel, takahiro.akashi, mingo, james.morse,
	jeyu, amit.kachhap, svens, duwe, linux-arm-kernel

On Tue, 22 Oct 2019 12:28:11 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> > To make the name even better, let's just rename it to:
> > 
> >  ftrace_nop_initialization()
> > 
> > I think that may be the best description for it.  
> 
> Perhaps ftrace_nop_initialize(), so that it's not a noun?
> 
> I've made it ftrace_nop_initialization() in my branch for now.

I'm fine with ftrace_nop_initialize().

> 
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index f296d89be757..afd7e210e595 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> > >  	return &iter->pg->records[iter->index];
> > >  }
> > >  
> > > +#ifndef ftrace_init_nop
> > > +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > > +{
> > > +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > > +}
> > > +#endif  
> > 
> > Can you place the above in the ftrace.h header. That's where that would
> > belong.
> > 
> > #ifndef ftrace_init_nop
> > struct module;
> > static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > {
> > 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > }
> > #endif  
> 
> True.
> 
> I've put this immediately after ftrace_make_nop() in the header, and
> given it a kerneldoc comment. There's a declaration for struct module at
> the top of the header, so I've just relied on that
> 
> That looks like:
> 
> | /**
> |  * ftrace_init_nop - initialize a nop call site
> |  * @mod: module structure if called by module load initialization
> |  * @rec: the mcount call site record

Perhaps say "mcount/fentry"

> |  *
> |  * This is a very sensitive operation and great care needs
> |  * to be taken by the arch.  The operation should carefully
> |  * read the location, check to see if what is read is indeed
> |  * what we expect it to be, and then on success of the compare,
> |  * it should write to the location.
> |  *
> |  * The code segment at @rec->ip should be as initialized by the

"should be as" is a bit confusing. What about?

 "The code segment at @rec->ip should contain the contents created by
  the compiler".

-- Steve


> |  * compiler
> |  *
> |  * Return must be:
> |  *  0 on success
> |  *  -EFAULT on error reading the location
> |  *  -EINVAL on a failed compare of the contents
> |  *  -EPERM  on error writing to the location
> |  * Any other value will be considered a failure.
> |  */
> | #ifndef ftrace_init_nop
> | static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> | {
> | 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> | }
> | #endif
> 
> Thanks,
> Mark.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-22 12:54         ` Steven Rostedt
@ 2019-10-22 15:30           ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-22 15:30 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-arm-kernel, linux-kernel, amit.kachhap, ard.biesheuvel,
	catalin.marinas, deller, duwe, james.morse, jeyu, jpoimboe,
	jthierry, mingo, peterz, svens, takahiro.akashi, will

On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:
> On Tue, 22 Oct 2019 12:28:11 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> > > To make the name even better, let's just rename it to:
> > > 
> > >  ftrace_nop_initialization()
> > > 
> > > I think that may be the best description for it.  
> > 
> > Perhaps ftrace_nop_initialize(), so that it's not a noun?
> > 
> > I've made it ftrace_nop_initialization() in my branch for now.
> 
> I'm fine with ftrace_nop_initialize().

It's settled, then. :)

[...]

> > | /**
> > |  * ftrace_init_nop - initialize a nop call site
> > |  * @mod: module structure if called by module load initialization
> > |  * @rec: the mcount call site record
> 
> Perhaps say "mcount/fentry"

This is the exact wording that ftrace_make_nop and ftrace_modify_call
have. For consistency, I think those should all match.

I can add " (e.g. mcount/fentry)" to all of those if you'd like?

... or leave them all as-is?

> > |  *
> > |  * This is a very sensitive operation and great care needs
> > |  * to be taken by the arch.  The operation should carefully
> > |  * read the location, check to see if what is read is indeed
> > |  * what we expect it to be, and then on success of the compare,
> > |  * it should write to the location.
> > |  *
> > |  * The code segment at @rec->ip should be as initialized by the
> 
> "should be as" is a bit confusing. What about?
> 
>  "The code segment at @rec->ip should contain the contents created by
>   the compiler".

Works for me.

Mark.

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-22 15:30           ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-22 15:30 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: jthierry, will, ard.biesheuvel, peterz, catalin.marinas, deller,
	jpoimboe, linux-kernel, takahiro.akashi, mingo, james.morse,
	jeyu, amit.kachhap, svens, duwe, linux-arm-kernel

On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:
> On Tue, 22 Oct 2019 12:28:11 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> > > To make the name even better, let's just rename it to:
> > > 
> > >  ftrace_nop_initialization()
> > > 
> > > I think that may be the best description for it.  
> > 
> > Perhaps ftrace_nop_initialize(), so that it's not a noun?
> > 
> > I've made it ftrace_nop_initialization() in my branch for now.
> 
> I'm fine with ftrace_nop_initialize().

It's settled, then. :)

[...]

> > | /**
> > |  * ftrace_init_nop - initialize a nop call site
> > |  * @mod: module structure if called by module load initialization
> > |  * @rec: the mcount call site record
> 
> Perhaps say "mcount/fentry"

This is the exact wording that ftrace_make_nop and ftrace_modify_call
have. For consistency, I think those should all match.

I can add " (e.g. mcount/fentry)" to all of those if you'd like?

... or leave them all as-is?

> > |  *
> > |  * This is a very sensitive operation and great care needs
> > |  * to be taken by the arch.  The operation should carefully
> > |  * read the location, check to see if what is read is indeed
> > |  * what we expect it to be, and then on success of the compare,
> > |  * it should write to the location.
> > |  *
> > |  * The code segment at @rec->ip should be as initialized by the
> 
> "should be as" is a bit confusing. What about?
> 
>  "The code segment at @rec->ip should contain the contents created by
>   the compiler".

Works for me.

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-22 15:30           ` Mark Rutland
@ 2019-10-22 15:33             ` Mark Rutland
  -1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-22 15:33 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: jthierry, will, ard.biesheuvel, peterz, catalin.marinas, deller,
	jpoimboe, linux-kernel, takahiro.akashi, mingo, james.morse,
	jeyu, amit.kachhap, svens, duwe, linux-arm-kernel

On Tue, Oct 22, 2019 at 04:30:35PM +0100, Mark Rutland wrote:
> On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:
> > On Tue, 22 Oct 2019 12:28:11 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > | /**
> > > |  * ftrace_init_nop - initialize a nop call site
> > > |  * @mod: module structure if called by module load initialization
> > > |  * @rec: the mcount call site record
> > 
> > Perhaps say "mcount/fentry"
> 
> This is the exact wording that ftrace_make_nop and ftrace_modify_call
> have. For consistency, I think those should all match.

Now that I read this again, I see what you meant.

If it's ok, I'll change those to:

| @rec: the call site record (e.g. mcount/fentry)

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-22 15:33             ` Mark Rutland
  0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2019-10-22 15:33 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: jthierry, svens, ard.biesheuvel, peterz, catalin.marinas, deller,
	jeyu, linux-kernel, takahiro.akashi, mingo, james.morse,
	jpoimboe, amit.kachhap, will, duwe, linux-arm-kernel

On Tue, Oct 22, 2019 at 04:30:35PM +0100, Mark Rutland wrote:
> On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:
> > On Tue, 22 Oct 2019 12:28:11 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > | /**
> > > |  * ftrace_init_nop - initialize a nop call site
> > > |  * @mod: module structure if called by module load initialization
> > > |  * @rec: the mcount call site record
> > 
> > Perhaps say "mcount/fentry"
> 
> This is the exact wording that ftrace_make_nop and ftrace_modify_call
> have. For consistency, I think those should all match.

Now that I read this again, I see what you meant.

If it's ok, I'll change those to:

| @rec: the call site record (e.g. mcount/fentry)

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
  2019-10-22 15:33             ` Mark Rutland
@ 2019-10-22 16:01               ` Steven Rostedt
  -1 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2019-10-22 16:01 UTC (permalink / raw)
  To: Mark Rutland
  Cc: jthierry, will, ard.biesheuvel, peterz, catalin.marinas, deller,
	jpoimboe, linux-kernel, takahiro.akashi, mingo, james.morse,
	jeyu, amit.kachhap, svens, duwe, linux-arm-kernel

On Tue, 22 Oct 2019 16:33:35 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Oct 22, 2019 at 04:30:35PM +0100, Mark Rutland wrote:
> > On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:  
> > > On Tue, 22 Oct 2019 12:28:11 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:  
> > > > | /**
> > > > |  * ftrace_init_nop - initialize a nop call site
> > > > |  * @mod: module structure if called by module load initialization
> > > > |  * @rec: the mcount call site record  
> > > 
> > > Perhaps say "mcount/fentry"  
> > 
> > This is the exact wording that ftrace_make_nop and ftrace_modify_call
> > have. For consistency, I think those should all match.  
> 
> Now that I read this again, I see what you meant.
> 
> If it's ok, I'll change those to:
> 
> | @rec: the call site record (e.g. mcount/fentry)
> 

Ack

-- Steve

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/8] ftrace: add ftrace_init_nop()
@ 2019-10-22 16:01               ` Steven Rostedt
  0 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2019-10-22 16:01 UTC (permalink / raw)
  To: Mark Rutland
  Cc: jthierry, svens, ard.biesheuvel, peterz, catalin.marinas, deller,
	jeyu, linux-kernel, takahiro.akashi, mingo, james.morse,
	jpoimboe, amit.kachhap, will, duwe, linux-arm-kernel

On Tue, 22 Oct 2019 16:33:35 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Oct 22, 2019 at 04:30:35PM +0100, Mark Rutland wrote:
> > On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:  
> > > On Tue, 22 Oct 2019 12:28:11 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:  
> > > > | /**
> > > > |  * ftrace_init_nop - initialize a nop call site
> > > > |  * @mod: module structure if called by module load initialization
> > > > |  * @rec: the mcount call site record  
> > > 
> > > Perhaps say "mcount/fentry"  
> > 
> > This is the exact wording that ftrace_make_nop and ftrace_modify_call
> > have. For consistency, I think those should all match.  
> 
> Now that I read this again, I see what you meant.
> 
> If it's ok, I'll change those to:
> 
> | @rec: the call site record (e.g. mcount/fentry)
> 

Ack

-- Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 0/8] arm64: ftrace cleanup + FTRACE_WITH_REGS
  2019-10-21 16:34 ` Mark Rutland
@ 2019-10-24 16:32   ` Ard Biesheuvel
  -1 siblings, 0 replies; 32+ messages in thread
From: Ard Biesheuvel @ 2019-10-24 16:32 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, Linux Kernel Mailing List, Amit Daniel Kachhap,
	Catalin Marinas, Helge Deller, Torsten Duwe, James Morse,
	Jessica Yu, Josh Poimboeuf, jthierry, Ingo Molnar,
	Peter Zijlstra, Steven Rostedt, svens, AKASHI Takahiro,
	Will Deacon

On Mon, 21 Oct 2019 at 18:34, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi,
>
> This series is a reworked version of Torsten's v8 FTRACE_WITH_REGS
> series [1]. I've tried to rework the existing code in preparatory
> patches so that the patchable-function-entry bits slot in with fewer
> surprises. This version is based on v5.4-rc3, and can be found in my
> arm64/ftrace-with-regs branch [2].
>
> I've added an (optional) ftrace_init_nop(), which the core code uses to
> initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> symbol, and more cleanly separates the one-time initialization of the
> callsite from dynamic NOP<->CALL modification. Architectures which don't
> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
>
> I've moved the module PLT initialization to module load time, which
> simplifies runtime callsite modification. This also means that we don't
> transitently mark the module text RW, and will allow for the removal of
> module_disable_ro().
>
> Since the last posting, parisc gained ftrace support using
> patchable-function-entry. I've made the handling of module callsite
> locations common in kernel/module.c with a new FTRACE_CALLSITE_SECTION
> definition, and removed the newly redundant bits from arch/parisc.
>
> Thanks,
> Mark.
>
> [1] https://lore.kernel.org/r/20190208150826.44EBC68DD2@newverein.lst.de
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs
>
> Mark Rutland (7):
>   ftrace: add ftrace_init_nop()
>   module/ftrace: handle patchable-function-entry
>   arm64: module: rework special section handling
>   arm64: module/ftrace: intialize PLT at load time
>   arm64: insn: add encoder for MOV (register)
>   arm64: asm-offsets: add S_FP
>   arm64: ftrace: minimize ifdeffery
>
> Torsten Duwe (1):
>   arm64: implement ftrace with regs
>

For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>  arch/arm64/Kconfig               |   2 +
>  arch/arm64/Makefile              |   5 ++
>  arch/arm64/include/asm/ftrace.h  |  23 +++++++
>  arch/arm64/include/asm/insn.h    |   3 +
>  arch/arm64/include/asm/module.h  |   2 +-
>  arch/arm64/kernel/asm-offsets.c  |   1 +
>  arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
>  arch/arm64/kernel/ftrace.c       | 123 ++++++++++++++++++++--------------
>  arch/arm64/kernel/insn.c         |  13 ++++
>  arch/arm64/kernel/module-plts.c  |   3 +-
>  arch/arm64/kernel/module.c       |  57 +++++++++++++---
>  arch/parisc/Makefile             |   1 -
>  arch/parisc/kernel/module.c      |  10 ++-
>  arch/parisc/kernel/module.lds    |   7 --
>  include/linux/ftrace.h           |   5 ++
>  kernel/module.c                  |   2 +-
>  kernel/trace/ftrace.c            |  13 +++-
>  17 files changed, 330 insertions(+), 80 deletions(-)
>  delete mode 100644 arch/parisc/kernel/module.lds
>
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 0/8] arm64: ftrace cleanup + FTRACE_WITH_REGS
@ 2019-10-24 16:32   ` Ard Biesheuvel
  0 siblings, 0 replies; 32+ messages in thread
From: Ard Biesheuvel @ 2019-10-24 16:32 UTC (permalink / raw)
  To: Mark Rutland
  Cc: jthierry, Will Deacon, Peter Zijlstra, Catalin Marinas,
	Helge Deller, Josh Poimboeuf, Linux Kernel Mailing List,
	Steven Rostedt, AKASHI Takahiro, Ingo Molnar, James Morse,
	Jessica Yu, Amit Daniel Kachhap, svens, Torsten Duwe,
	linux-arm-kernel

On Mon, 21 Oct 2019 at 18:34, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi,
>
> This series is a reworked version of Torsten's v8 FTRACE_WITH_REGS
> series [1]. I've tried to rework the existing code in preparatory
> patches so that the patchable-function-entry bits slot in with fewer
> surprises. This version is based on v5.4-rc3, and can be found in my
> arm64/ftrace-with-regs branch [2].
>
> I've added an (optional) ftrace_init_nop(), which the core code uses to
> initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR
> symbol, and more cleanly separates the one-time initialization of the
> callsite from dynamic NOP<->CALL modification. Architectures which don't
> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR.
>
> I've moved the module PLT initialization to module load time, which
> simplifies runtime callsite modification. This also means that we don't
> transitently mark the module text RW, and will allow for the removal of
> module_disable_ro().
>
> Since the last posting, parisc gained ftrace support using
> patchable-function-entry. I've made the handling of module callsite
> locations common in kernel/module.c with a new FTRACE_CALLSITE_SECTION
> definition, and removed the newly redundant bits from arch/parisc.
>
> Thanks,
> Mark.
>
> [1] https://lore.kernel.org/r/20190208150826.44EBC68DD2@newverein.lst.de
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs
>
> Mark Rutland (7):
>   ftrace: add ftrace_init_nop()
>   module/ftrace: handle patchable-function-entry
>   arm64: module: rework special section handling
>   arm64: module/ftrace: intialize PLT at load time
>   arm64: insn: add encoder for MOV (register)
>   arm64: asm-offsets: add S_FP
>   arm64: ftrace: minimize ifdeffery
>
> Torsten Duwe (1):
>   arm64: implement ftrace with regs
>

For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>  arch/arm64/Kconfig               |   2 +
>  arch/arm64/Makefile              |   5 ++
>  arch/arm64/include/asm/ftrace.h  |  23 +++++++
>  arch/arm64/include/asm/insn.h    |   3 +
>  arch/arm64/include/asm/module.h  |   2 +-
>  arch/arm64/kernel/asm-offsets.c  |   1 +
>  arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++--
>  arch/arm64/kernel/ftrace.c       | 123 ++++++++++++++++++++--------------
>  arch/arm64/kernel/insn.c         |  13 ++++
>  arch/arm64/kernel/module-plts.c  |   3 +-
>  arch/arm64/kernel/module.c       |  57 +++++++++++++---
>  arch/parisc/Makefile             |   1 -
>  arch/parisc/kernel/module.c      |  10 ++-
>  arch/parisc/kernel/module.lds    |   7 --
>  include/linux/ftrace.h           |   5 ++
>  kernel/module.c                  |   2 +-
>  kernel/trace/ftrace.c            |  13 +++-
>  17 files changed, 330 insertions(+), 80 deletions(-)
>  delete mode 100644 arch/parisc/kernel/module.lds
>
> --
> 2.11.0
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread
end of thread, other threads:[~2019-10-24 16:32 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 16:34 [PATCH 0/8] arm64: ftrace cleanup + FTRACE_WITH_REGS Mark Rutland
2019-10-21 16:34 ` Mark Rutland
2019-10-21 16:34 ` [PATCH 1/8] ftrace: add ftrace_init_nop() Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 18:07   ` Steven Rostedt
2019-10-21 18:07     ` Steven Rostedt
2019-10-22 11:28     ` Mark Rutland
2019-10-22 11:28       ` Mark Rutland
2019-10-22 12:54       ` Steven Rostedt
2019-10-22 12:54         ` Steven Rostedt
2019-10-22 15:30         ` Mark Rutland
2019-10-22 15:30           ` Mark Rutland
2019-10-22 15:33           ` Mark Rutland
2019-10-22 15:33             ` Mark Rutland
2019-10-22 16:01             ` Steven Rostedt
2019-10-22 16:01               ` Steven Rostedt
2019-10-21 16:34 ` [PATCH 2/8] module/ftrace: handle patchable-function-entry Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 16:34 ` [PATCH 3/8] arm64: module: rework special section handling Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 16:34 ` [PATCH 4/8] arm64: module/ftrace: intialize PLT at load time Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 16:34 ` [PATCH 5/8] arm64: insn: add encoder for MOV (register) Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 16:34 ` [PATCH 6/8] arm64: asm-offsets: add S_FP Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 16:34 ` [PATCH 7/8] arm64: implement ftrace with regs Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-21 16:34 ` [PATCH 8/8] arm64: ftrace: minimize ifdeffery Mark Rutland
2019-10-21 16:34   ` Mark Rutland
2019-10-24 16:32 ` [PATCH 0/8] arm64: ftrace cleanup + FTRACE_WITH_REGS Ard Biesheuvel
2019-10-24 16:32   ` Ard Biesheuvel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.