This series implements livepatch on PPC32 and implements
CONFIG_DYNAMIC_FTRACE_WITH_ARGS to simplify ftracing.
v2:
- Fix problem with strict modules RWX
- Convert powerpc to CONFIG_DYNAMIC_FTRACE_WITH_ARGS
- Convert function graph tracing to C
- Refactor PPC32 versus PPC64
Signed-off-by: Christophe Leroy <[email protected]>
Christophe Leroy (13):
livepatch: Fix build failure on 32 bits processors
tracing: Fix selftest config check for function graph start up test
powerpc/module_32: Fix livepatching for RO modules
powerpc/ftrace: Add support for livepatch to PPC32
powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32
powerpc/ftrace: Simplify PPC32's return_to_handler()
powerpc/ftrace: Prepare PPC32's ftrace_caller() for
CONFIG_DYNAMIC_FTRACE_WITH_ARGS
powerpc/ftrace: Prepare PPC64's ftrace_caller() for
CONFIG_DYNAMIC_FTRACE_WITH_ARGS
powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller
powerpc/ftrace: directly call of function graph tracer by ftrace
caller
powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
powerpc/ftrace: Remove ftrace_32.S
arch/powerpc/Kconfig | 7 +-
arch/powerpc/include/asm/ftrace.h | 62 +++---
arch/powerpc/include/asm/livepatch.h | 12 +-
arch/powerpc/include/asm/thread_info.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/kernel/module_32.c | 44 +++--
arch/powerpc/kernel/trace/Makefile | 7 +-
arch/powerpc/kernel/trace/ftrace.c | 32 +--
arch/powerpc/kernel/trace/ftrace_32.S | 187 ------------------
.../trace/{ftrace_64.S => ftrace_low.S} | 14 ++
...ftrace_64_mprofile.S => ftrace_mprofile.S} | 158 +++++++--------
kernel/livepatch/core.c | 4 +-
kernel/trace/trace_selftest.c | 6 +-
13 files changed, 178 insertions(+), 359 deletions(-)
delete mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
rename arch/powerpc/kernel/trace/{ftrace_64.S => ftrace_low.S} (85%)
rename arch/powerpc/kernel/trace/{ftrace_64_mprofile.S => ftrace_mprofile.S} (75%)
--
2.33.1
Trying to build livepatch on powerpc/32 results in:
kernel/livepatch/core.c: In function 'klp_resolve_symbols':
kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
221 | sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
| ^
kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
221 | sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
| ^
kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
312 | ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
| ^~~~~~~
| |
| Elf32_Shdr * {aka struct elf32_shdr *}
kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
| ~~~~~~~~~~~~^~~~~~~
Fix it by using the right types instead of forcing 64 bits types.
Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
Signed-off-by: Christophe Leroy <[email protected]>
Acked-by: Petr Mladek <[email protected]>
---
kernel/livepatch/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..c0789383807b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
return -EINVAL;
}
-static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
+static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
unsigned int symndx, Elf_Shdr *relasec,
const char *sec_objname)
{
@@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
relas = (Elf_Rela *) relasec->sh_addr;
/* For each rela in this klp relocation section */
for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
- sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
+ sym = (Elf_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
if (sym->st_shndx != SHN_LIVEPATCH) {
pr_err("symbol %s is not marked as a livepatch symbol\n",
strtab + sym->st_name);
--
2.33.1
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
direct tramp.
Signed-off-by: Christophe Leroy <[email protected]>
---
kernel/trace/trace_selftest.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index afd937a46496..abcadbe933bb 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -784,9 +784,7 @@ static struct fgraph_ops fgraph_ops __initdata = {
.retfunc = &trace_graph_return,
};
-#if defined(CONFIG_DYNAMIC_FTRACE) && \
- defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
-#define TEST_DIRECT_TRAMP
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
noinline __noclone static void trace_direct_tramp(void) { }
#endif
@@ -849,7 +847,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
goto out;
}
-#ifdef TEST_DIRECT_TRAMP
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
tracing_reset_online_cpus(&tr->array_buffer);
set_graph_array(tr);
--
2.33.1
Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y.
R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
the types generated by the kpatch-build userspace tool or klp-convert
kernel tree observed applying a relocation to a post-init module.
Use patch_instruction() to patch those relocations.
Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
RO modules") did similar change in module_64.
Signed-off-by: Christophe Leroy <[email protected]>
Cc: Russell Currey <[email protected]>
---
arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index a491ad481d85..a0432ef46967 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -18,6 +18,7 @@
#include <linux/bug.h>
#include <linux/sort.h>
#include <asm/setup.h>
+#include <asm/code-patching.h>
/* Count how many different relocations (different symbol, different
addend) */
@@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
entry++;
}
- entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
- entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
- entry->jump[2] = PPC_RAW_MTCTR(_R12);
- entry->jump[3] = PPC_RAW_BCTR();
+ if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
+ return 0;
+ if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
+ return 0;
+ if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
+ return 0;
+ if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
+ return 0;
pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
return (uint32_t)entry;
}
+static int patch_location_16(uint32_t *loc, u16 value)
+{
+ loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
+ return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
+}
+
int apply_relocate_add(Elf32_Shdr *sechdrs,
const char *strtab,
unsigned int symindex,
@@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
case R_PPC_ADDR16_LO:
/* Low half of the symbol */
- *(uint16_t *)location = value;
+ if (patch_location_16(location, PPC_LO(value)))
+ return -EFAULT;
break;
case R_PPC_ADDR16_HI:
/* Higher half of the symbol */
- *(uint16_t *)location = (value >> 16);
+ if (patch_location_16(location, PPC_HI(value)))
+ return -EFAULT;
break;
case R_PPC_ADDR16_HA:
- /* Sign-adjusted lower 16 bits: PPC ELF ABI says:
- (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
- This is the same, only sane.
- */
- *(uint16_t *)location = (value + 0x8000) >> 16;
+ if (patch_location_16(location, PPC_HA(value)))
+ return -EFAULT;
break;
case R_PPC_REL24:
if ((int)(value - (uint32_t)location) < -0x02000000
- || (int)(value - (uint32_t)location) >= 0x02000000)
+ || (int)(value - (uint32_t)location) >= 0x02000000) {
value = do_plt_call(location, value,
sechdrs, module);
+ if (!value)
+ return -EFAULT;
+ }
/* Only replace bits 2 through 26 */
pr_debug("REL24 value = %08X. location = %08X\n",
value, (uint32_t)location);
pr_debug("Location before: %08X.\n",
*(uint32_t *)location);
- *(uint32_t *)location
- = (*(uint32_t *)location & ~0x03fffffc)
+ value = (*(uint32_t *)location & ~0x03fffffc)
| ((value - (uint32_t)location)
& 0x03fffffc);
+
+ if (patch_instruction(location, ppc_inst(value)))
+ return -EFAULT;
+
pr_debug("Location after: %08X.\n",
*(uint32_t *)location);
pr_debug("ie. jump to %08X+%08X = %08X\n",
--
2.33.1
PPC64 needs some special logic to properly set up the TOC.
See commit 85baa095497f ("powerpc/livepatch: Add live patching support
on ppc64le") for details.
PPC32 doesn't have TOC so it doesn't need that logic, so adding
LIVEPATCH support is straight forward.
Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.
Livepatch sample modules all work.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/Kconfig | 6 +++++-
arch/powerpc/include/asm/livepatch.h | 8 +++++---
arch/powerpc/include/asm/thread_info.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 2 +-
4 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 0631c9241af3..cdac2115eb00 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -9,6 +9,10 @@ config 64BIT
bool
default y if PPC64
+config LIVEPATCH_64
+ def_bool PPC64
+ depends on LIVEPATCH
+
config MMU
bool
default y
@@ -230,7 +234,7 @@ config PPC
select HAVE_KPROBES_ON_FTRACE
select HAVE_KRETPROBES
select HAVE_LD_DEAD_CODE_DATA_ELIMINATION
- select HAVE_LIVEPATCH if HAVE_DYNAMIC_FTRACE_WITH_REGS && PPC64
+ select HAVE_LIVEPATCH if HAVE_DYNAMIC_FTRACE_WITH_REGS
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
select HAVE_OPTPROBES
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 4fe018cc207b..37af961eb74c 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -23,12 +23,14 @@ static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
{
/*
- * Live patch works only with -mprofile-kernel on PPC. In this case,
- * the ftrace location is always within the first 16 bytes.
+ * Live patch works on PPC32 and only with -mprofile-kernel on PPC64. In
+ * both cases, the ftrace location is always within the first 16 bytes.
*/
return ftrace_location_range(faddr, faddr + 16);
}
+#endif /* CONFIG_LIVEPATCH */
+#ifdef CONFIG_LIVEPATCH_64
static inline void klp_init_thread_info(struct task_struct *p)
{
/* + 1 to account for STACK_END_MAGIC */
@@ -36,6 +38,6 @@ static inline void klp_init_thread_info(struct task_struct *p)
}
#else
static inline void klp_init_thread_info(struct task_struct *p) { }
-#endif /* CONFIG_LIVEPATCH */
+#endif
#endif /* _ASM_POWERPC_LIVEPATCH_H */
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 5725029aaa29..42f8a1f99036 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -51,7 +51,7 @@ struct thread_info {
unsigned int cpu;
#endif
unsigned long local_flags; /* private flags for thread */
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
unsigned long *livepatch_sp;
#endif
#if defined(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) && defined(CONFIG_PPC32)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 7582f3e3a330..eec536aef83a 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -94,7 +94,7 @@ int main(void)
OFFSET(TASK_CPU, task_struct, thread_info.cpu);
#endif
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
OFFSET(TI_livepatch_sp, thread_info, livepatch_sp);
#endif
--
2.33.1
PPC32 mcount() caller already saves LR on stack,
no need to save it again.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace_32.S | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 0a02c0cb12d9..7e2fd729116b 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -53,9 +53,6 @@ _GLOBAL(ftrace_stub)
blr
_GLOBAL(ftrace_regs_caller)
- /* Save the original return address in A's stack frame */
- stw r0,LRSAVE(r1)
-
/* Create our stack frame + pt_regs */
stwu r1,-INT_FRAME_SIZE(r1)
--
2.33.1
return_to_handler() was copied from PPC64. For PPC32 it
just needs to save r3 and r4, and doesn't require any nop
after the bl.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace_32.S | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 7e2fd729116b..95ffea2bdc29 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -162,22 +162,18 @@ _GLOBAL(ftrace_graph_caller)
_GLOBAL(return_to_handler)
/* need to save return values */
- stwu r1, -32(r1)
- stw r3, 20(r1)
- stw r4, 16(r1)
- stw r31, 12(r1)
- mr r31, r1
+ stwu r1, -16(r1)
+ stw r3, 8(r1)
+ stw r4, 12(r1)
bl ftrace_return_to_handler
- nop
/* return value has real return address */
mtlr r3
- lwz r3, 20(r1)
- lwz r4, 16(r1)
- lwz r31,12(r1)
- lwz r1, 0(r1)
+ lwz r3, 8(r1)
+ lwz r4, 12(r1)
+ addi r1, r1, 16
/* Jump back to real return address */
blr
--
2.33.1
In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change
ftrace_caller() stack layout to match struct pt_regs.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/ftrace.h | 39 +--------------------------
arch/powerpc/kernel/trace/ftrace_32.S | 29 +++++++++++++++++---
2 files changed, 26 insertions(+), 42 deletions(-)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index debe8c4f7062..b3f6184f77ea 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -10,44 +10,7 @@
#define HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
-#ifdef __ASSEMBLY__
-
-/* Based off of objdump output from glibc */
-
-#define MCOUNT_SAVE_FRAME \
- stwu r1,-48(r1); \
- stw r3, 12(r1); \
- stw r4, 16(r1); \
- stw r5, 20(r1); \
- stw r6, 24(r1); \
- mflr r3; \
- lwz r4, 52(r1); \
- mfcr r5; \
- stw r7, 28(r1); \
- stw r8, 32(r1); \
- stw r9, 36(r1); \
- stw r10,40(r1); \
- stw r3, 44(r1); \
- stw r5, 8(r1)
-
-#define MCOUNT_RESTORE_FRAME \
- lwz r6, 8(r1); \
- lwz r0, 44(r1); \
- lwz r3, 12(r1); \
- mtctr r0; \
- lwz r4, 16(r1); \
- mtcr r6; \
- lwz r5, 20(r1); \
- lwz r6, 24(r1); \
- lwz r0, 52(r1); \
- lwz r7, 28(r1); \
- lwz r8, 32(r1); \
- mtlr r0; \
- lwz r9, 36(r1); \
- lwz r10,40(r1); \
- addi r1, r1, 48
-
-#else /* !__ASSEMBLY__ */
+#ifndef __ASSEMBLY__
extern void _mcount(void);
static inline unsigned long ftrace_call_adjust(unsigned long addr)
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 95ffea2bdc29..c4055b41af5f 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -27,17 +27,38 @@ _GLOBAL(_mcount)
EXPORT_SYMBOL(_mcount)
_GLOBAL(ftrace_caller)
- MCOUNT_SAVE_FRAME
- /* r3 ends up with link register */
+ stwu r1, -INT_FRAME_SIZE(r1)
+
+ SAVE_GPRS(3, 10, r1)
+
+ addi r8, r1, INT_FRAME_SIZE
+ stw r8, GPR1(r1)
+
+ mflr r3
+ stw r3, _NIP(r1)
subi r3, r3, MCOUNT_INSN_SIZE
+
+ stw r0, _LINK(r1)
+ mr r4, r0
+
lis r5,function_trace_op@ha
lwz r5,function_trace_op@l(r5)
- li r6, 0
+
+ addi r6, r1, STACK_FRAME_OVERHEAD
.globl ftrace_call
ftrace_call:
bl ftrace_stub
nop
- MCOUNT_RESTORE_FRAME
+
+ lwz r3, _NIP(r1)
+ mtctr r3
+
+ REST_GPRS(3, 10, r1)
+
+ lwz r0, _LINK(r1)
+ mtlr r0
+
+ addi r1, r1, INT_FRAME_SIZE
ftrace_caller_common:
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
.globl ftrace_graph_call
--
2.33.1
In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
to handle LIVEPATCH the same way as frace_caller_regs().
Signed-off-by: Christophe Leroy <[email protected]>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index d636fc755f60..f6f787819273 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -172,14 +172,19 @@ _GLOBAL(ftrace_caller)
addi r3, r3, function_trace_op@toc@l
ld r5, 0(r3)
+#ifdef CONFIG_LIVEPATCH_64
+ SAVE_GPR(14, r1)
+ mr r14,r7 /* remember old NIP */
+#endif
/* Calculate ip from nip-4 into r3 for call below */
subi r3, r7, MCOUNT_INSN_SIZE
/* Put the original return address in r4 as parent_ip */
+ std r0, _LINK(r1)
mr r4, r0
- /* Set pt_regs to NULL */
- li r6, 0
+ /* Load &pt_regs in r6 for call below */
+ addi r6, r1 ,STACK_FRAME_OVERHEAD
/* ftrace_call(r3, r4, r5, r6) */
.globl ftrace_call
@@ -189,6 +194,10 @@ ftrace_call:
ld r3, _NIP(r1)
mtctr r3
+#ifdef CONFIG_LIVEPATCH_64
+ cmpd r14, r3 /* has NIP been altered? */
+ REST_GPR(14, r1)
+#endif
/* Restore gprs */
REST_GPRS(3, 10, r1)
@@ -196,13 +205,17 @@ ftrace_call:
/* Restore callee's TOC */
ld r2, 24(r1)
+ /* Restore possibly modified LR */
+ ld r0, _LINK(r1)
+ mtlr r0
+
/* Pop our stack frame */
addi r1, r1, SWITCH_FRAME_SIZE
- /* Reload original LR */
- ld r0, LRSAVE(r1)
- mtlr r0
-
+#ifdef CONFIG_LIVEPATCH_64
+ /* Based on the cmpd above, if the NIP was altered handle livepatch */
+ bne- livepatch_handler
+#endif
/* Handle function_graph or go back */
b ftrace_caller_common
--
2.33.1
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
select HAVE_EBPF_JIT
select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
struct dyn_arch_ftrace {
struct module *mod;
};
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+ struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+ return &fregs->regs;
+}
+
+static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
+ unsigned long ip)
+{
+ regs_set_return_ip(&fregs->regs, ip);
+}
+#endif
#endif /* __ASSEMBLY__ */
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 37af961eb74c..6f10de6af6e3 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -14,9 +14,7 @@
#ifdef CONFIG_LIVEPATCH
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
{
- struct pt_regs *regs = ftrace_get_regs(fregs);
-
- regs_set_return_ip(regs, ip);
+ ftrace_instruction_pointer_set(fregs, ip);
}
#define klp_get_ftrace_location klp_get_ftrace_location
--
2.33.1
ftrace_enable_ftrace_graph_caller() and
ftrace_disable_ftrace_graph_caller() have common code.
They will have even more common code after following patch.
Refactor into a single ftrace_modify_ftrace_graph_caller() function.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 80b6285769f2..ce673764cb69 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -910,30 +910,27 @@ int __init ftrace_dyn_arch_init(void)
extern void ftrace_graph_call(void);
extern void ftrace_graph_stub(void);
-int ftrace_enable_ftrace_graph_caller(void)
+static int ftrace_modify_ftrace_graph_caller(bool enable)
{
unsigned long ip = (unsigned long)(&ftrace_graph_call);
unsigned long addr = (unsigned long)(&ftrace_graph_caller);
unsigned long stub = (unsigned long)(&ftrace_graph_stub);
ppc_inst_t old, new;
- old = ftrace_call_replace(ip, stub, 0);
- new = ftrace_call_replace(ip, addr, 0);
+ old = ftrace_call_replace(ip, enable ? stub : addr, 0);
+ new = ftrace_call_replace(ip, enable ? addr : stub, 0);
return ftrace_modify_code(ip, old, new);
}
-int ftrace_disable_ftrace_graph_caller(void)
+int ftrace_enable_ftrace_graph_caller(void)
{
- unsigned long ip = (unsigned long)(&ftrace_graph_call);
- unsigned long addr = (unsigned long)(&ftrace_graph_caller);
- unsigned long stub = (unsigned long)(&ftrace_graph_stub);
- ppc_inst_t old, new;
-
- old = ftrace_call_replace(ip, addr, 0);
- new = ftrace_call_replace(ip, stub, 0);
+ return ftrace_modify_ftrace_graph_caller(true);
+}
- return ftrace_modify_code(ip, old, new);
+int ftrace_disable_ftrace_graph_caller(void)
+{
+ return ftrace_modify_ftrace_graph_caller(false);
}
/*
--
2.33.1
Modify function graph tracer to be handled directly by the standard
ftrace caller.
This is made possible as powerpc now supports
CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
This change simplifies the call of function graph ftrace.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/ftrace.h | 6 ++
arch/powerpc/kernel/trace/ftrace.c | 11 ++++
arch/powerpc/kernel/trace/ftrace_32.S | 53 +--------------
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
4 files changed, 20 insertions(+), 114 deletions(-)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index 45c3d6f11daa..70b457097098 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
{
regs_set_return_ip(&fregs->regs, ip);
}
+
+struct ftrace_ops;
+
+#define ftrace_graph_func ftrace_graph_func
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *op, struct ftrace_regs *fregs);
#endif
#endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index ce673764cb69..74a176e394ef 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
unsigned long stub = (unsigned long)(&ftrace_graph_stub);
ppc_inst_t old, new;
+ if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
+ return 0;
+
old = ftrace_call_replace(ip, enable ? stub : addr, 0);
new = ftrace_call_replace(ip, enable ? addr : stub, 0);
@@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
out:
return parent;
}
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+ fregs->regs.link = prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]);
+}
+#endif
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
#ifdef PPC64_ELF_ABI_v1
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index c4055b41af5f..2b425da97a6b 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -59,13 +59,6 @@ ftrace_call:
mtlr r0
addi r1, r1, INT_FRAME_SIZE
-ftrace_caller_common:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
- b ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
/* old link register ends up in ctr reg */
bctr
@@ -135,52 +128,10 @@ ftrace_regs_call:
/* Pop our stack frame */
addi r1, r1, INT_FRAME_SIZE
-
- b ftrace_caller_common
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
- stwu r1,-48(r1)
- stw r3, 12(r1)
- stw r4, 16(r1)
- stw r5, 20(r1)
- stw r6, 24(r1)
- stw r7, 28(r1)
- stw r8, 32(r1)
- stw r9, 36(r1)
- stw r10,40(r1)
-
- addi r5, r1, 48
- mfctr r4 /* ftrace_caller has moved local addr here */
- stw r4, 44(r1)
- mflr r3 /* ftrace_caller has restored LR from stack */
- subi r4, r4, MCOUNT_INSN_SIZE
-
- bl prepare_ftrace_return
- nop
-
- /*
- * prepare_ftrace_return gives us the address we divert to.
- * Change the LR in the callers stack frame to this.
- */
- stw r3,52(r1)
- mtlr r3
- lwz r0,44(r1)
- mtctr r0
-
- lwz r3, 12(r1)
- lwz r4, 16(r1)
- lwz r5, 20(r1)
- lwz r6, 24(r1)
- lwz r7, 28(r1)
- lwz r8, 32(r1)
- lwz r9, 36(r1)
- lwz r10,40(r1)
-
- addi r1, r1, 48
-
+ /* old link register ends up in ctr reg */
bctr
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
_GLOBAL(return_to_handler)
/* need to save return values */
stwu r1, -16(r1)
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index f6f787819273..6071e0122797 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -124,15 +124,6 @@ ftrace_regs_call:
/* Based on the cmpd above, if the NIP was altered handle livepatch */
bne- livepatch_handler
#endif
-
-ftrace_caller_common:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
- b ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
-
bctr /* jump after _mcount site */
_GLOBAL(ftrace_stub)
@@ -216,8 +207,7 @@ ftrace_call:
/* Based on the cmpd above, if the NIP was altered handle livepatch */
bne- livepatch_handler
#endif
- /* Handle function_graph or go back */
- b ftrace_caller_common
+ bctr /* jump after _mcount site */
#ifdef CONFIG_LIVEPATCH
/*
@@ -286,55 +276,3 @@ livepatch_handler:
/* Return to original caller of live patched function */
blr
#endif /* CONFIG_LIVEPATCH */
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
- stdu r1, -112(r1)
- /* with -mprofile-kernel, parameter regs are still alive at _mcount */
- std r10, 104(r1)
- std r9, 96(r1)
- std r8, 88(r1)
- std r7, 80(r1)
- std r6, 72(r1)
- std r5, 64(r1)
- std r4, 56(r1)
- std r3, 48(r1)
-
- /* Save callee's TOC in the ABI compliant location */
- std r2, 24(r1)
- ld r2, PACATOC(r13) /* get kernel TOC in r2 */
-
- addi r5, r1, 112
- mfctr r4 /* ftrace_caller has moved local addr here */
- std r4, 40(r1)
- mflr r3 /* ftrace_caller has restored LR from stack */
- subi r4, r4, MCOUNT_INSN_SIZE
-
- bl prepare_ftrace_return
- nop
-
- /*
- * prepare_ftrace_return gives us the address we divert to.
- * Change the LR to this.
- */
- mtlr r3
-
- ld r0, 40(r1)
- mtctr r0
- ld r10, 104(r1)
- ld r9, 96(r1)
- ld r8, 88(r1)
- ld r7, 80(r1)
- ld r6, 72(r1)
- ld r5, 64(r1)
- ld r4, 56(r1)
- ld r3, 48(r1)
-
- /* Restore callee's TOC */
- ld r2, 24(r1)
-
- addi r1, r1, 112
- mflr r0
- std r0, LRSAVE(r1)
- bctr
-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
--
2.33.1
PPC64 mprofile versions and PPC32 are very similar.
Modify PPC64 version so that if can be reused for PPC32.
Signed-off-by: Christophe Leroy <[email protected]>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
1 file changed, 51 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index 6071e0122797..56da60e98327 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -34,13 +34,16 @@
*/
_GLOBAL(ftrace_regs_caller)
/* Save the original return address in A's stack frame */
- std r0,LRSAVE(r1)
+#ifdef CONFIG_MPROFILE_KERNEL
+ PPC_STL r0,LRSAVE(r1)
+#endif
/* Create our stack frame + pt_regs */
- stdu r1,-SWITCH_FRAME_SIZE(r1)
+ PPC_STLU r1,-SWITCH_FRAME_SIZE(r1)
/* Save all gprs to pt_regs */
SAVE_GPR(0, r1)
+#ifdef CONFIG_PPC64
SAVE_GPRS(2, 11, r1)
/* Ok to continue? */
@@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
beq ftrace_no_trace
SAVE_GPRS(12, 31, r1)
+#else
+ stmw r2, GPR2(r1)
+#endif
/* Save previous stack pointer (r1) */
addi r8, r1, SWITCH_FRAME_SIZE
- std r8, GPR1(r1)
+ PPC_STL r8, GPR1(r1)
/* Load special regs for save below */
mfmsr r8
@@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
/* Get the _mcount() call site out of LR */
mflr r7
/* Save it as pt_regs->nip */
- std r7, _NIP(r1)
+ PPC_STL r7, _NIP(r1)
/* Save the read LR in pt_regs->link */
- std r0, _LINK(r1)
+ PPC_STL r0, _LINK(r1)
+#ifdef CONFIG_PPC64
/* Save callee's TOC in the ABI compliant location */
std r2, 24(r1)
ld r2,PACATOC(r13) /* get kernel TOC in r2 */
@@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
addis r3,r2,function_trace_op@toc@ha
addi r3,r3,function_trace_op@toc@l
ld r5,0(r3)
+#else
+ lis r3,function_trace_op@ha
+ lwz r5,function_trace_op@l(r3)
+#endif
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
mr r14,r7 /* remember old NIP */
#endif
/* Calculate ip from nip-4 into r3 for call below */
@@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
mr r4, r0
/* Save special regs */
- std r8, _MSR(r1)
- std r9, _CTR(r1)
- std r10, _XER(r1)
- std r11, _CCR(r1)
+ PPC_STL r8, _MSR(r1)
+ PPC_STL r9, _CTR(r1)
+ PPC_STL r10, _XER(r1)
+ PPC_STL r11, _CCR(r1)
/* Load &pt_regs in r6 for call below */
addi r6, r1 ,STACK_FRAME_OVERHEAD
@@ -100,27 +111,32 @@ ftrace_regs_call:
nop
/* Load ctr with the possibly modified NIP */
- ld r3, _NIP(r1)
+ PPC_LL r3, _NIP(r1)
mtctr r3
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
cmpd r14, r3 /* has NIP been altered? */
#endif
/* Restore gprs */
- REST_GPR(0, r1)
+#ifdef CONFIG_PPC64
REST_GPRS(2, 31, r1)
+#else
+ lmw r2, GPR2(r1)
+#endif
/* Restore possibly modified LR */
- ld r0, _LINK(r1)
+ PPC_LL r0, _LINK(r1)
mtlr r0
+#ifdef CONFIG_PPC64
/* Restore callee's TOC */
ld r2, 24(r1)
+#endif
/* Pop our stack frame */
addi r1, r1, SWITCH_FRAME_SIZE
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
/* Based on the cmpd above, if the NIP was altered handle livepatch */
bne- livepatch_handler
#endif
@@ -129,6 +145,7 @@ ftrace_regs_call:
_GLOBAL(ftrace_stub)
blr
+#ifdef CONFIG_PPC64
ftrace_no_trace:
mflr r3
mtctr r3
@@ -136,25 +153,31 @@ ftrace_no_trace:
addi r1, r1, SWITCH_FRAME_SIZE
mtlr r0
bctr
+#endif
_GLOBAL(ftrace_caller)
/* Save the original return address in A's stack frame */
- std r0, LRSAVE(r1)
+#ifdef CONFIG_MPROFILE_KERNEL
+ PPC_STL r0, LRSAVE(r1)
+#endif
/* Create our stack frame + pt_regs */
- stdu r1, -SWITCH_FRAME_SIZE(r1)
+ PPC_STLU r1, -SWITCH_FRAME_SIZE(r1)
/* Save all gprs to pt_regs */
SAVE_GPRS(3, 10, r1)
+#ifdef CONFIG_PPC64
lbz r3, PACA_FTRACE_ENABLED(r13)
cmpdi r3, 0
beq ftrace_no_trace
+#endif
/* Get the _mcount() call site out of LR */
mflr r7
- std r7, _NIP(r1)
+ PPC_STL r7, _NIP(r1)
+#ifdef CONFIG_PPC64
/* Save callee's TOC in the ABI compliant location */
std r2, 24(r1)
ld r2, PACATOC(r13) /* get kernel TOC in r2 */
@@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
addis r3, r2, function_trace_op@toc@ha
addi r3, r3, function_trace_op@toc@l
ld r5, 0(r3)
+#else
+ lis r3,function_trace_op@ha
+ lwz r5,function_trace_op@l(r3)
+#endif
#ifdef CONFIG_LIVEPATCH_64
SAVE_GPR(14, r1)
@@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
subi r3, r7, MCOUNT_INSN_SIZE
/* Put the original return address in r4 as parent_ip */
- std r0, _LINK(r1)
+ PPC_STL r0, _LINK(r1)
mr r4, r0
/* Load &pt_regs in r6 for call below */
@@ -183,7 +210,7 @@ ftrace_call:
bl ftrace_stub
nop
- ld r3, _NIP(r1)
+ PPC_LL r3, _NIP(r1)
mtctr r3
#ifdef CONFIG_LIVEPATCH_64
cmpd r14, r3 /* has NIP been altered? */
@@ -193,11 +220,13 @@ ftrace_call:
/* Restore gprs */
REST_GPRS(3, 10, r1)
+#ifdef CONFIG_PPC64
/* Restore callee's TOC */
ld r2, 24(r1)
+#endif
/* Restore possibly modified LR */
- ld r0, _LINK(r1)
+ PPC_LL r0, _LINK(r1)
mtlr r0
/* Pop our stack frame */
@@ -209,7 +238,7 @@ ftrace_call:
#endif
bctr /* jump after _mcount site */
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
/*
* This function runs in the mcount context, between two functions. As
* such it can only clobber registers which are volatile and used in
--
2.33.1
Functions in ftrace_32.S are common with PPC64.
Reuse the ones defined for PPC64 with slight modification
when required.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/Makefile | 7 +-
arch/powerpc/kernel/trace/ftrace_32.S | 152 ------------------
.../trace/{ftrace_64.S => ftrace_low.S} | 14 ++
...ftrace_64_mprofile.S => ftrace_mprofile.S} | 0
4 files changed, 17 insertions(+), 156 deletions(-)
delete mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
rename arch/powerpc/kernel/trace/{ftrace_64.S => ftrace_low.S} (85%)
rename arch/powerpc/kernel/trace/{ftrace_64_mprofile.S => ftrace_mprofile.S} (100%)
diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index 858503775c58..ac7d42a4e8d0 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -8,15 +8,14 @@ ifdef CONFIG_FUNCTION_TRACER
CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
endif
-obj32-$(CONFIG_FUNCTION_TRACER) += ftrace_32.o
-obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_64.o
+obj32-$(CONFIG_FUNCTION_TRACER) += ftrace_mprofile.o
ifdef CONFIG_MPROFILE_KERNEL
-obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_64_mprofile.o
+obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_mprofile.o
else
obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_64_pg.o
endif
obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o ftrace_low.o
obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
obj-$(CONFIG_TRACING) += trace_clock.o
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
deleted file mode 100644
index 2b425da97a6b..000000000000
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ /dev/null
@@ -1,152 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Split from entry_32.S
- */
-
-#include <linux/magic.h>
-#include <asm/reg.h>
-#include <asm/ppc_asm.h>
-#include <asm/asm-offsets.h>
-#include <asm/ftrace.h>
-#include <asm/export.h>
-#include <asm/ptrace.h>
-
-_GLOBAL(mcount)
-_GLOBAL(_mcount)
- /*
- * It is required that _mcount on PPC32 must preserve the
- * link register. But we have r12 to play with. We use r12
- * to push the return address back to the caller of mcount
- * into the ctr register, restore the link register and
- * then jump back using the ctr register.
- */
- mflr r12
- mtctr r12
- mtlr r0
- bctr
-EXPORT_SYMBOL(_mcount)
-
-_GLOBAL(ftrace_caller)
- stwu r1, -INT_FRAME_SIZE(r1)
-
- SAVE_GPRS(3, 10, r1)
-
- addi r8, r1, INT_FRAME_SIZE
- stw r8, GPR1(r1)
-
- mflr r3
- stw r3, _NIP(r1)
- subi r3, r3, MCOUNT_INSN_SIZE
-
- stw r0, _LINK(r1)
- mr r4, r0
-
- lis r5,function_trace_op@ha
- lwz r5,function_trace_op@l(r5)
-
- addi r6, r1, STACK_FRAME_OVERHEAD
-.globl ftrace_call
-ftrace_call:
- bl ftrace_stub
- nop
-
- lwz r3, _NIP(r1)
- mtctr r3
-
- REST_GPRS(3, 10, r1)
-
- lwz r0, _LINK(r1)
- mtlr r0
-
- addi r1, r1, INT_FRAME_SIZE
- /* old link register ends up in ctr reg */
- bctr
-
-
-_GLOBAL(ftrace_stub)
- blr
-
-_GLOBAL(ftrace_regs_caller)
- /* Create our stack frame + pt_regs */
- stwu r1,-INT_FRAME_SIZE(r1)
-
- /* Save all gprs to pt_regs */
- stw r0, GPR0(r1)
- stmw r2, GPR2(r1)
-
- /* Save previous stack pointer (r1) */
- addi r8, r1, INT_FRAME_SIZE
- stw r8, GPR1(r1)
-
- /* Load special regs for save below */
- mfmsr r8
- mfctr r9
- mfxer r10
- mfcr r11
-
- /* Get the _mcount() call site out of LR */
- mflr r7
- /* Save it as pt_regs->nip */
- stw r7, _NIP(r1)
- /* Save the read LR in pt_regs->link */
- stw r0, _LINK(r1)
-
- lis r3,function_trace_op@ha
- lwz r5,function_trace_op@l(r3)
-
- /* Calculate ip from nip-4 into r3 for call below */
- subi r3, r7, MCOUNT_INSN_SIZE
-
- /* Put the original return address in r4 as parent_ip */
- mr r4, r0
-
- /* Save special regs */
- stw r8, _MSR(r1)
- stw r9, _CTR(r1)
- stw r10, _XER(r1)
- stw r11, _CCR(r1)
-
- /* Load &pt_regs in r6 for call below */
- addi r6, r1, STACK_FRAME_OVERHEAD
-
- /* ftrace_call(r3, r4, r5, r6) */
-.globl ftrace_regs_call
-ftrace_regs_call:
- bl ftrace_stub
- nop
-
- /* Load ctr with the possibly modified NIP */
- lwz r3, _NIP(r1)
- mtctr r3
-
- /* Restore gprs */
- lmw r2, GPR2(r1)
-
- /* Restore possibly modified LR */
- lwz r0, _LINK(r1)
- mtlr r0
-
- /* Pop our stack frame */
- addi r1, r1, INT_FRAME_SIZE
- /* old link register ends up in ctr reg */
- bctr
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(return_to_handler)
- /* need to save return values */
- stwu r1, -16(r1)
- stw r3, 8(r1)
- stw r4, 12(r1)
-
- bl ftrace_return_to_handler
-
- /* return value has real return address */
- mtlr r3
-
- lwz r3, 8(r1)
- lwz r4, 12(r1)
- addi r1, r1, 16
-
- /* Jump back to real return address */
- blr
-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/powerpc/kernel/trace/ftrace_64.S b/arch/powerpc/kernel/trace/ftrace_low.S
similarity index 85%
rename from arch/powerpc/kernel/trace/ftrace_64.S
rename to arch/powerpc/kernel/trace/ftrace_low.S
index 25e5b9e47c06..0bddf1fa6636 100644
--- a/arch/powerpc/kernel/trace/ftrace_64.S
+++ b/arch/powerpc/kernel/trace/ftrace_low.S
@@ -10,6 +10,7 @@
#include <asm/ppc-opcode.h>
#include <asm/export.h>
+#ifdef CONFIG_PPC64
.pushsection ".tramp.ftrace.text","aw",@progbits;
.globl ftrace_tramp_text
ftrace_tramp_text:
@@ -21,6 +22,7 @@ ftrace_tramp_text:
ftrace_tramp_init:
.space 64
.popsection
+#endif
_GLOBAL(mcount)
_GLOBAL(_mcount)
@@ -33,6 +35,7 @@ EXPORT_SYMBOL(_mcount)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
_GLOBAL(return_to_handler)
/* need to save return values */
+#ifdef CONFIG_PPC64
std r4, -32(r1)
std r3, -24(r1)
/* save TOC */
@@ -46,6 +49,11 @@ _GLOBAL(return_to_handler)
* Switch to our TOC to run inside the core kernel.
*/
ld r2, PACATOC(r13)
+#else
+ stwu r1, -16(r1)
+ stw r3, 8(r1)
+ stw r4, 12(r1)
+#endif
bl ftrace_return_to_handler
nop
@@ -53,11 +61,17 @@ _GLOBAL(return_to_handler)
/* return value has real return address */
mtlr r3
+#ifdef CONFIG_PPC64
ld r1, 0(r1)
ld r4, -32(r1)
ld r3, -24(r1)
ld r2, -16(r1)
ld r31, -8(r1)
+#else
+ lwz r3, 8(r1)
+ lwz r4, 12(r1)
+ addi r1, r1, 16
+#endif
/* Jump back to real return address */
blr
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_mprofile.S
similarity index 100%
rename from arch/powerpc/kernel/trace/ftrace_64_mprofile.S
rename to arch/powerpc/kernel/trace/ftrace_mprofile.S
--
2.33.1
On Mon, 20 Dec 2021, Christophe Leroy wrote:
> Trying to build livepatch on powerpc/32 results in:
>
> kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 221 | sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> | ^
> kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 221 | sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> | ^
> kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 312 | ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> | ^~~~~~~
> | |
> | Elf32_Shdr * {aka struct elf32_shdr *}
> kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> | ~~~~~~~~~~~~^~~~~~~
>
> Fix it by using the right types instead of forcing 64 bits types.
>
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <[email protected]>
> Acked-by: Petr Mladek <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
M
On Mon, 20 Dec 2021, Christophe Leroy wrote:
> PPC64 needs some special logic to properly set up the TOC.
> See commit 85baa095497f ("powerpc/livepatch: Add live patching support
> on ppc64le") for details.
>
> PPC32 doesn't have TOC so it doesn't need that logic, so adding
> LIVEPATCH support is straight forward.
>
> Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.
>
> Livepatch sample modules all work.
Great.
> Signed-off-by: Christophe Leroy <[email protected]>
FWIW the patch looks good to me.
Miroslav
On Mon, 20 Dec 2021, Christophe Leroy wrote:
> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
>
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.
Correct. We could replace it ftrace_instruction_pointer_set() and that is
it. In fact, livepatch.h in both arch/x86/include/asm/ and
arch/s390/include/asm/ could be removed with that.
On the other hand, there is arm64 live patching support being worked on
and I am not sure what their plans about DYNAMIC_FTRACE_WITH_ARGS are. The
above would make it a prerequisite.
Adding CCs... you can find the whole thread at
https://lore.kernel.org/all/[email protected]/
Miroslav
On Mon, Dec 20, 2021 at 04:38:02PM +0000, Christophe Leroy wrote:
> Trying to build livepatch on powerpc/32 results in:
>
> kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 221 | sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> | ^
> kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 221 | sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> | ^
> kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 312 | ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> | ^~~~~~~
> | |
> | Elf32_Shdr * {aka struct elf32_shdr *}
> kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> | ~~~~~~~~~~~~^~~~~~~
>
> Fix it by using the right types instead of forcing 64 bits types.
>
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <[email protected]>
> Acked-by: Petr Mladek <[email protected]>
> ---
> kernel/livepatch/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..c0789383807b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> return -EINVAL;
> }
>
> -static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> +static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
> unsigned int symndx, Elf_Shdr *relasec,
> const char *sec_objname)
> {
> @@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> relas = (Elf_Rela *) relasec->sh_addr;
> /* For each rela in this klp relocation section */
> for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> - sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> + sym = (Elf_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> if (sym->st_shndx != SHN_LIVEPATCH) {
> pr_err("symbol %s is not marked as a livepatch symbol\n",
> strtab + sym->st_name);
> --
> 2.33.1
>
Thanks for finding and fixing, lgtm.
Acked-by: Joe Lawrence <[email protected]>
-- Joe
On Mon, Dec 20, 2021 at 04:38:09PM +0000, Christophe Leroy wrote:
> Livepatching a loaded module involves applying relocations through
> apply_relocate_add(), which attempts to write to read-only memory when
> CONFIG_STRICT_MODULE_RWX=y.
>
> R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
> the types generated by the kpatch-build userspace tool or klp-convert
> kernel tree observed applying a relocation to a post-init module.
>
> Use patch_instruction() to patch those relocations.
>
> Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
> RO modules") did similar change in module_64.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> Cc: Russell Currey <[email protected]>
> ---
> arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
> index a491ad481d85..a0432ef46967 100644
> --- a/arch/powerpc/kernel/module_32.c
> +++ b/arch/powerpc/kernel/module_32.c
> @@ -18,6 +18,7 @@
> #include <linux/bug.h>
> #include <linux/sort.h>
> #include <asm/setup.h>
> +#include <asm/code-patching.h>
>
> /* Count how many different relocations (different symbol, different
> addend) */
> @@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
> entry++;
> }
>
> - entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
> - entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
> - entry->jump[2] = PPC_RAW_MTCTR(_R12);
> - entry->jump[3] = PPC_RAW_BCTR();
> + if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
> + return 0;
> + if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
> + return 0;
> + if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
> + return 0;
> + if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
> + return 0;
>
> pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
> return (uint32_t)entry;
> }
>
> +static int patch_location_16(uint32_t *loc, u16 value)
> +{
> + loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
> + return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
> +}
> +
> int apply_relocate_add(Elf32_Shdr *sechdrs,
> const char *strtab,
> unsigned int symindex,
> @@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
>
> case R_PPC_ADDR16_LO:
> /* Low half of the symbol */
> - *(uint16_t *)location = value;
> + if (patch_location_16(location, PPC_LO(value)))
> + return -EFAULT;
> break;
>
> case R_PPC_ADDR16_HI:
> /* Higher half of the symbol */
> - *(uint16_t *)location = (value >> 16);
> + if (patch_location_16(location, PPC_HI(value)))
> + return -EFAULT;
> break;
>
> case R_PPC_ADDR16_HA:
> - /* Sign-adjusted lower 16 bits: PPC ELF ABI says:
> - (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
> - This is the same, only sane.
> - */
> - *(uint16_t *)location = (value + 0x8000) >> 16;
> + if (patch_location_16(location, PPC_HA(value)))
> + return -EFAULT;
> break;
>
> case R_PPC_REL24:
> if ((int)(value - (uint32_t)location) < -0x02000000
> - || (int)(value - (uint32_t)location) >= 0x02000000)
> + || (int)(value - (uint32_t)location) >= 0x02000000) {
> value = do_plt_call(location, value,
> sechdrs, module);
> + if (!value)
> + return -EFAULT;
> + }
>
> /* Only replace bits 2 through 26 */
> pr_debug("REL24 value = %08X. location = %08X\n",
> value, (uint32_t)location);
> pr_debug("Location before: %08X.\n",
> *(uint32_t *)location);
> - *(uint32_t *)location
> - = (*(uint32_t *)location & ~0x03fffffc)
> + value = (*(uint32_t *)location & ~0x03fffffc)
> | ((value - (uint32_t)location)
> & 0x03fffffc);
> +
> + if (patch_instruction(location, ppc_inst(value)))
> + return -EFAULT;
> +
> pr_debug("Location after: %08X.\n",
> *(uint32_t *)location);
> pr_debug("ie. jump to %08X+%08X = %08X\n",
> --
> 2.33.1
>
IIRC, offlist we hacked up klp-convert to create the klp-relocations for
a 32-bit target and then you hit the selftest late relocation crash, so
I assume that part is happy after this fix. :) Thanks again for the
testing.
For the livepatching implications,
Acked-by: Joe Lawrence <[email protected]>
-- Joe
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index ac7d42a4e8d0..542aa7a8b2b4 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -14,8 +14,9 @@ obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_mprofile.o
else
obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_64_pg.o
endif
+obj-$(CONFIG_FUNCTION_TRACER) += ftrace_low.o
obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o ftrace_low.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
obj-$(CONFIG_TRACING) += trace_clock.o
--
2.34.1
Christophe Leroy wrote:
> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
>
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
> arch/powerpc/include/asm/livepatch.h | 4 +---
> 3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index cdac2115eb00..e2b1792b2aae 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -210,6 +210,7 @@ config PPC
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DEBUG_STACKOVERFLOW
> select HAVE_DYNAMIC_FTRACE
> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
> select HAVE_EBPF_JIT
> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index b3f6184f77ea..45c3d6f11daa 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
> struct dyn_arch_ftrace {
> struct module *mod;
> };
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> +struct ftrace_regs {
> + struct pt_regs regs;
> +};
> +
> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
> +{
> + return &fregs->regs;
> +}
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
> +
> +static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
> + unsigned long ip)
> +{
> + regs_set_return_ip(&fregs->regs, ip);
Should we use that helper here? regs_set_return_ip() also updates some
other state related to taking interrupts and I don't think it makes
sense for use with ftrace.
- Naveen
Christophe Leroy wrote:
> Modify function graph tracer to be handled directly by the standard
> ftrace caller.
>
> This is made possible as powerpc now supports
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
>
> This change simplifies the call of function graph ftrace.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/include/asm/ftrace.h | 6 ++
> arch/powerpc/kernel/trace/ftrace.c | 11 ++++
> arch/powerpc/kernel/trace/ftrace_32.S | 53 +--------------
> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
> 4 files changed, 20 insertions(+), 114 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index 45c3d6f11daa..70b457097098 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
> {
> regs_set_return_ip(&fregs->regs, ip);
> }
> +
> +struct ftrace_ops;
> +
> +#define ftrace_graph_func ftrace_graph_func
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *op, struct ftrace_regs *fregs);
> #endif
> #endif /* __ASSEMBLY__ */
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index ce673764cb69..74a176e394ef 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
> unsigned long stub = (unsigned long)(&ftrace_graph_stub);
> ppc_inst_t old, new;
>
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
> + return 0;
> +
> old = ftrace_call_replace(ip, enable ? stub : addr, 0);
> new = ftrace_call_replace(ip, enable ? addr : stub, 0);
>
> @@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
> out:
> return parent;
> }
For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use
ftrace directly") also adds recursion check before the call to
function_graph_enter() in prepare_ftrace_return(). Do we need that on
powerpc as well?
- Naveen
Hi Christophe,
Thanks for your work enabling DYNAMIC_FTRACE_WITH_ARGS on powerpc. Sorry
for the late review on this series, but I have a few comments below.
Christophe Leroy wrote:
> In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
> to handle LIVEPATCH the same way as frace_caller_regs().
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
> 1 file changed, 19 insertions(+), 6 deletions(-)
I think we also need to save r1 into pt_regs so that the stack pointer
is available in the callbacks.
Other than that, a few minor nits below...
>
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index d636fc755f60..f6f787819273 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -172,14 +172,19 @@ _GLOBAL(ftrace_caller)
> addi r3, r3, function_trace_op@toc@l
> ld r5, 0(r3)
>
> +#ifdef CONFIG_LIVEPATCH_64
> + SAVE_GPR(14, r1)
> + mr r14,r7 /* remember old NIP */
^ add a space
> +#endif
Please add a blank line here, to match the formatting for the rest of
this file.
> /* Calculate ip from nip-4 into r3 for call below */
> subi r3, r7, MCOUNT_INSN_SIZE
>
> /* Put the original return address in r4 as parent_ip */
> + std r0, _LINK(r1)
> mr r4, r0
>
> - /* Set pt_regs to NULL */
> - li r6, 0
> + /* Load &pt_regs in r6 for call below */
> + addi r6, r1 ,STACK_FRAME_OVERHEAD
^^ incorrect spacing
>
> /* ftrace_call(r3, r4, r5, r6) */
> .globl ftrace_call
> @@ -189,6 +194,10 @@ ftrace_call:
>
> ld r3, _NIP(r1)
> mtctr r3
Another blank line here.
> +#ifdef CONFIG_LIVEPATCH_64
> + cmpd r14, r3 /* has NIP been altered? */
> + REST_GPR(14, r1)
> +#endif
>
> /* Restore gprs */
> REST_GPRS(3, 10, r1)
> @@ -196,13 +205,17 @@ ftrace_call:
> /* Restore callee's TOC */
> ld r2, 24(r1)
>
> + /* Restore possibly modified LR */
> + ld r0, _LINK(r1)
> + mtlr r0
> +
> /* Pop our stack frame */
> addi r1, r1, SWITCH_FRAME_SIZE
>
> - /* Reload original LR */
> - ld r0, LRSAVE(r1)
> - mtlr r0
> -
> +#ifdef CONFIG_LIVEPATCH_64
> + /* Based on the cmpd above, if the NIP was altered handle livepatch */
> + bne- livepatch_handler
> +#endif
Here too.
> /* Handle function_graph or go back */
> b ftrace_caller_common
>
- Naveen
Christophe Leroy wrote:
> PPC64 mprofile versions and PPC32 are very similar.
>
> Modify PPC64 version so that if can be reused for PPC32.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
> 1 file changed, 51 insertions(+), 22 deletions(-)
While I agree that ppc32 and -mprofile-kernel ftrace code are very
similar, I think this patch adds way too many #ifdefs. IMHO, this
makes the resultant code quite difficult to follow.
- Naveen
>
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index 6071e0122797..56da60e98327 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -34,13 +34,16 @@
> */
> _GLOBAL(ftrace_regs_caller)
> /* Save the original return address in A's stack frame */
> - std r0,LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> + PPC_STL r0,LRSAVE(r1)
> +#endif
>
> /* Create our stack frame + pt_regs */
> - stdu r1,-SWITCH_FRAME_SIZE(r1)
> + PPC_STLU r1,-SWITCH_FRAME_SIZE(r1)
>
> /* Save all gprs to pt_regs */
> SAVE_GPR(0, r1)
> +#ifdef CONFIG_PPC64
> SAVE_GPRS(2, 11, r1)
>
> /* Ok to continue? */
> @@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
> beq ftrace_no_trace
>
> SAVE_GPRS(12, 31, r1)
> +#else
> + stmw r2, GPR2(r1)
> +#endif
>
> /* Save previous stack pointer (r1) */
> addi r8, r1, SWITCH_FRAME_SIZE
> - std r8, GPR1(r1)
> + PPC_STL r8, GPR1(r1)
>
> /* Load special regs for save below */
> mfmsr r8
> @@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
> /* Get the _mcount() call site out of LR */
> mflr r7
> /* Save it as pt_regs->nip */
> - std r7, _NIP(r1)
> + PPC_STL r7, _NIP(r1)
> /* Save the read LR in pt_regs->link */
> - std r0, _LINK(r1)
> + PPC_STL r0, _LINK(r1)
>
> +#ifdef CONFIG_PPC64
> /* Save callee's TOC in the ABI compliant location */
> std r2, 24(r1)
> ld r2,PACATOC(r13) /* get kernel TOC in r2 */
> @@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
> addis r3,r2,function_trace_op@toc@ha
> addi r3,r3,function_trace_op@toc@l
> ld r5,0(r3)
> +#else
> + lis r3,function_trace_op@ha
> + lwz r5,function_trace_op@l(r3)
> +#endif
>
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> mr r14,r7 /* remember old NIP */
> #endif
> /* Calculate ip from nip-4 into r3 for call below */
> @@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
> mr r4, r0
>
> /* Save special regs */
> - std r8, _MSR(r1)
> - std r9, _CTR(r1)
> - std r10, _XER(r1)
> - std r11, _CCR(r1)
> + PPC_STL r8, _MSR(r1)
> + PPC_STL r9, _CTR(r1)
> + PPC_STL r10, _XER(r1)
> + PPC_STL r11, _CCR(r1)
>
> /* Load &pt_regs in r6 for call below */
> addi r6, r1 ,STACK_FRAME_OVERHEAD
> @@ -100,27 +111,32 @@ ftrace_regs_call:
> nop
>
> /* Load ctr with the possibly modified NIP */
> - ld r3, _NIP(r1)
> + PPC_LL r3, _NIP(r1)
> mtctr r3
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> cmpd r14, r3 /* has NIP been altered? */
> #endif
>
> /* Restore gprs */
> - REST_GPR(0, r1)
> +#ifdef CONFIG_PPC64
> REST_GPRS(2, 31, r1)
> +#else
> + lmw r2, GPR2(r1)
> +#endif
>
> /* Restore possibly modified LR */
> - ld r0, _LINK(r1)
> + PPC_LL r0, _LINK(r1)
> mtlr r0
>
> +#ifdef CONFIG_PPC64
> /* Restore callee's TOC */
> ld r2, 24(r1)
> +#endif
>
> /* Pop our stack frame */
> addi r1, r1, SWITCH_FRAME_SIZE
>
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> /* Based on the cmpd above, if the NIP was altered handle livepatch */
> bne- livepatch_handler
> #endif
> @@ -129,6 +145,7 @@ ftrace_regs_call:
> _GLOBAL(ftrace_stub)
> blr
>
> +#ifdef CONFIG_PPC64
> ftrace_no_trace:
> mflr r3
> mtctr r3
> @@ -136,25 +153,31 @@ ftrace_no_trace:
> addi r1, r1, SWITCH_FRAME_SIZE
> mtlr r0
> bctr
> +#endif
>
> _GLOBAL(ftrace_caller)
> /* Save the original return address in A's stack frame */
> - std r0, LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> + PPC_STL r0, LRSAVE(r1)
> +#endif
>
> /* Create our stack frame + pt_regs */
> - stdu r1, -SWITCH_FRAME_SIZE(r1)
> + PPC_STLU r1, -SWITCH_FRAME_SIZE(r1)
>
> /* Save all gprs to pt_regs */
> SAVE_GPRS(3, 10, r1)
>
> +#ifdef CONFIG_PPC64
> lbz r3, PACA_FTRACE_ENABLED(r13)
> cmpdi r3, 0
> beq ftrace_no_trace
> +#endif
>
> /* Get the _mcount() call site out of LR */
> mflr r7
> - std r7, _NIP(r1)
> + PPC_STL r7, _NIP(r1)
>
> +#ifdef CONFIG_PPC64
> /* Save callee's TOC in the ABI compliant location */
> std r2, 24(r1)
> ld r2, PACATOC(r13) /* get kernel TOC in r2 */
> @@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
> addis r3, r2, function_trace_op@toc@ha
> addi r3, r3, function_trace_op@toc@l
> ld r5, 0(r3)
> +#else
> + lis r3,function_trace_op@ha
> + lwz r5,function_trace_op@l(r3)
> +#endif
>
> #ifdef CONFIG_LIVEPATCH_64
> SAVE_GPR(14, r1)
> @@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
> subi r3, r7, MCOUNT_INSN_SIZE
>
> /* Put the original return address in r4 as parent_ip */
> - std r0, _LINK(r1)
> + PPC_STL r0, _LINK(r1)
> mr r4, r0
>
> /* Load &pt_regs in r6 for call below */
> @@ -183,7 +210,7 @@ ftrace_call:
> bl ftrace_stub
> nop
>
> - ld r3, _NIP(r1)
> + PPC_LL r3, _NIP(r1)
> mtctr r3
> #ifdef CONFIG_LIVEPATCH_64
> cmpd r14, r3 /* has NIP been altered? */
> @@ -193,11 +220,13 @@ ftrace_call:
> /* Restore gprs */
> REST_GPRS(3, 10, r1)
>
> +#ifdef CONFIG_PPC64
> /* Restore callee's TOC */
> ld r2, 24(r1)
> +#endif
>
> /* Restore possibly modified LR */
> - ld r0, _LINK(r1)
> + PPC_LL r0, _LINK(r1)
> mtlr r0
>
> /* Pop our stack frame */
> @@ -209,7 +238,7 @@ ftrace_call:
> #endif
> bctr /* jump after _mcount site */
>
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> /*
> * This function runs in the mcount context, between two functions. As
> * such it can only clobber registers which are volatile and used in
> --
> 2.33.1
>
On Mon, 14 Feb 2022 22:54:23 +0530
"Naveen N. Rao" <[email protected]> wrote:
> For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use
> ftrace directly") also adds recursion check before the call to
> function_graph_enter() in prepare_ftrace_return(). Do we need that on
> powerpc as well?
Yes. The function_graph_enter() does not provide any recursion protection,
so if it were to call something that gets function graph traced, it will
crash the machine.
-- Steve
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>> of livepatching.
>>
>> Also note that powerpc being the last one to convert to
>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>> klp_arch_set_pc() on all architectures.
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>> arch/powerpc/include/asm/livepatch.h | 4 +---
>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index cdac2115eb00..e2b1792b2aae 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -210,6 +210,7 @@ config PPC
>> select HAVE_DEBUG_KMEMLEAK
>> select HAVE_DEBUG_STACKOVERFLOW
>> select HAVE_DYNAMIC_FTRACE
>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>> select HAVE_EBPF_JIT
>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>> && POWER7_CPU)
>> diff --git a/arch/powerpc/include/asm/ftrace.h
>> b/arch/powerpc/include/asm/ftrace.h
>> index b3f6184f77ea..45c3d6f11daa 100644
>> --- a/arch/powerpc/include/asm/ftrace.h
>> +++ b/arch/powerpc/include/asm/ftrace.h
>> @@ -22,6 +22,23 @@ static inline unsigned long
>> ftrace_call_adjust(unsigned long addr)
>> struct dyn_arch_ftrace {
>> struct module *mod;
>> };
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>> +struct ftrace_regs {
>> + struct pt_regs regs;
>> +};
>> +
>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>> ftrace_regs *fregs)
>> +{
>> + return &fregs->regs;
>> +}
>
> I think this is wrong. We need to differentiate between ftrace_caller()
> and ftrace_regs_caller() here, and only return pt_regs if coming in
> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
>
>> +
>> +static __always_inline void ftrace_instruction_pointer_set(struct
>> ftrace_regs *fregs,
>> + unsigned long ip)
>> +{
>> + regs_set_return_ip(&fregs->regs, ip);
>
> Should we use that helper here? regs_set_return_ip() also updates some
> other state related to taking interrupts and I don't think it makes
> sense for use with ftrace.
>
Today we have:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
struct pt_regs *regs = ftrace_get_regs(fregs);
regs_set_return_ip(regs, ip);
}
Which like x86 and s390 becomes:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
ftrace_instruction_pointer_set(fregs, ip);
}
That's the reason why I've been using regs_set_return_ip(). Do you think
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
registers if they are still valid")
Christophe
Le 14/02/2022 à 18:51, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> PPC64 mprofile versions and PPC32 are very similar.
>>
>> Modify PPC64 version so that if can be reused for PPC32.
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
>> 1 file changed, 51 insertions(+), 22 deletions(-)
>
> While I agree that ppc32 and -mprofile-kernel ftrace code are very
> similar, I think this patch adds way too many #ifdefs. IMHO, this
> makes the resultant code quite difficult to follow.
Ok, I can introduce some GAS macros for a few of them in a followup patch.
Christophe
+ S390 people
Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
>
>
> Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
>> Michael Ellerman wrote:
>>> Christophe Leroy <[email protected]> writes:
>>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>>> Christophe Leroy wrote:
>>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>>> of livepatching.
>>>>>>
>>>>>> Also note that powerpc being the last one to convert to
>>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>>> klp_arch_set_pc() on all architectures.
>>>>>>
>>>>>> Signed-off-by: Christophe Leroy <[email protected]>
>>>>>> ---
>>>>>> arch/powerpc/Kconfig | 1 +
>>>>>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>>>>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>>>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>>> --- a/arch/powerpc/Kconfig
>>>>>> +++ b/arch/powerpc/Kconfig
>>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>> select HAVE_DEBUG_KMEMLEAK
>>>>>> select HAVE_DEBUG_STACKOVERFLOW
>>>>>> select HAVE_DYNAMIC_FTRACE
>>>>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL ||
>>>>>> PPC32
>>>>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL ||
>>>>>> PPC32
>>>>>> select HAVE_EBPF_JIT
>>>>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if
>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>> struct dyn_arch_ftrace {
>>>>>> struct module *mod;
>>>>>> };
>>>>>> +
>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>> +struct ftrace_regs {
>>>>>> + struct pt_regs regs;
>>>>>> +};
>>>>>> +
>>>>>> +static __always_inline struct pt_regs
>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>> +{
>>>>>> + return &fregs->regs;
>>>>>> +}
>>>>>
>>>>> I think this is wrong. We need to differentiate between
>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return
>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e.,
>>>>> FL_SAVE_REGS is set).
>>>>
>>>> Not sure I follow you.
>>>>
>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add
>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>
>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
>>>> also with ftrace_caller().
>>>>
>>>> Sure you only have the params, but that's the same on s390, so what
>>>> did I miss ?
>>
>> It looks like s390 is special since it apparently saves all registers
>> even for ftrace_caller:
>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
>
> It is not what I understand from their code, see
> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
>
>
> They have a common macro called with argument 'allregs' which is set to
> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> When allregs == 1, the macro seems to save more.
>
> But ok, I can do like x86, but I need a trick to know whether
> FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> Any idea what the condition can be for powerpc ?
>
Finally, it looks like this change is done via commit 894979689d3a
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
implementations") four hours the same day after the implementation of
arch_ftrace_get_regs()
They may have forgotten to change arch_ftrace_get_regs() which was added
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
support") with the assumption that ftrace_caller and ftrace_regs_caller
where identical.
Christophe
Christophe Leroy <[email protected]> writes:
> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>> of livepatching.
>>>
>>> Also note that powerpc being the last one to convert to
>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>> klp_arch_set_pc() on all architectures.
>>>
>>> Signed-off-by: Christophe Leroy <[email protected]>
>>> ---
>>> arch/powerpc/Kconfig | 1 +
>>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index cdac2115eb00..e2b1792b2aae 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -210,6 +210,7 @@ config PPC
>>> select HAVE_DEBUG_KMEMLEAK
>>> select HAVE_DEBUG_STACKOVERFLOW
>>> select HAVE_DYNAMIC_FTRACE
>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>>> select HAVE_EBPF_JIT
>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>>> && POWER7_CPU)
>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>> b/arch/powerpc/include/asm/ftrace.h
>>> index b3f6184f77ea..45c3d6f11daa 100644
>>> --- a/arch/powerpc/include/asm/ftrace.h
>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>> ftrace_call_adjust(unsigned long addr)
>>> struct dyn_arch_ftrace {
>>> struct module *mod;
>>> };
>>> +
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>> +struct ftrace_regs {
>>> + struct pt_regs regs;
>>> +};
>>> +
>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>>> ftrace_regs *fregs)
>>> +{
>>> + return &fregs->regs;
>>> +}
>>
>> I think this is wrong. We need to differentiate between ftrace_caller()
>> and ftrace_regs_caller() here, and only return pt_regs if coming in
>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>
> Not sure I follow you.
>
> This is based on 5740a7c71ab6 ("s390/ftrace: add
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>
> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
> with ftrace_caller().
>
> Sure you only have the params, but that's the same on s390, so what did
> I miss ?
I already have this series in next, I can pull it out, but I'd rather
not.
I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.
>>> +static __always_inline void ftrace_instruction_pointer_set(struct
>>> ftrace_regs *fregs,
>>> + unsigned long ip)
>>> +{
>>> + regs_set_return_ip(&fregs->regs, ip);
>>
>> Should we use that helper here? regs_set_return_ip() also updates some
>> other state related to taking interrupts and I don't think it makes
>> sense for use with ftrace.
>
>
> Today we have:
>
> static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
> long ip)
> {
> struct pt_regs *regs = ftrace_get_regs(fregs);
>
> regs_set_return_ip(regs, ip);
> }
>
>
> Which like x86 and s390 becomes:
>
> static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
> long ip)
> {
> ftrace_instruction_pointer_set(fregs, ip);
> }
>
>
>
> That's the reason why I've been using regs_set_return_ip(). Do you think
> it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
>
> That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
> registers if they are still valid")
It's not wrong, but I think it's unnecessary. We need to use
regs_set_return_ip() if we're changing the regs->ip of an interrupt
frame, so that the interrupt return code will reload it.
But AIUI in this case we're not doing that, we're changing the regs->ip
of a pt_regs provided by ftrace, which shouldn't ever be an interrupt
frame.
So it's not a bug to use regs_set_return_ip(), but it is unncessary and
means we'll reload the interrupt state unnecessarily on the next
interrupt return.
cheers
Christophe Leroy wrote:
> + S390 people
>
> Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
>>
>>
>> Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
>>> Michael Ellerman wrote:
>>>> Christophe Leroy <[email protected]> writes:
>>>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>>>> Christophe Leroy wrote:
>>>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>>>> of livepatching.
>>>>>>>
>>>>>>> Also note that powerpc being the last one to convert to
>>>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>>>> klp_arch_set_pc() on all architectures.
>>>>>>>
>>>>>>> Signed-off-by: Christophe Leroy <[email protected]>
>>>>>>> ---
>>>>>>> arch/powerpc/Kconfig | 1 +
>>>>>>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>>>>>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>>>>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>>>> --- a/arch/powerpc/Kconfig
>>>>>>> +++ b/arch/powerpc/Kconfig
>>>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>>> select HAVE_DEBUG_KMEMLEAK
>>>>>>> select HAVE_DEBUG_STACKOVERFLOW
>>>>>>> select HAVE_DYNAMIC_FTRACE
>>>>>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL ||
>>>>>>> PPC32
>>>>>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL ||
>>>>>>> PPC32
>>>>>>> select HAVE_EBPF_JIT
>>>>>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if
>>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>>> struct dyn_arch_ftrace {
>>>>>>> struct module *mod;
>>>>>>> };
>>>>>>> +
>>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>>> +struct ftrace_regs {
>>>>>>> + struct pt_regs regs;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static __always_inline struct pt_regs
>>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>>> +{
>>>>>>> + return &fregs->regs;
>>>>>>> +}
>>>>>>
>>>>>> I think this is wrong. We need to differentiate between
>>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return
>>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e.,
>>>>>> FL_SAVE_REGS is set).
>>>>>
>>>>> Not sure I follow you.
>>>>>
>>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add
>>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>>
>>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
>>>>> also with ftrace_caller().
>>>>>
>>>>> Sure you only have the params, but that's the same on s390, so what
>>>>> did I miss ?
Steven has explained the rationale for this in his other response:
https://lore.kernel.org/all/[email protected]/
>>>
>>> It looks like s390 is special since it apparently saves all registers
>>> even for ftrace_caller:
>>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
>>
>> It is not what I understand from their code, see
>> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
>>
>>
>> They have a common macro called with argument 'allregs' which is set to
>> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
>> When allregs == 1, the macro seems to save more.
>>
>> But ok, I can do like x86, but I need a trick to know whether
>> FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
>> Any idea what the condition can be for powerpc ?
We'll need to explicitly zero-out something in pt_regs in
ftrace_caller(). We can probably use regs->msr since we don't expect it
to be zero when saved from ftrace_regs_caller().
>>
>
> Finally, it looks like this change is done via commit 894979689d3a
> ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
> implementations") four hours the same day after the implementation of
> arch_ftrace_get_regs()
>
> They may have forgotten to change arch_ftrace_get_regs() which was added
> in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
> support") with the assumption that ftrace_caller and ftrace_regs_caller
> where identical.
Indeed, good find!
Thanks,
Naveen
On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" <[email protected]> wrote:
> As I understand it, the reason ftrace_get_regs() was introduced was to
> be able to only return the pt_regs, if _all_ registers were saved into
> it, which we don't do when coming in through ftrace_caller(). See the
> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
> arguments to be passed in to ftrace_regs by default"), which returns
> pt_regs conditionally.
I can give you the history of ftrace_caller and ftrace_regs_caller.
ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.
As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.
I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.
The first user to use the args was live kernel patching, as they only need
that and the return pointer.
-- Steve
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
> Michael Ellerman wrote:
>> Christophe Leroy <[email protected]> writes:
>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>> Christophe Leroy wrote:
>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>> of livepatching.
>>>>>
>>>>> Also note that powerpc being the last one to convert to
>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>> klp_arch_set_pc() on all architectures.
>>>>>
>>>>> Signed-off-by: Christophe Leroy <[email protected]>
>>>>> ---
>>>>> arch/powerpc/Kconfig | 1 +
>>>>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>>>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>> --- a/arch/powerpc/Kconfig
>>>>> +++ b/arch/powerpc/Kconfig
>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>> select HAVE_DEBUG_KMEMLEAK
>>>>> select HAVE_DEBUG_STACKOVERFLOW
>>>>> select HAVE_DYNAMIC_FTRACE
>>>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL ||
>>>>> PPC32
>>>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL ||
>>>>> PPC32
>>>>> select HAVE_EBPF_JIT
>>>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if
>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>>>> ftrace_call_adjust(unsigned long addr)
>>>>> struct dyn_arch_ftrace {
>>>>> struct module *mod;
>>>>> };
>>>>> +
>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>> +struct ftrace_regs {
>>>>> + struct pt_regs regs;
>>>>> +};
>>>>> +
>>>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>>>>> ftrace_regs *fregs)
>>>>> +{
>>>>> + return &fregs->regs;
>>>>> +}
>>>>
>>>> I think this is wrong. We need to differentiate between
>>>> ftrace_caller() and ftrace_regs_caller() here, and only return
>>>> pt_regs if coming in through ftrace_regs_caller() (i.e.,
>>>> FL_SAVE_REGS is set).
>>>
>>> Not sure I follow you.
>>>
>>> This is based on 5740a7c71ab6 ("s390/ftrace: add
>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>
>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
>>> also with ftrace_caller().
>>>
>>> Sure you only have the params, but that's the same on s390, so what
>>> did I miss ?
>
> It looks like s390 is special since it apparently saves all registers
> even for ftrace_caller:
> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
Thanks
Christophe
Michael Ellerman wrote:
> Christophe Leroy <[email protected]> writes:
>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>> Christophe Leroy wrote:
>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>> of livepatching.
>>>>
>>>> Also note that powerpc being the last one to convert to
>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>> klp_arch_set_pc() on all architectures.
>>>>
>>>> Signed-off-by: Christophe Leroy <[email protected]>
>>>> ---
>>>> arch/powerpc/Kconfig | 1 +
>>>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>> --- a/arch/powerpc/Kconfig
>>>> +++ b/arch/powerpc/Kconfig
>>>> @@ -210,6 +210,7 @@ config PPC
>>>> select HAVE_DEBUG_KMEMLEAK
>>>> select HAVE_DEBUG_STACKOVERFLOW
>>>> select HAVE_DYNAMIC_FTRACE
>>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>>>> select HAVE_EBPF_JIT
>>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>>>> && POWER7_CPU)
>>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>>> b/arch/powerpc/include/asm/ftrace.h
>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>>> ftrace_call_adjust(unsigned long addr)
>>>> struct dyn_arch_ftrace {
>>>> struct module *mod;
>>>> };
>>>> +
>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>> +struct ftrace_regs {
>>>> + struct pt_regs regs;
>>>> +};
>>>> +
>>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>>>> ftrace_regs *fregs)
>>>> +{
>>>> + return &fregs->regs;
>>>> +}
>>>
>>> I think this is wrong. We need to differentiate between ftrace_caller()
>>> and ftrace_regs_caller() here, and only return pt_regs if coming in
>>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>>
>> Not sure I follow you.
>>
>> This is based on 5740a7c71ab6 ("s390/ftrace: add
>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>
>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
>> with ftrace_caller().
>>
>> Sure you only have the params, but that's the same on s390, so what did
>> I miss ?
It looks like s390 is special since it apparently saves all registers
even for ftrace_caller:
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
As I understand it, the reason ftrace_get_regs() was introduced was to
be able to only return the pt_regs, if _all_ registers were saved into
it, which we don't do when coming in through ftrace_caller(). See the
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
arguments to be passed in to ftrace_regs by default"), which returns
pt_regs conditionally.
>
> I already have this series in next, I can pull it out, but I'd rather
> not.
Yeah, I'm sorry about the late review on this one.
>
> I'll leave it in for now, hopefully you two can agree overnight my time
> whether this is a big problem or something we can fix with a fixup
> patch.
I think changes to this particular patch can be added as an incremental
patch. If anything, pt_regs won't have all valid registers, but no one
should depend on it without also setting FL_SAVE_REGS anyway.
I was concerned about patch 8 though, where we are missing saving r1
into pt_regs. That gets used in patch 11, and will be used during
unwinding when the function_graph tracer is active. But, this should
still just result in us being unable to unwind the stack, so I think
that can also be an incremental patch.
Thanks,
Naveen
Steven Rostedt wrote:
> On Tue, 15 Feb 2022 19:06:48 +0530
> "Naveen N. Rao" <[email protected]> wrote:
>
>> As I understand it, the reason ftrace_get_regs() was introduced was to
>> be able to only return the pt_regs, if _all_ registers were saved into
>> it, which we don't do when coming in through ftrace_caller(). See the
>> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
>> arguments to be passed in to ftrace_regs by default"), which returns
>> pt_regs conditionally.
>
> I can give you the history of ftrace_caller and ftrace_regs_caller.
>
> ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
> The new fentry which happens at the start of the function, whereas mcount
> happens after the stack frame is set up, may change the rules on some
> architectures.
>
> As for ftrace_regs_caller, that was created for kprobes. As the majority of
> kprobes were added at the start of the function, it made sense to hook into
> ftrace as the ftrace trampoline call is much faster than taking a
> breakpoint interrupt. But to keep compatibility with breakpoint
> interrupts, we needed to fill in all the registers, and make it act just
> like a breakpoint interrupt.
>
> I've been wanting to record function parameters, and because the ftrace
> trampoline must at a minimum save the function parameters before calling
> the ftrace callbacks, all the information for those parameters were being
> saved but were never exposed to the ftrace callbacks. I created the the
> DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
> just the parameters filled in, but that was criticized as it could be
> confusing where the non filled in pt_regs might be used and thinking they
> are legitimate. So I created ftrace_regs that would give you just the
> function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
> give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
> not, it will give you a NULL pointer.
>
> The first user to use the args was live kernel patching, as they only need
> that and the return pointer.
Thanks, that helps.
- Naveen
On Mon, 20 Dec 2021 16:37:58 +0000, Christophe Leroy wrote:
> This series implements livepatch on PPC32 and implements
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS to simplify ftracing.
>
> v2:
> - Fix problem with strict modules RWX
> - Convert powerpc to CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> - Convert function graph tracing to C
> - Refactor PPC32 versus PPC64
>
> [...]
Patches 1 and 3-13 applied to powerpc/next.
[01/13] livepatch: Fix build failure on 32 bits processors
https://git.kernel.org/powerpc/c/2f293651eca3eacaeb56747dede31edace7329d2
[03/13] powerpc/module_32: Fix livepatching for RO modules
https://git.kernel.org/powerpc/c/0c850965d6909d39fd69d6a3602bb62b48cad417
[04/13] powerpc/ftrace: Add support for livepatch to PPC32
https://git.kernel.org/powerpc/c/a4520b25276500f1abcfc55d24f1251b7b08eff6
[05/13] powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32
https://git.kernel.org/powerpc/c/7875bc9b07cde868784195e215f4deaa0fa928a2
[06/13] powerpc/ftrace: Simplify PPC32's return_to_handler()
https://git.kernel.org/powerpc/c/7bdb478c1d15cfd3a92db6331cb2d3dd3a8b9436
[07/13] powerpc/ftrace: Prepare PPC32's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS
https://git.kernel.org/powerpc/c/d95bf254be5f74c1e4c8f7cb64e2e21b9cc91717
[08/13] powerpc/ftrace: Prepare PPC64's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS
https://git.kernel.org/powerpc/c/c75388a8ceffbf1bf72c61afe66a72e58aa20c74
[09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
https://git.kernel.org/powerpc/c/40b035efe288f42bbf4483236cde652584ccb64e
[10/13] powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller
https://git.kernel.org/powerpc/c/0c81ed5ed43863d313cf253b0ebada6ea2f17676
[11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller
https://git.kernel.org/powerpc/c/830213786c498b0c488fedd2abc15a7ce442b42f
[12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
https://git.kernel.org/powerpc/c/41315494beed087011f256b4f1439bb3d8236904
[13/13] powerpc/ftrace: Remove ftrace_32.S
https://git.kernel.org/powerpc/c/4ee83a2cfbc46c13f2a08fe6d48dbcede53cdbf8
cheers
On Tue, Feb 15, 2022 at 09:55:52PM +0530, Naveen N. Rao wrote:
> > > > > > > I think this is wrong. We need to differentiate
> > > > > > > between ftrace_caller() and ftrace_regs_caller()
> > > > > > > here, and only return pt_regs if coming in through
> > > > > > > ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
> > > > > >
> > > > > > Not sure I follow you.
> > > > > >
> > > > > > This is based on 5740a7c71ab6 ("s390/ftrace: add
> > > > > > HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> > > > > >
> > > > > > It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS,
> > > > > > have the regs also with ftrace_caller().
> > > > > >
> > > > > > Sure you only have the params, but that's the same on
> > > > > > s390, so what did I miss ?
>
> Steven has explained the rationale for this in his other response:
> https://lore.kernel.org/all/[email protected]/
Thanks for this pointer, this clarifies a couple of things!
> > > > It looks like s390 is special since it apparently saves all
> > > > registers even for ftrace_caller:
> > > > https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
> > >
> > > It is not what I understand from their code, see https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
> > >
> > >
> > > They have a common macro called with argument 'allregs' which is set
> > > to 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> > > When allregs == 1, the macro seems to save more.
> > >
> > > But ok, I can do like x86, but I need a trick to know whether
> > > FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> > > Any idea what the condition can be for powerpc ?
>
> We'll need to explicitly zero-out something in pt_regs in ftrace_caller().
> We can probably use regs->msr since we don't expect it to be zero when saved
> from ftrace_regs_caller().
> >
> > Finally, it looks like this change is done via commit 894979689d3a
> > ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
> > implementations") four hours the same day after the implementation of
> > arch_ftrace_get_regs()
> >
> > They may have forgotten to change arch_ftrace_get_regs() which was added
> > in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > support") with the assumption that ftrace_caller and ftrace_regs_caller
> > where identical.
>
> Indeed, good find!
Thank you for bringing this up!
So, the in both variants s390 provides nearly identical data. The only
difference is that for FL_SAVE_REGS the program status word mask is
missing; therefore it is not possible to figure out the condition code
or if interrupts were enabled/disabled.
Vasily, Sven, I think we have two options here:
- don't provide sane psw mask contents at all and say (again) that
ptregs contents are identical
- provide (finally) a full psw mask contents using epsw, and indicate
validity with a flags bit in pt_regs
I would vote for the second option, even though epsw is slow. But this
is about the third or fourth time this came up in different
contexts. So I'd guess we should go for the slow but complete
solution. Opinions?
Heiko Carstens <[email protected]> writes:
> So, the in both variants s390 provides nearly identical data. The only
> difference is that for FL_SAVE_REGS the program status word mask is
> missing; therefore it is not possible to figure out the condition code
> or if interrupts were enabled/disabled.
>
> Vasily, Sven, I think we have two options here:
>
> - don't provide sane psw mask contents at all and say (again) that
> ptregs contents are identical
>
> - provide (finally) a full psw mask contents using epsw, and indicate
> validity with a flags bit in pt_regs
>
> I would vote for the second option, even though epsw is slow. But this
> is about the third or fourth time this came up in different
> contexts. So I'd guess we should go for the slow but complete
> solution. Opinions?
Given that this only affects ftrace_regs_caller, i would also vote for the
second option.
Hi Michael,
Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
> direct tramp.
>
> Signed-off-by: Christophe Leroy <[email protected]>
You didn't apply this patch when you merged the series. Without it I get
the following :
[ 6.191287] Testing ftrace recursion: PASSED
[ 6.473308] Testing ftrace recursion safe: PASSED
[ 6.755759] Testing ftrace regs: PASSED
[ 7.037994] Testing tracer nop: PASSED
[ 7.042256] Testing tracer function_graph: FAILED!
[ 12.216112] ------------[ cut here ]------------
[ 12.220436] WARNING: CPU: 0 PID: 1 at kernel/trace/trace.c:1953
run_tracer_selftest+0x138/0x1b4
[ 12.229045] CPU: 0 PID: 1 Comm: swapper Not tainted
5.17.0-rc2-s3k-dev-02096-g28b040bd2357 #1030
[ 12.237735] NIP: c00d01b4 LR: c00d01b4 CTR: c03d37fc
[ 12.242724] REGS: c902bd90 TRAP: 0700 Not tainted
(5.17.0-rc2-s3k-dev-02096-g28b040bd2357)
[ 12.251157] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 28000242 XER: 00000000
[ 12.257870]
[ 12.257870] GPR00: c00d01b4 c902be50 c2140000 00000007 c108d224
00000001 c11ed2e8 c108d340
[ 12.257870] GPR08: 3fffbfff 00000000 c129beac 3fffc000 22000244
00000000 c0004b78 00000000
[ 12.257870] GPR16: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 c1039020
[ 12.257870] GPR24: c12d0000 c1000144 c1223c48 c12b53c4 c12b55dc
c1293118 fffffdf4 c1223c38
[ 12.293843] NIP [c00d01b4] run_tracer_selftest+0x138/0x1b4
[ 12.299265] LR [c00d01b4] run_tracer_selftest+0x138/0x1b4
[ 12.304603] Call Trace:
[ 12.307012] [c902be50] [c00d01b4] run_tracer_selftest+0x138/0x1b4
(unreliable)
[ 12.314155] [c902be70] [c100cf44] register_tracer+0x14c/0x218
[ 12.319835] [c902be90] [c10011a0] do_one_initcall+0x8c/0x17c
[ 12.325430] [c902bef0] [c10014c0] kernel_init_freeable+0x1a8/0x254
[ 12.331540] [c902bf20] [c0004ba8] kernel_init+0x30/0x150
[ 12.336789] [c902bf30] [c001222c] ret_from_kernel_thread+0x5c/0x64
[ 12.342902] Instruction dump:
[ 12.345828] 4bf9a135 813d0030 7fc4f378 7d2903a6 7fa3eb78 4e800421
7c7e1b79 939f0f60
[ 12.353657] 41820014 3c60c08a 3863644c 4bf9a109 <0fe00000> 387f00b0
4bff76bd 893d0052
[ 12.361659] ---[ end trace 0000000000000000 ]---
With the patch I get:
[ 6.191286] Testing ftrace recursion: PASSED
[ 6.473307] Testing ftrace recursion safe: PASSED
[ 6.755758] Testing ftrace regs: PASSED
[ 7.037993] Testing tracer nop: PASSED
[ 7.042255] Testing tracer function_graph: PASSED
Is this patch going to be merged via another tree ?
Thanks
Christophe
> ---
> kernel/trace/trace_selftest.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index afd937a46496..abcadbe933bb 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -784,9 +784,7 @@ static struct fgraph_ops fgraph_ops __initdata = {
> .retfunc = &trace_graph_return,
> };
>
> -#if defined(CONFIG_DYNAMIC_FTRACE) && \
> - defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
> -#define TEST_DIRECT_TRAMP
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> noinline __noclone static void trace_direct_tramp(void) { }
> #endif
>
> @@ -849,7 +847,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
> goto out;
> }
>
> -#ifdef TEST_DIRECT_TRAMP
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> tracing_reset_online_cpus(&tr->array_buffer);
> set_graph_array(tr);
>
On Thu, 24 Feb 2022 13:43:02 +0000
Christophe Leroy <[email protected]> wrote:
> Hi Michael,
>
> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
> > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
> > direct tramp.
> >
> > Signed-off-by: Christophe Leroy <[email protected]>
>
> You didn't apply this patch when you merged the series. Without it I get
> the following :
Maybe they wanted my acked-by.
But I'm working on a series to send to Linus. I can pick this patch up, as
it touches just my code.
-- Steve
On Thu, 24 Feb 2022 15:13:12 +0000
Christophe Leroy <[email protected]> wrote:
> > But I'm working on a series to send to Linus. I can pick this patch up, as
> > it touches just my code.
> >
>
> That would be great, thanks.
It's in my queue and running through my tests, which take 7 to 13 hours to
complete (depending on the changes).
-- Steve
Le 24/02/2022 à 15:53, Steven Rostedt a écrit :
> On Thu, 24 Feb 2022 13:43:02 +0000
> Christophe Leroy <[email protected]> wrote:
>
>> Hi Michael,
>>
>> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
>>> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
>>> direct tramp.
>>>
>>> Signed-off-by: Christophe Leroy <[email protected]>
>>
>> You didn't apply this patch when you merged the series. Without it I get
>> the following :
>
> Maybe they wanted my acked-by.
>
> But I'm working on a series to send to Linus. I can pick this patch up, as
> it touches just my code.
>
That would be great, thanks.
Christophe
Steven Rostedt <[email protected]> writes:
> On Thu, 24 Feb 2022 13:43:02 +0000
> Christophe Leroy <[email protected]> wrote:
>
>> Hi Michael,
>>
>> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
>> > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
>> > direct tramp.
>> >
>> > Signed-off-by: Christophe Leroy <[email protected]>
>>
>> You didn't apply this patch when you merged the series. Without it I get
>> the following :
>
> Maybe they wanted my acked-by.
Yeah, I didn't want to take it via my tree without an ack. I meant to
reply to the patch saying that but ...
> But I'm working on a series to send to Linus. I can pick this patch up, as
> it touches just my code.
Thanks.
cheers