On 06/09/2017 20:26, Paul Burton wrote:
> Prior to this patch the xchg & cmpxchg functions have duplicated code
> which is for all intents & purposes identical apart from use of a
> branch-likely instruction in the R10000_LLSC_WAR case & a regular branch
> instruction in the non-R10000_LLSC_WAR case.
>
> This patch removes the duplication, declaring a __scbeqz macro to select
> the branch instruction suitable for use when checking the result of an
> sc instruction & making use of it to unify the 2 cases.
>
> In __xchg_u{32,64}() this means writing the branch in asm, where it was
> previously being done in C as a do...while loop for the
> non-R10000_LLSC_WAR case. As this is a single instruction, and adds
> consistency with the R10000_LLSC_WAR cases & the cmpxchg() code, this
> seems worthwhile.
IMHO, a good cleanup. I'll test shortly on my Octane once I get some things
moved around and a new kernel tree set up. That said, there are a number of
locations where R10000_LLSC_WAR is used in this manner, and I think this change
should be broken out into its own patch series, with the macro that selects
normal branch over branch-likely, placed into a main MIPS header file
somewhere, and the same change made to all locations at once. That'll give
some consistency to everything.
I don't think any of my SGI systems have the specific R10K revision that's
affected. I've been building kernels for a while now with a patch that splits
the R10K CPU selection up into vanilla R10K, and then R12K/R14K/R16K, and using
the non-R10000_LLSC_WAR path for R12K and up, as many of my SGI systems have at
least R12K processors. My IP28 might have the older, affected R10K, but I
haven't tested that out since 4.4 (it didn't survive for very long, either).
I also wonder if "__scbeqz" is a descriptive-enough name. It works in this
case because the macro definition is at the top, but if moved to a different
header file, perhaps something more like "__r10k_b_insn" or such may clue
casual readers in some more?
--J
> Signed-off-by: Paul Burton <paul.burton@xxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: linux-mips@xxxxxxxxxxxxxx
> ---
>
> arch/mips/include/asm/cmpxchg.h | 80 ++++++++++++-----------------------------
> 1 file changed, 22 insertions(+), 58 deletions(-)
>
> diff --git a/arch/mips/include/asm/cmpxchg.h b/arch/mips/include/asm/cmpxchg.h
> index b71ab4a5fd50..0582c01d229d 100644
> --- a/arch/mips/include/asm/cmpxchg.h
> +++ b/arch/mips/include/asm/cmpxchg.h
> @@ -13,44 +13,38 @@
> #include <asm/compiler.h>
> #include <asm/war.h>
>
> +/*
> + * Using a branch-likely instruction to check the result of an sc instruction
> + * works around a bug present in R10000 CPUs prior to revision 3.0 that could
> + * cause ll-sc sequences to execute non-atomically.
> + */
> +#if R10000_LLSC_WAR
> +# define __scbeqz "beqzl"
> +#else
> +# define __scbeqz "beqz"
> +#endif
> +
> static inline unsigned long __xchg_u32(volatile int * m, unsigned int val)
> {
> __u32 retval;
>
> smp_mb__before_llsc();
>
> - if (kernel_uses_llsc && R10000_LLSC_WAR) {
> + if (kernel_uses_llsc) {
> unsigned long dummy;
>
> __asm__ __volatile__(
> - " .set arch=r4000 \n"
> + " .set " MIPS_ISA_ARCH_LEVEL " \n"
> "1: ll %0, %3 # xchg_u32 \n"
> " .set mips0 \n"
> " move %2, %z4 \n"
> - " .set arch=r4000 \n"
> + " .set " MIPS_ISA_ARCH_LEVEL " \n"
> " sc %2, %1 \n"
> - " beqzl %2, 1b \n"
> + "\t" __scbeqz " %2, 1b \n"
> " .set mips0 \n"
> : "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m), "=&r" (dummy)
> : GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> : "memory");
> - } else if (kernel_uses_llsc) {
> - unsigned long dummy;
> -
> - do {
> - __asm__ __volatile__(
> - " .set "MIPS_ISA_ARCH_LEVEL" \n"
> - " ll %0, %3 # xchg_u32 \n"
> - " .set mips0 \n"
> - " move %2, %z4 \n"
> - " .set "MIPS_ISA_ARCH_LEVEL" \n"
> - " sc %2, %1 \n"
> - " .set mips0 \n"
> - : "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m),
> - "=&r" (dummy)
> - : GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> - : "memory");
> - } while (unlikely(!dummy));
> } else {
> unsigned long flags;
>
> @@ -72,34 +66,19 @@ static inline __u64 __xchg_u64(volatile __u64 * m, __u64 val)
>
> smp_mb__before_llsc();
>
> - if (kernel_uses_llsc && R10000_LLSC_WAR) {
> + if (kernel_uses_llsc) {
> unsigned long dummy;
>
> __asm__ __volatile__(
> - " .set arch=r4000 \n"
> + " .set " MIPS_ISA_ARCH_LEVEL " \n"
> "1: lld %0, %3 # xchg_u64 \n"
> " move %2, %z4 \n"
> " scd %2, %1 \n"
> - " beqzl %2, 1b \n"
> + "\t" __scbeqz " %2, 1b \n"
> " .set mips0 \n"
> : "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m), "=&r" (dummy)
> : GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> : "memory");
> - } else if (kernel_uses_llsc) {
> - unsigned long dummy;
> -
> - do {
> - __asm__ __volatile__(
> - " .set "MIPS_ISA_ARCH_LEVEL" \n"
> - " lld %0, %3 # xchg_u64 \n"
> - " move %2, %z4 \n"
> - " scd %2, %1 \n"
> - " .set mips0 \n"
> - : "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m),
> - "=&r" (dummy)
> - : GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> - : "memory");
> - } while (unlikely(!dummy));
> } else {
> unsigned long flags;
>
> @@ -142,24 +121,7 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
> ({ \
> __typeof(*(m)) __ret; \
> \
> - if (kernel_uses_llsc && R10000_LLSC_WAR) { \
> - __asm__ __volatile__( \
> - " .set push \n" \
> - " .set noat \n" \
> - " .set arch=r4000 \n" \
> - "1: " ld " %0, %2 # __cmpxchg_asm \n" \
> - " bne %0, %z3, 2f \n" \
> - " .set mips0 \n" \
> - " move $1, %z4 \n" \
> - " .set arch=r4000 \n" \
> - " " st " $1, %1 \n" \
> - " beqzl $1, 1b \n" \
> - "2: \n" \
> - " .set pop \n" \
> - : "=&r" (__ret), "=" GCC_OFF_SMALL_ASM() (*m) \
> - : GCC_OFF_SMALL_ASM() (*m), "Jr" (old), "Jr" (new) \
> - : "memory"); \
> - } else if (kernel_uses_llsc) { \
> + if (kernel_uses_llsc) { \
> __asm__ __volatile__( \
> " .set push \n" \
> " .set noat \n" \
> @@ -170,7 +132,7 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
> " move $1, %z4 \n" \
> " .set "MIPS_ISA_ARCH_LEVEL" \n" \
> " " st " $1, %1 \n" \
> - " beqz $1, 1b \n" \
> + "\t" __scbeqz " $1, 1b \n" \
> " .set pop \n" \
> "2: \n" \
> : "=&r" (__ret), "=" GCC_OFF_SMALL_ASM() (*m) \
> @@ -245,4 +207,6 @@ extern void __cmpxchg_called_with_bad_pointer(void);
> #define cmpxchg64(ptr, o, n) cmpxchg64_local((ptr), (o), (n))
> #endif
>
> +#undef __scbeqz
> +
> #endif /* __ASM_CMPXCHG_H */
>
--
Joshua Kinard
Gentoo/MIPS
kumba@xxxxxxxxxx
6144R/F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943
"The past tempts us, the present confuses us, the future frightens us. And our
lives slip away, moment by moment, lost in that vast, terrible in-between."
--Emperor Turhan, Centauri Republic
