From: WANG Rui <wangrui@loongson.cn>
To: bot+bpf-ci@kernel.org
Cc: yangtiezhu@loongson.cn, chenhuacai@kernel.org,
hengqi.chen@gmail.com, loongarch@lists.linux.dev,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
ast@kernel.org, andrii@kernel.org, daniel@iogearbox.net,
martin.lau@kernel.org, eddyz87@gmail.com,
yonghong.song@linux.dev, clm@meta.com, ihor.solodrai@linux.dev
Subject: Re: [PATCH v3 3/5] LoongArch: BPF: Support 8 and 16 bit read-modify-write instructions
Date: Mon, 13 Apr 2026 16:05:57 +0800 [thread overview]
Message-ID: <CAHirt9gy=HETVpi6bOrUOw65N0wuVVSMG-0usMSAQKm7G4OQww@mail.gmail.com> (raw)
In-Reply-To: <8822e0eb47985fc0bcb8bc6c9b0fcdfb55566144517d90a8e77f2690c44ee8df@mail.kernel.org>
On Mon, Apr 13, 2026 at 12:49 PM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> > index fefda4050a20..c9a32f124f5e 100644
> > --- a/arch/loongarch/net/bpf_jit.c
> > +++ b/arch/loongarch/net/bpf_jit.c
>
> [ ... ]
>
> > @@ -388,11 +408,32 @@ static int emit_atomic_rmw(const struct bpf_insn *insn, struct jit_ctx *ctx)
> > /* src = atomic_fetch_<op>(dst + off, src) */
> > case BPF_ADD | BPF_FETCH:
> > + switch (BPF_SIZE(insn->code)) {
> > + case BPF_B:
> > + if (cpu_has_lam_bh) {
> > + emit_insn(ctx, amaddb, src, t1, t3);
> > + emit_zext_32(ctx, src, true);
>
> Is emit_zext_32() sufficient for zero-extending an 8-bit value here?
>
> emit_zext_32() uses lu32id to clear bits [63:32], but for a byte-width
> atomic fetch the returned value needs bits [63:8] zeroed. If amadd.b
> follows the LoongArch convention of sign-extending sub-word results
> (as amadd.w does for 32-bit values), then a byte value like 0x80 would
> produce 0x00000000ffffff80 after emit_zext_32() instead of the expected
> 0x0000000000000080.
>
> Elsewhere in this file, 16-bit zero-extension already uses bstrpickd:
>
> emit_insn(ctx, bstrpickd, dst, dst, 15, 0);
>
> Should the BPF_B and BPF_H cases here use bstrpickd with the
> appropriate bit range (7,0 for bytes and 15,0 for halfwords) instead
> of emit_zext_32()?
I don't think truncating byte and sub-word results to only the low 8
or 16 bits is the right model here. The current behavior intentionally
keeps the low 32 bits of the computed value in the register, because
all uses explicitly extend from 32 bits to 64 bits when feeding
instructions that require 64-bit operands. The same reasoning should
apply to byte and sub-word types as well.
For example:
Def:
int8 a = amadd.b
int8 b = amadd.b
Use:
t1 = sext32 a
t2 = sext32 b
blt t1, t2 // a < b
Take a = -1 and b = 0. In the 64-bit registers:
If we keep the low 32 bits:
a: 0x0000_0000_ffff_ffff
b: 0x0000_0000_0000_0000
then:
t1: 0xffff_ffff_ffff_ffff
t2: 0x0000_0000_0000_0000
So a < b evaluates to true, as expected.
But if we only keep the low 8 bits:
a: 0x0000_0000_0000_00ff
b: 0x0000_0000_0000_0000
then:
t1: 0x0000_0000_0000_00ff
t2: 0x0000_0000_0000_0000
Now a < b becomes false, which is clearly not the intended result.
In other words, narrowing the result to 8 or 16 bits here would break
the existing 32-bit canonicalization rule for sub-word values.
More generally, the "always explicitly extend" pattern doesn't map
particularly well to the LoongArch ISA. In the long run, it would be
preferable if we could eliminate these explicit extensions altogether
rather than relying on them as part of the semantic model.
Thanks,
Rui
>
> > + case BPF_H:
> > + if (cpu_has_lam_bh) {
> > + emit_insn(ctx, amaddh, src, t1, t3);
> > + emit_zext_32(ctx, src, true);
>
> Same concern for the halfword case -- emit_zext_32() would leave
> bits [31:16] with sign-extended values if amadd.h sign-extends.
>
> [ ... ]
>
> > @@ -421,11 +462,32 @@ static int emit_atomic_rmw(const struct bpf_insn *insn, struct jit_ctx *ctx)
> > /* src = atomic_xchg(dst + off, src); */
> > case BPF_XCHG:
> > + switch (BPF_SIZE(insn->code)) {
> > + case BPF_B:
> > + if (cpu_has_lam_bh) {
> > + emit_insn(ctx, amswapb, src, t1, t3);
> > + emit_zext_32(ctx, src, true);
>
> Same question applies to the exchange path -- amswap.b and amswap.h
> return the old value and would need proper sub-word zero-extension
> rather than the 32-bit emit_zext_32().
>
> > + case BPF_H:
> > + if (cpu_has_lam_bh) {
> > + emit_insn(ctx, amswaph, src, t1, t3);
> > + emit_zext_32(ctx, src, true);
>
> Same here for amswap.h.
>
>
> ---
> 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/24325384786
next prev parent reply other threads:[~2026-04-13 8:06 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-13 4:05 [PATCH v3 0/5] LoongArch: BPF: Support more atomic instructions Tiezhu Yang
2026-04-13 4:05 ` [PATCH v3 1/5] LoongArch: Define instruction formats for AM{SWAP/ADD}.{B/H} and DBAR Tiezhu Yang
2026-04-15 2:24 ` Hengqi Chen
2026-04-13 4:05 ` [PATCH v3 2/5] LoongArch: BPF: Add the default case in emit_atomic() and rename it Tiezhu Yang
2026-04-15 2:24 ` Hengqi Chen
2026-04-13 4:05 ` [PATCH v3 3/5] LoongArch: BPF: Support 8 and 16 bit read-modify-write instructions Tiezhu Yang
2026-04-13 4:49 ` bot+bpf-ci
2026-04-13 8:05 ` WANG Rui [this message]
2026-04-13 4:05 ` [PATCH v3 4/5] LoongArch: BPF: Support load-acquire and store-release instructions Tiezhu Yang
2026-04-15 2:25 ` Hengqi Chen
2026-04-13 4:05 ` [PATCH v3 5/5] selftests/bpf: Make CAN_USE_LOAD_ACQ_STORE_REL usable for LoongArch Tiezhu Yang
2026-04-15 2:27 ` Hengqi Chen
2026-04-21 12:38 ` [PATCH v3 0/5] LoongArch: BPF: Support more atomic instructions Huacai Chen
2026-04-21 13:25 ` Tiezhu Yang
2026-04-21 13:46 ` Tiezhu Yang
2026-04-22 8:15 ` Huacai Chen
2026-04-23 2:56 ` Tiezhu Yang
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='CAHirt9gy=HETVpi6bOrUOw65N0wuVVSMG-0usMSAQKm7G4OQww@mail.gmail.com' \
--to=wangrui@loongson.cn \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bot+bpf-ci@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=chenhuacai@kernel.org \
--cc=clm@meta.com \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=hengqi.chen@gmail.com \
--cc=ihor.solodrai@linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=loongarch@lists.linux.dev \
--cc=martin.lau@kernel.org \
--cc=yangtiezhu@loongson.cn \
--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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).