From: Greg Ungerer <gerg@linux-m68k.org>
To: Kuan-Wei Chiu <visitorckw@gmail.com>
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: Mon, 12 Oct 2026 00:21:17 +1000 [thread overview]
Message-ID: <396dd2cb-6487-40fe-91fe-51975a7a1949@linux-m68k.org> (raw)
In-Reply-To: <askXM-fN-olasi7l@google.com>
Hi Kuan-Wei,
On 10/10/26 02:32, Kuan-Wei Chiu wrote:
> 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) ?
Sorry, yes, you are right.
Sloppy testing on my part, I forgot to enable bpf_jit_enable before
loading and running the test_bpf module.
I can see you fixed it in v9, so all good now.
Regards
Greg
> 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-11 14:21 UTC|newest]
Thread overview: 7+ 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
2026-10-09 16:32 ` Kuan-Wei Chiu
2026-10-11 14:21 ` Greg Ungerer [this message]
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=396dd2cb-6487-40fe-91fe-51975a7a1949@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®