linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc
@ 2022-06-24 18:32 Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 01/12] objtool: Fix SEGFAULT Sathvika Vasireddy
                   ` (12 more replies)
  0 siblings, 13 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

These patches are rebased on top of objtool/core
branch of the tip tree, and are tested on
ppc64le with ppc64le_defconfig.

Christophe Leroy (3):
  objtool: Fix SEGFAULT
  objtool: Use target file endianness instead of a compiled constant
  objtool: Use target file class size instead of a compiled constant

Sathvika Vasireddy(9):
  objtool: Add --mnop as an option to --mcount
  powerpc: Skip objtool from running on VDSO files
  objtool: Read special sections with alts only when specific options are selected
  objtool: Use macros to define arch specific reloc types
  objtool: Add arch specific function arch_ftrace_match()
  objtool/powerpc: Enable objtool to be built on ppc
  objtool/powerpc: Add --mcount specific implementation
  powerpc: Remove unreachable() from WARN_ON()
  objtool/powerpc: Fix unannotated intra-function call warnings

 Makefile                                      |  4 +-
 arch/powerpc/Kconfig                          |  2 +
 arch/powerpc/include/asm/bug.h                |  1 -
 arch/powerpc/kernel/entry_64.S                |  2 +
 arch/powerpc/kernel/exceptions-64s.S          |  7 +-
 arch/powerpc/kernel/head_64.S                 |  7 +-
 arch/powerpc/kernel/misc_64.S                 |  4 +-
 arch/powerpc/kernel/vdso/Makefile             |  2 +
 arch/powerpc/kernel/vector.S                  |  4 +-
 arch/powerpc/kvm/book3s_hv_interrupts.S       |  4 +-
 arch/powerpc/kvm/book3s_hv_rmhandlers.S       | 25 +++--
 arch/x86/Kconfig                              |  1 +
 scripts/Makefile.build                        |  1 +
 tools/objtool/arch/powerpc/Build              |  2 +
 tools/objtool/arch/powerpc/decode.c           | 96 +++++++++++++++++++
 .../arch/powerpc/include/arch/cfi_regs.h      | 11 +++
 tools/objtool/arch/powerpc/include/arch/elf.h | 10 ++
 .../arch/powerpc/include/arch/special.h       | 21 ++++
 tools/objtool/arch/powerpc/special.c          | 19 ++++
 tools/objtool/arch/x86/decode.c               |  8 ++
 tools/objtool/arch/x86/include/arch/elf.h     |  2 +
 .../arch/x86/include/arch/endianness.h        |  9 --
 tools/objtool/builtin-check.c                 | 14 +++
 tools/objtool/check.c                         | 51 +++++-----
 tools/objtool/elf.c                           |  8 +-
 tools/objtool/include/objtool/arch.h          |  2 +
 tools/objtool/include/objtool/builtin.h       |  1 +
 tools/objtool/include/objtool/elf.h           |  8 ++
 tools/objtool/include/objtool/endianness.h    | 32 +++----
 tools/objtool/orc_dump.c                      | 11 ++-
 tools/objtool/orc_gen.c                       |  4 +-
 tools/objtool/special.c                       |  3 +-
 32 files changed, 305 insertions(+), 71 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/special.h
 create mode 100644 tools/objtool/arch/powerpc/special.c
 delete mode 100644 tools/objtool/arch/x86/include/arch/endianness.h

-- 
2.25.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 01/12] objtool: Fix SEGFAULT
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-07-08 15:10   ` Christophe Leroy
  2022-06-24 18:32 ` [RFC PATCH v3 02/12] objtool: Use target file endianness instead of a compiled constant Sathvika Vasireddy
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

From: Christophe Leroy <christophe.leroy@csgroup.eu>

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 tools/objtool/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 190b2f6e360a..6cb07e151588 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -203,7 +203,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 		return false;
 
 	insn = find_insn(file, func->sec, func->offset);
-	if (!insn->func)
+	if (!insn || !insn->func)
 		return false;
 
 	func_for_each_insn(file, func, insn) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 02/12] objtool: Use target file endianness instead of a compiled constant
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 01/12] objtool: Fix SEGFAULT Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 03/12] objtool: Use target file class size " Sathvika Vasireddy
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

From: Christophe Leroy <christophe.leroy@csgroup.eu>

Some architectures like powerpc support both endianness, it's
therefore not possible to fix the endianness via arch/endianness.h
because there is no easy way to get the target endianness at
build time.

Use the endianness recorded in the file objtool is working on.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../arch/x86/include/arch/endianness.h        |  9 ------
 tools/objtool/check.c                         |  2 +-
 tools/objtool/include/objtool/endianness.h    | 32 +++++++++----------
 tools/objtool/orc_dump.c                      | 11 +++++--
 tools/objtool/orc_gen.c                       |  4 +--
 tools/objtool/special.c                       |  3 +-
 6 files changed, 30 insertions(+), 31 deletions(-)
 delete mode 100644 tools/objtool/arch/x86/include/arch/endianness.h

diff --git a/tools/objtool/arch/x86/include/arch/endianness.h b/tools/objtool/arch/x86/include/arch/endianness.h
deleted file mode 100644
index 7c362527da20..000000000000
--- a/tools/objtool/arch/x86/include/arch/endianness.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* 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/check.c b/tools/objtool/check.c
index 6cb07e151588..cef1dd54d505 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1971,7 +1971,7 @@ static int read_unwind_hints(struct objtool_file *file)
 			return -1;
 		}
 
-		cfi.cfa.offset = bswap_if_needed(hint->sp_offset);
+		cfi.cfa.offset = bswap_if_needed(file->elf, hint->sp_offset);
 		cfi.type = hint->type;
 		cfi.end = hint->end;
 
diff --git a/tools/objtool/include/objtool/endianness.h b/tools/objtool/include/objtool/endianness.h
index 10241341eff3..4d2aa9b0fe2f 100644
--- a/tools/objtool/include/objtool/endianness.h
+++ b/tools/objtool/include/objtool/endianness.h
@@ -2,33 +2,33 @@
 #ifndef _OBJTOOL_ENDIANNESS_H
 #define _OBJTOOL_ENDIANNESS_H
 
-#include <arch/endianness.h>
 #include <linux/kernel.h>
 #include <endian.h>
-
-#ifndef __TARGET_BYTE_ORDER
-#error undefined arch __TARGET_BYTE_ORDER
-#endif
-
-#if __BYTE_ORDER != __TARGET_BYTE_ORDER
-#define __NEED_BSWAP 1
-#else
-#define __NEED_BSWAP 0
-#endif
+#include <objtool/elf.h>
 
 /*
- * Does a byte swap if target endianness doesn't match the host, i.e. cross
+ * Does a byte swap if target file endianness doesn't match the host, i.e. cross
  * compilation for little endian on big endian and vice versa.
  * To be used for multi-byte values conversion, which are read from / about
  * to be written to a target native endianness ELF file.
  */
-#define bswap_if_needed(val)						\
+static inline bool need_bswap(struct elf *elf)
+{
+	return (__BYTE_ORDER == __LITTLE_ENDIAN) ^
+	       (elf->ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
+}
+
+#define bswap_if_needed(elf, val)					\
 ({									\
 	__typeof__(val) __ret;						\
+	bool __need_bswap = need_bswap(elf);				\
 	switch (sizeof(val)) {						\
-	case 8: __ret = __NEED_BSWAP ? bswap_64(val) : (val); break;	\
-	case 4: __ret = __NEED_BSWAP ? bswap_32(val) : (val); break;	\
-	case 2: __ret = __NEED_BSWAP ? bswap_16(val) : (val); break;	\
+	case 8:								\
+		__ret = __need_bswap ? bswap_64(val) : (val); break;	\
+	case 4:								\
+		__ret = __need_bswap ? bswap_32(val) : (val); break;	\
+	case 2:								\
+		__ret = __need_bswap ? bswap_16(val) : (val); break;	\
 	default:							\
 		BUILD_BUG(); break;					\
 	}								\
diff --git a/tools/objtool/orc_dump.c b/tools/objtool/orc_dump.c
index f5a8508c42d6..4f1211fec82c 100644
--- a/tools/objtool/orc_dump.c
+++ b/tools/objtool/orc_dump.c
@@ -76,6 +76,7 @@ int orc_dump(const char *_objname)
 	GElf_Rela rela;
 	GElf_Sym sym;
 	Elf_Data *data, *symtab = NULL, *rela_orc_ip = NULL;
+	struct elf dummy_elf = {};
 
 
 	objname = _objname;
@@ -94,6 +95,12 @@ int orc_dump(const char *_objname)
 		return -1;
 	}
 
+	if (!elf64_getehdr(elf)) {
+		WARN_ELF("elf64_getehdr");
+		return -1;
+	}
+	memcpy(&dummy_elf.ehdr, elf64_getehdr(elf), sizeof(dummy_elf.ehdr));
+
 	if (elf_getshdrnum(elf, &nr_sections)) {
 		WARN_ELF("elf_getshdrnum");
 		return -1;
@@ -198,11 +205,11 @@ int orc_dump(const char *_objname)
 
 		printf(" sp:");
 
-		print_reg(orc[i].sp_reg, bswap_if_needed(orc[i].sp_offset));
+		print_reg(orc[i].sp_reg, bswap_if_needed(&dummy_elf, orc[i].sp_offset));
 
 		printf(" bp:");
 
-		print_reg(orc[i].bp_reg, bswap_if_needed(orc[i].bp_offset));
+		print_reg(orc[i].bp_reg, bswap_if_needed(&dummy_elf, orc[i].bp_offset));
 
 		printf(" type:%s end:%d\n",
 		       orc_type_name(orc[i].type), orc[i].end);
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index dd3c64af9db2..1f22b7ebae58 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -97,8 +97,8 @@ static int write_orc_entry(struct elf *elf, struct section *orc_sec,
 	/* populate ORC data */
 	orc = (struct orc_entry *)orc_sec->data->d_buf + idx;
 	memcpy(orc, o, sizeof(*orc));
-	orc->sp_offset = bswap_if_needed(orc->sp_offset);
-	orc->bp_offset = bswap_if_needed(orc->bp_offset);
+	orc->sp_offset = bswap_if_needed(elf, orc->sp_offset);
+	orc->bp_offset = bswap_if_needed(elf, orc->bp_offset);
 
 	/* populate reloc for ip */
 	if (elf_add_reloc_to_insn(elf, ip_sec, idx * sizeof(int), R_X86_64_PC32,
diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index e2223dd91c37..9c8d827f69af 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -87,7 +87,8 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 	if (entry->feature) {
 		unsigned short feature;
 
-		feature = bswap_if_needed(*(unsigned short *)(sec->data->d_buf +
+		feature = bswap_if_needed(elf,
+					  *(unsigned short *)(sec->data->d_buf +
 							      offset +
 							      entry->feature));
 		arch_handle_alternative(feature, alt);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 03/12] objtool: Use target file class size instead of a compiled constant
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 01/12] objtool: Fix SEGFAULT Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 02/12] objtool: Use target file endianness instead of a compiled constant Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-07-08 17:35   ` Christophe Leroy
  2022-06-24 18:32 ` [RFC PATCH v3 04/12] objtool: Add --mnop as an option to --mcount Sathvika Vasireddy
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

From: Christophe Leroy <christophe.leroy@csgroup.eu>

In order to allow using objtool on cross-built kernels,
determine size of long from elf data instead of using
sizeof(long) at build time.

For the time being this covers only mcount.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 tools/objtool/check.c               | 16 +++++++++-------
 tools/objtool/elf.c                 |  8 ++++++--
 tools/objtool/include/objtool/elf.h |  8 ++++++++
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index cef1dd54d505..fabc0ea88747 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -802,9 +802,9 @@ static int create_ibt_endbr_seal_sections(struct objtool_file *file)
 static int create_mcount_loc_sections(struct objtool_file *file)
 {
 	struct section *sec;
-	unsigned long *loc;
 	struct instruction *insn;
 	int idx;
+	int size = elf_class_size(file->elf);
 
 	sec = find_section_by_name(file->elf, "__mcount_loc");
 	if (sec) {
@@ -820,23 +820,25 @@ static int create_mcount_loc_sections(struct objtool_file *file)
 	list_for_each_entry(insn, &file->mcount_loc_list, call_node)
 		idx++;
 
-	sec = elf_create_section(file->elf, "__mcount_loc", 0, sizeof(unsigned long), idx);
+	sec = elf_create_section(file->elf, "__mcount_loc", 0, size, idx);
 	if (!sec)
 		return -1;
 
+	sec->sh.sh_addralign = size;
+
 	idx = 0;
 	list_for_each_entry(insn, &file->mcount_loc_list, call_node) {
+		void *loc;
 
-		loc = (unsigned long *)sec->data->d_buf + idx;
-		memset(loc, 0, sizeof(unsigned long));
+		loc = sec->data->d_buf + idx;
+		memset(loc, 0, size);
 
-		if (elf_add_reloc_to_insn(file->elf, sec,
-					  idx * sizeof(unsigned long),
+		if (elf_add_reloc_to_insn(file->elf, sec, idx,
 					  R_X86_64_64,
 					  insn->sec, insn->offset))
 			return -1;
 
-		idx++;
+		idx += size;
 	}
 
 	return 0;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index c25e957c1e52..63218f5799c2 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -1124,6 +1124,7 @@ static struct section *elf_create_rela_reloc_section(struct elf *elf, struct sec
 {
 	char *relocname;
 	struct section *sec;
+	int size = elf_class_size(elf);
 
 	relocname = malloc(strlen(base->name) + strlen(".rela") + 1);
 	if (!relocname) {
@@ -1133,7 +1134,10 @@ static struct section *elf_create_rela_reloc_section(struct elf *elf, struct sec
 	strcpy(relocname, ".rela");
 	strcat(relocname, base->name);
 
-	sec = elf_create_section(elf, relocname, 0, sizeof(GElf_Rela), 0);
+	if (size == sizeof(u32))
+		sec = elf_create_section(elf, relocname, 0, sizeof(Elf32_Rela), 0);
+	else
+		sec = elf_create_section(elf, relocname, 0, sizeof(GElf_Rela), 0);
 	free(relocname);
 	if (!sec)
 		return NULL;
@@ -1142,7 +1146,7 @@ static struct section *elf_create_rela_reloc_section(struct elf *elf, struct sec
 	sec->base = base;
 
 	sec->sh.sh_type = SHT_RELA;
-	sec->sh.sh_addralign = 8;
+	sec->sh.sh_addralign = size;
 	sec->sh.sh_link = find_section_by_name(elf, ".symtab")->idx;
 	sec->sh.sh_info = base->idx;
 	sec->sh.sh_flags = SHF_INFO_LINK;
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index adebfbc2b518..c720c4476828 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -141,6 +141,14 @@ static inline bool has_multiple_files(struct elf *elf)
 	return elf->num_files > 1;
 }
 
+static inline int elf_class_size(struct elf *elf)
+{
+	if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
+		return sizeof(u32);
+	else
+		return sizeof(u64);
+}
+
 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);
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 04/12] objtool: Add --mnop as an option to --mcount
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (2 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 03/12] objtool: Use target file class size " Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 05/12] powerpc: Skip objtool from running on VDSO files Sathvika Vasireddy
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

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(-)

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 fabc0ea88747..7f0dc504dd92 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1177,17 +1177,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


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 05/12] powerpc: Skip objtool from running on VDSO files
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (3 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 04/12] objtool: Add --mnop as an option to --mcount Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 06/12] objtool: Read special sections with alts only when specific options are selected Sathvika Vasireddy
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

Do not run objtool on VDSO files, by using
OBJECT_FILES_NON_STANDARD

Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 arch/powerpc/kernel/vdso/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/vdso/Makefile b/arch/powerpc/kernel/vdso/Makefile
index 954974287ee7..853cda220467 100644
--- a/arch/powerpc/kernel/vdso/Makefile
+++ b/arch/powerpc/kernel/vdso/Makefile
@@ -101,3 +101,5 @@ quiet_cmd_vdso64ld_and_check = VDSO64L $@
       cmd_vdso64ld_and_check = $(VDSOCC) $(c_flags) $(CC64FLAGS) -o $@ -Wl,-T$(filter %.lds,$^) $(filter %.o,$^) ; $(cmd_vdso_check)
 quiet_cmd_vdso64as = VDSO64A $@
       cmd_vdso64as = $(VDSOCC) $(a_flags) $(CC64FLAGS) $(AS64FLAGS) -c -o $@ $<
+
+OBJECT_FILES_NON_STANDARD := y
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 06/12] objtool: Read special sections with alts only when specific options are selected
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (4 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 05/12] powerpc: Skip objtool from running on VDSO files Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types Sathvika Vasireddy
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

This patch reads special sections which have alternate
instructions, only when stackval or orc or uaccess or
noinstr options are passed to objtool.

Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 tools/objtool/check.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7f0dc504dd92..98e869721bc4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2261,9 +2261,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)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (5 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 06/12] objtool: Read special sections with alts only when specific options are selected Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-07-04 11:14   ` Peter Zijlstra
  2022-06-24 18:32 ` [RFC PATCH v3 08/12] objtool: Add arch specific function arch_ftrace_match() Sathvika Vasireddy
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

Make relocation types architecture specific.

Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 tools/objtool/arch/x86/include/arch/elf.h | 2 ++
 tools/objtool/check.c                     | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/arch/x86/include/arch/elf.h b/tools/objtool/arch/x86/include/arch/elf.h
index 69cc4264b28a..ac14987cf687 100644
--- a/tools/objtool/arch/x86/include/arch/elf.h
+++ b/tools/objtool/arch/x86/include/arch/elf.h
@@ -2,5 +2,7 @@
 #define _OBJTOOL_ARCH_ELF
 
 #define R_NONE R_X86_64_NONE
+#define R_ABS64 R_X86_64_64
+#define R_ABS32 R_X86_64_32
 
 #endif /* _OBJTOOL_ARCH_ELF */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 98e869721bc4..88f68269860e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -834,7 +834,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
 		memset(loc, 0, size);
 
 		if (elf_add_reloc_to_insn(file->elf, sec, idx,
-					  R_X86_64_64,
+					  size == sizeof(u64) ? R_ABS64 : R_ABS32,
 					  insn->sec, insn->offset))
 			return -1;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 08/12] objtool: Add arch specific function arch_ftrace_match()
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (6 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 09/12] objtool/powerpc: Enable objtool to be built on ppc Sathvika Vasireddy
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

Add architecture specific function to look for
relocation records pointing to arch specific
symbols.

Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 tools/objtool/arch/x86/decode.c      | 8 ++++++++
 tools/objtool/check.c                | 2 +-
 tools/objtool/include/objtool/arch.h | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 8b990a52aada..6a66b0c0dd2c 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -23,6 +23,14 @@
 #include <objtool/builtin.h>
 #include <arch/elf.h>
 
+bool arch_ftrace_match(char *name)
+{
+	if (!strcmp(func->name, "__fentry__"))
+		return true;
+
+	return false;
+}
+
 static int is_x86_64(const struct elf *elf)
 {
 	switch (elf->ehdr.e_machine) {
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 88f68269860e..51b6dcec8d6a 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2185,7 +2185,7 @@ static int classify_symbols(struct objtool_file *file)
 			if (arch_is_retpoline(func))
 				func->retpoline_thunk = true;
 
-			if (!strcmp(func->name, "__fentry__"))
+			if (arch_ftrace_match(func->name))
 				func->fentry = true;
 
 			if (is_profiling_func(func->name))
diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
index 9b19cc304195..99d0c8ba1b69 100644
--- a/tools/objtool/include/objtool/arch.h
+++ b/tools/objtool/include/objtool/arch.h
@@ -69,6 +69,8 @@ struct stack_op {
 
 struct instruction;
 
+bool arch_ftrace_match(char *name);
+
 void arch_initial_func_cfi_state(struct cfi_init_state *state);
 
 int arch_decode_instruction(struct objtool_file *file, const struct section *sec,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 09/12] objtool/powerpc: Enable objtool to be built on ppc
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (7 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 08/12] objtool: Add arch specific function arch_ftrace_match() Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 10/12] objtool/powerpc: Add --mcount specific implementation Sathvika Vasireddy
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

This patch adds [stub] implementations for required
functions, inorder to enable objtool build on powerpc.

Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 arch/powerpc/Kconfig                          |  1 +
 tools/objtool/arch/powerpc/Build              |  2 +
 tools/objtool/arch/powerpc/decode.c           | 74 +++++++++++++++++++
 .../arch/powerpc/include/arch/cfi_regs.h      | 11 +++
 tools/objtool/arch/powerpc/include/arch/elf.h |  8 ++
 .../arch/powerpc/include/arch/special.h       | 21 ++++++
 tools/objtool/arch/powerpc/special.c          | 19 +++++
 7 files changed, 136 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/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..8b6a14680da7
--- /dev/null
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -0,0 +1,74 @@
+// 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>
+#include <objtool/endianness.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)
+{
+	exit(-1);
+}
+
+const char *arch_nop_insn(int len)
+{
+	exit(-1);
+}
+
+const char *arch_ret_insn(int len)
+{
+	exit(-1);
+}
+
+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;
+	insn = bswap_if_needed(file->elf, *(u32 *)(sec->data->d_buf + offset));
+	*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/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..d33868147196
--- /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 <stdlib.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)
+{
+	exit(-1);
+}
+
+struct reloc *arch_find_switch_table(struct objtool_file *file,
+				    struct instruction *insn)
+{
+	exit(-1);
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 10/12] objtool/powerpc: Add --mcount specific implementation
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (8 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 09/12] objtool/powerpc: Enable objtool to be built on ppc Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-24 18:32 ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Sathvika Vasireddy
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

This patch enables objtool --mcount on powerpc, and
adds implementation specific to powerpc.

Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 arch/powerpc/Kconfig                          |  1 +
 tools/objtool/arch/powerpc/decode.c           | 22 +++++++++++++++++++
 tools/objtool/arch/powerpc/include/arch/elf.h |  2 ++
 3 files changed, 25 insertions(+)

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 8b6a14680da7..06fc0206bf8e 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -9,6 +9,14 @@
 #include <objtool/builtin.h>
 #include <objtool/endianness.h>
 
+bool arch_ftrace_match(char *name)
+{
+	if ((!strcmp(name, "_mcount")) || (!strcmp(name, "._mcount")))
+		return true;
+
+	return false;
+}
+
 unsigned long arch_dest_reloc_offset(int addend)
 {
 	return addend;
@@ -41,12 +49,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;
 	insn = bswap_if_needed(file->elf, *(u32 *)(sec->data->d_buf + offset));
 	*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/arch/powerpc/include/arch/elf.h b/tools/objtool/arch/powerpc/include/arch/elf.h
index 3c8ebb7d2a6b..73f9ae172fe5 100644
--- a/tools/objtool/arch/powerpc/include/arch/elf.h
+++ b/tools/objtool/arch/powerpc/include/arch/elf.h
@@ -4,5 +4,7 @@
 #define _OBJTOOL_ARCH_ELF
 
 #define R_NONE R_PPC_NONE
+#define R_ABS64 R_PPC64_ADDR64
+#define R_ABS32 R_PPC_ADDR32
 
 #endif /* _OBJTOOL_ARCH_ELF */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (9 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 10/12] objtool/powerpc: Add --mcount specific implementation Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-06-25  6:46   ` Christophe Leroy
  2022-06-24 18:32 ` [RFC PATCH v3 12/12] objtool/powerpc: Fix unannotated intra-function call warnings Sathvika Vasireddy
  2022-07-08 15:06 ` [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Christophe Leroy
  12 siblings, 1 reply; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

objtool is throwing *unannotated intra-function call*
warnings with a few instructions that are marked
unreachable. Remove unreachable() from WARN_ON()
to fix these warnings, as the codegen remains same
with and without unreachable() in WARN_ON().

Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 arch/powerpc/include/asm/bug.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index ecbae1832de3..df6c11e008b9 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -97,7 +97,6 @@
 	__label__ __label_warn_on;				\
 								\
 	WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
-	unreachable();						\
 								\
 __label_warn_on:						\
 	break;							\
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [RFC PATCH v3 12/12] objtool/powerpc: Fix unannotated intra-function call warnings
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (10 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Sathvika Vasireddy
@ 2022-06-24 18:32 ` Sathvika Vasireddy
  2022-07-08 15:06 ` [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Christophe Leroy
  12 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-24 18:32 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: peterz, linux-kernel, aik, mingo, sv, rostedt, jpoimboe, paulus,
	naveen.n.rao, mbenes

objtool throws unannotated intra-function call warnings
in the following assembly files.

arch/powerpc/kernel/head_64.o: warning: objtool: .text+0x358: unannotated intra-function call
arch/powerpc/kernel/vector.o: warning: objtool: .text+0x53c: unannotated intra-function call
arch/powerpc/kernel/entry_64.o: warning: objtool: .text+0x4: unannotated intra-function call
arch/powerpc/kernel/misc_64.o: warning: objtool: .text+0x64: unannotated intra-function call
arch/powerpc/kvm/book3s_hv_rmhandlers.o: warning: objtool: .text+0x60: unannotated intra-function call
arch/powerpc/kvm/book3s_hv_interrupts.o: warning: objtool: .text+0x6c: unannotated intra-function call

Remove a few of these warnings by putting ANNOTATE_INTRA_FUNCTION_CALL
directive before the call. And, the rest by annotating those functions
with SYM_FUNC_START_LOCAL() and SYM_FUNC_END() macros.

Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
---
 arch/powerpc/kernel/entry_64.S          |  2 ++
 arch/powerpc/kernel/exceptions-64s.S    |  7 +++++--
 arch/powerpc/kernel/head_64.S           |  7 +++++--
 arch/powerpc/kernel/misc_64.S           |  4 +++-
 arch/powerpc/kernel/vector.S            |  4 +++-
 arch/powerpc/kvm/book3s_hv_interrupts.S |  4 +++-
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 25 ++++++++++++++++++-------
 7 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 9581906b5ee9..11249cd6cfc5 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -14,6 +14,7 @@
  *  code, and exception/interrupt return code for PowerPC.
  */
 
+#include <linux/objtool.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <asm/cache.h>
@@ -73,6 +74,7 @@ flush_branch_caches:
 
 	// Flush the link stack
 	.rept 64
+	ANNOTATE_INTRA_FUNCTION_CALL
 	bl	.+4
 	.endr
 	b	1f
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index b66dd6f775a4..14ed17b8fe33 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/linkage.h>
 #include <asm/hw_irq.h>
 #include <asm/exception-64s.h>
 #include <asm/ptrace.h>
@@ -3075,7 +3076,7 @@ CLOSE_FIXED_SECTION(virt_trampolines);
 USE_TEXT_SECTION()
 
 /* MSR[RI] should be clear because this uses SRR[01] */
-enable_machine_check:
+SYM_FUNC_START_LOCAL(enable_machine_check)
 	mflr	r0
 	bcl	20,31,$+4
 0:	mflr	r3
@@ -3087,9 +3088,10 @@ enable_machine_check:
 	RFI_TO_KERNEL
 1:	mtlr	r0
 	blr
+SYM_FUNC_END(enable_machine_check)
 
 /* MSR[RI] should be clear because this uses SRR[01] */
-disable_machine_check:
+SYM_FUNC_START_LOCAL(disable_machine_check)
 	mflr	r0
 	bcl	20,31,$+4
 0:	mflr	r3
@@ -3102,3 +3104,4 @@ disable_machine_check:
 	RFI_TO_KERNEL
 1:	mtlr	r0
 	blr
+SYM_FUNC_END(disable_machine_check)
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 5c5181e8d5f1..d5b01a15cb39 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -18,6 +18,7 @@
  *  variants.
  */
 
+#include <linux/linkage.h>
 #include <linux/threads.h>
 #include <linux/init.h>
 #include <asm/reg.h>
@@ -465,7 +466,7 @@ generic_secondary_common_init:
  * Assumes we're mapped EA == RA if the MMU is on.
  */
 #ifdef CONFIG_PPC_BOOK3S
-__mmu_off:
+SYM_FUNC_START_LOCAL(__mmu_off)
 	mfmsr	r3
 	andi.	r0,r3,MSR_IR|MSR_DR
 	beqlr
@@ -476,6 +477,7 @@ __mmu_off:
 	sync
 	rfid
 	b	.	/* prevent speculative execution */
+SYM_FUNC_END(__mmu_off)
 #endif
 
 
@@ -869,7 +871,7 @@ _GLOBAL(start_secondary_resume)
 /*
  * This subroutine clobbers r11 and r12
  */
-enable_64b_mode:
+SYM_FUNC_START_LOCAL(enable_64b_mode)
 	mfmsr	r11			/* grab the current MSR */
 #ifdef CONFIG_PPC_BOOK3E
 	oris	r11,r11,0x8000		/* CM bit set, we'll set ICM later */
@@ -881,6 +883,7 @@ enable_64b_mode:
 	isync
 #endif
 	blr
+SYM_FUNC_END(enable_64b_mode)
 
 /*
  * This puts the TOC pointer into r2, offset by 0x8000 (as expected
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index d38a019b38e1..372d3dae25af 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -9,6 +9,7 @@
  * PPC64 updates by Dave Engebretsen (engebret@us.ibm.com)
  */
 
+#include <linux/linkage.h>
 #include <linux/sys.h>
 #include <asm/unistd.h>
 #include <asm/errno.h>
@@ -353,7 +354,7 @@ _GLOBAL(kexec_smp_wait)
  *
  * don't overwrite r3 here, it is live for kexec_wait above.
  */
-real_mode:	/* assume normal blr return */
+SYM_FUNC_START_LOCAL(real_mode)	/* assume normal blr return */
 #ifdef CONFIG_PPC_BOOK3E
 	/* Create an identity mapping. */
 	b	kexec_create_tlb
@@ -370,6 +371,7 @@ real_mode:	/* assume normal blr return */
 	mtspr	SPRN_SRR0,r11
 	rfid
 #endif
+SYM_FUNC_END(real_mode)
 
 /*
  * kexec_sequence(newstack, start, image, control, clear_all(),
diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
index 5cc24d8cce94..fb96aed2b5c3 100644
--- a/arch/powerpc/kernel/vector.S
+++ b/arch/powerpc/kernel/vector.S
@@ -9,6 +9,7 @@
 #include <asm/ptrace.h>
 #include <asm/export.h>
 #include <asm/asm-compat.h>
+#include <linux/linkage.h>
 
 /*
  * Load state from memory into VMX registers including VSCR.
@@ -186,7 +187,7 @@ fphalf:
  * Internal routine to enable floating point and set FPSCR to 0.
  * Don't call it from C; it doesn't use the normal calling convention.
  */
-fpenable:
+SYM_FUNC_START_LOCAL(fpenable)
 #ifdef CONFIG_PPC32
 	stwu	r1,-64(r1)
 #else
@@ -203,6 +204,7 @@ fpenable:
 	mffs	fr31
 	MTFSF_L(fr1)
 	blr
+SYM_FUNC_END(fpenable)
 
 fpdisable:
 	mtlr	r12
diff --git a/arch/powerpc/kvm/book3s_hv_interrupts.S b/arch/powerpc/kvm/book3s_hv_interrupts.S
index 59d89e4b154a..c0deeea7eef3 100644
--- a/arch/powerpc/kvm/book3s_hv_interrupts.S
+++ b/arch/powerpc/kvm/book3s_hv_interrupts.S
@@ -9,6 +9,7 @@
  * Authors: Alexander Graf <agraf@suse.de>
  */
 
+#include <linux/linkage.h>
 #include <asm/ppc_asm.h>
 #include <asm/kvm_asm.h>
 #include <asm/reg.h>
@@ -107,7 +108,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
 /*
  * void kvmhv_save_host_pmu(void)
  */
-kvmhv_save_host_pmu:
+SYM_FUNC_START_LOCAL(kvmhv_save_host_pmu)
 BEGIN_FTR_SECTION
 	/* Work around P8 PMAE bug */
 	li	r3, -1
@@ -154,3 +155,4 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	stw	r8, HSTATE_PMC5(r13)
 	stw	r9, HSTATE_PMC6(r13)
 31:	blr
+SYM_FUNC_END(kvmhv_save_host_pmu)
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index d185dee26026..154fa428d98e 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -29,6 +29,8 @@
 #include <asm/asm-compat.h>
 #include <asm/feature-fixups.h>
 #include <asm/cpuidle.h>
+#include <linux/linkage.h>
+#include <linux/objtool.h>
 
 /* Values in HSTATE_NAPPING(r13) */
 #define NAPPING_CEDE	1
@@ -1514,12 +1516,14 @@ kvm_flush_link_stack:
 
 	/* Flush the link stack. On Power8 it's up to 32 entries in size. */
 	.rept 32
+	ANNOTATE_INTRA_FUNCTION_CALL
 	bl	.+4
 	.endr
 
 	/* And on Power9 it's up to 64. */
 BEGIN_FTR_SECTION
 	.rept 32
+	ANNOTATE_INTRA_FUNCTION_CALL
 	bl	.+4
 	.endr
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
@@ -2360,7 +2364,7 @@ hmi_realmode:
  * This routine calls kvmppc_read_intr, a C function, if an external
  * interrupt is pending.
  */
-kvmppc_check_wake_reason:
+SYM_FUNC_START_LOCAL(kvmppc_check_wake_reason)
 	mfspr	r6, SPRN_SRR1
 BEGIN_FTR_SECTION
 	rlwinm	r6, r6, 45-31, 0xf	/* extract wake reason field (P8) */
@@ -2429,6 +2433,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	addi	r1, r1, PPC_MIN_STKFRM
 	mtlr	r0
 	blr
+SYM_FUNC_END(kvmppc_check_wake_reason)
 
 /*
  * Save away FP, VMX and VSX registers.
@@ -2436,7 +2441,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
  * N.B. r30 and r31 are volatile across this function,
  * thus it is not callable from C.
  */
-kvmppc_save_fp:
+SYM_FUNC_START_LOCAL(kvmppc_save_fp)
 	mflr	r30
 	mr	r31,r3
 	mfmsr	r5
@@ -2464,6 +2469,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
 	stw	r6,VCPU_VRSAVE(r31)
 	mtlr	r30
 	blr
+SYM_FUNC_END(kvmppc_save_fp)
 
 /*
  * Load up FP, VMX and VSX registers
@@ -2471,7 +2477,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
  * N.B. r30 and r31 are volatile across this function,
  * thus it is not callable from C.
  */
-kvmppc_load_fp:
+SYM_FUNC_START_LOCAL(kvmppc_load_fp)
 	mflr	r30
 	mr	r31,r4
 	mfmsr	r9
@@ -2500,6 +2506,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
 	mtlr	r30
 	mr	r4,r31
 	blr
+SYM_FUNC_END(kvmppc_load_fp)
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 /*
@@ -2748,7 +2755,7 @@ kvmppc_bad_host_intr:
  *   r9 has a vcpu pointer (in)
  *   r0 is used as a scratch register
  */
-kvmppc_msr_interrupt:
+SYM_FUNC_START_LOCAL(kvmppc_msr_interrupt)
 	rldicl	r0, r11, 64 - MSR_TS_S_LG, 62
 	cmpwi	r0, 2 /* Check if we are in transactional state..  */
 	ld	r11, VCPU_INTR_MSR(r9)
@@ -2757,13 +2764,14 @@ kvmppc_msr_interrupt:
 	li	r0, 1
 1:	rldimi	r11, r0, MSR_TS_S_LG, 63 - MSR_TS_T_LG
 	blr
+SYM_FUNC_END(kvmppc_msr_interrupt)
 
 /*
  * void kvmhv_load_guest_pmu(struct kvm_vcpu *vcpu)
  *
  * Load up guest PMU state.  R3 points to the vcpu struct.
  */
-kvmhv_load_guest_pmu:
+SYM_FUNC_START_LOCAL(kvmhv_load_guest_pmu)
 	mr	r4, r3
 	mflr	r0
 	li	r3, 1
@@ -2813,13 +2821,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	isync
 	mtlr	r0
 	blr
+SYM_FUNC_END(kvmhv_load_guest_pmu)
 
 /*
  * void kvmhv_load_host_pmu(void)
  *
  * Reload host PMU state saved in the PACA by kvmhv_save_host_pmu.
  */
-kvmhv_load_host_pmu:
+SYM_FUNC_START_LOCAL(kvmhv_load_host_pmu)
 	mflr	r0
 	lbz	r4, PACA_PMCINUSE(r13) /* is the host using the PMU? */
 	cmpwi	r4, 0
@@ -2861,6 +2870,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	isync
 	mtlr	r0
 23:	blr
+SYM_FUNC_END(kvmhv_load_host_pmu)
 
 /*
  * void kvmhv_save_guest_pmu(struct kvm_vcpu *vcpu, bool pmu_in_use)
@@ -2868,7 +2878,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
  * Save guest PMU state into the vcpu struct.
  * r3 = vcpu, r4 = full save flag (PMU in use flag set in VPA)
  */
-kvmhv_save_guest_pmu:
+SYM_FUNC_START_LOCAL(kvmhv_save_guest_pmu)
 	mr	r9, r3
 	mr	r8, r4
 BEGIN_FTR_SECTION
@@ -2944,6 +2954,7 @@ BEGIN_FTR_SECTION
 	mtspr	SPRN_MMCRS, r4
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 22:	blr
+SYM_FUNC_END(kvmhv_save_guest_pmu)
 
 /*
  * This works around a hardware bug on POWER8E processors, where
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-24 18:32 ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Sathvika Vasireddy
@ 2022-06-25  6:46   ` Christophe Leroy
  2022-06-27 15:21     ` Sathvika Vasireddy
                       ` (2 more replies)
  0 siblings, 3 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-06-25  6:46 UTC (permalink / raw)
  To: Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes



Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
> objtool is throwing *unannotated intra-function call*
> warnings with a few instructions that are marked
> unreachable. Remove unreachable() from WARN_ON()
> to fix these warnings, as the codegen remains same
> with and without unreachable() in WARN_ON().

Did you try the two exemples described in commit 1e688dd2a3d6 
("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with 
asm goto") ?

Without your patch:

00000640 <test>:
  640:	81 23 00 84 	lwz     r9,132(r3)
  644:	71 29 40 00 	andi.   r9,r9,16384
  648:	40 82 00 0c 	bne     654 <test+0x14>
  64c:	80 63 00 0c 	lwz     r3,12(r3)
  650:	4e 80 00 20 	blr
  654:	0f e0 00 00 	twui    r0,0

00000658 <test9w>:
  658:	2c 04 00 00 	cmpwi   r4,0
  65c:	41 82 00 0c 	beq     668 <test9w+0x10>
  660:	7c 63 23 96 	divwu   r3,r3,r4
  664:	4e 80 00 20 	blr
  668:	0f e0 00 00 	twui    r0,0
  66c:	38 60 00 00 	li      r3,0
  670:	4e 80 00 20 	blr


With your patch:

00000640 <test>:
  640:	81 23 00 84 	lwz     r9,132(r3)
  644:	71 29 40 00 	andi.   r9,r9,16384
  648:	40 82 00 0c 	bne     654 <test+0x14>
  64c:	80 63 00 0c 	lwz     r3,12(r3)
  650:	4e 80 00 20 	blr
  654:	0f e0 00 00 	twui    r0,0
  658:	4b ff ff f4 	b       64c <test+0xc>		<==

0000065c <test9w>:
  65c:	2c 04 00 00 	cmpwi   r4,0
  660:	41 82 00 0c 	beq     66c <test9w+0x10>
  664:	7c 63 23 96 	divwu   r3,r3,r4
  668:	4e 80 00 20 	blr
  66c:	0f e0 00 00 	twui    r0,0
  670:	38 60 00 00 	li      r3,0			<==
  674:	4e 80 00 20 	blr				<==
  678:	38 60 00 00 	li      r3,0
  67c:	4e 80 00 20 	blr


Christophe

> 
> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/bug.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index ecbae1832de3..df6c11e008b9 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -97,7 +97,6 @@
>   	__label__ __label_warn_on;				\
>   								\
>   	WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
> -	unreachable();						\
>   								\
>   __label_warn_on:						\
>   	break;							\

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-25  6:46   ` Christophe Leroy
@ 2022-06-27 15:21     ` Sathvika Vasireddy
  2022-06-27 15:35     ` Sathvika Vasireddy
  2022-07-04 12:05     ` Peter Zijlstra
  2 siblings, 0 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-27 15:21 UTC (permalink / raw)
  To: Christophe Leroy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	Sathvika Vasireddy, naveen.n.rao, mbenes

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]


On 25/06/22 12:16, Christophe Leroy wrote:
>
> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>> objtool is throwing *unannotated intra-function call*
>> warnings with a few instructions that are marked
>> unreachable. Remove unreachable() from WARN_ON()
>> to fix these warnings, as the codegen remains same
>> with and without unreachable() in WARN_ON().
> Did you try the two exemples described in commit 1e688dd2a3d6
> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
> asm goto") ?
>
> Without your patch:
>
> 00000640 <test>:
>    640:	81 23 00 84 	lwz     r9,132(r3)
>    644:	71 29 40 00 	andi.   r9,r9,16384
>    648:	40 82 00 0c 	bne     654 <test+0x14>
>    64c:	80 63 00 0c 	lwz     r3,12(r3)
>    650:	4e 80 00 20 	blr
>    654:	0f e0 00 00 	twui    r0,0
>
> 00000658 <test9w>:
>    658:	2c 04 00 00 	cmpwi   r4,0
>    65c:	41 82 00 0c 	beq     668 <test9w+0x10>
>    660:	7c 63 23 96 	divwu   r3,r3,r4
>    664:	4e 80 00 20 	blr
>    668:	0f e0 00 00 	twui    r0,0
>    66c:	38 60 00 00 	li      r3,0
>    670:	4e 80 00 20 	blr
>
>
> With your patch:
>
> 00000640 <test>:
>    640:	81 23 00 84 	lwz     r9,132(r3)
>    644:	71 29 40 00 	andi.   r9,r9,16384
>    648:	40 82 00 0c 	bne     654 <test+0x14>
>    64c:	80 63 00 0c 	lwz     r3,12(r3)
>    650:	4e 80 00 20 	blr
>    654:	0f e0 00 00 	twui    r0,0
>    658:	4b ff ff f4 	b       64c <test+0xc>		<==
>
> 0000065c <test9w>:
>    65c:	2c 04 00 00 	cmpwi   r4,0
>    660:	41 82 00 0c 	beq     66c <test9w+0x10>
>    664:	7c 63 23 96 	divwu   r3,r3,r4
>    668:	4e 80 00 20 	blr
>    66c:	0f e0 00 00 	twui    r0,0
>    670:	38 60 00 00 	li      r3,0			<==
>    674:	4e 80 00 20 	blr				<==
>    678:	38 60 00 00 	li      r3,0
>    67c:	4e 80 00 20 	blr
>
The builtin variant of unreachable (__builtin_unreachable()) works,
and the codegen remains the same.

How about using that instead of unreachable() ?


- Sathvika

[-- Attachment #2: Type: text/html, Size: 3146 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-25  6:46   ` Christophe Leroy
  2022-06-27 15:21     ` Sathvika Vasireddy
@ 2022-06-27 15:35     ` Sathvika Vasireddy
  2022-06-27 15:46       ` Christophe Leroy
  2022-06-29 18:30       ` Christophe Leroy
  2022-07-04 12:05     ` Peter Zijlstra
  2 siblings, 2 replies; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-06-27 15:35 UTC (permalink / raw)
  To: Christophe Leroy, Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes


On 25/06/22 12:16, Christophe Leroy wrote:
>
> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>> objtool is throwing *unannotated intra-function call*
>> warnings with a few instructions that are marked
>> unreachable. Remove unreachable() from WARN_ON()
>> to fix these warnings, as the codegen remains same
>> with and without unreachable() in WARN_ON().
> Did you try the two exemples described in commit 1e688dd2a3d6
> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
> asm goto") ?
>
> Without your patch:
>
> 00000640 <test>:
>    640:	81 23 00 84 	lwz     r9,132(r3)
>    644:	71 29 40 00 	andi.   r9,r9,16384
>    648:	40 82 00 0c 	bne     654 <test+0x14>
>    64c:	80 63 00 0c 	lwz     r3,12(r3)
>    650:	4e 80 00 20 	blr
>    654:	0f e0 00 00 	twui    r0,0
>
> 00000658 <test9w>:
>    658:	2c 04 00 00 	cmpwi   r4,0
>    65c:	41 82 00 0c 	beq     668 <test9w+0x10>
>    660:	7c 63 23 96 	divwu   r3,r3,r4
>    664:	4e 80 00 20 	blr
>    668:	0f e0 00 00 	twui    r0,0
>    66c:	38 60 00 00 	li      r3,0
>    670:	4e 80 00 20 	blr
>
>
> With your patch:
>
> 00000640 <test>:
>    640:	81 23 00 84 	lwz     r9,132(r3)
>    644:	71 29 40 00 	andi.   r9,r9,16384
>    648:	40 82 00 0c 	bne     654 <test+0x14>
>    64c:	80 63 00 0c 	lwz     r3,12(r3)
>    650:	4e 80 00 20 	blr
>    654:	0f e0 00 00 	twui    r0,0
>    658:	4b ff ff f4 	b       64c <test+0xc>		<==
>
> 0000065c <test9w>:
>    65c:	2c 04 00 00 	cmpwi   r4,0
>    660:	41 82 00 0c 	beq     66c <test9w+0x10>
>    664:	7c 63 23 96 	divwu   r3,r3,r4
>    668:	4e 80 00 20 	blr
>    66c:	0f e0 00 00 	twui    r0,0
>    670:	38 60 00 00 	li      r3,0			<==
>    674:	4e 80 00 20 	blr				<==
>    678:	38 60 00 00 	li      r3,0
>    67c:	4e 80 00 20 	blr
>
The builtin variant of unreachable (__builtin_unreachable()) works.

How about using that instead of unreachable() ?


- Sathvika


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-27 15:35     ` Sathvika Vasireddy
@ 2022-06-27 15:46       ` Christophe Leroy
  2022-06-29 18:30       ` Christophe Leroy
  1 sibling, 0 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-06-27 15:46 UTC (permalink / raw)
  To: Sathvika Vasireddy, Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes



Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
> 
> On 25/06/22 12:16, Christophe Leroy wrote:
>>
>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>> objtool is throwing *unannotated intra-function call*
>>> warnings with a few instructions that are marked
>>> unreachable. Remove unreachable() from WARN_ON()
>>> to fix these warnings, as the codegen remains same
>>> with and without unreachable() in WARN_ON().
>> Did you try the two exemples described in commit 1e688dd2a3d6
>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
>> asm goto") ?
>>
>> Without your patch:
>>
>> 00000640 <test>:
>>    640:    81 23 00 84     lwz     r9,132(r3)
>>    644:    71 29 40 00     andi.   r9,r9,16384
>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>    650:    4e 80 00 20     blr
>>    654:    0f e0 00 00     twui    r0,0
>>
>> 00000658 <test9w>:
>>    658:    2c 04 00 00     cmpwi   r4,0
>>    65c:    41 82 00 0c     beq     668 <test9w+0x10>
>>    660:    7c 63 23 96     divwu   r3,r3,r4
>>    664:    4e 80 00 20     blr
>>    668:    0f e0 00 00     twui    r0,0
>>    66c:    38 60 00 00     li      r3,0
>>    670:    4e 80 00 20     blr
>>
>>
>> With your patch:
>>
>> 00000640 <test>:
>>    640:    81 23 00 84     lwz     r9,132(r3)
>>    644:    71 29 40 00     andi.   r9,r9,16384
>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>    650:    4e 80 00 20     blr
>>    654:    0f e0 00 00     twui    r0,0
>>    658:    4b ff ff f4     b       64c <test+0xc>        <==
>>
>> 0000065c <test9w>:
>>    65c:    2c 04 00 00     cmpwi   r4,0
>>    660:    41 82 00 0c     beq     66c <test9w+0x10>
>>    664:    7c 63 23 96     divwu   r3,r3,r4
>>    668:    4e 80 00 20     blr
>>    66c:    0f e0 00 00     twui    r0,0
>>    670:    38 60 00 00     li      r3,0            <==
>>    674:    4e 80 00 20     blr                <==
>>    678:    38 60 00 00     li      r3,0
>>    67c:    4e 80 00 20     blr
>>
> The builtin variant of unreachable (__builtin_unreachable()) works.
> 
> How about using that instead of unreachable() ?
> 

That seems odd.

Look at linux/compiler.h

It seems like unreachable() exists to help objtool.

Christophe

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-27 15:35     ` Sathvika Vasireddy
  2022-06-27 15:46       ` Christophe Leroy
@ 2022-06-29 18:30       ` Christophe Leroy
  2022-06-30  8:05         ` Naveen N. Rao
  2022-07-04 11:45         ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Peter Zijlstra
  1 sibling, 2 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-06-29 18:30 UTC (permalink / raw)
  To: Sathvika Vasireddy, Sathvika Vasireddy, linuxppc-dev
  Cc: Marc Zyngier, peterz, linux-kernel, rostedt, aik, mingo, paulus,
	jpoimboe, naveen.n.rao, mbenes, Chen Zhongjin, Linux ARM

Hi Sathvika,

Adding ARM people as they seem to face the same kind of problem (see 
https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/)

Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
> 
> On 25/06/22 12:16, Christophe Leroy wrote:
>>
>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>> objtool is throwing *unannotated intra-function call*
>>> warnings with a few instructions that are marked
>>> unreachable. Remove unreachable() from WARN_ON()
>>> to fix these warnings, as the codegen remains same
>>> with and without unreachable() in WARN_ON().
>> Did you try the two exemples described in commit 1e688dd2a3d6
>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
>> asm goto") ?
>>
>> Without your patch:
>>
>> 00000640 <test>:
>>    640:    81 23 00 84     lwz     r9,132(r3)
>>    644:    71 29 40 00     andi.   r9,r9,16384
>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>    650:    4e 80 00 20     blr
>>    654:    0f e0 00 00     twui    r0,0
>>
>> 00000658 <test9w>:
>>    658:    2c 04 00 00     cmpwi   r4,0
>>    65c:    41 82 00 0c     beq     668 <test9w+0x10>
>>    660:    7c 63 23 96     divwu   r3,r3,r4
>>    664:    4e 80 00 20     blr
>>    668:    0f e0 00 00     twui    r0,0
>>    66c:    38 60 00 00     li      r3,0
>>    670:    4e 80 00 20     blr
>>
>>
>> With your patch:
>>
>> 00000640 <test>:
>>    640:    81 23 00 84     lwz     r9,132(r3)
>>    644:    71 29 40 00     andi.   r9,r9,16384
>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>    650:    4e 80 00 20     blr
>>    654:    0f e0 00 00     twui    r0,0
>>    658:    4b ff ff f4     b       64c <test+0xc>        <==
>>
>> 0000065c <test9w>:
>>    65c:    2c 04 00 00     cmpwi   r4,0
>>    660:    41 82 00 0c     beq     66c <test9w+0x10>
>>    664:    7c 63 23 96     divwu   r3,r3,r4
>>    668:    4e 80 00 20     blr
>>    66c:    0f e0 00 00     twui    r0,0
>>    670:    38 60 00 00     li      r3,0            <==
>>    674:    4e 80 00 20     blr                <==
>>    678:    38 60 00 00     li      r3,0
>>    67c:    4e 80 00 20     blr
>>
> The builtin variant of unreachable (__builtin_unreachable()) works.
> 
> How about using that instead of unreachable() ?
> 
> 

In fact the problem comes from the macro annotate_unreachable() which is 
called by unreachable() before calling __build_unreachable().

Seems like this macro adds (after the unconditional trap twui) a call to 
an empty function whose address is listed in section .discard.unreachable

     1c78:       00 00 e0 0f     twui    r0,0
     1c7c:       55 e7 ff 4b     bl      3d0 
<qdisc_root_sleeping_lock.part.0>


RELOCATION RECORDS FOR [.discard.unreachable]:
OFFSET           TYPE              VALUE
0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0

The problem is that that function has size 0:

00000000000003d0 l     F .text	0000000000000000 
qdisc_root_sleeping_lock.part.0


And objtool is not prepared for a function with size 0.


The following changes to objtool seem to fix the problem, most warning 
are gone with that change.

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 63218f5799c2..37c0a268b7ea 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
struct rb_node *node)

  	if (*o < s->offset)
  		return -1;
+	if (*o == s->offset && !s->len)
+		return 0;
  	if (*o >= s->offset + s->len)
  		return 1;

@@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, struct 
symbol *sym)
  	 * Don't store empty STT_NOTYPE symbols in the rbtree.  They
  	 * can exist within a function, confusing the sorting.
  	 */
-	if (!sym->len)
+	if (sym->type == STT_NOTYPE && !sym->len)
  		rb_erase(&sym->node, &sym->sec->symbol_tree);
  }

---

I also had objtool running for ever on 
arch/powerpc/sysdev/xics/icp-hv.o, which I fixed with the below hack:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 51b6dcec8d6a..ef2303ad6381 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -529,7 +529,7 @@ static struct instruction *find_last_insn(struct 
objtool_file *file,
  	unsigned int offset;
  	unsigned int end = (sec->sh.sh_size > 10) ? sec->sh.sh_size - 10 : 0;

-	for (offset = sec->sh.sh_size - 1; offset >= end && !insn; offset--)
+	for (offset = sec->sh.sh_size - 1; offset && offset >= end && !insn; 
offset--)
  		insn = find_insn(file, sec, offset);

  	return insn;
---

Now I only have the following two warnings:

arch/powerpc/sysdev/xics/icp-hv.o: warning: objtool: can't find 
unreachable insn at .text.unlikely+0x0
drivers/crypto/vmx/aesp8-ppc.o: warning: objtool: 
aes_p8_set_encrypt_key+0x44: unannotated intra-function call

The first one is linked to the infinite loop I hacked. So I now have to 
understand what the problem really is.

The second one is an assembly file aesp8-ppc.S which lacks the changes 
you did in other assembly files in patch 12.

Christophe

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-29 18:30       ` Christophe Leroy
@ 2022-06-30  8:05         ` Naveen N. Rao
  2022-06-30  9:58           ` Christophe Leroy
  2022-07-01  2:13           ` Chen Zhongjin
  2022-07-04 11:45         ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Peter Zijlstra
  1 sibling, 2 replies; 41+ messages in thread
From: Naveen N. Rao @ 2022-06-30  8:05 UTC (permalink / raw)
  To: Christophe Leroy, linuxppc-dev, Sathvika Vasireddy, Sathvika Vasireddy
  Cc: Marc Zyngier, aik, linux-kernel, rostedt, peterz, mingo, paulus,
	jpoimboe, mbenes, Chen Zhongjin, Linux ARM

Christophe Leroy wrote:
> Hi Sathvika,
> 
> Adding ARM people as they seem to face the same kind of problem (see 
> https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/)
> 
> Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
>> 
>> On 25/06/22 12:16, Christophe Leroy wrote:
>>>
>>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>>> objtool is throwing *unannotated intra-function call*
>>>> warnings with a few instructions that are marked
>>>> unreachable. Remove unreachable() from WARN_ON()
>>>> to fix these warnings, as the codegen remains same
>>>> with and without unreachable() in WARN_ON().
>>> Did you try the two exemples described in commit 1e688dd2a3d6
>>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
>>> asm goto") ?
>>>
>>> Without your patch:
>>>
>>> 00000640 <test>:
>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>    650:    4e 80 00 20     blr
>>>    654:    0f e0 00 00     twui    r0,0
>>>
>>> 00000658 <test9w>:
>>>    658:    2c 04 00 00     cmpwi   r4,0
>>>    65c:    41 82 00 0c     beq     668 <test9w+0x10>
>>>    660:    7c 63 23 96     divwu   r3,r3,r4
>>>    664:    4e 80 00 20     blr
>>>    668:    0f e0 00 00     twui    r0,0
>>>    66c:    38 60 00 00     li      r3,0
>>>    670:    4e 80 00 20     blr
>>>
>>>
>>> With your patch:
>>>
>>> 00000640 <test>:
>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>    650:    4e 80 00 20     blr
>>>    654:    0f e0 00 00     twui    r0,0
>>>    658:    4b ff ff f4     b       64c <test+0xc>        <==
>>>
>>> 0000065c <test9w>:
>>>    65c:    2c 04 00 00     cmpwi   r4,0
>>>    660:    41 82 00 0c     beq     66c <test9w+0x10>
>>>    664:    7c 63 23 96     divwu   r3,r3,r4
>>>    668:    4e 80 00 20     blr
>>>    66c:    0f e0 00 00     twui    r0,0
>>>    670:    38 60 00 00     li      r3,0            <==
>>>    674:    4e 80 00 20     blr                <==
>>>    678:    38 60 00 00     li      r3,0
>>>    67c:    4e 80 00 20     blr
>>>
>> The builtin variant of unreachable (__builtin_unreachable()) works.
>> 
>> How about using that instead of unreachable() ?
>> 
>> 
> 
> In fact the problem comes from the macro annotate_unreachable() which is 
> called by unreachable() before calling __build_unreachable().
> 
> Seems like this macro adds (after the unconditional trap twui) a call to 
> an empty function whose address is listed in section .discard.unreachable
> 
>      1c78:       00 00 e0 0f     twui    r0,0
>      1c7c:       55 e7 ff 4b     bl      3d0 
> <qdisc_root_sleeping_lock.part.0>
> 
> 
> RELOCATION RECORDS FOR [.discard.unreachable]:
> OFFSET           TYPE              VALUE
> 0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0
> 
> The problem is that that function has size 0:
> 
> 00000000000003d0 l     F .text	0000000000000000 
> qdisc_root_sleeping_lock.part.0
> 
> 
> And objtool is not prepared for a function with size 0.

annotate_unreachable() seems to have been introduced in commit 
649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead 
ends").

Objtool considers 'ud2' instruction to be fatal, so BUG() has 
__builtin_unreachable(), rather than unreachable(). See commit 
bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() 
asm"). For the same reason, __WARN_FLAGS() is annotated with 
_ASM_REACHABLE so that objtool can differentiate warnings from a BUG().

On powerpc, we use trap variants for both and don't have a special 
instruction for a BUG(). As such, for _WARN_FLAGS(), using 
__builtin_unreachable() suffices to achieve optimal code generation from 
the compiler. Objtool would consider subsequent instructions to be 
reachable. For BUG(), we can continue to use unreachable() so that 
objtool can differentiate these from traps used in warnings.

> 
> The following changes to objtool seem to fix the problem, most warning 
> are gone with that change.
> 
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index 63218f5799c2..37c0a268b7ea 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
> struct rb_node *node)
> 
>   	if (*o < s->offset)
>   		return -1;
> +	if (*o == s->offset && !s->len)
> +		return 0;
>   	if (*o >= s->offset + s->len)
>   		return 1;
> 
> @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, struct 
> symbol *sym)
>   	 * Don't store empty STT_NOTYPE symbols in the rbtree.  They
>   	 * can exist within a function, confusing the sorting.
>   	 */
> -	if (!sym->len)
> +	if (sym->type == STT_NOTYPE && !sym->len)
>   		rb_erase(&sym->node, &sym->sec->symbol_tree);
>   }

Is there a reason to do this, rather than change __WARN_FLAGS() to use 
__builtin_unreachable()? Or, are you seeing an issue with unreachable() 
elsewhere in the kernel?


- Naveen


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30  8:05         ` Naveen N. Rao
@ 2022-06-30  9:58           ` Christophe Leroy
  2022-06-30 10:33             ` Christophe Leroy
  2022-06-30 10:37             ` Naveen N. Rao
  2022-07-01  2:13           ` Chen Zhongjin
  1 sibling, 2 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-06-30  9:58 UTC (permalink / raw)
  To: Naveen N. Rao, linuxppc-dev, Sathvika Vasireddy, Sathvika Vasireddy
  Cc: Marc Zyngier, aik, linux-kernel, rostedt, peterz, mingo, paulus,
	jpoimboe, mbenes, Chen Zhongjin, Linux ARM



Le 30/06/2022 à 10:05, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Hi Sathvika,
>>
>> Adding ARM people as they seem to face the same kind of problem (see 
>> https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/) 
>>
>>
>> Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
>>>
>>> On 25/06/22 12:16, Christophe Leroy wrote:
>>>>
>>>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>>>> objtool is throwing *unannotated intra-function call*
>>>>> warnings with a few instructions that are marked
>>>>> unreachable. Remove unreachable() from WARN_ON()
>>>>> to fix these warnings, as the codegen remains same
>>>>> with and without unreachable() in WARN_ON().
>>>> Did you try the two exemples described in commit 1e688dd2a3d6
>>>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() 
>>>> with
>>>> asm goto") ?
>>>>
>>>> Without your patch:
>>>>
>>>> 00000640 <test>:
>>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>>    650:    4e 80 00 20     blr
>>>>    654:    0f e0 00 00     twui    r0,0
>>>>
>>>> 00000658 <test9w>:
>>>>    658:    2c 04 00 00     cmpwi   r4,0
>>>>    65c:    41 82 00 0c     beq     668 <test9w+0x10>
>>>>    660:    7c 63 23 96     divwu   r3,r3,r4
>>>>    664:    4e 80 00 20     blr
>>>>    668:    0f e0 00 00     twui    r0,0
>>>>    66c:    38 60 00 00     li      r3,0
>>>>    670:    4e 80 00 20     blr
>>>>
>>>>
>>>> With your patch:
>>>>
>>>> 00000640 <test>:
>>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>>    650:    4e 80 00 20     blr
>>>>    654:    0f e0 00 00     twui    r0,0
>>>>    658:    4b ff ff f4     b       64c <test+0xc>        <==
>>>>
>>>> 0000065c <test9w>:
>>>>    65c:    2c 04 00 00     cmpwi   r4,0
>>>>    660:    41 82 00 0c     beq     66c <test9w+0x10>
>>>>    664:    7c 63 23 96     divwu   r3,r3,r4
>>>>    668:    4e 80 00 20     blr
>>>>    66c:    0f e0 00 00     twui    r0,0
>>>>    670:    38 60 00 00     li      r3,0            <==
>>>>    674:    4e 80 00 20     blr                <==
>>>>    678:    38 60 00 00     li      r3,0
>>>>    67c:    4e 80 00 20     blr
>>>>
>>> The builtin variant of unreachable (__builtin_unreachable()) works.
>>>
>>> How about using that instead of unreachable() ?
>>>
>>>
>>
>> In fact the problem comes from the macro annotate_unreachable() which 
>> is called by unreachable() before calling __build_unreachable().
>>
>> Seems like this macro adds (after the unconditional trap twui) a call 
>> to an empty function whose address is listed in section 
>> .discard.unreachable
>>
>>      1c78:       00 00 e0 0f     twui    r0,0
>>      1c7c:       55 e7 ff 4b     bl      3d0 
>> <qdisc_root_sleeping_lock.part.0>
>>
>>
>> RELOCATION RECORDS FOR [.discard.unreachable]:
>> OFFSET           TYPE              VALUE
>> 0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0
>>
>> The problem is that that function has size 0:
>>
>> 00000000000003d0 l     F .text    0000000000000000 
>> qdisc_root_sleeping_lock.part.0
>>
>>
>> And objtool is not prepared for a function with size 0.
> 
> annotate_unreachable() seems to have been introduced in commit 
> 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead 
> ends").
> 
> Objtool considers 'ud2' instruction to be fatal, so BUG() has 
> __builtin_unreachable(), rather than unreachable(). See commit 
> bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() 
> asm"). For the same reason, __WARN_FLAGS() is annotated with 
> _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
> 
> On powerpc, we use trap variants for both and don't have a special 
> instruction for a BUG(). As such, for _WARN_FLAGS(), using 
> __builtin_unreachable() suffices to achieve optimal code generation from 
> the compiler. Objtool would consider subsequent instructions to be 
> reachable. For BUG(), we can continue to use unreachable() so that 
> objtool can differentiate these from traps used in warnings.

Not sure I understand what you mean.

__WARN_FLAGS() and BUG() both use 'twui' which is unconditionnal trap, 
as such both are the same.

On the other side, WARN_ON() and BUG_ON() use tlbnei which is a 
conditionnel trap.

> 
>>
>> The following changes to objtool seem to fix the problem, most warning 
>> are gone with that change.
>>
>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>> index 63218f5799c2..37c0a268b7ea 100644
>> --- a/tools/objtool/elf.c
>> +++ b/tools/objtool/elf.c
>> @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
>> struct rb_node *node)
>>
>>       if (*o < s->offset)
>>           return -1;
>> +    if (*o == s->offset && !s->len)
>> +        return 0;
>>       if (*o >= s->offset + s->len)
>>           return 1;
>>
>> @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, struct 
>> symbol *sym)
>>        * Don't store empty STT_NOTYPE symbols in the rbtree.  They
>>        * can exist within a function, confusing the sorting.
>>        */
>> -    if (!sym->len)
>> +    if (sym->type == STT_NOTYPE && !sym->len)
>>           rb_erase(&sym->node, &sym->sec->symbol_tree);
>>   }
> 
> Is there a reason to do this, rather than change __WARN_FLAGS() to use 
> __builtin_unreachable()? Or, are you seeing an issue with unreachable() 
> elsewhere in the kernel?
> 

At the moment I'm trying to understand what the issue is, and explore 
possible fixes. I guess if we tell objtool that after 'twui' subsequent 
instructions are unreachable, then __builtin_unreachable() is enough.

I think we should also understand why annotate_unreachable() gives us a 
so bad result and see if it can be changed to something cleaner than a 
'bl' to an empty function that has no instructions.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30  9:58           ` Christophe Leroy
@ 2022-06-30 10:33             ` Christophe Leroy
  2022-06-30 10:37             ` Naveen N. Rao
  1 sibling, 0 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-06-30 10:33 UTC (permalink / raw)
  To: Naveen N. Rao, linuxppc-dev, Sathvika Vasireddy, Sathvika Vasireddy
  Cc: Marc Zyngier, aik, linux-kernel, rostedt, peterz, mingo, paulus,
	jpoimboe, mbenes, Chen Zhongjin, Linux ARM



Le 30/06/2022 à 11:58, Christophe Leroy a écrit :
> 
> 
> Le 30/06/2022 à 10:05, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Hi Sathvika,
>>>
>>> Adding ARM people as they seem to face the same kind of problem (see 
>>> https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/) 
>>>
>>>
>>> Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
>>>>
>>>> On 25/06/22 12:16, Christophe Leroy wrote:
>>>>>
>>>>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>>>>> objtool is throwing *unannotated intra-function call*
>>>>>> warnings with a few instructions that are marked
>>>>>> unreachable. Remove unreachable() from WARN_ON()
>>>>>> to fix these warnings, as the codegen remains same
>>>>>> with and without unreachable() in WARN_ON().
>>>>> Did you try the two exemples described in commit 1e688dd2a3d6
>>>>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() 
>>>>> with
>>>>> asm goto") ?
>>>>>
>>>>> Without your patch:
>>>>>
>>>>> 00000640 <test>:
>>>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>>>    650:    4e 80 00 20     blr
>>>>>    654:    0f e0 00 00     twui    r0,0
>>>>>
>>>>> 00000658 <test9w>:
>>>>>    658:    2c 04 00 00     cmpwi   r4,0
>>>>>    65c:    41 82 00 0c     beq     668 <test9w+0x10>
>>>>>    660:    7c 63 23 96     divwu   r3,r3,r4
>>>>>    664:    4e 80 00 20     blr
>>>>>    668:    0f e0 00 00     twui    r0,0
>>>>>    66c:    38 60 00 00     li      r3,0
>>>>>    670:    4e 80 00 20     blr
>>>>>
>>>>>
>>>>> With your patch:
>>>>>
>>>>> 00000640 <test>:
>>>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>>>    650:    4e 80 00 20     blr
>>>>>    654:    0f e0 00 00     twui    r0,0
>>>>>    658:    4b ff ff f4     b       64c <test+0xc>        <==
>>>>>
>>>>> 0000065c <test9w>:
>>>>>    65c:    2c 04 00 00     cmpwi   r4,0
>>>>>    660:    41 82 00 0c     beq     66c <test9w+0x10>
>>>>>    664:    7c 63 23 96     divwu   r3,r3,r4
>>>>>    668:    4e 80 00 20     blr
>>>>>    66c:    0f e0 00 00     twui    r0,0
>>>>>    670:    38 60 00 00     li      r3,0            <==
>>>>>    674:    4e 80 00 20     blr                <==
>>>>>    678:    38 60 00 00     li      r3,0
>>>>>    67c:    4e 80 00 20     blr
>>>>>
>>>> The builtin variant of unreachable (__builtin_unreachable()) works.
>>>>
>>>> How about using that instead of unreachable() ?
>>>>
>>>>
>>>
>>> In fact the problem comes from the macro annotate_unreachable() which 
>>> is called by unreachable() before calling __build_unreachable().
>>>
>>> Seems like this macro adds (after the unconditional trap twui) a call 
>>> to an empty function whose address is listed in section 
>>> .discard.unreachable
>>>
>>>      1c78:       00 00 e0 0f     twui    r0,0
>>>      1c7c:       55 e7 ff 4b     bl      3d0 
>>> <qdisc_root_sleeping_lock.part.0>
>>>
>>>
>>> RELOCATION RECORDS FOR [.discard.unreachable]:
>>> OFFSET           TYPE              VALUE
>>> 0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0
>>>
>>> The problem is that that function has size 0:
>>>
>>> 00000000000003d0 l     F .text    0000000000000000 
>>> qdisc_root_sleeping_lock.part.0
>>>
>>>
>>> And objtool is not prepared for a function with size 0.
>>
>> annotate_unreachable() seems to have been introduced in commit 
>> 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead 
>> ends").
>>
>> Objtool considers 'ud2' instruction to be fatal, so BUG() has 
>> __builtin_unreachable(), rather than unreachable(). See commit 
>> bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() 
>> asm"). For the same reason, __WARN_FLAGS() is annotated with 
>> _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
>>
>> On powerpc, we use trap variants for both and don't have a special 
>> instruction for a BUG(). As such, for _WARN_FLAGS(), using 
>> __builtin_unreachable() suffices to achieve optimal code generation 
>> from the compiler. Objtool would consider subsequent instructions to 
>> be reachable. For BUG(), we can continue to use unreachable() so that 
>> objtool can differentiate these from traps used in warnings.
> 
> Not sure I understand what you mean.
> 
> __WARN_FLAGS() and BUG() both use 'twui' which is unconditionnal trap, 
> as such both are the same.
> 
> On the other side, WARN_ON() and BUG_ON() use tlbnei which is a 
> conditionnel trap.
> 
>>
>>>
>>> The following changes to objtool seem to fix the problem, most 
>>> warning are gone with that change.
>>>
>>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>>> index 63218f5799c2..37c0a268b7ea 100644
>>> --- a/tools/objtool/elf.c
>>> +++ b/tools/objtool/elf.c
>>> @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
>>> struct rb_node *node)
>>>
>>>       if (*o < s->offset)
>>>           return -1;
>>> +    if (*o == s->offset && !s->len)
>>> +        return 0;
>>>       if (*o >= s->offset + s->len)
>>>           return 1;
>>>
>>> @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, 
>>> struct symbol *sym)
>>>        * Don't store empty STT_NOTYPE symbols in the rbtree.  They
>>>        * can exist within a function, confusing the sorting.
>>>        */
>>> -    if (!sym->len)
>>> +    if (sym->type == STT_NOTYPE && !sym->len)
>>>           rb_erase(&sym->node, &sym->sec->symbol_tree);
>>>   }
>>
>> Is there a reason to do this, rather than change __WARN_FLAGS() to use 
>> __builtin_unreachable()? Or, are you seeing an issue with 
>> unreachable() elsewhere in the kernel?
>>
> 
> At the moment I'm trying to understand what the issue is, and explore 
> possible fixes. I guess if we tell objtool that after 'twui' subsequent 
> instructions are unreachable, then __builtin_unreachable() is enough.

I get a nice result with the following changes (on top of Sathvika's 
series):

diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index df6c11e008b9..73f5650f98df 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -89,7 +89,7 @@

  #define BUG() do {						\
  	BUG_ENTRY("twi 31, 0, 0", 0);				\
-	unreachable();						\
+	__builtin_unreachable();				\
  } while (0)
  #define HAVE_ARCH_BUG

@@ -97,6 +97,7 @@
  	__label__ __label_warn_on;				\
  								\
  	WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
+	__builtin_unreachable();				\
  								\
  __label_warn_on:						\
  	break;							\
diff --git a/tools/objtool/arch/powerpc/decode.c 
b/tools/objtool/arch/powerpc/decode.c
index 06fc0206bf8e..9a0303304923 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -68,6 +68,8 @@ int arch_decode_instruction(struct objtool_file *file, 
const struct section *sec
  		}
  		break;
  	}
+	if (insn == 0x0fe00000) /* twui */
+		*type = INSN_BUG;

  	return 0;
  }
---

Christophe

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30  9:58           ` Christophe Leroy
  2022-06-30 10:33             ` Christophe Leroy
@ 2022-06-30 10:37             ` Naveen N. Rao
  2022-06-30 15:58               ` Segher Boessenkool
  2022-07-04 11:43               ` Peter Zijlstra
  1 sibling, 2 replies; 41+ messages in thread
From: Naveen N. Rao @ 2022-06-30 10:37 UTC (permalink / raw)
  To: Christophe Leroy, linuxppc-dev, Sathvika Vasireddy, Sathvika Vasireddy
  Cc: Marc Zyngier, aik, linux-kernel, rostedt, peterz, mingo, paulus,
	jpoimboe, mbenes, Chen Zhongjin, Linux ARM

Christophe Leroy wrote:
> 
> 
> Le 30/06/2022 à 10:05, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>>> The builtin variant of unreachable (__builtin_unreachable()) works.
>>>>
>>>> How about using that instead of unreachable() ?
>>>>
>>>>
>>>
>>> In fact the problem comes from the macro annotate_unreachable() which 
>>> is called by unreachable() before calling __build_unreachable().
>>>
>>> Seems like this macro adds (after the unconditional trap twui) a call 
>>> to an empty function whose address is listed in section 
>>> .discard.unreachable
>>>
>>>      1c78:       00 00 e0 0f     twui    r0,0
>>>      1c7c:       55 e7 ff 4b     bl      3d0 
>>> <qdisc_root_sleeping_lock.part.0>
>>>
>>>
>>> RELOCATION RECORDS FOR [.discard.unreachable]:
>>> OFFSET           TYPE              VALUE
>>> 0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0
>>>
>>> The problem is that that function has size 0:
>>>
>>> 00000000000003d0 l     F .text    0000000000000000 
>>> qdisc_root_sleeping_lock.part.0
>>>
>>>
>>> And objtool is not prepared for a function with size 0.
>> 
>> annotate_unreachable() seems to have been introduced in commit 
>> 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead 
>> ends").
>> 
>> Objtool considers 'ud2' instruction to be fatal, so BUG() has 
>> __builtin_unreachable(), rather than unreachable(). See commit 
>> bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() 
>> asm"). For the same reason, __WARN_FLAGS() is annotated with 
>> _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
>> 
>> On powerpc, we use trap variants for both and don't have a special 
>> instruction for a BUG(). As such, for _WARN_FLAGS(), using 
>> __builtin_unreachable() suffices to achieve optimal code generation from 
>> the compiler. Objtool would consider subsequent instructions to be 
>> reachable. For BUG(), we can continue to use unreachable() so that 
>> objtool can differentiate these from traps used in warnings.
> 
> Not sure I understand what you mean.
> 
> __WARN_FLAGS() and BUG() both use 'twui' which is unconditionnal trap, 
> as such both are the same.
> 
> On the other side, WARN_ON() and BUG_ON() use tlbnei which is a 
> conditionnel trap.

Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
BUG(), there is no need for an annotation since objtool assumes that 
'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
used, an explicit annotate_reachable() is needed. That's _reachable_, to 
indicate that the control flow can continue with the next instruction.

On powerpc, we should (eventually) classify all trap variants as 
INSN_TRAP. Even in the absence of that classification today, objtool 
assumes that control flow continues with the next instruction. With your 
work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
being generated, I think it is appropriate to just use 
__builtin_unreachable() and to not use the annotation.

In any case, we are only hitting this since gcc is generating a 'bl' due 
to that annotation. We are not yet enabling full objtool validation on 
powerpc, so I think we can revisit this at that point.

> 
>> 
>>>
>>> The following changes to objtool seem to fix the problem, most warning 
>>> are gone with that change.
>>>
>>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>>> index 63218f5799c2..37c0a268b7ea 100644
>>> --- a/tools/objtool/elf.c
>>> +++ b/tools/objtool/elf.c
>>> @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
>>> struct rb_node *node)
>>>
>>>       if (*o < s->offset)
>>>           return -1;
>>> +    if (*o == s->offset && !s->len)
>>> +        return 0;
>>>       if (*o >= s->offset + s->len)
>>>           return 1;
>>>
>>> @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, struct 
>>> symbol *sym)
>>>        * Don't store empty STT_NOTYPE symbols in the rbtree.  They
>>>        * can exist within a function, confusing the sorting.
>>>        */
>>> -    if (!sym->len)
>>> +    if (sym->type == STT_NOTYPE && !sym->len)
>>>           rb_erase(&sym->node, &sym->sec->symbol_tree);
>>>   }
>> 
>> Is there a reason to do this, rather than change __WARN_FLAGS() to use 
>> __builtin_unreachable()? Or, are you seeing an issue with unreachable() 
>> elsewhere in the kernel?
>> 
> 
> At the moment I'm trying to understand what the issue is, and explore 
> possible fixes. I guess if we tell objtool that after 'twui' subsequent 
> instructions are unreachable, then __builtin_unreachable() is enough.

Yes, see my explanation above. Since no 'bl' is emitted with the 
builtin, objtool won't complain, especially for mcount.

> 
> I think we should also understand why annotate_unreachable() gives us a 
> so bad result and see if it can be changed to something cleaner than a 
> 'bl' to an empty function that has no instructions.

Indeed. Not really sure. annotate_unreachable() wants to take the 
address of the instruction after the trap. But, in reality, due to use 
of asm goto for __WARN_FLAGS, no instructions would be generated. I 
wonder if that combination causes such code to be emitted.


- Naveen


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30 10:37             ` Naveen N. Rao
@ 2022-06-30 15:58               ` Segher Boessenkool
  2022-07-04 12:01                 ` Peter Zijlstra
  2022-07-04 11:43               ` Peter Zijlstra
  1 sibling, 1 reply; 41+ messages in thread
From: Segher Boessenkool @ 2022-06-30 15:58 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: aik, Marc Zyngier, Sathvika Vasireddy, linux-kernel,
	Chen Zhongjin, peterz, mingo, Sathvika Vasireddy, rostedt,
	jpoimboe, paulus, mbenes, linuxppc-dev, Linux ARM

On Thu, Jun 30, 2022 at 04:07:47PM +0530, Naveen N. Rao wrote:
> Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
> BUG(), there is no need for an annotation since objtool assumes that 
> 'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
> used, an explicit annotate_reachable() is needed. That's _reachable_, to 
> indicate that the control flow can continue with the next instruction.
> 
> On powerpc, we should (eventually) classify all trap variants as 
> INSN_TRAP. Even in the absence of that classification today, objtool 
> assumes that control flow continues with the next instruction. With your 
> work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
> being generated, I think it is appropriate to just use 
> __builtin_unreachable() and to not use the annotation.
> 
> In any case, we are only hitting this since gcc is generating a 'bl' due 
> to that annotation. We are not yet enabling full objtool validation on 
> powerpc, so I think we can revisit this at that point.

See also <https://gcc.gnu.org/PR99299> that asks for a __builtin_trap()
variant that does not terminate control flow ("that is recoverable").


Segher

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30  8:05         ` Naveen N. Rao
  2022-06-30  9:58           ` Christophe Leroy
@ 2022-07-01  2:13           ` Chen Zhongjin
  2022-07-01  6:56             ` Sathvika Vasireddy
  1 sibling, 1 reply; 41+ messages in thread
From: Chen Zhongjin @ 2022-07-01  2:13 UTC (permalink / raw)
  To: Naveen N. Rao, Christophe Leroy, linuxppc-dev,
	Sathvika Vasireddy, Sathvika Vasireddy
  Cc: aik, jpoimboe, linux-kernel, rostedt, peterz, mingo, paulus,
	Marc Zyngier, mbenes, Linux ARM

Hi everyone,

Hope I'm not too late for this discussion.

I'm not familiar with ppc so it spent me some time to reproduce this. 
But at last I didn't make it.

What I did:

1 checkout to tip/objtool/core

2 apply this patch

3 recover the unreachable() after WARN_ENTRY(..

4 compile on defconfig/allyesconfig

If anyone can point out which file will generate this problem it will be 
perfect.

On 2022/6/30 16:05, Naveen N. Rao wrote:
> Christophe Leroy wrote:
>> Hi Sathvika,
>>
>> Adding ARM people as they seem to face the same kind of problem (see 
>> https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/)

For my situation, the problem is, if there is an unreachable() used in 
the switch default case with nothing else, compiler will generate a NOP 
and is still a jump to this NOP branch while it is marked in 
.discard.unreachable.

When objtool deal with .discard.unreachable, it will *do nothing* to 
this NOP itself, but mark the previous instruction as "dead_end" (see 
check.c:ignore_unreachable_insn()). And checking will stop when reach 
this dead_end insn.

0x4: jne 0x14        <= jump for switch case

..

0x10: ret                <= dead_end

0x14: nop              <= real unreachable instructiond

However, actually we have a jump to NOP, which makes this reachable to 
this branch, and when this NOP is at end of function, it get a "fall 
through" warning.


I changed the unreachable to -EINVAL but it was criticized by the 
committer because he thought it is objtool's job to deal with these 
special cases.

>>
>> Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
>>>
>>> On 25/06/22 12:16, Christophe Leroy wrote:
>>>>
>>>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>>>> objtool is throwing *unannotated intra-function call*
>>>>> warnings with a few instructions that are marked
>>>>> unreachable. Remove unreachable() from WARN_ON()
>>>>> to fix these warnings, as the codegen remains same
>>>>> with and without unreachable() in WARN_ON().
>>>> Did you try the two exemples described in commit 1e688dd2a3d6
>>>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() 
>>>> with
>>>> asm goto") ?
>>>>
>>>> Without your patch:
>>>>
>>>> 00000640 <test>:
>>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>>    650:    4e 80 00 20     blr
>>>>    654:    0f e0 00 00     twui    r0,0
>>>>
>>>> 00000658 <test9w>:
>>>>    658:    2c 04 00 00     cmpwi   r4,0
>>>>    65c:    41 82 00 0c     beq     668 <test9w+0x10>
>>>>    660:    7c 63 23 96     divwu   r3,r3,r4
>>>>    664:    4e 80 00 20     blr
>>>>    668:    0f e0 00 00     twui    r0,0
>>>>    66c:    38 60 00 00     li      r3,0
>>>>    670:    4e 80 00 20     blr
>>>>
>>>>
>>>> With your patch:
>>>>
>>>> 00000640 <test>:
>>>>    640:    81 23 00 84     lwz     r9,132(r3)
>>>>    644:    71 29 40 00     andi.   r9,r9,16384
>>>>    648:    40 82 00 0c     bne     654 <test+0x14>
>>>>    64c:    80 63 00 0c     lwz     r3,12(r3)
>>>>    650:    4e 80 00 20     blr
>>>>    654:    0f e0 00 00     twui    r0,0
>>>>    658:    4b ff ff f4     b       64c <test+0xc>        <==
>>>>
>>>> 0000065c <test9w>:
>>>>    65c:    2c 04 00 00     cmpwi   r4,0
>>>>    660:    41 82 00 0c     beq     66c <test9w+0x10>
>>>>    664:    7c 63 23 96     divwu   r3,r3,r4
>>>>    668:    4e 80 00 20     blr
>>>>    66c:    0f e0 00 00     twui    r0,0
>>>>    670:    38 60 00 00     li      r3,0            <==
>>>>    674:    4e 80 00 20     blr                <==
>>>>    678:    38 60 00 00     li      r3,0
>>>>    67c:    4e 80 00 20     blr
>>>>
>>> The builtin variant of unreachable (__builtin_unreachable()) works.
>>>
>>> How about using that instead of unreachable() ?
>>>
>>>
>>
>> In fact the problem comes from the macro annotate_unreachable() which 
>> is called by unreachable() before calling __build_unreachable().
>>
>> Seems like this macro adds (after the unconditional trap twui) a call 
>> to an empty function whose address is listed in section 
>> .discard.unreachable
>>
>>      1c78:       00 00 e0 0f     twui    r0,0
>>      1c7c:       55 e7 ff 4b     bl      3d0 
>> <qdisc_root_sleeping_lock.part.0>
>>
>>
>> RELOCATION RECORDS FOR [.discard.unreachable]:
>> OFFSET           TYPE              VALUE
>> 0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0
>>
>> The problem is that that function has size 0:
>>
>> 00000000000003d0 l     F .text    0000000000000000 
>> qdisc_root_sleeping_lock.part.0
>>
>>
>> And objtool is not prepared for a function with size 0.
>
> annotate_unreachable() seems to have been introduced in commit 
> 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead 
> ends").
>
> Objtool considers 'ud2' instruction to be fatal, so BUG() has 
> __builtin_unreachable(), rather than unreachable(). See commit 
> bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() 
> asm"). For the same reason, __WARN_FLAGS() is annotated with 
> _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
>
> On powerpc, we use trap variants for both and don't have a special 
> instruction for a BUG(). As such, for _WARN_FLAGS(), using 
> __builtin_unreachable() suffices to achieve optimal code generation 
> from the compiler. Objtool would consider subsequent instructions to 
> be reachable. For BUG(), we can continue to use unreachable() so that 
> objtool can differentiate these from traps used in warnings.
>
It is right and on arm64 only BUG() has unreachable and there is no 
annotation for __WARN_FLAGS(). Objtool works correctly on this. For that 
I support that unreachable() annotation shouldn't be with __WARN_FLAGS() 
because there should be an accessible branch after WARN() micro. However 
in the previous case, it's wired that compiler generates a bl to 
unreachable symbol, IIUC it is not a illegal call? (if it is allowed on 
ppc then objtool should be tell to recognize this)


It seems that your decoding only care about INSN_CALL for mcount, so 
maybe temporarily these control flow checking makes non-sense for you so 
the solution could actually be looser.

Anyway, maybe I can help more if I can reproduce that on my own machine.


Best,

Chen

.



^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-07-01  2:13           ` Chen Zhongjin
@ 2022-07-01  6:56             ` Sathvika Vasireddy
  2022-07-01 11:40               ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() (gcc issue ?) Christophe Leroy
  0 siblings, 1 reply; 41+ messages in thread
From: Sathvika Vasireddy @ 2022-07-01  6:56 UTC (permalink / raw)
  To: Chen Zhongjin, Naveen N. Rao, Christophe Leroy, linuxppc-dev,
	Sathvika Vasireddy
  Cc: aik, jpoimboe, linux-kernel, rostedt, peterz, mingo, paulus,
	Marc Zyngier, mbenes, Linux ARM

Hi Chen,

Thanks for pitching in and providing your inputs :-)

On 01/07/22 07:43, Chen Zhongjin wrote:
> Hi everyone,
>
> Hope I'm not too late for this discussion.
>
> I'm not familiar with ppc so it spent me some time to reproduce this. 
> But at last I didn't make it.
>
> What I did:
>
> 1 checkout to tip/objtool/core
>
> 2 apply this patch
>
> 3 recover the unreachable() after WARN_ENTRY(..
>
> 4 compile on defconfig/allyesconfig
>
> If anyone can point out which file will generate this problem it will 
> be perfect.

To reproduce this problem, you may want to apply this patch series on 
top of objtool/core branch of the tip tree, and compile on 
ppc64le_defconfig.

There are a couple of C files that are generating these warnings. One 
such file is: arch/powerpc/kvm/../../../virt/kvm/kvm_main.o which gives
*arch/powerpc/kvm/../../../virt/kvm/kvm_main.o: warning: objtool: 
kvm_mmu_notifier_release+0x6c: unannotated intra-function call* warning.

With unreachable() in __WARN_FLAGS(), we get unannotated intra-function 
call warnings, but without unreachable() like in patch 11/12 or with 
just the builtin variant of unreachable (__builtin_unreachable()) 
instead of unreachable(), we do not get those warnings.


- Sathvika


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() (gcc issue ?)
  2022-07-01  6:56             ` Sathvika Vasireddy
@ 2022-07-01 11:40               ` Christophe Leroy
  0 siblings, 0 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-07-01 11:40 UTC (permalink / raw)
  To: Sathvika Vasireddy, Chen Zhongjin, Naveen N. Rao, linuxppc-dev,
	Sathvika Vasireddy, Segher Boessenkool
  Cc: aik, jpoimboe, linux-kernel, rostedt, peterz, mingo, paulus,
	Marc Zyngier, mbenes, Linux ARM

Hi Segher, your help might be welcome,

Le 01/07/2022 à 08:56, Sathvika Vasireddy a écrit :
> Hi Chen,
> 
> Thanks for pitching in and providing your inputs :-)
> 
> On 01/07/22 07:43, Chen Zhongjin wrote:
>> Hi everyone,
>>
>> Hope I'm not too late for this discussion.
>>
>> I'm not familiar with ppc so it spent me some time to reproduce this. 
>> But at last I didn't make it.
>>
>> What I did:
>>
>> 1 checkout to tip/objtool/core
>>
>> 2 apply this patch
>>
>> 3 recover the unreachable() after WARN_ENTRY(..
>>
>> 4 compile on defconfig/allyesconfig
>>
>> If anyone can point out which file will generate this problem it will 
>> be perfect.
> 
> To reproduce this problem, you may want to apply this patch series on 
> top of objtool/core branch of the tip tree, and compile on 
> ppc64le_defconfig.
> 
> There are a couple of C files that are generating these warnings. One 
> such file is: arch/powerpc/kvm/../../../virt/kvm/kvm_main.o which gives
> *arch/powerpc/kvm/../../../virt/kvm/kvm_main.o: warning: objtool: 
> kvm_mmu_notifier_release+0x6c: unannotated intra-function call* warning.
> 
> With unreachable() in __WARN_FLAGS(), we get unannotated intra-function 
> call warnings, but without unreachable() like in patch 11/12 or with 
> just the builtin variant of unreachable (__builtin_unreachable()) 
> instead of unreachable(), we do not get those warnings.
> 


I made a simple test aside our issue to see if I get something similar 
to ARM. I don't if it is the same at the end, but it looks odd anyway:

int test(int x)
{
	switch(x) {
	case 0 : return 3;
	case 1 : return 5;
	default : unreachable();
	}
}

0000000000001c80 <test>:
     1c80:       a6 02 08 7c     mflr    r0
     1c84:       01 00 00 48     bl      1c84 <test+0x4>
                         1c84: R_PPC64_REL24     _mcount
     1c88:       00 00 23 2c     cmpdi   r3,0
     1c8c:       14 00 82 41     beq     1ca0 <test+0x20>
     1c90:       01 00 03 2c     cmpwi   r3,1
     1c94:       05 00 60 38     li      r3,5
     1c98:       20 00 82 4d     beqlr
     1c9c:       00 00 42 60     ori     r2,r2,0
     1ca0:       03 00 60 38     li      r3,3
     1ca4:       20 00 80 4e     blr

So it looks like it takes no real benefit from the unreachable marking.

Now with __builtin_unreachable() instead of unreachable() :

int test(int x)
{
	switch(x) {
	case 0 : return 3;
	case 1 : return 5;
	default : __builtin_unreachable();
	}
}

0000000000001c80 <test>:
     1c80:       a6 02 08 7c     mflr    r0
     1c84:       01 00 00 48     bl      1c84 <test+0x4>
                         1c84: R_PPC64_REL24     _mcount
     1c88:       00 00 63 20     subfic  r3,r3,0
     1c8c:       10 19 63 7c     subfe   r3,r3,r3
     1c90:       bc 07 63 54     rlwinm  r3,r3,0,30,30
     1c94:       03 00 63 38     addi    r3,r3,3
     1c98:       20 00 80 4e     blr


Here the generated code takes advantage of the unreachable marking and 
results in a branchless code.


Christophe

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types
  2022-06-24 18:32 ` [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types Sathvika Vasireddy
@ 2022-07-04 11:14   ` Peter Zijlstra
  2022-07-04 15:53     ` Christophe Leroy
  0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 11:14 UTC (permalink / raw)
  To: Sathvika Vasireddy, g
  Cc: aik, linux-kernel, mingo, paulus, rostedt, jpoimboe,
	naveen.n.rao, mbenes, linuxppc-dev

On Sat, Jun 25, 2022 at 12:02:33AM +0530, Sathvika Vasireddy wrote:
> Make relocation types architecture specific.
> 
> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
> ---
>  tools/objtool/arch/x86/include/arch/elf.h | 2 ++
>  tools/objtool/check.c                     | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/arch/x86/include/arch/elf.h b/tools/objtool/arch/x86/include/arch/elf.h
> index 69cc4264b28a..ac14987cf687 100644
> --- a/tools/objtool/arch/x86/include/arch/elf.h
> +++ b/tools/objtool/arch/x86/include/arch/elf.h
> @@ -2,5 +2,7 @@
>  #define _OBJTOOL_ARCH_ELF
>  
>  #define R_NONE R_X86_64_NONE
> +#define R_ABS64 R_X86_64_64
> +#define R_ABS32 R_X86_64_32
>  
>  #endif /* _OBJTOOL_ARCH_ELF */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 98e869721bc4..88f68269860e 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -834,7 +834,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
>  		memset(loc, 0, size);
>  
>  		if (elf_add_reloc_to_insn(file->elf, sec, idx,
> -					  R_X86_64_64,
> +					  size == sizeof(u64) ? R_ABS64 : R_ABS32,
>  					  insn->sec, insn->offset))
>  			return -1;
>  

Given cross compiles, should this not also be elf dependent?

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30 10:37             ` Naveen N. Rao
  2022-06-30 15:58               ` Segher Boessenkool
@ 2022-07-04 11:43               ` Peter Zijlstra
  1 sibling, 0 replies; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 11:43 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Marc Zyngier, aik, Sathvika Vasireddy, linux-kernel,
	Chen Zhongjin, mingo, Sathvika Vasireddy, jpoimboe, paulus,
	mbenes, linuxppc-dev, rostedt, Linux ARM

On Thu, Jun 30, 2022 at 04:07:47PM +0530, Naveen N. Rao wrote:

> Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 BUG(),

Yes, ud2 is the traditional 'kill' instruction and a number of emulators
treat it as such, however it also being the shortest encoding (2 bytes)
for #UD Linux has opted to (ab)use it to implement WARN/BUG.

As such interpretation of 'ud2' needs to assume control flow stops
(compiler will also emit ud2 in a number of cases with that intent).
However, if it's used as WARN we then need to annotate the thing to not
be terminal.

> there is no need for an annotation since objtool assumes that 'ud2'
> terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is used, an
> explicit annotate_reachable() is needed. That's _reachable_, to indicate
> that the control flow can continue with the next instruction.
> 
> On powerpc, we should (eventually) classify all trap variants as INSN_TRAP.

Careful.. INSN_TRAP is mostly used for purposes of speculation stop and
padding. That is, INSN_TRAP does indeed not affect control flow, but the
way objtool treats it might not be quite what you want.

Specifically, straight-line-speculation checks want INT3 after indirect
control transfers (indirect jump and return -- indirect call is
'difficult'); these locations are architecturally not executed and as
such placing a random trap instruction there is 'harmless'. Of course,
were the branch predictor to go wobbly and attempt to execute it, the
fact that it's a trap will stop speculation dead.

Additionally, int3, being a single byte instruction, is also used to
fill dead code space, any #BP trap on it will not have a descriptor and
mostly cause the kernel to go splat.

Per the last usage, validate_reachable_instructions() will ignore it.
I'm not sure you want to always ignore all your (unreachable) trap
instructions.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-29 18:30       ` Christophe Leroy
  2022-06-30  8:05         ` Naveen N. Rao
@ 2022-07-04 11:45         ` Peter Zijlstra
  2022-07-04 12:34           ` Christophe Leroy
  1 sibling, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 11:45 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Marc Zyngier, aik, Sathvika Vasireddy, linux-kernel, rostedt,
	Chen Zhongjin, mingo, Sathvika Vasireddy, jpoimboe, paulus,
	naveen.n.rao, mbenes, linuxppc-dev, Linux ARM

On Wed, Jun 29, 2022 at 06:30:23PM +0000, Christophe Leroy wrote:


> The problem is that that function has size 0:
> 
> 00000000000003d0 l     F .text	0000000000000000 
> qdisc_root_sleeping_lock.part.0

I'm somewhat confused; how is an empty STT_FUNC a valid construct on
Power?

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-30 15:58               ` Segher Boessenkool
@ 2022-07-04 12:01                 ` Peter Zijlstra
  0 siblings, 0 replies; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 12:01 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: aik, Marc Zyngier, Sathvika Vasireddy, linux-kernel,
	Chen Zhongjin, mingo, Sathvika Vasireddy, rostedt, jpoimboe,
	paulus, Naveen N. Rao, mbenes, linuxppc-dev, Linux ARM

On Thu, Jun 30, 2022 at 10:58:11AM -0500, Segher Boessenkool wrote:
> On Thu, Jun 30, 2022 at 04:07:47PM +0530, Naveen N. Rao wrote:
> > Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
> > BUG(), there is no need for an annotation since objtool assumes that 
> > 'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
> > used, an explicit annotate_reachable() is needed. That's _reachable_, to 
> > indicate that the control flow can continue with the next instruction.
> > 
> > On powerpc, we should (eventually) classify all trap variants as 
> > INSN_TRAP. Even in the absence of that classification today, objtool 
> > assumes that control flow continues with the next instruction. With your 
> > work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
> > being generated, I think it is appropriate to just use 
> > __builtin_unreachable() and to not use the annotation.
> > 
> > In any case, we are only hitting this since gcc is generating a 'bl' due 
> > to that annotation. We are not yet enabling full objtool validation on 
> > powerpc, so I think we can revisit this at that point.
> 
> See also <https://gcc.gnu.org/PR99299> that asks for a __builtin_trap()
> variant that does not terminate control flow ("that is recoverable").

Re comment 9 there, x86 must assume ud2 will abort. If the compiler were
to emit ud2 for non-abort purposes then it needs to somehow annotate
this so that both objtool and the kernel can determine this.

That said; the whole annotate_reachable() thing in our WARN is
superfluous, we should read __bug_table instead. That said, we still
need _ASM_REACHABLE to handle a few noreturn cases, so there is no real
pressing reason to go clean that up. (if only the compiler would tell us
about noreturn)

ARM has an immediate in their break instruction and varies the desired
behaviour depending on the immediate.


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-06-25  6:46   ` Christophe Leroy
  2022-06-27 15:21     ` Sathvika Vasireddy
  2022-06-27 15:35     ` Sathvika Vasireddy
@ 2022-07-04 12:05     ` Peter Zijlstra
  2022-07-04 12:44       ` Christophe Leroy
  2 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 12:05 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: aik, linux-kernel, rostedt, mingo, Sathvika Vasireddy, jpoimboe,
	paulus, naveen.n.rao, mbenes, linuxppc-dev

On Sat, Jun 25, 2022 at 06:46:54AM +0000, Christophe Leroy wrote:
> 
> 
> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
> > objtool is throwing *unannotated intra-function call*
> > warnings with a few instructions that are marked
> > unreachable. Remove unreachable() from WARN_ON()
> > to fix these warnings, as the codegen remains same
> > with and without unreachable() in WARN_ON().
> 
> Did you try the two exemples described in commit 1e688dd2a3d6 
> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with 
> asm goto") ?
> 
> Without your patch:
> 
> 00000640 <test>:
>   640:	81 23 00 84 	lwz     r9,132(r3)
>   644:	71 29 40 00 	andi.   r9,r9,16384
>   648:	40 82 00 0c 	bne     654 <test+0x14>
>   64c:	80 63 00 0c 	lwz     r3,12(r3)
>   650:	4e 80 00 20 	blr
>   654:	0f e0 00 00 	twui    r0,0
> 
> 00000658 <test9w>:
>   658:	2c 04 00 00 	cmpwi   r4,0
>   65c:	41 82 00 0c 	beq     668 <test9w+0x10>
>   660:	7c 63 23 96 	divwu   r3,r3,r4
>   664:	4e 80 00 20 	blr
>   668:	0f e0 00 00 	twui    r0,0
>   66c:	38 60 00 00 	li      r3,0
>   670:	4e 80 00 20 	blr

Per this construct you should do as x86 does and assume twui terminates
control flow and explicitly annotate the WARN case. That is, given the
fact that BUG as no instructions following it, you can't very well
annotate that.

Alternatively, you can teach objtool to look at __bug_table to
distinguish these cases.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-07-04 11:45         ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Peter Zijlstra
@ 2022-07-04 12:34           ` Christophe Leroy
  2022-07-05 15:48             ` Segher Boessenkool
  0 siblings, 1 reply; 41+ messages in thread
From: Christophe Leroy @ 2022-07-04 12:34 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Marc Zyngier, aik, Sathvika Vasireddy, linux-kernel, rostedt,
	Chen Zhongjin, mingo, Sathvika Vasireddy, jpoimboe, paulus,
	naveen.n.rao, mbenes, linuxppc-dev, Linux ARM



Le 04/07/2022 à 13:45, Peter Zijlstra a écrit :
> On Wed, Jun 29, 2022 at 06:30:23PM +0000, Christophe Leroy wrote:
> 
> 
>> The problem is that that function has size 0:
>>
>> 00000000000003d0 l     F .text	0000000000000000
>> qdisc_root_sleeping_lock.part.0
> 
> I'm somewhat confused; how is an empty STT_FUNC a valid construct on
> Power?

So am I. It is likely not a valid construct, but that's what GCC seems 
to generate when you call annotate_unreachable().

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-07-04 12:05     ` Peter Zijlstra
@ 2022-07-04 12:44       ` Christophe Leroy
  2022-07-04 14:19         ` Peter Zijlstra
  0 siblings, 1 reply; 41+ messages in thread
From: Christophe Leroy @ 2022-07-04 12:44 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: aik, linux-kernel, rostedt, mingo, Sathvika Vasireddy, jpoimboe,
	paulus, naveen.n.rao, mbenes, linuxppc-dev



Le 04/07/2022 à 14:05, Peter Zijlstra a écrit :
> On Sat, Jun 25, 2022 at 06:46:54AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>> objtool is throwing *unannotated intra-function call*
>>> warnings with a few instructions that are marked
>>> unreachable. Remove unreachable() from WARN_ON()
>>> to fix these warnings, as the codegen remains same
>>> with and without unreachable() in WARN_ON().
>>
>> Did you try the two exemples described in commit 1e688dd2a3d6
>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
>> asm goto") ?
>>
>> Without your patch:
>>
>> 00000640 <test>:
>>    640:	81 23 00 84 	lwz     r9,132(r3)
>>    644:	71 29 40 00 	andi.   r9,r9,16384
>>    648:	40 82 00 0c 	bne     654 <test+0x14>
>>    64c:	80 63 00 0c 	lwz     r3,12(r3)
>>    650:	4e 80 00 20 	blr
>>    654:	0f e0 00 00 	twui    r0,0
>>
>> 00000658 <test9w>:
>>    658:	2c 04 00 00 	cmpwi   r4,0
>>    65c:	41 82 00 0c 	beq     668 <test9w+0x10>
>>    660:	7c 63 23 96 	divwu   r3,r3,r4
>>    664:	4e 80 00 20 	blr
>>    668:	0f e0 00 00 	twui    r0,0
>>    66c:	38 60 00 00 	li      r3,0
>>    670:	4e 80 00 20 	blr
> 
> Per this construct you should do as x86 does and assume twui terminates
> control flow and explicitly annotate the WARN case. That is, given the
> fact that BUG as no instructions following it, you can't very well
> annotate that.

That exactly the problem I guess. I'm fine with replacing the 
unreachable() by __builtin_unreachable() with our __WARN_FLAGS() and 
BUG() but we will still have a problem with some of the unrachable() 
that are in core parts of the kernel.

Even the ones in arch/powerpc/, they are valid and should remain. The 
point seems that the generic annotate_unreachable() is wrong for powerpc 
as is, and activating CONFIG_OBJTOOL lead to bad code generation.

By the way, for which functionnalities of objtool is that analysis 
necessary ? I understand it is not necessary to mcount accounting, so 
maybe the not empty annotate_unreachable() should be limited to those 
those functionnalities ?

> 
> Alternatively, you can teach objtool to look at __bug_table to
> distinguish these cases.

Isn't it enough to tell objtool that execution never go past twui, using 
INSN_BUG ?
By the way, for __WARN_FLAGS, we use the __extable for the continuation. 
Is objtools able to follow __extable ?

Christophe

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-07-04 12:44       ` Christophe Leroy
@ 2022-07-04 14:19         ` Peter Zijlstra
  0 siblings, 0 replies; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 14:19 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: aik, linux-kernel, rostedt, mingo, Sathvika Vasireddy, jpoimboe,
	paulus, naveen.n.rao, mbenes, linuxppc-dev

On Mon, Jul 04, 2022 at 12:44:30PM +0000, Christophe Leroy wrote:
> 
> 
> Le 04/07/2022 à 14:05, Peter Zijlstra a écrit :
> > On Sat, Jun 25, 2022 at 06:46:54AM +0000, Christophe Leroy wrote:
> >>
> >>
> >> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
> >>> objtool is throwing *unannotated intra-function call*
> >>> warnings with a few instructions that are marked
> >>> unreachable. Remove unreachable() from WARN_ON()
> >>> to fix these warnings, as the codegen remains same
> >>> with and without unreachable() in WARN_ON().
> >>
> >> Did you try the two exemples described in commit 1e688dd2a3d6
> >> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with
> >> asm goto") ?
> >>
> >> Without your patch:
> >>
> >> 00000640 <test>:
> >>    640:	81 23 00 84 	lwz     r9,132(r3)
> >>    644:	71 29 40 00 	andi.   r9,r9,16384
> >>    648:	40 82 00 0c 	bne     654 <test+0x14>
> >>    64c:	80 63 00 0c 	lwz     r3,12(r3)
> >>    650:	4e 80 00 20 	blr
> >>    654:	0f e0 00 00 	twui    r0,0
> >>
> >> 00000658 <test9w>:
> >>    658:	2c 04 00 00 	cmpwi   r4,0
> >>    65c:	41 82 00 0c 	beq     668 <test9w+0x10>
> >>    660:	7c 63 23 96 	divwu   r3,r3,r4
> >>    664:	4e 80 00 20 	blr
> >>    668:	0f e0 00 00 	twui    r0,0
> >>    66c:	38 60 00 00 	li      r3,0
> >>    670:	4e 80 00 20 	blr
> > 
> > Per this construct you should do as x86 does and assume twui terminates
> > control flow and explicitly annotate the WARN case. That is, given the
> > fact that BUG as no instructions following it, you can't very well
> > annotate that.
> 
> That exactly the problem I guess. I'm fine with replacing the 
> unreachable() by __builtin_unreachable() with our __WARN_FLAGS() and 
> BUG() but we will still have a problem with some of the unrachable() 
> that are in core parts of the kernel.
> 
> Even the ones in arch/powerpc/, they are valid and should remain. The 
> point seems that the generic annotate_unreachable() is wrong for powerpc 
> as is, and activating CONFIG_OBJTOOL lead to bad code generation.

Right; I'm not against making that depend on yet-another OBJTOOL_$config
thing.

> By the way, for which functionnalities of objtool is that analysis 
> necessary ? I understand it is not necessary to mcount accounting, so 
> maybe the not empty annotate_unreachable() should be limited to those 
> those functionnalities ?

For all the things where it needs to follow control flow, so stack
validation, ORC generation, unreachable instruction validation etc..

I'd need to double check code-gen on x86, but the way
__builtin_unreachable() makes code-gen stop dead, I'm not entirely sure
it's right for us either.

> > Alternatively, you can teach objtool to look at __bug_table to
> > distinguish these cases.
> 
> Isn't it enough to tell objtool that execution never go past twui, using 
> INSN_BUG ?

That should work I suppose.

> By the way, for __WARN_FLAGS, we use the __extable for the continuation. 
> Is objtools able to follow __extable ?

Yes.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types
  2022-07-04 11:14   ` Peter Zijlstra
@ 2022-07-04 15:53     ` Christophe Leroy
  2022-07-04 16:18       ` Peter Zijlstra
  0 siblings, 1 reply; 41+ messages in thread
From: Christophe Leroy @ 2022-07-04 15:53 UTC (permalink / raw)
  To: Peter Zijlstra, Sathvika Vasireddy, g
  Cc: aik, linux-kernel, rostedt, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes, linuxppc-dev



Le 04/07/2022 à 13:14, Peter Zijlstra a écrit :
> On Sat, Jun 25, 2022 at 12:02:33AM +0530, Sathvika Vasireddy wrote:
>> Make relocation types architecture specific.
>>
>> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
>> ---
>>   tools/objtool/arch/x86/include/arch/elf.h | 2 ++
>>   tools/objtool/check.c                     | 2 +-
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/objtool/arch/x86/include/arch/elf.h b/tools/objtool/arch/x86/include/arch/elf.h
>> index 69cc4264b28a..ac14987cf687 100644
>> --- a/tools/objtool/arch/x86/include/arch/elf.h
>> +++ b/tools/objtool/arch/x86/include/arch/elf.h
>> @@ -2,5 +2,7 @@
>>   #define _OBJTOOL_ARCH_ELF
>>   
>>   #define R_NONE R_X86_64_NONE
>> +#define R_ABS64 R_X86_64_64
>> +#define R_ABS32 R_X86_64_32
>>   
>>   #endif /* _OBJTOOL_ARCH_ELF */
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 98e869721bc4..88f68269860e 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -834,7 +834,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
>>   		memset(loc, 0, size);
>>   
>>   		if (elf_add_reloc_to_insn(file->elf, sec, idx,
>> -					  R_X86_64_64,
>> +					  size == sizeof(u64) ? R_ABS64 : R_ABS32,
>>   					  insn->sec, insn->offset))
>>   			return -1;
>>   
> 
> Given cross compiles, should this not also be elf dependent?

size is elf dependent (From the same series [RFC PATCH v3 03/12] 
objtool: Use target file class size instead of a compiled constant)

R_ABS64 and R_ABS32 are defined in the architecture elf.h, and this is 
the architecture for which you are building your kernel, not the 
architecture you cross compile on.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types
  2022-07-04 15:53     ` Christophe Leroy
@ 2022-07-04 16:18       ` Peter Zijlstra
  0 siblings, 0 replies; 41+ messages in thread
From: Peter Zijlstra @ 2022-07-04 16:18 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: mbenes, aik, linux-kernel, rostedt, mingo, Sathvika Vasireddy,
	jpoimboe, paulus, naveen.n.rao, g, linuxppc-dev

On Mon, Jul 04, 2022 at 03:53:50PM +0000, Christophe Leroy wrote:
> 
> 
> Le 04/07/2022 à 13:14, Peter Zijlstra a écrit :
> > On Sat, Jun 25, 2022 at 12:02:33AM +0530, Sathvika Vasireddy wrote:
> >> Make relocation types architecture specific.
> >>
> >> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
> >> ---
> >>   tools/objtool/arch/x86/include/arch/elf.h | 2 ++
> >>   tools/objtool/check.c                     | 2 +-
> >>   2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/objtool/arch/x86/include/arch/elf.h b/tools/objtool/arch/x86/include/arch/elf.h
> >> index 69cc4264b28a..ac14987cf687 100644
> >> --- a/tools/objtool/arch/x86/include/arch/elf.h
> >> +++ b/tools/objtool/arch/x86/include/arch/elf.h
> >> @@ -2,5 +2,7 @@
> >>   #define _OBJTOOL_ARCH_ELF
> >>   
> >>   #define R_NONE R_X86_64_NONE
> >> +#define R_ABS64 R_X86_64_64
> >> +#define R_ABS32 R_X86_64_32
> >>   
> >>   #endif /* _OBJTOOL_ARCH_ELF */
> >> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> >> index 98e869721bc4..88f68269860e 100644
> >> --- a/tools/objtool/check.c
> >> +++ b/tools/objtool/check.c
> >> @@ -834,7 +834,7 @@ static int create_mcount_loc_sections(struct objtool_file *file)
> >>   		memset(loc, 0, size);
> >>   
> >>   		if (elf_add_reloc_to_insn(file->elf, sec, idx,
> >> -					  R_X86_64_64,
> >> +					  size == sizeof(u64) ? R_ABS64 : R_ABS32,
> >>   					  insn->sec, insn->offset))
> >>   			return -1;
> >>   
> > 
> > Given cross compiles, should this not also be elf dependent?
> 
> size is elf dependent (From the same series [RFC PATCH v3 03/12] 
> objtool: Use target file class size instead of a compiled constant)
> 
> R_ABS64 and R_ABS32 are defined in the architecture elf.h, and this is 
> the architecture for which you are building your kernel, not the 
> architecture you cross compile on.

Duh. Thanks!

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
  2022-07-04 12:34           ` Christophe Leroy
@ 2022-07-05 15:48             ` Segher Boessenkool
  0 siblings, 0 replies; 41+ messages in thread
From: Segher Boessenkool @ 2022-07-05 15:48 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Peter Zijlstra, Marc Zyngier, Sathvika Vasireddy, linuxppc-dev,
	linux-kernel, rostedt, aik, mingo, Sathvika Vasireddy, jpoimboe,
	paulus, naveen.n.rao, mbenes, Chen Zhongjin, Linux ARM

On Mon, Jul 04, 2022 at 12:34:08PM +0000, Christophe Leroy wrote:
> Le 04/07/2022 à 13:45, Peter Zijlstra a écrit :
> > I'm somewhat confused; how is an empty STT_FUNC a valid construct on
> > Power?
> 
> So am I. It is likely not a valid construct, but that's what GCC seems 
> to generate when you call annotate_unreachable().

It is a valid construct on (almost) all targets.  If the user chooses to
have executable code terminate in limbo, that is what the compiler will
do (and this can result in a code symbol with size 0).  Compare this to
data symbols with no size, the situation is quite similar.


Segher

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc
  2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
                   ` (11 preceding siblings ...)
  2022-06-24 18:32 ` [RFC PATCH v3 12/12] objtool/powerpc: Fix unannotated intra-function call warnings Sathvika Vasireddy
@ 2022-07-08 15:06 ` Christophe Leroy
  2022-07-08 15:42   ` Christophe Leroy
  12 siblings, 1 reply; 41+ messages in thread
From: Christophe Leroy @ 2022-07-08 15:06 UTC (permalink / raw)
  To: Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes



Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
> These patches are rebased on top of objtool/core
> branch of the tip tree, and are tested on
> ppc64le with ppc64le_defconfig.

Seems like objtool/core has been merged in 5.19. I was able to apply 
your series on top of powerpc/merge branch. Only a small conflict with 
patch "objtool: Add --mnop as an option to --mcount" because of commit 
b42d23065024 ("kbuild: factor out the common objtool arguments")

> 
> Christophe Leroy (3):
>    objtool: Fix SEGFAULT
>    objtool: Use target file endianness instead of a compiled constant
>    objtool: Use target file class size instead of a compiled constant
> 
> Sathvika Vasireddy(9):
>    objtool: Add --mnop as an option to --mcount
>    powerpc: Skip objtool from running on VDSO files
>    objtool: Read special sections with alts only when specific options are selected
>    objtool: Use macros to define arch specific reloc types
>    objtool: Add arch specific function arch_ftrace_match()
>    objtool/powerpc: Enable objtool to be built on ppc
>    objtool/powerpc: Add --mcount specific implementation
>    powerpc: Remove unreachable() from WARN_ON()
>    objtool/powerpc: Fix unannotated intra-function call warnings
> 
>   Makefile                                      |  4 +-
>   arch/powerpc/Kconfig                          |  2 +
>   arch/powerpc/include/asm/bug.h                |  1 -
>   arch/powerpc/kernel/entry_64.S                |  2 +
>   arch/powerpc/kernel/exceptions-64s.S          |  7 +-
>   arch/powerpc/kernel/head_64.S                 |  7 +-
>   arch/powerpc/kernel/misc_64.S                 |  4 +-
>   arch/powerpc/kernel/vdso/Makefile             |  2 +
>   arch/powerpc/kernel/vector.S                  |  4 +-
>   arch/powerpc/kvm/book3s_hv_interrupts.S       |  4 +-
>   arch/powerpc/kvm/book3s_hv_rmhandlers.S       | 25 +++--
>   arch/x86/Kconfig                              |  1 +
>   scripts/Makefile.build                        |  1 +
>   tools/objtool/arch/powerpc/Build              |  2 +
>   tools/objtool/arch/powerpc/decode.c           | 96 +++++++++++++++++++
>   .../arch/powerpc/include/arch/cfi_regs.h      | 11 +++
>   tools/objtool/arch/powerpc/include/arch/elf.h | 10 ++
>   .../arch/powerpc/include/arch/special.h       | 21 ++++
>   tools/objtool/arch/powerpc/special.c          | 19 ++++
>   tools/objtool/arch/x86/decode.c               |  8 ++
>   tools/objtool/arch/x86/include/arch/elf.h     |  2 +
>   .../arch/x86/include/arch/endianness.h        |  9 --
>   tools/objtool/builtin-check.c                 | 14 +++
>   tools/objtool/check.c                         | 51 +++++-----
>   tools/objtool/elf.c                           |  8 +-
>   tools/objtool/include/objtool/arch.h          |  2 +
>   tools/objtool/include/objtool/builtin.h       |  1 +
>   tools/objtool/include/objtool/elf.h           |  8 ++
>   tools/objtool/include/objtool/endianness.h    | 32 +++----
>   tools/objtool/orc_dump.c                      | 11 ++-
>   tools/objtool/orc_gen.c                       |  4 +-
>   tools/objtool/special.c                       |  3 +-
>   32 files changed, 305 insertions(+), 71 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/special.h
>   create mode 100644 tools/objtool/arch/powerpc/special.c
>   delete mode 100644 tools/objtool/arch/x86/include/arch/endianness.h
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 01/12] objtool: Fix SEGFAULT
  2022-06-24 18:32 ` [RFC PATCH v3 01/12] objtool: Fix SEGFAULT Sathvika Vasireddy
@ 2022-07-08 15:10   ` Christophe Leroy
  0 siblings, 0 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-07-08 15:10 UTC (permalink / raw)
  To: Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes



Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
> From: Christophe Leroy <christophe.leroy@csgroup.eu>

This patch needs some description. Could be:

   find_insn() will return NULL in case of failure. Check insn in order
   to avoid a kernel Oops for NULL pointer dereference.

> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>   tools/objtool/check.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 190b2f6e360a..6cb07e151588 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -203,7 +203,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
>   		return false;
>   
>   	insn = find_insn(file, func->sec, func->offset);
> -	if (!insn->func)
> +	if (!insn || !insn->func)
>   		return false;
>   
>   	func_for_each_insn(file, func, insn) {

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc
  2022-07-08 15:06 ` [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Christophe Leroy
@ 2022-07-08 15:42   ` Christophe Leroy
  0 siblings, 0 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-07-08 15:42 UTC (permalink / raw)
  To: Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes



Le 08/07/2022 à 17:06, Christophe Leroy a écrit :
> 
> 
> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>> These patches are rebased on top of objtool/core
>> branch of the tip tree, and are tested on
>> ppc64le with ppc64le_defconfig.
> 
> Seems like objtool/core has been merged in 5.19. I was able to apply 
> your series on top of powerpc/merge branch. Only a small conflict with 
> patch "objtool: Add --mnop as an option to --mcount" because of commit 
> b42d23065024 ("kbuild: factor out the common objtool arguments")


In order to build, you will also need the following new file:

diff --git a/arch/powerpc/include/asm/asm.h b/arch/powerpc/include/asm/asm.h
new file mode 100644
index 000000000000..86f46b604e9a
--- /dev/null
+++ b/arch/powerpc/include/asm/asm.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_ASM_H
+#define _ASM_POWERPC_ASM_H
+
+#define _ASM_PTR	" .long "
+
+#endif /* _ASM_POWERPC_ASM_H */



> 
>>
>> Christophe Leroy (3):
>>    objtool: Fix SEGFAULT
>>    objtool: Use target file endianness instead of a compiled constant
>>    objtool: Use target file class size instead of a compiled constant
>>
>> Sathvika Vasireddy(9):
>>    objtool: Add --mnop as an option to --mcount
>>    powerpc: Skip objtool from running on VDSO files
>>    objtool: Read special sections with alts only when specific options 
>> are selected
>>    objtool: Use macros to define arch specific reloc types
>>    objtool: Add arch specific function arch_ftrace_match()
>>    objtool/powerpc: Enable objtool to be built on ppc
>>    objtool/powerpc: Add --mcount specific implementation
>>    powerpc: Remove unreachable() from WARN_ON()
>>    objtool/powerpc: Fix unannotated intra-function call warnings
>>
>>   Makefile                                      |  4 +-
>>   arch/powerpc/Kconfig                          |  2 +
>>   arch/powerpc/include/asm/bug.h                |  1 -
>>   arch/powerpc/kernel/entry_64.S                |  2 +
>>   arch/powerpc/kernel/exceptions-64s.S          |  7 +-
>>   arch/powerpc/kernel/head_64.S                 |  7 +-
>>   arch/powerpc/kernel/misc_64.S                 |  4 +-
>>   arch/powerpc/kernel/vdso/Makefile             |  2 +
>>   arch/powerpc/kernel/vector.S                  |  4 +-
>>   arch/powerpc/kvm/book3s_hv_interrupts.S       |  4 +-
>>   arch/powerpc/kvm/book3s_hv_rmhandlers.S       | 25 +++--
>>   arch/x86/Kconfig                              |  1 +
>>   scripts/Makefile.build                        |  1 +
>>   tools/objtool/arch/powerpc/Build              |  2 +
>>   tools/objtool/arch/powerpc/decode.c           | 96 +++++++++++++++++++
>>   .../arch/powerpc/include/arch/cfi_regs.h      | 11 +++
>>   tools/objtool/arch/powerpc/include/arch/elf.h | 10 ++
>>   .../arch/powerpc/include/arch/special.h       | 21 ++++
>>   tools/objtool/arch/powerpc/special.c          | 19 ++++
>>   tools/objtool/arch/x86/decode.c               |  8 ++
>>   tools/objtool/arch/x86/include/arch/elf.h     |  2 +
>>   .../arch/x86/include/arch/endianness.h        |  9 --
>>   tools/objtool/builtin-check.c                 | 14 +++
>>   tools/objtool/check.c                         | 51 +++++-----
>>   tools/objtool/elf.c                           |  8 +-
>>   tools/objtool/include/objtool/arch.h          |  2 +
>>   tools/objtool/include/objtool/builtin.h       |  1 +
>>   tools/objtool/include/objtool/elf.h           |  8 ++
>>   tools/objtool/include/objtool/endianness.h    | 32 +++----
>>   tools/objtool/orc_dump.c                      | 11 ++-
>>   tools/objtool/orc_gen.c                       |  4 +-
>>   tools/objtool/special.c                       |  3 +-
>>   32 files changed, 305 insertions(+), 71 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/special.h
>>   create mode 100644 tools/objtool/arch/powerpc/special.c
>>   delete mode 100644 tools/objtool/arch/x86/include/arch/endianness.h
>>

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [RFC PATCH v3 03/12] objtool: Use target file class size instead of a compiled constant
  2022-06-24 18:32 ` [RFC PATCH v3 03/12] objtool: Use target file class size " Sathvika Vasireddy
@ 2022-07-08 17:35   ` Christophe Leroy
  0 siblings, 0 replies; 41+ messages in thread
From: Christophe Leroy @ 2022-07-08 17:35 UTC (permalink / raw)
  To: Sathvika Vasireddy, linuxppc-dev
  Cc: peterz, linux-kernel, rostedt, aik, mingo, paulus, jpoimboe,
	naveen.n.rao, mbenes



Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
> From: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> In order to allow using objtool on cross-built kernels,
> determine size of long from elf data instead of using
> sizeof(long) at build time.
> 
> For the time being this covers only mcount.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>   tools/objtool/check.c               | 16 +++++++++-------
>   tools/objtool/elf.c                 |  8 ++++++--
>   tools/objtool/include/objtool/elf.h |  8 ++++++++
>   3 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index cef1dd54d505..fabc0ea88747 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -802,9 +802,9 @@ static int create_ibt_endbr_seal_sections(struct objtool_file *file)
>   static int create_mcount_loc_sections(struct objtool_file *file)
>   {
>   	struct section *sec;
> -	unsigned long *loc;
>   	struct instruction *insn;
>   	int idx;
> +	int size = elf_class_size(file->elf);

Should be renamed addrsize as per Naveen comment.

>   
>   	sec = find_section_by_name(file->elf, "__mcount_loc");
>   	if (sec) {
> @@ -820,23 +820,25 @@ static int create_mcount_loc_sections(struct objtool_file *file)
>   	list_for_each_entry(insn, &file->mcount_loc_list, call_node)
>   		idx++;
>   
> -	sec = elf_create_section(file->elf, "__mcount_loc", 0, sizeof(unsigned long), idx);
> +	sec = elf_create_section(file->elf, "__mcount_loc", 0, size, idx);
>   	if (!sec)
>   		return -1;
>   
> +	sec->sh.sh_addralign = size;
> +
>   	idx = 0;
>   	list_for_each_entry(insn, &file->mcount_loc_list, call_node) {
> +		void *loc;
>   
> -		loc = (unsigned long *)sec->data->d_buf + idx;
> -		memset(loc, 0, sizeof(unsigned long));
> +		loc = sec->data->d_buf + idx;
> +		memset(loc, 0, size);
>   
> -		if (elf_add_reloc_to_insn(file->elf, sec,
> -					  idx * sizeof(unsigned long),
> +		if (elf_add_reloc_to_insn(file->elf, sec, idx,
>   					  R_X86_64_64,
>   					  insn->sec, insn->offset))
>   			return -1;
>   
> -		idx++;
> +		idx += size;
>   	}
>   
>   	return 0;
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index c25e957c1e52..63218f5799c2 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -1124,6 +1124,7 @@ static struct section *elf_create_rela_reloc_section(struct elf *elf, struct sec
>   {
>   	char *relocname;
>   	struct section *sec;
> +	int size = elf_class_size(elf);

Should be renamed addrsize as per Naveen comment.

>   
>   	relocname = malloc(strlen(base->name) + strlen(".rela") + 1);
>   	if (!relocname) {
> @@ -1133,7 +1134,10 @@ static struct section *elf_create_rela_reloc_section(struct elf *elf, struct sec
>   	strcpy(relocname, ".rela");
>   	strcat(relocname, base->name);
>   
> -	sec = elf_create_section(elf, relocname, 0, sizeof(GElf_Rela), 0);
> +	if (size == sizeof(u32))
> +		sec = elf_create_section(elf, relocname, 0, sizeof(Elf32_Rela), 0);
> +	else
> +		sec = elf_create_section(elf, relocname, 0, sizeof(GElf_Rela), 0);
>   	free(relocname);
>   	if (!sec)
>   		return NULL;
> @@ -1142,7 +1146,7 @@ static struct section *elf_create_rela_reloc_section(struct elf *elf, struct sec
>   	sec->base = base;
>   
>   	sec->sh.sh_type = SHT_RELA;
> -	sec->sh.sh_addralign = 8;
> +	sec->sh.sh_addralign = size;
>   	sec->sh.sh_link = find_section_by_name(elf, ".symtab")->idx;
>   	sec->sh.sh_info = base->idx;
>   	sec->sh.sh_flags = SHF_INFO_LINK;
> diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
> index adebfbc2b518..c720c4476828 100644
> --- a/tools/objtool/include/objtool/elf.h
> +++ b/tools/objtool/include/objtool/elf.h
> @@ -141,6 +141,14 @@ static inline bool has_multiple_files(struct elf *elf)
>   	return elf->num_files > 1;
>   }
>   
> +static inline int elf_class_size(struct elf *elf)

Should be renamed elf_class_addrsize() as per Naveen comment.

> +{
> +	if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
> +		return sizeof(u32);
> +	else
> +		return sizeof(u64);
> +}
> +
>   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);
>   

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2022-07-08 17:37 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-24 18:32 [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 01/12] objtool: Fix SEGFAULT Sathvika Vasireddy
2022-07-08 15:10   ` Christophe Leroy
2022-06-24 18:32 ` [RFC PATCH v3 02/12] objtool: Use target file endianness instead of a compiled constant Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 03/12] objtool: Use target file class size " Sathvika Vasireddy
2022-07-08 17:35   ` Christophe Leroy
2022-06-24 18:32 ` [RFC PATCH v3 04/12] objtool: Add --mnop as an option to --mcount Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 05/12] powerpc: Skip objtool from running on VDSO files Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 06/12] objtool: Read special sections with alts only when specific options are selected Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 07/12] objtool: Use macros to define arch specific reloc types Sathvika Vasireddy
2022-07-04 11:14   ` Peter Zijlstra
2022-07-04 15:53     ` Christophe Leroy
2022-07-04 16:18       ` Peter Zijlstra
2022-06-24 18:32 ` [RFC PATCH v3 08/12] objtool: Add arch specific function arch_ftrace_match() Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 09/12] objtool/powerpc: Enable objtool to be built on ppc Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 10/12] objtool/powerpc: Add --mcount specific implementation Sathvika Vasireddy
2022-06-24 18:32 ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Sathvika Vasireddy
2022-06-25  6:46   ` Christophe Leroy
2022-06-27 15:21     ` Sathvika Vasireddy
2022-06-27 15:35     ` Sathvika Vasireddy
2022-06-27 15:46       ` Christophe Leroy
2022-06-29 18:30       ` Christophe Leroy
2022-06-30  8:05         ` Naveen N. Rao
2022-06-30  9:58           ` Christophe Leroy
2022-06-30 10:33             ` Christophe Leroy
2022-06-30 10:37             ` Naveen N. Rao
2022-06-30 15:58               ` Segher Boessenkool
2022-07-04 12:01                 ` Peter Zijlstra
2022-07-04 11:43               ` Peter Zijlstra
2022-07-01  2:13           ` Chen Zhongjin
2022-07-01  6:56             ` Sathvika Vasireddy
2022-07-01 11:40               ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() (gcc issue ?) Christophe Leroy
2022-07-04 11:45         ` [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() Peter Zijlstra
2022-07-04 12:34           ` Christophe Leroy
2022-07-05 15:48             ` Segher Boessenkool
2022-07-04 12:05     ` Peter Zijlstra
2022-07-04 12:44       ` Christophe Leroy
2022-07-04 14:19         ` Peter Zijlstra
2022-06-24 18:32 ` [RFC PATCH v3 12/12] objtool/powerpc: Fix unannotated intra-function call warnings Sathvika Vasireddy
2022-07-08 15:06 ` [RFC PATCH v3 00/12] objtool: Enable and implement --mcount option on powerpc Christophe Leroy
2022-07-08 15:42   ` Christophe Leroy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).