| Message ID | 20220523175548.922671-2-sv@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | objtool: Enable and implement --mcount option on powerpc | expand |
Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit : > Architectures can select HAVE_NOP_MCOUNT if they choose > to nop out mcount call sites. If that config option is > selected, then --mnop is passed as an option to objtool, > along with --mcount. > Is there a reason not to nop out mcount call sites on powerpc as well ? > Also, make sure that --mnop can be passed as an option > to objtool only when --mcount is passed. > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > Makefile | 4 +++- > arch/x86/Kconfig | 1 + > scripts/Makefile.build | 1 + > tools/objtool/builtin-check.c | 14 ++++++++++++++ > tools/objtool/check.c | 19 ++++++++++--------- > tools/objtool/include/objtool/builtin.h | 1 + > 6 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/Makefile b/Makefile > index 250707647359..acaf88e3c694 100644 > --- a/Makefile > +++ b/Makefile > @@ -851,7 +851,9 @@ ifdef CONFIG_FTRACE_MCOUNT_USE_CC > endif > endif > ifdef CONFIG_FTRACE_MCOUNT_USE_OBJTOOL > - CC_FLAGS_USING += -DCC_USING_NOP_MCOUNT > + ifdef CONFIG_HAVE_NOP_MCOUNT > + CC_FLAGS_USING += -DCC_USING_NOP_MCOUNT > + endif > endif > ifdef CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT > ifdef CONFIG_HAVE_C_RECORDMCOUNT > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 1847d6e974a1..4a41bfb644f0 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -189,6 +189,7 @@ config X86 > select HAVE_CONTEXT_TRACKING_OFFSTACK if HAVE_CONTEXT_TRACKING > select HAVE_C_RECORDMCOUNT > select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL > + select HAVE_NOP_MCOUNT if HAVE_OBJTOOL_MCOUNT > select HAVE_BUILDTIME_MCOUNT_SORT > select HAVE_DEBUG_KMEMLEAK > select HAVE_DMA_CONTIGUOUS > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index ac8167227bc0..2e0c3f9c1459 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -231,6 +231,7 @@ objtool_args = \ > $(if $(CONFIG_HAVE_NOINSTR_HACK), --hacks=noinstr) \ > $(if $(CONFIG_X86_KERNEL_IBT), --ibt) \ > $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \ > + $(if $(CONFIG_HAVE_NOP_MCOUNT), --mnop) \ > $(if $(CONFIG_UNWINDER_ORC), --orc) \ > $(if $(CONFIG_RETPOLINE), --retpoline) \ > $(if $(CONFIG_SLS), --sls) \ > diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c > index f4c3a5091737..b05e2108c0c3 100644 > --- a/tools/objtool/builtin-check.c > +++ b/tools/objtool/builtin-check.c > @@ -80,6 +80,7 @@ const struct option check_options[] = { > OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"), > OPT_BOOLEAN(0, "link", &opts.link, "object is a linked object"), > OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"), > + OPT_BOOLEAN(0, "mnop", &opts.mnop, "nop out mcount call sites"), > OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"), > OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"), > OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"), > @@ -142,6 +143,16 @@ static bool opts_valid(void) > return false; > } > > +static bool mnop_opts_valid(void) > +{ > + if (opts.mnop && !opts.mcount) { > + ERROR("--mnop requires --mcount"); > + return false; > + } > + > + return true; > +} > + > static bool link_opts_valid(struct objtool_file *file) > { > if (opts.link) > @@ -185,6 +196,9 @@ int objtool_run(int argc, const char **argv) > if (!file) > return 1; > > + if (!mnop_opts_valid()) > + return 1; > + > if (!link_opts_valid(file)) > return 1; > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 190b2f6e360a..056302d58e23 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -1175,17 +1175,18 @@ static void annotate_call_site(struct objtool_file *file, > if (opts.mcount && sym->fentry) { > if (sibling) > WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset); > + if (opts.mnop) { > + if (reloc) { > + reloc->type = R_NONE; > + elf_write_reloc(file->elf, reloc); > + } > > - if (reloc) { > - reloc->type = R_NONE; > - elf_write_reloc(file->elf, reloc); > - } > - > - elf_write_insn(file->elf, insn->sec, > - insn->offset, insn->len, > - arch_nop_insn(insn->len)); > + elf_write_insn(file->elf, insn->sec, > + insn->offset, insn->len, > + arch_nop_insn(insn->len)); > > - insn->type = INSN_NOP; > + insn->type = INSN_NOP; > + } > > list_add_tail(&insn->call_node, &file->mcount_loc_list); > return; > diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h > index 280ea18b7f2b..71ed3152a18e 100644 > --- a/tools/objtool/include/objtool/builtin.h > +++ b/tools/objtool/include/objtool/builtin.h > @@ -29,6 +29,7 @@ struct opts { > bool backup; > bool dryrun; > bool link; > + bool mnop; > bool module; > bool no_unreachable; > bool sec_address;
Christophe Leroy wrote: > > > Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit : >> Architectures can select HAVE_NOP_MCOUNT if they choose >> to nop out mcount call sites. If that config option is >> selected, then --mnop is passed as an option to objtool, >> along with --mcount. >> > > Is there a reason not to nop out mcount call sites on powerpc as well ? Yes, if there are functions that are out of range of _mcount(), then the linker would have inserted long branch trampolines. We detect such cases during boot. But, if we nop out the _mcount call sites during build time, we will need some other way to identify these. - Naveen
Le 24/05/2022 à 12:15, Naveen N. Rao a écrit : > Christophe Leroy wrote: >> >> >> Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit : >>> Architectures can select HAVE_NOP_MCOUNT if they choose >>> to nop out mcount call sites. If that config option is >>> selected, then --mnop is passed as an option to objtool, >>> along with --mcount. >>> >> >> Is there a reason not to nop out mcount call sites on powerpc as well ? > > Yes, if there are functions that are out of range of _mcount(), then the > linker would have inserted long branch trampolines. We detect such cases > during boot. But, if we nop out the _mcount call sites during build > time, we will need some other way to identify these. > But does it really matter whether _mcount is reachable or not ? _mcount is never used, and the function we want to call in lieu of _mcount might be reachable while _mcount is not or might be unreachable while _mcount is. Christophe
Christophe Leroy wrote: > > > Le 24/05/2022 à 12:15, Naveen N. Rao a écrit : >> Christophe Leroy wrote: >>> >>> >>> Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit : >>>> Architectures can select HAVE_NOP_MCOUNT if they choose >>>> to nop out mcount call sites. If that config option is >>>> selected, then --mnop is passed as an option to objtool, >>>> along with --mcount. >>>> >>> >>> Is there a reason not to nop out mcount call sites on powerpc as well ? >> >> Yes, if there are functions that are out of range of _mcount(), then the >> linker would have inserted long branch trampolines. We detect such cases >> during boot. But, if we nop out the _mcount call sites during build >> time, we will need some other way to identify these. >> > > But does it really matter whether _mcount is reachable or not ? > > _mcount is never used, and the function we want to call in lieu of > _mcount might be reachable while _mcount is not or might be unreachable > while _mcount is. For the most part, we will end up having to go to ftrace_caller or ftrace_regs_caller, both of which will usually be close to _mcount. We need to know for sure either way. Nop'ing out the _mcount locations at boot allows us to discover existing long branch trampolines. If we want to avoid it, we need to note down those locations during build time. Do you have a different approach in mind? - Naveen
On Tue, May 24, 2022 at 04:01:48PM +0530, Naveen N. Rao wrote: > We need to know for sure either way. Nop'ing out the _mcount locations at > boot allows us to discover existing long branch trampolines. If we want to > avoid it, we need to note down those locations during build time. > > Do you have a different approach in mind? If you put _mcount in a separate section then the compiler cannot tell where it is and is forced to always emit a long branch trampoline. Does that help?
Peter Zijlstra wrote: > On Tue, May 24, 2022 at 04:01:48PM +0530, Naveen N. Rao wrote: > >> We need to know for sure either way. Nop'ing out the _mcount locations at >> boot allows us to discover existing long branch trampolines. If we want to >> avoid it, we need to note down those locations during build time. >> >> Do you have a different approach in mind? > > If you put _mcount in a separate section then the compiler cannot tell > where it is and is forced to always emit a long branch trampoline. > > Does that help? That's an interesting thought. Depending on the type of trampoline the compiler emits, I might be able to use this approach. We will still need objtool on powerpc so that we can note down those trampoline locations. Thanks, Naveen
diff --git a/Makefile b/Makefile index 250707647359..acaf88e3c694 100644 --- a/Makefile +++ b/Makefile @@ -851,7 +851,9 @@ ifdef CONFIG_FTRACE_MCOUNT_USE_CC endif endif ifdef CONFIG_FTRACE_MCOUNT_USE_OBJTOOL - CC_FLAGS_USING += -DCC_USING_NOP_MCOUNT + ifdef CONFIG_HAVE_NOP_MCOUNT + CC_FLAGS_USING += -DCC_USING_NOP_MCOUNT + endif endif ifdef CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT ifdef CONFIG_HAVE_C_RECORDMCOUNT diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 1847d6e974a1..4a41bfb644f0 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -189,6 +189,7 @@ config X86 select HAVE_CONTEXT_TRACKING_OFFSTACK if HAVE_CONTEXT_TRACKING select HAVE_C_RECORDMCOUNT select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL + select HAVE_NOP_MCOUNT if HAVE_OBJTOOL_MCOUNT select HAVE_BUILDTIME_MCOUNT_SORT select HAVE_DEBUG_KMEMLEAK select HAVE_DMA_CONTIGUOUS diff --git a/scripts/Makefile.build b/scripts/Makefile.build index ac8167227bc0..2e0c3f9c1459 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -231,6 +231,7 @@ objtool_args = \ $(if $(CONFIG_HAVE_NOINSTR_HACK), --hacks=noinstr) \ $(if $(CONFIG_X86_KERNEL_IBT), --ibt) \ $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \ + $(if $(CONFIG_HAVE_NOP_MCOUNT), --mnop) \ $(if $(CONFIG_UNWINDER_ORC), --orc) \ $(if $(CONFIG_RETPOLINE), --retpoline) \ $(if $(CONFIG_SLS), --sls) \ diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c index f4c3a5091737..b05e2108c0c3 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -80,6 +80,7 @@ const struct option check_options[] = { OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"), OPT_BOOLEAN(0, "link", &opts.link, "object is a linked object"), OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"), + OPT_BOOLEAN(0, "mnop", &opts.mnop, "nop out mcount call sites"), OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"), OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"), OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"), @@ -142,6 +143,16 @@ static bool opts_valid(void) return false; } +static bool mnop_opts_valid(void) +{ + if (opts.mnop && !opts.mcount) { + ERROR("--mnop requires --mcount"); + return false; + } + + return true; +} + static bool link_opts_valid(struct objtool_file *file) { if (opts.link) @@ -185,6 +196,9 @@ int objtool_run(int argc, const char **argv) if (!file) return 1; + if (!mnop_opts_valid()) + return 1; + if (!link_opts_valid(file)) return 1; diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 190b2f6e360a..056302d58e23 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1175,17 +1175,18 @@ static void annotate_call_site(struct objtool_file *file, if (opts.mcount && sym->fentry) { if (sibling) WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset); + if (opts.mnop) { + if (reloc) { + reloc->type = R_NONE; + elf_write_reloc(file->elf, reloc); + } - if (reloc) { - reloc->type = R_NONE; - elf_write_reloc(file->elf, reloc); - } - - elf_write_insn(file->elf, insn->sec, - insn->offset, insn->len, - arch_nop_insn(insn->len)); + elf_write_insn(file->elf, insn->sec, + insn->offset, insn->len, + arch_nop_insn(insn->len)); - insn->type = INSN_NOP; + insn->type = INSN_NOP; + } list_add_tail(&insn->call_node, &file->mcount_loc_list); return; diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h index 280ea18b7f2b..71ed3152a18e 100644 --- a/tools/objtool/include/objtool/builtin.h +++ b/tools/objtool/include/objtool/builtin.h @@ -29,6 +29,7 @@ struct opts { bool backup; bool dryrun; bool link; + bool mnop; bool module; bool no_unreachable; bool sec_address;
Architectures can select HAVE_NOP_MCOUNT if they choose to nop out mcount call sites. If that config option is selected, then --mnop is passed as an option to objtool, along with --mcount. Also, make sure that --mnop can be passed as an option to objtool only when --mcount is passed. Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> --- Makefile | 4 +++- arch/x86/Kconfig | 1 + scripts/Makefile.build | 1 + tools/objtool/builtin-check.c | 14 ++++++++++++++ tools/objtool/check.c | 19 ++++++++++--------- tools/objtool/include/objtool/builtin.h | 1 + 6 files changed, 30 insertions(+), 10 deletions(-)