This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/livepatch.h | 4 +-
arch/powerpc/kernel/module_32.c | 33 +++++
arch/powerpc/kernel/trace/ftrace.c | 53 +++-----
arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
kernel/livepatch/core.c | 4 +-
6 files changed, 230 insertions(+), 53 deletions(-)
--
2.31.1
module_trampoline_target() is used by __ftrace_modify_call().
Implement it for PPC32 so that CONFIG_DYNAMIC_FTRACE_WITH_REGS
can be activated on PPC32 as well.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/module_32.c | 25 ++++++++++++++++++++
arch/powerpc/kernel/trace/ftrace.c | 37 ++++--------------------------
2 files changed, 29 insertions(+), 33 deletions(-)
@@ -273,6 +273,31 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,}#ifdef CONFIG_DYNAMIC_FTRACE+intmodule_trampoline_target(structmodule*mod,unsignedlongaddr,+unsignedlong*target)+{+unsignedintjmp[4];++/* Find where the trampoline jumps to */+if(copy_from_kernel_nofault(jmp,(void*)addr,sizeof(jmp)))+return-EFAULT;++/* verify that this is what we expect it to be */+if((jmp[0]&0xffff0000)!=PPC_RAW_LIS(_R12,0)||+(jmp[1]&0xffff0000)!=PPC_RAW_ADDI(_R12,_R12,0)||+jmp[2]!=PPC_RAW_MTCTR(_R12)||+jmp[3]!=PPC_RAW_BCTR())+return-EINVAL;++addr=(jmp[1]&0xffff)|((jmp[0]&0xffff)<<16);+if(addr&0x8000)+addr-=0x10000;++*target=addr;++return0;+}+intmodule_finalize_ftrace(structmodule*module,constElf_Shdr*sechdrs){module->arch.tramp=do_plt_call(module->core_layout.base,
@@ -238,41 +237,13 @@ __ftrace_make_nop(struct module *mod,/* lets find where the pointer goes */tramp=find_bl_target(ip,op);-/*-*OnPPC32thetrampolinelookslike:-*0x3d,0x80,0x00,0x00lisr12,sym@ha-*0x39,0x8c,0x00,0x00addir12,r12,sym@l-*0x7d,0x89,0x03,0xa6mtctrr12-*0x4e,0x80,0x04,0x20bctr-*/--pr_devel("ip:%lx jumps to %lx",ip,tramp);-/* Find where the trampoline jumps to */-if(copy_from_kernel_nofault(jmp,(void*)tramp,sizeof(jmp))){-pr_err("Failed to read %lx\n",tramp);+if(module_trampoline_target(mod,tramp,&ptr)){+pr_err("Failed to get trampoline target\n");return-EFAULT;}-pr_devel(" %08x %08x ",jmp[0],jmp[1]);--/* verify that this is what we expect it to be */-if(((jmp[0]&0xffff0000)!=0x3d800000)||-((jmp[1]&0xffff0000)!=0x398c0000)||-(jmp[2]!=0x7d8903a6)||-(jmp[3]!=0x4e800420)){-pr_err("Not a trampoline\n");-return-EINVAL;-}--tramp=(jmp[1]&0xffff)|-((jmp[0]&0xffff)<<16);-if(tramp&0x8000)-tramp-=0x10000;--pr_devel(" %lx ",tramp);--if(tramp!=addr){+if(ptr!=addr){pr_err("Trampoline location %08lx does not match addr\n",tramp);return-EINVAL;
Unlike PPC64, PPC32 doesn't require any special compiler option
to get _mcount() call not clobbering registers.
Provide ftrace_regs_caller() and ftrace_regs_call() and activate
HAVE_DYNAMIC_FTRACE_WITH_REGS.
That's heavily copied from ftrace_64_mprofile.S
For the time being leave livepatching aside, it will come with
following patch.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 4 +-
arch/powerpc/kernel/module_32.c | 8 ++
arch/powerpc/kernel/trace/ftrace.c | 16 +++-
arch/powerpc/kernel/trace/ftrace_32.S | 109 ++++++++++++++++++++++++--
4 files changed, 125 insertions(+), 12 deletions(-)
@@ -561,6 +561,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)interr;structppc_instop;u32*ip=(u32*)rec->ip;+structmodule*mod=rec->arch.mod;+unsignedlongtramp;/* read where this goes */if(copy_inst_from_kernel_nofault(&op,ip))
@@ -573,13 +575,23 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)}/* If we never set up a trampoline to ftrace_caller, then bail */-if(!rec->arch.mod->arch.tramp){+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS+if(!mod->arch.tramp||!mod->arch.tramp_regs){+#else+if(!mod->arch.tramp){+#endifpr_err("No ftrace trampoline\n");return-EINVAL;}+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS+if(rec->flags&FTRACE_FL_REGS)+tramp=mod->arch.tramp_regs;+else+#endif+tramp=mod->arch.tramp;/* create the branch to the trampoline */-err=create_branch(&op,ip,rec->arch.mod->arch.tramp,BRANCH_SET_LINK);+err=create_branch(&op,ip,tramp,BRANCH_SET_LINK);if(err){pr_err("REL24 out of range!\n");return-EINVAL;
@@ -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);
This is heavily copied from PPC64. Not much to say about it.
Livepatch sample modules all work.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/livepatch.h | 4 +-
arch/powerpc/kernel/trace/ftrace_32.S | 69 +++++++++++++++++++++++++++
3 files changed, 72 insertions(+), 3 deletions(-)
All functions calling _mcount do it exactly the same way, with the
following sequence of instructions:
c07de788: 7c 08 02 a6 mflr r0
c07de78c: 90 01 00 04 stw r0,4(r1)
c07de790: 4b 84 13 65 bl c001faf4 <_mcount>
Allthough LR is pushed on stack, it is still in r0 while entering
_mcount().
Function arguments are in r3-r10, so r11 and r12 are still available
at that point.
Do like PPC64 and use r12 to move LR into CTR, so that r0 is preserved
and doesn't need to be restored from the stack.
While at it, bring back the EXPORT_SYMBOL at the end of _mcount.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/trace/ftrace_32.S | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-28 13:35:53
On Thu, 28 Oct 2021 14:24:00 +0200
Christophe Leroy [off-list ref] wrote:
This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/livepatch.h | 4 +-
arch/powerpc/kernel/module_32.c | 33 +++++
arch/powerpc/kernel/trace/ftrace.c | 53 +++-----
arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
kernel/livepatch/core.c | 4 +-
6 files changed, 230 insertions(+), 53 deletions(-)
This is great that you are doing this, but I wonder if it would even be
easier, and more efficient, if you could implement
HAVE_DYNAMIC_FTRACE_WITH_ARGS?
Then you don't need to save all regs for live kernel patching. And I am
also working on function tracing with arguments with this too.
That is, to call a generic ftrace callback, you need to save all the args
that are stored in registers to prevent the callback from clobbering them.
As live kernel patching only needs to have the arguments of the functions,
you save time from having to save the other regs as well.
The callbacks now have "struct ftrace_regs" instead of pt_regs, because it
will allow non ftrace_regs_caller functions to access the arguments if it
is supported.
Look at how x86_64 implements this. It should be possible to do this for
all other archs as well.
Also note, by doing this, we can then get rid of the ftrace_graph_caller,
and have function graph tracer be a function tracing callback, as it will
allow ftrace_graph_caller to have access to the stack and the return as
well.
If you need any more help or information to do this, I'd be happy to assist
you.
Note, you can implement this first, (I looked over the patches and they
seem fine) and then update both ppc64 and ppc32 to implement
DYNAMIC_FTRACE_WITH_ARGS.
Cheers,
-- Steve
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-11-01 14:51:09
Hi,
On Thu, 28 Oct 2021, Christophe Leroy wrote:
This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/livepatch.h | 4 +-
arch/powerpc/kernel/module_32.c | 33 +++++
arch/powerpc/kernel/trace/ftrace.c | 53 +++-----
arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
kernel/livepatch/core.c | 4 +-
6 files changed, 230 insertions(+), 53 deletions(-)
thanks for the patch set!
I wondered whether the reliability of stack traces also applies to PPC32.
This was obviously resolved by accdd093f260 ("powerpc: Activate
HAVE_RELIABLE_STACKTRACE for all").
Did the patch set pass the selftests in
tools/testing/selftests/livepatch/ ?
Regards
Miroslav
Nit: I had some problems to parse it. I wonder if the following is
better:
* Live patch works on PPC32 out of box and on PPC64 only with
* -mprofile-kernel. In both cases, the ftrace location is always
* within the first 16 bytes.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-11-24 22:35:06
Christophe Leroy [off-list ref] writes:
This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
I think we know patch 5 will need a respin because of the STRICT RWX vs
livepatching issue (https://github.com/linuxppc/issues/issues/375).
So should I take patches 2,3,4 for now?
cheers
This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
From: Michael Ellerman <hidden> Date: 2021-12-07 13:28:49
On Thu, 28 Oct 2021 14:24:00 +0200, Christophe Leroy wrote:
This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
[...]
On Thu, 28 Oct 2021 14:24:00 +0200
Christophe Leroy [off-list ref] wrote:
quoted
This series implements livepatch on PPC32.
This is largely copied from what's done on PPC64.
Christophe Leroy (5):
livepatch: Fix build failure on 32 bits processors
powerpc/ftrace: No need to read LR from stack in _mcount()
powerpc/ftrace: Add module_trampoline_target() for PPC32
powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
powerpc/ftrace: Add support for livepatch to PPC32
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/livepatch.h | 4 +-
arch/powerpc/kernel/module_32.c | 33 +++++
arch/powerpc/kernel/trace/ftrace.c | 53 +++-----
arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
kernel/livepatch/core.c | 4 +-
6 files changed, 230 insertions(+), 53 deletions(-)
This is great that you are doing this, but I wonder if it would even be
easier, and more efficient, if you could implement
HAVE_DYNAMIC_FTRACE_WITH_ARGS?
Then you don't need to save all regs for live kernel patching. And I am
also working on function tracing with arguments with this too.
That is, to call a generic ftrace callback, you need to save all the args
that are stored in registers to prevent the callback from clobbering them.
As live kernel patching only needs to have the arguments of the functions,
you save time from having to save the other regs as well.
The callbacks now have "struct ftrace_regs" instead of pt_regs, because it
will allow non ftrace_regs_caller functions to access the arguments if it
is supported.
Look at how x86_64 implements this. It should be possible to do this for
all other archs as well.
Also note, by doing this, we can then get rid of the ftrace_graph_caller,
and have function graph tracer be a function tracing callback, as it will
allow ftrace_graph_caller to have access to the stack and the return as
well.
If you need any more help or information to do this, I'd be happy to assist
you.
Note, you can implement this first, (I looked over the patches and they
seem fine) and then update both ppc64 and ppc32 to implement
DYNAMIC_FTRACE_WITH_ARGS.
I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
Ftrace selftests tell "Testing tracer function_graph: FAILED!".
Is there anything else to do ?
Thanks for your help
Christophe
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-12-13 17:15:42
On Mon, 13 Dec 2021 14:39:15 +0000
Christophe Leroy [off-list ref] wrote:
quoted
Note, you can implement this first, (I looked over the patches and they
seem fine) and then update both ppc64 and ppc32 to implement
DYNAMIC_FTRACE_WITH_ARGS.
I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
Ftrace selftests tell "Testing tracer function_graph: FAILED!".
Is there anything else to do ?
Yes. Because BPF is now hooking into the function callbacks, it causes
issues with function graph tracer. So what we did was to have function
graph tracing to now use the function tracer callback as well (this allows
both the BPF direct trampolines to work with function graph tracer).
As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
support that for now, I decided to make all the archs change function graph
tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
pain to have too many variants of function tracing between the archs).
The change that did this for x86 was:
0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")
This actually simplifies the function graph tracer, as you no longer need
it's own entry trampoline (still need the trampoline for the return of the
function).
What you need to do is:
In your arch/*/include/asm/ftrace.h add:
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);
Where ftrace_graph_func() is now what is called for the function graph
tracer, directly from the ftrace callbacks (no longer a secondary
trampoline).
Define the ftrace_graph_func() to be something like:
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *op, struct ftrace_regs *fregs)
{
struct pt_regs *regs = &fregs->regs;
unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
prepare_ftrace_return(ip, (unsigned long *)stack, 0);
}
This is called by the function tracer code. But because with
DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
also have access to the link register and the stack. Then you can use that
to modify the stack and or link register to jump to the the return
trampoline.
This should all work with powerpc (both 64 and 32) but if it does not, let
me know. I'm happy to help out.
-- Steve
On Mon, 13 Dec 2021 14:39:15 +0000
Christophe Leroy [off-list ref] wrote:
quoted
quoted
Note, you can implement this first, (I looked over the patches and they
seem fine) and then update both ppc64 and ppc32 to implement
DYNAMIC_FTRACE_WITH_ARGS.
I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
Ftrace selftests tell "Testing tracer function_graph: FAILED!".
Is there anything else to do ?
Yes. Because BPF is now hooking into the function callbacks, it causes
issues with function graph tracer. So what we did was to have function
graph tracing to now use the function tracer callback as well (this allows
both the BPF direct trampolines to work with function graph tracer).
As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
support that for now, I decided to make all the archs change function graph
tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
pain to have too many variants of function tracing between the archs).
The change that did this for x86 was:
0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")
This actually simplifies the function graph tracer, as you no longer need
it's own entry trampoline (still need the trampoline for the return of the
function).
What you need to do is:
In your arch/*/include/asm/ftrace.h add:
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);
Where ftrace_graph_func() is now what is called for the function graph
tracer, directly from the ftrace callbacks (no longer a secondary
trampoline).
Define the ftrace_graph_func() to be something like:
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *op, struct ftrace_regs *fregs)
{
struct pt_regs *regs = &fregs->regs;
unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
prepare_ftrace_return(ip, (unsigned long *)stack, 0);
}
This is called by the function tracer code. But because with
DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
also have access to the link register and the stack. Then you can use that
to modify the stack and or link register to jump to the the return
trampoline.
This should all work with powerpc (both 64 and 32) but if it does not, let
me know. I'm happy to help out.
Thanks, I will try that.
I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
have a working function tracer anymore ?
I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
Christophe
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-12-13 17:33:46
On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy [off-list ref] wrote:
Thanks, I will try that.
I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
have a working function tracer anymore ?
I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
Hmm, maybe not. I can't test it.
This needs to be fixed if that's the case.
Thanks for bringing it up!
-- Steve
On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy [off-list ref] wrote:
quoted
Thanks, I will try that.
I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
have a working function tracer anymore ?
I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
Hmm, maybe not. I can't test it.
This needs to be fixed if that's the case.
Thanks for bringing it up!
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-12-13 18:54:17
On Mon, 13 Dec 2021 17:50:52 +0000
Christophe Leroy [off-list ref] wrote:
quoted hunk
@@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long
parent, unsigned long ip,
out:
return parent;
}
+
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+ prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
+}
I have for powerpc prepare_ftrace_return as:
unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
unsigned long sp)
{
unsigned long return_hooker;
if (unlikely(ftrace_graph_is_dead()))
goto out;
if (unlikely(atomic_read(¤t->tracing_graph_pause)))
goto out;
return_hooker = ppc_function_entry(return_to_handler);
if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
parent = return_hooker;
out:
return parent;
}
Which means you'll need different parameters to it than what x86 has, which
has the prototype of:
void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
unsigned long frame_pointer)
and it does not use the frame_pointer for this case, which is why it is
zero.
For powerpc though, it uses the stack pointer, so you parameters are
incorrect. Looks like it should be:
prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
And that will likely not be enough. I'll need to update the ctr register,
as that is where the return address is saved. So you'll probably need it to be:
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *op, struct ftrace_regs *fregs)
{
unsigned long parent;
parent = prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
fregs->regs.ctr = parent;
}
-- Steve
On Mon, 13 Dec 2021 17:50:52 +0000
Christophe Leroy [off-list ref] wrote:
quoted
@@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long
parent, unsigned long ip,
out:
return parent;
}
+
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+ prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
+}
I have for powerpc prepare_ftrace_return as:
unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
unsigned long sp)
{
unsigned long return_hooker;
if (unlikely(ftrace_graph_is_dead()))
goto out;
if (unlikely(atomic_read(¤t->tracing_graph_pause)))
goto out;
return_hooker = ppc_function_entry(return_to_handler);
if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
parent = return_hooker;
out:
return parent;
}
Which means you'll need different parameters to it than what x86 has, which
has the prototype of:
void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
unsigned long frame_pointer)
and it does not use the frame_pointer for this case, which is why it is
zero.
For powerpc though, it uses the stack pointer, so you parameters are
incorrect. Looks like it should be:
prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
And that will likely not be enough. I'll need to update the ctr register,
as that is where the return address is saved. So you'll probably need it to be:
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *op, struct ftrace_regs *fregs)
{
unsigned long parent;
parent = prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
fregs->regs.ctr = parent;
}
On Mon, 13 Dec 2021 19:33:47 +0000
Christophe Leroy [off-list ref] wrote:
quoted
STill the same Oops, below
Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
this.
quoted
I will look more closely tomorrow.
OK, thanks.
The Oops was due to ftrace_caller() setting the regs argument to NULL.
After fixing that, I'm back into a situation where I get "Testing tracer
function_graph: FAILED!"
Will continue investigating.
Christophe
On Mon, 13 Dec 2021 19:33:47 +0000
Christophe Leroy [off-list ref] wrote:
quoted
STill the same Oops, below
Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
this.
quoted
I will look more closely tomorrow.
OK, thanks.
The Oops was due to ftrace_caller() setting the regs argument to NULL.
After fixing that, I'm back into a situation where I get "Testing tracer
function_graph: FAILED!"
Will continue investigating.
trace_selftest_startup_function_graph() calls register_ftrace_direct()
which returns -ENOSUPP because powerpc doesn't select
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?
Christophe
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-12-14 14:01:55
On Tue, 14 Dec 2021 08:35:14 +0100
Christophe Leroy [off-list ref] wrote:
quoted
Will continue investigating.
trace_selftest_startup_function_graph() calls register_ftrace_direct()
which returns -ENOSUPP because powerpc doesn't select
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?
Yes, that should be:
#if defined(CONFIG_DYNAMIC_FTRACE) && \
defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS)
#define TEST_DIRECT_TRAMP
noinline __noclone static void trace_direct_tramp(void) { }
#endif
And make it test it with or without the args.
Thanks for finding this.
-- Steve
On Mon, Dec 13, 2021 at 05:50:52PM +0000, Christophe Leroy wrote:
Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
quoted
On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy [off-list ref] wrote:
quoted
Thanks, I will try that.
I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
have a working function tracer anymore ?
I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
Hmm, maybe not. I can't test it.
This needs to be fixed if that's the case.
Thanks for bringing it up!
It still works, we run the full ftrace/kprobes selftests from the
kernel every day on multiple machines with several kernels (besides
other Linus' tree, but also linux-next). That said, I wanted to change
s390's code follow what x86 is currently doing anyway.
One thing to note: commit 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support") looks only that simple because
ftrace_caller _and_ ftrace_regs_caller used to save all register
contents into the pt_regs structure, which never was a requirement,
but implicitly fulfills the HAVE_DYNAMIC_FTRACE_WITH_ARGS
requirements.
Not sure if powerpc passes enough register contents via pt_regs for
HAVE_DYNAMIC_FTRACE_WITH_ARGS though. Might be something to check?
On Mon, Dec 13, 2021 at 05:50:52PM +0000, Christophe Leroy wrote:
quoted
Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
quoted
On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy [off-list ref] wrote:
quoted
Thanks, I will try that.
I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
have a working function tracer anymore ?
I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
Hmm, maybe not. I can't test it.
This needs to be fixed if that's the case.
Thanks for bringing it up!
It still works, we run the full ftrace/kprobes selftests from the
kernel every day on multiple machines with several kernels (besides
other Linus' tree, but also linux-next). That said, I wanted to change
s390's code follow what x86 is currently doing anyway.
One thing to note: commit 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support") looks only that simple because
ftrace_caller _and_ ftrace_regs_caller used to save all register
contents into the pt_regs structure, which never was a requirement,
but implicitly fulfills the HAVE_DYNAMIC_FTRACE_WITH_ARGS
requirements.
Not sure if powerpc passes enough register contents via pt_regs for
HAVE_DYNAMIC_FTRACE_WITH_ARGS though. Might be something to check?
In fact there is no need to rework the function graph logic. It still
works as is with HAVE_DYNAMIC_FTRACE_WITH_ARGS.
The problem was that the sefltests were failing with
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS not being selected on powerpc.
As s390 selects CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, there is no
problem.
Thanks
Christophe
On Tue, 14 Dec 2021 08:35:14 +0100
Christophe Leroy [off-list ref] wrote:
quoted
quoted
Will continue investigating.
trace_selftest_startup_function_graph() calls register_ftrace_direct()
which returns -ENOSUPP because powerpc doesn't select
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?
Yes, that should be:
#if defined(CONFIG_DYNAMIC_FTRACE) && \
defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS)
#define TEST_DIRECT_TRAMP
noinline __noclone static void trace_direct_tramp(void) { }
#endif
And make it test it with or without the args.
Shouldn't it just be:
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
Because
register_ftrace_direct() depends on that symbol, so if you have
CONFIG_DYNAMIC_FTRACE && CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
but not DYNAMIC_FTRACE_WITH_REGS then
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is unset and
register_ftrace_direct() returns -ENOTSUPP
Christophe