[PATCH v2 02/20] arm64: percpu: Fix this_cpu_and() mask generation
Mark Rutland
mark.rutland at arm.com
Wed Aug 5 06:02:03 PDT 2026
On Wed, Aug 05, 2026 at 10:14:16AM +0100, David Laight wrote:
> On Tue, 4 Aug 2026 18:04:45 +0100
> Mark Rutland <mark.rutland at arm.com> wrote:
>
> > The arm64 implementation of this_cpu_and(pcp, val) is built in terms of
> > ANDNOT operations, which requires the 'val' argument to be bitwise
> > negated. The bitwise negation is not implemented correctly, with two
> > bugs described below.
> >
> > (1) The bitwise negation is performed as '~val' rather than '~(val)'.
> > This won't always generate the expected value when 'val' is an
> > expression.
> >
> > For example, for this_cpu_and(pcp, 1 - 1):
> >
> > * 'val' is '1 - 1' ===> (int) 0x00000000
> > * '~val' is '~1 - 1' ===> (int) 0xfffffffd
> > * '~(val)' is '~(1 - 1)' ===> (int) 0xffffffff
> >
> > ... and thus bit[1] of 'pcp' would be preserved unexpectedly by the
> > ANDNOT operation.
> >
> > (2) The bitwise negation is performed on 'val' before it has been cast
> > to (at least) the width of 'pcp'. This won't always generate the
> > expected value for the upper bits.
> >
> > For example, for this_cpu_and(pcp, zero), where 'pcp' is a u64 and
> > 'zero' is a u32:
> >
> > * 'zero' ===> (u32) 0x00000000
> > * '~(zero)' ===> (u32) 0xffffffff
> > * '(u64)~(zero)' ===> (u64) 0x00000000ffffffff
> > * '~((u64)(zero))' ===> (u64) 0xffffffffffffffff
> >
> > ... and thus bits[63:32] of 'pcp' would be preserved unexpectedly by
> > the ANDNOT operation.
> >
> > Fix these issues by adding brackets around 'val', and by casting 'val'
> > to an appropriately-sized type before bitwise negation.
> > diff --git a/arch/arm64/include/asm/percpu.h b/arch/arm64/include/asm/percpu.h
> > index 63bbfd4944a37..31193bcf89a2b 100644
> > --- a/arch/arm64/include/asm/percpu.h
> > +++ b/arch/arm64/include/asm/percpu.h
> > @@ -206,13 +206,13 @@ PERCPU_RET_OP(add, add, ldadd)
> > _pcp_protect_return(__percpu_add_return_case_64, pcp, val)
> >
> > #define this_cpu_and_1(pcp, val) \
> > - _pcp_protect(__percpu_andnot_case_8, pcp, ~val)
> > + _pcp_protect(__percpu_andnot_case_8, pcp, ~(u8)(val))
> > #define this_cpu_and_2(pcp, val) \
> > - _pcp_protect(__percpu_andnot_case_16, pcp, ~val)
> > + _pcp_protect(__percpu_andnot_case_16, pcp, ~(u16)(val))
>
> I don't think the (u8) or (u16) casts are needed.
They're not strictly needed, but I added them for consistency with the
other cases.
> They force the high 24/16 bits to be ones, but the asm should
> ignore those bits (or possible even prefer they be zeros).
For 'sz' bits, the asm for this op only cares about val[sz-1:0], and
val[63:sz] is immaterial. There's no preference.
> They might also force the compiler to emit code to mask the high bits.
If __percpu_andnot_case_##sz() gets outlined, sure. When
__percpu_andnot_case_##sz() is inlined (which we expect in almost all cases
today), the compiler has visibility that bits [63:sz] are unused, and won't
generate redundant code.
That's a minor redundancy, not a functional issue. If we're worried about that,
we can have __percpu_andnot_case_##sz() take its argument as a u##sz (which TBH
we probably should anyway).
For example, see the code generated for:
| void this_cpu_and_u8__0xf0(u8 __percpu *p)
| {
| this_cpu_and(*p, 0xf0);
| }
At this point in the series, GCC 15.2.0 generates:
| <this_cpu_and_u8__0xf0>:
| paciasp
| stp x29, x30, [sp, #-16]!
| mrs x1, sp_el0
| mov x29, sp
| ldr w2, [x1, #8]
| add w2, w2, #0x1
| str w2, [x1, #8]
| mov w3, #0xf // <------ Low 8 bits only!
| mrs x2, tpidr_el1
| add x0, x0, x2
| 1: ldxrb w5, [x0]
| bic w5, w5, w3
| stxrb w4, w5, [x0]
| cbnz w4, 1b
| ldr x0, [x1, #8]
| sub x0, x0, #0x1
| str w0, [x1, #8]
| cbz x0, 2f
| ldr x0, [x1, #8]
| cbnz x0, 3f
| 2: bl preempt_schedule_notrace
| 3: ldp x29, x30, [sp], #16
| autiasp
| ret
> Actually the (u16) cast is wrong for (s8)128.
> That is tricky to fix, maybe:
> ~(sizeof(val) == 1 ? (u8)(val) : (val))
> (Remember ?: promotes its operands to int.)
I do not follow, and I think you are wrong.
My understanding is that the value arguments to a this_cpu_*() operation
should be subject to the usual type promotion rules. For a u16 'pcp' and
an s8 'v', 'pcp & v' should result in sign-extension of v, and
this_cpu_and(pcp, v) should do the same.
What makes you believe the semantic you propose is correct, and the
semantic I've implemented is wrong? Is there some documentation?
Tvhe semantic youe propose doesn't match what __this_cpu_and() does, and
it doesn't match what this_cpu_and() does on x86_64.
Note how __this_cpu_and() behaves. For the following test case:
| void outline_and__u16__s8_128(u16 __percpu *p)
| {
| s8 v = 128;
| __this_cpu_and(*p, v);
| }
|
| void outline_and__s16__s8_128(s16 __percpu *p)
| {
| s8 v = 128;
| __this_cpu_and(*p, v);
| }
For arm64 this generates:
| <outline_and__u16__s8_128>:
| mrs x2, tpidr_el1
| ldrh w1, [x0, x2]
| and w1, w1, #0xffffff80
| strh w1, [x0, x2]
|
| <outline_and__s16__s8_128>:
| mrs x2, tpidr_el1
| ldrh w1, [x0, x2]
| and w1, w1, #0xffffff80
| strh w1, [x0, x2]
| ret
... where '(s8)128' is sign-extended to (at least) 16 bits.
Note that LDRH and STRH only use the low 16 bits of the register, the
upper bits of the AND are irrelevant.
Note that for x86, those tests generate:
| andw $0xff80,%gs:(%rdi)
... which is clearly sign-extending to 16 bits.
> > #define this_cpu_and_4(pcp, val) \
> > - _pcp_protect(__percpu_andnot_case_32, pcp, ~val)
> > + _pcp_protect(__percpu_andnot_case_32, pcp, ~(u32)(val))
>
> The (u32) cast isn't needed (and has pretty much no effect).
As above, this is for consistency. I agree it happens to do nothing.
> > #define this_cpu_and_8(pcp, val) \
> > - _pcp_protect(__percpu_andnot_case_64, pcp, ~val)
> > + _pcp_protect(__percpu_andnot_case_64, pcp, ~(u64)(val))
>
> This one still isn't right.
> If val is a signed int with a negative value then it is sign extended
> before being inverted.
> val (int)0x80000000
> (u64)(val) 0xffffffff80000000
> ~(u64)(val) 0x000000007fffffff
> Something like ~(u64)((val) + 0u) will DTRT.
As above, where have you got that idea from?
AFAICT, a smaller signed type *should* be sign extended, and that must
happen before bitwise negation, since that bitwise negation is to cancel
out the NOT part of the ANDNOT operation.
Think:
'pcp' is (u64) 0x0123456789abcdef
'val' is (int) 0x800000000
'(u64)(val)' is (u64) 0xffffffff80000000
'pcp & (u64)(val)' is (u64) 0x0123456780000000
'~(u64)(val)' is (u64) 0x000000007fffffff
'pcp ANDNOT ~(u64)(val)' is (u64) 0x0123456780000000
See:
| void outline_and__u64__int_0x80000000(u64 __percpu *p)
| {
| int v = 0x80000000;
| __this_cpu_and(*p, v);
| }
|
| void outline_and__s64__int_0x80000000(s64 __percpu *p)
| {
| int v = 0x80000000;
| __this_cpu_and(*p, v);
| }
For which GCC 15.2.0 generates the following:
| <outline_and__u64__int_0x80000000>:
| mrs x2, tpidr_el1
| ldr x1, [x0, x2]
| and x1, x1, #0xffffffff80000000
| str x1, [x0, x2]
| ret
|
| <outline_and__s64__int_0x80000000>:
| mrs x2, tpidr_el1
| ldr x1, [x0, x2]
| and x1, x1, #0xffffffff80000000
| str x1, [x0, x2]
| ret
Likewise on x86 this generates:
| andq $0xffffffff80000000,%gs:(%rdi)
Mark.
More information about the linux-arm-kernel
mailing list