Re: [PATCH bpf-next 2/5] bpf, x86: Extract the i386 JIT load emitter

[bot+bpf-ci@kernel.org]

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

> bpf, x86: Extract the i386 JIT load emitter
>
> Move ordinary LDX lowering into an instruction-owned helper. Keep address
> staging, displacement encoding and register-pair completion together,
> including verifier-managed zero extension for narrow loads.
>
> Use the helper for the existing BPF_MEM cases before adding signed loads.
> This separates the code movement from the MEMSX instruction handling.
> The emitted instructions are preserved.

This isn't a bug, but could the middle paragraph be trimmed?  It
enumerates what the moved block contains rather than adding rationale
beyond what the diff already shows.  Would it read more clearly if the
changelog led with why the helper is being split out ahead of the MEMSX
patch, and used "No functional change intended" instead of "The emitted
instructions are preserved"?

> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index 9cbeabac4f27f..6839c233cb734 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c

[ ... ]

> @@ -1214,6 +1214,89 @@ static inline void emit_ia32_mul_i64(const u8 dst[], const u32 val,
>  	*pprog = prog;
>  }
>
> +static void emit_ia32_ldx(const struct bpf_insn *insn, u8 **pprog,
> +			  const struct bpf_prog_aux *aux)
> +{
> +	const u8 *dst = bpf2ia32[insn->dst_reg];
> +	const u8 *src = bpf2ia32[insn->src_reg];
> +	bool dstk = insn->dst_reg != BPF_REG_AX;
> +	bool sstk = insn->src_reg != BPF_REG_AX;
> +	u8 *prog = *pprog;
> +	int cnt = 0;
> +
> +	/* Stage the address in EAX; dst may alias src. */
> +	if (sstk)
> +		/* mov eax,dword ptr [ebp+off] */
> +		EMIT3(0x8B, add_2reg(0x40, IA32_EBP, IA32_EAX),
> +		      STACK_VAR(src_lo));
> +	else
> +		/* mov eax,src_lo */
> +		EMIT2(0x8B, add_2reg(0xC0, src_lo, IA32_EAX));
> +
> +	switch (BPF_SIZE(insn->code)) {
> +	case BPF_B:
> +		/* movzx edx,byte ptr [eax+off] */
> +		EMIT2(0x0F, 0xB6);
> +		break;
> +	case BPF_H:
> +		/* movzx edx,word ptr [eax+off] */
> +		EMIT2(0x0F, 0xB7);
> +		break;
> +	case BPF_W:
> +	case BPF_DW:
> +		/* mov edx,dword ptr [eax+off] */
> +		EMIT1(0x8B);
> +		break;
> +	}

This isn't a bug, but since the patch is meant to be a pure move, would
it read more easily as a verbatim move plus a separate follow-up for the
cosmetic changes?  The moved block is reworked here: the three case arms
are reflowed from the original one-line form, EMIT(0x8B, 1) becomes
EMIT1(0x8B), several EMITs gain new mnemonic comments, and the
pre-existing comment is corrected from "mov eax,dword ptr [ebp+off]" to
"mov eax,src_lo".  Each edit is an improvement, but folding them into
the movement makes "emission is unchanged" harder to confirm by reading
the diff.

[ ... ]


---
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/34921540254