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

On Wed, Apr 29, 2026 at 01:39:10PM +0000, [email protected] wrote:
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 13f49378b157..9d6e73c1caaf 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4699,6 +4699,12 @@ L:	[email protected]
> >  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'.

I doubt anyone really cares about the casing here, but I will fix it in
v2 anyway.

> 
> > +M:	Kuan-Wei Chiu <[email protected]>
> > +L:	[email protected]
> > +S:	Maintained
> > +F:	arch/m68k/net/
> > +
> >  BPF JIT for MIPS (32-BIT AND 64-BIT)
> >  M:	Johan Almbladh <[email protected]>
> >  M:	Paul Burton <[email protected]>
> 
> [ ... ]
> 
> > 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.

Ah, I actually had a local fixup for this but forgot to squash it
before sending.

Out of curiosity, is there any checker tool that warns about
referencing non-existent symbols?

> 
> >  	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).

Ack. Will fix the comment in v2.

> 
> > +		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.

Ack. Will change the mask to 0x3c00.

Regards,
Kuan-Wei

> 
> > +	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