Re: [PATCH bpf-next v3] m68k, bpf: Add initial BPF JIT compiler support

[bot+bpf-ci@kernel.org]

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

> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2040011a386..261cc1124685 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4699,6 +4699,12 @@ L:	bpf@vger.kernel.org
>  S:	Maintained
>  F:	arch/loongarch/net/
>
> +BPF JIT for M68K

This isn't a bug, but the surrounding BPF JIT entries all use lowercase
'for': 'BPF JIT for ARM', 'BPF JIT for ARM64', 'BPF JIT for LOONGARCH',
'BPF JIT for MIPS', etc. For consistency, should this be
'BPF JIT for M68K'?

Reference: https://lore.kernel.org/bpf/20260429133910.6F6DCC19425@smtp.kernel.org/

> +M:	Kuan-Wei Chiu <visitorckw@gmail.com>
> +L:	bpf@vger.kernel.org
> +S:	Maintained
> +F:	arch/m68k/net/
> +
>  BPF JIT for MIPS (32-BIT AND 64-BIT)
>  M:	Johan Almbladh <johan.almbladh@anyfinetworks.com>
>  M:	Paul Burton <paulburton@kernel.org>

[ ... ]

> diff --git a/arch/m68k/net/bpf_jit_comp.c b/arch/m68k/net/bpf_jit_comp.c
> new file mode 100644
> index 000000000000..1531b321410a
> --- /dev/null
> +++ b/arch/m68k/net/bpf_jit_comp.c

[ ... ]

> +static int emit_call(const struct bpf_insn *insn, struct jit_ctx *ctx)
> +{
> +	const s8 arg_regs[] = { BPF_REG_5, BPF_REG_4, BPF_REG_3, BPF_REG_2, BPF_REG_1 };
> +	const s8 *tmp1 = bpf2m68k[TMP_REG_1];
> +	bool extra_pass = ctx->target != NULL;
> +	u64 func_addr;
> +	bool fixed;
> +	int i, err;
> +
> +	err = bpf_jit_get_func_addr(ctx->prog, insn, extra_pass, &func_addr, &fixed);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < 5; i++) {
> +		const s8 *reg = bpf2m68k[arg_regs[i]];
> +		s8 d_lo = bpf_get_reg32(reg[1], tmp1[1], ctx);
> +		s8 d_hi = bpf_get_reg32(reg[0], tmp1[0], ctx);
> +
> +		emit_16(ctx, 0x2f00 | d_lo);			/* move.l d_lo, -(%sp) */
> +		emit_16(ctx, 0x2f00 | d_hi);			/* move.l d_hi, -(%sp) */
> +	}
> +
> +	emit_16(ctx, 0x207c);					/* movea.l #func_addr, %a0 */
> +	emit_32(ctx, (u32)func_addr);
> +
> +	emit_16(ctx, 0x4e90);					/* jsr (%a0) */
> +
> +	emit_16(ctx, 0x4fef);					/* lea 40(%sp), %sp */
> +	emit_16(ctx, 40);
> +
> +	emit_16(ctx, 0xc340);					/* exg %d0, %d1 */
> +
> +	return 0;
> +}

Does the exg %d0, %d1 swap the halves of the return value incorrectly
for BPF-to-BPF subprogram calls (BPF_PSEUDO_CALL)?

The emit_call() function unconditionally swaps %d0 and %d1 after all
calls, which appears correct for C ABI helper functions (where high 32
bits are in %d0, low in %d1). However, BPF subprograms place their 64-bit
return value in BPF_REG_0, which is mapped to %d1 and %d0.

If a BPF subprogram returns a value via BPF_REG_0:
    BPF subprogram: BPF_REG_0 -> %d1:%d0 (high:low)
    After exg:      %d0:%d1 (swapped - incorrect ordering)

Additionally, the prologue assumes C ABI and reads a context pointer
from 8(%fp):

arch/m68k/net/bpf_jit_comp.c:build_prologue() {
    emit_16(ctx, 0x202e);       /* move.l 8(%fp), %d0 */
    emit_16(ctx, 8);
}

But BPF-to-BPF calls pass arguments via the stack (pushed by emit_call),
not via the C calling convention. Wouldn't this cause the prologue to
read an incorrect context pointer for BPF subprogram entry points?

Reference: https://lore.kernel.org/bpf/6736ffb5.050a0220.11da83.0029.GAE@google.com/


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