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 */
>
>
>
>
prev parent reply other threads:[~2026-10-09 16:33 UTC|newest]
Thread overview: 5+ 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]
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox