arm64: avoid out-of-line ll/sc atomics

From: Andrew Murray <[email protected]>

When building for LSE atomics (CONFIG_ARM64_LSE_ATOMICS), if the hardware or toolchain doesn't support it the existing code will fallback to ll/sc atomics. It achieves this by branching from inline assembly to a function that is built with special compile flags. Further this results in the clobbering of registers even when the fallback isn't used increasing register pressure.

Improve this by providing inline implementations of both LSE and ll/sc and use a static key to select between them, which allows for the compiler to generate better atomics code. Put the LL/SC fallback atomics in their own subsection to improve icache performance.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 14 lines · Will Deacon

From: Andrew Murray <[email protected]>

When building for LSE atomics (CONFIG_ARM64_LSE_ATOMICS), if the hardware or toolchain doesn't support it the existing code will fallback to ll/sc atomics. It achieves this by branching from inline assembly to a function that is built with special compile flags. Further this results in the clobbering of registers even when the fallback isn't used increasing register pressure.

Improve this by providing inline implementations of both LSE and ll/sc and use a static key to select between them, which allows for the compiler to generate better atomics code. Put the LL/SC fallback atomics in their own subsection to improve icache performance.

Quoted 2 lines

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

For some reason, this causes a clang built kernel to fail to boot in QEMU. There are no logs, it just never starts. I am off for the next two days so I am going to try to look into this but you might have some immediate ideas.

There is another weird failure that might be somewhat related but I have no idea.

https://github.com/ClangBuiltLinux/linux/issues/648

Quoted 9 lines · Will Deacon

From: Andrew Murray <[email protected]>

When building for LSE atomics (CONFIG_ARM64_LSE_ATOMICS), if the hardware or toolchain doesn't support it the existing code will fallback to ll/sc atomics. It achieves this by branching from inline assembly to a function that is built with special compile flags. Further this results in the clobbering of registers even when the fallback isn't used increasing register pressure.

Quoted 7 lines

Improve this by providing inline implementations of both LSE and ll/sc and use a static key to select between them, which allows for the compiler to generate better atomics code. Put the LL/SC fallback atomics in their own subsection to improve icache performance.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

For some reason, this causes a clang built kernel to fail to boot in QEMU. There are no logs, it just never starts. I am off for the next two days so I am going to try to look into this but you might have some immediate ideas.

https://github.com/ClangBuiltLinux/linux/issues/649

There is another weird failure that might be somewhat related but I have no idea.

https://github.com/ClangBuiltLinux/linux/issues/648

Cheers,
Nathan

Quoted 18 lines · Nathan Chancellor

Quoted 9 lines · Will Deacon

From: Andrew Murray <[email protected]>

When building for LSE atomics (CONFIG_ARM64_LSE_ATOMICS), if the hardware or toolchain doesn't support it the existing code will fallback to ll/sc atomics. It achieves this by branching from inline assembly to a function that is built with special compile flags. Further this results in the clobbering of registers even when the fallback isn't used increasing register pressure.

Quoted 7 lines

Improve this by providing inline implementations of both LSE and ll/sc and use a static key to select between them, which allows for the compiler to generate better atomics code. Put the LL/SC fallback atomics in their own subsection to improve icache performance.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 6 lines

For some reason, this causes a clang built kernel to fail to boot in QEMU. There are no logs, it just never starts. I am off for the next two days so I am going to try to look into this but you might have some immediate ideas.

https://github.com/ClangBuiltLinux/linux/issues/649

I've been able to reproduce this - with clang 9.0.6 and qemu (without KVM) and only when ARM64_LSE_ATOMICS is enabled.

This is slightly concerning...

(gdb) b __lse__cmpxchg_case_acq_32
Breakpoint 1 at 0xffff80001012b3cc: __lse__cmpxchg_case_acq_32. (19 locations) (gdb) continue
Continuing.

Any ideas?

Quoted 29 lines · Andrew Murray

Quoted 18 lines · Nathan Chancellor

Quoted 9 lines · Will Deacon

From: Andrew Murray <[email protected]>

When building for LSE atomics (CONFIG_ARM64_LSE_ATOMICS), if the hardware or toolchain doesn't support it the existing code will fallback to ll/sc atomics. It achieves this by branching from inline assembly to a function that is built with special compile flags. Further this results in the clobbering of registers even when the fallback isn't used increasing register pressure.

Quoted 7 lines

Improve this by providing inline implementations of both LSE and ll/sc and use a static key to select between them, which allows for the compiler to generate better atomics code. Put the LL/SC fallback atomics in their own subsection to improve icache performance.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 6 lines

For some reason, this causes a clang built kernel to fail to boot in QEMU. There are no logs, it just never starts. I am off for the next two days so I am going to try to look into this but you might have some immediate ideas.

https://github.com/ClangBuiltLinux/linux/issues/649

I've been able to reproduce this - with clang 9.0.6 and qemu (without KVM) and only when ARM64_LSE_ATOMICS is enabled.

Quoted 6 lines

This is slightly concerning...

(gdb) b __lse__cmpxchg_case_acq_32
Breakpoint 1 at 0xffff80001012b3cc: __lse__cmpxchg_case_acq_32. (19 locations) (gdb) continue
Continuing.

[...]

Quoted 1 line

Any ideas?

Does it work if the only thing you change is the toolchain, and use GCC instead?

Could be some teething issues in the 'asm goto' support for clang?

Will

Quoted 37 lines · Will Deacon

Quoted 35 lines · Andrew Murray

Quoted 18 lines · Nathan Chancellor

Quoted 9 lines · Will Deacon

From: Andrew Murray <[email protected]>

When building for LSE atomics (CONFIG_ARM64_LSE_ATOMICS), if the hardware or toolchain doesn't support it the existing code will fallback to ll/sc atomics. It achieves this by branching from inline assembly to a function that is built with special compile flags. Further this results in the clobbering of registers even when the fallback isn't used increasing register pressure.

Quoted 7 lines

Improve this by providing inline implementations of both LSE and ll/sc and use a static key to select between them, which allows for the compiler to generate better atomics code. Put the LL/SC fallback atomics in their own subsection to improve icache performance.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Quoted 6 lines

For some reason, this causes a clang built kernel to fail to boot in QEMU. There are no logs, it just never starts. I am off for the next two days so I am going to try to look into this but you might have some immediate ideas.

https://github.com/ClangBuiltLinux/linux/issues/649

I've been able to reproduce this - with clang 9.0.6 and qemu (without KVM) and only when ARM64_LSE_ATOMICS is enabled.

This is slightly concerning...

(gdb) b __lse__cmpxchg_case_acq_32
Breakpoint 1 at 0xffff80001012b3cc: __lse__cmpxchg_case_acq_32. (19 locations) (gdb) continue
Continuing.

Quoted 6 lines

[...]

Quoted 1 line

Any ideas?

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Quoted 3 lines

Could be some teething issues in the 'asm goto' support for clang?

Thanks,

Andrew Murray

Quoted 1 line

Quoted 1 line

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Andrew Murray

Quoted 2 lines · Will Deacon

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Also this is Clang generation:

ffff8000100f2700 <__ptrace_link>:

ffff8000100f2700:       f9426009        ldr     x9, [x0, #1216]
ffff8000100f2704:       91130008        add     x8, x0, #0x4c0
ffff8000100f2708:       eb09011f        cmp     x8, x9
ffff8000100f270c:       540002a1        b.ne    ffff8000100f2760 <__ptrace_link+0x60>  // b.any
ffff8000100f2710:       f9425829        ldr     x9, [x1, #1200]
ffff8000100f2714:       9112c02a        add     x10, x1, #0x4b0
ffff8000100f2718:       f9000528        str     x8, [x9, #8]
ffff8000100f271c:       f9026009        str     x9, [x0, #1216]
ffff8000100f2720:       f902640a        str     x10, [x0, #1224]
ffff8000100f2724:       f9025828        str     x8, [x1, #1200]
ffff8000100f2728:       f9024001        str     x1, [x0, #1152]
ffff8000100f272c:       b4000162        cbz     x2, ffff8000100f2758 <__ptrace_link+0x58>
ffff8000100f2730:       b900985f        str     wzr, [x2, #152]
ffff8000100f2734:       14000004        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2738:       14000001        b       ffff8000100f273c <__ptrace_link+0x3c>
ffff8000100f273c:       14000006        b       ffff8000100f2754 <__ptrace_link+0x54>
ffff8000100f2740:       14000001        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2744:       52800028        mov     w8, #0x1                        // #1
ffff8000100f2748:       b828005f        stadd   w8, [x2]
ffff8000100f274c:       f9030002        str     x2, [x0, #1536]
ffff8000100f2750:       d65f03c0        ret
ffff8000100f2754:       140007fd        b       ffff8000100f4748 <ptrace_check_attach+0xf8>

...

This looks like the default path (before we write over it) will take you to the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at least not what we expected to see. Also why 4 branches?

Quoted 32 lines · Andrew Murray

Quoted 5 lines · Andrew Murray

Quoted 2 lines · Will Deacon

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Also this is Clang generation:

ffff8000100f2700 <__ptrace_link>:

ffff8000100f2700:       f9426009        ldr     x9, [x0, #1216]
ffff8000100f2704:       91130008        add     x8, x0, #0x4c0
ffff8000100f2708:       eb09011f        cmp     x8, x9
ffff8000100f270c:       540002a1        b.ne    ffff8000100f2760 <__ptrace_link+0x60>  // b.any
ffff8000100f2710:       f9425829        ldr     x9, [x1, #1200]
ffff8000100f2714:       9112c02a        add     x10, x1, #0x4b0
ffff8000100f2718:       f9000528        str     x8, [x9, #8]
ffff8000100f271c:       f9026009        str     x9, [x0, #1216]
ffff8000100f2720:       f902640a        str     x10, [x0, #1224]
ffff8000100f2724:       f9025828        str     x8, [x1, #1200]
ffff8000100f2728:       f9024001        str     x1, [x0, #1152]
ffff8000100f272c:       b4000162        cbz     x2, ffff8000100f2758 <__ptrace_link+0x58>
ffff8000100f2730:       b900985f        str     wzr, [x2, #152]
ffff8000100f2734:       14000004        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2738:       14000001        b       ffff8000100f273c <__ptrace_link+0x3c>
ffff8000100f273c:       14000006        b       ffff8000100f2754 <__ptrace_link+0x54>
ffff8000100f2740:       14000001        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2744:       52800028        mov     w8, #0x1                        // #1
ffff8000100f2748:       b828005f        stadd   w8, [x2]
ffff8000100f274c:       f9030002        str     x2, [x0, #1536]
ffff8000100f2750:       d65f03c0        ret
ffff8000100f2754:       140007fd        b       ffff8000100f4748 <ptrace_check_attach+0xf8>

Quoted 5 lines

...

This looks like the default path (before we write over it) will take you to the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at least not what we expected to see. Also why 4 branches?

So I reproduced this with a silly atomic_inc wrapper:

void will_atomic_inc(atomic_t *v)

{
        atomic_inc(v);
}

Compiles to:

0000000000000018 <will_atomic_inc>:
18: 14000004 b 28 <will_atomic_inc+0x10>
1c: 14000001 b 20 <will_atomic_inc+0x8>
20: 14000005 b 34 <will_atomic_inc+0x1c>
24: 14000001 b 28 <will_atomic_inc+0x10>
28: 52800028 mov w8, #0x1 // #1
2c: b828001f stadd w8, [x0]
30: d65f03c0 ret
34: 14000027 b d0 <dump_kernel_offset+0x60>
38: d65f03c0 ret

which is going to explode.

Will

Quoted 59 lines · Will Deacon

Quoted 32 lines · Andrew Murray

Quoted 5 lines · Andrew Murray

Quoted 2 lines · Will Deacon

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Also this is Clang generation:

ffff8000100f2700 <__ptrace_link>:

ffff8000100f2700:       f9426009        ldr     x9, [x0, #1216]
ffff8000100f2704:       91130008        add     x8, x0, #0x4c0
ffff8000100f2708:       eb09011f        cmp     x8, x9
ffff8000100f270c:       540002a1        b.ne    ffff8000100f2760 <__ptrace_link+0x60>  // b.any
ffff8000100f2710:       f9425829        ldr     x9, [x1, #1200]
ffff8000100f2714:       9112c02a        add     x10, x1, #0x4b0
ffff8000100f2718:       f9000528        str     x8, [x9, #8]
ffff8000100f271c:       f9026009        str     x9, [x0, #1216]
ffff8000100f2720:       f902640a        str     x10, [x0, #1224]
ffff8000100f2724:       f9025828        str     x8, [x1, #1200]
ffff8000100f2728:       f9024001        str     x1, [x0, #1152]
ffff8000100f272c:       b4000162        cbz     x2, ffff8000100f2758 <__ptrace_link+0x58>
ffff8000100f2730:       b900985f        str     wzr, [x2, #152]
ffff8000100f2734:       14000004        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2738:       14000001        b       ffff8000100f273c <__ptrace_link+0x3c>
ffff8000100f273c:       14000006        b       ffff8000100f2754 <__ptrace_link+0x54>
ffff8000100f2740:       14000001        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2744:       52800028        mov     w8, #0x1                        // #1
ffff8000100f2748:       b828005f        stadd   w8, [x2]
ffff8000100f274c:       f9030002        str     x2, [x0, #1536]
ffff8000100f2750:       d65f03c0        ret
ffff8000100f2754:       140007fd        b       ffff8000100f4748 <ptrace_check_attach+0xf8>

Quoted 5 lines

...

This looks like the default path (before we write over it) will take you to the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at least not what we expected to see. Also why 4 branches?

So I reproduced this with a silly atomic_inc wrapper:

void will_atomic_inc(atomic_t *v)

{
        atomic_inc(v);
}

Compiles to:

0000000000000018 <will_atomic_inc>:

  18: 14000004        b       28 <will_atomic_inc+0x10>
  1c: 14000001        b       20 <will_atomic_inc+0x8>
  20: 14000005        b       34 <will_atomic_inc+0x1c>
  24: 14000001        b       28 <will_atomic_inc+0x10>
  28: 52800028        mov     w8, #0x1                        // #1
  2c: b828001f        stadd   w8, [x0]
  30: d65f03c0        ret
  34: 14000027        b       d0 <dump_kernel_offset+0x60>
  38: d65f03c0        ret

Quoted 1 line

which is going to explode.

I've come up with a simple reproducer for this issue:

static bool branch_jump()

{
        asm_volatile_goto(
                "1: b %l[l_yes2]"
                 : : : : l_yes2);

        return false;
l_yes2:
        return true;
}

static bool branch_test()

{
        return (!branch_jump() && !branch_jump());
}

void andy_test(int *v)

{
        if (branch_test())
                *v = 0xff;
}

This leads to the following (it shouldn't do anything):

0000000000000000 <andy_test>:

   0:   14000004        b       10 <andy_test+0x10>
   4:   14000001        b       8 <andy_test+0x8>
   8:   14000004        b       18 <andy_test+0x18>
   c:   14000001        b       10 <andy_test+0x10>
  10:   52801fe8        mov     w8, #0xff                       // #255
  14:   b9000008        str     w8, [x0]
  18:   d65f03c0        ret

The issue goes away with any of the following hunks:

(quoted diff) +1 -1

@@ -55,7 +55,7 @@ static bool branch_jump()
 
 static bool branch_test()
 {
-       return (!branch_jump() && !branch_jump());
+       return (!branch_jump());
 }
 
 void andy_test(int *v)

or:

(quoted diff) +0 -1

@@ -53,14 +53,10 @@ static bool branch_jump()
         return true;
 }
 
-static bool branch_test()

Quoted 6 lines

Thanks for the report. We squashed many bugs related to asm goto, but it's difficult to say with 100% certainty that the current implementation is bug free. Simply throwing more exotic forms of control flow at it often shake out corner cases. Thank you very much for the reduced test case, and I'll look into getting a fix ready hopefully in time to make the clang-9 release train.

Do you mean that you'd do a backport to Clang 9 as well as the trunk contribution?

Quoted 7 lines · Nick Desaulniers

Quoted 6 lines

Thanks for the report. We squashed many bugs related to asm goto, but it's difficult to say with 100% certainty that the current implementation is bug free. Simply throwing more exotic forms of control flow at it often shake out corner cases. Thank you very much for the reduced test case, and I'll look into getting a fix ready hopefully in time to make the clang-9 release train.

Quoted 2 lines · Itaru Kitayama

Do you mean that you'd do a backport to Clang 9 as well as the trunk contribution?

Yes; I think the window for merging things in the 9.0 release is still open, though they are late in the -rc cycle. If not 9.1 bugfix is possible.

Quoted 59 lines · Will Deacon

Quoted 32 lines · Andrew Murray

Quoted 5 lines · Andrew Murray

Quoted 2 lines · Will Deacon

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Also this is Clang generation:

ffff8000100f2700 <__ptrace_link>:

ffff8000100f2700:       f9426009        ldr     x9, [x0, #1216]
ffff8000100f2704:       91130008        add     x8, x0, #0x4c0
ffff8000100f2708:       eb09011f        cmp     x8, x9
ffff8000100f270c:       540002a1        b.ne    ffff8000100f2760 <__ptrace_link+0x60>  // b.any
ffff8000100f2710:       f9425829        ldr     x9, [x1, #1200]
ffff8000100f2714:       9112c02a        add     x10, x1, #0x4b0
ffff8000100f2718:       f9000528        str     x8, [x9, #8]
ffff8000100f271c:       f9026009        str     x9, [x0, #1216]
ffff8000100f2720:       f902640a        str     x10, [x0, #1224]
ffff8000100f2724:       f9025828        str     x8, [x1, #1200]
ffff8000100f2728:       f9024001        str     x1, [x0, #1152]
ffff8000100f272c:       b4000162        cbz     x2, ffff8000100f2758 <__ptrace_link+0x58>
ffff8000100f2730:       b900985f        str     wzr, [x2, #152]
ffff8000100f2734:       14000004        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2738:       14000001        b       ffff8000100f273c <__ptrace_link+0x3c>
ffff8000100f273c:       14000006        b       ffff8000100f2754 <__ptrace_link+0x54>
ffff8000100f2740:       14000001        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2744:       52800028        mov     w8, #0x1                        // #1
ffff8000100f2748:       b828005f        stadd   w8, [x2]
ffff8000100f274c:       f9030002        str     x2, [x0, #1536]
ffff8000100f2750:       d65f03c0        ret
ffff8000100f2754:       140007fd        b       ffff8000100f4748 <ptrace_check_attach+0xf8>

Quoted 5 lines

...

This looks like the default path (before we write over it) will take you to the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at least not what we expected to see. Also why 4 branches?

So I reproduced this with a silly atomic_inc wrapper:

void will_atomic_inc(atomic_t *v)

{
        atomic_inc(v);
}

Compiles to:

0000000000000018 <will_atomic_inc>:

  18: 14000004        b       28 <will_atomic_inc+0x10>
  1c: 14000001        b       20 <will_atomic_inc+0x8>
  20: 14000005        b       34 <will_atomic_inc+0x1c>
  24: 14000001        b       28 <will_atomic_inc+0x10>
  28: 52800028        mov     w8, #0x1                        // #1
  2c: b828001f        stadd   w8, [x0]
  30: d65f03c0        ret
  34: 14000027        b       d0 <dump_kernel_offset+0x60>
  38: d65f03c0        ret

Quoted 1 line

which is going to explode.

I've come up with a simple reproducer for this issue:

static bool branch_jump()

{
        asm_volatile_goto(
                "1: b %l[l_yes2]"
                 : : : : l_yes2);

        return false;
l_yes2:
        return true;
}

static bool branch_test()

{
        return (!branch_jump() && !branch_jump());
}

void andy_test(int *v)

{
        if (branch_test())
                *v = 0xff;
}

This leads to the following (it shouldn't do anything):

0000000000000000 <andy_test>:

   0:   14000004        b       10 <andy_test+0x10>
   4:   14000001        b       8 <andy_test+0x8>
   8:   14000004        b       18 <andy_test+0x18>
   c:   14000001        b       10 <andy_test+0x10>
  10:   52801fe8        mov     w8, #0xff                       // #255
  14:   b9000008        str     w8, [x0]
  18:   d65f03c0        ret

The issue goes away with any of the following hunks:

(quoted diff) +1 -1

@@ -55,7 +55,7 @@ static bool branch_jump()
 
 static bool branch_test()
 {
-       return (!branch_jump() && !branch_jump());
+       return (!branch_jump());
 }
 
 void andy_test(int *v)

or:

(quoted diff) +0 -1

@@ -53,14 +53,10 @@ static bool branch_jump()
         return true;
 }
 
-static bool branch_test()

Quoted 124 lines · Andrew Murray

Quoted 59 lines · Will Deacon

Quoted 32 lines · Andrew Murray

Quoted 5 lines · Andrew Murray

Quoted 2 lines · Will Deacon

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Also this is Clang generation:

ffff8000100f2700 <__ptrace_link>:

ffff8000100f2700:       f9426009        ldr     x9, [x0, #1216]
ffff8000100f2704:       91130008        add     x8, x0, #0x4c0
ffff8000100f2708:       eb09011f        cmp     x8, x9
ffff8000100f270c:       540002a1        b.ne    ffff8000100f2760 <__ptrace_link+0x60>  // b.any
ffff8000100f2710:       f9425829        ldr     x9, [x1, #1200]
ffff8000100f2714:       9112c02a        add     x10, x1, #0x4b0
ffff8000100f2718:       f9000528        str     x8, [x9, #8]
ffff8000100f271c:       f9026009        str     x9, [x0, #1216]
ffff8000100f2720:       f902640a        str     x10, [x0, #1224]
ffff8000100f2724:       f9025828        str     x8, [x1, #1200]
ffff8000100f2728:       f9024001        str     x1, [x0, #1152]
ffff8000100f272c:       b4000162        cbz     x2, ffff8000100f2758 <__ptrace_link+0x58>
ffff8000100f2730:       b900985f        str     wzr, [x2, #152]
ffff8000100f2734:       14000004        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2738:       14000001        b       ffff8000100f273c <__ptrace_link+0x3c>
ffff8000100f273c:       14000006        b       ffff8000100f2754 <__ptrace_link+0x54>
ffff8000100f2740:       14000001        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2744:       52800028        mov     w8, #0x1                        // #1
ffff8000100f2748:       b828005f        stadd   w8, [x2]
ffff8000100f274c:       f9030002        str     x2, [x0, #1536]
ffff8000100f2750:       d65f03c0        ret
ffff8000100f2754:       140007fd        b       ffff8000100f4748 <ptrace_check_attach+0xf8>

Quoted 5 lines

...

This looks like the default path (before we write over it) will take you to the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at least not what we expected to see. Also why 4 branches?

So I reproduced this with a silly atomic_inc wrapper:

void will_atomic_inc(atomic_t *v)

{
        atomic_inc(v);
}

Compiles to:

0000000000000018 <will_atomic_inc>:

  18: 14000004        b       28 <will_atomic_inc+0x10>
  1c: 14000001        b       20 <will_atomic_inc+0x8>
  20: 14000005        b       34 <will_atomic_inc+0x1c>
  24: 14000001        b       28 <will_atomic_inc+0x10>
  28: 52800028        mov     w8, #0x1                        // #1
  2c: b828001f        stadd   w8, [x0]
  30: d65f03c0        ret
  34: 14000027        b       d0 <dump_kernel_offset+0x60>
  38: d65f03c0        ret

Quoted 1 line

which is going to explode.

I've come up with a simple reproducer for this issue:

static bool branch_jump()

{
        asm_volatile_goto(
                "1: b %l[l_yes2]"
                 : : : : l_yes2);

        return false;
l_yes2:
        return true;
}

static bool branch_test()

{
        return (!branch_jump() && !branch_jump());
}

void andy_test(int *v)

{
        if (branch_test())
                *v = 0xff;
}

This leads to the following (it shouldn't do anything):

0000000000000000 <andy_test>:

   0:   14000004        b       10 <andy_test+0x10>
   4:   14000001        b       8 <andy_test+0x8>
   8:   14000004        b       18 <andy_test+0x18>
   c:   14000001        b       10 <andy_test+0x10>
  10:   52801fe8        mov     w8, #0xff                       // #255
  14:   b9000008        str     w8, [x0]
  18:   d65f03c0        ret

The issue goes away with any of the following hunks:

(quoted diff) +1 -1

@@ -55,7 +55,7 @@ static bool branch_jump()
 
 static bool branch_test()
 {
-       return (!branch_jump() && !branch_jump());
+       return (!branch_jump());
 }
 
 void andy_test(int *v)

or:

(quoted diff) +0 -4

@@ -53,14 +53,10 @@ static bool branch_jump()
         return true;
 }
 
-static bool branch_test()
-{
-       return (!branch_jump() && !branch_jump());
-}
 

Review static bool branch_jump() · line 61

Quoted 6 lines

(quoted diff) +1 -1

 void andy_test(int *v)
 {
-       if (branch_test())
+       if (!branch_jump() && !branch_jump())
                *v = 0xff;
 }

Indeed, playing with the definition of __lse_ll_sc_body, I can get the kernel to boot again.

So I think your very helpful test cases are illustrating two different problems:
https://godbolt.org/z/dMf7x-
See the disassembly of andy_test2. Reference to the correct label is emitted in the inline asm, but there's some silly unconditional branches to the next instruction. That's issue #1 and part of the reason you see superfluous branches. With that fixed, andy_test2 would match between GCC and Clang. I think that can be a very late peephole optimization (and further, we could probably combine labels that refer to the same location, oh and .Lfunc_endX could just use ., too!). LLVM devs noted that the x86 backend doesn't have this issue, but this is a curiously recurring pattern I'm noticing in LLVM where some arch agnostic optimization is only implemented for x86... I'm reading through our Branch Folding pass which I think should handle this, but I'll need to fire up a debugger.

Issue #2 is the more critical issue, but may be conflated with issue #1. Issue #2 is the nonsensical control flow with one level of inlining. See how in the disassembly of andy_test, the first label referenced from inline assembly is *before* the mov/str when it should have been *after*. Not sure where we could be going wrong, but it's straightforward for me to observe the code change as its transformed through LLVM, and I've debugged and fixed issues related to inlining asm goto before.

Signature · 2 lines

Thanks,
~Nick Desaulniers

Quoted 125 lines · Nick Desaulniers

Quoted 124 lines · Andrew Murray

Quoted 59 lines · Will Deacon

Quoted 32 lines · Andrew Murray

Quoted 5 lines · Andrew Murray

Quoted 2 lines · Will Deacon

Does it work if the only thing you change is the toolchain, and use GCC instead?

Yup.

Also this is Clang generation:

ffff8000100f2700 <__ptrace_link>:

ffff8000100f2700:       f9426009        ldr     x9, [x0, #1216]
ffff8000100f2704:       91130008        add     x8, x0, #0x4c0
ffff8000100f2708:       eb09011f        cmp     x8, x9
ffff8000100f270c:       540002a1        b.ne    ffff8000100f2760 <__ptrace_link+0x60>  // b.any
ffff8000100f2710:       f9425829        ldr     x9, [x1, #1200]
ffff8000100f2714:       9112c02a        add     x10, x1, #0x4b0
ffff8000100f2718:       f9000528        str     x8, [x9, #8]
ffff8000100f271c:       f9026009        str     x9, [x0, #1216]
ffff8000100f2720:       f902640a        str     x10, [x0, #1224]
ffff8000100f2724:       f9025828        str     x8, [x1, #1200]
ffff8000100f2728:       f9024001        str     x1, [x0, #1152]
ffff8000100f272c:       b4000162        cbz     x2, ffff8000100f2758 <__ptrace_link+0x58>
ffff8000100f2730:       b900985f        str     wzr, [x2, #152]
ffff8000100f2734:       14000004        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2738:       14000001        b       ffff8000100f273c <__ptrace_link+0x3c>
ffff8000100f273c:       14000006        b       ffff8000100f2754 <__ptrace_link+0x54>
ffff8000100f2740:       14000001        b       ffff8000100f2744 <__ptrace_link+0x44>
ffff8000100f2744:       52800028        mov     w8, #0x1                        // #1
ffff8000100f2748:       b828005f        stadd   w8, [x2]
ffff8000100f274c:       f9030002        str     x2, [x0, #1536]
ffff8000100f2750:       d65f03c0        ret
ffff8000100f2754:       140007fd        b       ffff8000100f4748 <ptrace_check_attach+0xf8>

Quoted 5 lines

...

This looks like the default path (before we write over it) will take you to the LSE code (e.g. ffff8000100f2734). I'm pretty sure this is wrong, or at least not what we expected to see. Also why 4 branches?

So I reproduced this with a silly atomic_inc wrapper:

void will_atomic_inc(atomic_t *v)

{
        atomic_inc(v);
}

Compiles to:

0000000000000018 <will_atomic_inc>:

  18: 14000004        b       28 <will_atomic_inc+0x10>
  1c: 14000001        b       20 <will_atomic_inc+0x8>
  20: 14000005        b       34 <will_atomic_inc+0x1c>
  24: 14000001        b       28 <will_atomic_inc+0x10>
  28: 52800028        mov     w8, #0x1                        // #1
  2c: b828001f        stadd   w8, [x0]
  30: d65f03c0        ret
  34: 14000027        b       d0 <dump_kernel_offset+0x60>
  38: d65f03c0        ret

Quoted 1 line

which is going to explode.

I've come up with a simple reproducer for this issue:

static bool branch_jump()

{
        asm_volatile_goto(
                "1: b %l[l_yes2]"
                 : : : : l_yes2);

        return false;
l_yes2:
        return true;
}

static bool branch_test()

{
        return (!branch_jump() && !branch_jump());
}

void andy_test(int *v)

{
        if (branch_test())
                *v = 0xff;
}

This leads to the following (it shouldn't do anything):

0000000000000000 <andy_test>:

   0:   14000004        b       10 <andy_test+0x10>
   4:   14000001        b       8 <andy_test+0x8>
   8:   14000004        b       18 <andy_test+0x18>
   c:   14000001        b       10 <andy_test+0x10>
  10:   52801fe8        mov     w8, #0xff                       // #255
  14:   b9000008        str     w8, [x0]
  18:   d65f03c0        ret

The issue goes away with any of the following hunks:

(quoted diff) +1 -1

@@ -55,7 +55,7 @@ static bool branch_jump()
 
 static bool branch_test()
 {
-       return (!branch_jump() && !branch_jump());
+       return (!branch_jump());
 }
 
 void andy_test(int *v)

or:

(quoted diff) +0 -4

@@ -53,14 +53,10 @@ static bool branch_jump()
         return true;
 }
 
-static bool branch_test()
-{
-       return (!branch_jump() && !branch_jump());
-}
 

Quoted 9 lines

Review static bool branch_jump() · line 61

Quoted 6 lines

(quoted diff) +1 -1

 void andy_test(int *v)
 {
-       if (branch_test())
+       if (!branch_jump() && !branch_jump())
                *v = 0xff;
 }

Indeed, playing with the definition of __lse_ll_sc_body, I can get the kernel to boot again.

Thanks for investigating this.

Did it boot to a prompt? I played with the structure of the code and too was able to get it to boot, but I found that it hung later-on during boot. Thus I lost a bit of confidence in it.

Quoted 16 lines

So I think your very helpful test cases are illustrating two different problems:
https://godbolt.org/z/dMf7x-
See the disassembly of andy_test2. Reference to the correct label is emitted in the inline asm, but there's some silly unconditional branches to the next instruction. That's issue #1 and part of the reason you see superfluous branches. With that fixed, andy_test2 would match between GCC and Clang. I think that can be a very late peephole optimization (and further, we could probably combine labels that refer to the same location, oh and .Lfunc_endX could just use ., too!). LLVM devs noted that the x86 backend doesn't have this issue, but this is a curiously recurring pattern I'm noticing in LLVM where some arch agnostic optimization is only implemented for x86... I'm reading through our Branch Folding pass which I think should handle this, but I'll need to fire up a debugger.

Quoted 8 lines

Issue #2 is the more critical issue, but may be conflated with issue #1. Issue #2 is the nonsensical control flow with one level of inlining. See how in the disassembly of andy_test, the first label referenced from inline assembly is *before* the mov/str when it should have been *after*. Not sure where we could be going wrong, but it's straightforward for me to observe the code change as its transformed through LLVM, and I've debugged and fixed issues related to inlining asm goto before.

You may also be interested in this:

https://godbolt.org/z/8OthP2

void andy_test3(int *v)

{
    if (!branch_jump())
        return;

    if (!branch_jump())
        return;

    *v = 0xff;
}

(I used a similar approach with system_uses_lse_atomics to get the kernel to boot a bit more).

This generated code does the right thing here (in comparison to andy_test2). I felt like this gave an insight as to what is going on, but I don't have the knowledge to know what. It's as if the early return prevents the compiler from getting confused when it should otherwise jump to the second goto.

From: Andrew Murray <[email protected]>

We no longer fall back to out-of-line atomics on systems with CONFIG_ARM64_LSE_ATOMICS where ARM64_HAS_LSE_ATOMICS is not set.

Remove the unused compilation unit which provided these symbols.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/lib/Makefile       | 19 -------------------
 arch/arm64/lib/atomic_ll_sc.c |  3 ---
 2 files changed, 22 deletions(-)
 delete mode 100644 arch/arm64/lib/atomic_ll_sc.c

arch/arm64/lib/Makefile +0 -3

index 33c2a4abda04..f10809ef1690 100644
@@ -11,25 +11,6 @@ CFLAGS_REMOVE_xor-neon.o     += -mgeneral-regs-only
 CFLAGS_xor-neon.o              += -ffreestanding
 endif
 
-# Tell the compiler to treat all general purpose registers (with the
-# exception of the IP registers, which are already handled by the caller
-# in case of a PLT) as callee-saved, which allows for efficient runtime

arch/arm64/lib/Makefile +0 -6

-CFLAGS_REMOVE_atomic_ll_sc.o := $(CC_FLAGS_FTRACE)
-GCOV_PROFILE_atomic_ll_sc.o    := n
-KASAN_SANITIZE_atomic_ll_sc.o  := n
-KCOV_INSTRUMENT_atomic_ll_sc.o := n
-UBSAN_SANITIZE_atomic_ll_sc.o  := n
-
 lib-$(CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE) += uaccess_flushcache.o
 
 obj-$(CONFIG_CRC32) += crc32.o

arch/arm64/lib/atomic_ll_sc.c deleted +0 -4

deleted file mode 100644
index b0c538b0da28..000000000000
@@ -1,3 +0,0 @@
-#include <asm/atomic.h>
-#define __ARM64_IN_ATOMIC_IMPL
-#include <asm/atomic_ll_sc.h>
--

2.11.0

Quoted 24 lines · Will Deacon

From: Andrew Murray <[email protected]>

We no longer fall back to out-of-line atomics on systems with CONFIG_ARM64_LSE_ATOMICS where ARM64_HAS_LSE_ATOMICS is not set.

Remove the unused compilation unit which provided these symbols.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/lib/Makefile       | 19 -------------------
 arch/arm64/lib/atomic_ll_sc.c |  3 ---
 2 files changed, 22 deletions(-)
 delete mode 100644 arch/arm64/lib/atomic_ll_sc.c

arch/arm64/lib/Makefile +0 -0

index 33c2a4abda04..f10809ef1690 100644
@@ -11,25 +11,6 @@ CFLAGS_REMOVE_xor-neon.o     += -mgeneral-regs-only
 CFLAGS_xor-neon.o              += -ffreestanding
 endif
 

arch/arm64/lib/Makefile CFLAGS_REMOVE_xor-neon.o += -mgeneral-regs-only · line 26

Quoted 13 lines

arch/arm64/lib/Makefile +0 -13

-# Tell the compiler to treat all general purpose registers (with the
-# exception of the IP registers, which are already handled by the caller
-# in case of a PLT) as callee-saved, which allows for efficient runtime
-# patching of the bl instruction in the caller with an atomic instruction
-# when supported by the CPU. Result and argument registers are handled
-# correctly, based on the function prototype.
-lib-$(CONFIG_ARM64_LSE_ATOMICS) += atomic_ll_sc.o
-CFLAGS_atomic_ll_sc.o  := -ffixed-x1 -ffixed-x2                        \
-                  -ffixed-x3 -ffixed-x4 -ffixed-x5 -ffixed-x6          \
-                  -ffixed-x7 -fcall-saved-x8 -fcall-saved-x9           \
-                  -fcall-saved-x10 -fcall-saved-x11 -fcall-saved-x12   \
-                  -fcall-saved-x13 -fcall-saved-x14 -fcall-saved-x15   \
-                  -fcall-saved-x18 -fomit-frame-pointer

+ Tri (who implemented support for -fcall-saved-x*, -ffixed-x* in Clang). I won't be sad to see the use of these flags go.

Quoted 26 lines · Nick Desaulniers

Quoted 24 lines · Will Deacon

From: Andrew Murray <[email protected]>

We no longer fall back to out-of-line atomics on systems with CONFIG_ARM64_LSE_ATOMICS where ARM64_HAS_LSE_ATOMICS is not set.

Remove the unused compilation unit which provided these symbols.

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/lib/Makefile       | 19 -------------------
 arch/arm64/lib/atomic_ll_sc.c |  3 ---
 2 files changed, 22 deletions(-)
 delete mode 100644 arch/arm64/lib/atomic_ll_sc.c

arch/arm64/lib/Makefile +0 -0

index 33c2a4abda04..f10809ef1690 100644
@@ -11,25 +11,6 @@ CFLAGS_REMOVE_xor-neon.o     += -mgeneral-regs-only
 CFLAGS_xor-neon.o              += -ffreestanding
 endif
 

Quoted 16 lines

arch/arm64/lib/Makefile CFLAGS_REMOVE_xor-neon.o += -mgeneral-regs-only · line 26

Quoted 13 lines

arch/arm64/lib/Makefile +0 -13

-# Tell the compiler to treat all general purpose registers (with the
-# exception of the IP registers, which are already handled by the caller
-# in case of a PLT) as callee-saved, which allows for efficient runtime
-# patching of the bl instruction in the caller with an atomic instruction
-# when supported by the CPU. Result and argument registers are handled
-# correctly, based on the function prototype.
-lib-$(CONFIG_ARM64_LSE_ATOMICS) += atomic_ll_sc.o
-CFLAGS_atomic_ll_sc.o  := -ffixed-x1 -ffixed-x2                        \
-                  -ffixed-x3 -ffixed-x4 -ffixed-x5 -ffixed-x6          \
-                  -ffixed-x7 -fcall-saved-x8 -fcall-saved-x9           \
-                  -fcall-saved-x10 -fcall-saved-x11 -fcall-saved-x12   \
-                  -fcall-saved-x13 -fcall-saved-x14 -fcall-saved-x15   \
-                  -fcall-saved-x18 -fomit-frame-pointer

+ Tri (who implemented support for -fcall-saved-x*, -ffixed-x* in Clang). I won't be sad to see the use of these flags go.

Nice! IMO these flags made the code hard to read.

The 'alt_lse' assembly macro has been unused since 7c8fc35dfc32 ("locking/atomics/arm64: Replace our atomic/lock bitop implementations with asm-generic").

Remove it.

Signed-off-by Will Deacon <[email protected]>

---

 arch/arm64/include/asm/lse.h | 22 ----------------------
 1 file changed, 22 deletions(-)

arch/arm64/include/asm/lse.h +0 -22

index 52b80846d1b7..08e818e53ed7 100644
@@ -10,37 +10,15 @@
 #include <asm/alternative.h>
 #include <asm/cpucaps.h>
 
-#ifdef __ASSEMBLER__
-
-.arch_extension	lse
-
-.macro alt_lse, llsc, lse
-	alternative_insn "\llsc", "\lse", ARM64_HAS_LSE_ATOMICS
-.endm
-
-#else	/* __ASSEMBLER__ */
-
 __asm__(".arch_extension	lse");
 
-
 /* In-line patching at runtime */
 #define ARM64_LSE_ATOMIC_INSN(llsc, lse)				\
 	ALTERNATIVE(llsc, lse, ARM64_HAS_LSE_ATOMICS)
 
-#endif	/* __ASSEMBLER__ */
 #else	/* CONFIG_AS_LSE && CONFIG_ARM64_LSE_ATOMICS */
 
-#ifdef __ASSEMBLER__
-
-.macro alt_lse, llsc, lse
-	\llsc
-.endm
-
-#else	/* __ASSEMBLER__ */
-
-
 #define ARM64_LSE_ATOMIC_INSN(llsc, lse)	llsc
 
-#endif	/* __ASSEMBLER__ */
 #endif	/* CONFIG_AS_LSE && CONFIG_ARM64_LSE_ATOMICS */
 #endif	/* __ASM_LSE_H */

Signature · 1 line

2.11.0

The contents of 'asm/atomic_arch.h' can be split across some of our other 'asm/' headers. Remove it.

Signed-off-by Will Deacon <[email protected]>

---

 arch/arm64/include/asm/atomic.h      |  77 ++++++++++++++++-
 arch/arm64/include/asm/atomic_arch.h | 155 -----------------------------------
 arch/arm64/include/asm/cmpxchg.h     |  41 ++++++++-
 arch/arm64/include/asm/lse.h         |  24 ++++++
 4 files changed, 140 insertions(+), 157 deletions(-)
 delete mode 100644 arch/arm64/include/asm/atomic_arch.h

arch/arm64/include/asm/atomic.h +76 -1

index c70d3f389d29..7c334337674d 100644
@@ -17,9 +17,84 @@
 
 #ifdef __KERNEL__
 
-#include <asm/atomic_arch.h>
 #include <asm/cmpxchg.h>
 
+#define ATOMIC_OP(op)							\
+static inline void arch_##op(int i, atomic_t *v)			\
+{									\
+	__lse_ll_sc_body(op, i, v);					\
+}
+
+ATOMIC_OP(atomic_andnot)
+ATOMIC_OP(atomic_or)
+ATOMIC_OP(atomic_xor)
+ATOMIC_OP(atomic_add)
+ATOMIC_OP(atomic_and)
+ATOMIC_OP(atomic_sub)
+
+
+#define ATOMIC_FETCH_OP(name, op)					\
+static inline int arch_##op##name(int i, atomic_t *v)			\
+{									\
+	return __lse_ll_sc_body(op##name, i, v);			\
+}
+
+#define ATOMIC_FETCH_OPS(op)						\
+	ATOMIC_FETCH_OP(_relaxed, op)					\
+	ATOMIC_FETCH_OP(_acquire, op)					\
+	ATOMIC_FETCH_OP(_release, op)					\
+	ATOMIC_FETCH_OP(        , op)
+
+ATOMIC_FETCH_OPS(atomic_fetch_andnot)
+ATOMIC_FETCH_OPS(atomic_fetch_or)
+ATOMIC_FETCH_OPS(atomic_fetch_xor)
+ATOMIC_FETCH_OPS(atomic_fetch_add)
+ATOMIC_FETCH_OPS(atomic_fetch_and)
+ATOMIC_FETCH_OPS(atomic_fetch_sub)
+ATOMIC_FETCH_OPS(atomic_add_return)
+ATOMIC_FETCH_OPS(atomic_sub_return)
+
+
+#define ATOMIC64_OP(op)							\
+static inline void arch_##op(long i, atomic64_t *v)			\
+{									\
+	__lse_ll_sc_body(op, i, v);					\
+}
+
+ATOMIC64_OP(atomic64_andnot)
+ATOMIC64_OP(atomic64_or)
+ATOMIC64_OP(atomic64_xor)
+ATOMIC64_OP(atomic64_add)
+ATOMIC64_OP(atomic64_and)
+ATOMIC64_OP(atomic64_sub)
+
+
+#define ATOMIC64_FETCH_OP(name, op)					\
+static inline long arch_##op##name(long i, atomic64_t *v)		\
+{									\
+	return __lse_ll_sc_body(op##name, i, v);			\
+}
+
+#define ATOMIC64_FETCH_OPS(op)						\
+	ATOMIC64_FETCH_OP(_relaxed, op)					\
+	ATOMIC64_FETCH_OP(_acquire, op)					\
+	ATOMIC64_FETCH_OP(_release, op)					\
+	ATOMIC64_FETCH_OP(        , op)
+
+ATOMIC64_FETCH_OPS(atomic64_fetch_andnot)
+ATOMIC64_FETCH_OPS(atomic64_fetch_or)
+ATOMIC64_FETCH_OPS(atomic64_fetch_xor)
+ATOMIC64_FETCH_OPS(atomic64_fetch_add)
+ATOMIC64_FETCH_OPS(atomic64_fetch_and)
+ATOMIC64_FETCH_OPS(atomic64_fetch_sub)
+ATOMIC64_FETCH_OPS(atomic64_add_return)
+ATOMIC64_FETCH_OPS(atomic64_sub_return)
+
+static inline long arch_atomic64_dec_if_positive(atomic64_t *v)
+{
+	return __lse_ll_sc_body(atomic64_dec_if_positive, v);
+}
+
 #define ATOMIC_INIT(i)	{ (i) }
 
 #define arch_atomic_read(v)			READ_ONCE((v)->counter)

arch/arm64/include/asm/atomic_arch.h deleted +0 -155

deleted file mode 100644
index 1aac7fc65084..000000000000
@@ -1,155 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Selection between LSE and LL/SC atomics.
- *
- * Copyright (C) 2018 ARM Ltd.
- * Author: Andrew Murray <[email protected]>
- */
-
-#ifndef __ASM_ATOMIC_ARCH_H
-#define __ASM_ATOMIC_ARCH_H
-
-
-#include <linux/jump_label.h>
-
-#include <asm/cpucaps.h>
-#include <asm/atomic_ll_sc.h>
-#include <asm/atomic_lse.h>
-
-extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS];
-extern struct static_key_false arm64_const_caps_ready;
-
-static inline bool system_uses_lse_atomics(void)
-{
-	return (IS_ENABLED(CONFIG_ARM64_LSE_ATOMICS) &&
-		IS_ENABLED(CONFIG_AS_LSE) &&
-		static_branch_likely(&arm64_const_caps_ready)) &&
-		static_branch_likely(&cpu_hwcap_keys[ARM64_HAS_LSE_ATOMICS]);
-}
-
-#define __lse_ll_sc_body(op, ...)					\
-({									\
-	system_uses_lse_atomics() ?					\
-		__lse_##op(__VA_ARGS__) :				\
-		__ll_sc_##op(__VA_ARGS__);				\
-})
-
-#define ATOMIC_OP(op)							\
-static inline void arch_##op(int i, atomic_t *v)			\
-{									\
-	__lse_ll_sc_body(op, i, v);					\
-}
-
-ATOMIC_OP(atomic_andnot)
-ATOMIC_OP(atomic_or)
-ATOMIC_OP(atomic_xor)
-ATOMIC_OP(atomic_add)
-ATOMIC_OP(atomic_and)
-ATOMIC_OP(atomic_sub)
-
-
-#define ATOMIC_FETCH_OP(name, op)					\
-static inline int arch_##op##name(int i, atomic_t *v)			\
-{									\
-	return __lse_ll_sc_body(op##name, i, v);			\
-}
-
-#define ATOMIC_FETCH_OPS(op)						\
-	ATOMIC_FETCH_OP(_relaxed, op)					\
-	ATOMIC_FETCH_OP(_acquire, op)					\
-	ATOMIC_FETCH_OP(_release, op)					\
-	ATOMIC_FETCH_OP(        , op)
-
-ATOMIC_FETCH_OPS(atomic_fetch_andnot)
-ATOMIC_FETCH_OPS(atomic_fetch_or)
-ATOMIC_FETCH_OPS(atomic_fetch_xor)
-ATOMIC_FETCH_OPS(atomic_fetch_add)
-ATOMIC_FETCH_OPS(atomic_fetch_and)
-ATOMIC_FETCH_OPS(atomic_fetch_sub)
-ATOMIC_FETCH_OPS(atomic_add_return)
-ATOMIC_FETCH_OPS(atomic_sub_return)
-
-
-#define ATOMIC64_OP(op)							\
-static inline void arch_##op(long i, atomic64_t *v)			\
-{									\
-	__lse_ll_sc_body(op, i, v);					\
-}
-
-ATOMIC64_OP(atomic64_andnot)
-ATOMIC64_OP(atomic64_or)
-ATOMIC64_OP(atomic64_xor)
-ATOMIC64_OP(atomic64_add)
-ATOMIC64_OP(atomic64_and)
-ATOMIC64_OP(atomic64_sub)
-
-
-#define ATOMIC64_FETCH_OP(name, op)					\
-static inline long arch_##op##name(long i, atomic64_t *v)		\
-{									\
-	return __lse_ll_sc_body(op##name, i, v);			\
-}
-
-#define ATOMIC64_FETCH_OPS(op)						\
-	ATOMIC64_FETCH_OP(_relaxed, op)					\
-	ATOMIC64_FETCH_OP(_acquire, op)					\
-	ATOMIC64_FETCH_OP(_release, op)					\
-	ATOMIC64_FETCH_OP(        , op)
-
-ATOMIC64_FETCH_OPS(atomic64_fetch_andnot)
-ATOMIC64_FETCH_OPS(atomic64_fetch_or)
-ATOMIC64_FETCH_OPS(atomic64_fetch_xor)
-ATOMIC64_FETCH_OPS(atomic64_fetch_add)
-ATOMIC64_FETCH_OPS(atomic64_fetch_and)
-ATOMIC64_FETCH_OPS(atomic64_fetch_sub)
-ATOMIC64_FETCH_OPS(atomic64_add_return)
-ATOMIC64_FETCH_OPS(atomic64_sub_return)
-
-
-static inline long arch_atomic64_dec_if_positive(atomic64_t *v)
-{
-	return __lse_ll_sc_body(atomic64_dec_if_positive, v);
-}
-
-#define __CMPXCHG_CASE(name, sz)			\
-static inline u##sz __cmpxchg_case_##name##sz(volatile void *ptr,	\
-					      u##sz old,		\
-					      u##sz new)		\
-{									\
-	return __lse_ll_sc_body(_cmpxchg_case_##name##sz,		\
-				ptr, old, new);				\
-}
-
-__CMPXCHG_CASE(    ,  8)
-__CMPXCHG_CASE(    , 16)
-__CMPXCHG_CASE(    , 32)
-__CMPXCHG_CASE(    , 64)
-__CMPXCHG_CASE(acq_,  8)
-__CMPXCHG_CASE(acq_, 16)
-__CMPXCHG_CASE(acq_, 32)
-__CMPXCHG_CASE(acq_, 64)
-__CMPXCHG_CASE(rel_,  8)
-__CMPXCHG_CASE(rel_, 16)
-__CMPXCHG_CASE(rel_, 32)
-__CMPXCHG_CASE(rel_, 64)
-__CMPXCHG_CASE(mb_,  8)
-__CMPXCHG_CASE(mb_, 16)
-__CMPXCHG_CASE(mb_, 32)
-__CMPXCHG_CASE(mb_, 64)
-
-
-#define __CMPXCHG_DBL(name)						\
-static inline long __cmpxchg_double##name(unsigned long old1,		\
-					 unsigned long old2,		\
-					 unsigned long new1,		\
-					 unsigned long new2,		\
-					 volatile void *ptr)		\
-{									\
-	return __lse_ll_sc_body(_cmpxchg_double##name, 			\
-				old1, old2, new1, new2, ptr);		\
-}
-
-__CMPXCHG_DBL(   )
-__CMPXCHG_DBL(_mb)
-
-#endif	/* __ASM_ATOMIC_LSE_H */

arch/arm64/include/asm/cmpxchg.h +40 -1

index e5fff8cd4904..afaba73e0b2c 100644
@@ -10,7 +10,6 @@
 #include <linux/build_bug.h>
 #include <linux/compiler.h>
 
-#include <asm/atomic_arch.h>
 #include <asm/barrier.h>
 #include <asm/lse.h>
 

@@ -104,6 +103,46 @@ __XCHG_GEN(_mb)
 #define arch_xchg_release(...)	__xchg_wrapper(_rel, __VA_ARGS__)
 #define arch_xchg(...)		__xchg_wrapper( _mb, __VA_ARGS__)
 
+#define __CMPXCHG_CASE(name, sz)			\
+static inline u##sz __cmpxchg_case_##name##sz(volatile void *ptr,	\
+					      u##sz old,		\
+					      u##sz new)		\
+{									\
+	return __lse_ll_sc_body(_cmpxchg_case_##name##sz,		\
+				ptr, old, new);				\
+}
+
+__CMPXCHG_CASE(    ,  8)
+__CMPXCHG_CASE(    , 16)
+__CMPXCHG_CASE(    , 32)
+__CMPXCHG_CASE(    , 64)
+__CMPXCHG_CASE(acq_,  8)
+__CMPXCHG_CASE(acq_, 16)
+__CMPXCHG_CASE(acq_, 32)
+__CMPXCHG_CASE(acq_, 64)
+__CMPXCHG_CASE(rel_,  8)
+__CMPXCHG_CASE(rel_, 16)
+__CMPXCHG_CASE(rel_, 32)
+__CMPXCHG_CASE(rel_, 64)
+__CMPXCHG_CASE(mb_,  8)
+__CMPXCHG_CASE(mb_, 16)
+__CMPXCHG_CASE(mb_, 32)
+__CMPXCHG_CASE(mb_, 64)
+
+#define __CMPXCHG_DBL(name)						\
+static inline long __cmpxchg_double##name(unsigned long old1,		\
+					 unsigned long old2,		\
+					 unsigned long new1,		\
+					 unsigned long new2,		\
+					 volatile void *ptr)		\
+{									\
+	return __lse_ll_sc_body(_cmpxchg_double##name, 			\
+				old1, old2, new1, new2, ptr);		\
+}
+
+__CMPXCHG_DBL(   )
+__CMPXCHG_DBL(_mb)
+
 #define __CMPXCHG_GEN(sfx)						\
 static inline unsigned long __cmpxchg##sfx(volatile void *ptr,		\
 					   unsigned long old,		\

arch/arm64/include/asm/lse.h +24 -0

index 08e818e53ed7..80b388278149 100644
@@ -2,22 +2,46 @@
 #ifndef __ASM_LSE_H
 #define __ASM_LSE_H
 
+#include <asm/atomic_ll_sc.h>
+
 #if defined(CONFIG_AS_LSE) && defined(CONFIG_ARM64_LSE_ATOMICS)
 
 #include <linux/compiler_types.h>
 #include <linux/export.h>
+#include <linux/jump_label.h>
 #include <linux/stringify.h>
 #include <asm/alternative.h>
+#include <asm/atomic_lse.h>
 #include <asm/cpucaps.h>
 
 __asm__(".arch_extension	lse");
 
+extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS];
+extern struct static_key_false arm64_const_caps_ready;
+
+static inline bool system_uses_lse_atomics(void)
+{
+	return (static_branch_likely(&arm64_const_caps_ready)) &&
+		static_branch_likely(&cpu_hwcap_keys[ARM64_HAS_LSE_ATOMICS]);
+}
+
+#define __lse_ll_sc_body(op, ...)					\
+({									\
+	system_uses_lse_atomics() ?					\
+		__lse_##op(__VA_ARGS__) :				\
+		__ll_sc_##op(__VA_ARGS__);				\
+})
+
 /* In-line patching at runtime */
 #define ARM64_LSE_ATOMIC_INSN(llsc, lse)				\
 	ALTERNATIVE(llsc, lse, ARM64_HAS_LSE_ATOMICS)
 
 #else	/* CONFIG_AS_LSE && CONFIG_ARM64_LSE_ATOMICS */
 
+static inline bool system_uses_lse_atomics(void) { return false; }
+
+#define __lse_ll_sc_body(op, ...)		__ll_sc_##op(__VA_ARGS__)
+
 #define ARM64_LSE_ATOMIC_INSN(llsc, lse)	llsc
 
 #endif	/* CONFIG_AS_LSE && CONFIG_ARM64_LSE_ATOMICS */

Signature · 1 line

2.11.0

Support for LSE atomic instructions (CONFIG_ARM64_LSE_ATOMICS) relies on a static key to select between the legacy LL/SC implementation which is available on all arm64 CPUs and the super-duper LSE implementation which is available on CPUs implementing v8.1 and later.

Unfortunately, when building a kernel with CONFIG_JUMP_LABEL disabled (e.g. because the toolchain doesn't support 'asm goto'), the static key inside the atomics code tries to use atomics itself. This results in a mess of circular includes and a build failure:

In file included from ./arch/arm64/include/asm/lse.h:11,

                 from ./arch/arm64/include/asm/atomic.h:16,
                 from ./include/linux/atomic.h:7,
                 from ./include/asm-generic/bitops/atomic.h:5,
                 from ./arch/arm64/include/asm/bitops.h:26,
                 from ./include/linux/bitops.h:19,
                 from ./include/linux/kernel.h:12,
                 from ./include/asm-generic/bug.h:18,
                 from ./arch/arm64/include/asm/bug.h:26,
                 from ./include/linux/bug.h:5,
                 from ./include/linux/page-flags.h:10,
                 from kernel/bounds.c:10:
./include/linux/jump_label.h: In function ‘static_key_count’:
./include/linux/jump_label.h:254:9: error: implicit declaration of function ‘atomic_read’ [-Werror=implicit-function-declaration]
  return atomic_read(&key->enabled);
         ^~~~~~~~~~~

[ ... more of the same ... ]

Since LSE atomic instructions are not critical to the operation of the kernel, make them depend on JUMP_LABEL at compile time.

Signed-off-by Will Deacon <[email protected]>

---

 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

arch/arm64/Kconfig +1 -0

index 3adcec05b1f6..27405ac94228 100644
@@ -1263,6 +1263,7 @@ config ARM64_PAN
 
 config ARM64_LSE_ATOMICS
 	bool "Atomic instructions"
+	depends on JUMP_LABEL
 	default y
 	help
 	  As part of the Large System Extensions, ARMv8.1 introduces new

Signature · 1 line

2.11.0

We use a bunch of internal macros when constructing our atomic and cmpxchg routines in order to save on boilerplate. Avoid exposing these directly to users of the header files.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/atomic.h  | 7 +++++++
 arch/arm64/include/asm/cmpxchg.h | 4 ++++
 2 files changed, 11 insertions(+)

arch/arm64/include/asm/atomic.h +7 -0

index 7c334337674d..916e5a6d5454 100644
@@ -32,6 +32,7 @@ ATOMIC_OP(atomic_add)
 ATOMIC_OP(atomic_and)
 ATOMIC_OP(atomic_sub)
 
+#undef ATOMIC_OP
 
 #define ATOMIC_FETCH_OP(name, op)					\
 static inline int arch_##op##name(int i, atomic_t *v)			\

@@ -54,6 +55,8 @@ ATOMIC_FETCH_OPS(atomic_fetch_sub)
 ATOMIC_FETCH_OPS(atomic_add_return)
 ATOMIC_FETCH_OPS(atomic_sub_return)
 
+#undef ATOMIC_FETCH_OP
+#undef ATOMIC_FETCH_OPS
 
 #define ATOMIC64_OP(op)							\
 static inline void arch_##op(long i, atomic64_t *v)			\

@@ -68,6 +71,7 @@ ATOMIC64_OP(atomic64_add)
 ATOMIC64_OP(atomic64_and)
 ATOMIC64_OP(atomic64_sub)
 
+#undef ATOMIC64_OP
 
 #define ATOMIC64_FETCH_OP(name, op)					\
 static inline long arch_##op##name(long i, atomic64_t *v)		\

@@ -90,6 +94,9 @@ ATOMIC64_FETCH_OPS(atomic64_fetch_sub)
 ATOMIC64_FETCH_OPS(atomic64_add_return)
 ATOMIC64_FETCH_OPS(atomic64_sub_return)
 
+#undef ATOMIC64_FETCH_OP
+#undef ATOMIC64_FETCH_OPS
+
 static inline long arch_atomic64_dec_if_positive(atomic64_t *v)
 {
 	return __lse_ll_sc_body(atomic64_dec_if_positive, v);

arch/arm64/include/asm/cmpxchg.h +4 -0

index afaba73e0b2c..a1398f2f9994 100644
@@ -129,6 +129,8 @@ __CMPXCHG_CASE(mb_, 16)
 __CMPXCHG_CASE(mb_, 32)
 __CMPXCHG_CASE(mb_, 64)
 
+#undef __CMPXCHG_CASE
+
 #define __CMPXCHG_DBL(name)						\
 static inline long __cmpxchg_double##name(unsigned long old1,		\
 					 unsigned long old2,		\

@@ -143,6 +145,8 @@ static inline long __cmpxchg_double##name(unsigned long old1,		\
 __CMPXCHG_DBL(   )
 __CMPXCHG_DBL(_mb)
 
+#undef __CMPXCHG_DBL
+
 #define __CMPXCHG_GEN(sfx)						\
 static inline unsigned long __cmpxchg##sfx(volatile void *ptr,		\
 					   unsigned long old,		\

Signature · 1 line

2.11.0

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"                                                               \
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Bah, I need to use something like __stringify when the constraint is used in order for this to get expanded properly. Updated diff below.

Will

--->8

arch/arm64/Makefile +8 -1

index 61de992bbea3..0cef056b5fb1 100644
@@ -39,6 +39,12 @@ $(warning LSE atomics not supported by binutils)
   endif
 endif
 
+cc_has_k_constraint := $(call try-run,echo                             \
+       'int main(void) {                                               \
+               asm volatile("and w0, w0, %w0" :: "K" (4294967295));    \
+               return 0;                                               \
+       }' | $(CC) -S -x c -o "$$TMP" -,,-DCONFIG_CC_HAS_K_CONSTRAINT=1)
+
 ifeq ($(CONFIG_ARM64), y)
 brokengasinst := $(call as-instr,1:\n.inst 0\n.rept . - 1b\n\nnop\n.endr\n,,-DCONFIG_BROKEN_GAS_INST=1)
 

@@ -63,7 +69,8 @@ ifeq ($(CONFIG_GENERIC_COMPAT_VDSO), y)
   endif
 endif
 
-KBUILD_CFLAGS  += -mgeneral-regs-only $(lseinstr) $(brokengasinst) $(compat_vdso)
+KBUILD_CFLAGS  += -mgeneral-regs-only $(lseinstr) $(brokengasinst)     \
+                  $(compat_vdso) $(cc_has_k_constraint)
 KBUILD_CFLAGS  += -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS  += $(call cc-disable-warning, psabi)
 KBUILD_AFLAGS  += $(lseinstr) $(brokengasinst) $(compat_vdso)

arch/arm64/include/asm/atomic_ll_sc.h +40 -23

index 95091f72228b..7b012148bfd6 100644
@@ -10,6 +10,8 @@
 #ifndef __ASM_ATOMIC_LL_SC_H
 #define __ASM_ATOMIC_LL_SC_H
 
+#include <linux/stringify.h>
+
 #if IS_ENABLED(CONFIG_ARM64_LSE_ATOMICS) && IS_ENABLED(CONFIG_AS_LSE)
 #define __LL_SC_FALLBACK(asm_ops)                                      \
 "      b       3f\n"                                                   \

@@ -23,6 +25,10 @@ asm_ops "\n"                                                         \
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 
+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif
+
 /*
  * AArch64 UP and SMP safe atomic ops.  We use load exclusive and
  * store exclusive to ensure that these are atomic.  We may loop

@@ -44,7 +50,7 @@ __ll_sc_atomic_##op(int i, atomic_t *v)                                       \
 "      stxr    %w1, %w0, %2\n"                                         \
 "      cbnz    %w1, 1b\n")                                             \
        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \
-       : #constraint "r" (i));                                         \
+       : __stringify(constraint) "r" (i));                             \
 }
 
 #define ATOMIC_OP_RETURN(name, mb, acq, rel, cl, op, asm_op, constraint)\

@@ -63,7 +69,7 @@ __ll_sc_atomic_##op##_return##name(int i, atomic_t *v)                        \
 "      cbnz    %w1, 1b\n"                                              \
 "      " #mb )                                                         \
        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \
-       : #constraint "r" (i)                                           \
+       : __stringify(constraint) "r" (i)                               \
        : cl);                                                          \
                                                                        \
        return result;                                                  \

@@ -85,7 +91,7 @@ __ll_sc_atomic_fetch_##op##name(int i, atomic_t *v)                   \
 "      cbnz    %w2, 1b\n"                                              \
 "      " #mb )                                                         \
        : "=&r" (result), "=&r" (val), "=&r" (tmp), "+Q" (v->counter)   \
-       : #constraint "r" (i)                                           \
+       : __stringify(constraint) "r" (i)                               \
        : cl);                                                          \
                                                                        \
        return result;                                                  \

@@ -113,10 +119,15 @@ ATOMIC_OPS(sub, sub, J)
        ATOMIC_FETCH_OP (_acquire,        , a,  , "memory", __VA_ARGS__)\
        ATOMIC_FETCH_OP (_release,        ,  , l, "memory", __VA_ARGS__)
 
-ATOMIC_OPS(and, and, )
+ATOMIC_OPS(and, and, K)
+ATOMIC_OPS(or, orr, K)
+ATOMIC_OPS(xor, eor, K)
+/*
+ * GAS converts the mysterious and undocumented BIC (immediate) alias to
+ * an AND (immediate) instruction with the immediate inverted. We don't
+ * have a constraint for this, so fall back to register.
+ */
 ATOMIC_OPS(andnot, bic, )
-ATOMIC_OPS(or, orr, )
-ATOMIC_OPS(xor, eor, )
 
 #undef ATOMIC_OPS
 #undef ATOMIC_FETCH_OP

@@ -138,7 +149,7 @@ __ll_sc_atomic64_##op(s64 i, atomic64_t *v)                         \
 "      stxr    %w1, %0, %2\n"                                          \
 "      cbnz    %w1, 1b")                                               \
        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \
-       : #constraint "r" (i));                                         \
+       : __stringify(constraint) "r" (i));                             \
 }
 
 #define ATOMIC64_OP_RETURN(name, mb, acq, rel, cl, op, asm_op, constraint)\

@@ -157,7 +168,7 @@ __ll_sc_atomic64_##op##_return##name(s64 i, atomic64_t *v)          \
 "      cbnz    %w1, 1b\n"                                              \
 "      " #mb )                                                         \
        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \
-       : #constraint "r" (i)                                           \
+       : __stringify(constraint) "r" (i)                               \
        : cl);                                                          \
                                                                        \
        return result;                                                  \

@@ -179,7 +190,7 @@ __ll_sc_atomic64_fetch_##op##name(s64 i, atomic64_t *v)             \
 "      cbnz    %w2, 1b\n"                                              \
 "      " #mb )                                                         \
        : "=&r" (result), "=&r" (val), "=&r" (tmp), "+Q" (v->counter)   \
-       : #constraint "r" (i)                                           \
+       : __stringify(constraint) "r" (i)                               \
        : cl);                                                          \
                                                                        \
        return result;                                                  \

@@ -208,9 +219,14 @@ ATOMIC64_OPS(sub, sub, J)
        ATOMIC64_FETCH_OP (_release,,  , l, "memory", __VA_ARGS__)
 
 ATOMIC64_OPS(and, and, L)
-ATOMIC64_OPS(andnot, bic, )
 ATOMIC64_OPS(or, orr, L)
 ATOMIC64_OPS(xor, eor, L)
+/*
+ * GAS converts the mysterious and undocumented BIC (immediate) alias to
+ * an AND (immediate) instruction with the immediate inverted. We don't
+ * have a constraint for this, so fall back to register.
+ */
+ATOMIC64_OPS(andnot, bic, )
 
 #undef ATOMIC64_OPS
 #undef ATOMIC64_FETCH_OP

@@ -269,7 +285,7 @@ __ll_sc__cmpxchg_case_##name##sz(volatile void *ptr,                        \
        "2:")                                                           \
        : [tmp] "=&r" (tmp), [oldval] "=&r" (oldval),                   \
          [v] "+Q" (*(u##sz *)ptr)                                      \
-       : [old] #constraint "r" (old), [new] "r" (new)                  \
+       : [old] __stringify(constraint) "r" (old), [new] "r" (new)      \
        : cl);                                                          \
                                                                        \
        return oldval;                                                  \

@@ -280,21 +296,21 @@ __ll_sc__cmpxchg_case_##name##sz(volatile void *ptr,                      \
  * handle the 'K' constraint for the value 4294967295 - thus we use no
  * constraint for 32 bit operations.
  */
-__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , )
-__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , )
-__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , )
+__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , K)
+__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , K)
+__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , K)
 __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(w, b, acq_,  8,        , a,  , "memory", K)
+__CMPXCHG_CASE(w, h, acq_, 16,        , a,  , "memory", K)
+__CMPXCHG_CASE(w,  , acq_, 32,        , a,  , "memory", K)
 __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(w, b, rel_,  8,        ,  , l, "memory", K)
+__CMPXCHG_CASE(w, h, rel_, 16,        ,  , l, "memory", K)
+__CMPXCHG_CASE(w,  , rel_, 32,        ,  , l, "memory", K)
 __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(w, b,  mb_,  8, dmb ish,  , l, "memory", K)
+__CMPXCHG_CASE(w, h,  mb_, 16, dmb ish,  , l, "memory", K)
+__CMPXCHG_CASE(w,  ,  mb_, 32, dmb ish,  , l, "memory", K)
 __CMPXCHG_CASE( ,  ,  mb_, 64, dmb ish,  , l, "memory", L)
 
 #undef __CMPXCHG_CASE

@@ -332,5 +348,6 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+#undef K
 
 #endif /* __ASM_ATOMIC_LL_SC_H */

Quoted 17 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 28

Quoted 11 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"                                                               \
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 
+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Bah, I need to use something like __stringify when the constraint is used in order for this to get expanded properly. Updated diff below.

Quoted 1 line

Will

Hi Will, thanks for cc'ing me on the patch set. I'd be happy to help test w/ Clang. Would you mind pushing this set with the below diff to a publicly available tree+branch I can pull from? (I haven't yet figured out how to download multiple diff's from gmail rather than 1 by 1, and TBH I'd rather just use git).

Quoted 18 lines · Nick Desaulniers

Quoted 17 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 28

Quoted 11 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"                                                               \
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 
+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Bah, I need to use something like __stringify when the constraint is used in order for this to get expanded properly. Updated diff below.

Quoted 7 lines

Quoted 1 line

Will

Hi Will, thanks for cc'ing me on the patch set. I'd be happy to help test w/ Clang. Would you mind pushing this set with the below diff to a publicly available tree+branch I can pull from? (I haven't yet figured out how to download multiple diff's from gmail rather than 1 by 1, and TBH I'd rather just use git).

Sorry, of course. I should've mentioned this in the cover letter:

https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/atomics

FWIW, I did test (defconfig + boot) with clang, but this does mean that LSE atomics are disabled for that configuration when asm goto is not supported.

Will

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Bah, I need to use something like __stringify when the constraint is used in order for this to get expanded properly. Updated diff below.

Will

--->8

arch/arm64/Makefile +8 -1

index 61de992bbea3..0cef056b5fb1 100644
@@ -39,6 +39,12 @@ $(warning LSE atomics not supported by binutils)
   endif
 endif
 
+cc_has_k_constraint := $(call try-run,echo				\
+	'int main(void) {						\
+		asm volatile("and w0, w0, %w0" :: "K" (4294967295));	\
+		return 0;						\
+	}' | $(CC) -S -x c -o "$$TMP" -,,-DCONFIG_CC_HAS_K_CONSTRAINT=1)
+
 ifeq ($(CONFIG_ARM64), y)
 brokengasinst := $(call as-instr,1:\n.inst 0\n.rept . - 1b\n\nnop\n.endr\n,,-DCONFIG_BROKEN_GAS_INST=1)
 

@@ -63,7 +69,8 @@ ifeq ($(CONFIG_GENERIC_COMPAT_VDSO), y)
   endif
 endif
 
-KBUILD_CFLAGS	+= -mgeneral-regs-only $(lseinstr) $(brokengasinst) $(compat_vdso)
+KBUILD_CFLAGS	+= -mgeneral-regs-only $(lseinstr) $(brokengasinst)	\
+		   $(compat_vdso) $(cc_has_k_constraint)
 KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS	+= $(call cc-disable-warning, psabi)
 KBUILD_AFLAGS	+= $(lseinstr) $(brokengasinst) $(compat_vdso)

arch/arm64/include/asm/atomic_ll_sc.h +39 -23

index 95091f72228b..7b012148bfd6 100644
@@ -10,6 +10,8 @@
 #ifndef __ASM_ATOMIC_LL_SC_H
 #define __ASM_ATOMIC_LL_SC_H
 
+#include <linux/stringify.h>
+
 #if IS_ENABLED(CONFIG_ARM64_LSE_ATOMICS) && IS_ENABLED(CONFIG_AS_LSE)
 #define __LL_SC_FALLBACK(asm_ops)					\
 "	b	3f\n"							\

@@ -23,6 +25,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 
+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif
+
 /*
  * AArch64 UP and SMP safe atomic ops.  We use load exclusive and
  * store exclusive to ensure that these are atomic.  We may loop

@@ -44,7 +50,7 @@ __ll_sc_atomic_##op(int i, atomic_t *v)					\
 "	stxr	%w1, %w0, %2\n"						\
 "	cbnz	%w1, 1b\n")						\
 	: "=&r" (result), "=&r" (tmp), "+Q" (v->counter)		\
-	: #constraint "r" (i));						\
+	: __stringify(constraint) "r" (i));				\
 }
 
 #define ATOMIC_OP_RETURN(name, mb, acq, rel, cl, op, asm_op, constraint)\

@@ -63,7 +69,7 @@ __ll_sc_atomic_##op##_return##name(int i, atomic_t *v)			\
 "	cbnz	%w1, 1b\n"						\
 "	" #mb )								\
 	: "=&r" (result), "=&r" (tmp), "+Q" (v->counter)		\
-	: #constraint "r" (i)						\
+	: __stringify(constraint) "r" (i)				\
 	: cl);								\
 									\
 	return result;							\

@@ -85,7 +91,7 @@ __ll_sc_atomic_fetch_##op##name(int i, atomic_t *v)			\
 "	cbnz	%w2, 1b\n"						\
 "	" #mb )								\
 	: "=&r" (result), "=&r" (val), "=&r" (tmp), "+Q" (v->counter)	\
-	: #constraint "r" (i)						\
+	: __stringify(constraint) "r" (i)				\
 	: cl);								\
 									\
 	return result;							\

@@ -113,10 +119,15 @@ ATOMIC_OPS(sub, sub, J)
 	ATOMIC_FETCH_OP (_acquire,        , a,  , "memory", __VA_ARGS__)\
 	ATOMIC_FETCH_OP (_release,        ,  , l, "memory", __VA_ARGS__)
 
-ATOMIC_OPS(and, and, )
+ATOMIC_OPS(and, and, K)
+ATOMIC_OPS(or, orr, K)
+ATOMIC_OPS(xor, eor, K)
+/*
+ * GAS converts the mysterious and undocumented BIC (immediate) alias to
+ * an AND (immediate) instruction with the immediate inverted. We don't
+ * have a constraint for this, so fall back to register.
+ */
 ATOMIC_OPS(andnot, bic, )
-ATOMIC_OPS(or, orr, )
-ATOMIC_OPS(xor, eor, )
 
 #undef ATOMIC_OPS
 #undef ATOMIC_FETCH_OP

@@ -138,7 +149,7 @@ __ll_sc_atomic64_##op(s64 i, atomic64_t *v)				\
 "	stxr	%w1, %0, %2\n"						\
 "	cbnz	%w1, 1b")						\
 	: "=&r" (result), "=&r" (tmp), "+Q" (v->counter)		\
-	: #constraint "r" (i));						\
+	: __stringify(constraint) "r" (i));				\
 }
 
 #define ATOMIC64_OP_RETURN(name, mb, acq, rel, cl, op, asm_op, constraint)\

@@ -157,7 +168,7 @@ __ll_sc_atomic64_##op##_return##name(s64 i, atomic64_t *v)		\
 "	cbnz	%w1, 1b\n"						\
 "	" #mb )								\
 	: "=&r" (result), "=&r" (tmp), "+Q" (v->counter)		\
-	: #constraint "r" (i)						\
+	: __stringify(constraint) "r" (i)				\
 	: cl);								\
 									\
 	return result;							\

@@ -179,7 +190,7 @@ __ll_sc_atomic64_fetch_##op##name(s64 i, atomic64_t *v)		\
 "	cbnz	%w2, 1b\n"						\
 "	" #mb )								\
 	: "=&r" (result), "=&r" (val), "=&r" (tmp), "+Q" (v->counter)	\
-	: #constraint "r" (i)						\
+	: __stringify(constraint) "r" (i)				\
 	: cl);								\
 									\
 	return result;							\

@@ -208,9 +219,14 @@ ATOMIC64_OPS(sub, sub, J)
 	ATOMIC64_FETCH_OP (_release,,  , l, "memory", __VA_ARGS__)
 
 ATOMIC64_OPS(and, and, L)
-ATOMIC64_OPS(andnot, bic, )
 ATOMIC64_OPS(or, orr, L)
 ATOMIC64_OPS(xor, eor, L)
+/*
+ * GAS converts the mysterious and undocumented BIC (immediate) alias to
+ * an AND (immediate) instruction with the immediate inverted. We don't
+ * have a constraint for this, so fall back to register.
+ */
+ATOMIC64_OPS(andnot, bic, )
 
 #undef ATOMIC64_OPS
 #undef ATOMIC64_FETCH_OP

@@ -269,7 +285,7 @@ __ll_sc__cmpxchg_case_##name##sz(volatile void *ptr,			\
 	"2:")								\
 	: [tmp] "=&r" (tmp), [oldval] "=&r" (oldval),			\
 	  [v] "+Q" (*(u##sz *)ptr)					\
-	: [old] #constraint "r" (old), [new] "r" (new)			\
+	: [old] __stringify(constraint) "r" (old), [new] "r" (new)	\
 	: cl);								\
 									\
 	return oldval;							\

@@ -280,21 +296,21 @@ __ll_sc__cmpxchg_case_##name##sz(volatile void *ptr,			\
  * handle the 'K' constraint for the value 4294967295 - thus we use no
  * constraint for 32 bit operations.
  */
-__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , )
-__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , )
-__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , )
+__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , K)
+__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , K)
+__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , K)
 __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(w, b, acq_,  8,        , a,  , "memory", K)
+__CMPXCHG_CASE(w, h, acq_, 16,        , a,  , "memory", K)
+__CMPXCHG_CASE(w,  , acq_, 32,        , a,  , "memory", K)
 __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(w, b, rel_,  8,        ,  , l, "memory", K)
+__CMPXCHG_CASE(w, h, rel_, 16,        ,  , l, "memory", K)
+__CMPXCHG_CASE(w,  , rel_, 32,        ,  , l, "memory", K)
 __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(w, b,  mb_,  8, dmb ish,  , l, "memory", K)
+__CMPXCHG_CASE(w, h,  mb_, 16, dmb ish,  , l, "memory", K)
+__CMPXCHG_CASE(w,  ,  mb_, 32, dmb ish,  , l, "memory", K)
 __CMPXCHG_CASE( ,  ,  mb_, 64, dmb ish,  , l, "memory", L)
 
 #undef __CMPXCHG_CASE

Quoted 9 lines · Will Deacon

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 

Quoted 6 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Bah, I need to use something like __stringify when the constraint is used in order for this to get expanded properly. Updated diff below.

I don't think the changes in your updated diff are required. We successfully combine 'asm_op' with the remainder of the assembly string without using
__stringify, and this is no different to how the original patch combined
'constraint' with "r".

You can verify this by looking at the preprocessed .i files generated with something like:

make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/spi-rockchip.i

I see no difference (with GCC 7.3.1) between the original approach and your use of __stringify. Incidentally you end up with "K" "r" instead of "Kr" but it seems to have the desired effect (e.g. supress/emit out of range errors). I have a couple of macros that resolves this to "Kr" but I don't think it's necessary.

Did you find that it didn't work without your changes? I found it hard to reproduce the out-of-range errors until I made the following change, I could then easily see the effect of changing the constraint:

        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \

Quoted 9 lines · Andrew Murray

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 24

Quoted 7 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif

Quoted 4 lines

Quoted 4 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 4 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Quoted 8 lines

Quoted 3 lines

Bah, I need to use something like __stringify when the constraint is used in order for this to get expanded properly. Updated diff below.

I don't think the changes in your updated diff are required. We successfully combine 'asm_op' with the remainder of the assembly string without using
__stringify, and this is no different to how the original patch combined
'constraint' with "r".

It's a hack: __stringify expands its arguments, so I figured I may as well use that rather than do it manually with an extra macro.

Quoted 16 lines

You can verify this by looking at the preprocessed .i files generated with something like:

make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/spi-rockchip.i

I see no difference (with GCC 7.3.1) between the original approach and your use of __stringify. Incidentally you end up with "K" "r" instead of "Kr" but it seems to have the desired effect (e.g. supress/emit out of range errors). I have a couple of macros that resolves this to "Kr" but I don't think it's necessary.

Did you find that it didn't work without your changes? I found it hard to reproduce the out-of-range errors until I made the following change, I could then easily see the effect of changing the constraint:

        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \

arch/arm64/include/asm/atomic_ll_sc.h __ll_sc_atomic_##op(int i, atomic_t *v) \ · line 54

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +1 -1

-       : #constraint "r" (i));                                         \
+       : #constraint) "r" (4294967295));                                               \
 }

Without the __stringify I get a compilation failure when building kernel/panic.o because it tries to cmpxchg a 32-bit variable with -1 (PANIC_CPU_INVALID). Looking at panic.s, I see that constraint parameter isn't being expanded. For example if I do:

  #ifndef CONFIG_CC_HAS_K_CONSTRAINT
  #define INVALID_CONSTRAINT
  #else
  #define INVALID_CONSTRAINT	K
  #endif

and then pass INVALID_CONSTRAINT to the generator macros, we'll end up with INVALID_CONSTRAINT in the .s file and gas will barf.

The reason I didn't see this initially is because my silly testcase had a typo and was using atomic_add instead of atomic_and.

Will

Quoted 10 lines · Will Deacon

Quoted 9 lines · Andrew Murray

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 24

Quoted 7 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif

I downloaded your original patches and tried them, and also got the build error. After playing with this I think something isn't quite right...

This is your current test:

echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' | aarch64-linux-gnu-gcc -S -x c - ; echo $?

But on my machine this returns 0, i.e. no error. If I drop the -S:

echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' | aarch64-linux-gnu-gcc -x c - ; echo $?

Then this returns 1.

So I guess the -S flag or something similar is needed.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Also, isn't this the wrong way around?

It looks like when using $(call try-run,echo - it's the last argument that is used when the condition is false. Thus at present we seem to be setting CONFIG_CC_HAS_K_CONSTRAINT when 'K' is broken.

Quoted 11 lines

Without the __stringify I get a compilation failure when building kernel/panic.o because it tries to cmpxchg a 32-bit variable with -1 (PANIC_CPU_INVALID). Looking at panic.s, I see that constraint parameter isn't being expanded. For example if I do:

  #ifndef CONFIG_CC_HAS_K_CONSTRAINT
  #define INVALID_CONSTRAINT
  #else
  #define INVALID_CONSTRAINT	K
  #endif

Quoted 2 lines

and then pass INVALID_CONSTRAINT to the generator macros, we'll end up with INVALID_CONSTRAINT in the .s file and gas will barf.

This still isn't an issue for me. Your patches cause the build to fail because it's using the K flag - if I invert the CONFIG_CC_HAS_K_CONSTRAINT test then it builds correctly (because it expands the K to nothing).

If there is an issue with the expansion of constraint, shouldn't we also __stringify 'asm_op'?

Quoted 19 lines · Andrew Murray

Quoted 10 lines · Will Deacon

Quoted 9 lines · Andrew Murray

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 24

Quoted 7 lines · Will Deacon

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif

I downloaded your original patches and tried them, and also got the build error. After playing with this I think something isn't quite right...

This is your current test:

echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' | aarch64-linux-gnu-gcc -S -x c - ; echo $?

Quoted 7 lines

But on my machine this returns 0, i.e. no error. If I drop the -S:

echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' | aarch64-linux-gnu-gcc -x c - ; echo $?

Then this returns 1.

So I guess the -S flag or something similar is needed.

This seems correct to me, and is the reason we pass -S in the Makefile. Why are you dropping it?

In the first case, the (broken) compiler is emitted an assembly file containing "and w0, w0, 4294967295", and so we will not define CONFIG_CC_HAS_K_CONSTRAINT.

In the second case, you're passing the bad assembly file to GAS, which rejects it.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Also, isn't this the wrong way around?

No. If the compiler doesn't support the K constraint, then we get:

	[old] "" "r" (old)

because we've defined K as being nothing. Otherwise, we get:

	[old] "K" "r" (old)

because K isn't defined as anything.

Quoted 3 lines

It looks like when using $(call try-run,echo - it's the last argument that is used when the condition is false. Thus at present we seem to be setting CONFIG_CC_HAS_K_CONSTRAINT when 'K' is broken.

No. We set CONFIG_CC_HAS_K_CONSTRAINT when the compiler fails to generate an assembly file with the invalid immediate.

Quoted 14 lines

Quoted 13 lines

Without the __stringify I get a compilation failure when building kernel/panic.o because it tries to cmpxchg a 32-bit variable with -1 (PANIC_CPU_INVALID). Looking at panic.s, I see that constraint parameter isn't being expanded. For example if I do:

  #ifndef CONFIG_CC_HAS_K_CONSTRAINT
  #define INVALID_CONSTRAINT
  #else
  #define INVALID_CONSTRAINT	K
  #endif

and then pass INVALID_CONSTRAINT to the generator macros, we'll end up with INVALID_CONSTRAINT in the .s file and gas will barf.

Quoted 3 lines

This still isn't an issue for me. Your patches cause the build to fail because it's using the K flag - if I invert the CONFIG_CC_HAS_K_CONSTRAINT test then it builds correctly (because it expands the K to nothing).

That doesn't make any sense :/ Is this after you've dropped the -S parameter?

If you think there's a bug, please can you send a patch? However, inverting the check breaks the build for me. Which toolchain are you using?

Quoted 2 lines

If there is an issue with the expansion of constraint, shouldn't we also __stringify 'asm_op'?

It would be harmless, but there's no need because asm_op doesn't ever require further expansion.

Will

Quoted 3 lines · Will Deacon

Quoted 2 lines · Andrew Murray

Quoted 1 line · Will Deacon

On Thu, Aug 29, 2019 at 04:48:34PM +0100, Will Deacon wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 24

Quoted 7 lines

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif

I downloaded your original patches and tried them, and also got the build error. After playing with this I think something isn't quite right...

This is your current test:

echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' | aarch64-linux-gnu-gcc -S -x c - ; echo $?

But on my machine this returns 0, i.e. no error.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Quoted 4 lines · Andrew Murray

Quoted 3 lines · Will Deacon

Quoted 2 lines · Andrew Murray

Quoted 1 line · Will Deacon

On Thu, Aug 29, 2019 at 04:48:34PM +0100, Will Deacon wrote:

Quoted 10 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 24

Quoted 7 lines

arch/arm64/include/asm/atomic_ll_sc.h +0 -0

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif

I downloaded your original patches and tried them, and also got the build error. After playing with this I think something isn't quite right...

Can you post the error you see?

Quoted 6 lines

This is your current test:

echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' | aarch64-linux-gnu-gcc -S -x c - ; echo $?

But on my machine this returns 0, i.e. no error.

IIUC that's expected, as this is testing if the compiler erroneously accepts the invalid immediate.

Note that try-run takes (option,option-ok,otherwise), so:

| cc_has_k_constraint := $(call try-run,echo                             \
|        'int main(void) {                                               \
|                asm volatile("and w0, w0, %w0" :: "K" (4294967295));    \
|                return 0;                                               \
|        }' | $(CC) -S -x c -o "$$TMP" -,,-DCONFIG_CC_HAS_K_CONSTRAINT=1)

... means we do nothing when the compile is successful (i.e. when the compiler is broken), and we set -DCONFIG_CC_HAS_K_CONSTRAINT=1 when the compiler correctly rejects the invalid immediate.

If we drop the -S, we'll get an error in all cases, as either:

* GCC silently accepts the immediate, GAS aborts
* GCC aborts as it can't satisfy the constraint

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h asm_ops "\n" \ · line 30

Quoted 3 lines

arch/arm64/include/asm/atomic_ll_sc.h +3 -0

+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Here we define K to nothing if the compiler accepts the broken immediate.

If the compiler rejects invalid immediates we don't define K to anything, so it's treated as a literal later on, and gets added as a constaint.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The 'K' constraint is a documented AArch64 machine constraint supported by GCC for matching integer constants that can be used with a 32-bit logical instruction. Unfortunately, some released compilers erroneously accept the immediate '4294967295' for this constraint, which is later refused by GAS at assembly time. This had led us to avoid the use of the 'K' constraint altogether.

Instead, detect whether the compiler is up to the job when building the kernel and pass the 'K' constraint to our 32-bit atomic macros when it appears to be supported.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/Makefile                   |  9 ++++++-
 arch/arm64/include/asm/atomic_ll_sc.h | 47 +++++++++++++++++++++++------------
 2 files changed, 39 insertions(+), 17 deletions(-)

arch/arm64/Makefile +8 -1

index 61de992bbea3..0cef056b5fb1 100644
@@ -39,6 +39,12 @@ $(warning LSE atomics not supported by binutils)
   endif
 endif
 
+cc_has_k_constraint := $(call try-run,echo				\
+	'int main(void) {						\
+		asm volatile("and w0, w0, %w0" :: "K" (4294967295));	\
+		return 0;						\
+	}' | $(CC) -S -x c -o "$$TMP" -,,-DCONFIG_CC_HAS_K_CONSTRAINT=1)
+
 ifeq ($(CONFIG_ARM64), y)
 brokengasinst := $(call as-instr,1:\n.inst 0\n.rept . - 1b\n\nnop\n.endr\n,,-DCONFIG_BROKEN_GAS_INST=1)
 

@@ -63,7 +69,8 @@ ifeq ($(CONFIG_GENERIC_COMPAT_VDSO), y)
   endif
 endif
 
-KBUILD_CFLAGS	+= -mgeneral-regs-only $(lseinstr) $(brokengasinst) $(compat_vdso)
+KBUILD_CFLAGS	+= -mgeneral-regs-only $(lseinstr) $(brokengasinst)	\
+		   $(compat_vdso) $(cc_has_k_constraint)
 KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS	+= $(call cc-disable-warning, psabi)
 KBUILD_AFLAGS	+= $(lseinstr) $(brokengasinst) $(compat_vdso)

arch/arm64/include/asm/atomic_ll_sc.h +31 -16

index 95091f72228b..7fa042f5444e 100644
@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif
 
+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif
+
 /*
  * AArch64 UP and SMP safe atomic ops.  We use load exclusive and
  * store exclusive to ensure that these are atomic.  We may loop

@@ -113,10 +117,15 @@ ATOMIC_OPS(sub, sub, J)
 	ATOMIC_FETCH_OP (_acquire,        , a,  , "memory", __VA_ARGS__)\
 	ATOMIC_FETCH_OP (_release,        ,  , l, "memory", __VA_ARGS__)
 
-ATOMIC_OPS(and, and, )
+ATOMIC_OPS(and, and, K)
+ATOMIC_OPS(or, orr, K)
+ATOMIC_OPS(xor, eor, K)
+/*
+ * GAS converts the mysterious and undocumented BIC (immediate) alias to
+ * an AND (immediate) instruction with the immediate inverted. We don't
+ * have a constraint for this, so fall back to register.
+ */
 ATOMIC_OPS(andnot, bic, )
-ATOMIC_OPS(or, orr, )
-ATOMIC_OPS(xor, eor, )
 
 #undef ATOMIC_OPS
 #undef ATOMIC_FETCH_OP

@@ -208,9 +217,14 @@ ATOMIC64_OPS(sub, sub, J)
 	ATOMIC64_FETCH_OP (_release,,  , l, "memory", __VA_ARGS__)
 
 ATOMIC64_OPS(and, and, L)
-ATOMIC64_OPS(andnot, bic, )
 ATOMIC64_OPS(or, orr, L)
 ATOMIC64_OPS(xor, eor, L)
+/*
+ * GAS converts the mysterious and undocumented BIC (immediate) alias to
+ * an AND (immediate) instruction with the immediate inverted. We don't
+ * have a constraint for this, so fall back to register.
+ */
+ATOMIC64_OPS(andnot, bic, )
 
 #undef ATOMIC64_OPS
 #undef ATOMIC64_FETCH_OP

@@ -280,21 +294,21 @@ __ll_sc__cmpxchg_case_##name##sz(volatile void *ptr,			\
  * handle the 'K' constraint for the value 4294967295 - thus we use no
  * constraint for 32 bit operations.
  */
-__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , )
-__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , )
-__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , )
+__CMPXCHG_CASE(w, b,     ,  8,        ,  ,  ,         , K)
+__CMPXCHG_CASE(w, h,     , 16,        ,  ,  ,         , K)
+__CMPXCHG_CASE(w,  ,     , 32,        ,  ,  ,         , K)
 __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(w, b, acq_,  8,        , a,  , "memory", K)
+__CMPXCHG_CASE(w, h, acq_, 16,        , a,  , "memory", K)
+__CMPXCHG_CASE(w,  , acq_, 32,        , a,  , "memory", K)
 __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(w, b, rel_,  8,        ,  , l, "memory", K)
+__CMPXCHG_CASE(w, h, rel_, 16,        ,  , l, "memory", K)
+__CMPXCHG_CASE(w,  , rel_, 32,        ,  , l, "memory", K)
 __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(w, b,  mb_,  8, dmb ish,  , l, "memory", K)
+__CMPXCHG_CASE(w, h,  mb_, 16, dmb ish,  , l, "memory", K)
+__CMPXCHG_CASE(w,  ,  mb_, 32, dmb ish,  , l, "memory", K)
 __CMPXCHG_CASE( ,  ,  mb_, 64, dmb ish,  , l, "memory", L)
 
 #undef __CMPXCHG_CASE

@@ -332,5 +346,6 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+#undef K
 
 #endif	/* __ASM_ATOMIC_LL_SC_H */

Signature · 1 line

2.11.0