| Message ID | 20220829055223.24767-12-sv@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | objtool: Enable and implement --mcount option on powerpc | expand |
Le 29/08/2022 à 07:52, 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. > > 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> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > Makefile | 4 +++- > arch/x86/Kconfig | 1 + > scripts/Makefile.lib | 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 c7705f749601..99dd33d8bcfa 100644 > --- a/Makefile > +++ b/Makefile > @@ -857,7 +857,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 f9920f1341c8..a8dd138df637 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -189,6 +189,7 @@ config X86 > select HAVE_CONTEXT_TRACKING_USER_OFFSTACK if HAVE_CONTEXT_TRACKING_USER > 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.lib b/scripts/Makefile.lib > index 3fb6a99e78c4..0610078e057a 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -234,6 +234,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_RETHUNK), --rethunk) \ > diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c > index 24fbe803a0d3..9bd347d3c244 100644 > --- a/tools/objtool/builtin-check.c > +++ b/tools/objtool/builtin-check.c > @@ -82,6 +82,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"), > @@ -150,6 +151,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) > @@ -198,6 +209,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 0ecf41ee73f0..3cea58f73878 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -1231,17 +1231,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 42a52f1a0add..0785707c5a92 100644 > --- a/tools/objtool/include/objtool/builtin.h > +++ b/tools/objtool/include/objtool/builtin.h > @@ -31,6 +31,7 @@ struct opts { > bool backup; > bool dryrun; > bool link; > + bool mnop; > bool module; > bool no_unreachable; > bool sec_address;
On Mon, Aug 29, 2022 at 11:22:18AM +0530, Sathvika Vasireddy wrote: > 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> IIRC you want to nop the things yourself because PLT entries or something? Anyway, patch looks fine, even though the Changelog doesn't relaly justify the change. Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Peter Zijlstra wrote: > On Mon, Aug 29, 2022 at 11:22:18AM +0530, Sathvika Vasireddy wrote: >> 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> > > IIRC you want to nop the things yourself because PLT entries or > something? For two reasons: 1. We want to maintain a pointer to 'struct module' as part of the ftrace rec so that we can patch ftrace sites in modules to go to module stubs. 2. We depend on compiler generated long branches to support large kernels. > > Anyway, patch looks fine, even though the Changelog doesn't relaly > justify the change. Sure. We should add that powerpc kernel does not support nop'ed out ftrace locations. > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Thanks, Naveen
diff --git a/Makefile b/Makefile index c7705f749601..99dd33d8bcfa 100644 --- a/Makefile +++ b/Makefile @@ -857,7 +857,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 f9920f1341c8..a8dd138df637 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -189,6 +189,7 @@ config X86 select HAVE_CONTEXT_TRACKING_USER_OFFSTACK if HAVE_CONTEXT_TRACKING_USER 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.lib b/scripts/Makefile.lib index 3fb6a99e78c4..0610078e057a 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -234,6 +234,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_RETHUNK), --rethunk) \ diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c index 24fbe803a0d3..9bd347d3c244 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -82,6 +82,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"), @@ -150,6 +151,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) @@ -198,6 +209,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 0ecf41ee73f0..3cea58f73878 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1231,17 +1231,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 42a52f1a0add..0785707c5a92 100644 --- a/tools/objtool/include/objtool/builtin.h +++ b/tools/objtool/include/objtool/builtin.h @@ -31,6 +31,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.lib | 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(-)