Messages in this thread | | | Date | Fri, 9 Apr 2021 16:33:09 +0100 | From | Qais Yousef <> | Subject | Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE |
| |
Adding Linus Walleij back to CC
On 03/30/21 13:40, Alexander A Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@nokia.com> > > FTRACE's function tracer currently doesn't always work on ARM with > MODULE_PLT option enabled. If the module is loaded too far, FTRACE's > code modifier cannot cope with introduced veneers and turns the > function tracer off globally. > > ARM64 already has a solution for the problem, refer to the following > patches: > > arm64: ftrace: emit ftrace-mod.o contents through code > arm64: module-plts: factor out PLT generation code for ftrace > arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels > arm64: ftrace: fix building without CONFIG_MODULES > arm64: ftrace: add support for far branches to dynamic ftrace > arm64: ftrace: don't validate branch via PLT in ftrace_make_nop() > > But the presented ARM variant has just a half of the footprint in terms of > the changed LoCs. It also retains the code validation-before-modification > instead of switching it off. > > Changelog: > v8: > * Add warn suppress parameter to arm_gen_branch_link()
I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't work out for you? I struggle to see how this is better and why it was hard to incorporate my suggestion.
For example
- old = ftrace_call_replace(ip, adjust_address(rec, addr)); +#ifdef CONFIG_ARM_MODULE_PLTS + /* mod is only supplied during module loading */ + if (!mod) + mod = rec->arch.mod; + else + rec->arch.mod = mod; +#endif + + old = ftrace_call_replace(ip, aaddr, + !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || !mod); +#ifdef CONFIG_ARM_MODULE_PLTS + if (!old && mod) { + aaddr = get_module_plt(mod, ip, aaddr); + old = ftrace_call_replace(ip, aaddr, true); + } +#endif +
There's an ifdef, followed by a code that embeds !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/
And there was no need to make the new warn arg visible all the way to ftrace_call_repalce() and all of its users.
FWIW
Tested-by: Qais Yousef <qais.yousef@arm.com>
If this gets accepted as-is, I'll send a patch to improve on this.
Thanks
-- Qais Yousef
| |