objtool/x86: noinstr vs PARAVIRT

The asm_cpu_bringup_and_idle() function is required to push the return value on the stack in order to make ORC happy, but the only reason objtool doesn't complain is because of a happy accident.

The thing is that asm_cpu_bringup_and_idle() doesn't return, so validate_branch() never terminates and falls through to the next function, which in the normal case is the hypercall_page. And that, as it happens, is 4095 NOPs and a RET.

Make asm_cpu_bringup_and_idle() terminate on it's own, by making the function it calls as a dead-end. This way we no longer rely on what code happens to come after.

Fixes c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

The asm_cpu_bringup_and_idle() function is required to push the return value on the stack in order to make ORC happy, but the only reason objtool doesn't complain is because of a happy accident.

The thing is that asm_cpu_bringup_and_idle() doesn't return, so validate_branch() never terminates and falls through to the next function, which in the normal case is the hypercall_page. And that, as it happens, is 4095 NOPs and a RET.

Make asm_cpu_bringup_and_idle() terminate on it's own, by making the function it calls as a dead-end. This way we no longer rely on what code happens to come after.

Fixes c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

The asm_cpu_bringup_and_idle() function is required to push the return value on the stack in order to make ORC happy, but the only reason objtool doesn't complain is because of a happy accident.

The thing is that asm_cpu_bringup_and_idle() doesn't return, so validate_branch() never terminates and falls through to the next function, which in the normal case is the hypercall_page. And that, as it happens, is 4095 NOPs and a RET.

Make asm_cpu_bringup_and_idle() terminate on it's own, by making the function it calls as a dead-end. This way we no longer rely on what code happens to come after.

Fixes c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Quoted 9 lines · Peter Zijlstra

The asm_cpu_bringup_and_idle() function is required to push the return value on the stack in order to make ORC happy, but the only reason objtool doesn't complain is because of a happy accident.

The thing is that asm_cpu_bringup_and_idle() doesn't return, so validate_branch() never terminates and falls through to the next function, which in the normal case is the hypercall_page. And that, as it happens, is 4095 NOPs and a RET.

Quoted 6 lines

Make asm_cpu_bringup_and_idle() terminate on it's own, by making the function it calls as a dead-end. This way we no longer rely on what code happens to come after.

Fixes c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Looks right. Only problem is, with my assembler I get this:

arch/x86/kernel/head_64.o: warning: objtool: .text+0x5: unreachable instruction

Because gas insists on jumping over the page of nops...

0000000000000000 <asm_cpu_bringup_and_idle>:

       0:	e8 00 00 00 00       	callq  5 <asm_cpu_bringup_and_idle+0x5>
			1: R_X86_64_PLT32	cpu_bringup_and_idle-0x4
       5:	e9 f6 0f 00 00       	jmpq   1000 <xen_hypercall_set_trap_table>
       a:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
      11:	00 00 00 00 
      15:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
      1c:	00 00 00 00 
      20:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
      27:	00 00 00 00 
      2b:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
      32:	00 00 00 00 
      36:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
      3d:	00 00 00 00 

Quoted 24 lines · Josh Poimboeuf

Quoted 9 lines · Peter Zijlstra

The asm_cpu_bringup_and_idle() function is required to push the return value on the stack in order to make ORC happy, but the only reason objtool doesn't complain is because of a happy accident.

The thing is that asm_cpu_bringup_and_idle() doesn't return, so validate_branch() never terminates and falls through to the next function, which in the normal case is the hypercall_page. And that, as it happens, is 4095 NOPs and a RET.

Quoted 6 lines

Make asm_cpu_bringup_and_idle() terminate on it's own, by making the function it calls as a dead-end. This way we no longer rely on what code happens to come after.

Fixes c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Looks right. Only problem is, with my assembler I get this:

   arch/x86/kernel/head_64.o: warning: objtool: .text+0x5: unreachable instruction

Because gas insists on jumping over the page of nops...

0000000000000000 <asm_cpu_bringup_and_idle>:

Quoted 13 lines

        0:	e8 00 00 00 00       	callq  5 <asm_cpu_bringup_and_idle+0x5>
			1: R_X86_64_PLT32	cpu_bringup_and_idle-0x4
        5:	e9 f6 0f 00 00       	jmpq   1000 <xen_hypercall_set_trap_table>
        a:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
       11:	00 00 00 00
       15:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
       1c:	00 00 00 00
       20:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
       27:	00 00 00 00
       2b:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
       32:	00 00 00 00
       36:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
       3d:	00 00 00 00

Here's a fix:

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] x86/xen: Move hypercall_page to top of the file

Because hypercall_page is page-aligned, the assembler inexplicably adds an unreachable jump from after the end of the previous code to the beginning of hypercall_page.

That confuses objtool, understandably. It also creates significant text fragmentation. As a result, much of the object file is wasted text (nops).

Move hypercall_page to the beginning of the file to both prevent the text fragmentation and avoid the dead jump instruction.

$ size /tmp/head_64.before.o /tmp/head_64.after.o
    text	   data	    bss	    dec	    hex	filename
   10924	 307252	   4096	 322272	  4eae0	/tmp/head_64.before.o
    6823	 307252	   4096	 318171	  4dadb	/tmp/head_64.after.o

Signed-off-by Josh Poimboeuf <[email protected]

---
  arch/x86/xen/xen-head.S | 34 +++++++++++++++++-----------------
  1 file changed, 17 insertions(+), 17 deletions(-)

arch/x86/xen/xen-head.S +17 -17

index cb6538ae2fe0..488944d6d430 100644
@@ -20,6 +20,23 @@
  #include <xen/interface/xen-mca.h>
  #include <asm/xen/interface.h>
 
+.pushsection .text
+	.balign PAGE_SIZE
+SYM_CODE_START(hypercall_page)
+	.rept (PAGE_SIZE / 32)
+		UNWIND_HINT_FUNC
+		.skip 31, 0x90
+		ret
+	.endr
+
+#define HYPERCALL(n) \
+	.equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
+	.type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
+#include <asm/xen-hypercalls.h>
+#undef HYPERCALL
+SYM_CODE_END(hypercall_page)
+.popsection
+
  #ifdef CONFIG_XEN_PV
  	__INIT
  SYM_CODE_START(startup_xen)

@@ -64,23 +81,6 @@ SYM_CODE_END(asm_cpu_bringup_and_idle)
  #endif
  #endif
 
-.pushsection .text
-	.balign PAGE_SIZE
-SYM_CODE_START(hypercall_page)
-	.rept (PAGE_SIZE / 32)
-		UNWIND_HINT_FUNC
-		.skip 31, 0x90
-		ret
-	.endr
-
-#define HYPERCALL(n) \
-	.equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
-	.type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
-#include <asm/xen-hypercalls.h>
-#undef HYPERCALL
-SYM_CODE_END(hypercall_page)
-.popsection
-
  	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
  	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
  	ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")

Quoted 55 lines · Josh Poimboeuf

Quoted 23 lines · Josh Poimboeuf

Quoted 9 lines · Peter Zijlstra

The asm_cpu_bringup_and_idle() function is required to push the return value on the stack in order to make ORC happy, but the only reason objtool doesn't complain is because of a happy accident.

The thing is that asm_cpu_bringup_and_idle() doesn't return, so validate_branch() never terminates and falls through to the next function, which in the normal case is the hypercall_page. And that, as it happens, is 4095 NOPs and a RET.

Quoted 6 lines

Make asm_cpu_bringup_and_idle() terminate on it's own, by making the function it calls as a dead-end. This way we no longer rely on what code happens to come after.

Fixes c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Looks right.  Only problem is, with my assembler I get this:

   arch/x86/kernel/head_64.o: warning: objtool: .text+0x5: unreachable instruction

Because gas insists on jumping over the page of nops...

Quoted 14 lines

0000000000000000 <asm_cpu_bringup_and_idle>:
        0:    e8 00 00 00 00           callq  5 <asm_cpu_bringup_and_idle+0x5> 1: R_X86_64_PLT32    cpu_bringup_and_idle-0x4
        5:    e9 f6 0f 00 00           jmpq   1000 <xen_hypercall_set_trap_table> a:    66 66 2e 0f 1f 84 00     data16 nopw %cs:0x0(%rax,%rax,1) 11:    00 00 00 00
       15:    66 66 2e 0f 1f 84 00     data16 nopw %cs:0x0(%rax,%rax,1) 1c:    00 00 00 00
       20:    66 66 2e 0f 1f 84 00     data16 nopw %cs:0x0(%rax,%rax,1) 27:    00 00 00 00
       2b:    66 66 2e 0f 1f 84 00     data16 nopw %cs:0x0(%rax,%rax,1) 32:    00 00 00 00
       36:    66 66 2e 0f 1f 84 00     data16 nopw %cs:0x0(%rax,%rax,1) 3d:    00 00 00 00

Here's a fix:

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] x86/xen: Move hypercall_page to top of the file

Because hypercall_page is page-aligned, the assembler inexplicably adds an unreachable jump from after the end of the previous code to the beginning of hypercall_page.

That confuses objtool, understandably.  It also creates significant text fragmentation.  As a result, much of the object file is wasted text (nops).

Move hypercall_page to the beginning of the file to both prevent the text fragmentation and avoid the dead jump instruction.

Quoted 6 lines

$ size /tmp/head_64.before.o /tmp/head_64.after.o
    text       data        bss        dec        hex    filename
   10924     307252       4096     322272      4eae0    /tmp/head_64.before.o
    6823     307252       4096     318171      4dadb    /tmp/head_64.after.o

Signed-off-by Josh Poimboeuf <[email protected]

Reviewed-by Juergen Gross <[email protected]>

Juergen

Quoted 55 lines

---
  arch/x86/xen/xen-head.S | 34 +++++++++++++++++-----------------

  1 file changed, 17 insertions(+), 17 deletions(-)

arch/x86/xen/xen-head.S +17 -0

index cb6538ae2fe0..488944d6d430 100644
@@ -20,6 +20,23 @@
   #include <xen/interface/xen-mca.h>
   #include <asm/xen/interface.h>
+.pushsection .text
+    .balign PAGE_SIZE
+SYM_CODE_START(hypercall_page)
+    .rept (PAGE_SIZE / 32)
+        UNWIND_HINT_FUNC
+        .skip 31, 0x90
+        ret
+    .endr
+
+#define HYPERCALL(n) \
+    .equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
+    .type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
+#include <asm/xen-hypercalls.h>
+#undef HYPERCALL
+SYM_CODE_END(hypercall_page)
+.popsection
+
   #ifdef CONFIG_XEN_PV

      __INIT
  SYM_CODE_START(startup_xen)

arch/x86/xen/xen-head.S +0 -17

@@ -64,23 +81,6 @@ SYM_CODE_END(asm_cpu_bringup_and_idle)
   #endif
   #endif
-.pushsection .text
-    .balign PAGE_SIZE
-SYM_CODE_START(hypercall_page)
-    .rept (PAGE_SIZE / 32)
-        UNWIND_HINT_FUNC
-        .skip 31, 0x90
-        ret
-    .endr
-
-#define HYPERCALL(n) \
-    .equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
-    .type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
-#include <asm/xen-hypercalls.h>
-#undef HYPERCALL
-SYM_CODE_END(hypercall_page)
-.popsection
-
       ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
       ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
       ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")
 

Quoted 21 lines

Quoted 21 lines

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] x86/xen: Move hypercall_page to top of the file

Because hypercall_page is page-aligned, the assembler inexplicably adds an unreachable jump from after the end of the previous code to the beginning of hypercall_page.

That confuses objtool, understandably.  It also creates significant text fragmentation.  As a result, much of the object file is wasted text (nops).

Move hypercall_page to the beginning of the file to both prevent the text fragmentation and avoid the dead jump instruction.

$ size /tmp/head_64.before.o /tmp/head_64.after.o
    text       data        bss        dec        hex    filename
   10924     307252       4096     322272      4eae0
/tmp/head_64.before.o
    6823     307252       4096     318171      4dadb
/tmp/head_64.after.o

Quoted 3 lines

Quoted 1 line

Signed-off-by Josh Poimboeuf <[email protected]

Reviewed-by Juergen Gross <[email protected]>

Umm, will this be carried through the tip tree, or shall I take it in the xen tree?

Quoted 23 lines · Juergen Gross

Quoted 23 lines

Quoted 22 lines

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] x86/xen: Move hypercall_page to top of the file

Because hypercall_page is page-aligned, the assembler inexplicably adds an unreachable jump from after the end of the previous code to the beginning of hypercall_page.

That confuses objtool, understandably.  It also creates significant text fragmentation.  As a result, much of the object file is wasted text (nops).

Move hypercall_page to the beginning of the file to both prevent the text fragmentation and avoid the dead jump instruction.

$ size /tmp/head_64.before.o /tmp/head_64.after.o
     text       data        bss        dec        hex    filename
    10924     307252       4096     322272      4eae0
/tmp/head_64.before.o
     6823     307252       4096     318171      4dadb
/tmp/head_64.after.o

Signed-off-by Josh Poimboeuf <[email protected]

Quoted 4 lines

Quoted 1 line

Reviewed-by Juergen Gross <[email protected]>

Umm, will this be carried through the tip tree, or shall I take it in the xen tree?

I have it in the whole x86/objtool/paravirt series. I you want it in the Xen tree, I'll be happy to drop it from there, although I hope to get all that merged this cycles somewhere.

tools/objtool/check.c +8 -2

+		printf("nr_cfi: %ld\n", nr_cfi);
+		printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
+		printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
+	}
+
+++ b/tools/objtool/orc_gen.c

@@ -13,13 +13,19 @@
 #include <objtool/warn.h>
 #include <objtool/endianness.h>
 
-static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi)
+static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
+			  struct instruction *insn)
 {
-	struct instruction *insn = container_of(cfi, struct instruction, cfi);

tools/objtool/orc_gen.c +1 -1

 	orc->end = cfi->end;
 
 	if (cfi->cfa.base == CFI_UNDEFINED) {

@@ -162,7 +168,7 @@ int orc_create(struct objtool_file *file
 			int i;
 
 			if (!alt_group) {
-				if (init_orc_entry(&orc, &insn->cfi))
+				if (init_orc_entry(&orc, insn->cfip, insn))
 					return -1;
 				if (!memcmp(&prev_orc, &orc, sizeof(orc)))
 					continue;

tools/objtool/check.c static int validate_branch(struct objtoo · line 2824

Quoted 7 lines

tools/objtool/check.c +2 -1

@@ -2725,15 +2820,24 @@ static int validate_branch(struct objtoo
 
 			if (insn->visited & visited)
 				return 0;
-		}
+		} else
+			nr_visited++;

just a coding style

	} else {
		nr_visited++;
	}

tools/objtool/check.c int check(struct objtool_file *file) · line 3308

Quoted 6 lines

tools/objtool/check.c +2 -0

@@ -3192,6 +3304,13 @@ int check(struct objtool_file *file)
 		warnings += ret;
 	}
 
+	if (stats) {
+		printf("nr_visited: %ld\n", nr_visited);

Could we make this nr_insns_visited?

Quoted 19 lines

tools/objtool/check.c +8 -2

+		printf("nr_cfi: %ld\n", nr_cfi);
+		printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
+		printf("nr_cfi_cache: %ld\n", nr_cfi_cache);
+	}
+
+++ b/tools/objtool/orc_gen.c

@@ -13,13 +13,19 @@
 #include <objtool/warn.h>
 #include <objtool/endianness.h>
 
-static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi)
+static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
+			  struct instruction *insn)
 {
-	struct instruction *insn = container_of(cfi, struct instruction, cfi);
 	struct cfi_reg *bp = &cfi->regs[CFI_BP];
 
 	memset(orc, 0, sizeof(*orc));
 

tools/objtool/orc_gen.c line 28

Quoted 6 lines

tools/objtool/orc_gen.c +6 -0

+	if (!cfi) {
+		orc->end = 0;
+		orc->sp_reg = ORC_REG_UNDEFINED;
+		return 0;
+	}
+

This looks strange. You access cfi a couple of lines earlier (bp cfi_reg initialization).

Is it even possible to have cfi == NULL here? The second call site below has a check and the first one should not happen (insn->cfip should be always set if I am not missing something).

Quoted 14 lines

Quoted 14 lines

+++ b/tools/objtool/orc_gen.c

tools/objtool/orc_gen.c +2 -2

@@ -13,13 +13,19 @@
 #include <objtool/warn.h>
 #include <objtool/endianness.h>
 
-static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi)
+static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
+			  struct instruction *insn)
 {
-	struct instruction *insn = container_of(cfi, struct instruction, cfi);
 	struct cfi_reg *bp = &cfi->regs[CFI_BP];
 
 	memset(orc, 0, sizeof(*orc));
 

Quoted 9 lines

tools/objtool/orc_gen.c line 28

Quoted 6 lines

tools/objtool/orc_gen.c +6 -0

+	if (!cfi) {
+		orc->end = 0;
+		orc->sp_reg = ORC_REG_UNDEFINED;
+		return 0;
+	}
+

This looks strange. You access cfi a couple of lines earlier (bp cfi_reg initialization).

That's a relative address compute, it doesn't actually dereference the pointer.

Quoted 3 lines

Is it even possible to have cfi == NULL here? The second call site below has a check and the first one should not happen (insn->cfip should be always set if I am not missing something).

I can happen for instructions that are not visited, like NOP fillers between functions. Remove the check and try to build a file :-)

Andi reported that objtool on vmlinux.o consumes more memory than his system has, leading to horrific performance.

This is in part because we keep a struct instruction for every instruction in the file in-memory. Shrink struct instruction by removing the CFI state (which includes full register state) from it and demand allocating it.

Given most instructions don't actually change CFI state, there's lots of repetition there, so add a hash table to find previous CFI instances.

Reduces memory consumption (and runtime) for processing an x86_64-allyesconfig:

  pre:  4:40.84 real,   143.99 user,    44.18 sys,      30624988 mem
  post: 2:14.61 real,   108.58 user,    25.04 sys,      16396184 mem

tools/objtool/check.c +7 -0

+static struct cfi_state *cfi_hash_find(struct cfi_state *cfi)
+{
+	struct hlist_head *head = &cfi_hash[hash_min(cfi_key(cfi), cfi_bits)];
+	struct cfi_state *obj;
+
+	hlist_for_each_entry(obj, head, hash) {
+		if (!cficmp(cfi, obj)) {

tools/objtool/check.c +2 -1

@@ -2725,15 +2820,24 @@ static int validate_branch(struct objtoo
 
 			if (insn->visited & visited)
 				return 0;
-		}
+		} else
+			nr_visited++;
 
 		if (state.noinstr)

Quoted 7 lines

(quoted diff) +7 -0

+static struct cfi_state *cfi_alloc(void)
+{
+	struct cfi_state *cfi = calloc(sizeof(struct cfi_state), 1);
+	if (!cfi) {
+		WARN("calloc failed");
+		exit(1);
+	}

Review

Quoted 4 lines

(quoted diff) +4 -0

+	nr_cfi++;
+	init_cfi_state(cfi);
+	return cfi;
+}

I'm thinking this should also add it to the hash. i.e. I don't think there's a scenario where you'd alloc a cfi and not want to add it to the hash. The more sharing the better.

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+struct cfi_state *insn_get_cfi(struct instruction *insn)
+{
+	if (!insn->cfip)
+		insn->cfip = cfi_alloc();
+
+	return insn->cfip;
+}

Call it something like insn_get_or_alloc_cfi()?

Also, the function can be static.

Quoted 11 lines

(quoted diff) +11 -0

+static struct cfi_state *cfi_hash_find(struct cfi_state *cfi)
+{
+	struct hlist_head *head = &cfi_hash[hash_min(cfi_key(cfi), cfi_bits)];
+	struct cfi_state *obj;
+
+	hlist_for_each_entry(obj, head, hash) {
+		if (!cficmp(cfi, obj)) {
+			nr_cfi_cache++;
+			return obj;
+		}
+	}

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+	obj = cfi_alloc();
+	*obj = *cfi;
+	hlist_add_head(&obj->hash, head);
+
+	return obj;

cfi_hash_find_or_alloc_cfi()?

Quoted 15 lines · Josh Poimboeuf

Review

Quoted 11 lines

(quoted diff) +11 -0

+static struct cfi_state *cfi_alloc(void)
+{
+	struct cfi_state *cfi = calloc(sizeof(struct cfi_state), 1);
+	if (!cfi) {
+		WARN("calloc failed");
+		exit(1);
+	}
+	nr_cfi++;
+	init_cfi_state(cfi);
+	return cfi;
+}

I'm thinking this should also add it to the hash. i.e. I don't think there's a scenario where you'd alloc a cfi and not want to add it to the hash. The more sharing the better.

Quoted 11 lines

tools/objtool/check.c +2 -1

@@ -2725,15 +2820,24 @@ static int validate_branch(struct objtoo
 
 			if (insn->visited & visited)
 				return 0;
-		}
+		} else
+			nr_visited++;
 
 		if (state.noinstr)
 			state.instr += insn->instr;
 

tools/objtool/check.c static int validate_branch(struct objtoo · line 2832

Quoted 8 lines

tools/objtool/check.c +4 -4

-		if (insn->hint)
-			state.cfi = insn->cfi;
-		else
-			insn->cfi = state.cfi;
+		if (insn->hint) {
+			state.cfi = *insn->cfip;
+		} else {
+			/* XXX track if we actually changed state.cfi */

Why would we do that?

tools/objtool/include/objtool/check.h struct instruction { · line 63

Quoted 7 lines

tools/objtool/include/objtool/check.h +2 -1

+++ b/tools/objtool/include/objtool/check.h

@@ -60,9 +60,11 @@ struct instruction {
 	struct list_head alts;
 	struct symbol *func;
 	struct list_head stack_ops;
-	struct cfi_state cfi;
+	struct cfi_state *cfip;

Not sure about this rename. Pointers generally don't need a 'p' postfix.

Turns out the compilers also generate tail calls to __sanitize_cov*(), make sure to also patch those out in noinstr code.

Fixes 0f1441b44e82 ("objtool: Fix noinstr vs KCOV")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 tools/objtool/arch/x86/decode.c      |   20 ++++
 tools/objtool/check.c                |  153 ++++++++++++++++++-----------------
 tools/objtool/include/objtool/arch.h |    1 
 3 files changed, 100 insertions(+), 74 deletions(-)

tools/objtool/arch/x86/decode.c +20 -0

@@ -659,6 +659,26 @@ const char *arch_nop_insn(int len)
 	return nops[len-1];
 }
 
+#define BYTE_RET	0xC3
+
+const char *arch_ret_insn(int len)
+{
+	static const char ret[5][5] = {
+		{ BYTE_RET },
+		{ BYTE_RET, BYTES_NOP1 },
+		{ BYTE_RET, BYTES_NOP2 },
+		{ BYTE_RET, BYTES_NOP3 },
+		{ BYTE_RET, BYTES_NOP4 },
+	};
+
+	if (len < 1 || len > 5) {
+		WARN("invalid RET size: %d\n", len);
+		return NULL;
+	}
+
+	return ret[len-1];
+}
+
 /* asm/alternative.h ? */
 
 #define ALTINSTR_FLAG_INV	(1 << 15)

tools/objtool/check.c +79 -74

@@ -828,6 +828,74 @@ static struct reloc *insn_reloc(struct o
 	return insn->reloc;
 }
 
+static void remove_insn_ops(struct instruction *insn)
+{
+	struct stack_op *op, *tmp;
+
+	list_for_each_entry_safe(op, tmp, &insn->stack_ops, list) {
+		list_del(&op->list);
+		free(op);
+	}
+}
+
+static void add_call_dest(struct objtool_file *file, struct instruction *insn,
+			  struct symbol *dest, bool sibling)
+{
+	struct reloc *reloc = insn_reloc(file, insn);
+
+	insn->call_dest = dest;
+	if (!dest)
+		return;
+
+	if (insn->call_dest->static_call_tramp) {
+		list_add_tail(&insn->call_node,
+			      &file->static_call_list);
+	}
+
+	if (insn->sec->noinstr &&
+	    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
+		if (reloc) {
+			reloc->type = R_NONE;
+			elf_write_reloc(file->elf, reloc);
+		}
+
+		elf_write_insn(file->elf, insn->sec,
+			       insn->offset, insn->len,
+			       sibling ? arch_ret_insn(insn->len)
+			               : arch_nop_insn(insn->len));
+
+		insn->type = sibling ? INSN_RETURN : INSN_NOP;
+	}
+
+	if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) {
+		if (sibling)
+			WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset);
+
+		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));
+
+		insn->type = INSN_NOP;
+
+		list_add_tail(&insn->mcount_loc_node,
+			      &file->mcount_loc_list);
+	}
+
+	/*
+	 * Whatever stack impact regular CALLs have, should be undone
+	 * by the RETURN of the called function.
+	 *
+	 * Annotated intra-function calls retain the stack_ops but
+	 * are converted to JUMP, see read_intra_function_calls().
+	 */
+	remove_insn_ops(insn);
+}
+
 /*
  * Find the destination instructions for all jumps.
  */

@@ -866,11 +934,7 @@ static int add_jump_destinations(struct
 			continue;
 		} else if (insn->func) {
 			/* internal or external sibling call (with reloc) */
-			insn->call_dest = reloc->sym;
-			if (insn->call_dest->static_call_tramp) {
-				list_add_tail(&insn->call_node,
-					      &file->static_call_list);
-			}
+			add_call_dest(file, insn, reloc->sym, true);
 			continue;
 		} else if (reloc->sym->sec->idx) {
 			dest_sec = reloc->sym->sec;

@@ -926,13 +990,8 @@ static int add_jump_destinations(struct
 
 			} else if (insn->jump_dest->func->pfunc != insn->func->pfunc &&
 				   insn->jump_dest->offset == insn->jump_dest->func->offset) {
-
 				/* internal sibling call (without reloc) */
-				insn->call_dest = insn->jump_dest->func;
-				if (insn->call_dest->static_call_tramp) {
-					list_add_tail(&insn->call_node,
-						      &file->static_call_list);
-				}
+				add_call_dest(file, insn, insn->jump_dest->func, true);
 			}
 		}
 	}

@@ -940,16 +999,6 @@ static int add_jump_destinations(struct
 	return 0;
 }
 
-static void remove_insn_ops(struct instruction *insn)
-{
-	struct stack_op *op, *tmp;
-
-	list_for_each_entry_safe(op, tmp, &insn->stack_ops, list) {
-		list_del(&op->list);
-		free(op);
-	}
-}
-
 static struct symbol *find_call_destination(struct section *sec, unsigned long offset)
 {
 	struct symbol *call_dest;

@@ -968,6 +1017,7 @@ static int add_call_destinations(struct
 {
 	struct instruction *insn;
 	unsigned long dest_off;
+	struct symbol *dest;
 	struct reloc *reloc;
 
 	for_each_insn(file, insn) {

@@ -977,7 +1027,9 @@ static int add_call_destinations(struct
 		reloc = insn_reloc(file, insn);
 		if (!reloc) {
 			dest_off = arch_jump_destination(insn);
-			insn->call_dest = find_call_destination(insn->sec, dest_off);
+			dest = find_call_destination(insn->sec, dest_off);
+
+			add_call_dest(file, insn, dest, false);
 
 			if (insn->ignore)
 				continue;

@@ -995,9 +1047,8 @@ static int add_call_destinations(struct
 
 		} else if (reloc->sym->type == STT_SECTION) {
 			dest_off = arch_dest_reloc_offset(reloc->addend);
-			insn->call_dest = find_call_destination(reloc->sym->sec,
-								dest_off);
-			if (!insn->call_dest) {
+			dest = find_call_destination(reloc->sym->sec, dest_off);
+			if (!dest) {
 				WARN_FUNC("can't find call dest symbol at %s+0x%lx",
 					  insn->sec, insn->offset,
 					  reloc->sym->sec->name,

@@ -1005,6 +1056,8 @@ static int add_call_destinations(struct
 				return -1;
 			}
 
+			add_call_dest(file, insn, dest, false);
+
 		} else if (arch_is_retpoline(reloc->sym)) {
 			/*
 			 * Retpoline calls are really dynamic calls in

@@ -1020,55 +1073,7 @@ static int add_call_destinations(struct
 			continue;
 
 		} else
-			insn->call_dest = reloc->sym;
-
-		if (insn->call_dest && insn->call_dest->static_call_tramp) {
-			list_add_tail(&insn->call_node,
-				      &file->static_call_list);
-		}
-
-		/*
-		 * Many compilers cannot disable KCOV with a function attribute
-		 * so they need a little help, NOP out any KCOV calls from noinstr
-		 * text.
-		 */
-		if (insn->sec->noinstr &&
-		    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
-			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));
-			insn->type = INSN_NOP;
-		}
-
-		if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) {
-			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));
-
-			insn->type = INSN_NOP;
-
-			list_add_tail(&insn->mcount_loc_node,
-				      &file->mcount_loc_list);
-		}
-
-		/*
-		 * Whatever stack impact regular CALLs have, should be undone
-		 * by the RETURN of the called function.
-		 *
-		 * Annotated intra-function calls retain the stack_ops but
-		 * are converted to JUMP, see read_intra_function_calls().
-		 */
-		remove_insn_ops(insn);
+			add_call_dest(file, insn, reloc->sym, false);
 	}
 
 	return 0;

tools/objtool/include/objtool/arch.h +1 -0

@@ -82,6 +82,7 @@ unsigned long arch_jump_destination(stru
 unsigned long arch_dest_reloc_offset(int addend);
 
 const char *arch_nop_insn(int len);
+const char *arch_ret_insn(int len);
 
 int arch_decode_hint_reg(struct instruction *insn, u8 sp_reg);
 

Quoted 5 lines · Peter Zijlstra

Turns out the compilers also generate tail calls to __sanitize_cov*(), make sure to also patch those out in noinstr code.

Fixes 0f1441b44e82 ("objtool: Fix noinstr vs KCOV")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Marco Elver <[email protected]>

Thanks! More reason to not even try to do the same for other architectures, and the compiler needing the attribute...

tools/objtool/check.c +22 -0

+static void add_call_dest(struct objtool_file *file, struct instruction *insn,
+			  struct symbol *dest, bool sibling)
+{
+	struct reloc *reloc = insn_reloc(file, insn);
+
+	insn->call_dest = dest;
+	if (!dest)
+		return;
+
+	if (insn->call_dest->static_call_tramp) {
+		list_add_tail(&insn->call_node,
+			      &file->static_call_list);
+	}
+
+	if (insn->sec->noinstr &&
+	    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
+		if (reloc) {
+			reloc->type = R_NONE;
+			elf_write_reloc(file->elf, reloc);
+		}
+
+		elf_write_insn(file->elf, insn->sec,

tools/objtool/check.c +6 -0

+
+		if (reloc) {
+			reloc->type = R_NONE;
+			elf_write_reloc(file->elf, reloc);
+		}
+

Quoted 26 lines · On Thu, 24 Jun 2021 11:41:02 +0200 Peter Zijlstra <[email protected]> wrote:

tools/objtool/check.c +26 -0

+static void add_call_dest(struct objtool_file *file, struct instruction *insn,
+			  struct symbol *dest, bool sibling)
+{
+	struct reloc *reloc = insn_reloc(file, insn);
+
+	insn->call_dest = dest;
+	if (!dest)
+		return;
+
+	if (insn->call_dest->static_call_tramp) {
+		list_add_tail(&insn->call_node,
+			      &file->static_call_list);
+	}
+
+	if (insn->sec->noinstr &&
+	    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
+		if (reloc) {
+			reloc->type = R_NONE;
+			elf_write_reloc(file->elf, reloc);
+		}
+
+		elf_write_insn(file->elf, insn->sec,
+			       insn->offset, insn->len,
+			       sibling ? arch_ret_insn(insn->len)
+			               : arch_nop_insn(insn->len));
+

tools/objtool/check.c static struct reloc *insn_reloc(struct o · line 872

Quoted 6 lines

tools/objtool/check.c +6 -0

+		insn->type = sibling ? INSN_RETURN : INSN_NOP;
+	}
+
+	if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) {
+		if (sibling)
+			WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset);

Have you ever triggered the above?

Quoted 5 lines

tools/objtool/check.c static struct reloc *insn_reloc(struct o · line 872

Quoted 3 lines

tools/objtool/check.c +3 -0

+	if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) {
+		if (sibling)
+			WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset);  

Have you ever triggered the above?

No, that would be really daft. But since this function is to be called for both regular and tail calls, I figured I'd at least pretend to handle the case.

tools/objtool/check.c +4 -0

+	if (insn->sec->noinstr &&
+	    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
+		if (reloc) {
+			reloc->type = R_NONE;

Quoted 9 lines · Peter Zijlstra

tools/objtool/check.c +9 -0

+	if (insn->sec->noinstr &&
+	    !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) {
+		if (reloc) {
+			reloc->type = R_NONE;
+			elf_write_reloc(file->elf, reloc);
+		}
+
+		elf_write_insn(file->elf, insn->sec,
+			       insn->offset, insn->len,

tools/objtool/check.c static struct reloc *insn_reloc(struct o · line 868

Quoted 5 lines

tools/objtool/check.c +5 -0

+			       sibling ? arch_ret_insn(insn->len)
+			               : arch_nop_insn(insn->len));
+
+		insn->type = sibling ? INSN_RETURN : INSN_NOP;
+	}

It'd be nice to keep the comment for this case that was in add_call_destinations().

Yes, it really did out-of-line this....

vmlinux.o: warning: objtool: vmx_vcpu_enter_exit()+0x31: call to context_tracking_guest_enter() leaves .noinstr.text section

000000000019f660 <context_tracking_guest_enter>:

  19f660:	e8 00 00 00 00       	callq  19f665 <context_tracking_guest_enter+0x5>	19f661: R_X86_64_PLT32	__sanitizer_cov_trace_pc-0x4
  19f665:	31 c0                	xor    %eax,%eax
  19f667:	c3                   	retq   

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Quoted 9 lines · Peter Zijlstra

Yes, it really did out-of-line this....

vmlinux.o: warning: objtool: vmx_vcpu_enter_exit()+0x31: call to context_tracking_guest_enter() leaves .noinstr.text section

000000000019f660 <context_tracking_guest_enter>:

  19f660:	e8 00 00 00 00       	callq  19f665 <context_tracking_guest_enter+0x5>	19f661: R_X86_64_PLT32	__sanitizer_cov_trace_pc-0x4
  19f665:	31 c0                	xor    %eax,%eax
  19f667:	c3                   	retq   

Quoted 1 line

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Ha. I'm guessing this was from CONFIG_DEBUG_SECTION_MISMATCH, which is only good for triggering a lot of these useless __always_inline patches.

I have a patch somewhere to get rid of it.

WARNING: This patch may shrink your patch set and improve your sanity.

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH

Modpost's section mismatch detection warns when a non-init function references an __init function. The warning is needed because __init memory is freed during boot, so the non-init function might end up calling into some random memory.

CONFIG_DEBUG_SECTION_MISMATCH is intended to root out more of these issues by adding the -fno-inline-functions-called-once compiler flag, which forces once-called static functions to not be inlined. As described in the option's help description:

- Add the option -fno-inline-functions-called-once to gcc commands. When inlining a function annotated with __init in a non-init function, we would lose the section information and thus the analysis would not catch the illegal reference. This option tells gcc to inline less (but it does result in a larger kernel).

It's true that if an __init function is inlined in a non-init function, it's no longer __init. But that's not actually a problem.

If an inlined __init function doesn't access any other __init functions or data, the inlining is harmless.

If the inlined __init function does access other __init functions/data, __init relocations will be generated for a non-init section, and modpost's section mismatch detection will discover it.

So the option isn't actually useful. It's not even useful for finding issues on other configs, which might have different inlining decisions, because with -O2, once-called static functions are always inlined. And anyway, nowadays we have build bots everywhere, which are great at finding such edge cases.

I get the feeling this option has been responsible for dozens of "convert inline to __always_inline" patches over the years. Such patches usually complain about the compiler's inlining decisions being unpredictable. Turns out the culprit is this CONFIG_DEBUG_SECTION_MISMATCH option, at least for once-called functions.

This option really has no benefits, and only drawbacks, causing needless churn. Remove it.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 Makefile                                      |  5 -----
 arch/arc/configs/tb10x_defconfig              |  1 -
 arch/nds32/configs/defconfig                  |  1 -
 arch/s390/configs/debug_defconfig             |  1 -
 arch/s390/configs/defconfig                   |  1 -
 lib/Kconfig.debug                             | 21 -------------------
 .../selftests/wireguard/qemu/debug.config     |  1 -
 7 files changed, 31 deletions(-)

Makefile +0 -5

index c19d1638da25..195d4f4914e3 100644
@@ -913,11 +913,6 @@ KBUILD_CFLAGS	+= $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING)
 KBUILD_AFLAGS	+= $(CC_FLAGS_USING)
 endif
 
-# We trigger additional mismatches with less inlining
-ifdef CONFIG_DEBUG_SECTION_MISMATCH
-KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
-endif
-
 ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
 KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections
 LDFLAGS_vmlinux += --gc-sections

arch/arc/configs/tb10x_defconfig +0 -1

index a12656ec0072..5acf8cc3e7b0 100644
@@ -96,7 +96,6 @@ CONFIG_STRIP_ASM_SYMS=y
 CONFIG_DEBUG_FS=y
 CONFIG_HEADERS_INSTALL=y
 CONFIG_HEADERS_CHECK=y
-CONFIG_DEBUG_SECTION_MISMATCH=y
 CONFIG_MAGIC_SYSRQ=y
 CONFIG_DEBUG_MEMORY_INIT=y
 CONFIG_DEBUG_STACKOVERFLOW=y

arch/nds32/configs/defconfig +0 -1

index f9a89cf00aa6..61b5b83bd20e 100644
@@ -93,7 +93,6 @@ CONFIG_GDB_SCRIPTS=y
 CONFIG_READABLE_ASM=y
 CONFIG_HEADERS_INSTALL=y
 CONFIG_HEADERS_CHECK=y
-CONFIG_DEBUG_SECTION_MISMATCH=y
 CONFIG_MAGIC_SYSRQ=y
 CONFIG_DEBUG_KERNEL=y
 CONFIG_PANIC_ON_OOPS=y

arch/s390/configs/debug_defconfig +0 -1

index b88184019af9..68104f82cc9d 100644
@@ -782,7 +782,6 @@ CONFIG_DEBUG_INFO=y
 CONFIG_DEBUG_INFO_DWARF4=y
 CONFIG_GDB_SCRIPTS=y
 CONFIG_HEADERS_INSTALL=y
-CONFIG_DEBUG_SECTION_MISMATCH=y
 CONFIG_MAGIC_SYSRQ=y
 CONFIG_DEBUG_PAGEALLOC=y
 CONFIG_PAGE_OWNER=y

arch/s390/configs/defconfig +0 -1

index 1667a3cdcf0a..a5e7eb0276c6 100644
@@ -767,7 +767,6 @@ CONFIG_DYNAMIC_DEBUG=y
 CONFIG_DEBUG_INFO=y
 CONFIG_DEBUG_INFO_DWARF4=y
 CONFIG_GDB_SCRIPTS=y
-CONFIG_DEBUG_SECTION_MISMATCH=y
 CONFIG_MAGIC_SYSRQ=y
 CONFIG_DEBUG_WX=y
 CONFIG_PTDUMP_DEBUGFS=y

lib/Kconfig.debug +0 -21

index e5cdf98f50c2..93f9c00783b2 100644
@@ -381,27 +381,6 @@ config HEADERS_INSTALL
 	  user-space program samples. It is also needed by some features such
 	  as uapi header sanity checks.
 
-config DEBUG_SECTION_MISMATCH
-	bool "Enable full Section mismatch analysis"
-	help
-	  The section mismatch analysis checks if there are illegal
-	  references from one section to another section.
-	  During linktime or runtime, some sections are dropped;
-	  any use of code/data previously in these sections would
-	  most likely result in an oops.
-	  In the code, functions and variables are annotated with
-	  __init,, etc. (see the full list in include/linux/init.h),
-	  which results in the code/data being placed in specific sections.
-	  The section mismatch analysis is always performed after a full
-	  kernel build, and enabling this option causes the following
-	  additional step to occur:
-	  - Add the option -fno-inline-functions-called-once to gcc commands.
-	    When inlining a function annotated with __init in a non-init
-	    function, we would lose the section information and thus
-	    the analysis would not catch the illegal reference.
-	    This option tells gcc to inline less (but it does result in
-	    a larger kernel).
-
 config SECTION_MISMATCH_WARN_ONLY
 	bool "Make section mismatch errors non-fatal"
 	default y

tools/testing/selftests/wireguard/qemu/debug.config +0 -1

index fe07d97df9fa..83588bb08fde 100644
@@ -57,7 +57,6 @@ CONFIG_USER_STACKTRACE_SUPPORT=y
 CONFIG_DEBUG_SG=y
 CONFIG_DEBUG_NOTIFIERS=y
 CONFIG_X86_DEBUG_FPU=y
-CONFIG_DEBUG_SECTION_MISMATCH=y
 CONFIG_DEBUG_PAGEALLOC=y
 CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
 CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y

Signature · 1 line

2.31.1

Hi Josh,

Quoted 15 lines · Peter Zijlstra

Quoted 1 line · Josh Poimboeuf

WARNING: This patch may shrink your patch set and improve your sanity.

I'll go rebuild me some kernels to see how many __always_inline things I can drop.

Lets add a few Cc's though.

Quoted 6 lines

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH

So the option isn't actually useful. It's not even useful for finding issues on other configs, which might have different inlining decisions, because with -O2, once-called static functions are always inlined.

We have following -O possibilities:

    ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE
    KBUILD_CFLAGS += -O2
    else ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE_O3
    KBUILD_CFLAGS += -O3
    else ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
    KBUILD_CFLAGS += -Os
    endif

We always built with -Os or -O2 (and ARC supports -O3).
Do we know that gcc and clang will always inline once-called functions with -O2 and -Os for the relevant architectures?

If you can answer yes to this with some confidence then you can add my:

Acked-by Sam Ravnborg <[email protected]>

Note: I added this option long time ago and is unhappy if this only have resulted in useless code changes.

Quoted 10 lines · Josh Poimboeuf

Quoted 9 lines · Peter Zijlstra

Yes, it really did out-of-line this....

vmlinux.o: warning: objtool: vmx_vcpu_enter_exit()+0x31: call to context_tracking_guest_enter() leaves .noinstr.text section

000000000019f660 <context_tracking_guest_enter>:

  19f660:	e8 00 00 00 00       	callq  19f665 <context_tracking_guest_enter+0x5>	19f661: R_X86_64_PLT32	__sanitizer_cov_trace_pc-0x4
  19f665:	31 c0                	xor    %eax,%eax
  19f667:	c3                   	retq   

Quoted 6 lines

Quoted 1 line

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Ha. I'm guessing this was from CONFIG_DEBUG_SECTION_MISMATCH, which is only good for triggering a lot of these useless __always_inline patches.

I have a patch somewhere to get rid of it.

Found it.

vmlinux.o: warning: objtool: lockdep_hardirqs_on()+0x72: call to arch_local_save_flags() leaves .noinstr.text section vmlinux.o: warning: objtool: lockdep_hardirqs_off()+0x73: call to arch_local_save_flags() leaves .noinstr.text section vmlinux.o: warning: objtool: match_held_lock()+0x11f: call to arch_local_save_flags() leaves .noinstr.text section

vmlinux.o: warning: objtool: lock_is_held_type()+0x4e: call to arch_local_irq_save() leaves .noinstr.text section

vmlinux.o: warning: objtool: lock_is_held_type()+0x65: call to arch_local_irq_disable() leaves .noinstr.text section

vmlinux.o: warning: objtool: lock_is_held_type()+0xfe: call to arch_local_irq_enable() leaves .noinstr.text section

It makes no sense to not inline these things.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Doing unconditional indirect calls through the pv_ops vector is weird.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

vmlinux.o: warning: objtool: pv_ops[41]: native_read_cr2
vmlinux.o: warning: objtool: pv_ops[41]: xen_read_cr2
vmlinux.o: warning: objtool: pv_ops[41]: xen_read_cr2_direct
vmlinux.o: warning: objtool: exc_double_fault()+0x15: call to pv_ops[41]() leaves .noinstr.text section

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

vmlinux.o: warning: objtool: pv_ops[42]: native_write_cr2
vmlinux.o: warning: objtool: pv_ops[42]: xen_write_cr2
vmlinux.o: warning: objtool: exc_nmi()+0x127: call to pv_ops[42]() leaves .noinstr.text section

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

vmlinux.o: warning: objtool: pv_ops[1]: xen_get_debugreg
vmlinux.o: warning: objtool: pv_ops[1]: native_get_debugreg
vmlinux.o: warning: objtool: exc_debug()+0x25: call to pv_ops[1]() leaves .noinstr.text section

Signed-off-by Peter Zijlstra (Intel) <[email protected]>