Hi Jisheng, Would you be still working on this series? If you are still want to put a probe on func+4, it is OK if you can completely emulate the 1st instruction. (lr save on the stack and change the regs->sp) Thank you, On Thu, 26 Dec 2019 18:26:07 +0900 Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > On Thu, 26 Dec 2019 04:25:24 +0000 > Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx> wrote: > > > > > > +/* > > > > > + * In arm64 FTRACE_WITH_REGS implementation, we patch two nop instructions: > > > > > + * the lr saver and bl ftrace-entry. Both these instructions are claimed > > > > > + * by ftrace and we should allow probing on either instruction. > > > > > > > > No, the 2nd bl ftrace-entry must not be probed. > > > > The pair of lr-saver and bl ftrace-entry is tightly coupled. You can not > > > > decouple it. > > > > > > This is the key. different viewing of this results in different implementation. > > > I'm just wondering why are the two instructions considered as coupled. I think > > > here we met similar situation as powerpc: https://lkml.org/lkml/2019/6/18/646 > > > the "mflr r0" equals to lr-saver here, branch to _mcount equals to bl ftrace-entry > > > could you please kindly comment more? > > > > > > Thanks in advance > > > > > > > hmm, I think I may get some part of your opinion. In v7 implementation: > > > > if probe on func+4, that's bl ftrace-entry, similar as mcount call on > > other architectures, we allow this probe as normal. > > > > if probe on func+0, the first param ip in kprobe_ftrace_handler() points > > to func+4(this is adjusted by ftrace), regs->ip points to func+8, so in > > kprobe_ftrace_handler() we modify regs->ip to func+0 to call kprobe > > pre handler, then modify regs->ip to func+8 to call kprobe post handler. > > As can be seen, the first two instructions are considered as a virtual > > mcount call. From this point of view, lr saver and the bl <ftrace-entry> > > is coupled. > > Yes, this is good. But probing on func+4 is meaningless. Both func+0 and > func+4 call a handler with same pt_regs. And it should have the stack > pointer which is NOT modified by lr-saver and regs->lr must point original > call address. (ftrace regs caller must do this fixup for supporting live > patching correctly) > > And in this case, func+4 has fake pt_regs because it skips lr-saver's > effects. > > And even if you fixed up the pt_regs, there is another problem of what > user expects on the target instructions. > > As you know, dynamic ftrace will fill the instruction with NOP (2 NOPs > in arm64), in this case, maybe pt_regs are same except pc on func+0 and > func+4. But if ftrace already enabled on the function, user will see > there are lr-saver and bl, oops. In this case we have to change pt_regs > between func+0 and func+4. So it depends on the current mode. > > However, IMHO, it is not worth to pay such simulation cost. No one want > to probe such simulated intermediate address. It is easy to expect the > result from the code. Moreover, the func+4 will not appear on debuginfo > because those 2 special insturctions are just appended by the compiler, > not generated by the code. > > So I don't think we need to support func+4. We only need func+0, or func+8 > (this must be same as func+0 except regs->pc anyway) > > Thank you, > > > > > If we split patch3 into two: > > one to support kprobes func+4 > > the second to support kprobe on func+0 > > it would be much clearer. > > > > Then the key here is whether we could allow both kprobes on func+0 and func+4 > > > > Thanks > > > -- > Masami Hiramatsu <mhiramat@xxxxxxxxxx> -- Masami Hiramatsu <mhiramat@xxxxxxxxxx>
