From: Greg Ungerer <gerg@linux-m68k.org>
To: bot+bpf-ci@kernel.org, visitorckw@gmail.com, 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
Cc: 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: Tue, 6 Oct 2026 23:53:22 +1000 [thread overview]
Message-ID: <342ca584-c4ef-4d73-9017-9d21bbf9ce10@linux-m68k.org> (raw)
In-Reply-To: <506b804c97aaa3a66660bb0eebd85c6041e241532c1227f1012166785d240fae@mail.kernel.org>
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.
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 */
+ 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-06 13:53 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-05 18:52 Kuan-Wei Chiu
2026-10-05 19:48 ` bot+bpf-ci
2026-10-06 13:53 ` Greg Ungerer [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=342ca584-c4ef-4d73-9017-9d21bbf9ce10@linux-m68k.org \
--to=gerg@linux-m68k.org \
--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=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=visitorckw@gmail.com \
--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
all inboxes | Powered by JetHome®