Hi Andrew,

On Mon, Aug 12, 2019 at 03:36:22PM +0100, Andrew Murray wrote:

For many of the ll/sc atomic operations we use the 'I' machine constraint
regardless to the instruction used - this may not be optimal.

Let's add an additional parameter to the ATOMIC_xx macros that allows the
caller to specify an appropriate machine constraint.

Let's also improve __CMPXCHG_CASE by replacing the 'K' constraint with a
caller provided constraint. Please note that whilst we would like to use
the 'K' constraint on 32 bit operations, we choose not to provide any
constraint to avoid a GCC bug which results in a build error.

Earlier versions of GCC (no later than 8.1.0) appear to incorrectly handle
the 'K' constraint for the value 4294967295.

From reading the above, it's difficult to discern what's a fix and
what's an improvement, and I think we need to be more explicit about
that. It would also be helpful to have the necessary context up-front.

How about:

| The A64 ISA accepts distinct (but overlapping) ranges of immediates for:
| 
| * add arithmetic instructions ('I' machine constraint)
| * sub arithmetic instructions ('J' machine constraint)
| * 32-bit logical instructions ('K' machine constraint)
| * 64-bit logical instructions ('L' machine constraint)
| 
| ... but we currently use the 'I' constraint for many atomic operations
| using sub or logical instructions, which is not always valid.
| 
| When CONFIG_ARM64_LSE_ATOMICS is not set, this allows invalid immediates
| to be passed to instructions, potentially resulting in a build failure.
| When CONFIG_ARM64_LSE_ATOMICS is selected the out-of-line ll/sc atomics
| always use a register as they have no visibility of the value passed by
| the caller.
|
| This patch adds a constraint parameter to the ATOMIC_xx and
| __CMPXCHG_CASE macros so that we can pass appropriate constraints for
| each case, with uses updated accordingly.
| 
| Unfortunately prior to GCC 8.1.0 the 'K' constraint erroneously accepted
| 0xffffffff, so we must instead force the use of a register.
 
Given we haven't had any bug reports, I'm not sure whether this needs a
Fixes tag or Cc stable. This has been a latent issue for a long time,
but upstream code doesn't seem to have tickled it.

[...]

-ATOMIC_OPS(and, and)
-ATOMIC_OPS(andnot, bic)
-ATOMIC_OPS(or, orr)
-ATOMIC_OPS(xor, eor)
+ATOMIC_OPS(and, and, K)
+ATOMIC_OPS(andnot, bic, )
+ATOMIC_OPS(or, orr, K)
+ATOMIC_OPS(xor, eor, K)

Surely it's not safe to use the K constraint here, either? AFAICT code
like:

  atomic_xor(~0, &atom);

... would suffer from the same problem as described for cmpxchg.

[...]

-ATOMIC64_OPS(and, and)
-ATOMIC64_OPS(andnot, bic)
-ATOMIC64_OPS(or, orr)
-ATOMIC64_OPS(xor, eor)
+ATOMIC64_OPS(and, and, K)
+ATOMIC64_OPS(andnot, bic, )
+ATOMIC64_OPS(or, orr, K)
+ATOMIC64_OPS(xor, eor, K)

Shouldn't these be 'L'?

IIUC K is a subset of L, so if that's deliberate we should call that out
explicitly...

+__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , )
+__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , )
+__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , )
+__CMPXCHG_CASE( ,  ,     , 64,        ,  ,  ,         , L)
+__CMPXCHG_CASE(w, b, acq_,  8,        , a,  , "memory", )
+__CMPXCHG_CASE(w, h, acq_, 16,        , a,  , "memory", )
+__CMPXCHG_CASE(w,  , acq_, 32,        , a,  , "memory", )
+__CMPXCHG_CASE( ,  , acq_, 64,        , a,  , "memory", L)
+__CMPXCHG_CASE(w, b, rel_,  8,        ,  , l, "memory", )
+__CMPXCHG_CASE(w, h, rel_, 16,        ,  , l, "memory", )
+__CMPXCHG_CASE(w,  , rel_, 32,        ,  , l, "memory", )
+__CMPXCHG_CASE( ,  , rel_, 64,        ,  , l, "memory", L)
+__CMPXCHG_CASE(w, b,  mb_,  8, dmb ish,  , l, "memory", )
+__CMPXCHG_CASE(w, h,  mb_, 16, dmb ish,  , l, "memory", )
+__CMPXCHG_CASE(w,  ,  mb_, 32, dmb ish,  , l, "memory", )
+__CMPXCHG_CASE( ,  ,  mb_, 64, dmb ish,  , l, "memory", L)

... but these uses imply that's not the case.

Otherwise this looks good to me.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel