[PATCH 0/9] x86/bpf: unwinder fixes

[PATCH 0/9] x86/bpf: unwinder fixes

[Josh Poimboeuf]

The following commit

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

was a step in the right direction, but it triggered some BPF selftest
failures.  That commit exposed the fact that we currently can't unwind
through BPF code.

- Patch 1 (originally from Song Liu) fixes a bug in the above commit
  (regs->ip getting skipped in the stack trace).

- Patch 2 fixes non-JIT BPF for the ORC unwinder.

- Patches 3-5 are preparatory improvements for patch 6.

- Patch 6 fixes JIT BPF for the FP unwinder.

- Patch 7 fixes JIT BPF for the ORC unwinder (building on patch 6).

- Patches 8-9 are some readability cleanups.


Josh Poimboeuf (8):
  objtool: Fix ORC unwinding in non-JIT BPF generated code
  x86/bpf: Move epilogue generation to a dedicated function
  x86/bpf: Simplify prologue generation
  x86/bpf: Support SIB byte generation
  x86/bpf: Fix JIT frame pointer usage
  x86/unwind/orc: Fall back to using frame pointers for generated code
  x86/bpf: Convert asm comments to AT&T syntax
  x86/bpf: Convert MOV function/macro argument ordering to AT&T syntax

Song Liu (1):
  perf/x86: Always store regs->ip in perf_callchain_kernel()

 arch/x86/events/core.c       |  10 +-
 arch/x86/kernel/unwind_orc.c |  26 ++-
 arch/x86/net/bpf_jit_comp.c  | 355 ++++++++++++++++++++---------------
 kernel/bpf/core.c            |   5 +-
 tools/objtool/check.c        |  16 +-
 5 files changed, 246 insertions(+), 166 deletions(-)

-- 
2.20.1

[PATCH 1/9] perf/x86: Always store regs->ip in perf_callchain_kernel()

[Josh Poimboeuf]

From: Song Liu <songliubraving@fb.com>

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 <jpoimboe@redhat.com>
---
 arch/x86/events/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index f0e4804515d8..6a7cfcadfc1e 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2328,13 +2328,13 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
 		return;
 	}
 
-	if (perf_hw_regs(regs)) {
-		if (perf_callchain_store(entry, regs->ip))
-			return;
+	if (perf_callchain_store(entry, regs->ip))
+		return;
+
+	if (perf_hw_regs(regs))
 		unwind_start(&state, current, regs, NULL);
-	} else {
+	else
 		unwind_start(&state, current, NULL, (void *)regs->sp);
-	}
 
 	for (; !unwind_done(&state); unwind_next_frame(&state)) {
 		addr = unwind_get_return_address(&state);
-- 
2.20.1

[PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[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 <songliubraving@fb.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/bpf/core.c     |  5 ++---
 tools/objtool/check.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 7c473f208a10..aa546ef7dbdc 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -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) \
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 172f99195726..8341c2fff14f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -18,6 +18,8 @@
 
 #define FAKE_JUMP_OFFSET -1
 
+#define JUMP_TABLE_SYM_PREFIX "jump_table."
+
 struct alternative {
 	struct list_head list;
 	struct instruction *insn;
@@ -997,6 +999,7 @@ static struct rela *find_switch_table(struct objtool_file *file,
 	struct instruction *orig_insn = insn;
 	struct section *rodata_sec;
 	unsigned long table_offset;
+	struct symbol *sym;
 
 	/*
 	 * Backward search using the @first_jump_src links, these help avoid
@@ -1035,9 +1038,18 @@ static struct rela *find_switch_table(struct objtool_file *file,
 
 		/*
 		 * Make sure the .rodata address isn't associated with a
-		 * symbol.  gcc jump tables are anonymous data.
+		 * symbol.  GCC jump tables are anonymous data.
+		 *
+		 * Also support C jump tables which are in the same format as
+		 * switch jump tables.  Each jump table should be a static
+		 * local const array named "jump_table" for objtool to
+		 * recognize it.  Note: GCC will add a numbered suffix to the
+		 * ELF symbol name, like "jump_table.12345", which it does for
+		 * all static local variables.
 		 */
-		if (find_symbol_containing(rodata_sec, table_offset))
+		sym = find_symbol_containing(rodata_sec, table_offset);
+		if (sym && strncmp(sym->name, JUMP_TABLE_SYM_PREFIX,
+				   strlen(JUMP_TABLE_SYM_PREFIX)))
 			continue;
 
 		rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
-- 
2.20.1

[PATCH 3/9] x86/bpf: Move epilogue generation to a dedicated function

[Josh Poimboeuf]

Improve code readability by moving the BPF JIT function epilogue
generation code to a dedicated emit_epilogue() function, analagous to
the existing emit_prologue() function.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 32bfab4e21eb..da8c988b0f0f 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -240,6 +240,28 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
 	*pprog = prog;
 }
 
+static void emit_epilogue(u8 **pprog)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	/* mov rbx, qword ptr [rbp+0] */
+	EMIT4(0x48, 0x8B, 0x5D, 0);
+	/* mov r13, qword ptr [rbp+8] */
+	EMIT4(0x4C, 0x8B, 0x6D, 8);
+	/* mov r14, qword ptr [rbp+16] */
+	EMIT4(0x4C, 0x8B, 0x75, 16);
+	/* mov r15, qword ptr [rbp+24] */
+	EMIT4(0x4C, 0x8B, 0x7D, 24);
+
+	/* add rbp, AUX_STACK_SPACE */
+	EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
+	EMIT1(0xC9); /* leave */
+	EMIT1(0xC3); /* ret */
+
+	*pprog = prog;
+}
+
 /*
  * Generate the following code:
  *
@@ -1036,19 +1058,8 @@ xadd:			if (is_imm8(insn->off))
 			seen_exit = true;
 			/* Update cleanup_addr */
 			ctx->cleanup_addr = proglen;
-			/* mov rbx, qword ptr [rbp+0] */
-			EMIT4(0x48, 0x8B, 0x5D, 0);
-			/* mov r13, qword ptr [rbp+8] */
-			EMIT4(0x4C, 0x8B, 0x6D, 8);
-			/* mov r14, qword ptr [rbp+16] */
-			EMIT4(0x4C, 0x8B, 0x75, 16);
-			/* mov r15, qword ptr [rbp+24] */
-			EMIT4(0x4C, 0x8B, 0x7D, 24);
-
-			/* add rbp, AUX_STACK_SPACE */
-			EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
-			EMIT1(0xC9); /* leave */
-			EMIT1(0xC3); /* ret */
+
+			emit_epilogue(&prog);
 			break;
 
 		default:
-- 
2.20.1

[PATCH 4/9] x86/bpf: Simplify prologue generation

[Josh Poimboeuf]

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.

The frame pointer setup instructions at the beginning don't actually
accomplish anything because RBP gets clobbered anyway later in the
prologue.  So remove those instructions for now.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 100 +++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 53 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index da8c988b0f0f..485692d4b163 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -186,56 +186,48 @@ 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		20
 
 /*
  * 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 r15 */
+	EMIT2(0x41, 0x57);
+	/* push r14 */
+	EMIT2(0x41, 0x56);
+	/* push r13 */
+	EMIT2(0x41, 0x55);
 	/* push rbp */
 	EMIT1(0x55);
+	/* push rbx */
+	EMIT1(0x53);
 
-	/* 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);
-
-	/* 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.
-		 */
+	/*
+	 * RBP is used for the BPF program's FP register.  It points to the end
+	 * of the program's stack area.
+	 *
+	 * mov rbp, rsp
+	 */
+	EMIT3(0x48, 0x89, 0xE5);
 
-		/* xor eax, eax */
-		EMIT2(0x31, 0xc0);
-		/* mov qword ptr [rbp+32], rax */
-		EMIT4(0x48, 0x89, 0x45, 32);
+	/* sub rsp, rounded_stack_depth */
+	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
 
-		BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
-	}
+	BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
 
 	*pprog = prog;
 }
@@ -245,19 +237,22 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* mov rbx, qword ptr [rbp+0] */
-	EMIT4(0x48, 0x8B, 0x5D, 0);
-	/* mov r13, qword ptr [rbp+8] */
-	EMIT4(0x4C, 0x8B, 0x6D, 8);
-	/* mov r14, qword ptr [rbp+16] */
-	EMIT4(0x4C, 0x8B, 0x75, 16);
-	/* mov r15, qword ptr [rbp+24] */
-	EMIT4(0x4C, 0x8B, 0x7D, 24);
+	/* lea rsp, [rbp+0x8] */
+	EMIT4(0x48, 0x8D, 0x65, 0x08);
+
+	/* pop rbx */
+	EMIT1(0x5B);
+	/* pop rbp */
+	EMIT1(0x5D);
+	/* pop r13 */
+	EMIT2(0x41, 0x5D);
+	/* pop r14 */
+	EMIT2(0x41, 0x5E);
+	/* pop r15 */
+	EMIT2(0x41, 0x5F);
 
-	/* add rbp, AUX_STACK_SPACE */
-	EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
-	EMIT1(0xC9); /* leave */
-	EMIT1(0xC3); /* ret */
+	/* ret */
+	EMIT1(0xC3);
 
 	*pprog = prog;
 }
@@ -295,7 +290,7 @@ static void emit_bpf_tail_call(u8 **pprog)
 	EMIT2(0x89, 0xD2);                        /* mov edx, edx */
 	EMIT3(0x39, 0x56,                         /* cmp dword ptr [rsi + 16], edx */
 	      offsetof(struct bpf_array, map.max_entries));
-#define OFFSET1 (41 + RETPOLINE_RAX_BPF_JIT_SIZE) /* Number of bytes to jump */
+#define OFFSET1 (35 + RETPOLINE_RAX_BPF_JIT_SIZE) /* Number of bytes to jump */
 	EMIT2(X86_JBE, OFFSET1);                  /* jbe out */
 	label1 = cnt;
 
@@ -303,13 +298,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT2_off32(0x8B, 0x85, 36);              /* mov eax, dword ptr [rbp + 36] */
+	EMIT3(0x8B, 0x45, 0x04);                  /* mov eax, dword ptr [rbp + 4] */
 	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
-#define OFFSET2 (30 + RETPOLINE_RAX_BPF_JIT_SIZE)
+#define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT2_off32(0x89, 0x85, 36);              /* mov dword ptr [rbp + 36], eax */
+	EMIT3(0x89, 0x45, 0x04);                  /* mov dword ptr [rbp + 4], eax */
 
 	/* prog = array->ptrs[index]; */
 	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
@@ -437,8 +432,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 	int proglen = 0;
 	u8 *prog = temp;
 
-	emit_prologue(&prog, bpf_prog->aux->stack_depth,
-		      bpf_prog_was_classic(bpf_prog));
+	emit_prologue(&prog, bpf_prog->aux->stack_depth);
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
 		const s32 imm32 = insn->imm;
-- 
2.20.1

[PATCH 5/9] x86/bpf: Support SIB byte generation

[Josh Poimboeuf]

In preparation for using R12 indexing instructions in BPF JIT code, add
support for generating the x86 SIB byte.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 69 +++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 15 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 485692d4b163..e649f977f8e1 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -143,6 +143,12 @@ static bool is_axreg(u32 reg)
 	return reg == BPF_REG_0;
 }
 
+static bool is_sib_reg(u32 reg)
+{
+	/* R12 isn't used yet */
+	false;
+}
+
 /* Add modifiers if 'reg' maps to x86-64 registers R8..R15 */
 static u8 add_1mod(u8 byte, u32 reg)
 {
@@ -779,10 +785,19 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_ST | BPF_MEM | BPF_DW:
 			EMIT2(add_1mod(0x48, dst_reg), 0xC7);
 
-st:			if (is_imm8(insn->off))
-				EMIT2(add_1reg(0x40, dst_reg), insn->off);
+st:
+			if (is_imm8(insn->off))
+				EMIT1(add_1reg(0x40, dst_reg));
+			else
+				EMIT1(add_1reg(0x80, dst_reg));
+
+			if (is_sib_reg(dst_reg))
+				EMIT1(add_1reg(0x20, dst_reg));
+
+			if (is_imm8(insn->off))
+				EMIT1(insn->off);
 			else
-				EMIT1_off32(add_1reg(0x80, dst_reg), insn->off);
+				EMIT(insn->off, 4);
 
 			EMIT(imm32, bpf_size_to_x86_bytes(BPF_SIZE(insn->code)));
 			break;
@@ -811,11 +826,19 @@ st:			if (is_imm8(insn->off))
 			goto stx;
 		case BPF_STX | BPF_MEM | BPF_DW:
 			EMIT2(add_2mod(0x48, dst_reg, src_reg), 0x89);
-stx:			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
+stx:
+			if (is_imm8(insn->off))
+				EMIT1(add_2reg(0x40, dst_reg, src_reg));
 			else
-				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
-					    insn->off);
+				EMIT1(add_2reg(0x80, dst_reg, src_reg));
+
+			if (is_sib_reg(dst_reg))
+				EMIT1(add_1reg(0x20, dst_reg));
+
+			if (is_imm8(insn->off))
+				EMIT1(insn->off);
+			else
+				EMIT(insn->off, 4);
 			break;
 
 			/* LDX: dst_reg = *(u8*)(src_reg + off) */
@@ -837,16 +860,24 @@ stx:			if (is_imm8(insn->off))
 		case BPF_LDX | BPF_MEM | BPF_DW:
 			/* Emit 'mov rax, qword ptr [rax+0x14]' */
 			EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x8B);
-ldx:			/*
+ldx:
+			/*
 			 * If insn->off == 0 we can save one extra byte, but
 			 * special case of x86 R13 which always needs an offset
 			 * is not worth the hassle
 			 */
 			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, src_reg, dst_reg), insn->off);
+				EMIT1(add_2reg(0x40, src_reg, dst_reg));
+			else
+				EMIT1(add_2reg(0x80, src_reg, dst_reg));
+
+			if (is_sib_reg(src_reg))
+				EMIT1(add_1reg(0x20, src_reg));
+
+			if (is_imm8(insn->off))
+				EMIT1(insn->off);
 			else
-				EMIT1_off32(add_2reg(0x80, src_reg, dst_reg),
-					    insn->off);
+				EMIT(insn->off, 4);
 			break;
 
 			/* STX XADD: lock *(u32*)(dst_reg + off) += src_reg */
@@ -859,11 +890,19 @@ stx:			if (is_imm8(insn->off))
 			goto xadd;
 		case BPF_STX | BPF_XADD | BPF_DW:
 			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
-xadd:			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
+xadd:
+			if (is_imm8(insn->off))
+				EMIT1(add_2reg(0x40, dst_reg, src_reg));
+			else
+				EMIT1(add_2reg(0x80, dst_reg, src_reg));
+
+			if (is_sib_reg(dst_reg))
+				EMIT1(add_1reg(0x20, dst_reg));
+
+			if (is_imm8(insn->off))
+				EMIT1(insn->off);
 			else
-				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
-					    insn->off);
+				EMIT(insn->off, 4);
 			break;
 
 			/* call */
-- 
2.20.1

[PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Josh Poimboeuf]

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

RBP is currently used as the BPF stack frame pointer register.  The
actual register used is opaque to the user, as long as it's a
callee-saved register.  Change it to use R12 instead.

Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
Reported-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 43 +++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index e649f977f8e1..bb1968fea50a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -100,9 +100,8 @@ static int bpf_size_to_x86_bytes(int bpf_size)
 /*
  * The following table maps BPF registers to x86-64 registers.
  *
- * x86-64 register R12 is unused, since if used as base address
- * register in load/store instructions, it always needs an
- * extra byte of encoding and is callee saved.
+ * RBP isn't used; it needs to be preserved to allow the unwinder to move
+ * through generated code stacks.
  *
  * Also x86-64 register R9 is unused. x86-64 register R10 is
  * used for blinding (if enabled).
@@ -118,7 +117,7 @@ static const int reg2hex[] = {
 	[BPF_REG_7] = 5,  /* R13 callee saved */
 	[BPF_REG_8] = 6,  /* R14 callee saved */
 	[BPF_REG_9] = 7,  /* R15 callee saved */
-	[BPF_REG_FP] = 5, /* RBP readonly */
+	[BPF_REG_FP] = 4, /* R12 readonly */
 	[BPF_REG_AX] = 2, /* R10 temp register */
 	[AUX_REG] = 3,    /* R11 temp register */
 };
@@ -135,6 +134,7 @@ static bool is_ereg(u32 reg)
 			     BIT(BPF_REG_7) |
 			     BIT(BPF_REG_8) |
 			     BIT(BPF_REG_9) |
+			     BIT(BPF_REG_FP) |
 			     BIT(BPF_REG_AX));
 }
 
@@ -145,8 +145,7 @@ static bool is_axreg(u32 reg)
 
 static bool is_sib_reg(u32 reg)
 {
-	/* R12 isn't used yet */
-	false;
+	return reg == BPF_REG_FP;
 }
 
 /* Add modifiers if 'reg' maps to x86-64 registers R8..R15 */
@@ -192,7 +191,7 @@ struct jit_context {
 #define BPF_MAX_INSN_SIZE	128
 #define BPF_INSN_SAFETY		64
 
-#define PROLOGUE_SIZE		20
+#define PROLOGUE_SIZE		25
 
 /*
  * Emit x86-64 prologue code for BPF program and check its size.
@@ -203,14 +202,20 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
+	/* push rbp */
+	EMIT1(0x55);
+
+	/* mov rbp, rsp */
+	EMIT3(0x48, 0x89, 0xE5);
+
 	/* push r15 */
 	EMIT2(0x41, 0x57);
 	/* push r14 */
 	EMIT2(0x41, 0x56);
 	/* push r13 */
 	EMIT2(0x41, 0x55);
-	/* push rbp */
-	EMIT1(0x55);
+	/* push r12 */
+	EMIT2(0x41, 0x54);
 	/* push rbx */
 	EMIT1(0x53);
 
@@ -223,12 +228,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	EMIT2(0x6a, 0x00);
 
 	/*
-	 * RBP is used for the BPF program's FP register.  It points to the end
+	 * R12 is used for the BPF program's FP register.  It points to the end
 	 * of the program's stack area.
 	 *
-	 * mov rbp, rsp
+	 * mov r12, rsp
 	 */
-	EMIT3(0x48, 0x89, 0xE5);
+	EMIT3(0x49, 0x89, 0xE4);
 
 	/* sub rsp, rounded_stack_depth */
 	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
@@ -243,19 +248,21 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* lea rsp, [rbp+0x8] */
-	EMIT4(0x48, 0x8D, 0x65, 0x08);
+	/* lea rsp, [rbp-0x28] */
+	EMIT4(0x48, 0x8D, 0x65, 0xD8);
 
 	/* pop rbx */
 	EMIT1(0x5B);
-	/* pop rbp */
-	EMIT1(0x5D);
+	/* pop r12 */
+	EMIT2(0x41, 0x5C);
 	/* pop r13 */
 	EMIT2(0x41, 0x5D);
 	/* pop r14 */
 	EMIT2(0x41, 0x5E);
 	/* pop r15 */
 	EMIT2(0x41, 0x5F);
+	/* pop rbp */
+	EMIT1(0x5D);
 
 	/* ret */
 	EMIT1(0xC3);
@@ -304,13 +311,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT3(0x8B, 0x45, 0x04);                  /* mov eax, dword ptr [rbp + 4] */
+	EMIT3(0x8B, 0x45, 0xD4);                  /* mov eax, dword ptr [rbp - 44] */
 	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
 #define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT3(0x89, 0x45, 0x04);                  /* mov dword ptr [rbp + 4], eax */
+	EMIT3(0x89, 0x45, 0xD4);                  /* mov dword ptr [rbp - 44], eax */
 
 	/* prog = array->ptrs[index]; */
 	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
-- 
2.20.1

[PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

The ORC unwinder can't unwind through BPF JIT generated code because
there are no ORC entries associated with the code.

If an ORC entry isn't available, try to fall back to frame pointers.  If
BPF and other generated code always do frame pointer setup (even with
CONFIG_FRAME_POINTERS=n) then this will allow ORC to unwind through most
generated code despite there being no corresponding ORC entries.

Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
Reported-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/unwind_orc.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 33b66b5c5aec..72b997eaa1fc 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -82,9 +82,9 @@ static struct orc_entry *orc_find(unsigned long ip);
  * But they are copies of the ftrace entries that are static and
  * defined in ftrace_*.S, which do have orc entries.
  *
- * If the undwinder comes across a ftrace trampoline, then find the
+ * If the unwinder comes across a ftrace trampoline, then find the
  * ftrace function that was used to create it, and use that ftrace
- * function's orc entrie, as the placement of the return code in
+ * function's orc entry, as the placement of the return code in
  * the stack will be identical.
  */
 static struct orc_entry *orc_ftrace_find(unsigned long ip)
@@ -128,6 +128,16 @@ static struct orc_entry null_orc_entry = {
 	.type = ORC_TYPE_CALL
 };
 
+/* Fake frame pointer entry -- used as a fallback for generated code */
+static struct orc_entry orc_fp_entry = {
+	.type		= ORC_TYPE_CALL,
+	.sp_reg		= ORC_REG_BP,
+	.sp_offset	= 16,
+	.bp_reg		= ORC_REG_PREV_SP,
+	.bp_offset	= -16,
+	.end		= 0,
+};
+
 static struct orc_entry *orc_find(unsigned long ip)
 {
 	static struct orc_entry *orc;
@@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
 	 * calls and calls to noreturn functions.
 	 */
 	orc = orc_find(state->signal ? state->ip : state->ip - 1);
-	if (!orc)
-		goto err;
+	if (!orc) {
+		/*
+		 * As a fallback, try to assume this code uses a frame pointer.
+		 * This is useful for generated code, like BPF, which ORC
+		 * doesn't know about.  This is just a guess, so the rest of
+		 * the unwind is no longer considered reliable.
+		 */
+		orc = &orc_fp_entry;
+		state->error = true;
+	}
 
 	/* End-of-stack check for kernel threads: */
 	if (orc->sp_reg == ORC_REG_UNDEFINED) {
-- 
2.20.1

[PATCH 8/9] x86/bpf: Convert asm comments to AT&T syntax

[Josh Poimboeuf]

Convert the BPF JIT assembly comments to AT&T syntax to reduce
confusion.  AT&T syntax is the default standard, used throughout Linux
and by the GNU assembler.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 156 ++++++++++++++++++------------------
 1 file changed, 78 insertions(+), 78 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index bb1968fea50a..a92c2445441d 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -58,7 +58,7 @@ static bool is_uimm32(u64 value)
 	return value == (u64)(u32)value;
 }
 
-/* mov dst, src */
+/* mov src, dst */
 #define EMIT_mov(DST, SRC)								 \
 	do {										 \
 		if (DST != SRC)								 \
@@ -202,21 +202,21 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* push rbp */
+	/* push %rbp */
 	EMIT1(0x55);
 
-	/* mov rbp, rsp */
+	/* mov %rsp, %rbp */
 	EMIT3(0x48, 0x89, 0xE5);
 
-	/* push r15 */
+	/* push %r15 */
 	EMIT2(0x41, 0x57);
-	/* push r14 */
+	/* push %r14 */
 	EMIT2(0x41, 0x56);
-	/* push r13 */
+	/* push %r13 */
 	EMIT2(0x41, 0x55);
-	/* push r12 */
+	/* push %r12 */
 	EMIT2(0x41, 0x54);
-	/* push rbx */
+	/* push %rbx */
 	EMIT1(0x53);
 
 	/*
@@ -231,11 +231,11 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	 * R12 is used for the BPF program's FP register.  It points to the end
 	 * of the program's stack area.
 	 *
-	 * mov r12, rsp
+	 * mov %rsp, %r12
 	 */
 	EMIT3(0x49, 0x89, 0xE4);
 
-	/* sub rsp, rounded_stack_depth */
+	/* sub rounded_stack_depth, %rsp */
 	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
 
 	BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
@@ -248,20 +248,20 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* lea rsp, [rbp-0x28] */
+	/* lea -0x28(%rbp), %rsp */
 	EMIT4(0x48, 0x8D, 0x65, 0xD8);
 
-	/* pop rbx */
+	/* pop %rbx */
 	EMIT1(0x5B);
-	/* pop r12 */
+	/* pop %r12 */
 	EMIT2(0x41, 0x5C);
-	/* pop r13 */
+	/* pop %r13 */
 	EMIT2(0x41, 0x5D);
-	/* pop r14 */
+	/* pop %r14 */
 	EMIT2(0x41, 0x5E);
-	/* pop r15 */
+	/* pop %r15 */
 	EMIT2(0x41, 0x5F);
-	/* pop rbp */
+	/* pop %rbp */
 	EMIT1(0x5D);
 
 	/* ret */
@@ -300,8 +300,8 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (index >= array->map.max_entries)
 	 *	goto out;
 	 */
-	EMIT2(0x89, 0xD2);                        /* mov edx, edx */
-	EMIT3(0x39, 0x56,                         /* cmp dword ptr [rsi + 16], edx */
+	EMIT2(0x89, 0xD2);                        /* mov %edx, %edx */
+	EMIT3(0x39, 0x56,                         /* cmp %edx, 0x10(%rsi) */
 	      offsetof(struct bpf_array, map.max_entries));
 #define OFFSET1 (35 + RETPOLINE_RAX_BPF_JIT_SIZE) /* Number of bytes to jump */
 	EMIT2(X86_JBE, OFFSET1);                  /* jbe out */
@@ -311,31 +311,31 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT3(0x8B, 0x45, 0xD4);                  /* mov eax, dword ptr [rbp - 44] */
-	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
+	EMIT3(0x8B, 0x45, 0xD4);                  /* mov -0x2c(%rbp), %eax */
+	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp MAX_TAIL_CALL_CNT, %eax */
 #define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
-	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT3(0x89, 0x45, 0xD4);                  /* mov dword ptr [rbp - 44], eax */
+	EMIT3(0x83, 0xC0, 0x01);                  /* add $0x1, %eax */
+	EMIT3(0x89, 0x45, 0xD4);                  /* mov %eax, -0x2c(%rbp) */
 
 	/* prog = array->ptrs[index]; */
-	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
+	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov offsetof(ptrs)(%rsi,%rdx,8), %rax */
 		    offsetof(struct bpf_array, ptrs));
 
 	/*
 	 * if (prog == NULL)
 	 *	goto out;
 	 */
-	EMIT3(0x48, 0x85, 0xC0);		  /* test rax,rax */
+	EMIT3(0x48, 0x85, 0xC0);		  /* test %rax, %rax */
 #define OFFSET3 (8 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JE, OFFSET3);                   /* je out */
 	label3 = cnt;
 
 	/* goto *(prog->bpf_func + prologue_size); */
-	EMIT4(0x48, 0x8B, 0x40,                   /* mov rax, qword ptr [rax + 32] */
+	EMIT4(0x48, 0x8B, 0x40,                   /* mov offsetof(bpf_func)(%rax), %rax */
 	      offsetof(struct bpf_prog, bpf_func));
-	EMIT4(0x48, 0x83, 0xC0, PROLOGUE_SIZE);   /* add rax, prologue_size */
+	EMIT4(0x48, 0x83, 0xC0, PROLOGUE_SIZE);   /* add $PROLOGUE_SIZE, %rax */
 
 	/*
 	 * Wow we're ready to jump into next BPF program
@@ -359,11 +359,11 @@ static void emit_mov_imm32(u8 **pprog, bool sign_propagate,
 	int cnt = 0;
 
 	/*
-	 * Optimization: if imm32 is positive, use 'mov %eax, imm32'
+	 * Optimization: if imm32 is positive, use 'mov imm32, %eax'
 	 * (which zero-extends imm32) to save 2 bytes.
 	 */
 	if (sign_propagate && (s32)imm32 < 0) {
-		/* 'mov %rax, imm32' sign extends imm32 */
+		/* 'mov imm32, %rax' sign extends imm32 */
 		b1 = add_1mod(0x48, dst_reg);
 		b2 = 0xC7;
 		b3 = 0xC0;
@@ -384,7 +384,7 @@ static void emit_mov_imm32(u8 **pprog, bool sign_propagate,
 		goto done;
 	}
 
-	/* mov %eax, imm32 */
+	/* mov imm32, %eax */
 	if (is_ereg(dst_reg))
 		EMIT1(add_1mod(0x40, dst_reg));
 	EMIT1_off32(add_1reg(0xB8, dst_reg), imm32);
@@ -403,11 +403,11 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
 		 * For emitting plain u32, where sign bit must not be
 		 * propagated LLVM tends to load imm64 over mov32
 		 * directly, so save couple of bytes by just doing
-		 * 'mov %eax, imm32' instead.
+		 * 'mov imm32, %eax' instead.
 		 */
 		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
 	} else {
-		/* movabsq %rax, imm64 */
+		/* movabs imm64, %rax */
 		EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
 		EMIT(imm32_lo, 4);
 		EMIT(imm32_hi, 4);
@@ -422,10 +422,10 @@ static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
 	int cnt = 0;
 
 	if (is64) {
-		/* mov dst, src */
+		/* mov src, dst */
 		EMIT_mov(dst_reg, src_reg);
 	} else {
-		/* mov32 dst, src */
+		/* mov32 src, dst */
 		if (is_ereg(dst_reg) || is_ereg(src_reg))
 			EMIT1(add_2mod(0x40, dst_reg, src_reg));
 		EMIT2(0x89, add_2reg(0xC0, dst_reg, src_reg));
@@ -571,43 +571,43 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_ALU64 | BPF_DIV | BPF_X:
 		case BPF_ALU64 | BPF_MOD | BPF_K:
 		case BPF_ALU64 | BPF_DIV | BPF_K:
-			EMIT1(0x50); /* push rax */
-			EMIT1(0x52); /* push rdx */
+			EMIT1(0x50); /* push %rax */
+			EMIT1(0x52); /* push %rdx */
 
 			if (BPF_SRC(insn->code) == BPF_X)
-				/* mov r11, src_reg */
+				/* mov src_reg, %r11 */
 				EMIT_mov(AUX_REG, src_reg);
 			else
-				/* mov r11, imm32 */
+				/* mov imm32, %r11 */
 				EMIT3_off32(0x49, 0xC7, 0xC3, imm32);
 
-			/* mov rax, dst_reg */
+			/* mov dst_reg, %rax */
 			EMIT_mov(BPF_REG_0, dst_reg);
 
 			/*
-			 * xor edx, edx
-			 * equivalent to 'xor rdx, rdx', but one byte less
+			 * xor %edx, %edx
+			 * equivalent to 'xor %rdx, %rdx', but one byte less
 			 */
 			EMIT2(0x31, 0xd2);
 
 			if (BPF_CLASS(insn->code) == BPF_ALU64)
-				/* div r11 */
+				/* div %r11 */
 				EMIT3(0x49, 0xF7, 0xF3);
 			else
-				/* div r11d */
+				/* div %r11d */
 				EMIT3(0x41, 0xF7, 0xF3);
 
 			if (BPF_OP(insn->code) == BPF_MOD)
-				/* mov r11, rdx */
+				/* mov %r11, %rdx */
 				EMIT3(0x49, 0x89, 0xD3);
 			else
-				/* mov r11, rax */
+				/* mov %r11, %rax */
 				EMIT3(0x49, 0x89, 0xC3);
 
-			EMIT1(0x5A); /* pop rdx */
-			EMIT1(0x58); /* pop rax */
+			EMIT1(0x5A); /* pop %rdx */
+			EMIT1(0x58); /* pop %rax */
 
-			/* mov dst_reg, r11 */
+			/* mov %r11, dst_reg */
 			EMIT_mov(dst_reg, AUX_REG);
 			break;
 
@@ -619,11 +619,11 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
 
 			if (dst_reg != BPF_REG_0)
-				EMIT1(0x50); /* push rax */
+				EMIT1(0x50); /* push %rax */
 			if (dst_reg != BPF_REG_3)
-				EMIT1(0x52); /* push rdx */
+				EMIT1(0x52); /* push %rdx */
 
-			/* mov r11, dst_reg */
+			/* mov dst_reg, %r11 */
 			EMIT_mov(AUX_REG, dst_reg);
 
 			if (BPF_SRC(insn->code) == BPF_X)
@@ -635,15 +635,15 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(add_1mod(0x48, AUX_REG));
 			else if (is_ereg(AUX_REG))
 				EMIT1(add_1mod(0x40, AUX_REG));
-			/* mul(q) r11 */
+			/* mul(q) %r11 */
 			EMIT2(0xF7, add_1reg(0xE0, AUX_REG));
 
 			if (dst_reg != BPF_REG_3)
-				EMIT1(0x5A); /* pop rdx */
+				EMIT1(0x5A); /* pop %rdx */
 			if (dst_reg != BPF_REG_0) {
-				/* mov dst_reg, rax */
+				/* mov %rax, dst_reg */
 				EMIT_mov(dst_reg, BPF_REG_0);
-				EMIT1(0x58); /* pop rax */
+				EMIT1(0x58); /* pop %rax */
 			}
 			break;
 		}
@@ -678,21 +678,21 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_ALU64 | BPF_RSH | BPF_X:
 		case BPF_ALU64 | BPF_ARSH | BPF_X:
 
-			/* Check for bad case when dst_reg == rcx */
+			/* Check for bad case when dst_reg == %rcx */
 			if (dst_reg == BPF_REG_4) {
-				/* mov r11, dst_reg */
+				/* mov dst_reg, %r11 */
 				EMIT_mov(AUX_REG, dst_reg);
 				dst_reg = AUX_REG;
 			}
 
 			if (src_reg != BPF_REG_4) { /* common case */
-				EMIT1(0x51); /* push rcx */
+				EMIT1(0x51); /* push %rcx */
 
-				/* mov rcx, src_reg */
+				/* mov src_reg, %rcx */
 				EMIT_mov(BPF_REG_4, src_reg);
 			}
 
-			/* shl %rax, %cl | shr %rax, %cl | sar %rax, %cl */
+			/* shl %cl, %rax | shr %cl, %rax | sar %cl, %rax */
 			if (BPF_CLASS(insn->code) == BPF_ALU64)
 				EMIT1(add_1mod(0x48, dst_reg));
 			else if (is_ereg(dst_reg))
@@ -706,23 +706,23 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			EMIT2(0xD3, add_1reg(b3, dst_reg));
 
 			if (src_reg != BPF_REG_4)
-				EMIT1(0x59); /* pop rcx */
+				EMIT1(0x59); /* pop %rcx */
 
 			if (insn->dst_reg == BPF_REG_4)
-				/* mov dst_reg, r11 */
+				/* mov %r11, dst_reg */
 				EMIT_mov(insn->dst_reg, AUX_REG);
 			break;
 
 		case BPF_ALU | BPF_END | BPF_FROM_BE:
 			switch (imm32) {
 			case 16:
-				/* Emit 'ror %ax, 8' to swap lower 2 bytes */
+				/* Emit 'ror $0x8, %ax' to swap lower 2 bytes */
 				EMIT1(0x66);
 				if (is_ereg(dst_reg))
 					EMIT1(0x41);
 				EMIT3(0xC1, add_1reg(0xC8, dst_reg), 8);
 
-				/* Emit 'movzwl eax, ax' */
+				/* Emit 'movzwl %ax, %eax' */
 				if (is_ereg(dst_reg))
 					EMIT3(0x45, 0x0F, 0xB7);
 				else
@@ -730,7 +730,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(add_2reg(0xC0, dst_reg, dst_reg));
 				break;
 			case 32:
-				/* Emit 'bswap eax' to swap lower 4 bytes */
+				/* Emit 'bswap %eax' to swap lower 4 bytes */
 				if (is_ereg(dst_reg))
 					EMIT2(0x41, 0x0F);
 				else
@@ -738,7 +738,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(add_1reg(0xC8, dst_reg));
 				break;
 			case 64:
-				/* Emit 'bswap rax' to swap 8 bytes */
+				/* Emit 'bswap %rax' to swap 8 bytes */
 				EMIT3(add_1mod(0x48, dst_reg), 0x0F,
 				      add_1reg(0xC8, dst_reg));
 				break;
@@ -749,7 +749,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			switch (imm32) {
 			case 16:
 				/*
-				 * Emit 'movzwl eax, ax' to zero extend 16-bit
+				 * Emit 'movzwl %ax, %eax' to zero extend 16-bit
 				 * into 64 bit
 				 */
 				if (is_ereg(dst_reg))
@@ -759,7 +759,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(add_2reg(0xC0, dst_reg, dst_reg));
 				break;
 			case 32:
-				/* Emit 'mov eax, eax' to clear upper 32-bits */
+				/* Emit 'mov %eax, %eax' to clear upper 32-bits */
 				if (is_ereg(dst_reg))
 					EMIT1(0x45);
 				EMIT2(0x89, add_2reg(0xC0, dst_reg, dst_reg));
@@ -811,7 +811,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 			/* STX: *(u8*)(dst_reg + off) = src_reg */
 		case BPF_STX | BPF_MEM | BPF_B:
-			/* Emit 'mov byte ptr [rax + off], al' */
+			/* Emit 'mov %al, off(%rax)' */
 			if (is_ereg(dst_reg) || is_ereg(src_reg) ||
 			    /* We have to add extra byte for x86 SIL, DIL regs */
 			    src_reg == BPF_REG_1 || src_reg == BPF_REG_2)
@@ -850,22 +850,22 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 			/* LDX: dst_reg = *(u8*)(src_reg + off) */
 		case BPF_LDX | BPF_MEM | BPF_B:
-			/* Emit 'movzx rax, byte ptr [rax + off]' */
+			/* Emit 'movzbl off(%rax), %rax' */
 			EMIT3(add_2mod(0x48, src_reg, dst_reg), 0x0F, 0xB6);
 			goto ldx;
 		case BPF_LDX | BPF_MEM | BPF_H:
-			/* Emit 'movzx rax, word ptr [rax + off]' */
+			/* Emit 'movzwl off(%rax), %rax' */
 			EMIT3(add_2mod(0x48, src_reg, dst_reg), 0x0F, 0xB7);
 			goto ldx;
 		case BPF_LDX | BPF_MEM | BPF_W:
-			/* Emit 'mov eax, dword ptr [rax+0x14]' */
+			/* Emit 'mov 0x14(%rax), %eax' */
 			if (is_ereg(dst_reg) || is_ereg(src_reg))
 				EMIT2(add_2mod(0x40, src_reg, dst_reg), 0x8B);
 			else
 				EMIT1(0x8B);
 			goto ldx;
 		case BPF_LDX | BPF_MEM | BPF_DW:
-			/* Emit 'mov rax, qword ptr [rax+0x14]' */
+			/* Emit 'mov 0x14(%rax), %rax' */
 			EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x8B);
 ldx:
 			/*
@@ -889,7 +889,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 			/* STX XADD: lock *(u32*)(dst_reg + off) += src_reg */
 		case BPF_STX | BPF_XADD | BPF_W:
-			/* Emit 'lock add dword ptr [rax + off], eax' */
+			/* Emit 'lock add %eax, off(%rax)' */
 			if (is_ereg(dst_reg) || is_ereg(src_reg))
 				EMIT3(0xF0, add_2mod(0x40, dst_reg, src_reg), 0x01);
 			else
@@ -949,7 +949,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_JMP32 | BPF_JSLT | BPF_X:
 		case BPF_JMP32 | BPF_JSGE | BPF_X:
 		case BPF_JMP32 | BPF_JSLE | BPF_X:
-			/* cmp dst_reg, src_reg */
+			/* cmp src_reg, dst_reg */
 			if (BPF_CLASS(insn->code) == BPF_JMP)
 				EMIT1(add_2mod(0x48, dst_reg, src_reg));
 			else if (is_ereg(dst_reg) || is_ereg(src_reg))
@@ -959,7 +959,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 		case BPF_JMP | BPF_JSET | BPF_X:
 		case BPF_JMP32 | BPF_JSET | BPF_X:
-			/* test dst_reg, src_reg */
+			/* test src_reg, dst_reg */
 			if (BPF_CLASS(insn->code) == BPF_JMP)
 				EMIT1(add_2mod(0x48, dst_reg, src_reg));
 			else if (is_ereg(dst_reg) || is_ereg(src_reg))
@@ -969,7 +969,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 		case BPF_JMP | BPF_JSET | BPF_K:
 		case BPF_JMP32 | BPF_JSET | BPF_K:
-			/* test dst_reg, imm32 */
+			/* test imm32, dst_reg */
 			if (BPF_CLASS(insn->code) == BPF_JMP)
 				EMIT1(add_1mod(0x48, dst_reg));
 			else if (is_ereg(dst_reg))
@@ -997,7 +997,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_JMP32 | BPF_JSLT | BPF_K:
 		case BPF_JMP32 | BPF_JSGE | BPF_K:
 		case BPF_JMP32 | BPF_JSLE | BPF_K:
-			/* cmp dst_reg, imm8/32 */
+			/* cmp imm8/32, dst_reg */
 			if (BPF_CLASS(insn->code) == BPF_JMP)
 				EMIT1(add_1mod(0x48, dst_reg));
 			else if (is_ereg(dst_reg))
-- 
2.20.1

[PATCH 9/9] x86/bpf: Convert MOV function/macro argument ordering to AT&T syntax

[Josh Poimboeuf]

Now that the comments have been converted to AT&T syntax, swap the order
of the src/dst arguments in the MOV-related functions and macros to
match the ordering of AT&T syntax.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 44 ++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a92c2445441d..0d0e96f84992 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -59,9 +59,9 @@ static bool is_uimm32(u64 value)
 }
 
 /* mov src, dst */
-#define EMIT_mov(DST, SRC)								 \
+#define EMIT_mov(SRC, DST)								 \
 	do {										 \
-		if (DST != SRC)								 \
+		if (SRC != DST)								 \
 			EMIT3(add_2mod(0x48, DST, SRC), 0x89, add_2reg(0xC0, DST, SRC)); \
 	} while (0)
 
@@ -352,7 +352,7 @@ static void emit_bpf_tail_call(u8 **pprog)
 }
 
 static void emit_mov_imm32(u8 **pprog, bool sign_propagate,
-			   u32 dst_reg, const u32 imm32)
+			   const u32 imm32, u32 dst_reg)
 {
 	u8 *prog = *pprog;
 	u8 b1, b2, b3;
@@ -392,8 +392,8 @@ static void emit_mov_imm32(u8 **pprog, bool sign_propagate,
 	*pprog = prog;
 }
 
-static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
-			   const u32 imm32_hi, const u32 imm32_lo)
+static void emit_mov_imm64(u8 **pprog, const u32 imm32_hi, const u32 imm32_lo,
+			   u32 dst_reg)
 {
 	u8 *prog = *pprog;
 	int cnt = 0;
@@ -405,7 +405,7 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
 		 * directly, so save couple of bytes by just doing
 		 * 'mov imm32, %eax' instead.
 		 */
-		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
+		emit_mov_imm32(&prog, false, imm32_lo, dst_reg);
 	} else {
 		/* movabs imm64, %rax */
 		EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
@@ -416,17 +416,17 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
 	*pprog = prog;
 }
 
-static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
+static void emit_mov_reg(u8 **pprog, bool is64, u32 src_reg, u32 dst_reg)
 {
 	u8 *prog = *pprog;
 	int cnt = 0;
 
 	if (is64) {
 		/* mov src, dst */
-		EMIT_mov(dst_reg, src_reg);
+		EMIT_mov(src_reg, dst_reg);
 	} else {
 		/* mov32 src, dst */
-		if (is_ereg(dst_reg) || is_ereg(src_reg))
+		if (is_ereg(src_reg) || is_ereg(dst_reg))
 			EMIT1(add_2mod(0x40, dst_reg, src_reg));
 		EMIT2(0x89, add_2reg(0xC0, dst_reg, src_reg));
 	}
@@ -487,7 +487,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_ALU | BPF_MOV | BPF_X:
 			emit_mov_reg(&prog,
 				     BPF_CLASS(insn->code) == BPF_ALU64,
-				     dst_reg, src_reg);
+				     src_reg, dst_reg);
 			break;
 
 			/* neg dst */
@@ -553,11 +553,11 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_ALU64 | BPF_MOV | BPF_K:
 		case BPF_ALU | BPF_MOV | BPF_K:
 			emit_mov_imm32(&prog, BPF_CLASS(insn->code) == BPF_ALU64,
-				       dst_reg, imm32);
+				       imm32, dst_reg);
 			break;
 
 		case BPF_LD | BPF_IMM | BPF_DW:
-			emit_mov_imm64(&prog, dst_reg, insn[1].imm, insn[0].imm);
+			emit_mov_imm64(&prog, insn[1].imm, insn[0].imm, dst_reg);
 			insn++;
 			i++;
 			break;
@@ -576,13 +576,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 			if (BPF_SRC(insn->code) == BPF_X)
 				/* mov src_reg, %r11 */
-				EMIT_mov(AUX_REG, src_reg);
+				EMIT_mov(src_reg, AUX_REG);
 			else
 				/* mov imm32, %r11 */
 				EMIT3_off32(0x49, 0xC7, 0xC3, imm32);
 
 			/* mov dst_reg, %rax */
-			EMIT_mov(BPF_REG_0, dst_reg);
+			EMIT_mov(dst_reg, BPF_REG_0);
 
 			/*
 			 * xor %edx, %edx
@@ -608,7 +608,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			EMIT1(0x58); /* pop %rax */
 
 			/* mov %r11, dst_reg */
-			EMIT_mov(dst_reg, AUX_REG);
+			EMIT_mov(AUX_REG, dst_reg);
 			break;
 
 		case BPF_ALU | BPF_MUL | BPF_K:
@@ -624,12 +624,12 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(0x52); /* push %rdx */
 
 			/* mov dst_reg, %r11 */
-			EMIT_mov(AUX_REG, dst_reg);
+			EMIT_mov(dst_reg, AUX_REG);
 
 			if (BPF_SRC(insn->code) == BPF_X)
-				emit_mov_reg(&prog, is64, BPF_REG_0, src_reg);
+				emit_mov_reg(&prog, is64, src_reg, BPF_REG_0);
 			else
-				emit_mov_imm32(&prog, is64, BPF_REG_0, imm32);
+				emit_mov_imm32(&prog, is64, imm32, BPF_REG_0);
 
 			if (is64)
 				EMIT1(add_1mod(0x48, AUX_REG));
@@ -642,7 +642,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(0x5A); /* pop %rdx */
 			if (dst_reg != BPF_REG_0) {
 				/* mov %rax, dst_reg */
-				EMIT_mov(dst_reg, BPF_REG_0);
+				EMIT_mov(BPF_REG_0, dst_reg);
 				EMIT1(0x58); /* pop %rax */
 			}
 			break;
@@ -681,7 +681,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			/* Check for bad case when dst_reg == %rcx */
 			if (dst_reg == BPF_REG_4) {
 				/* mov dst_reg, %r11 */
-				EMIT_mov(AUX_REG, dst_reg);
+				EMIT_mov(dst_reg, AUX_REG);
 				dst_reg = AUX_REG;
 			}
 
@@ -689,7 +689,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 				EMIT1(0x51); /* push %rcx */
 
 				/* mov src_reg, %rcx */
-				EMIT_mov(BPF_REG_4, src_reg);
+				EMIT_mov(src_reg, BPF_REG_4);
 			}
 
 			/* shl %cl, %rax | shr %cl, %rax | sar %cl, %rax */
@@ -710,7 +710,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 			if (insn->dst_reg == BPF_REG_4)
 				/* mov %r11, dst_reg */
-				EMIT_mov(insn->dst_reg, AUX_REG);
+				EMIT_mov(AUX_REG, insn->dst_reg);
 			break;
 
 		case BPF_ALU | BPF_END | BPF_FROM_BE:
-- 
2.20.1

Re: [PATCH 8/9] x86/bpf: Convert asm comments to AT&T syntax

[Song Liu]

> On Jun 13, 2019, at 6:21 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> Convert the BPF JIT assembly comments to AT&T syntax to reduce
> confusion.  AT&T syntax is the default standard, used throughout Linux
> and by the GNU assembler.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/net/bpf_jit_comp.c | 156 ++++++++++++++++++------------------
> 1 file changed, 78 insertions(+), 78 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index bb1968fea50a..a92c2445441d 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -58,7 +58,7 @@ static bool is_uimm32(u64 value)
> 	return value == (u64)(u32)value;
> }
> 
> -/* mov dst, src */
> +/* mov src, dst */
> #define EMIT_mov(DST, SRC)								 \
> 	do {										 \
> 		if (DST != SRC)								 \
> @@ -202,21 +202,21 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
> 	u8 *prog = *pprog;
> 	int cnt = 0;
> 
> -	/* push rbp */
> +	/* push %rbp */
> 	EMIT1(0x55);
> 
> -	/* mov rbp, rsp */
> +	/* mov %rsp, %rbp */
> 	EMIT3(0x48, 0x89, 0xE5);
> 
> -	/* push r15 */
> +	/* push %r15 */
> 	EMIT2(0x41, 0x57);
> -	/* push r14 */
> +	/* push %r14 */
> 	EMIT2(0x41, 0x56);
> -	/* push r13 */
> +	/* push %r13 */
> 	EMIT2(0x41, 0x55);
> -	/* push r12 */
> +	/* push %r12 */
> 	EMIT2(0x41, 0x54);
> -	/* push rbx */
> +	/* push %rbx */
> 	EMIT1(0x53);
> 
> 	/*
> @@ -231,11 +231,11 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
> 	 * R12 is used for the BPF program's FP register.  It points to the end
> 	 * of the program's stack area.
> 	 *
> -	 * mov r12, rsp
> +	 * mov %rsp, %r12
> 	 */
> 	EMIT3(0x49, 0x89, 0xE4);
> 
> -	/* sub rsp, rounded_stack_depth */
> +	/* sub rounded_stack_depth, %rsp */
> 	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
> 
> 	BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
> @@ -248,20 +248,20 @@ static void emit_epilogue(u8 **pprog)
> 	u8 *prog = *pprog;
> 	int cnt = 0;
> 
> -	/* lea rsp, [rbp-0x28] */
> +	/* lea -0x28(%rbp), %rsp */
> 	EMIT4(0x48, 0x8D, 0x65, 0xD8);
> 
> -	/* pop rbx */
> +	/* pop %rbx */
> 	EMIT1(0x5B);
> -	/* pop r12 */
> +	/* pop %r12 */
> 	EMIT2(0x41, 0x5C);
> -	/* pop r13 */
> +	/* pop %r13 */
> 	EMIT2(0x41, 0x5D);
> -	/* pop r14 */
> +	/* pop %r14 */
> 	EMIT2(0x41, 0x5E);
> -	/* pop r15 */
> +	/* pop %r15 */
> 	EMIT2(0x41, 0x5F);
> -	/* pop rbp */
> +	/* pop %rbp */
> 	EMIT1(0x5D);
> 
> 	/* ret */
> @@ -300,8 +300,8 @@ static void emit_bpf_tail_call(u8 **pprog)
> 	 * if (index >= array->map.max_entries)
> 	 *	goto out;
> 	 */
> -	EMIT2(0x89, 0xD2);                        /* mov edx, edx */
> -	EMIT3(0x39, 0x56,                         /* cmp dword ptr [rsi + 16], edx */
> +	EMIT2(0x89, 0xD2);                        /* mov %edx, %edx */
> +	EMIT3(0x39, 0x56,                         /* cmp %edx, 0x10(%rsi) */
> 	      offsetof(struct bpf_array, map.max_entries));
> #define OFFSET1 (35 + RETPOLINE_RAX_BPF_JIT_SIZE) /* Number of bytes to jump */
> 	EMIT2(X86_JBE, OFFSET1);                  /* jbe out */
> @@ -311,31 +311,31 @@ static void emit_bpf_tail_call(u8 **pprog)
> 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
> 	 *	goto out;
> 	 */
> -	EMIT3(0x8B, 0x45, 0xD4);                  /* mov eax, dword ptr [rbp - 44] */
> -	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
> +	EMIT3(0x8B, 0x45, 0xD4);                  /* mov -0x2c(%rbp), %eax */
> +	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp MAX_TAIL_CALL_CNT, %eax */
> #define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
> 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
> 	label2 = cnt;
> -	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
> -	EMIT3(0x89, 0x45, 0xD4);                  /* mov dword ptr [rbp - 44], eax */
> +	EMIT3(0x83, 0xC0, 0x01);                  /* add $0x1, %eax */
> +	EMIT3(0x89, 0x45, 0xD4);                  /* mov %eax, -0x2c(%rbp) */
> 
> 	/* prog = array->ptrs[index]; */
> -	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
> +	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov offsetof(ptrs)(%rsi,%rdx,8), %rax */
> 		    offsetof(struct bpf_array, ptrs));
> 
> 	/*
> 	 * if (prog == NULL)
> 	 *	goto out;
> 	 */
> -	EMIT3(0x48, 0x85, 0xC0);		  /* test rax,rax */
> +	EMIT3(0x48, 0x85, 0xC0);		  /* test %rax, %rax */
> #define OFFSET3 (8 + RETPOLINE_RAX_BPF_JIT_SIZE)
> 	EMIT2(X86_JE, OFFSET3);                   /* je out */
> 	label3 = cnt;
> 
> 	/* goto *(prog->bpf_func + prologue_size); */
> -	EMIT4(0x48, 0x8B, 0x40,                   /* mov rax, qword ptr [rax + 32] */
> +	EMIT4(0x48, 0x8B, 0x40,                   /* mov offsetof(bpf_func)(%rax), %rax */
> 	      offsetof(struct bpf_prog, bpf_func));
> -	EMIT4(0x48, 0x83, 0xC0, PROLOGUE_SIZE);   /* add rax, prologue_size */
> +	EMIT4(0x48, 0x83, 0xC0, PROLOGUE_SIZE);   /* add $PROLOGUE_SIZE, %rax */
> 
> 	/*
> 	 * Wow we're ready to jump into next BPF program
> @@ -359,11 +359,11 @@ static void emit_mov_imm32(u8 **pprog, bool sign_propagate,
> 	int cnt = 0;
> 
> 	/*
> -	 * Optimization: if imm32 is positive, use 'mov %eax, imm32'
> +	 * Optimization: if imm32 is positive, use 'mov imm32, %eax'
> 	 * (which zero-extends imm32) to save 2 bytes.
> 	 */
> 	if (sign_propagate && (s32)imm32 < 0) {
> -		/* 'mov %rax, imm32' sign extends imm32 */
> +		/* 'mov imm32, %rax' sign extends imm32 */
> 		b1 = add_1mod(0x48, dst_reg);
> 		b2 = 0xC7;
> 		b3 = 0xC0;
> @@ -384,7 +384,7 @@ static void emit_mov_imm32(u8 **pprog, bool sign_propagate,
> 		goto done;
> 	}
> 
> -	/* mov %eax, imm32 */
> +	/* mov imm32, %eax */
> 	if (is_ereg(dst_reg))
> 		EMIT1(add_1mod(0x40, dst_reg));
> 	EMIT1_off32(add_1reg(0xB8, dst_reg), imm32);
> @@ -403,11 +403,11 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
> 		 * For emitting plain u32, where sign bit must not be
> 		 * propagated LLVM tends to load imm64 over mov32
> 		 * directly, so save couple of bytes by just doing
> -		 * 'mov %eax, imm32' instead.
> +		 * 'mov imm32, %eax' instead.
> 		 */
> 		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
> 	} else {
> -		/* movabsq %rax, imm64 */
> +		/* movabs imm64, %rax */

		^^^^^ Should this be moveabsq? 

> 		EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
> 		EMIT(imm32_lo, 4);
> 		EMIT(imm32_hi, 4);
> @@ -422,10 +422,10 @@ static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
> 	int cnt = 0;
> 
> 	if (is64) {
> -		/* mov dst, src */
> +		/* mov src, dst */
> 		EMIT_mov(dst_reg, src_reg);
> 	} else {
> -		/* mov32 dst, src */
> +		/* mov32 src, dst */
> 		if (is_ereg(dst_reg) || is_ereg(src_reg))
> 			EMIT1(add_2mod(0x40, dst_reg, src_reg));
> 		EMIT2(0x89, add_2reg(0xC0, dst_reg, src_reg));
> @@ -571,43 +571,43 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 		case BPF_ALU64 | BPF_DIV | BPF_X:
> 		case BPF_ALU64 | BPF_MOD | BPF_K:
> 		case BPF_ALU64 | BPF_DIV | BPF_K:
> -			EMIT1(0x50); /* push rax */
> -			EMIT1(0x52); /* push rdx */
> +			EMIT1(0x50); /* push %rax */
> +			EMIT1(0x52); /* push %rdx */
> 
> 			if (BPF_SRC(insn->code) == BPF_X)
> -				/* mov r11, src_reg */
> +				/* mov src_reg, %r11 */
> 				EMIT_mov(AUX_REG, src_reg);
> 			else
> -				/* mov r11, imm32 */
> +				/* mov imm32, %r11 */
> 				EMIT3_off32(0x49, 0xC7, 0xC3, imm32);
> 
> -			/* mov rax, dst_reg */
> +			/* mov dst_reg, %rax */
> 			EMIT_mov(BPF_REG_0, dst_reg);
> 
> 			/*
> -			 * xor edx, edx
> -			 * equivalent to 'xor rdx, rdx', but one byte less
> +			 * xor %edx, %edx
> +			 * equivalent to 'xor %rdx, %rdx', but one byte less
> 			 */
> 			EMIT2(0x31, 0xd2);
> 
> 			if (BPF_CLASS(insn->code) == BPF_ALU64)
> -				/* div r11 */
> +				/* div %r11 */
> 				EMIT3(0x49, 0xF7, 0xF3);
> 			else
> -				/* div r11d */
> +				/* div %r11d */
> 				EMIT3(0x41, 0xF7, 0xF3);
> 
> 			if (BPF_OP(insn->code) == BPF_MOD)
> -				/* mov r11, rdx */
> +				/* mov %r11, %rdx */
> 				EMIT3(0x49, 0x89, 0xD3);
> 			else
> -				/* mov r11, rax */
> +				/* mov %r11, %rax */
> 				EMIT3(0x49, 0x89, 0xC3);
> 
> -			EMIT1(0x5A); /* pop rdx */
> -			EMIT1(0x58); /* pop rax */
> +			EMIT1(0x5A); /* pop %rdx */
> +			EMIT1(0x58); /* pop %rax */
> 
> -			/* mov dst_reg, r11 */
> +			/* mov %r11, dst_reg */
> 			EMIT_mov(dst_reg, AUX_REG);
> 			break;
> 
> @@ -619,11 +619,11 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 			bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
> 
> 			if (dst_reg != BPF_REG_0)
> -				EMIT1(0x50); /* push rax */
> +				EMIT1(0x50); /* push %rax */
> 			if (dst_reg != BPF_REG_3)
> -				EMIT1(0x52); /* push rdx */
> +				EMIT1(0x52); /* push %rdx */
> 
> -			/* mov r11, dst_reg */
> +			/* mov dst_reg, %r11 */
> 			EMIT_mov(AUX_REG, dst_reg);
> 
> 			if (BPF_SRC(insn->code) == BPF_X)
> @@ -635,15 +635,15 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 				EMIT1(add_1mod(0x48, AUX_REG));
> 			else if (is_ereg(AUX_REG))
> 				EMIT1(add_1mod(0x40, AUX_REG));
> -			/* mul(q) r11 */
> +			/* mul(q) %r11 */
> 			EMIT2(0xF7, add_1reg(0xE0, AUX_REG));
> 
> 			if (dst_reg != BPF_REG_3)
> -				EMIT1(0x5A); /* pop rdx */
> +				EMIT1(0x5A); /* pop %rdx */
> 			if (dst_reg != BPF_REG_0) {
> -				/* mov dst_reg, rax */
> +				/* mov %rax, dst_reg */
> 				EMIT_mov(dst_reg, BPF_REG_0);
> -				EMIT1(0x58); /* pop rax */
> +				EMIT1(0x58); /* pop %rax */
> 			}
> 			break;
> 		}
> @@ -678,21 +678,21 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 		case BPF_ALU64 | BPF_RSH | BPF_X:
> 		case BPF_ALU64 | BPF_ARSH | BPF_X:
> 
> -			/* Check for bad case when dst_reg == rcx */
> +			/* Check for bad case when dst_reg == %rcx */
> 			if (dst_reg == BPF_REG_4) {
> -				/* mov r11, dst_reg */
> +				/* mov dst_reg, %r11 */
> 				EMIT_mov(AUX_REG, dst_reg);
> 				dst_reg = AUX_REG;
> 			}
> 
> 			if (src_reg != BPF_REG_4) { /* common case */
> -				EMIT1(0x51); /* push rcx */
> +				EMIT1(0x51); /* push %rcx */
> 
> -				/* mov rcx, src_reg */
> +				/* mov src_reg, %rcx */
> 				EMIT_mov(BPF_REG_4, src_reg);
> 			}
> 
> -			/* shl %rax, %cl | shr %rax, %cl | sar %rax, %cl */
> +			/* shl %cl, %rax | shr %cl, %rax | sar %cl, %rax */
> 			if (BPF_CLASS(insn->code) == BPF_ALU64)
> 				EMIT1(add_1mod(0x48, dst_reg));
> 			else if (is_ereg(dst_reg))
> @@ -706,23 +706,23 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 			EMIT2(0xD3, add_1reg(b3, dst_reg));
> 
> 			if (src_reg != BPF_REG_4)
> -				EMIT1(0x59); /* pop rcx */
> +				EMIT1(0x59); /* pop %rcx */
> 
> 			if (insn->dst_reg == BPF_REG_4)
> -				/* mov dst_reg, r11 */
> +				/* mov %r11, dst_reg */
> 				EMIT_mov(insn->dst_reg, AUX_REG);
> 			break;
> 
> 		case BPF_ALU | BPF_END | BPF_FROM_BE:
> 			switch (imm32) {
> 			case 16:
> -				/* Emit 'ror %ax, 8' to swap lower 2 bytes */
> +				/* Emit 'ror $0x8, %ax' to swap lower 2 bytes */
> 				EMIT1(0x66);
> 				if (is_ereg(dst_reg))
> 					EMIT1(0x41);
> 				EMIT3(0xC1, add_1reg(0xC8, dst_reg), 8);
> 
> -				/* Emit 'movzwl eax, ax' */
> +				/* Emit 'movzwl %ax, %eax' */
> 				if (is_ereg(dst_reg))
> 					EMIT3(0x45, 0x0F, 0xB7);
> 				else
> @@ -730,7 +730,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 				EMIT1(add_2reg(0xC0, dst_reg, dst_reg));
> 				break;
> 			case 32:
> -				/* Emit 'bswap eax' to swap lower 4 bytes */
> +				/* Emit 'bswap %eax' to swap lower 4 bytes */
> 				if (is_ereg(dst_reg))
> 					EMIT2(0x41, 0x0F);
> 				else
> @@ -738,7 +738,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 				EMIT1(add_1reg(0xC8, dst_reg));
> 				break;
> 			case 64:
> -				/* Emit 'bswap rax' to swap 8 bytes */
> +				/* Emit 'bswap %rax' to swap 8 bytes */
> 				EMIT3(add_1mod(0x48, dst_reg), 0x0F,
> 				      add_1reg(0xC8, dst_reg));
> 				break;
> @@ -749,7 +749,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 			switch (imm32) {
> 			case 16:
> 				/*
> -				 * Emit 'movzwl eax, ax' to zero extend 16-bit
> +				 * Emit 'movzwl %ax, %eax' to zero extend 16-bit
> 				 * into 64 bit
> 				 */
> 				if (is_ereg(dst_reg))
> @@ -759,7 +759,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 				EMIT1(add_2reg(0xC0, dst_reg, dst_reg));
> 				break;
> 			case 32:
> -				/* Emit 'mov eax, eax' to clear upper 32-bits */
> +				/* Emit 'mov %eax, %eax' to clear upper 32-bits */
> 				if (is_ereg(dst_reg))
> 					EMIT1(0x45);
> 				EMIT2(0x89, add_2reg(0xC0, dst_reg, dst_reg));
> @@ -811,7 +811,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 
> 			/* STX: *(u8*)(dst_reg + off) = src_reg */
> 		case BPF_STX | BPF_MEM | BPF_B:
> -			/* Emit 'mov byte ptr [rax + off], al' */
> +			/* Emit 'mov %al, off(%rax)' */
> 			if (is_ereg(dst_reg) || is_ereg(src_reg) ||
> 			    /* We have to add extra byte for x86 SIL, DIL regs */
> 			    src_reg == BPF_REG_1 || src_reg == BPF_REG_2)
> @@ -850,22 +850,22 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 
> 			/* LDX: dst_reg = *(u8*)(src_reg + off) */
> 		case BPF_LDX | BPF_MEM | BPF_B:
> -			/* Emit 'movzx rax, byte ptr [rax + off]' */
> +			/* Emit 'movzbl off(%rax), %rax' */
> 			EMIT3(add_2mod(0x48, src_reg, dst_reg), 0x0F, 0xB6);
> 			goto ldx;
> 		case BPF_LDX | BPF_MEM | BPF_H:
> -			/* Emit 'movzx rax, word ptr [rax + off]' */
> +			/* Emit 'movzwl off(%rax), %rax' */
> 			EMIT3(add_2mod(0x48, src_reg, dst_reg), 0x0F, 0xB7);
> 			goto ldx;
> 		case BPF_LDX | BPF_MEM | BPF_W:
> -			/* Emit 'mov eax, dword ptr [rax+0x14]' */
> +			/* Emit 'mov 0x14(%rax), %eax' */
> 			if (is_ereg(dst_reg) || is_ereg(src_reg))
> 				EMIT2(add_2mod(0x40, src_reg, dst_reg), 0x8B);
> 			else
> 				EMIT1(0x8B);
> 			goto ldx;
> 		case BPF_LDX | BPF_MEM | BPF_DW:
> -			/* Emit 'mov rax, qword ptr [rax+0x14]' */
> +			/* Emit 'mov 0x14(%rax), %rax' */
> 			EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x8B);
> ldx:
> 			/*
> @@ -889,7 +889,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 
> 			/* STX XADD: lock *(u32*)(dst_reg + off) += src_reg */
> 		case BPF_STX | BPF_XADD | BPF_W:
> -			/* Emit 'lock add dword ptr [rax + off], eax' */
> +			/* Emit 'lock add %eax, off(%rax)' */
> 			if (is_ereg(dst_reg) || is_ereg(src_reg))
> 				EMIT3(0xF0, add_2mod(0x40, dst_reg, src_reg), 0x01);
> 			else
> @@ -949,7 +949,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 		case BPF_JMP32 | BPF_JSLT | BPF_X:
> 		case BPF_JMP32 | BPF_JSGE | BPF_X:
> 		case BPF_JMP32 | BPF_JSLE | BPF_X:
> -			/* cmp dst_reg, src_reg */
> +			/* cmp src_reg, dst_reg */
> 			if (BPF_CLASS(insn->code) == BPF_JMP)
> 				EMIT1(add_2mod(0x48, dst_reg, src_reg));
> 			else if (is_ereg(dst_reg) || is_ereg(src_reg))
> @@ -959,7 +959,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 
> 		case BPF_JMP | BPF_JSET | BPF_X:
> 		case BPF_JMP32 | BPF_JSET | BPF_X:
> -			/* test dst_reg, src_reg */
> +			/* test src_reg, dst_reg */
> 			if (BPF_CLASS(insn->code) == BPF_JMP)
> 				EMIT1(add_2mod(0x48, dst_reg, src_reg));
> 			else if (is_ereg(dst_reg) || is_ereg(src_reg))
> @@ -969,7 +969,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 
> 		case BPF_JMP | BPF_JSET | BPF_K:
> 		case BPF_JMP32 | BPF_JSET | BPF_K:
> -			/* test dst_reg, imm32 */
> +			/* test imm32, dst_reg */
> 			if (BPF_CLASS(insn->code) == BPF_JMP)
> 				EMIT1(add_1mod(0x48, dst_reg));
> 			else if (is_ereg(dst_reg))
> @@ -997,7 +997,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> 		case BPF_JMP32 | BPF_JSLT | BPF_K:
> 		case BPF_JMP32 | BPF_JSGE | BPF_K:
> 		case BPF_JMP32 | BPF_JSLE | BPF_K:
> -			/* cmp dst_reg, imm8/32 */
> +			/* cmp imm8/32, dst_reg */
> 			if (BPF_CLASS(insn->code) == BPF_JMP)
> 				EMIT1(add_1mod(0x48, dst_reg));
> 			else if (is_ereg(dst_reg))
> -- 
> 2.20.1
> 

Re: [PATCH 3/9] x86/bpf: Move epilogue generation to a dedicated function

[Song Liu]

> On Jun 13, 2019, at 6:21 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> Improve code readability by moving the BPF JIT function epilogue
> generation code to a dedicated emit_epilogue() function, analagous to
> the existing emit_prologue() function.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/net/bpf_jit_comp.c | 37 ++++++++++++++++++++++++-------------
> 1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 32bfab4e21eb..da8c988b0f0f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -240,6 +240,28 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
> 	*pprog = prog;
> }
> 
> +static void emit_epilogue(u8 **pprog)
> +{
> +	u8 *prog = *pprog;
> +	int cnt = 0;
> +
> +	/* mov rbx, qword ptr [rbp+0] */
> +	EMIT4(0x48, 0x8B, 0x5D, 0);
> +	/* mov r13, qword ptr [rbp+8] */
> +	EMIT4(0x4C, 0x8B, 0x6D, 8);
> +	/* mov r14, qword ptr [rbp+16] */
> +	EMIT4(0x4C, 0x8B, 0x75, 16);
> +	/* mov r15, qword ptr [rbp+24] */
> +	EMIT4(0x4C, 0x8B, 0x7D, 24);

Shall we update these comments to AT&T syntax? 

Thanks,
Song

> +
> +	/* add rbp, AUX_STACK_SPACE */
> +	EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
> +	EMIT1(0xC9); /* leave */
> +	EMIT1(0xC3); /* ret */
> +
> +	*pprog = prog;
> +}
> +
> /*
>  * Generate the following code:
>  *
> @@ -1036,19 +1058,8 @@ xadd:			if (is_imm8(insn->off))
> 			seen_exit = true;
> 			/* Update cleanup_addr */
> 			ctx->cleanup_addr = proglen;
> -			/* mov rbx, qword ptr [rbp+0] */
> -			EMIT4(0x48, 0x8B, 0x5D, 0);
> -			/* mov r13, qword ptr [rbp+8] */
> -			EMIT4(0x4C, 0x8B, 0x6D, 8);
> -			/* mov r14, qword ptr [rbp+16] */
> -			EMIT4(0x4C, 0x8B, 0x75, 16);
> -			/* mov r15, qword ptr [rbp+24] */
> -			EMIT4(0x4C, 0x8B, 0x7D, 24);
> -
> -			/* add rbp, AUX_STACK_SPACE */
> -			EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
> -			EMIT1(0xC9); /* leave */
> -			EMIT1(0xC3); /* ret */
> +
> +			emit_epilogue(&prog);
> 			break;
> 
> 		default:
> -- 
> 2.20.1
> 

Re: [PATCH 0/9] x86/bpf: unwinder fixes

[Song Liu]

> On Jun 13, 2019, at 6:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> The following commit
> 
>  d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> 
> was a step in the right direction, but it triggered some BPF selftest
> failures.  That commit exposed the fact that we currently can't unwind
> through BPF code.
> 
> - Patch 1 (originally from Song Liu) fixes a bug in the above commit
>  (regs->ip getting skipped in the stack trace).
> 
> - Patch 2 fixes non-JIT BPF for the ORC unwinder.
> 
> - Patches 3-5 are preparatory improvements for patch 6.
> 
> - Patch 6 fixes JIT BPF for the FP unwinder.
> 
> - Patch 7 fixes JIT BPF for the ORC unwinder (building on patch 6).
> 
> - Patches 8-9 are some readability cleanups.

These work well in my tests. Thanks!

Tested-by: Song Liu <songliubraving@fb.com>

> 
> 
> Josh Poimboeuf (8):
>  objtool: Fix ORC unwinding in non-JIT BPF generated code
>  x86/bpf: Move epilogue generation to a dedicated function
>  x86/bpf: Simplify prologue generation
>  x86/bpf: Support SIB byte generation
>  x86/bpf: Fix JIT frame pointer usage
>  x86/unwind/orc: Fall back to using frame pointers for generated code
>  x86/bpf: Convert asm comments to AT&T syntax
>  x86/bpf: Convert MOV function/macro argument ordering to AT&T syntax
> 
> Song Liu (1):
>  perf/x86: Always store regs->ip in perf_callchain_kernel()
> 
> arch/x86/events/core.c       |  10 +-
> arch/x86/kernel/unwind_orc.c |  26 ++-
> arch/x86/net/bpf_jit_comp.c  | 355 ++++++++++++++++++++---------------
> kernel/bpf/core.c            |   5 +-
> tools/objtool/check.c        |  16 +-
> 5 files changed, 246 insertions(+), 166 deletions(-)
> 
> -- 
> 2.20.1
> 

Re: [PATCH 8/9] x86/bpf: Convert asm comments to AT&T syntax

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 06:52:24PM +0000, Song Liu wrote:
> > @@ -403,11 +403,11 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
> > 		 * For emitting plain u32, where sign bit must not be
> > 		 * propagated LLVM tends to load imm64 over mov32
> > 		 * directly, so save couple of bytes by just doing
> > -		 * 'mov %eax, imm32' instead.
> > +		 * 'mov imm32, %eax' instead.
> > 		 */
> > 		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
> > 	} else {
> > -		/* movabsq %rax, imm64 */
> > +		/* movabs imm64, %rax */
> 
> 		^^^^^ Should this be moveabsq? 

Not for AT&T syntax:

~ $ cat test.S
movabs $0x1111111111111111, %rax
~ $ as test.S
~ $ objdump -d a.out

a.out:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <.text>:
   0:	48 b8 11 11 11 11 11 	movabs $0x1111111111111111,%rax
   7:	11 11 11

-- 
Josh

Re: [PATCH 3/9] x86/bpf: Move epilogue generation to a dedicated function

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 06:57:10PM +0000, Song Liu wrote:
> 
> 
> > On Jun 13, 2019, at 6:21 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > Improve code readability by moving the BPF JIT function epilogue
> > generation code to a dedicated emit_epilogue() function, analagous to
> > the existing emit_prologue() function.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> > arch/x86/net/bpf_jit_comp.c | 37 ++++++++++++++++++++++++-------------
> > 1 file changed, 24 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index 32bfab4e21eb..da8c988b0f0f 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -240,6 +240,28 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
> > 	*pprog = prog;
> > }
> > 
> > +static void emit_epilogue(u8 **pprog)
> > +{
> > +	u8 *prog = *pprog;
> > +	int cnt = 0;
> > +
> > +	/* mov rbx, qword ptr [rbp+0] */
> > +	EMIT4(0x48, 0x8B, 0x5D, 0);
> > +	/* mov r13, qword ptr [rbp+8] */
> > +	EMIT4(0x4C, 0x8B, 0x6D, 8);
> > +	/* mov r14, qword ptr [rbp+16] */
> > +	EMIT4(0x4C, 0x8B, 0x75, 16);
> > +	/* mov r15, qword ptr [rbp+24] */
> > +	EMIT4(0x4C, 0x8B, 0x7D, 24);
> 
> Shall we update these comments to AT&T syntax? 

Yes, but they're updated with all the others in patch 8.

-- 
Josh

Re: [PATCH 0/9] x86/bpf: unwinder fixes

[Peter Zijlstra]

On Thu, Jun 13, 2019 at 08:20:57AM -0500, Josh Poimboeuf wrote:

> Josh Poimboeuf (8):
>   objtool: Fix ORC unwinding in non-JIT BPF generated code
>   x86/bpf: Move epilogue generation to a dedicated function
>   x86/bpf: Simplify prologue generation
>   x86/bpf: Support SIB byte generation
>   x86/bpf: Fix JIT frame pointer usage
>   x86/unwind/orc: Fall back to using frame pointers for generated code
>   x86/bpf: Convert asm comments to AT&T syntax
>   x86/bpf: Convert MOV function/macro argument ordering to AT&T syntax
> 
> Song Liu (1):
>   perf/x86: Always store regs->ip in perf_callchain_kernel()
> 
>  arch/x86/events/core.c       |  10 +-
>  arch/x86/kernel/unwind_orc.c |  26 ++-
>  arch/x86/net/bpf_jit_comp.c  | 355 ++++++++++++++++++++---------------
>  kernel/bpf/core.c            |   5 +-
>  tools/objtool/check.c        |  16 +-
>  5 files changed, 246 insertions(+), 166 deletions(-)

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:20:59AM -0500, Josh Poimboeuf wrote:
> 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 <songliubraving@fb.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  kernel/bpf/core.c     |  5 ++---
>  tools/objtool/check.c | 16 ++++++++++++++--
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 7c473f208a10..aa546ef7dbdc 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -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] = {

Nack to the change like above and to patches 8 and 9.
Everyone has different stylistic preferences.
My preference is to keep things as they are.

Please respin the rest. We'll take it via bpf tree.

Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> thus prevents the FP unwinder from unwinding through JIT generated code.
> 
> RBP is currently used as the BPF stack frame pointer register.  The
> actual register used is opaque to the user, as long as it's a
> callee-saved register.  Change it to use R12 instead.
> 
> Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> Reported-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 43 +++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index e649f977f8e1..bb1968fea50a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -100,9 +100,8 @@ static int bpf_size_to_x86_bytes(int bpf_size)
>  /*
>   * The following table maps BPF registers to x86-64 registers.
>   *
> - * x86-64 register R12 is unused, since if used as base address
> - * register in load/store instructions, it always needs an
> - * extra byte of encoding and is callee saved.
> + * RBP isn't used; it needs to be preserved to allow the unwinder to move
> + * through generated code stacks.

Extra register save/restore is kinda annoying just to fix ORC.
Also every stack access from bpf prog will be encoded via r12 and consume
extra byte of encoding. I really don't like this approach.
Can you teach ORC to understand JIT-ed frames instead?

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:21:04AM -0500, Josh Poimboeuf wrote:
> The ORC unwinder can't unwind through BPF JIT generated code because
> there are no ORC entries associated with the code.
> 
> If an ORC entry isn't available, try to fall back to frame pointers.  If
> BPF and other generated code always do frame pointer setup (even with
> CONFIG_FRAME_POINTERS=n) then this will allow ORC to unwind through most
> generated code despite there being no corresponding ORC entries.
> 
> Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> Reported-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/unwind_orc.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 33b66b5c5aec..72b997eaa1fc 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -82,9 +82,9 @@ static struct orc_entry *orc_find(unsigned long ip);
>   * But they are copies of the ftrace entries that are static and
>   * defined in ftrace_*.S, which do have orc entries.
>   *
> - * If the undwinder comes across a ftrace trampoline, then find the
> + * If the unwinder comes across a ftrace trampoline, then find the
>   * ftrace function that was used to create it, and use that ftrace
> - * function's orc entrie, as the placement of the return code in
> + * function's orc entry, as the placement of the return code in
>   * the stack will be identical.
>   */
>  static struct orc_entry *orc_ftrace_find(unsigned long ip)
> @@ -128,6 +128,16 @@ static struct orc_entry null_orc_entry = {
>  	.type = ORC_TYPE_CALL
>  };
>  
> +/* Fake frame pointer entry -- used as a fallback for generated code */
> +static struct orc_entry orc_fp_entry = {
> +	.type		= ORC_TYPE_CALL,
> +	.sp_reg		= ORC_REG_BP,
> +	.sp_offset	= 16,
> +	.bp_reg		= ORC_REG_PREV_SP,
> +	.bp_offset	= -16,
> +	.end		= 0,
> +};
> +
>  static struct orc_entry *orc_find(unsigned long ip)
>  {
>  	static struct orc_entry *orc;
> @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
>  	 * calls and calls to noreturn functions.
>  	 */
>  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> -	if (!orc)
> -		goto err;
> +	if (!orc) {
> +		/*
> +		 * As a fallback, try to assume this code uses a frame pointer.
> +		 * This is useful for generated code, like BPF, which ORC
> +		 * doesn't know about.  This is just a guess, so the rest of
> +		 * the unwind is no longer considered reliable.
> +		 */
> +		orc = &orc_fp_entry;
> +		state->error = true;

That seems fragile.
Can't we populate orc_unwind tables after JIT ?

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:20:59AM -0500, Josh Poimboeuf wrote:
> > 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 <songliubraving@fb.com>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  kernel/bpf/core.c     |  5 ++---
> >  tools/objtool/check.c | 16 ++++++++++++++--
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 7c473f208a10..aa546ef7dbdc 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -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] = {
> 
> Nack to the change like above

"jump table" is two words, so does it not make sense to separate them
with an underscore for readability?

I created a generic feature in objtool for this so that other code can
also use it.  So a generic name (and typical Linux naming convention --
separating words with an underscore) makes sense here.

> and to patches 8 and 9.

Well, it's your code, but ... can I ask why?  AT&T syntax is the
standard for Linux, which is in fact the OS we are developing for.

It makes the code extra confusing for it to be annotated differently
than all other Linux asm code.  And due to the inherent complexity of
generating code at runtime, I'd think we'd want to make the code as
readable as possible, for as many people as possible (i.e. other Linux
developers).

-- 
Josh

Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 02:58:09PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > thus prevents the FP unwinder from unwinding through JIT generated code.
> > 
> > RBP is currently used as the BPF stack frame pointer register.  The
> > actual register used is opaque to the user, as long as it's a
> > callee-saved register.  Change it to use R12 instead.
> > 
> > Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> > Reported-by: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 43 +++++++++++++++++++++----------------
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index e649f977f8e1..bb1968fea50a 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -100,9 +100,8 @@ static int bpf_size_to_x86_bytes(int bpf_size)
> >  /*
> >   * The following table maps BPF registers to x86-64 registers.
> >   *
> > - * x86-64 register R12 is unused, since if used as base address
> > - * register in load/store instructions, it always needs an
> > - * extra byte of encoding and is callee saved.
> > + * RBP isn't used; it needs to be preserved to allow the unwinder to move
> > + * through generated code stacks.
> 
> Extra register save/restore is kinda annoying just to fix ORC.

It's not just for the ORC unwinder.  It also fixes the frame pointer
unwinder (see above commit msg).  And it's standard frame pointer
practice to not clobber RBP.

> Also every stack access from bpf prog will be encoded via r12 and consume
> extra byte of encoding. I really don't like this approach.

Do you have another callee-saved register you'd prefer to use as the
stack pointer?

> Can you teach ORC to understand JIT-ed frames instead?

We could, but it would add a lot more complexity than this.  And anyway,
the frame pointer unwinder would still be broken.

-- 
Josh

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 03:00:55PM -0700, Alexei Starovoitov wrote:
> > @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
> >  	 * calls and calls to noreturn functions.
> >  	 */
> >  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> > -	if (!orc)
> > -		goto err;
> > +	if (!orc) {
> > +		/*
> > +		 * As a fallback, try to assume this code uses a frame pointer.
> > +		 * This is useful for generated code, like BPF, which ORC
> > +		 * doesn't know about.  This is just a guess, so the rest of
> > +		 * the unwind is no longer considered reliable.
> > +		 */
> > +		orc = &orc_fp_entry;
> > +		state->error = true;
> 
> That seems fragile.

I don't think so.  The unwinder has sanity checks to make sure it
doesn't go off the rails.  And it works just fine.  The beauty is that
it should work for all generated code (not just BPF).

> Can't we populate orc_unwind tables after JIT ?

As I mentioned it would introduce a lot more complexity.  For each JIT
function, BPF would have to tell ORC the following:

- where the BPF function lives
- how big the stack frame is
- where RBP and other callee-saved regs are on the stack

There's a lot more fragility lurking there, compared to the above.

Not to mention the unwinder would need BPF-specific knowledge, unless we
created some generic abstraction for generated code to register their
functions (which we have actually considered in the past).  But the
above approach is much simpler: just have all generated code use frame
pointers.

-- 
Josh

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> > On Thu, Jun 13, 2019 at 08:20:59AM -0500, Josh Poimboeuf wrote:
> > > 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 <songliubraving@fb.com>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > ---
> > >  kernel/bpf/core.c     |  5 ++---
> > >  tools/objtool/check.c | 16 ++++++++++++++--
> > >  2 files changed, 16 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > index 7c473f208a10..aa546ef7dbdc 100644
> > > --- a/kernel/bpf/core.c
> > > +++ b/kernel/bpf/core.c
> > > @@ -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] = {
> > 
> > Nack to the change like above
> 
> "jump table" is two words, so does it not make sense to separate them
> with an underscore for readability?
> 
> I created a generic feature in objtool for this so that other code can
> also use it.  So a generic name (and typical Linux naming convention --
> separating words with an underscore) makes sense here.
> 
> > and to patches 8 and 9.
> 
> Well, it's your code, but ... can I ask why?  AT&T syntax is the
> standard for Linux, which is in fact the OS we are developing for.
> 
> It makes the code extra confusing for it to be annotated differently
> than all other Linux asm code.  And due to the inherent complexity of
> generating code at runtime, I'd think we'd want to make the code as
> readable as possible, for as many people as possible (i.e. other Linux
> developers).

imo your changes make it less readable.
please do not randomly change names and style based on your own preferences.
dst=src
mov(dst,src)
memcpy(dst,src)
if people want to have more bugs in assembler code. it's their call.
bpf_jit_comp.c is C code. dest is on the left.

Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:22:48PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 02:58:09PM -0700, Alexei Starovoitov wrote:
> > On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > > thus prevents the FP unwinder from unwinding through JIT generated code.
> > > 
> > > RBP is currently used as the BPF stack frame pointer register.  The
> > > actual register used is opaque to the user, as long as it's a
> > > callee-saved register.  Change it to use R12 instead.
> > > 
> > > Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> > > Reported-by: Song Liu <songliubraving@fb.com>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 43 +++++++++++++++++++++----------------
> > >  1 file changed, 25 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index e649f977f8e1..bb1968fea50a 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -100,9 +100,8 @@ static int bpf_size_to_x86_bytes(int bpf_size)
> > >  /*
> > >   * The following table maps BPF registers to x86-64 registers.
> > >   *
> > > - * x86-64 register R12 is unused, since if used as base address
> > > - * register in load/store instructions, it always needs an
> > > - * extra byte of encoding and is callee saved.
> > > + * RBP isn't used; it needs to be preserved to allow the unwinder to move
> > > + * through generated code stacks.
> > 
> > Extra register save/restore is kinda annoying just to fix ORC.
> 
> It's not just for the ORC unwinder.  It also fixes the frame pointer
> unwinder (see above commit msg).  And it's standard frame pointer
> practice to not clobber RBP.

not true.
generated JITed code has no issues with regular stack unwinder.
it breaks down under ORC only.

> > Also every stack access from bpf prog will be encoded via r12 and consume
> > extra byte of encoding. I really don't like this approach.
> 
> Do you have another callee-saved register you'd prefer to use as the
> stack pointer?

RBP must be used.

> > Can you teach ORC to understand JIT-ed frames instead?
> 
> We could, but it would add a lot more complexity than this.  And anyway,
> the frame pointer unwinder would still be broken.

I disagree. See above. Only ORC is broken. Hence ORC should be fixed.

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:30:51PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 03:00:55PM -0700, Alexei Starovoitov wrote:
> > > @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
> > >  	 * calls and calls to noreturn functions.
> > >  	 */
> > >  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> > > -	if (!orc)
> > > -		goto err;
> > > +	if (!orc) {
> > > +		/*
> > > +		 * As a fallback, try to assume this code uses a frame pointer.
> > > +		 * This is useful for generated code, like BPF, which ORC
> > > +		 * doesn't know about.  This is just a guess, so the rest of
> > > +		 * the unwind is no longer considered reliable.
> > > +		 */
> > > +		orc = &orc_fp_entry;
> > > +		state->error = true;
> > 
> > That seems fragile.
> 
> I don't think so.  The unwinder has sanity checks to make sure it
> doesn't go off the rails.  And it works just fine.  The beauty is that
> it should work for all generated code (not just BPF).
> 
> > Can't we populate orc_unwind tables after JIT ?
> 
> As I mentioned it would introduce a lot more complexity.  For each JIT
> function, BPF would have to tell ORC the following:
> 
> - where the BPF function lives
> - how big the stack frame is
> - where RBP and other callee-saved regs are on the stack

that sounds like straightforward addition that ORC should have anyway.
right now we're not using rbp in the jit-ed code,
but one day we definitely will.
Same goes for r12. It's reserved right now for 'strategic use'.
We've been thinking to add another register to bpf isa.
It will map to r12 on x86. arm64 and others have plenty of regs to use.
The programs are getting bigger and register spill/fill starting to
become a performance concern. Extra register will give us more room.

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 06:37:21PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Jun 13, 2019 at 08:20:59AM -0500, Josh Poimboeuf wrote:
> > > > 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 <songliubraving@fb.com>
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > ---
> > > >  kernel/bpf/core.c     |  5 ++---
> > > >  tools/objtool/check.c | 16 ++++++++++++++--
> > > >  2 files changed, 16 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > > index 7c473f208a10..aa546ef7dbdc 100644
> > > > --- a/kernel/bpf/core.c
> > > > +++ b/kernel/bpf/core.c
> > > > @@ -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] = {
> > > 
> > > Nack to the change like above
> > 
> > "jump table" is two words, so does it not make sense to separate them
> > with an underscore for readability?
> > 
> > I created a generic feature in objtool for this so that other code can
> > also use it.  So a generic name (and typical Linux naming convention --
> > separating words with an underscore) makes sense here.
> > 
> > > and to patches 8 and 9.
> > 
> > Well, it's your code, but ... can I ask why?  AT&T syntax is the
> > standard for Linux, which is in fact the OS we are developing for.
> > 
> > It makes the code extra confusing for it to be annotated differently
> > than all other Linux asm code.  And due to the inherent complexity of
> > generating code at runtime, I'd think we'd want to make the code as
> > readable as possible, for as many people as possible (i.e. other Linux
> > developers).
> 
> imo your changes make it less readable.

How does introducing an underscore between two words make them less
readable?

> please do not randomly change names and style based on your own preferences.

These are Linux standards, not my own preferences.

> dst=src
> mov(dst,src)
> memcpy(dst,src)
> if people want to have more bugs in assembler code. it's their call.
> bpf_jit_comp.c is C code. dest is on the left.

So you don't like the ordering of the src,dst function arguments?  Ok.

But what do you think about the AT&T syntax comments?

-- 
Josh

Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 06:39:05PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:22:48PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 02:58:09PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > > > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > > > thus prevents the FP unwinder from unwinding through JIT generated code.
> > > > 
> > > > RBP is currently used as the BPF stack frame pointer register.  The
> > > > actual register used is opaque to the user, as long as it's a
> > > > callee-saved register.  Change it to use R12 instead.
> > > > 
> > > > Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> > > > Reported-by: Song Liu <songliubraving@fb.com>
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c | 43 +++++++++++++++++++++----------------
> > > >  1 file changed, 25 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > > index e649f977f8e1..bb1968fea50a 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -100,9 +100,8 @@ static int bpf_size_to_x86_bytes(int bpf_size)
> > > >  /*
> > > >   * The following table maps BPF registers to x86-64 registers.
> > > >   *
> > > > - * x86-64 register R12 is unused, since if used as base address
> > > > - * register in load/store instructions, it always needs an
> > > > - * extra byte of encoding and is callee saved.
> > > > + * RBP isn't used; it needs to be preserved to allow the unwinder to move
> > > > + * through generated code stacks.
> > > 
> > > Extra register save/restore is kinda annoying just to fix ORC.
> > 
> > It's not just for the ORC unwinder.  It also fixes the frame pointer
> > unwinder (see above commit msg).  And it's standard frame pointer
> > practice to not clobber RBP.
> 
> not true.
> generated JITed code has no issues with regular stack unwinder.
> it breaks down under ORC only.
> 
> > > Also every stack access from bpf prog will be encoded via r12 and consume
> > > extra byte of encoding. I really don't like this approach.
> > 
> > Do you have another callee-saved register you'd prefer to use as the
> > stack pointer?
> 
> RBP must be used.
> 
> > > Can you teach ORC to understand JIT-ed frames instead?
> > 
> > We could, but it would add a lot more complexity than this.  And anyway,
> > the frame pointer unwinder would still be broken.
> 
> I disagree. See above. Only ORC is broken. Hence ORC should be fixed.

You're clobbering RBP.  Frame pointer unwinding is broken.  Period.

-- 
Josh

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 06:42:45PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:30:51PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 03:00:55PM -0700, Alexei Starovoitov wrote:
> > > > @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
> > > >  	 * calls and calls to noreturn functions.
> > > >  	 */
> > > >  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> > > > -	if (!orc)
> > > > -		goto err;
> > > > +	if (!orc) {
> > > > +		/*
> > > > +		 * As a fallback, try to assume this code uses a frame pointer.
> > > > +		 * This is useful for generated code, like BPF, which ORC
> > > > +		 * doesn't know about.  This is just a guess, so the rest of
> > > > +		 * the unwind is no longer considered reliable.
> > > > +		 */
> > > > +		orc = &orc_fp_entry;
> > > > +		state->error = true;
> > > 
> > > That seems fragile.
> > 
> > I don't think so.  The unwinder has sanity checks to make sure it
> > doesn't go off the rails.  And it works just fine.  The beauty is that
> > it should work for all generated code (not just BPF).
> > 
> > > Can't we populate orc_unwind tables after JIT ?
> > 
> > As I mentioned it would introduce a lot more complexity.  For each JIT
> > function, BPF would have to tell ORC the following:
> > 
> > - where the BPF function lives
> > - how big the stack frame is
> > - where RBP and other callee-saved regs are on the stack
> 
> that sounds like straightforward addition that ORC should have anyway.
> right now we're not using rbp in the jit-ed code,
> but one day we definitely will.
> Same goes for r12. It's reserved right now for 'strategic use'.
> We've been thinking to add another register to bpf isa.
> It will map to r12 on x86. arm64 and others have plenty of regs to use.
> The programs are getting bigger and register spill/fill starting to
> become a performance concern. Extra register will give us more room.

With CONFIG_FRAME_POINTER, RBP isn't available.  If you look at all the
code in the entire kernel you'll notice that BPF JIT is pretty much the
only one still clobbering it.

-- 
Josh

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 08:58:48PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 06:42:45PM -0700, Alexei Starovoitov wrote:
> > On Thu, Jun 13, 2019 at 08:30:51PM -0500, Josh Poimboeuf wrote:
> > > On Thu, Jun 13, 2019 at 03:00:55PM -0700, Alexei Starovoitov wrote:
> > > > > @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
> > > > >  	 * calls and calls to noreturn functions.
> > > > >  	 */
> > > > >  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> > > > > -	if (!orc)
> > > > > -		goto err;
> > > > > +	if (!orc) {
> > > > > +		/*
> > > > > +		 * As a fallback, try to assume this code uses a frame pointer.
> > > > > +		 * This is useful for generated code, like BPF, which ORC
> > > > > +		 * doesn't know about.  This is just a guess, so the rest of
> > > > > +		 * the unwind is no longer considered reliable.
> > > > > +		 */
> > > > > +		orc = &orc_fp_entry;
> > > > > +		state->error = true;
> > > > 
> > > > That seems fragile.
> > > 
> > > I don't think so.  The unwinder has sanity checks to make sure it
> > > doesn't go off the rails.  And it works just fine.  The beauty is that
> > > it should work for all generated code (not just BPF).
> > > 
> > > > Can't we populate orc_unwind tables after JIT ?
> > > 
> > > As I mentioned it would introduce a lot more complexity.  For each JIT
> > > function, BPF would have to tell ORC the following:
> > > 
> > > - where the BPF function lives
> > > - how big the stack frame is
> > > - where RBP and other callee-saved regs are on the stack
> > 
> > that sounds like straightforward addition that ORC should have anyway.
> > right now we're not using rbp in the jit-ed code,
> > but one day we definitely will.
> > Same goes for r12. It's reserved right now for 'strategic use'.
> > We've been thinking to add another register to bpf isa.
> > It will map to r12 on x86. arm64 and others have plenty of regs to use.
> > The programs are getting bigger and register spill/fill starting to
> > become a performance concern. Extra register will give us more room.
> 
> With CONFIG_FRAME_POINTER, RBP isn't available.  If you look at all the
> code in the entire kernel you'll notice that BPF JIT is pretty much the
> only one still clobbering it.

Hm.  If you wanted to eventually use R12 for other purposes, there might
be a way to abstract BPF_REG_FP such that it doesn't actually need a
dedicated register.  The BPF program's frame pointer will always be a
certain constant offset away from RBP (real frame pointer), so accesses
to BPF_REG_FP could still be based on RBP, but with an offset added to
it.

-- 
Josh

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 09:28:48PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 08:58:48PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 06:42:45PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Jun 13, 2019 at 08:30:51PM -0500, Josh Poimboeuf wrote:
> > > > On Thu, Jun 13, 2019 at 03:00:55PM -0700, Alexei Starovoitov wrote:
> > > > > > @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
> > > > > >  	 * calls and calls to noreturn functions.
> > > > > >  	 */
> > > > > >  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> > > > > > -	if (!orc)
> > > > > > -		goto err;
> > > > > > +	if (!orc) {
> > > > > > +		/*
> > > > > > +		 * As a fallback, try to assume this code uses a frame pointer.
> > > > > > +		 * This is useful for generated code, like BPF, which ORC
> > > > > > +		 * doesn't know about.  This is just a guess, so the rest of
> > > > > > +		 * the unwind is no longer considered reliable.
> > > > > > +		 */
> > > > > > +		orc = &orc_fp_entry;
> > > > > > +		state->error = true;
> > > > > 
> > > > > That seems fragile.
> > > > 
> > > > I don't think so.  The unwinder has sanity checks to make sure it
> > > > doesn't go off the rails.  And it works just fine.  The beauty is that
> > > > it should work for all generated code (not just BPF).
> > > > 
> > > > > Can't we populate orc_unwind tables after JIT ?
> > > > 
> > > > As I mentioned it would introduce a lot more complexity.  For each JIT
> > > > function, BPF would have to tell ORC the following:
> > > > 
> > > > - where the BPF function lives
> > > > - how big the stack frame is
> > > > - where RBP and other callee-saved regs are on the stack
> > > 
> > > that sounds like straightforward addition that ORC should have anyway.
> > > right now we're not using rbp in the jit-ed code,
> > > but one day we definitely will.
> > > Same goes for r12. It's reserved right now for 'strategic use'.
> > > We've been thinking to add another register to bpf isa.
> > > It will map to r12 on x86. arm64 and others have plenty of regs to use.
> > > The programs are getting bigger and register spill/fill starting to
> > > become a performance concern. Extra register will give us more room.
> > 
> > With CONFIG_FRAME_POINTER, RBP isn't available.  If you look at all the
> > code in the entire kernel you'll notice that BPF JIT is pretty much the
> > only one still clobbering it.
> 
> Hm.  If you wanted to eventually use R12 for other purposes, there might
> be a way to abstract BPF_REG_FP such that it doesn't actually need a
> dedicated register.  The BPF program's frame pointer will always be a
> certain constant offset away from RBP (real frame pointer), so accesses
> to BPF_REG_FP could still be based on RBP, but with an offset added to
> it.

How about something like this (based on top of patch 4)?  This fixes
frame pointers without using R12, by making BPF_REG_FP equivalent to
RBP, minus a constant offset (callee-save area + tail_call_cnt = 40).

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 485692d4b163..2f313622c741 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -104,6 +104,9 @@ static int bpf_size_to_x86_bytes(int bpf_size)
  * register in load/store instructions, it always needs an
  * extra byte of encoding and is callee saved.
  *
+ * BPF_REG_FP corresponds to x86-64 register RBP, but 40 bytes must be
+ * subtracted from it to get the BPF_REG_FP value.
+ *
  * Also x86-64 register R9 is unused. x86-64 register R10 is
  * used for blinding (if enabled).
  */
@@ -118,11 +121,18 @@ static const int reg2hex[] = {
 	[BPF_REG_7] = 5,  /* R13 callee saved */
 	[BPF_REG_8] = 6,  /* R14 callee saved */
 	[BPF_REG_9] = 7,  /* R15 callee saved */
-	[BPF_REG_FP] = 5, /* RBP readonly */
+	[BPF_REG_FP] = 5, /* (RBP - 40 bytes) readonly */
 	[BPF_REG_AX] = 2, /* R10 temp register */
 	[AUX_REG] = 3,    /* R11 temp register */
 };
 
+static s16 offset(struct bpf_insn *insn)
+{
+	if (insn->src_reg == BPF_REG_FP || insn->dst_reg == BPF_REG_FP)
+		return insn->off - 40;
+	return insn->off;
+}
+
 /*
  * is_ereg() == true if BPF register 'reg' maps to x86-64 r8..r15
  * which need extra byte of encoding.
@@ -197,14 +207,18 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
+	/* push rbp */
+	EMIT1(0x55);
+
+	/* mov rbp,rsp */
+	EMIT3(0x48, 0x89, 0xE5);
+
 	/* push r15 */
 	EMIT2(0x41, 0x57);
 	/* push r14 */
 	EMIT2(0x41, 0x56);
 	/* push r13 */
 	EMIT2(0x41, 0x55);
-	/* push rbp */
-	EMIT1(0x55);
 	/* push rbx */
 	EMIT1(0x53);
 
@@ -216,14 +230,6 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	 */
 	EMIT2(0x6a, 0x00);
 
-	/*
-	 * RBP is used for the BPF program's FP register.  It points to the end
-	 * of the program's stack area.
-	 *
-	 * mov rbp, rsp
-	 */
-	EMIT3(0x48, 0x89, 0xE5);
-
 	/* sub rsp, rounded_stack_depth */
 	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
 
@@ -237,19 +243,19 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* lea rsp, [rbp+0x8] */
-	EMIT4(0x48, 0x8D, 0x65, 0x08);
+	/* lea rsp, [rbp-0x20] */
+	EMIT4(0x48, 0x8D, 0x65, 0xE0);
 
 	/* pop rbx */
 	EMIT1(0x5B);
-	/* pop rbp */
-	EMIT1(0x5D);
 	/* pop r13 */
 	EMIT2(0x41, 0x5D);
 	/* pop r14 */
 	EMIT2(0x41, 0x5E);
 	/* pop r15 */
 	EMIT2(0x41, 0x5F);
+	/* pop rbp */
+	EMIT1(0x5D);
 
 	/* ret */
 	EMIT1(0xC3);
@@ -298,13 +304,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT3(0x8B, 0x45, 0x04);                  /* mov eax, dword ptr [rbp + 4] */
+	EMIT3(0x8B, 0x45, 0xDC);                  /* mov eax, dword ptr [rbp - 36] */
 	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
 #define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT3(0x89, 0x45, 0x04);                  /* mov dword ptr [rbp + 4], eax */
+	EMIT3(0x89, 0x45, 0xDC);                  /* mov dword ptr [rbp - 36], eax */
 
 	/* prog = array->ptrs[index]; */
 	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
@@ -418,6 +424,17 @@ static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
 		EMIT2(0x89, add_2reg(0xC0, dst_reg, src_reg));
 	}
 
+	if (src_reg == BPF_REG_FP) {
+		/*
+		 * If the value was copied from RBP (real frame pointer),
+		 * adjust it to the BPF program's frame pointer value.
+		 *
+		 * add dst, -40
+		 */
+		EMIT4(add_1mod(0x48, dst_reg), 0x83, add_1reg(0xC0, dst_reg),
+		      0xD8);
+	}
+
 	*pprog = prog;
 }
 
@@ -779,10 +796,10 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		case BPF_ST | BPF_MEM | BPF_DW:
 			EMIT2(add_1mod(0x48, dst_reg), 0xC7);
 
-st:			if (is_imm8(insn->off))
-				EMIT2(add_1reg(0x40, dst_reg), insn->off);
+st:			if (is_imm8(offset(insn)))
+				EMIT2(add_1reg(0x40, dst_reg), offset(insn));
 			else
-				EMIT1_off32(add_1reg(0x80, dst_reg), insn->off);
+				EMIT1_off32(add_1reg(0x80, dst_reg), offset(insn));
 
 			EMIT(imm32, bpf_size_to_x86_bytes(BPF_SIZE(insn->code)));
 			break;
@@ -811,11 +828,11 @@ st:			if (is_imm8(insn->off))
 			goto stx;
 		case BPF_STX | BPF_MEM | BPF_DW:
 			EMIT2(add_2mod(0x48, dst_reg, src_reg), 0x89);
-stx:			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
+stx:			if (is_imm8(offset(insn)))
+				EMIT2(add_2reg(0x40, dst_reg, src_reg), offset(insn));
 			else
 				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
-					    insn->off);
+					    offset(insn));
 			break;
 
 			/* LDX: dst_reg = *(u8*)(src_reg + off) */
@@ -838,15 +855,15 @@ stx:			if (is_imm8(insn->off))
 			/* Emit 'mov rax, qword ptr [rax+0x14]' */
 			EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x8B);
 ldx:			/*
-			 * If insn->off == 0 we can save one extra byte, but
+			 * If offset(insn) == 0 we can save one extra byte, but
 			 * special case of x86 R13 which always needs an offset
 			 * is not worth the hassle
 			 */
-			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, src_reg, dst_reg), insn->off);
+			if (is_imm8(offset(insn)))
+				EMIT2(add_2reg(0x40, src_reg, dst_reg), offset(insn));
 			else
 				EMIT1_off32(add_2reg(0x80, src_reg, dst_reg),
-					    insn->off);
+					    offset(insn));
 			break;
 
 			/* STX XADD: lock *(u32*)(dst_reg + off) += src_reg */
@@ -859,11 +876,11 @@ stx:			if (is_imm8(insn->off))
 			goto xadd;
 		case BPF_STX | BPF_XADD | BPF_DW:
 			EMIT3(0xF0, add_2mod(0x48, dst_reg, src_reg), 0x01);
-xadd:			if (is_imm8(insn->off))
-				EMIT2(add_2reg(0x40, dst_reg, src_reg), insn->off);
+xadd:			if (is_imm8(offset(insn)))
+				EMIT2(add_2reg(0x40, dst_reg, src_reg), offset(insn));
 			else
 				EMIT1_off32(add_2reg(0x80, dst_reg, src_reg),
-					    insn->off);
+					    offset(insn));
 			break;
 
 			/* call */

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 11:50:37PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 09:28:48PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 08:58:48PM -0500, Josh Poimboeuf wrote:
> > > On Thu, Jun 13, 2019 at 06:42:45PM -0700, Alexei Starovoitov wrote:
> > > > On Thu, Jun 13, 2019 at 08:30:51PM -0500, Josh Poimboeuf wrote:
> > > > > On Thu, Jun 13, 2019 at 03:00:55PM -0700, Alexei Starovoitov wrote:
> > > > > > > @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
> > > > > > >  	 * calls and calls to noreturn functions.
> > > > > > >  	 */
> > > > > > >  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> > > > > > > -	if (!orc)
> > > > > > > -		goto err;
> > > > > > > +	if (!orc) {
> > > > > > > +		/*
> > > > > > > +		 * As a fallback, try to assume this code uses a frame pointer.
> > > > > > > +		 * This is useful for generated code, like BPF, which ORC
> > > > > > > +		 * doesn't know about.  This is just a guess, so the rest of
> > > > > > > +		 * the unwind is no longer considered reliable.
> > > > > > > +		 */
> > > > > > > +		orc = &orc_fp_entry;
> > > > > > > +		state->error = true;
> > > > > > 
> > > > > > That seems fragile.
> > > > > 
> > > > > I don't think so.  The unwinder has sanity checks to make sure it
> > > > > doesn't go off the rails.  And it works just fine.  The beauty is that
> > > > > it should work for all generated code (not just BPF).
> > > > > 
> > > > > > Can't we populate orc_unwind tables after JIT ?
> > > > > 
> > > > > As I mentioned it would introduce a lot more complexity.  For each JIT
> > > > > function, BPF would have to tell ORC the following:
> > > > > 
> > > > > - where the BPF function lives
> > > > > - how big the stack frame is
> > > > > - where RBP and other callee-saved regs are on the stack
> > > > 
> > > > that sounds like straightforward addition that ORC should have anyway.
> > > > right now we're not using rbp in the jit-ed code,
> > > > but one day we definitely will.
> > > > Same goes for r12. It's reserved right now for 'strategic use'.
> > > > We've been thinking to add another register to bpf isa.
> > > > It will map to r12 on x86. arm64 and others have plenty of regs to use.
> > > > The programs are getting bigger and register spill/fill starting to
> > > > become a performance concern. Extra register will give us more room.
> > > 
> > > With CONFIG_FRAME_POINTER, RBP isn't available.  If you look at all the
> > > code in the entire kernel you'll notice that BPF JIT is pretty much the
> > > only one still clobbering it.
> > 
> > Hm.  If you wanted to eventually use R12 for other purposes, there might
> > be a way to abstract BPF_REG_FP such that it doesn't actually need a
> > dedicated register.  The BPF program's frame pointer will always be a
> > certain constant offset away from RBP (real frame pointer), so accesses
> > to BPF_REG_FP could still be based on RBP, but with an offset added to
> > it.
> 
> How about something like this (based on top of patch 4)?  This fixes
> frame pointers without using R12, by making BPF_REG_FP equivalent to
> RBP, minus a constant offset (callee-save area + tail_call_cnt = 40).
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 485692d4b163..2f313622c741 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -104,6 +104,9 @@ static int bpf_size_to_x86_bytes(int bpf_size)
>   * register in load/store instructions, it always needs an
>   * extra byte of encoding and is callee saved.
>   *
> + * BPF_REG_FP corresponds to x86-64 register RBP, but 40 bytes must be
> + * subtracted from it to get the BPF_REG_FP value.
> + *
>   * Also x86-64 register R9 is unused. x86-64 register R10 is
>   * used for blinding (if enabled).
>   */
> @@ -118,11 +121,18 @@ static const int reg2hex[] = {
>  	[BPF_REG_7] = 5,  /* R13 callee saved */
>  	[BPF_REG_8] = 6,  /* R14 callee saved */
>  	[BPF_REG_9] = 7,  /* R15 callee saved */
> -	[BPF_REG_FP] = 5, /* RBP readonly */
> +	[BPF_REG_FP] = 5, /* (RBP - 40 bytes) readonly */
>  	[BPF_REG_AX] = 2, /* R10 temp register */
>  	[AUX_REG] = 3,    /* R11 temp register */
>  };
>  
> +static s16 offset(struct bpf_insn *insn)
> +{
> +	if (insn->src_reg == BPF_REG_FP || insn->dst_reg == BPF_REG_FP)
> +		return insn->off - 40;
> +	return insn->off;
> +}
> +
>  /*
>   * is_ereg() == true if BPF register 'reg' maps to x86-64 r8..r15
>   * which need extra byte of encoding.
> @@ -197,14 +207,18 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
>  	u8 *prog = *pprog;
>  	int cnt = 0;
>  
> +	/* push rbp */
> +	EMIT1(0x55);
> +
> +	/* mov rbp,rsp */
> +	EMIT3(0x48, 0x89, 0xE5);
> +
>  	/* push r15 */
>  	EMIT2(0x41, 0x57);
>  	/* push r14 */
>  	EMIT2(0x41, 0x56);
>  	/* push r13 */
>  	EMIT2(0x41, 0x55);
> -	/* push rbp */
> -	EMIT1(0x55);
>  	/* push rbx */
>  	EMIT1(0x53);
>  
> @@ -216,14 +230,6 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
>  	 */
>  	EMIT2(0x6a, 0x00);
>  
> -	/*
> -	 * RBP is used for the BPF program's FP register.  It points to the end
> -	 * of the program's stack area.
> -	 *
> -	 * mov rbp, rsp
> -	 */
> -	EMIT3(0x48, 0x89, 0xE5);
> -
>  	/* sub rsp, rounded_stack_depth */
>  	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
>  
> @@ -237,19 +243,19 @@ static void emit_epilogue(u8 **pprog)
>  	u8 *prog = *pprog;
>  	int cnt = 0;
>  
> -	/* lea rsp, [rbp+0x8] */
> -	EMIT4(0x48, 0x8D, 0x65, 0x08);
> +	/* lea rsp, [rbp-0x20] */
> +	EMIT4(0x48, 0x8D, 0x65, 0xE0);
>  
>  	/* pop rbx */
>  	EMIT1(0x5B);
> -	/* pop rbp */
> -	EMIT1(0x5D);
>  	/* pop r13 */
>  	EMIT2(0x41, 0x5D);
>  	/* pop r14 */
>  	EMIT2(0x41, 0x5E);
>  	/* pop r15 */
>  	EMIT2(0x41, 0x5F);
> +	/* pop rbp */
> +	EMIT1(0x5D);
>  
>  	/* ret */
>  	EMIT1(0xC3);
> @@ -298,13 +304,13 @@ static void emit_bpf_tail_call(u8 **pprog)
>  	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
>  	 *	goto out;
>  	 */
> -	EMIT3(0x8B, 0x45, 0x04);                  /* mov eax, dword ptr [rbp + 4] */
> +	EMIT3(0x8B, 0x45, 0xDC);                  /* mov eax, dword ptr [rbp - 36] */
>  	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
>  #define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
>  	EMIT2(X86_JA, OFFSET2);                   /* ja out */
>  	label2 = cnt;
>  	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
> -	EMIT3(0x89, 0x45, 0x04);                  /* mov dword ptr [rbp + 4], eax */
> +	EMIT3(0x89, 0x45, 0xDC);                  /* mov dword ptr [rbp - 36], eax */
>  
>  	/* prog = array->ptrs[index]; */
>  	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
> @@ -418,6 +424,17 @@ static void emit_mov_reg(u8 **pprog, bool is64, u32 dst_reg, u32 src_reg)
>  		EMIT2(0x89, add_2reg(0xC0, dst_reg, src_reg));
>  	}
>  
> +	if (src_reg == BPF_REG_FP) {
> +		/*
> +		 * If the value was copied from RBP (real frame pointer),
> +		 * adjust it to the BPF program's frame pointer value.
> +		 *
> +		 * add dst, -40
> +		 */
> +		EMIT4(add_1mod(0x48, dst_reg), 0x83, add_1reg(0xC0, dst_reg),
> +		      0xD8);
> +	}
> +

That won't work. Any register can point to a stack.
The register can point to a stack of a different JITed function as well.

There is something wrong with
commit d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

If I simply revert it and have CONFIG_UNWINDER_FRAME_POINTER=y
JITed stacks work just fine, because
bpf_get_stackid()->get_perf_callchain()
need to start unwinding before any bpf stuff.
After that commit it needs to go through which is a bug on its own.
imo patch 1 doesn't really fix that issue.

As far as mangled rbp can we partially undo old
commit 177366bf7ceb ("bpf: change x86 JITed program stack layout")
that introduced that rbp adjustment.
Going through bpf code is only interesting in case of panics somewhere
in bpf helpers. Back then we didn't even have ksym of jited code.

Anyhow I agree that we need to make the jited frame proper,
but unwinding need to start before any bpf stuff.
That's a bigger issue.

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Peter Zijlstra]

On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:

> > and to patches 8 and 9.
> 
> Well, it's your code, but ... can I ask why?  AT&T syntax is the
> standard for Linux, which is in fact the OS we are developing for.

I agree, all assembly in Linux is AT&T, adding Intel notation only
serves to cause confusion.

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote:
> On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> 
> > > and to patches 8 and 9.
> > 
> > Well, it's your code, but ... can I ask why?  AT&T syntax is the
> > standard for Linux, which is in fact the OS we are developing for.
> 
> I agree, all assembly in Linux is AT&T, adding Intel notation only
> serves to cause confusion.

It's not assembly. It's C code that generates binary and here
we're talking about comments.
I'm sure you're not proposing to do:
/* mov src, dst */
#define EMIT_mov(DST, SRC)                                                               \
right?
bpf_jit_comp.c stays as-is. Enough of it.

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Peter Zijlstra]

On Thu, Jun 13, 2019 at 11:00:09PM -0700, Alexei Starovoitov wrote:

> There is something wrong with
> commit d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

It assumes we can always unwind stack, which is, imo, not a weird thing.

> If I simply revert it and have CONFIG_UNWINDER_FRAME_POINTER=y
> JITed stacks work just fine, because
> bpf_get_stackid()->get_perf_callchain()
> need to start unwinding before any bpf stuff.

How does stack unwinding work if we try and unwind from an interrupt
that hits inside a BPF program? That too needs to work properly.

> After that commit it needs to go through which is a bug on its own.
> imo patch 1 doesn't really fix that issue.

This we agree on, patch 1 doesn't solve that at all. But we also should
not loose the initial regs->ip value.

> As far as mangled rbp can we partially undo old
> commit 177366bf7ceb ("bpf: change x86 JITed program stack layout")
> that introduced that rbp adjustment.

> Going through bpf code is only interesting in case of panics somewhere
> in bpf helpers. Back then we didn't even have ksym of jited code.

I disagree here, interrupts/NMIs hitting inside BPF should be able to
reliably unwind the entire stack. Back then is irrelevant, these days we
expect a reliable unwind.

> Anyhow I agree that we need to make the jited frame proper,
> but unwinding need to start before any bpf stuff.
> That's a bigger issue.

I strongly disagree, we should be able to unwind through bpf.

Re: [PATCH 8/9] x86/bpf: Convert asm comments to AT&T syntax

[Peter Zijlstra]

On Thu, Jun 13, 2019 at 06:52:24PM +0000, Song Liu wrote:
> > On Jun 13, 2019, at 6:21 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > @@ -403,11 +403,11 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
> > 		 * For emitting plain u32, where sign bit must not be
> > 		 * propagated LLVM tends to load imm64 over mov32
> > 		 * directly, so save couple of bytes by just doing
> > -		 * 'mov %eax, imm32' instead.
> > +		 * 'mov imm32, %eax' instead.
> > 		 */
> > 		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
> > 	} else {
> > -		/* movabsq %rax, imm64 */
> > +		/* movabs imm64, %rax */
> 
> 		^^^^^ Should this be moveabsq? 
> 
> > 		EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
> > 		EMIT(imm32_lo, 4);
> > 		EMIT(imm32_hi, 4);

Song, can you please trim replies; I only found what you said because of
Josh's reply.

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 12:35:38AM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote:
> > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> > 
> > > > and to patches 8 and 9.
> > > 
> > > Well, it's your code, but ... can I ask why?  AT&T syntax is the
> > > standard for Linux, which is in fact the OS we are developing for.
> > 
> > I agree, all assembly in Linux is AT&T, adding Intel notation only
> > serves to cause confusion.
> 
> It's not assembly. It's C code that generates binary and here
> we're talking about comments.

And comments are useless if they don't clarify. Intel syntax confuses.

> I'm sure you're not proposing to do:
> /* mov src, dst */
> #define EMIT_mov(DST, SRC)                                                               \
> right?

Which is why Josh reversed both of them. The current Intel order is just
terribly confusing. And I don't see any of the other JITs having
confusing comments like this.

> bpf_jit_comp.c stays as-is. Enough of it.

I think you're forgetting this is also arch/x86 code, and no, it needs
changes because its broken wrt unwinding.

RE: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[David Laight]

On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> thus prevents the FP unwinder from unwinding through JIT generated code.
>
> RBP is currently used as the BPF stack frame pointer register.  The
> actual register used is opaque to the user, as long as it's a
> callee-saved register.  Change it to use R12 instead.

Could you maintain the system %rbp chain through the BPF stack?
It might even be possible to put something relevant in the %rip
location.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 09:41:37AM +0200, Peter Zijlstra wrote:
> On Thu, Jun 13, 2019 at 11:00:09PM -0700, Alexei Starovoitov wrote:
> 
> > There is something wrong with
> > commit d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> 
> It assumes we can always unwind stack, which is, imo, not a weird thing.
> 
> > If I simply revert it and have CONFIG_UNWINDER_FRAME_POINTER=y
> > JITed stacks work just fine, because
> > bpf_get_stackid()->get_perf_callchain()
> > need to start unwinding before any bpf stuff.
> 
> How does stack unwinding work if we try and unwind from an interrupt
> that hits inside a BPF program? That too needs to work properly.
>
> > After that commit it needs to go through which is a bug on its own.
> > imo patch 1 doesn't really fix that issue.
> 
> This we agree on, patch 1 doesn't solve that at all. But we also should
> not loose the initial regs->ip value.
> 
> > As far as mangled rbp can we partially undo old
> > commit 177366bf7ceb ("bpf: change x86 JITed program stack layout")
> > that introduced that rbp adjustment.
> 
> > Going through bpf code is only interesting in case of panics somewhere
> > in bpf helpers. Back then we didn't even have ksym of jited code.
> 
> I disagree here, interrupts/NMIs hitting inside BPF should be able to
> reliably unwind the entire stack. Back then is irrelevant, these days we
> expect a reliable unwind.

Right.  Also, JIT code can call into C code, which could

a) try to unwind
b) call WARN()
c) hit a panic
d) get preempted by code which does any of the above
e) etc

> > Anyhow I agree that we need to make the jited frame proper,
> > but unwinding need to start before any bpf stuff.
> > That's a bigger issue.
> 
> I strongly disagree, we should be able to unwind through bpf.

-- 
Josh

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 11:00:09PM -0700, Alexei Starovoitov wrote:
> > +	if (src_reg == BPF_REG_FP) {
> > +		/*
> > +		 * If the value was copied from RBP (real frame pointer),
> > +		 * adjust it to the BPF program's frame pointer value.
> > +		 *
> > +		 * add dst, -40
> > +		 */
> > +		EMIT4(add_1mod(0x48, dst_reg), 0x83, add_1reg(0xC0, dst_reg),
> > +		      0xD8);
> > +	}
> > +
> 
> That won't work. Any register can point to a stack.

Right, but if the stack pointer comes from BPF_REG_FP then won't the
above correct it?  Then if the pointer gets passed around to other
registers it will have the correct value.  Or did I miss your point?

> The register can point to a stack of a different JITed function as well.

Do you mean tail calls?  Or something else?  For tail calls the stack is
shared and the stack layout is the same.

-- 
Josh

Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 10:50:23AM +0000, David Laight wrote:
> On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > thus prevents the FP unwinder from unwinding through JIT generated code.
> >
> > RBP is currently used as the BPF stack frame pointer register.  The
> > actual register used is opaque to the user, as long as it's a
> > callee-saved register.  Change it to use R12 instead.
> 
> Could you maintain the system %rbp chain through the BPF stack?

Do you mean to save RBP again before changing it again, so that we
create another stack frame inside the BPF stack?  That might work.

> It might even be possible to put something relevant in the %rip
> location.

I'm not sure what you mean here.

-- 
Josh

RE: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[David Laight]

From: Josh Poimboeuf
> Sent: 14 June 2019 14:44
> 
> On Fri, Jun 14, 2019 at 10:50:23AM +0000, David Laight wrote:
> > On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > > thus prevents the FP unwinder from unwinding through JIT generated code.
> > >
> > > RBP is currently used as the BPF stack frame pointer register.  The
> > > actual register used is opaque to the user, as long as it's a
> > > callee-saved register.  Change it to use R12 instead.
> >
> > Could you maintain the system %rbp chain through the BPF stack?
> 
> Do you mean to save RBP again before changing it again, so that we
> create another stack frame inside the BPF stack?  That might work.

The unwinder will (IIRC) expect *%rbp to be the previous %rbp value.
If you maintain that it will probably all work.

> > It might even be possible to put something relevant in the %rip
> > location.
> 
> I'm not sure what you mean here.

The return address is (again IIRC) %rbp[-8] so the unwinder will
expect that address to be a symbol.

I do remember a stack trace printer for x86 this didn't need
any annotation of the object code and didn't need frame pointers.
The only downside was that it had to 'guess' (ie scan the stack)
to get out of functions that couldn't return.
Basically it followed the control flow forwards tracking the
values of %sp and %bp until it found a return instuction.
All it has to do is detect loops and retry from the other
target of conditional branches.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 8/9] x86/bpf: Convert asm comments to AT&T syntax

[Song Liu]

> On Jun 14, 2019, at 12:42 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Jun 13, 2019 at 06:52:24PM +0000, Song Liu wrote:
>>> On Jun 13, 2019, at 6:21 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
>>> @@ -403,11 +403,11 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
>>> 		 * For emitting plain u32, where sign bit must not be
>>> 		 * propagated LLVM tends to load imm64 over mov32
>>> 		 * directly, so save couple of bytes by just doing
>>> -		 * 'mov %eax, imm32' instead.
>>> +		 * 'mov imm32, %eax' instead.
>>> 		 */
>>> 		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
>>> 	} else {
>>> -		/* movabsq %rax, imm64 */
>>> +		/* movabs imm64, %rax */
>> 
>> 		^^^^^ Should this be moveabsq? 
>> 
>>> 		EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
>>> 		EMIT(imm32_lo, 4);
>>> 		EMIT(imm32_hi, 4);
> 
> Song, can you please trim replies; I only found what you said because of
> Josh's reply.

Sorry for the problem. I will trim in the future. 

Song

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 1:11 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jun 14, 2019 at 12:35:38AM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote:
> > > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> > > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> > >
> > > > > and to patches 8 and 9.
> > > >
> > > > Well, it's your code, but ... can I ask why?  AT&T syntax is the
> > > > standard for Linux, which is in fact the OS we are developing for.
> > >
> > > I agree, all assembly in Linux is AT&T, adding Intel notation only
> > > serves to cause confusion.
> >
> > It's not assembly. It's C code that generates binary and here
> > we're talking about comments.
>
> And comments are useless if they don't clarify. Intel syntax confuses.
>
> > I'm sure you're not proposing to do:
> > /* mov src, dst */
> > #define EMIT_mov(DST, SRC)                                                               \
> > right?
>
> Which is why Josh reversed both of them. The current Intel order is just
> terribly confusing. And I don't see any of the other JITs having
> confusing comments like this.
>
> > bpf_jit_comp.c stays as-is. Enough of it.
>
> I think you're forgetting this is also arch/x86 code, and no, it needs
> changes because its broken wrt unwinding.

See MAINTAINERS file.
If you guys keep insisting on pointless churn like this
we'll move arch/x86/net/ into net/ where it probably belongs.
netdev has its own comment style too.
And it is also probably confusing to some folks.

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 12:41 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 13, 2019 at 11:00:09PM -0700, Alexei Starovoitov wrote:
>
> > There is something wrong with
> > commit d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
>
> It assumes we can always unwind stack, which is, imo, not a weird thing.
>
> > If I simply revert it and have CONFIG_UNWINDER_FRAME_POINTER=y
> > JITed stacks work just fine, because
> > bpf_get_stackid()->get_perf_callchain()
> > need to start unwinding before any bpf stuff.
>
> How does stack unwinding work if we try and unwind from an interrupt
> that hits inside a BPF program? That too needs to work properly.
>
> > After that commit it needs to go through which is a bug on its own.
> > imo patch 1 doesn't really fix that issue.
>
> This we agree on, patch 1 doesn't solve that at all. But we also should
> not loose the initial regs->ip value.
>
> > As far as mangled rbp can we partially undo old
> > commit 177366bf7ceb ("bpf: change x86 JITed program stack layout")
> > that introduced that rbp adjustment.
>
> > Going through bpf code is only interesting in case of panics somewhere
> > in bpf helpers. Back then we didn't even have ksym of jited code.
>
> I disagree here, interrupts/NMIs hitting inside BPF should be able to
> reliably unwind the entire stack. Back then is irrelevant, these days we
> expect a reliable unwind.
>
> > Anyhow I agree that we need to make the jited frame proper,
> > but unwinding need to start before any bpf stuff.
> > That's a bigger issue.
>
> I strongly disagree, we should be able to unwind through bpf.

I'm not saying that we should not.
I'm saying that unwinding through jited code was not working
for long time and it wasn't an issue. The issue is start of unwind.
This is user expected behavior that we cannot just break.
The users expect stack trace to be the same JITed vs interpreted.
Hence unwinder should start before interpreter/jit.
That's the first problem to address.
In parallel we can discuss how to make unwinding work through jited code.

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 6:34 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Jun 13, 2019 at 11:00:09PM -0700, Alexei Starovoitov wrote:
> > > +   if (src_reg == BPF_REG_FP) {
> > > +           /*
> > > +            * If the value was copied from RBP (real frame pointer),
> > > +            * adjust it to the BPF program's frame pointer value.
> > > +            *
> > > +            * add dst, -40
> > > +            */
> > > +           EMIT4(add_1mod(0x48, dst_reg), 0x83, add_1reg(0xC0, dst_reg),
> > > +                 0xD8);
> > > +   }
> > > +
> >
> > That won't work. Any register can point to a stack.
>
> Right, but if the stack pointer comes from BPF_REG_FP then won't the
> above correct it?  Then if the pointer gets passed around to other
> registers it will have the correct value.  Or did I miss your point?

At the beginning of the program frame pointer is bpf_reg_fp,
but later it can be in any register. It can be spilled into stack.
Some math done on it and that adjusted pointer passed into
another jited function.
It's perfectly fine for one bpf program to modify stack of
another bpf program. The verifier checks the safety bounds, etc.

Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 08:31:53AM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 6:34 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Thu, Jun 13, 2019 at 11:00:09PM -0700, Alexei Starovoitov wrote:
> > > > +   if (src_reg == BPF_REG_FP) {
> > > > +           /*
> > > > +            * If the value was copied from RBP (real frame pointer),
> > > > +            * adjust it to the BPF program's frame pointer value.
> > > > +            *
> > > > +            * add dst, -40
> > > > +            */
> > > > +           EMIT4(add_1mod(0x48, dst_reg), 0x83, add_1reg(0xC0, dst_reg),
> > > > +                 0xD8);
> > > > +   }
> > > > +
> > >
> > > That won't work. Any register can point to a stack.
> >
> > Right, but if the stack pointer comes from BPF_REG_FP then won't the
> > above correct it?  Then if the pointer gets passed around to other
> > registers it will have the correct value.  Or did I miss your point?
> 
> At the beginning of the program frame pointer is bpf_reg_fp,
> but later it can be in any register. It can be spilled into stack.
> Some math done on it and that adjusted pointer passed into
> another jited function.
> It's perfectly fine for one bpf program to modify stack of
> another bpf program. The verifier checks the safety bounds, etc.

I still don't get what you're saying.  The above patch attempted to
cover all those scenarios by always subtracting an offset from all movs
and stack accesses relating to BPF_REG_FP.  It might be missing a case
or two but it seems like it should work.  From the program's point of
view, BPF_REG_FP should always show the right value no matter where it
gets moved to.

But anyway, David L's nested frame idea might be a much simpler change.
I'll look at that.

-- 
Josh

Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 08:13:49AM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 1:11 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Fri, Jun 14, 2019 at 12:35:38AM -0700, Alexei Starovoitov wrote:
> > > On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote:
> > > > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote:
> > > > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> > > >
> > > > > > and to patches 8 and 9.
> > > > >
> > > > > Well, it's your code, but ... can I ask why?  AT&T syntax is the
> > > > > standard for Linux, which is in fact the OS we are developing for.
> > > >
> > > > I agree, all assembly in Linux is AT&T, adding Intel notation only
> > > > serves to cause confusion.
> > >
> > > It's not assembly. It's C code that generates binary and here
> > > we're talking about comments.
> >
> > And comments are useless if they don't clarify. Intel syntax confuses.
> >
> > > I'm sure you're not proposing to do:
> > > /* mov src, dst */
> > > #define EMIT_mov(DST, SRC)                                                               \
> > > right?
> >
> > Which is why Josh reversed both of them. The current Intel order is just
> > terribly confusing. And I don't see any of the other JITs having
> > confusing comments like this.
> >
> > > bpf_jit_comp.c stays as-is. Enough of it.
> >
> > I think you're forgetting this is also arch/x86 code, and no, it needs
> > changes because its broken wrt unwinding.
> 
> See MAINTAINERS file.
> If you guys keep insisting on pointless churn like this
> we'll move arch/x86/net/ into net/ where it probably belongs.
> netdev has its own comment style too.
> And it is also probably confusing to some folks.

So if I understand correctly, you're proposing that we move x86-specific
code to net/arch/x86 so you don't have to make your code readable to
others and adhere to kernel style guidelines?

-- 
Josh

Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 01:58:21PM +0000, David Laight wrote:
> From: Josh Poimboeuf
> > Sent: 14 June 2019 14:44
> > 
> > On Fri, Jun 14, 2019 at 10:50:23AM +0000, David Laight wrote:
> > > On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > > > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > > > thus prevents the FP unwinder from unwinding through JIT generated code.
> > > >
> > > > RBP is currently used as the BPF stack frame pointer register.  The
> > > > actual register used is opaque to the user, as long as it's a
> > > > callee-saved register.  Change it to use R12 instead.
> > >
> > > Could you maintain the system %rbp chain through the BPF stack?
> > 
> > Do you mean to save RBP again before changing it again, so that we
> > create another stack frame inside the BPF stack?  That might work.
> 
> The unwinder will (IIRC) expect *%rbp to be the previous %rbp value.
> If you maintain that it will probably all work.
> 
> > > It might even be possible to put something relevant in the %rip
> > > location.
> > 
> > I'm not sure what you mean here.
> 
> The return address is (again IIRC) %rbp[-8] so the unwinder will
> expect that address to be a symbol.

Ah, gotcha.  We don't necessarily need the real rip on the stack as the
unwinder can handle bad text addresses ok.  Though the real one would be
better.

> I do remember a stack trace printer for x86 this didn't need
> any annotation of the object code and didn't need frame pointers.
> The only downside was that it had to 'guess' (ie scan the stack)
> to get out of functions that couldn't return.
> Basically it followed the control flow forwards tracking the
> values of %sp and %bp until it found a return instuction.
> All it has to do is detect loops and retry from the other
> target of conditional branches.

That actually sounds kind of cool, though I don't think we need that for
the kernel.

Anyway here's a patch with your suggestion.  I think it's the best idea
so far because it doesn't require the use of R12, nor does it require
abstracting BPF_REG_FP with an offset.  And the diffstat is pretty
small and self-contained.

It seems to work, though I didn't put a real RIP on the stack yet.  This
is based on top of the "x86/bpf: Simplify prologue generation" patch.

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 485692d4b163..fa1fe65c4cb4 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -186,7 +186,7 @@ struct jit_context {
 #define BPF_MAX_INSN_SIZE	128
 #define BPF_INSN_SAFETY		64
 
-#define PROLOGUE_SIZE		20
+#define PROLOGUE_SIZE		24
 
 /*
  * Emit x86-64 prologue code for BPF program and check its size.
@@ -197,14 +197,17 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
+	/* push rbp */
+	EMIT1(0x55);
+	/* mov rbp, rsp */
+	EMIT3(0x48, 0x89, 0xE5);
+
 	/* push r15 */
 	EMIT2(0x41, 0x57);
 	/* push r14 */
 	EMIT2(0x41, 0x56);
 	/* push r13 */
 	EMIT2(0x41, 0x55);
-	/* push rbp */
-	EMIT1(0x55);
 	/* push rbx */
 	EMIT1(0x53);
 
@@ -218,10 +221,13 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 
 	/*
 	 * RBP is used for the BPF program's FP register.  It points to the end
-	 * of the program's stack area.
-	 *
-	 * mov rbp, rsp
+	 * of the program's stack area.  Create another stack frame so the
+	 * unwinder can unwind through the generated code.  The tail_call_cnt
+	 * value doubles as an (invalid) RIP address.
 	 */
+	/* push rbp */
+	EMIT1(0x55);
+	/* mov rbp, rsp */
 	EMIT3(0x48, 0x89, 0xE5);
 
 	/* sub rsp, rounded_stack_depth */
@@ -237,19 +243,21 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* lea rsp, [rbp+0x8] */
-	EMIT4(0x48, 0x8D, 0x65, 0x08);
+	/* leave (restore rsp and rbp) */
+	EMIT1(0xC9);
+	/* pop rbx (skip over tail_call_cnt) */
+	EMIT1(0x5B);
 
 	/* pop rbx */
 	EMIT1(0x5B);
-	/* pop rbp */
-	EMIT1(0x5D);
 	/* pop r13 */
 	EMIT2(0x41, 0x5D);
 	/* pop r14 */
 	EMIT2(0x41, 0x5E);
 	/* pop r15 */
 	EMIT2(0x41, 0x5F);
+	/* pop rbp */
+	EMIT1(0x5D);
 
 	/* ret */
 	EMIT1(0xC3);
@@ -298,13 +306,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT3(0x8B, 0x45, 0x04);                  /* mov eax, dword ptr [rbp + 4] */
+	EMIT3(0x8B, 0x45, 0x0C);                  /* mov eax, dword ptr [rbp + 12] */
 	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
 #define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT3(0x89, 0x45, 0x04);                  /* mov dword ptr [rbp + 4], eax */
+	EMIT3(0x89, 0x45, 0x0C);                  /* mov dword ptr [rbp + 12], eax */
 
 	/* prog = array->ptrs[index]; */
 	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */

RE: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[David Laight]

From: Josh Poimboeuf
> Sent: 14 June 2019 18:07
...
> > I do remember a stack trace printer for x86 this didn't need
> > any annotation of the object code and didn't need frame pointers.
> > The only downside was that it had to 'guess' (ie scan the stack)
> > to get out of functions that couldn't return.
> > Basically it followed the control flow forwards tracking the
> > values of %sp and %bp until it found a return instuction.
> > All it has to do is detect loops and retry from the other
> > target of conditional branches.
> 
> That actually sounds kind of cool, though I don't think we need that for
> the kernel.

No reason why not.
It would save most of the instrumentation the orc unwinder needs.
It isn't as though the performance of kernel tracebacks is that
important (printf and symbol lookup probably always dominates).
The only difficulty for x86 is quickly determining the size of
instructions.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)