These patches are rebased on top of objtool/core
branch of the tip tree, and work only on ppc64le
for now.
Note: With this patch set, there are still some
warnings seen with ppc64le kernel build.
Sathvika Vasireddy (4):
objtool: Add --mnop as an option to --mcount
objtool: Enable objtool to run only on files with ftrace enabled
objtool/powerpc: Enable objtool to be built on ppc
objtool/powerpc: Add --mcount specific implementation
Makefile | 4 +-
arch/powerpc/Kconfig | 2 +
arch/x86/Kconfig | 1 +
scripts/Makefile.build | 5 +-
tools/objtool/arch/powerpc/Build | 2 +
tools/objtool/arch/powerpc/decode.c | 87 +++++++++++++++++++
.../arch/powerpc/include/arch/cfi_regs.h | 11 +++
tools/objtool/arch/powerpc/include/arch/elf.h | 8 ++
.../arch/powerpc/include/arch/endianness.h | 9 ++
.../arch/powerpc/include/arch/special.h | 21 +++++
tools/objtool/arch/powerpc/special.c | 19 ++++
tools/objtool/builtin-check.c | 14 +++
tools/objtool/check.c | 31 ++++---
tools/objtool/elf.c | 13 +++
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/elf.h | 1 +
16 files changed, 212 insertions(+), 17 deletions(-)
create mode 100644 tools/objtool/arch/powerpc/Build
create mode 100644 tools/objtool/arch/powerpc/decode.c
create mode 100644 tools/objtool/arch/powerpc/include/arch/cfi_regs.h
create mode 100644 tools/objtool/arch/powerpc/include/arch/elf.h
create mode 100644 tools/objtool/arch/powerpc/include/arch/endianness.h
create mode 100644 tools/objtool/arch/powerpc/include/arch/special.h
create mode 100644 tools/objtool/arch/powerpc/special.c
--
2.25.1
This patch adds [stub] implementations for required
functions, inorder to enable objtool build on powerpc.
Signed-off-by: Sathvika Vasireddy <[email protected]>
---
arch/powerpc/Kconfig | 1 +
tools/objtool/arch/powerpc/Build | 2 +
tools/objtool/arch/powerpc/decode.c | 73 +++++++++++++++++++
.../arch/powerpc/include/arch/cfi_regs.h | 11 +++
tools/objtool/arch/powerpc/include/arch/elf.h | 8 ++
.../arch/powerpc/include/arch/endianness.h | 9 +++
.../arch/powerpc/include/arch/special.h | 21 ++++++
tools/objtool/arch/powerpc/special.c | 19 +++++
8 files changed, 144 insertions(+)
create mode 100644 tools/objtool/arch/powerpc/Build
create mode 100644 tools/objtool/arch/powerpc/decode.c
create mode 100644 tools/objtool/arch/powerpc/include/arch/cfi_regs.h
create mode 100644 tools/objtool/arch/powerpc/include/arch/elf.h
create mode 100644 tools/objtool/arch/powerpc/include/arch/endianness.h
create mode 100644 tools/objtool/arch/powerpc/include/arch/special.h
create mode 100644 tools/objtool/arch/powerpc/special.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 174edabb74fa..732a3f91ee5e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -232,6 +232,7 @@ config PPC
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
select HAVE_OPTPROBES
+ select HAVE_OBJTOOL if PPC64
select HAVE_PERF_EVENTS
select HAVE_PERF_EVENTS_NMI if PPC64
select HAVE_PERF_REGS
diff --git a/tools/objtool/arch/powerpc/Build b/tools/objtool/arch/powerpc/Build
new file mode 100644
index 000000000000..d24d5636a5b8
--- /dev/null
+++ b/tools/objtool/arch/powerpc/Build
@@ -0,0 +1,2 @@
+objtool-y += decode.o
+objtool-y += special.o
diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
new file mode 100644
index 000000000000..e3b77a6ce357
--- /dev/null
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <objtool/check.h>
+#include <objtool/elf.h>
+#include <objtool/arch.h>
+#include <objtool/warn.h>
+#include <objtool/builtin.h>
+
+unsigned long arch_dest_reloc_offset(int addend)
+{
+ return addend;
+}
+
+bool arch_callee_saved_reg(unsigned char reg)
+{
+ return false;
+}
+
+int arch_decode_hint_reg(u8 sp_reg, int *base)
+{
+ return 0;
+}
+
+const char *arch_nop_insn(int len)
+{
+ return NULL;
+}
+
+const char *arch_ret_insn(int len)
+{
+ return NULL;
+}
+
+int arch_decode_instruction(struct objtool_file *file, const struct section *sec,
+ unsigned long offset, unsigned int maxlen,
+ unsigned int *len, enum insn_type *type,
+ unsigned long *immediate,
+ struct list_head *ops_list)
+{
+ u32 insn;
+
+ *immediate = 0;
+ memcpy(&insn, sec->data->d_buf+offset, 4);
+ *len = 4;
+ *type = INSN_OTHER;
+
+ return 0;
+}
+
+unsigned long arch_jump_destination(struct instruction *insn)
+{
+ return insn->offset + insn->immediate;
+}
+
+void arch_initial_func_cfi_state(struct cfi_init_state *state)
+{
+ int i;
+
+ for (i = 0; i < CFI_NUM_REGS; i++) {
+ state->regs[i].base = CFI_UNDEFINED;
+ state->regs[i].offset = 0;
+ }
+
+ /* initial CFA (call frame address) */
+ state->cfa.base = CFI_SP;
+ state->cfa.offset = 0;
+
+ /* initial LR (return address) */
+ state->regs[CFI_RA].base = CFI_CFA;
+ state->regs[CFI_RA].offset = 0;
+}
diff --git a/tools/objtool/arch/powerpc/include/arch/cfi_regs.h b/tools/objtool/arch/powerpc/include/arch/cfi_regs.h
new file mode 100644
index 000000000000..59638ebeafc8
--- /dev/null
+++ b/tools/objtool/arch/powerpc/include/arch/cfi_regs.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _OBJTOOL_CFI_REGS_H
+#define _OBJTOOL_CFI_REGS_H
+
+#define CFI_BP 1
+#define CFI_SP CFI_BP
+#define CFI_RA 32
+#define CFI_NUM_REGS 33
+
+#endif
diff --git a/tools/objtool/arch/powerpc/include/arch/elf.h b/tools/objtool/arch/powerpc/include/arch/elf.h
new file mode 100644
index 000000000000..3c8ebb7d2a6b
--- /dev/null
+++ b/tools/objtool/arch/powerpc/include/arch/elf.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _OBJTOOL_ARCH_ELF
+#define _OBJTOOL_ARCH_ELF
+
+#define R_NONE R_PPC_NONE
+
+#endif /* _OBJTOOL_ARCH_ELF */
diff --git a/tools/objtool/arch/powerpc/include/arch/endianness.h b/tools/objtool/arch/powerpc/include/arch/endianness.h
new file mode 100644
index 000000000000..7c362527da20
--- /dev/null
+++ b/tools/objtool/arch/powerpc/include/arch/endianness.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _ARCH_ENDIANNESS_H
+#define _ARCH_ENDIANNESS_H
+
+#include <endian.h>
+
+#define __TARGET_BYTE_ORDER __LITTLE_ENDIAN
+
+#endif /* _ARCH_ENDIANNESS_H */
diff --git a/tools/objtool/arch/powerpc/include/arch/special.h b/tools/objtool/arch/powerpc/include/arch/special.h
new file mode 100644
index 000000000000..ffef9ada7133
--- /dev/null
+++ b/tools/objtool/arch/powerpc/include/arch/special.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _PPC_ARCH_SPECIAL_H
+#define _PPC_ARCH_SPECIAL_H
+
+#define EX_ENTRY_SIZE 8
+#define EX_ORIG_OFFSET 0
+#define EX_NEW_OFFSET 4
+
+#define JUMP_ENTRY_SIZE 16
+#define JUMP_ORIG_OFFSET 0
+#define JUMP_NEW_OFFSET 4
+#define JUMP_KEY_OFFSET 8
+
+#define ALT_ENTRY_SIZE 12
+#define ALT_ORIG_OFFSET 0
+#define ALT_NEW_OFFSET 4
+#define ALT_FEATURE_OFFSET 8
+#define ALT_ORIG_LEN_OFFSET 10
+#define ALT_NEW_LEN_OFFSET 11
+
+#endif /* _PPC_ARCH_SPECIAL_H */
diff --git a/tools/objtool/arch/powerpc/special.c b/tools/objtool/arch/powerpc/special.c
new file mode 100644
index 000000000000..e3e75cbab858
--- /dev/null
+++ b/tools/objtool/arch/powerpc/special.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <string.h>
+
+#include <objtool/special.h>
+#include <objtool/builtin.h>
+
+
+bool arch_support_alt_relocation(struct special_alt *special_alt,
+ struct instruction *insn,
+ struct reloc *reloc)
+{
+ return false;
+}
+
+struct reloc *arch_find_switch_table(struct objtool_file *file,
+ struct instruction *insn)
+{
+ return NULL;
+}
--
2.25.1
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 <[email protected]>
---
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;
--
2.25.1
This patch enables objtool --mcount on powerpc, and
adds implementation specific to powerpc.
Signed-off-by: Sathvika Vasireddy <[email protected]>
---
arch/powerpc/Kconfig | 1 +
tools/objtool/arch/powerpc/decode.c | 14 ++++++++++++++
tools/objtool/check.c | 12 +++++++-----
tools/objtool/elf.c | 13 +++++++++++++
tools/objtool/include/objtool/elf.h | 1 +
5 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 732a3f91ee5e..3373d44a1298 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -233,6 +233,7 @@ config PPC
select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
select HAVE_OPTPROBES
select HAVE_OBJTOOL if PPC64
+ select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
select HAVE_PERF_EVENTS
select HAVE_PERF_EVENTS_NMI if PPC64
select HAVE_PERF_REGS
diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
index e3b77a6ce357..ad3d79fffac2 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -40,12 +40,26 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
struct list_head *ops_list)
{
u32 insn;
+ unsigned int opcode;
*immediate = 0;
memcpy(&insn, sec->data->d_buf+offset, 4);
*len = 4;
*type = INSN_OTHER;
+ opcode = (insn >> 26);
+
+ switch (opcode) {
+ case 18: /* bl */
+ if ((insn & 3) == 1) {
+ *type = INSN_CALL;
+ *immediate = insn & 0x3fffffc;
+ if (*immediate & 0x2000000)
+ *immediate -= 0x4000000;
+ }
+ break;
+ }
+
return 0;
}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 056302d58e23..fd8bad092f89 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -832,7 +832,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
if (elf_add_reloc_to_insn(file->elf, sec,
idx * sizeof(unsigned long),
- R_X86_64_64,
+ elf_reloc_type_long(file->elf),
insn->sec, insn->offset))
return -1;
@@ -2183,7 +2183,7 @@ static int classify_symbols(struct objtool_file *file)
if (arch_is_retpoline(func))
func->retpoline_thunk = true;
- if (!strcmp(func->name, "__fentry__"))
+ if ((!strcmp(func->name, "__fentry__")) || (!strcmp(func->name, "_mcount")))
func->fentry = true;
if (is_profiling_func(func->name))
@@ -2259,9 +2259,11 @@ static int decode_sections(struct objtool_file *file)
* Must be before add_jump_destinations(), which depends on 'func'
* being set for alternatives, to enable proper sibling call detection.
*/
- ret = add_special_section_alts(file);
- if (ret)
- return ret;
+ if (opts.stackval || opts.orc || opts.uaccess || opts.noinstr) {
+ ret = add_special_section_alts(file);
+ if (ret)
+ return ret;
+ }
ret = add_jump_destinations(file);
if (ret)
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index c25e957c1e52..95763060d551 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -793,6 +793,19 @@ elf_create_section_symbol(struct elf *elf, struct section *sec)
return sym;
}
+int elf_reloc_type_long(struct elf *elf)
+{
+ switch (elf->ehdr.e_machine) {
+ case EM_X86_64:
+ return R_X86_64_64;
+ case EM_PPC64:
+ return R_PPC64_ADDR64;
+ default:
+ WARN("unknown machine...");
+ exit(-1);
+ }
+}
+
int elf_add_reloc_to_insn(struct elf *elf, struct section *sec,
unsigned long offset, unsigned int type,
struct section *insn_sec, unsigned long insn_off)
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index adebfbc2b518..c43e23c0b3c8 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -144,6 +144,7 @@ static inline bool has_multiple_files(struct elf *elf)
struct elf *elf_open_read(const char *name, int flags);
struct section *elf_create_section(struct elf *elf, const char *name, unsigned int sh_flags, size_t entsize, int nr);
+int elf_reloc_type_long(struct elf *elf);
int elf_add_reloc(struct elf *elf, struct section *sec, unsigned long offset,
unsigned int type, struct symbol *sym, s64 addend);
int elf_add_reloc_to_insn(struct elf *elf, struct section *sec,
--
2.25.1
Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit :
> This patch enables objtool --mcount on powerpc, and
> adds implementation specific to powerpc.
>
> Signed-off-by: Sathvika Vasireddy <[email protected]>
> ---
> arch/powerpc/Kconfig | 1 +
> tools/objtool/arch/powerpc/decode.c | 14 ++++++++++++++
> tools/objtool/check.c | 12 +++++++-----
> tools/objtool/elf.c | 13 +++++++++++++
> tools/objtool/include/objtool/elf.h | 1 +
> 5 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 732a3f91ee5e..3373d44a1298 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -233,6 +233,7 @@ config PPC
> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
> select HAVE_OPTPROBES
> select HAVE_OBJTOOL if PPC64
> + select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
> select HAVE_PERF_EVENTS
> select HAVE_PERF_EVENTS_NMI if PPC64
> select HAVE_PERF_REGS
> diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
> index e3b77a6ce357..ad3d79fffac2 100644
> --- a/tools/objtool/arch/powerpc/decode.c
> +++ b/tools/objtool/arch/powerpc/decode.c
> @@ -40,12 +40,26 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
> struct list_head *ops_list)
> {
> u32 insn;
> + unsigned int opcode;
>
> *immediate = 0;
> memcpy(&insn, sec->data->d_buf+offset, 4);
> *len = 4;
> *type = INSN_OTHER;
>
> + opcode = (insn >> 26);
You dont need the brackets here.
> +
> + switch (opcode) {
> + case 18: /* bl */
> + if ((insn & 3) == 1) {
> + *type = INSN_CALL;
> + *immediate = insn & 0x3fffffc;
> + if (*immediate & 0x2000000)
> + *immediate -= 0x4000000;
> + }
> + break;
> + }
> +
> return 0;
> }
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 056302d58e23..fd8bad092f89 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -832,7 +832,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
>
> if (elf_add_reloc_to_insn(file->elf, sec,
> idx * sizeof(unsigned long),
> - R_X86_64_64,
> + elf_reloc_type_long(file->elf),
> insn->sec, insn->offset))
> return -1;
>
> @@ -2183,7 +2183,7 @@ static int classify_symbols(struct objtool_file *file)
> if (arch_is_retpoline(func))
> func->retpoline_thunk = true;
>
> - if (!strcmp(func->name, "__fentry__"))
> + if ((!strcmp(func->name, "__fentry__")) || (!strcmp(func->name, "_mcount")))
> func->fentry = true;
>
> if (is_profiling_func(func->name))
> @@ -2259,9 +2259,11 @@ static int decode_sections(struct objtool_file *file)
> * Must be before add_jump_destinations(), which depends on 'func'
> * being set for alternatives, to enable proper sibling call detection.
> */
> - ret = add_special_section_alts(file);
> - if (ret)
> - return ret;
> + if (opts.stackval || opts.orc || opts.uaccess || opts.noinstr) {
> + ret = add_special_section_alts(file);
> + if (ret)
> + return ret;
> + }
I think this change should be a patch by itself, it's not related to
powerpc.
>
> ret = add_jump_destinations(file);
> if (ret)
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index c25e957c1e52..95763060d551 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -793,6 +793,19 @@ elf_create_section_symbol(struct elf *elf, struct section *sec)
> return sym;
> }
>
> +int elf_reloc_type_long(struct elf *elf)
Not sure it's a good name, because for 32 bits we have to use 'int'.
> +{
> + switch (elf->ehdr.e_machine) {
> + case EM_X86_64:
> + return R_X86_64_64;
> + case EM_PPC64:
> + return R_PPC64_ADDR64;
> + default:
> + WARN("unknown machine...");
> + exit(-1);
> + }
> +}
Wouldn't it be better to make that function arch specific ?
> +
> int elf_add_reloc_to_insn(struct elf *elf, struct section *sec,
> unsigned long offset, unsigned int type,
> struct section *insn_sec, unsigned long insn_off)
> diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
> index adebfbc2b518..c43e23c0b3c8 100644
> --- a/tools/objtool/include/objtool/elf.h
> +++ b/tools/objtool/include/objtool/elf.h
> @@ -144,6 +144,7 @@ static inline bool has_multiple_files(struct elf *elf)
> struct elf *elf_open_read(const char *name, int flags);
> struct section *elf_create_section(struct elf *elf, const char *name, unsigned int sh_flags, size_t entsize, int nr);
>
> +int elf_reloc_type_long(struct elf *elf);
> int elf_add_reloc(struct elf *elf, struct section *sec, unsigned long offset,
> unsigned int type, struct symbol *sym, s64 addend);
> int elf_add_reloc_to_insn(struct elf *elf, struct section *sec,
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
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
On 24/05/22 15:05, Christophe Leroy wrote:
>
> Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit :
>> This patch enables objtool --mcount on powerpc, and
>> adds implementation specific to powerpc.
>>
>> Signed-off-by: Sathvika Vasireddy <[email protected]>
>> ---
>> arch/powerpc/Kconfig | 1 +
>> tools/objtool/arch/powerpc/decode.c | 14 ++++++++++++++
>> tools/objtool/check.c | 12 +++++++-----
>> tools/objtool/elf.c | 13 +++++++++++++
>> tools/objtool/include/objtool/elf.h | 1 +
>> 5 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 732a3f91ee5e..3373d44a1298 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -233,6 +233,7 @@ config PPC
>> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
>> select HAVE_OPTPROBES
>> select HAVE_OBJTOOL if PPC64
>> + select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
>> select HAVE_PERF_EVENTS
>> select HAVE_PERF_EVENTS_NMI if PPC64
>> select HAVE_PERF_REGS
>> diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
>> index e3b77a6ce357..ad3d79fffac2 100644
>> --- a/tools/objtool/arch/powerpc/decode.c
>> +++ b/tools/objtool/arch/powerpc/decode.c
>> @@ -40,12 +40,26 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
>> struct list_head *ops_list)
>> {
>> u32 insn;
>> + unsigned int opcode;
>>
>> *immediate = 0;
>> memcpy(&insn, sec->data->d_buf+offset, 4);
>> *len = 4;
>> *type = INSN_OTHER;
>>
>> + opcode = (insn >> 26);
> You dont need the brackets here.
>
>> +
>> + switch (opcode) {
>> + case 18: /* bl */
>> + if ((insn & 3) == 1) {
>> + *type = INSN_CALL;
>> + *immediate = insn & 0x3fffffc;
>> + if (*immediate & 0x2000000)
>> + *immediate -= 0x4000000;
>> + }
>> + break;
>> + }
>> +
>> return 0;
>> }
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 056302d58e23..fd8bad092f89 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -832,7 +832,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
>>
>> if (elf_add_reloc_to_insn(file->elf, sec,
>> idx * sizeof(unsigned long),
>> - R_X86_64_64,
>> + elf_reloc_type_long(file->elf),
>> insn->sec, insn->offset))
>> return -1;
>>
>> @@ -2183,7 +2183,7 @@ static int classify_symbols(struct objtool_file *file)
>> if (arch_is_retpoline(func))
>> func->retpoline_thunk = true;
>>
>> - if (!strcmp(func->name, "__fentry__"))
>> + if ((!strcmp(func->name, "__fentry__")) || (!strcmp(func->name, "_mcount")))
>> func->fentry = true;
>>
>> if (is_profiling_func(func->name))
>> @@ -2259,9 +2259,11 @@ static int decode_sections(struct objtool_file *file)
>> * Must be before add_jump_destinations(), which depends on 'func'
>> * being set for alternatives, to enable proper sibling call detection.
>> */
>> - ret = add_special_section_alts(file);
>> - if (ret)
>> - return ret;
>> + if (opts.stackval || opts.orc || opts.uaccess || opts.noinstr) {
>> + ret = add_special_section_alts(file);
>> + if (ret)
>> + return ret;
>> + }
> I think this change should be a patch by itself, it's not related to
> powerpc.
Makes sense. I'll make this a separate patch in the next revision.
>
>>
>> ret = add_jump_destinations(file);
>> if (ret)
>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>> index c25e957c1e52..95763060d551 100644
>> --- a/tools/objtool/elf.c
>> +++ b/tools/objtool/elf.c
>> @@ -793,6 +793,19 @@ elf_create_section_symbol(struct elf *elf, struct section *sec)
>> return sym;
>> }
>>
>> +int elf_reloc_type_long(struct elf *elf)
> Not sure it's a good name, because for 32 bits we have to use 'int'.
Sure, I'll rename it to elf_reloc_type() or some such.
>
>> +{
>> + switch (elf->ehdr.e_machine) {
>> + case EM_X86_64:
>> + return R_X86_64_64;
>> + case EM_PPC64:
>> + return R_PPC64_ADDR64;
>> + default:
>> + WARN("unknown machine...");
>> + exit(-1);
>> + }
>> +}
> Wouldn't it be better to make that function arch specific ?
This is so that we can support cross architecture builds.
Thanks for reviewing!
- Sathvika
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 <[email protected]>
> ---
> 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 23/05/2022 à 19:55, Sathvika Vasireddy a écrit :
> This patch adds [stub] implementations for required
> functions, inorder to enable objtool build on powerpc.
Should we put some exit() or abort() in the stubs that are not supposed
to be used at all ?
>
> Signed-off-by: Sathvika Vasireddy <[email protected]>
> ---
> arch/powerpc/Kconfig | 1 +
> tools/objtool/arch/powerpc/Build | 2 +
> tools/objtool/arch/powerpc/decode.c | 73 +++++++++++++++++++
> .../arch/powerpc/include/arch/cfi_regs.h | 11 +++
> tools/objtool/arch/powerpc/include/arch/elf.h | 8 ++
> .../arch/powerpc/include/arch/endianness.h | 9 +++
> .../arch/powerpc/include/arch/special.h | 21 ++++++
> tools/objtool/arch/powerpc/special.c | 19 +++++
> 8 files changed, 144 insertions(+)
> create mode 100644 tools/objtool/arch/powerpc/Build
> create mode 100644 tools/objtool/arch/powerpc/decode.c
> create mode 100644 tools/objtool/arch/powerpc/include/arch/cfi_regs.h
> create mode 100644 tools/objtool/arch/powerpc/include/arch/elf.h
> create mode 100644 tools/objtool/arch/powerpc/include/arch/endianness.h
> create mode 100644 tools/objtool/arch/powerpc/include/arch/special.h
> create mode 100644 tools/objtool/arch/powerpc/special.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 174edabb74fa..732a3f91ee5e 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -232,6 +232,7 @@ config PPC
> select HAVE_MOD_ARCH_SPECIFIC
> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
> select HAVE_OPTPROBES
> + select HAVE_OBJTOOL if PPC64
> select HAVE_PERF_EVENTS
> select HAVE_PERF_EVENTS_NMI if PPC64
> select HAVE_PERF_REGS
> diff --git a/tools/objtool/arch/powerpc/Build b/tools/objtool/arch/powerpc/Build
> new file mode 100644
> index 000000000000..d24d5636a5b8
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/Build
> @@ -0,0 +1,2 @@
> +objtool-y += decode.o
> +objtool-y += special.o
> diff --git a/tools/objtool/arch/powerpc/decode.c b/tools/objtool/arch/powerpc/decode.c
> new file mode 100644
> index 000000000000..e3b77a6ce357
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/decode.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <objtool/check.h>
> +#include <objtool/elf.h>
> +#include <objtool/arch.h>
> +#include <objtool/warn.h>
> +#include <objtool/builtin.h>
> +
> +unsigned long arch_dest_reloc_offset(int addend)
> +{
> + return addend;
> +}
> +
> +bool arch_callee_saved_reg(unsigned char reg)
> +{
> + return false;
> +}
> +
> +int arch_decode_hint_reg(u8 sp_reg, int *base)
> +{
> + return 0;
> +}
> +
> +const char *arch_nop_insn(int len)
> +{
> + return NULL;
> +}
> +
> +const char *arch_ret_insn(int len)
> +{
> + return NULL;
> +}
> +
> +int arch_decode_instruction(struct objtool_file *file, const struct section *sec,
> + unsigned long offset, unsigned int maxlen,
> + unsigned int *len, enum insn_type *type,
> + unsigned long *immediate,
> + struct list_head *ops_list)
> +{
> + u32 insn;
> +
> + *immediate = 0;
> + memcpy(&insn, sec->data->d_buf+offset, 4);
> + *len = 4;
> + *type = INSN_OTHER;
> +
> + return 0;
> +}
> +
> +unsigned long arch_jump_destination(struct instruction *insn)
> +{
> + return insn->offset + insn->immediate;
> +}
> +
> +void arch_initial_func_cfi_state(struct cfi_init_state *state)
> +{
> + int i;
> +
> + for (i = 0; i < CFI_NUM_REGS; i++) {
> + state->regs[i].base = CFI_UNDEFINED;
> + state->regs[i].offset = 0;
> + }
> +
> + /* initial CFA (call frame address) */
> + state->cfa.base = CFI_SP;
> + state->cfa.offset = 0;
> +
> + /* initial LR (return address) */
> + state->regs[CFI_RA].base = CFI_CFA;
> + state->regs[CFI_RA].offset = 0;
> +}
> diff --git a/tools/objtool/arch/powerpc/include/arch/cfi_regs.h b/tools/objtool/arch/powerpc/include/arch/cfi_regs.h
> new file mode 100644
> index 000000000000..59638ebeafc8
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/include/arch/cfi_regs.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef _OBJTOOL_CFI_REGS_H
> +#define _OBJTOOL_CFI_REGS_H
> +
> +#define CFI_BP 1
> +#define CFI_SP CFI_BP
> +#define CFI_RA 32
> +#define CFI_NUM_REGS 33
> +
> +#endif
> diff --git a/tools/objtool/arch/powerpc/include/arch/elf.h b/tools/objtool/arch/powerpc/include/arch/elf.h
> new file mode 100644
> index 000000000000..3c8ebb7d2a6b
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/include/arch/elf.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef _OBJTOOL_ARCH_ELF
> +#define _OBJTOOL_ARCH_ELF
> +
> +#define R_NONE R_PPC_NONE
> +
> +#endif /* _OBJTOOL_ARCH_ELF */
> diff --git a/tools/objtool/arch/powerpc/include/arch/endianness.h b/tools/objtool/arch/powerpc/include/arch/endianness.h
> new file mode 100644
> index 000000000000..7c362527da20
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/include/arch/endianness.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _ARCH_ENDIANNESS_H
> +#define _ARCH_ENDIANNESS_H
> +
> +#include <endian.h>
> +
> +#define __TARGET_BYTE_ORDER __LITTLE_ENDIAN
> +
> +#endif /* _ARCH_ENDIANNESS_H */
> diff --git a/tools/objtool/arch/powerpc/include/arch/special.h b/tools/objtool/arch/powerpc/include/arch/special.h
> new file mode 100644
> index 000000000000..ffef9ada7133
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/include/arch/special.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _PPC_ARCH_SPECIAL_H
> +#define _PPC_ARCH_SPECIAL_H
> +
> +#define EX_ENTRY_SIZE 8
> +#define EX_ORIG_OFFSET 0
> +#define EX_NEW_OFFSET 4
> +
> +#define JUMP_ENTRY_SIZE 16
> +#define JUMP_ORIG_OFFSET 0
> +#define JUMP_NEW_OFFSET 4
> +#define JUMP_KEY_OFFSET 8
> +
> +#define ALT_ENTRY_SIZE 12
> +#define ALT_ORIG_OFFSET 0
> +#define ALT_NEW_OFFSET 4
> +#define ALT_FEATURE_OFFSET 8
> +#define ALT_ORIG_LEN_OFFSET 10
> +#define ALT_NEW_LEN_OFFSET 11
> +
> +#endif /* _PPC_ARCH_SPECIAL_H */
> diff --git a/tools/objtool/arch/powerpc/special.c b/tools/objtool/arch/powerpc/special.c
> new file mode 100644
> index 000000000000..e3e75cbab858
> --- /dev/null
> +++ b/tools/objtool/arch/powerpc/special.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <string.h>
> +
> +#include <objtool/special.h>
> +#include <objtool/builtin.h>
> +
> +
> +bool arch_support_alt_relocation(struct special_alt *special_alt,
> + struct instruction *insn,
> + struct reloc *reloc)
> +{
> + return false;
> +}
> +
> +struct reloc *arch_find_switch_table(struct objtool_file *file,
> + struct instruction *insn)
> +{
> + return NULL;
> +}
Le 24/05/2022 à 13:00, Sathvika Vasireddy a écrit :
> [Vous ne recevez pas souvent de courriers de la part de
> [email protected]. Découvrez pourquoi cela peut être important à
> l’adresse https://aka.ms/LearnAboutSenderIdentification.]
>
> On 24/05/22 15:05, Christophe Leroy wrote:
>>
>> Le 23/05/2022 à 19:55, Sathvika Vasireddy a écrit :
>>> This patch enables objtool --mcount on powerpc, and
>>> adds implementation specific to powerpc.
>>>
>>> Signed-off-by: Sathvika Vasireddy <[email protected]>
>>> ---
>>> arch/powerpc/Kconfig | 1 +
>>> tools/objtool/arch/powerpc/decode.c | 14 ++++++++++++++
>>> tools/objtool/check.c | 12 +++++++-----
>>> tools/objtool/elf.c | 13 +++++++++++++
>>> tools/objtool/include/objtool/elf.h | 1 +
>>> 5 files changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index 732a3f91ee5e..3373d44a1298 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -233,6 +233,7 @@ config PPC
>>> select HAVE_NMI if PERF_EVENTS || (PPC64
>>> && PPC_BOOK3S)
>>> select HAVE_OPTPROBES
>>> select HAVE_OBJTOOL if PPC64
>>> + select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
>>> select HAVE_PERF_EVENTS
>>> select HAVE_PERF_EVENTS_NMI if PPC64
>>> select HAVE_PERF_REGS
>>> diff --git a/tools/objtool/arch/powerpc/decode.c
>>> b/tools/objtool/arch/powerpc/decode.c
>>> index e3b77a6ce357..ad3d79fffac2 100644
>>> --- a/tools/objtool/arch/powerpc/decode.c
>>> +++ b/tools/objtool/arch/powerpc/decode.c
>>> @@ -40,12 +40,26 @@ int arch_decode_instruction(struct objtool_file
>>> *file, const struct section *sec
>>> struct list_head *ops_list)
>>> {
>>> u32 insn;
>>> + unsigned int opcode;
>>>
>>> *immediate = 0;
>>> memcpy(&insn, sec->data->d_buf+offset, 4);
>>> *len = 4;
>>> *type = INSN_OTHER;
>>>
>>> + opcode = (insn >> 26);
>> You dont need the brackets here.
>>
>>> +
>>> + switch (opcode) {
>>> + case 18: /* bl */
>>> + if ((insn & 3) == 1) {
>>> + *type = INSN_CALL;
>>> + *immediate = insn & 0x3fffffc;
>>> + if (*immediate & 0x2000000)
>>> + *immediate -= 0x4000000;
>>> + }
>>> + break;
>>> + }
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>> index 056302d58e23..fd8bad092f89 100644
>>> --- a/tools/objtool/check.c
>>> +++ b/tools/objtool/check.c
>>> @@ -832,7 +832,7 @@ static int create_mcount_loc_sections(struct
>>> objtool_file *file)
>>>
>>> if (elf_add_reloc_to_insn(file->elf, sec,
>>> idx * sizeof(unsigned long),
>>> - R_X86_64_64,
>>> + elf_reloc_type_long(file->elf),
>>> insn->sec, insn->offset))
>>> return -1;
>>>
>>> @@ -2183,7 +2183,7 @@ static int classify_symbols(struct objtool_file
>>> *file)
>>> if (arch_is_retpoline(func))
>>> func->retpoline_thunk = true;
>>>
>>> - if (!strcmp(func->name, "__fentry__"))
>>> + if ((!strcmp(func->name, "__fentry__")) ||
>>> (!strcmp(func->name, "_mcount")))
>>> func->fentry = true;
>>>
>>> if (is_profiling_func(func->name))
>>> @@ -2259,9 +2259,11 @@ static int decode_sections(struct objtool_file
>>> *file)
>>> * Must be before add_jump_destinations(), which depends on 'func'
>>> * being set for alternatives, to enable proper sibling call
>>> detection.
>>> */
>>> - ret = add_special_section_alts(file);
>>> - if (ret)
>>> - return ret;
>>> + if (opts.stackval || opts.orc || opts.uaccess || opts.noinstr) {
>>> + ret = add_special_section_alts(file);
>>> + if (ret)
>>> + return ret;
>>> + }
>> I think this change should be a patch by itself, it's not related to
>> powerpc.
> Makes sense. I'll make this a separate patch in the next revision.
Great. Can you base your next revision on the one I just sent out ?
I will now start looking at inline static calls for PPC32.
>>
>>>
>>> ret = add_jump_destinations(file);
>>> if (ret)
>>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>>> index c25e957c1e52..95763060d551 100644
>>> --- a/tools/objtool/elf.c
>>> +++ b/tools/objtool/elf.c
>>> @@ -793,6 +793,19 @@ elf_create_section_symbol(struct elf *elf,
>>> struct section *sec)
>>> return sym;
>>> }
>>>
>>> +int elf_reloc_type_long(struct elf *elf)
>> Not sure it's a good name, because for 32 bits we have to use 'int'.
> Sure, I'll rename it to elf_reloc_type() or some such.
>>
>>> +{
>>> + switch (elf->ehdr.e_machine) {
>>> + case EM_X86_64:
>>> + return R_X86_64_64;
>>> + case EM_PPC64:
>>> + return R_PPC64_ADDR64;
>>> + default:
>>> + WARN("unknown machine...");
>>> + exit(-1);
>>> + }
>>> +}
>> Wouldn't it be better to make that function arch specific ?
>
> This is so that we can support cross architecture builds.
>
I'm not sure I follow you here.
This is only based on the target, it doesn't depend on the build host so
I can't the link with cross arch builds.
The same as you have arch_decode_instruction(), you could have
arch_elf_reloc_type_long()
It would make sense indeed, because there is no point in supporting X86
relocation when you don't support X86 instruction decoding.
Christophe
Le 24/05/2022 à 15:33, Christophe Leroy a écrit :
>
>
> Le 24/05/2022 à 13:00, Sathvika Vasireddy a écrit :
>>>
>>>> +{
>>>> + switch (elf->ehdr.e_machine) {
>>>> + case EM_X86_64:
>>>> + return R_X86_64_64;
>>>> + case EM_PPC64:
>>>> + return R_PPC64_ADDR64;
>>>> + default:
>>>> + WARN("unknown machine...");
>>>> + exit(-1);
>>>> + }
>>>> +}
>>> Wouldn't it be better to make that function arch specific ?
>>
>> This is so that we can support cross architecture builds.
>>
>
>
> I'm not sure I follow you here.
>
> This is only based on the target, it doesn't depend on the build host so
> I can't the link with cross arch builds.
>
> The same as you have arch_decode_instruction(), you could have
> arch_elf_reloc_type_long()
> It would make sense indeed, because there is no point in supporting X86
> relocation when you don't support X86 instruction decoding.
>
Could simply be some macro defined in
tools/objtool/arch/powerpc/include/arch/elf.h and
tools/objtool/arch/x86/include/arch/elf.h
The x86 version would be:
#define R_ADDR(elf) R_X86_64_64
And the powerpc version would be:
#define R_ADDR(elf) (elf->ehdr.e_machine == EM_PPC64 ? R_PPC64_ADDR64 :
R_PPC_ADDR32)
Christophe
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
Le 25/05/2022 à 19:27, Christophe Leroy a écrit :
>
>
> Le 24/05/2022 à 15:33, Christophe Leroy a écrit :
>>
>>
>> Le 24/05/2022 à 13:00, Sathvika Vasireddy a écrit :
>>>>
>>>>> +{
>>>>> + switch (elf->ehdr.e_machine) {
>>>>> + case EM_X86_64:
>>>>> + return R_X86_64_64;
>>>>> + case EM_PPC64:
>>>>> + return R_PPC64_ADDR64;
>>>>> + default:
>>>>> + WARN("unknown machine...");
>>>>> + exit(-1);
>>>>> + }
>>>>> +}
>>>> Wouldn't it be better to make that function arch specific ?
>>>
>>> This is so that we can support cross architecture builds.
>>>
>>
>>
>> I'm not sure I follow you here.
>>
>> This is only based on the target, it doesn't depend on the build host so
>> I can't the link with cross arch builds.
>>
>> The same as you have arch_decode_instruction(), you could have
>> arch_elf_reloc_type_long()
>> It would make sense indeed, because there is no point in supporting X86
>> relocation when you don't support X86 instruction decoding.
>>
>
> Could simply be some macro defined in
> tools/objtool/arch/powerpc/include/arch/elf.h and
> tools/objtool/arch/x86/include/arch/elf.h
>
> The x86 version would be:
>
> #define R_ADDR(elf) R_X86_64_64
>
> And the powerpc version would be:
>
> #define R_ADDR(elf) (elf->ehdr.e_machine == EM_PPC64 ? R_PPC64_ADDR64 :
> R_PPC_ADDR32)
>
Well, looking once more, and taking into account the patch from Chen
https://lore.kernel.org/lkml/[email protected]/
It would be easier to just define two macros:
#define R_ABS64 R_PPC64_ADDR64
#define R_ABS32 R_PPC_ADDR32
And then in the caller, as we know the size, do something like
size == sizeof(u64) ? R_ABS64 : R_ABS32;
Christophe
Christophe Leroy wrote:
>
>
> Le 25/05/2022 à 19:27, Christophe Leroy a écrit :
>>
>>
>> Le 24/05/2022 à 15:33, Christophe Leroy a écrit :
>>>
>>>
>>> Le 24/05/2022 à 13:00, Sathvika Vasireddy a écrit :
>>>>>
>>>>>> +{
>>>>>> + switch (elf->ehdr.e_machine) {
>>>>>> + case EM_X86_64:
>>>>>> + return R_X86_64_64;
>>>>>> + case EM_PPC64:
>>>>>> + return R_PPC64_ADDR64;
>>>>>> + default:
>>>>>> + WARN("unknown machine...");
>>>>>> + exit(-1);
>>>>>> + }
>>>>>> +}
>>>>> Wouldn't it be better to make that function arch specific ?
>>>>
>>>> This is so that we can support cross architecture builds.
>>>>
>>>
>>>
>>> I'm not sure I follow you here.
>>>
>>> This is only based on the target, it doesn't depend on the build host so
>>> I can't the link with cross arch builds.
>>>
>>> The same as you have arch_decode_instruction(), you could have
>>> arch_elf_reloc_type_long()
>>> It would make sense indeed, because there is no point in supporting X86
>>> relocation when you don't support X86 instruction decoding.
>>>
>>
>> Could simply be some macro defined in
>> tools/objtool/arch/powerpc/include/arch/elf.h and
>> tools/objtool/arch/x86/include/arch/elf.h
>>
>> The x86 version would be:
>>
>> #define R_ADDR(elf) R_X86_64_64
>>
>> And the powerpc version would be:
>>
>> #define R_ADDR(elf) (elf->ehdr.e_machine == EM_PPC64 ? R_PPC64_ADDR64 :
>> R_PPC_ADDR32)
>>
>
> Well, looking once more, and taking into account the patch from Chen
> https://lore.kernel.org/lkml/[email protected]/
>
> It would be easier to just define two macros:
>
> #define R_ABS64 R_PPC64_ADDR64
> #define R_ABS32 R_PPC_ADDR32
>
> And then in the caller, as we know the size, do something like
>
> size == sizeof(u64) ? R_ABS64 : R_ABS32;
How does 'sizeof(u64)' work here?
- Naveen
Le 16/06/2022 à 15:57, Peter Zijlstra a écrit :
> On Thu, Jun 16, 2022 at 01:40:34PM +0000, Christophe Leroy wrote:
>> sizeof(u64) is always 8 by definition.
>>
>> So if size is 8 we are working on a binary file for a 64 bits target, if
>> not it means we are working for a 32 bits target.
>
> Cross-builds invalidate this I think. Best to look at something like:
>
> elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32
>
>
Yes that's what it does indirectly:
int size = elf_class_size(elf);
With
static inline int elf_class_size(struct elf *elf)
{
if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
return sizeof(u32);
else
return sizeof(u64);
}
On Thu, Jun 16, 2022 at 01:40:34PM +0000, Christophe Leroy wrote:
> sizeof(u64) is always 8 by definition.
>
> So if size is 8 we are working on a binary file for a 64 bits target, if
> not it means we are working for a 32 bits target.
Cross-builds invalidate this I think. Best to look at something like:
elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32
Le 16/06/2022 à 15:34, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>>
>>
>> Le 25/05/2022 à 19:27, Christophe Leroy a écrit :
>>>
>>>
>>> Le 24/05/2022 à 15:33, Christophe Leroy a écrit :
>>>>
>>>>
>>>> Le 24/05/2022 à 13:00, Sathvika Vasireddy a écrit :
>>>>>>
>>>>>>> +{
>>>>>>> + switch (elf->ehdr.e_machine) {
>>>>>>> + case EM_X86_64:
>>>>>>> + return R_X86_64_64;
>>>>>>> + case EM_PPC64:
>>>>>>> + return R_PPC64_ADDR64;
>>>>>>> + default:
>>>>>>> + WARN("unknown machine...");
>>>>>>> + exit(-1);
>>>>>>> + }
>>>>>>> +}
>>>>>> Wouldn't it be better to make that function arch specific ?
>>>>>
>>>>> This is so that we can support cross architecture builds.
>>>>>
>>>>
>>>>
>>>> I'm not sure I follow you here.
>>>>
>>>> This is only based on the target, it doesn't depend on the build
>>>> host so
>>>> I can't the link with cross arch builds.
>>>>
>>>> The same as you have arch_decode_instruction(), you could have
>>>> arch_elf_reloc_type_long()
>>>> It would make sense indeed, because there is no point in supporting X86
>>>> relocation when you don't support X86 instruction decoding.
>>>>
>>>
>>> Could simply be some macro defined in
>>> tools/objtool/arch/powerpc/include/arch/elf.h and
>>> tools/objtool/arch/x86/include/arch/elf.h
>>>
>>> The x86 version would be:
>>>
>>> #define R_ADDR(elf) R_X86_64_64
>>>
>>> And the powerpc version would be:
>>>
>>> #define R_ADDR(elf) (elf->ehdr.e_machine == EM_PPC64 ? R_PPC64_ADDR64
>>> : R_PPC_ADDR32)
>>>
>>
>> Well, looking once more, and taking into account the patch from Chen
>> https://lore.kernel.org/lkml/[email protected]/
>>
>>
>> It would be easier to just define two macros:
>>
>> #define R_ABS64 R_PPC64_ADDR64
>> #define R_ABS32 R_PPC_ADDR32
>>
>> And then in the caller, as we know the size, do something like
>>
>> size == sizeof(u64) ? R_ABS64 : R_ABS32;
>
> How does 'sizeof(u64)' work here?
>
sizeof(u64) is always 8 by definition.
So if size is 8 we are working on a binary file for a 64 bits target, if
not it means we are working for a 32 bits target.
Christophe
Christophe Leroy wrote:
>
>
> Le 16/06/2022 à 15:57, Peter Zijlstra a écrit :
>> On Thu, Jun 16, 2022 at 01:40:34PM +0000, Christophe Leroy wrote:
>>> sizeof(u64) is always 8 by definition.
>>>
>>> So if size is 8 we are working on a binary file for a 64 bits target, if
>>> not it means we are working for a 32 bits target.
>>
>> Cross-builds invalidate this I think. Best to look at something like:
>>
>> elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32
>>
>>
>
> Yes that's what it does indirectly:
>
> int size = elf_class_size(elf);
>
>
> With
>
> static inline int elf_class_size(struct elf *elf)
> {
> if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
> return sizeof(u32);
> else
> return sizeof(u64);
> }
Ok, those come from the below patch:
https://lore.kernel.org/all/c4b06b5b314183d85615765a5ce421a057674bd8.1653398233.git.christophe.leroy@csgroup.eu/T/#u
I guess it would have been clearer if 'size' was named differently:
'addr_size' perhaps?
- Naveen