Re: [PATCH bpf-next v6 3/9] bpf, x86: refactor BPF_ST management in do_jit

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3415 bytes --]

> 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