Steven Rostedt wrote:
On Mon, 7 Feb 2022 12:37:21 +0530
"Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> wrote:
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -1137,3 +1137,14 @@ char *arch_ftrace_match_adjust(char *str, const char *search)
return str;
}
#endif /* PPC64_ELF_ABI_v1 */
+
+#ifdef CONFIG_MPROFILE_KERNEL
+unsigned long ftrace_location_lookup(unsigned long ip)
+{
+ /*
+ * Per livepatch.h, ftrace location is always within the first
+ * 16 bytes of a function on powerpc with -mprofile-kernel.
+ */
+ return ftrace_location_range(ip, ip + 16);
I think this is the wrong approach for the implementation and error prone.
+}
+#endif
--
What I believe is a safer approach is to use the record address and add the
range to it.
That is, you know that the ftrace modification site is a range (multiple
instructions), where in the ftrace infrastructure, only one ip represents
that range. What you want is if you pass in an ip, and that ip is within
that range, you return the ip that represents that range to ftrace.
It looks like we need to change the compare function in the bsearch.
Perhaps add a new macro to define the size of the range to be searched,
instead of just using MCOUNT_INSN_SIZE? Then we may not even need this new
lookup function?
static int ftrace_cmp_recs(const void *a, const void *b)
{
const struct dyn_ftrace *key = a;
const struct dyn_ftrace *rec = b;
if (key->flags < rec->ip)
return -1;
if (key->ip >= rec->ip + ARCH_IP_SIZE)
return 1;
return 0;
}
Where ARCH_IP_SIZE is defined to MCOUNT_INSN_SIZE by default, but an arch
could define it to something else, like 16.
Would that work for you, or am I missing something?
Yes, I hadn't realized that [un]register_ftrace_direct() and
modify_ftrace_direct() internally lookup the correct ftrace location,
and act on that. So, changing ftrace_cmp_recs() does look like it will
work well for powerpc. Thanks for this suggestion.
However, I think we will not be able to use a fixed range. I would like
to reserve instructions from function entry till the branch to
_mcount(), and it can be two or four instructions depending on whether a
function has a global entry point. For this, I am considering adding a
field in 'struct dyn_arch_ftrace', and a hook in ftrace_process_locs()
to initialize the same. I may need to override ftrace_cmp_recs().
Thanks,
- Naveen
