From: Kuan-Wei Chiu <visitorckw@gmail.com>
To: Greg Ungerer <gerg@linux-m68k.org>
Cc: bot+bpf-ci@kernel.org, corbet@lwn.net, skhan@linuxfoundation.org,
geert@linux-m68k.org, ast@kernel.org, daniel@iogearbox.net,
eddyz87@gmail.com, memxor@gmail.com, andrii@kernel.org,
rdunlap@infradead.org, martin.lau@linux.dev, song@kernel.org,
yonghong.song@linux.dev, jolsa@kernel.org, emil@etsalapatis.com,
ihor.solodrai@linux.dev, jserv@ccns.ncku.edu.tw,
marscheng@google.com, eleanor15x@gmail.com,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-m68k@lists.linux-m68k.org, bpf@vger.kernel.org,
daniel@thingy.jp, martin.lau@kernel.org, mason@kernel.org
Subject: Re: [PATCH v7] m68k, bpf: Add initial BPF JIT compiler support
Date: Sat, 10 Oct 2026 00:32:51 +0800 [thread overview]
Message-ID: <askXM-fN-olasi7l@google.com> (raw)
In-Reply-To: <342ca584-c4ef-4d73-9017-9d21bbf9ce10@linux-m68k.org>
On Tue, Oct 06, 2026 at 11:53:22PM +1000, Greg Ungerer wrote:
> Why thank you LLM review :-)
>
> On 6/10/26 05:48, bot+bpf-ci@kernel.org wrote:
> >> m68k, bpf: Add initial BPF JIT compiler support
> >
> > TL;DR: Nit only, not a bug: the ColdFire byte-swap instruction sequences are
> > open-coded repeatedly in emit_to_le16/32/64() in the m68k JIT; suggests small
> > shared helpers, as arm's bpf_jit_32.c does.
> >
> >> diff --git a/arch/m68k/net/bpf_jit_comp.c b/arch/m68k/net/bpf_jit_comp.c
> >> new file mode 100644
> >> index 000000000000..6c40d358acd4
> >> --- /dev/null
> >> +++ b/arch/m68k/net/bpf_jit_comp.c
> >> @@ -0,0 +1,1840 @@
> > [ ... ]
> >> +static inline void emit_to_le16(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> >> +{
> >> + if (CPU_IS_COLDFIRE) {
> >> + emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> >> + emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> >> + emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> >> + emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> >> + emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> > [ ... ]
> >> +static inline void emit_to_le32(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> >> +{
> >> + if (CPU_IS_COLDFIRE) {
> >> + emit_16(ctx, 0x2f00 | d_lo); /* move.l d_lo, -(%sp) */
> >> + emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> >> + emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> >> + emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> >> + emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> >> + emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> >> + emit_16(ctx, 0x8080 | (d_hi << 9) | d_lo); /* or.l d_lo, d_hi */
> >> + emit_16(ctx, 0x2017 | (d_lo << 9)); /* move.l (%sp), d_lo */
> >> + emit_16(ctx, 0x2e80 | d_hi); /* move.l d_hi, (%sp) */
> >> + emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> >> + emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> >> + emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> >> + emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> >> + emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> >> + emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> >> + emit_16(ctx, 0x809f | (d_lo << 9)); /* or.l (%sp)+, d_lo */
> > [ ... ]
> >> +static inline void emit_to_le64(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> >> +{
> >> + if (CPU_IS_COLDFIRE) {
> >> + 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, 0x4840 | d_lo); /* swap d_lo */
> >> + emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> >> + emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> >> + emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> >> + emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> >> + emit_16(ctx, 0x8080 | (d_hi << 9) | d_lo); /* or.l d_lo, d_hi */
> >> + emit_16(ctx, 0x202f | (d_lo << 9)); /* move.l 4(%sp), d_lo */
> >> + emit_16(ctx, 0x0004);
> >> + emit_16(ctx, 0x2f40 | d_hi); /* move.l d_hi, 4(%sp) */
> >> + emit_16(ctx, 0x0004);
> >> + emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> >> + emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> >> + emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> > [ ... ]
> >
> > This isn't a bug, but the ColdFire byte-swap sequence (mvz.w / mvz.b /
> > lsr.l #8 / lsl.l #8 / or.l) is open-coded once in emit_to_le16(), twice in
> > emit_to_le32() and four times in emit_to_le64(), differing only in which
> > register receives the result.
>
> I did consider using helpers for these during coding, but ultimately decided
> against it. The open coded versions allow for a handful of extra instruction
> optimizations - due to that flexibility of producing the 16bit swap sequence
> result in different registers. For example I could combine a stack pop with
> the or'ing of the result, and more efficiently store an intermediate result
> into the temporary stack storage.
>
> The patch below gives an example of an implementation using helpers. I am not tied
> to the open coded version: Kuan-Wei if you prefer the code with helpers feel free
> to use this instead.
I don't have a strong opinion either way, since it sounds like you
already made a deliberate judgment call when writing it. Maybe I can
switch to the helper version for now to make this already large patch a
bit smaller. If anyone cares or if it ever shows up in benchmarks in
the future, we can always go back to the open-coded version to optimize
it.
>
> FWIW, the to_le32 coded sequence is 1 instruction longer (17 instructions to 18).
> The to_le64 coded sequence is 5 instructions longer (34 instructions to 39).
> Total byte count differs less, due to use of offsets in the open coded versions.
>
> Regards
> Greg
>
>
>
> --- arch/m68k/net/bpf_jit_comp.c.org 2026-10-06 23:08:25.924094287 +1000
> +++ arch/m68k/net/bpf_jit_comp.c 2026-10-06 22:51:13.099505355 +1000
> @@ -640,14 +640,19 @@
> bpf_put_reg32(dst[0], d_hi, ctx);
> }
>
> +static inline void emit_cf_swap16(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> +{
> + emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> + emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> + emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> + emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> + emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> +}
> +
> static inline void emit_to_le16(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> {
> if (CPU_IS_COLDFIRE) {
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> + emit_cf_swap16(ctx, d_lo, d_hi);
> } else {
> emit_16(ctx, 0x0280 | d_lo); /* andi.l #0xffff, d_lo */
> emit_32(ctx, 0xffff);
> @@ -657,25 +662,23 @@
> emit_16(ctx, 0x7000 | (d_hi << 9)); /* moveq #0, d_hi */
> }
>
> +static inline void emit_cf_swap32(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> +{
> + emit_16(ctx, 0x2f00 | d_lo); /* move.l d_lo, -(%sp) */
> + emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> + emit_cf_swap16(ctx, d_lo, d_hi);
> + emit_16(ctx, 0x2000 | (d_lo << 9) | d_hi); /* move.l d_lo, d_hi */
Should be emit_16(ctx, 0x2000 | (d_hi << 9) | d_lo) ?
Regards,
Kuan-Wei
> + emit_16(ctx, 0x2017 | (d_lo << 9)); /* move.l (%sp), d_lo */
> + emit_16(ctx, 0x2e80 | d_hi); /* move.l d_hi, (%sp) */
> + emit_cf_swap16(ctx, d_lo, d_hi);
> + emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> + emit_16(ctx, 0x809f | (d_lo << 9)); /* or.l (%sp)+, d_lo */
> +}
> +
> static inline void emit_to_le32(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> {
> if (CPU_IS_COLDFIRE) {
> - emit_16(ctx, 0x2f00 | d_lo); /* move.l d_lo, -(%sp) */
> - emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_hi << 9) | d_lo); /* or.l d_lo, d_hi */
> - emit_16(ctx, 0x2017 | (d_lo << 9)); /* move.l (%sp), d_lo */
> - emit_16(ctx, 0x2e80 | d_hi); /* move.l d_hi, (%sp) */
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> - emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> - emit_16(ctx, 0x809f | (d_lo << 9)); /* or.l (%sp)+, d_lo */
> + emit_cf_swap32(ctx, d_lo, d_hi);
> } else {
> emit_16(ctx, 0xe058 | d_lo); /* ror.w #8, d_lo */
> emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> @@ -688,45 +691,12 @@
> static inline void emit_to_le64(struct jit_ctx *ctx, s8 d_lo, s8 d_hi)
> {
> if (CPU_IS_COLDFIRE) {
> - 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, 0x4840 | d_lo); /* swap d_lo */
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_hi << 9) | d_lo); /* or.l d_lo, d_hi */
> - emit_16(ctx, 0x202f | (d_lo << 9)); /* move.l 4(%sp), d_lo */
> - emit_16(ctx, 0x0004);
> - emit_16(ctx, 0x2f40 | d_hi); /* move.l d_hi, 4(%sp) */
> - emit_16(ctx, 0x0004);
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> - emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> - emit_16(ctx, 0x81af | (d_lo << 9)); /* or.l d_lo, 4(%sp) */
> - emit_16(ctx, 0x0004);
> -
> - emit_16(ctx, 0x2017 | (d_lo << 9)); /* move.l (%sp), d_lo */
> - emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_hi << 9) | d_lo); /* or.l d_lo, d_hi */
> + emit_cf_swap32(ctx, d_lo, d_hi);
> + emit_16(ctx, 0x2000 | (d_lo << 9) | d_hi); /* move.l d_lo, d_hi */
> emit_16(ctx, 0x2017 | (d_lo << 9)); /* move.l (%sp), d_lo */
> emit_16(ctx, 0x2e80 | d_hi); /* move.l d_hi, (%sp) */
> - emit_16(ctx, 0x71c0 | (d_lo << 9) | d_lo); /* mvz.w d_lo, d_lo */
> - emit_16(ctx, 0x7180 | (d_hi << 9) | d_lo); /* mvz.b d_lo, d_hi */
> - emit_16(ctx, 0xe088 | d_lo); /* lsr.l #8, d_lo */
> - emit_16(ctx, 0xe188 | d_hi); /* lsl.l #8, d_hi */
> - emit_16(ctx, 0x8080 | (d_lo << 9) | d_hi); /* or.l d_hi, d_lo */
> - emit_16(ctx, 0x4840 | d_lo); /* swap d_lo */
> - emit_16(ctx, 0x809f | (d_lo << 9)); /* or.l (%sp)+, d_lo */
> -
> + emit_cf_swap32(ctx, d_lo, d_hi);
> emit_16(ctx, 0x201f | (d_hi << 9)); /* move.l (%sp)+, d_hi */
> } else {
> emit_16(ctx, 0xe058 | d_lo); /* ror.w #8, d_lo */
>
>
>
>
next prev parent reply other threads:[~2026-10-09 16:33 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-05 18:52 [PATCH v7] m68k, bpf: Add initial BPF JIT compiler support Kuan-Wei Chiu
2026-10-05 19:07 ` sashiko-bot
2026-10-05 19:48 ` bot+bpf-ci
2026-10-06 13:53 ` Greg Ungerer
2026-10-09 16:32 ` Kuan-Wei Chiu [this message]
2026-10-11 14:21 ` Greg Ungerer
2026-10-10 15:16 ` Alexei Starovoitov
2026-10-10 17:26 ` Kuan-Wei Chiu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=askXM-fN-olasi7l@google.com \
--to=visitorckw@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bot+bpf-ci@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=corbet@lwn.net \
--cc=daniel@iogearbox.net \
--cc=daniel@thingy.jp \
--cc=eddyz87@gmail.com \
--cc=eleanor15x@gmail.com \
--cc=emil@etsalapatis.com \
--cc=geert@linux-m68k.org \
--cc=gerg@linux-m68k.org \
--cc=ihor.solodrai@linux.dev \
--cc=jolsa@kernel.org \
--cc=jserv@ccns.ncku.edu.tw \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=marscheng@google.com \
--cc=martin.lau@kernel.org \
--cc=martin.lau@linux.dev \
--cc=mason@kernel.org \
--cc=memxor@gmail.com \
--cc=rdunlap@infradead.org \
--cc=skhan@linuxfoundation.org \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.