x86/bpf: unwinder fixes

From: Song Liu <[email protected]>

The stacktrace_map_raw_tp BPF selftest is failing because the RIP saved by perf_arch_fetch_caller_regs() isn't getting saved by perf_callchain_kernel().

This was broken by the following commit:

d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

With that change, when starting with non-HW regs, the unwinder starts with the current stack frame and unwinds until it passes up the frame which called perf_arch_fetch_caller_regs(). So regs->ip needs to be saved deliberately.

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

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

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

Quoted 15 lines · Josh Poimboeuf

From: Song Liu <[email protected]>

The stacktrace_map_raw_tp BPF selftest is failing because the RIP saved by perf_arch_fetch_caller_regs() isn't getting saved by perf_callchain_kernel().

This was broken by the following commit:

d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

With that change, when starting with non-HW regs, the unwinder starts with the current stack frame and unwinds until it passes up the frame which called perf_arch_fetch_caller_regs(). So regs->ip needs to be saved deliberately.

Quoted 3 lines

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

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

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

It's not cool to remove people's SOB.
It's Song's patch. His should be first and your second.

Quoted 2 lines

It's not cool to remove people's SOB.
It's Song's patch. His should be first and your second.

His original patch didn't have an SOB. I preserved the "From" field.

Quoted 4 lines · On Fri, 14 Jun 2019 16:06:19 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 2 lines

It's not cool to remove people's SOB.
It's Song's patch. His should be first and your second.

His original patch didn't have an SOB. I preserved the "From" field.

Then it can't be accepted. It needs an SOB from the original author.

Song, Please reply with a Signed-off-by tag.

Thanks!

-- Steve

Objtool currently ignores ___bpf_prog_run() because it doesn't understand the jump table. This results in the ORC unwinder not being able to unwind through non-JIT BPF code.

Luckily, the BPF jump table resembles a GCC switch jump table, which objtool already knows how to read.

Add generic support for reading any static local jump table array named "jump_table", and rename the BPF variable accordingly, so objtool can generate ORC data for ___bpf_prog_run().

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

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

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

---
 kernel/bpf/core.c     |  5 ++---
 tools/objtool/check.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/bpf/core.c +2 -3

index 7c473f208a10..aa546ef7dbdc 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-	static const void *jumptable[256] = {
+	static const void *jump_table[256] = {
 		[0 ... 255] = &&default_label,
 		/* Now overwrite non-defaults ... */
 		BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
-	goto *jumptable[insn->code];
+	goto *jump_table[insn->code];
 
 	/* ALU */
 #define ALU(OPCODE, OP)			\

@@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 		BUG_ON(1);
 		return 0;
 }
-STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
 
 #define PROG_NAME(stack_size) __bpf_prog_run##stack_size
 #define DEFINE_BPF_PROG_RUN(stack_size) \

tools/objtool/check.c +0 -0

index 172f99195726..8341c2fff14f 100644

Quoted 56 lines · Josh Poimboeuf

Objtool currently ignores ___bpf_prog_run() because it doesn't understand the jump table. This results in the ORC unwinder not being able to unwind through non-JIT BPF code.

Luckily, the BPF jump table resembles a GCC switch jump table, which objtool already knows how to read.

Add generic support for reading any static local jump table array named "jump_table", and rename the BPF variable accordingly, so objtool can generate ORC data for ___bpf_prog_run().

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

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

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

---
 kernel/bpf/core.c     |  5 ++---
 tools/objtool/check.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/bpf/core.c +2 -3

index 7c473f208a10..aa546ef7dbdc 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-	static const void *jumptable[256] = {
+	static const void *jump_table[256] = {
 		[0 ... 255] = &&default_label,
 		/* Now overwrite non-defaults ... */
 		BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
-	goto *jumptable[insn->code];
+	goto *jump_table[insn->code];
 
 	/* ALU */
 #define ALU(OPCODE, OP)			\

@@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 		BUG_ON(1);
 		return 0;
 }
-STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
 
 #define PROG_NAME(stack_size) __bpf_prog_run##stack_size
 #define DEFINE_BPF_PROG_RUN(stack_size) \

tools/objtool/check.c +0 -0

index 172f99195726..8341c2fff14f 100644
@@ -18,6 +18,8 @@
 

tools/objtool/check.c line 21

Quoted 3 lines

tools/objtool/check.c +1 -0

 #define FAKE_JUMP_OFFSET -1
 
+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

Quoted 57 lines · Alexei Starovoitov

Quoted 56 lines · Josh Poimboeuf

Objtool currently ignores ___bpf_prog_run() because it doesn't understand the jump table. This results in the ORC unwinder not being able to unwind through non-JIT BPF code.

Luckily, the BPF jump table resembles a GCC switch jump table, which objtool already knows how to read.

Add generic support for reading any static local jump table array named "jump_table", and rename the BPF variable accordingly, so objtool can generate ORC data for ___bpf_prog_run().

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

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

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

---
 kernel/bpf/core.c     |  5 ++---
 tools/objtool/check.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/bpf/core.c +2 -3

index 7c473f208a10..aa546ef7dbdc 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-   static const void *jumptable[256] = {
+   static const void *jump_table[256] = {
            [0 ... 255] = &&default_label,
            /* Now overwrite non-defaults ... */
            BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
-   goto *jumptable[insn->code];
+   goto *jump_table[insn->code];
 
    /* ALU */
 #define ALU(OPCODE, OP)                    \

@@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
            BUG_ON(1);
            return 0;
 }
-STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
 
 #define PROG_NAME(stack_size) __bpf_prog_run##stack_size
 #define DEFINE_BPF_PROG_RUN(stack_size) \

tools/objtool/check.c +0 -0

index 172f99195726..8341c2fff14f 100644
@@ -18,6 +18,8 @@
 

Quoted 8 lines

tools/objtool/check.c line 21

Quoted 3 lines

tools/objtool/check.c +1 -0

 #define FAKE_JUMP_OFFSET -1
 
+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

Quoted 68 lines · Josh Poimboeuf

Quoted 65 lines · Alexei Starovoitov

Quoted 55 lines · Josh Poimboeuf

Objtool currently ignores ___bpf_prog_run() because it doesn't understand the jump table. This results in the ORC unwinder not being able to unwind through non-JIT BPF code.

Luckily, the BPF jump table resembles a GCC switch jump table, which objtool already knows how to read.

Add generic support for reading any static local jump table array named "jump_table", and rename the BPF variable accordingly, so objtool can generate ORC data for ___bpf_prog_run().

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

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

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

---
 kernel/bpf/core.c     |  5 ++---
 tools/objtool/check.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/bpf/core.c +2 -3

index 7c473f208a10..aa546ef7dbdc 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-   static const void *jumptable[256] = {
+   static const void *jump_table[256] = {
            [0 ... 255] = &&default_label,
            /* Now overwrite non-defaults ... */
            BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
-   goto *jumptable[insn->code];
+   goto *jump_table[insn->code];
 
    /* ALU */
 #define ALU(OPCODE, OP)                    \

@@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
            BUG_ON(1);
            return 0;
 }
-STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
 
 #define PROG_NAME(stack_size) __bpf_prog_run##stack_size
 #define DEFINE_BPF_PROG_RUN(stack_size) \

tools/objtool/check.c +0 -0

index 172f99195726..8341c2fff14f 100644

tools/objtool/check.c line 21

Quoted 4 lines

tools/objtool/check.c +1 -0

@@ -18,6 +18,8 @@
 
 #define FAKE_JUMP_OFFSET -1
 
+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

Quoted 2 lines

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

Quoted 5 lines

Quoted 2 lines

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

That's the idea.

Quoted 11 lines · Josh Poimboeuf

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

Quoted 4 lines

Quoted 2 lines

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

That's the idea.

Then it needs much wider discussion.

Quoted 15 lines · Alexei Starovoitov

Quoted 14 lines · Josh Poimboeuf

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

Quoted 3 lines

Quoted 1 line

That's the idea.

Then it needs much wider discussion.

Why would it need wider discussion? It only has one user. If you honestly believe that it will be controversial to require future users to call a static jump table "jump_table" then we can have that discussion when it comes up.

Quoted 17 lines · Josh Poimboeuf

Quoted 15 lines · Alexei Starovoitov

Quoted 14 lines · Josh Poimboeuf

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

That's the idea.

Then it needs much wider discussion.

Why would it need wider discussion? It only has one user. If you honestly believe that it will be controversial to require future users to call a static jump table "jump_table" then we can have that discussion when it comes up.

It's clearly controversial.
I nacked it already on pointless name change from "jumptable" to "jump_table" and now you're saying that no one will complain about "jump_table" name for all jump tables in the kernel that will ever appear?

Quoted 30 lines · Josh Poimboeuf

Quoted 20 lines · Alexei Starovoitov

Quoted 18 lines · Josh Poimboeuf

On Fri, Jun 14, 2019 at 02:22:59PM -0700, Alexei Starovoitov wrote:

Quoted 14 lines · Josh Poimboeuf

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

That's the idea.

Then it needs much wider discussion.

Why would it need wider discussion? It only has one user. If you honestly believe that it will be controversial to require future users to call a static jump table "jump_table" then we can have that discussion when it comes up.

Quoted 5 lines

It's clearly controversial.
I nacked it already on pointless name change from "jumptable" to "jump_table" and now you're saying that no one will complain about "jump_table" name for all jump tables in the kernel that will ever appear?

Let me get this straight. You're saying that "jumptable" and "bpf_interpreter_jump_table" are both acceptable.

But NACK to "jump_table".

Ok...

Signature · 1 line

Josh

Quoted 27 lines · Alexei Starovoitov

Quoted 17 lines · Josh Poimboeuf

Quoted 15 lines · Alexei Starovoitov

Quoted 14 lines · Josh Poimboeuf

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel as static foo jump_table[] ?

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

That's the idea.

Then it needs much wider discussion.

Why would it need wider discussion? It only has one user. If you honestly believe that it will be controversial to require future users to call a static jump table "jump_table" then we can have that discussion when it comes up.

Quoted 5 lines

It's clearly controversial.
I nacked it already on pointless name change from "jumptable" to "jump_table" and now you're saying that no one will complain about "jump_table" name for all jump tables in the kernel that will ever appear?

Let me get this straight. You're saying that "jumptable" and "bpf_interpreter_jump_table" are both acceptable.

But NACK to "jump_table".

Ok...

Sent: 14 June 2019 21:59

Quoted 56 lines · Josh Poimboeuf

Objtool currently ignores ___bpf_prog_run() because it doesn't understand the jump table. This results in the ORC unwinder not being able to unwind through non-JIT BPF code.

Luckily, the BPF jump table resembles a GCC switch jump table, which objtool already knows how to read.

Add generic support for reading any static local jump table array named "jump_table", and rename the BPF variable accordingly, so objtool can generate ORC data for ___bpf_prog_run().

Fixes d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

Reported-by Song Liu <[email protected]>

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

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

---
 kernel/bpf/core.c     |  5 ++---
 tools/objtool/check.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/bpf/core.c +2 -3

index 7c473f208a10..aa546ef7dbdc 100644
@@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-	static const void *jumptable[256] = {
+	static const void *jump_table[256] = {
 		[0 ... 255] = &&default_label,
 		/* Now overwrite non-defaults ... */
 		BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),

@@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
-	goto *jumptable[insn->code];
+	goto *jump_table[insn->code];
 
 	/* ALU */
 #define ALU(OPCODE, OP)			\

@@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 		BUG_ON(1);
 		return 0;
 }
-STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
 
 #define PROG_NAME(stack_size) __bpf_prog_run##stack_size
 #define DEFINE_BPF_PROG_RUN(stack_size) \

tools/objtool/check.c +0 -0

index 172f99195726..8341c2fff14f 100644
@@ -18,6 +18,8 @@
 

tools/objtool/check.c line 21

Quoted 3 lines

tools/objtool/check.c +1 -0

 #define FAKE_JUMP_OFFSET -1
 
+#define JUMP_TABLE_SYM_PREFIX "jump_table."

since external tool will be looking at it should it be named "bpf_jump_table." to avoid potential name conflicts? Or even more unique name?
Like "bpf_interpreter_jump_table." ?

The BPF JIT code clobbers RBP. This breaks frame pointer convention and thus prevents the FP unwinder from unwinding through JIT generated code.

Fix it by saving the new RBP value on the stack before updating it. This effectively creates a new stack frame which the unwinder can understand.

Also, simplify the BPF JIT prologue such that it more closely resembles a typical compiler-generated prologue. This also reduces the prologue size quite a bit overall.

Suggested-by David Laight <[email protected]>

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

---
 arch/x86/net/bpf_jit_comp.c | 106 ++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

arch/x86/net/bpf_jit_comp.c +18 -27

index da8c988b0f0f..fa1fe65c4cb4 100644
@@ -186,56 +186,54 @@ struct jit_context {
 #define BPF_MAX_INSN_SIZE	128
 #define BPF_INSN_SAFETY		64
 
-#define AUX_STACK_SPACE		40 /* Space for RBX, R13, R14, R15, tailcnt */
-
-#define PROLOGUE_SIZE		37
+#define PROLOGUE_SIZE		24
 
 /*
  * Emit x86-64 prologue code for BPF program and check its size.
  * bpf_tail_call helper will skip it while jumping into another program
  */
-static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
+static void emit_prologue(u8 **pprog, u32 stack_depth)
 {
 	u8 *prog = *pprog;
 	int cnt = 0;
 
 	/* push rbp */
 	EMIT1(0x55);
-
-	/* mov rbp,rsp */
+	/* mov rbp, rsp */
 	EMIT3(0x48, 0x89, 0xE5);
 
-	/* sub rsp, rounded_stack_depth + AUX_STACK_SPACE */
-	EMIT3_off32(0x48, 0x81, 0xEC,
-		    round_up(stack_depth, 8) + AUX_STACK_SPACE);
+	/* push r15 */
+	EMIT2(0x41, 0x57);
+	/* push r14 */
+	EMIT2(0x41, 0x56);
+	/* push r13 */
+	EMIT2(0x41, 0x55);
+	/* push rbx */
+	EMIT1(0x53);
 
-	/* sub rbp, AUX_STACK_SPACE */
-	EMIT4(0x48, 0x83, 0xED, AUX_STACK_SPACE);
-
-	/* mov qword ptr [rbp+0],rbx */
-	EMIT4(0x48, 0x89, 0x5D, 0);
-	/* mov qword ptr [rbp+8],r13 */
-	EMIT4(0x4C, 0x89, 0x6D, 8);
-	/* mov qword ptr [rbp+16],r14 */
-	EMIT4(0x4C, 0x89, 0x75, 16);
-	/* mov qword ptr [rbp+24],r15 */
-	EMIT4(0x4C, 0x89, 0x7D, 24);
+	/*
+	 * Push the tail call counter (tail_call_cnt) for eBPF tail calls.
+	 * Initialized to zero.
+	 *
+	 * push $0
+	 */
+	EMIT2(0x6a, 0x00);
 
-	if (!ebpf_from_cbpf) {
-		/*
-		 * Clear the tail call counter (tail_call_cnt): for eBPF tail
-		 * calls we need to reset the counter to 0. It's done in two
-		 * instructions, resetting RAX register to 0, and moving it
-		 * to the counter location.
-		 */

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Here is much simple patch that I mentioned in the email yesterday, but you failed to listen instead of focusing on perceived 'code readability'.

It makes one proper frame and both frame and orc unwinders are happy.

Quoted 3 lines · Alexei Starovoitov

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Hm, are you seeing selftest failures? They seem to work for me.

Quoted 3 lines

Here is much simple patch that I mentioned in the email yesterday, but you failed to listen instead of focusing on perceived 'code readability'.

Quoted 1 line

It makes one proper frame and both frame and orc unwinders are happy.

I'm on my way out the door and I just skimmed it, but it looks fine.

Some of the code and patch description look familiar, please be sure to give me proper credit.

Quoted 11 lines · Josh Poimboeuf

Quoted 3 lines · Alexei Starovoitov

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Hm, are you seeing selftest failures? They seem to work for me.

Quoted 3 lines

Here is much simple patch that I mentioned in the email yesterday, but you failed to listen instead of focusing on perceived 'code readability'.

Quoted 6 lines

Quoted 1 line

It makes one proper frame and both frame and orc unwinders are happy.

I'm on my way out the door and I just skimmed it, but it looks fine.

Some of the code and patch description look familiar, please be sure to give me proper credit.

credit means something positive.

your contribution to bpf jit fix was negative.

Quoted 16 lines · Alexei Starovoitov

Quoted 15 lines · Josh Poimboeuf

Quoted 3 lines · Alexei Starovoitov

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Hm, are you seeing selftest failures? They seem to work for me.

Quoted 4 lines

Here is much simple patch that I mentioned in the email yesterday, but you failed to listen instead of focusing on perceived 'code readability'.

It makes one proper frame and both frame and orc unwinders are happy.

I'm on my way out the door and I just skimmed it, but it looks fine.

Quoted 4 lines

Quoted 2 lines

Some of the code and patch description look familiar, please be sure to give me proper credit.

credit means something positive.

So you only give credit for *good* stolen code. I must have missed that section of the kernel patch guidelines.

Quoted 18 lines · Josh Poimboeuf

Quoted 16 lines · Alexei Starovoitov

Quoted 15 lines · Josh Poimboeuf

Quoted 3 lines · Alexei Starovoitov

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Hm, are you seeing selftest failures? They seem to work for me.

Quoted 4 lines

Here is much simple patch that I mentioned in the email yesterday, but you failed to listen instead of focusing on perceived 'code readability'.

It makes one proper frame and both frame and orc unwinders are happy.

I'm on my way out the door and I just skimmed it, but it looks fine.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Some of the code and patch description look familiar, please be sure to give me proper credit.

credit means something positive.

So you only give credit for *good* stolen code. I must have missed that section of the kernel patch guidelines.

what are you talking about?
you've posted one bad patch. I pointed out multiple issues in it. Then proposed another bad idea. I pointed out another set of issues. Than David proposed yet another idea that you've implemented and claimed that it's working when it was not.
Then I got fed up with this thread and fix it for real by reverting that old commit that I mentioned way earlier.
https://patchwork.ozlabs.org/patch/1116307/
Where do you see your code or ideas being used?
I see none.

Quoted 27 lines · Alexei Starovoitov

Quoted 18 lines · Josh Poimboeuf

Quoted 16 lines · Alexei Starovoitov

Quoted 15 lines · Josh Poimboeuf

Quoted 3 lines · Alexei Starovoitov

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Hm, are you seeing selftest failures? They seem to work for me.

Quoted 4 lines

Here is much simple patch that I mentioned in the email yesterday, but you failed to listen instead of focusing on perceived 'code readability'.

It makes one proper frame and both frame and orc unwinders are happy.

I'm on my way out the door and I just skimmed it, but it looks fine.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Some of the code and patch description look familiar, please be sure to give me proper credit.

credit means something positive.

So you only give credit for *good* stolen code. I must have missed that section of the kernel patch guidelines.

Quoted 10 lines

what are you talking about?
you've posted one bad patch. I pointed out multiple issues in it. Then proposed another bad idea. I pointed out another set of issues. Than David proposed yet another idea that you've implemented and claimed that it's working when it was not.
Then I got fed up with this thread and fix it for real by reverting that old commit that I mentioned way earlier.
https://patchwork.ozlabs.org/patch/1116307/
Where do you see your code or ideas being used?
I see none.

Obviously I wasn't referring to this new whitewashed patch for which I wasn't even on Cc, despite being one of the people (along with Peter Z) who convinced you that there was a problem to begin with.

The previous patch you posted has my patch description, push/pop and comment changes, with no credit:

Quoted 4 lines · Josh Poimboeuf

The previous patch you posted has my patch description, push/pop and comment changes, with no credit:

https://lkml.kernel.org/r/[email protected]

I'm sorry for reusing one sentence from your commit log and not realizing you want credit for that.
Will not happen again.

I also suggest you never touch anything bpf related. Just to avoid this credit claims and threads like this one.

Quoted 9 lines · Alexei Starovoitov

Quoted 4 lines · Josh Poimboeuf

The previous patch you posted has my patch description, push/pop and comment changes, with no credit:

https://lkml.kernel.org/r/[email protected]

I'm sorry for reusing one sentence from your commit log and not realizing you want credit for that.
Will not happen again.

Um. What are you talking about? The entire patch was clearly derived from mine. Not just "one sentence from your commit log". The title, the pushes/pops in the prologue/epilogue, the removal of the "ebpf_from_cbpf" argument, the code spacing, and some of the non trivial comment changes were the same.

Quoted 2 lines

I also suggest you never touch anything bpf related. Just to avoid this credit claims and threads like this one.

Wth. I made a simple request for credit. Anybody can see the patch was derived from mine. It's not like I really care. It's just basic human decency.

Quoted 18 lines · Josh Poimboeuf

Quoted 9 lines · Alexei Starovoitov

Quoted 4 lines · Josh Poimboeuf

The previous patch you posted has my patch description, push/pop and comment changes, with no credit:

https://lkml.kernel.org/r/[email protected]

I'm sorry for reusing one sentence from your commit log and not realizing you want credit for that.
Will not happen again.

Um. What are you talking about? The entire patch was clearly derived from mine. Not just "one sentence from your commit log". The title, the pushes/pops in the prologue/epilogue, the removal of the "ebpf_from_cbpf" argument, the code spacing, and some of the non trivial comment changes were the same.

Quoted 6 lines

Quoted 2 lines

I also suggest you never touch anything bpf related. Just to avoid this credit claims and threads like this one.

Wth. I made a simple request for credit. Anybody can see the patch was derived from mine. It's not like I really care. It's just basic human decency.

derived? do you really think so ?
Please fix your orc stuff that is still broken.
Human decency is fixing stuff that you're responsible for. Your commit d15d356887e7 on April 23 broke stack traces. And we reported it 3 weeks ago.
Yet instead of fixing it you kept arguing about JIT frame pointers that is orthogonal issue and was in this state for the last 2 years.