Re: [PATCH v3 3/5] LoongArch: BPF: Support 8 and 16 bit read-modify-write instructions

[bot+bpf-ci]

> 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()?

> +		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