[PATCH v2 0/4] Remove dependency of check subcmd upon orc

[PATCH v2 0/4] Remove dependency of check subcmd upon orc

[Julien Thierry]

Hi,

Matt Helsley's change[1] provided a base framework to opt-in/out
objtool subcommands at compile time. This makes it easier for
architectures to port objtool, one subcommand at a time.

Orc generation relies on the check operation implementation. However,
the way this is done causes the check implementation to depend on the
implementation of orc generation functions to call if orc generation is
requested. This means that in order to implement check subcmd, orc
subcmd also need to be implemented.

These patches aim at removing that dependency, having orc subcmd
being built on top of the check subcmd.

Changes since v1 [2]:
- Remove redundant check in create_orc pointed out by Miroslav

[1] https://www.spinics.net/lists/kernel/msg3510844.html
[2] https://lkml.org/lkml/2020/6/4/675

Cheers,

Julien

-->

Julien Thierry (4):
  objtool: Move object file loading out of check
  objtool: Move orc outside of check
  objtool: orc: Skip setting orc_entry for non-text sections
  objtool: orc_gen: Move orc_entry out of instruction structure

 tools/objtool/builtin-check.c |  7 ++-
 tools/objtool/builtin-orc.c   | 24 +++++++++-
 tools/objtool/check.c         | 45 ++++---------------
 tools/objtool/check.h         |  1 -
 tools/objtool/objtool.c       | 30 +++++++++++++
 tools/objtool/objtool.h       |  5 ++-
 tools/objtool/orc_gen.c       | 83 ++++++++++++++++++++---------------
 tools/objtool/weak.c          |  4 +-
 8 files changed, 119 insertions(+), 80 deletions(-)

--
2.21.1

[PATCH v2 1/4] objtool: Move object file loading out of check

[Julien Thierry]

Structure objtool_file can be used by different subcommands. In fact
it already is, by check and orc.

Provide a function that allows to initialize objtool_file, that builtin
can call, without relying on check to do the correct setup for them and
explicitly hand the objtool_file to them.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/builtin-check.c |  7 ++++++-
 tools/objtool/builtin-orc.c   |  8 +++++++-
 tools/objtool/check.c         | 37 +++++++++++------------------------
 tools/objtool/objtool.c       | 29 +++++++++++++++++++++++++++
 tools/objtool/objtool.h       |  4 +++-
 tools/objtool/weak.c          |  4 +---
 6 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 7a44174967b5..9f525d497308 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -41,6 +41,7 @@ const struct option check_options[] = {
 int cmd_check(int argc, const char **argv)
 {
 	const char *objname, *s;
+	struct objtool_file *file;
 
 	argc = parse_options(argc, argv, check_options, check_usage, 0);
 
@@ -53,5 +54,9 @@ int cmd_check(int argc, const char **argv)
 	if (s && !s[9])
 		vmlinux = true;
 
-	return check(objname, false);
+	file = objtool_setup_file(objname, false);
+	if (!file)
+		return 1;
+
+	return check(file, false);
 }
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index b1dfe2007962..3b700f477a11 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -31,13 +31,19 @@ int cmd_orc(int argc, const char **argv)
 		usage_with_options(orc_usage, check_options);
 
 	if (!strncmp(argv[0], "gen", 3)) {
+		struct objtool_file *file;
+
 		argc = parse_options(argc, argv, check_options, orc_usage, 0);
 		if (argc != 1)
 			usage_with_options(orc_usage, check_options);
 
 		objname = argv[0];
 
-		return check(objname, true);
+		file = objtool_setup_file(objname, true);
+		if (!file)
+			return 1;
+
+		return check(file, true);
 	}
 
 	if (!strcmp(argv[0], "dump")) {
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 63d65a702900..1638428df454 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -26,7 +26,6 @@ struct alternative {
 	bool skip_orig;
 };
 
-const char *objname;
 struct cfi_init_state initial_func_cfi;
 
 struct instruction *find_insn(struct objtool_file *file,
@@ -2710,36 +2709,22 @@ static int validate_reachable_instructions(struct objtool_file *file)
 	return 0;
 }
 
-static struct objtool_file file;
-
-int check(const char *_objname, bool orc)
+int check(struct objtool_file *file, bool orc)
 {
 	int ret, warnings = 0;
 
-	objname = _objname;
-
-	file.elf = elf_open_read(objname, orc ? O_RDWR : O_RDONLY);
-	if (!file.elf)
-		return 1;
-
-	INIT_LIST_HEAD(&file.insn_list);
-	hash_init(file.insn_hash);
-	file.c_file = find_section_by_name(file.elf, ".comment");
-	file.ignore_unreachables = no_unreachable;
-	file.hints = false;
-
 	arch_initial_func_cfi_state(&initial_func_cfi);
 
-	ret = decode_sections(&file);
+	ret = decode_sections(file);
 	if (ret < 0)
 		goto out;
 	warnings += ret;
 
-	if (list_empty(&file.insn_list))
+	if (list_empty(&file->insn_list))
 		goto out;
 
 	if (vmlinux && !validate_dup) {
-		ret = validate_vmlinux_functions(&file);
+		ret = validate_vmlinux_functions(file);
 		if (ret < 0)
 			goto out;
 
@@ -2748,39 +2733,39 @@ int check(const char *_objname, bool orc)
 	}
 
 	if (retpoline) {
-		ret = validate_retpoline(&file);
+		ret = validate_retpoline(file);
 		if (ret < 0)
 			return ret;
 		warnings += ret;
 	}
 
-	ret = validate_functions(&file);
+	ret = validate_functions(file);
 	if (ret < 0)
 		goto out;
 	warnings += ret;
 
-	ret = validate_unwind_hints(&file, NULL);
+	ret = validate_unwind_hints(file, NULL);
 	if (ret < 0)
 		goto out;
 	warnings += ret;
 
 	if (!warnings) {
-		ret = validate_reachable_instructions(&file);
+		ret = validate_reachable_instructions(file);
 		if (ret < 0)
 			goto out;
 		warnings += ret;
 	}
 
 	if (orc) {
-		ret = create_orc(&file);
+		ret = create_orc(file);
 		if (ret < 0)
 			goto out;
 
-		ret = create_orc_sections(&file);
+		ret = create_orc_sections(file);
 		if (ret < 0)
 			goto out;
 
-		ret = elf_write(file.elf);
+		ret = elf_write(file->elf);
 		if (ret < 0)
 			goto out;
 	}
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 58fdda510653..71c4122cf491 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -22,6 +22,8 @@
 #include <linux/kernel.h>
 
 #include "builtin.h"
+#include "objtool.h"
+#include "warn.h"
 
 struct cmd_struct {
 	const char *name;
@@ -39,6 +41,33 @@ static struct cmd_struct objtool_cmds[] = {
 
 bool help;
 
+const char *objname;
+static struct objtool_file file;
+
+struct objtool_file *objtool_setup_file(const char *_objname, bool writable)
+{
+	if (objname) {
+		if (strcmp(objname, _objname)) {
+			WARN("won't handle more than one file at a time");
+			return NULL;
+		}
+		return &file;
+	}
+	objname = _objname;
+
+	file.elf = elf_open_read(objname, writable ? O_RDWR : O_RDONLY);
+	if (!file.elf)
+		return NULL;
+
+	INIT_LIST_HEAD(&file.insn_list);
+	hash_init(file.insn_hash);
+	file.c_file = find_section_by_name(file.elf, ".comment");
+	file.ignore_unreachables = no_unreachable;
+	file.hints = false;
+
+	return &file;
+}
+
 static void cmd_usage(void)
 {
 	unsigned int i, longest = 0;
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index 528028a66816..e4f0ab5a4094 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -19,7 +19,9 @@ struct objtool_file {
 	bool ignore_unreachables, c_file, hints, rodata;
 };
 
-int check(const char *objname, bool orc);
+struct objtool_file *objtool_setup_file(const char *_objname, bool writable);
+
+int check(struct objtool_file *file, bool orc);
 int orc_dump(const char *objname);
 int create_orc(struct objtool_file *file);
 int create_orc_sections(struct objtool_file *file);
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 942ea5e8ac36..82698319f008 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -17,9 +17,7 @@
 	return ENOSYS;							\
 })
 
-const char __weak *objname;
-
-int __weak check(const char *_objname, bool orc)
+int __weak check(struct objtool_file *file, bool orc)
 {
 	UNSUPPORTED("check subcommand");
 }
-- 
2.21.1

Re: [PATCH v2 1/4] objtool: Move object file loading out of check

[Matt Helsley]

On Mon, Jun 08, 2020 at 08:12:00AM +0100, Julien Thierry wrote:
> Structure objtool_file can be used by different subcommands. In fact
> it already is, by check and orc.
> 
> Provide a function that allows to initialize objtool_file, that builtin
> can call, without relying on check to do the correct setup for them and
> explicitly hand the objtool_file to them.
> 
> Signed-off-by: Julien Thierry <jthierry@redhat.com>

Reviewed-by: Matt Helsley <mhelsley@vmware.com>

> ---
>  tools/objtool/builtin-check.c |  7 ++++++-
>  tools/objtool/builtin-orc.c   |  8 +++++++-
>  tools/objtool/check.c         | 37 +++++++++++------------------------
>  tools/objtool/objtool.c       | 29 +++++++++++++++++++++++++++
>  tools/objtool/objtool.h       |  4 +++-
>  tools/objtool/weak.c          |  4 +---
>  6 files changed, 57 insertions(+), 32 deletions(-)

[PATCH v2 2/4] objtool: Move orc outside of check

[Julien Thierry]

Now that the objtool_file can be obtained outside of the check function,
orc generation builtin no longer requires check to explicitly call its
orc related functions.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/builtin-check.c |  2 +-
 tools/objtool/builtin-orc.c   | 18 +++++++++++++++++-
 tools/objtool/check.c         | 16 +---------------
 tools/objtool/objtool.h       |  2 +-
 tools/objtool/weak.c          |  2 +-
 5 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 9f525d497308..2bd520446ef8 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -58,5 +58,5 @@ int cmd_check(int argc, const char **argv)
 	if (!file)
 		return 1;
 
-	return check(file, false);
+	return check(file);
 }
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index 3b700f477a11..833ce587c3d4 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -32,6 +32,7 @@ int cmd_orc(int argc, const char **argv)
 
 	if (!strncmp(argv[0], "gen", 3)) {
 		struct objtool_file *file;
+		int ret;
 
 		argc = parse_options(argc, argv, check_options, orc_usage, 0);
 		if (argc != 1)
@@ -43,7 +44,22 @@ int cmd_orc(int argc, const char **argv)
 		if (!file)
 			return 1;
 
-		return check(file, true);
+		ret = check(file);
+		if (ret)
+			return ret;
+
+		if (list_empty(&file->insn_list))
+			return 0;
+
+		ret = create_orc(file);
+		if (ret < 0)
+			return ret;
+
+		ret = create_orc_sections(file);
+		if (ret < 0)
+			return ret;
+
+		return elf_write(file->elf);
 	}
 
 	if (!strcmp(argv[0], "dump")) {
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 1638428df454..3fbb60fe94df 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2709,7 +2709,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
 	return 0;
 }
 
-int check(struct objtool_file *file, bool orc)
+int check(struct objtool_file *file)
 {
 	int ret, warnings = 0;
 
@@ -2756,20 +2756,6 @@ int check(struct objtool_file *file, bool orc)
 		warnings += ret;
 	}
 
-	if (orc) {
-		ret = create_orc(file);
-		if (ret < 0)
-			goto out;
-
-		ret = create_orc_sections(file);
-		if (ret < 0)
-			goto out;
-
-		ret = elf_write(file->elf);
-		if (ret < 0)
-			goto out;
-	}
-
 out:
 	if (ret < 0) {
 		/*
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index e4f0ab5a4094..be526f3d294d 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -21,7 +21,7 @@ struct objtool_file {
 
 struct objtool_file *objtool_setup_file(const char *_objname, bool writable);
 
-int check(struct objtool_file *file, bool orc);
+int check(struct objtool_file *file);
 int orc_dump(const char *objname);
 int create_orc(struct objtool_file *file);
 int create_orc_sections(struct objtool_file *file);
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 82698319f008..29180d599b08 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -17,7 +17,7 @@
 	return ENOSYS;							\
 })
 
-int __weak check(struct objtool_file *file, bool orc)
+int __weak check(struct objtool_file *file)
 {
 	UNSUPPORTED("check subcommand");
 }
-- 
2.21.1

Re: [PATCH v2 2/4] objtool: Move orc outside of check

[Matt Helsley]

On Mon, Jun 08, 2020 at 08:12:01AM +0100, Julien Thierry wrote:
> Now that the objtool_file can be obtained outside of the check function,
> orc generation builtin no longer requires check to explicitly call its
> orc related functions.
> 
> Signed-off-by: Julien Thierry <jthierry@redhat.com>

Reviewed-by: Matt Helsley <mhelsley@vmware.com>

> ---
>  tools/objtool/builtin-check.c |  2 +-
>  tools/objtool/builtin-orc.c   | 18 +++++++++++++++++-
>  tools/objtool/check.c         | 16 +---------------
>  tools/objtool/objtool.h       |  2 +-
>  tools/objtool/weak.c          |  2 +-
>  5 files changed, 21 insertions(+), 19 deletions(-)

[PATCH v2 3/4] objtool: orc: Skip setting orc_entry for non-text sections

[Julien Thierry]

Orc generation is only done for text sections, but some instructions
can be found in non-text sections (e.g. .discard.text sections).

Skip setting their orc sections since their whole sections will be
skipped for orc generation.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/orc_gen.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index c9549988121a..e74578640705 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -18,6 +18,9 @@ int create_orc(struct objtool_file *file)
 		struct cfi_reg *cfa = &insn->cfi.cfa;
 		struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
 
+		if (!insn->sec->text)
+			continue;
+
 		orc->end = insn->cfi.end;
 
 		if (cfa->base == CFI_UNDEFINED) {
-- 
2.21.1

[PATCH v2 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

[Julien Thierry]

One orc_entry is associated with each instruction in the object file,
but having the orc_entry contained by the instruction structure forces
architectures not implementing the orc subcommands to provide a dummy
definition of the orc_entry.

Avoid that by having orc_entries in a separate list, part of the
objtool_file.

Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 tools/objtool/check.h   |  1 -
 tools/objtool/objtool.c |  1 +
 tools/objtool/objtool.h |  1 +
 tools/objtool/orc_gen.c | 80 ++++++++++++++++++++++-------------------
 4 files changed, 46 insertions(+), 37 deletions(-)

diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 906b5210f7ca..49f9a5cc4228 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -42,7 +42,6 @@ struct instruction {
 	struct symbol *func;
 	struct list_head stack_ops;
 	struct cfi_state cfi;
-	struct orc_entry orc;
 };
 
 struct instruction *find_insn(struct objtool_file *file,
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 71c4122cf491..4b2e8013edb8 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -61,6 +61,7 @@ struct objtool_file *objtool_setup_file(const char *_objname, bool writable)
 
 	INIT_LIST_HEAD(&file.insn_list);
 	hash_init(file.insn_hash);
+	INIT_LIST_HEAD(&file.orc_data_list);
 	file.c_file = find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index be526f3d294d..e782c4206cb2 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -16,6 +16,7 @@ struct objtool_file {
 	struct elf *elf;
 	struct list_head insn_list;
 	DECLARE_HASHTABLE(insn_hash, 20);
+	struct list_head orc_data_list;
 	bool ignore_unreachables, c_file, hints, rodata;
 };
 
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index e74578640705..2c4e1974bbb5 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -9,18 +9,33 @@
 #include "check.h"
 #include "warn.h"
 
+struct orc_data {
+	struct list_head list;
+	struct instruction *insn;
+	struct orc_entry orc;
+};
+
 int create_orc(struct objtool_file *file)
 {
 	struct instruction *insn;
 
 	for_each_insn(file, insn) {
-		struct orc_entry *orc = &insn->orc;
 		struct cfi_reg *cfa = &insn->cfi.cfa;
 		struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
+		struct orc_entry *orc;
+		struct orc_data *od;
 
 		if (!insn->sec->text)
 			continue;
 
+		od = calloc(1, sizeof(*od));
+		if (!od)
+			return -1;
+		od->insn = insn;
+		list_add_tail(&od->list, &file->orc_data_list);
+
+		orc = &od->orc;
+
 		orc->end = insn->cfi.end;
 
 		if (cfa->base == CFI_UNDEFINED) {
@@ -139,7 +154,7 @@ static int create_orc_entry(struct elf *elf, struct section *u_sec, struct secti
 
 int create_orc_sections(struct objtool_file *file)
 {
-	struct instruction *insn, *prev_insn;
+	struct orc_data *od, *prev_od;
 	struct section *sec, *u_sec, *ip_relasec;
 	unsigned int idx;
 
@@ -157,23 +172,21 @@ int create_orc_sections(struct objtool_file *file)
 
 	/* count the number of needed orcs */
 	idx = 0;
-	for_each_sec(file, sec) {
-		if (!sec->text)
-			continue;
-
-		prev_insn = NULL;
-		sec_for_each_insn(file, sec, insn) {
-			if (!prev_insn ||
-			    memcmp(&insn->orc, &prev_insn->orc,
-				   sizeof(struct orc_entry))) {
-				idx++;
-			}
-			prev_insn = insn;
+	prev_od = NULL;
+	list_for_each_entry(od, &file->orc_data_list, list) {
+		if (!prev_od ||
+		    memcmp(&od->orc, &prev_od->orc, sizeof(struct orc_entry))) {
+			idx++;
 		}
 
+		prev_od = od;
+
 		/* section terminator */
-		if (prev_insn)
+		if (list_is_last(&od->insn->list, &file->insn_list) ||
+		    list_next_entry(od->insn, list)->sec != od->insn->sec) {
+			prev_od = NULL;
 			idx++;
+		}
 	}
 	if (!idx)
 		return -1;
@@ -194,33 +207,28 @@ int create_orc_sections(struct objtool_file *file)
 
 	/* populate sections */
 	idx = 0;
-	for_each_sec(file, sec) {
-		if (!sec->text)
-			continue;
-
-		prev_insn = NULL;
-		sec_for_each_insn(file, sec, insn) {
-			if (!prev_insn || memcmp(&insn->orc, &prev_insn->orc,
-						 sizeof(struct orc_entry))) {
-
-				if (create_orc_entry(file->elf, u_sec, ip_relasec, idx,
-						     insn->sec, insn->offset,
-						     &insn->orc))
-					return -1;
-
-				idx++;
-			}
-			prev_insn = insn;
+	prev_od = NULL;
+	list_for_each_entry(od, &file->orc_data_list, list) {
+		if (!prev_od ||
+		    memcmp(&od->orc, &prev_od->orc, sizeof(struct orc_entry))) {
+			if (create_orc_entry(file->elf, u_sec, ip_relasec, idx,
+					     od->insn->sec, od->insn->offset,
+					     &od->orc))
+				return -1;
+			idx++;
 		}
 
+		prev_od = od;
+
 		/* section terminator */
-		if (prev_insn) {
+		if (list_is_last(&od->insn->list, &file->insn_list) ||
+		    list_next_entry(od->insn, list)->sec != od->insn->sec) {
 			if (create_orc_entry(file->elf, u_sec, ip_relasec, idx,
-					     prev_insn->sec,
-					     prev_insn->offset + prev_insn->len,
+					     prev_od->insn->sec,
+					     prev_od->insn->offset + prev_od->insn->len,
 					     &empty))
 				return -1;
-
+			prev_od = NULL;
 			idx++;
 		}
 	}
-- 
2.21.1

Re: [PATCH v2 0/4] Remove dependency of check subcmd upon orc

[Miroslav Benes]

On Mon, 8 Jun 2020, Julien Thierry wrote:

> Hi,
> 
> Matt Helsley's change[1] provided a base framework to opt-in/out
> objtool subcommands at compile time. This makes it easier for
> architectures to port objtool, one subcommand at a time.
> 
> Orc generation relies on the check operation implementation. However,
> the way this is done causes the check implementation to depend on the
> implementation of orc generation functions to call if orc generation is
> requested. This means that in order to implement check subcmd, orc
> subcmd also need to be implemented.
> 
> These patches aim at removing that dependency, having orc subcmd
> being built on top of the check subcmd.
> 
> Changes since v1 [2]:
> - Remove redundant check in create_orc pointed out by Miroslav
> 
> [1] https://www.spinics.net/lists/kernel/msg3510844.html
> [2] https://lkml.org/lkml/2020/6/4/675

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M