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

[bot+bpf-ci]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 13f49378b157..9d6e73c1caaf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4699,6 +4699,12 @@ L:	bpf@xxxxxxxxxxxxxxx
>  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, this should be 'BPF JIT for
M68K'.

> +M:	Kuan-Wei Chiu <visitorckw@xxxxxxxxx>
> +L:	bpf@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	arch/m68k/net/
> +
>  BPF JIT for MIPS (32-BIT AND 64-BIT)
>  M:	Johan Almbladh <johan.almbladh@xxxxxxxxxxxxxxxxx>
>  M:	Paul Burton <paulburton@xxxxxxxxxx>

[ ... ]

> diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
> index 11835eb59d94..67ac01c9ab42 100644
> --- a/arch/m68k/Kconfig
> +++ b/arch/m68k/Kconfig
> @@ -8,6 +8,7 @@ config M68K
>  	select ARCH_HAS_CPU_FINALIZE_INIT if MMU
>  	select ARCH_HAS_CURRENT_STACK_POINTER
>  	select ARCH_HAS_DMA_PREP_COHERENT if M68K_NONCOHERENT_DMA && !COLDFIRE
> +	select HAVE_EBPF_JIT if (!COLDFIRE && !M68000 && !M68010)
                                                          ^^^^^^^

Does this reference a valid Kconfig symbol? The symbol M68010 doesn't
appear to be defined anywhere in the kernel tree. A search across
arch/m68k/ and the wider tree shows M68010 is referenced only in this
new line. The m68k CPU Kconfig (arch/m68k/Kconfig.cpu) defines
COLDFIRE, M68000, M68020, M68030, M68040, M68060, SUN3, M68KCLASSIC,
etc., but no M68010.

Because undefined Kconfig symbols evaluate to 'n', the condition
!M68010 is always true. This means the gating for 68010 hardware that
the commit message describes is never actually enforced (dead
sub-expression).

The commit message states the JIT targets 'm68020+ processors...
excludes 68000/68010 and coldfire', so the intent is clearly to exclude
68010 - but that exclusion is not functional as written.

>  	select ARCH_HAS_SYNC_DMA_FOR_DEVICE if M68K_NONCOHERENT_DMA
>  	select ARCH_HAVE_NMI_SAFE_CMPXCHG if RMW_INSNS
>  	select ARCH_MIGHT_HAVE_PC_PARPORT if ISA

[ ... ]

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

[ ... ]

> @@ -0,0 +1,1543 @@

[ ... ]

> +static void emit_alu64_x(const struct bpf_insn *insn, struct jit_ctx *ctx)
> +{

[ ... ]

> +	case BPF_XOR:
> +		emit_16(ctx, 0xb180 | (s_lo << 9) | d_lo);		/* eor.l d_lo, s_lo */
> +		emit_16(ctx, 0xb180 | (s_hi << 9) | d_hi);		/* eor.l d_hi, s_hi */
                                                                       ^^^^^^^^^^^

The comments describe the operand order in reverse. The m68k 'eor.l Dn,
<ea>' opcode 0xB180 has Dn (bits 11-9) encode the source register and
EA (bits 2-0) encode the destination (EA = EA XOR Dn). The emitted
instruction is:

  0xb180 | (s_lo << 9) | d_lo    /* d_lo := d_lo ^ s_lo */

which is functionally correct. However, the comment reads 'eor.l d_lo,
s_lo', which in m68k assembler convention (source first) describes the
wrong operation: s_lo := d_lo ^ s_lo.

This is inconsistent with the correctly-worded sibling comment in
emit_alu32_x:

  emit_16(ctx, 0xb180 | (s_reg << 9) | d_reg);  /* eor.l src, dst */

The emitted machine code is correct; only the comments are reversed.
This also affects the BPF_XOR case in emit_alu64_k (four lines total).

> +		break;
> +	}
> +
> +	bpf_put_reg32(dst[1], d_lo, ctx);
> +	bpf_put_reg32(dst[0], d_hi, ctx);
> +}

[ ... ]

> +static void build_prologue(struct jit_ctx *ctx)
> +{
> +	int bpf_stack = 512;
> +	int total_stack = SCRATCH_SIZE + bpf_stack;
> +
> +	emit_16(ctx, 0x7021);					/* moveq #33, %d0 */
> +
> +	emit_16(ctx, 0x4e56);					/* link %a6, #-total_stack */
> +	emit_16(ctx, -total_stack);
> +
> +	emit_16(ctx, 0x48e7);					/* movem.l d2-d5, -(%sp) */
                                                                  ^^^^^^

Does this comment accurately describe the register mask 0x3f3c? The
mask 0x3f3c in predecrement mode (bits 2-5 and 8-13 set) encodes the
register set {D2, D3, D4, D5, D6, D7, A2, A3, A4, A5} - that's 10
registers, not just d2-d5.

Predecrement register mask encoding uses bit 0=A7, bit 1=A6, ..., bit
7=A0, bit 8=D7, bit 9=D6, ..., bit 15=D0. A true 'movem.l d2-d5,
-(%sp)' would use mask 0x3c00.

The save matches the corresponding epilogue restore (mask 0x3cfc, which
in postincrement mode covers the same 10 registers), so the code is
functionally correct and the stack is balanced. However, D6, D7, A2-A5
are never modified by the JIT and do not need to be saved, causing 24
bytes of wasted stack space and 6 unnecessary register saves per JIT'd
function call.

> +	emit_16(ctx, 0x3f3c);
> +
> +	emit_16(ctx, 0x2d40);					/* move.l %d0, off(%fp) */
> +	emit_16(ctx, (u16)STACK_OFFSET(BPF_TC_LO));

[ ... ]

> +static void build_epilogue(struct jit_ctx *ctx)
> +{
> +	emit_16(ctx, 0x4cdf);					/* movem.l (%sp)+, d2-d5 */
                                                                  ^^^^^^

Similar to the prologue comment issue, the mask 0x3cfc in postincrement
mode (bits 2-7 and 10-13 set) encodes restoring {D2, D3, D4, D5, D6,
D7, A2, A3, A4, A5}. A true 'movem.l (%sp)+, d2-d5' would use mask
0x003c.

Postincrement mask encoding uses bit 0=D0, bit 1=D1, ..., bit 7=D7, bit
8=A0, ..., bit 15=A7. This matches the prologue's save, so the stack is
balanced and values are correctly restored, but the comment is
inaccurate.

> +	emit_16(ctx, 0x3cfc);
> +
> +	emit_16(ctx, 0x4e5e);					/* unlk %fp */
> +	emit_16(ctx, 0x4e75);					/* rts */
> +}


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