> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 01e7ce569c1ed..0b8b5dfe37ab0 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1315,6 +1315,64 @@ static void emit_st_index(u8 **pprog, u32 size, u32
> dst_reg, u32 index_reg, int
> *pprog = prog;
> }
>
> +static void emit_st(u8 **pprog, struct bpf_insn *insn, int dst_reg,
> + s32 outgoing_arg_base, u16 outgoing_rsp)
This isn't a bug, but any reason for `int dst_reg` here rather than the
`u32 dst_reg` used by emit_stx()/emit_st_index() alongside it?
> +{
> + s32 imm32 = insn->imm;
> + u8 *prog = *pprog;
> + s32 insn_off;
> +
> + switch (BPF_SIZE(insn->code)) {
> + case BPF_B:
> + if (is_ereg(dst_reg))
> + EMIT2(0x41, 0xC6);
> + else
> + EMIT1(0xC6);
> + break;
> + case BPF_H:
> + if (is_ereg(dst_reg))
> + EMIT3(0x66, 0x41, 0xC7);
> + else
> + EMIT2(0x66, 0xC7);
> + break;
> + case BPF_W:
> + if (is_ereg(dst_reg))
> + EMIT2(0x41, 0xC7);
> + else
> + EMIT1(0xC7);
> + break;
> + case BPF_DW:
> + if (dst_reg == BPF_REG_PARAMS && insn->off == -8) {
> + /* Arg 6: store immediate in r9 register */
> + emit_mov_imm64(&prog, X86_REG_R9, imm32 >> 31,
> + (u32)imm32);
This isn't a bug, but since these two lines now sit two levels shallower
and fit in 80 columns unwrapped, could they stay on one line to keep the
diff a pure code move?
The call unwrapped is:
emit_mov_imm64(&prog, X86_REG_R9, imm32 >> 31, (u32)imm32);
which is 75 columns at the new indentation level.
> + *pprog = prog;
> + return;
> + }
> + EMIT2(add_1mod(0x48, dst_reg), 0xC7);
> + break;
> + }
> +
> + insn_off = insn->off;
> + if (dst_reg == BPF_REG_PARAMS) {
> + /*
> + * Args 7+: reverse BPF negative offsets to
> + * x86 positive rsp offsets.
> + * BPF off=-16 → [rsp+0], off=-24 → [rsp+8], ...
> + */
> + insn_off = outgoing_arg_base - outgoing_rsp -
> + insn_off - 16;
This isn't a bug, but this line also became shorter by two indentation
levels. Unwrapped it is:
insn_off = outgoing_arg_base - outgoing_rsp - insn_off - 16;
which is 76 columns. Could it stay on one line rather than splitting the
arithmetic expression mid-subtraction?
> + dst_reg = BPF_REG_FP;
> + }
> + if (is_imm8(insn_off))
> + EMIT2(add_1reg(0x40, dst_reg), insn_off);
> + else
> + EMIT1_off32(add_1reg(0x80, dst_reg), insn_off);
> +
> + EMIT(imm32, bpf_size_to_x86_bytes(BPF_SIZE(insn->code)));
> + *pprog = prog;
> +}
This isn't a bug, but would it be worth carrying the `/* ST: *(u8*)(dst_reg
+ off) = imm */` line over onto the new emit_st(), so it matches
emit_ldx()/emit_stx()/emit_st_index() just above?
Looking at the neighbouring base emit helpers in this file, they all carry
that one-line semantic summary: emit_ldx has `/* LDX: dst_reg = *(u8*)(src_reg
+ off) */`, emit_ldsx, emit_stx, and emit_st_index has `/* ST: *(u8*)(dst_reg +
index_reg + off) = imm32 */`. The hunk in do_jit deletes the `/* ST:
*(u8*)(dst_reg + off) = imm */` one-liner that documented the BPF_ST arm, and
the new emit_st() helper is added without an equivalent, making emit_st the only
one of that group with no such line.
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30936192563
