From: Alexei Starovoitov x86 has no fetching AND/OR/XOR. Thus the JIT emits a loop with cmpxchg. cmpxchg uses RAX, and RAX holds BPF R0. Thus the JIT saves R0 before the loop. prog the address is in the JIT saves R0 in -------------------------- ------------------ ------------------- without the flag a BPF register BPF_REG_AX with BPF_F_ARENA_SCALAR BPF_REG_AX the stack The flag is removed. Remove the second variant. Signed-off-by: Alexei Starovoitov --- arch/x86/net/bpf_jit_comp.c | 58 +++++++++---------------------------- 1 file changed, 14 insertions(+), 44 deletions(-) diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index 7bfb7210484e..d90b6d238b3a 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -1745,26 +1745,20 @@ static int emit_atomic_ld_st_index(u8 **pprog, u32 atomic_op, u32 size, /* * A fetching AND/OR/XOR can't be implemented with a single x86 insn, so do a * CMPXCHG loop. @index_reg is X86_REG_R12 for an arena access or -1 otherwise. - * Unless @r0_on_stack, the caller has substituted BPF_REG_AX for R0 in - * @dst_reg and @src_reg. + * The caller has saved R0 in BPF_REG_AX and has substituted BPF_REG_AX for R0 + * in @dst_reg and @src_reg. * * Both the load and the CMPXCHG can fault when accessing the arena, so their * addresses are handed back in @fault[0] and @fault[1]. A fault has to resume * at @resume, which is past the loop and past the move that delivers the old * value, but before R0 is restored. Both are NULL for the non-arena case, * which needs no fixup. - * - * R0 is expected to be saved in BPF_REG_AX by the caller, or pushed on the - * stack if @r0_on_stack. In that case an R0 operand stays in the stack slot, - * RAX ends up holding the old value that R0 is to return, and the slot is - * dropped rather than popped. */ static int emit_atomic_fetch_rmw(u8 **pprog, struct bpf_insn *insn, u32 dst_reg, - u32 src_reg, int index_reg, bool r0_on_stack, + u32 src_reg, int index_reg, u8 **fault, u8 **resume) { bool is64 = BPF_SIZE(insn->code) == BPF_DW; - bool operand_on_stack = r0_on_stack && src_reg == BPF_REG_0; u8 *branch_target, *prog = *pprog; int err; @@ -1783,15 +1777,7 @@ static int emit_atomic_fetch_rmw(u8 **pprog, struct bpf_insn *insn, u32 dst_reg, * Perform the (commutative) operation locally, put the result in * the AUX_REG. */ - if (operand_on_stack) { - /* mov r11, [rsp]: the operand is the R0 saved there, RAX is the old value */ - if (is64) - EMIT4(0x4C, 0x8B, 0x1C, 0x24); - else - EMIT4(0x44, 0x8B, 0x1C, 0x24); - } else { - emit_mov_reg(&prog, is64, AUX_REG, BPF_REG_0); - } + emit_mov_reg(&prog, is64, AUX_REG, BPF_REG_0); maybe_emit_mod(&prog, AUX_REG, src_reg, is64); EMIT2(simple_alu_opcodes[BPF_OP(insn->imm)], add_2reg(0xC0, AUX_REG, src_reg)); @@ -1810,20 +1796,14 @@ static int emit_atomic_fetch_rmw(u8 **pprog, struct bpf_insn *insn, u32 dst_reg, /* ZF tells us whether we won the race. If it's cleared we need to try again. */ EMIT2(X86_JNE, -(prog - branch_target) - 2); - /* Return the pre-modification value, which RAX already is for R0 */ - if (!operand_on_stack) - emit_mov_reg(&prog, is64, src_reg, BPF_REG_0); + /* Return the pre-modification value */ + emit_mov_reg(&prog, is64, src_reg, BPF_REG_0); if (resume) *resume = prog; /* Restore R0 after clobbering RAX */ - if (!r0_on_stack) - emit_mov_reg(&prog, true, BPF_REG_0, BPF_REG_AX); - else if (operand_on_stack) - EMIT2(0x41, 0x5B); /* pop r11, dropping the operand */ - else - EMIT1(0x58); /* pop rax */ + emit_mov_reg(&prog, true, BPF_REG_0, BPF_REG_AX); *pprog = prog; return 0; @@ -3354,7 +3334,7 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int * } if (is_atomic_fetch) { err = emit_atomic_fetch_rmw(&prog, insn, real_dst_reg, - real_src_reg, -1, false, + real_src_reg, -1, NULL, NULL); if (err) return err; @@ -3383,29 +3363,19 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int * case BPF_STX | BPF_PROBE_ATOMIC | BPF_DW: if (is_atomic_fetch_op(insn)) { u32 real_src_reg = src_reg, real_dst_reg = dst_reg; - /* - * An access through a number has its address in - * BPF_REG_AX (see arena_scalar_access()), so R0 is - * saved on the stack instead, as div and mod do. - */ - bool r0_on_stack = dst_reg == BPF_REG_AX; u8 *fault[2], *resume; int j; /* Will need RAX as a CMPXCHG operand so save R0 */ - if (r0_on_stack) { - EMIT1(0x50); /* push rax */ - } else { - emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0); - if (src_reg == BPF_REG_0) - real_src_reg = BPF_REG_AX; - if (dst_reg == BPF_REG_0) - real_dst_reg = BPF_REG_AX; - } + emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0); + if (src_reg == BPF_REG_0) + real_src_reg = BPF_REG_AX; + if (dst_reg == BPF_REG_0) + real_dst_reg = BPF_REG_AX; err = emit_atomic_fetch_rmw(&prog, insn, real_dst_reg, real_src_reg, X86_REG_R12, - r0_on_stack, fault, &resume); + fault, &resume); if (err) return err; -- 2.55.0