Re: [PATCH v7 2/3] arm64: implement ftrace with regs

On 06/02/2019 15:05, Torsten Duwe wrote:

On Wed, Feb 06, 2019 at 08:59:44AM +0000, Julien Thierry wrote:

quoted

Hi Torsten,

On 18/01/2019 16:39, Torsten Duwe wrote:

quoted

--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c

@@ -133,17 +163,45 @@ int ftrace_make_call(struct dyn_ftrace *
 	return ftrace_modify_code(pc, old, new, true);
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+			unsigned long addr)
+{
+	unsigned long pc = rec->ip + REC_IP_BRANCH_OFFSET;
+	u32 old, new;
+
+	old = aarch64_insn_gen_branch_imm(pc, old_addr, true);
+	new = aarch64_insn_gen_branch_imm(pc, addr, true);
+
+	return ftrace_modify_code(pc, old, new, true);
+}
+#endif
+
 /*
  * Turn off the call to ftrace_caller() in instrumented function
  */
 int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 		    unsigned long addr)
 {
-	unsigned long pc = rec->ip;
+	unsigned long pc = rec->ip + REC_IP_BRANCH_OFFSET;

Sorry to come back on this patch again, but I was looking at the ftrace
code a bit, and I see that when processing the ftrace call locations,
ftrace calls ftrace_call_adjust() on every ip registered as mcount
caller (or in our case patchable entries). This ftrace_call_adjust() is
arch specific, so I was thinking we could place the offset in here once
and for all so we don't have to worry about it in the future.

Now that you mention it - yes indeed that's the correct facility to fix
the deviating address, as Steve has also confirmed. I had totally forgotten
about this hook.

quoted

Also, I'm unsure whether it would be safe, but we could patch the "mov
x9, lr" there as well. In theory, this would be called at init time
(before secondary CPUs are brought up) and when loading a module (so I'd
expect no-one is executing that code *yet*.

If this is possible, I think it would make things a bit cleaner.

This is in fact very tempting, but it will introduce a nasty side effect
to ftrace_call_adjust. Is there any obvious documentation that specifies
guarantees about ftrace_call_adjust being called exactly once for each site?

I don't see really much documentation on that function. As far as I can
tell it is only called once for each site (and if it didn't, we'd always
be placing the same instruction, but I agree it wouldn't be nice). It
could depend on how far you can expand the notion of "adjusting" :) .

Steven, do you have an opinion on whether it would be acceptable to
modify function entry code in ftrace_call_adjust() ?

Thanks,

-- 
Julien Thierry

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