Re: [External] Re: [PATCH v4 2/3] riscv: introduce percpu.h into include/asm
yunhui cui <[email protected]> Wed, 1 Jul 2026 11:20:30 +0800
| Newsgroups | dev.linux.lists.sashiko,org.kernel.vger.bpf |
|---|---|
| Message-ID | <CAEEQ3w=KmjNTn5NwyoU8DX8hkB74PiTyp=vNKnG_1P1WVCekrA@mail.gmail.com> |
Hi sashiko, On Tue, May 5, 2026 at 3:26=E2=80=AFPM <[email protected]> wrote: > > Thank you for your contribution! Sashiko AI review found 3 potential issu= e(s) to consider: > - [Critical] The 8-bit and 16-bit `this_cpu_add` fallback logic fails to = mask arithmetic overflow, corrupting adjacent percpu variables. > - [High] The 32-bit and 64-bit `this_cpu_and` macros incorrectly perform = an AND-NOT operation instead of a bitwise AND. > - [High] Missing early-clobber modifiers (`=3D&r`) on output operands in = `PERCPU_8_16_RET_OP` allow destructive register overlapping. > -- > > commit f4a490dc0b24e17cbd874e03d02e506d017e61b6 > Author: Yunhui Cui <[email protected]> > > riscv: introduce percpu.h into include/asm > > This commit introduces a RISC-V percpu header to implement direct this_cp= u > operations. It provides macros for 8, 16, 32, and 64-bit read, write, and > atomic memory operations. > > > diff --git a/arch/riscv/include/asm/percpu.h b/arch/riscv/include/asm/p= ercpu.h > > new file mode 100644 > > index 0000000000000..3b26fe45e70f4 > > --- /dev/null > > +++ b/arch/riscv/include/asm/percpu.h > > [ ... ] > > > +#define PERCPU_8_16_OP(name, amo_insn, sz, sfx, val_type, new_val_expr= , asm_op) \ > > +static inline void __percpu_##name##_amo_case_##sz(void *ptr, unsigned= long val) \ > > +{ = \ > > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) && = \ > > + riscv_has_extension_unlikely(RISCV_ISA_EXT_ZABHA)) { = \ > > + asm volatile ("amo" #amo_insn #sfx " zero, %[val], %[ptr]= " \ > > + : [ptr] "+A"(*(val_type *)ptr) = \ > > + : [val] "r"((val_type)((new_val_expr) & PERCPU_8_= 16_GET_MASK(sz))) \ > > + : "memory"); = \ > > + } else { = \ > > + u32 *ptr32 =3D PERCPU_8_16_GET_PTR32(ptr); = \ > > + const unsigned long shift =3D PERCPU_8_16_GET_SHIFT(ptr);= \ > > + const u32 mask =3D PERCPU_8_16_GET_MASK(sz) << shift; = \ > > + const val_type val_trunc =3D (val_type)((new_val_expr) = \ > > + & PERCPU_8_16_GET_MASK(sz)); = \ > > + u32 retx, rc; = \ > > + val_type new_val_type; = \ > > + = \ > > + asm volatile ( = \ > > + "0: lr.w %0, %2\n" = \ > > + "and %3, %0, %4\n" = \ > > + "srl %3, %3, %5\n" = \ > > + #asm_op " %3, %3, %6\n" = \ > > Is it possible for an 8-bit or 16-bit addition to overflow here? If there= are > carry bits outside the subword boundary, they aren't masked out before th= e > result is shifted left. > > Could this silent overflow spill into and corrupt adjacent per-CPU variab= les? Thanks, will fix this in the next version. > > > + "sll %3, %3, %5\n" = \ > > + "and %1, %0, %7\n" = \ > > + "or %1, %1, %3\n" = \ > > + "sc.w %1, %1, %2\n" = \ > > + "bnez %1, 0b\n" = \ > > + : "=3D&r"(retx), "=3D&r"(rc), "+A"(*ptr32), "=3D&= r"(new_val_type) \ > > + : "r"(mask), "r"(shift), "r"(val_trunc), "r"(~mas= k) \ > > + : "memory"); = \ > > + } = \ > > +} > > [ ... ] > > > +#define PERCPU_8_16_RET_OP(name, amo_insn, sz, sfx, val_type, new_val_= expr) \ > > +static inline val_type __percpu_##name##_return_amo_case_##sz(void *pt= r, unsigned long val) \ > > +{ = \ > > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) && = \ > > + riscv_has_extension_unlikely(RISCV_ISA_EXT_ZABHA)) { = \ > > [ ... ] > > > + } else { = \ > > + u32 *ptr32 =3D PERCPU_8_16_GET_PTR32(ptr); = \ > > + const unsigned long shift =3D PERCPU_8_16_GET_SHIFT(ptr);= \ > > + const u32 mask =3D (PERCPU_8_16_GET_MASK(sz) << shift); = \ > > + const u32 inv_mask =3D ~mask; = \ > > + const val_type val_trunc =3D (val_type)((new_val_expr) = \ > > + & PERCPU_8_16_GET_MASK(sz)); = \ > > + u32 old, new, tmp; = \ > > + = \ > > + asm volatile ( = \ > > + "0: lr.w %0, %3\n" = \ > > + "and %1, %0, %4\n" = \ > > + "srl %1, %1, %5\n" = \ > > + "add %1, %1, %6\n" = \ > > + "and %1, %1, %7\n" = \ > > + "sll %1, %1, %5\n" = \ > > + "and %2, %0, %8\n" = \ > > + "or %2, %2, %1\n" = \ > > + "sc.w %2, %2, %3\n" = \ > > + "bnez %2, 0b\n" = \ > > + : "=3Dr"(old), "=3Dr"(tmp), "=3D&r"(new), "+A"(*p= tr32) \ > > Should the old and tmp output operands use the early-clobber modifier (= =3D&r)? > > Since they are written to early in the instruction sequence before inputs > like mask (%4), shift (%5), or val_trunc (%6) are consumed, could the com= piler > allocate them to the same physical registers, leading to clobbered inputs= ? > > > + : "r"(mask), "r"(shift), "r"(val_trunc), "r"(PERC= PU_8_16_GET_MASK(sz)), \ > > + "r"(inv_mask) = \ > > + : "memory"); = \ > > + return (val_type)(tmp >> shift); = \ > > + } = \ > > +} > > [ ... ] > > > +#define this_cpu_and_1(pcp, val) _pcp_protect(__percpu_andnot_amo_= case_8, pcp, ~(val)) > > +#define this_cpu_and_2(pcp, val) _pcp_protect(__percpu_andnot_amo_= case_16, pcp, ~(val)) > > +#define this_cpu_and_4(pcp, val) _pcp_protect(__percpu_andnot_amo_= case_32, pcp, ~(val)) > > + > > +#ifdef CONFIG_64BIT > > +#define this_cpu_and_8(pcp, val) _pcp_protect(__percpu_andnot_amo_= case_64, pcp, ~(val)) > > +#endif > > Since the underlying __percpu_andnot_amo_case_32 and 64 macros directly u= se the > amoand instruction on the value passed, does negating val here with ~(val= ) > cause the hardware to perform an AND-NOT operation (pcp &=3D ~val) instea= d of a > bitwise AND (pcp &=3D val)? > > > + > > +#define this_cpu_or_1(pcp, val) _pcp_protect(__percpu_or_amo_case= _8, pcp, val) > > -- > Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260505062026.91= [email protected]?part=3D2 Thanks, Yunhui