[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Pranith Kumar]

In cmpxchg(), we do a load exclusive on an address and upon a comparison fail,
we skip the store exclusive instruction. This can result in the exclusive bit
still set. If there was a store exclusive after this to the same address, that
will see the exclusive bit set. This should not happen.

We need to clear the exclusive bit in these cases.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 arch/arm64/include/asm/cmpxchg.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/cmpxchg.h b/arch/arm64/include/asm/cmpxchg.h
index cb95930..ed2cf90 100644
--- a/arch/arm64/include/asm/cmpxchg.h
+++ b/arch/arm64/include/asm/cmpxchg.h
@@ -94,7 +94,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 			"	cmp	%w1, %w3\n"
 			"	b.ne	1f\n"
 			"	stxrb	%w0, %w4, %2\n"
-			"1:\n"
+			"1:	clrex\n"
 				: "=&r" (res), "=&r" (oldval), "+Q" (*(u8 *)ptr)
 				: "Ir" (old), "r" (new)
 				: "cc");
@@ -109,7 +109,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 			"	cmp	%w1, %w3\n"
 			"	b.ne	1f\n"
 			"	stxrh	%w0, %w4, %2\n"
-			"1:\n"
+			"1:	clrex\n"
 				: "=&r" (res), "=&r" (oldval), "+Q" (*(u16 *)ptr)
 				: "Ir" (old), "r" (new)
 				: "cc");
@@ -124,7 +124,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 			"	cmp	%w1, %w3\n"
 			"	b.ne	1f\n"
 			"	stxr	%w0, %w4, %2\n"
-			"1:\n"
+			"1:	clrex\n"
 				: "=&r" (res), "=&r" (oldval), "+Q" (*(u32 *)ptr)
 				: "Ir" (old), "r" (new)
 				: "cc");
@@ -139,7 +139,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 			"	cmp	%1, %3\n"
 			"	b.ne	1f\n"
 			"	stxr	%w0, %4, %2\n"
-			"1:\n"
+			"1:	clrex\n"
 				: "=&r" (res), "=&r" (oldval), "+Q" (*(u64 *)ptr)
 				: "Ir" (old), "r" (new)
 				: "cc");
@@ -173,7 +173,7 @@ static inline int __cmpxchg_double(volatile void *ptr1, volatile void *ptr2,
 			"	mov	%w0, #0\n"
 			"	cbnz	%1, 1f\n"
 			"	stxp	%w0, %5, %6, %2\n"
-			"1:\n"
+			"1:	clrex\n"
 				: "=&r"(loop), "=&r"(lost), "+Q" (*(u64 *)ptr1)
 				: "r" (old1), "r"(old2), "r"(new1), "r"(new2));
 		} while (loop);
-- 
1.9.1

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Will Deacon]

On Fri, Feb 27, 2015 at 05:46:55AM +0000, Pranith Kumar wrote:
> In cmpxchg(), we do a load exclusive on an address and upon a comparison fail,
> we skip the store exclusive instruction. This can result in the exclusive bit
> still set. If there was a store exclusive after this to the same address, that
> will see the exclusive bit set. This should not happen.

... and the problem with that is?

> We need to clear the exclusive bit in these cases.

Why?

Will

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Pranith Kumar]

On Fri, Feb 27, 2015 at 5:06 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Feb 27, 2015 at 05:46:55AM +0000, Pranith Kumar wrote:
>> In cmpxchg(), we do a load exclusive on an address and upon a comparison fail,
>> we skip the store exclusive instruction. This can result in the exclusive bit
>> still set. If there was a store exclusive after this to the same address, that
>> will see the exclusive bit set. This should not happen.
>
> ... and the problem with that is?

Consider the following scenario:

            P0                    P1
            ---------------------------------
            ldxr x7, [B]                         // exclusive bit set
            add  x7, x7, #1
                                str ..., [B]     // exclusive bit cleared
cmpxchg:
            ldxr x0, [B]                         // exclusive bit set
            cmp  x0, #0                        // cmp fails
            b.ne 1f                               // branch taken
            stxr x1, [B]                         // end of cmpxchg
1:
            stxr x7, [B]                        // succeeds?


    The last store exclusive succeeds since the exclusive bit is set which
should not happen. Clearing the exclusive bit before returning from cmpxchg
prevents this happening.

Now I am not sure how likely this will happen. One can argue that a cmpxchg()
will not happen between an external ldxr/stxr. But isn't clearing the exclusive
bit better?

-- 
Pranith

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Catalin Marinas]

On Fri, Feb 27, 2015 at 06:25:25PM +0000, Pranith Kumar wrote:
> On Fri, Feb 27, 2015 at 5:06 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Feb 27, 2015 at 05:46:55AM +0000, Pranith Kumar wrote:
> >> In cmpxchg(), we do a load exclusive on an address and upon a comparison fail,
> >> we skip the store exclusive instruction. This can result in the exclusive bit
> >> still set. If there was a store exclusive after this to the same address, that
> >> will see the exclusive bit set. This should not happen.
> >
> > ... and the problem with that is?
> 
> Consider the following scenario:
> 
>             P0                    P1
>             ---------------------------------
>             ldxr x7, [B]                         // exclusive bit set
>             add  x7, x7, #1
>                                 str ..., [B]     // exclusive bit cleared
> cmpxchg:
>             ldxr x0, [B]                         // exclusive bit set
>             cmp  x0, #0                        // cmp fails
>             b.ne 1f                               // branch taken
>             stxr x1, [B]                         // end of cmpxchg
> 1:
>             stxr x7, [B]                        // succeeds?

It's either badly formatted or I don't get it. Are the "stxr x1" and
"stxr x7" happening on the same CPU (P0)? If yes, that's badly written
code, not even architecturally compliant (you are not allowed other
memory accesses between ldxr and stxr).

>     The last store exclusive succeeds since the exclusive bit is set which
> should not happen. Clearing the exclusive bit before returning from cmpxchg
> prevents this happening.
> 
> Now I am not sure how likely this will happen. One can argue that a cmpxchg()
> will not happen between an external ldxr/stxr. But isn't clearing the exclusive
> bit better?

The only way cmpxchg() could happen between a different ldxr/stxr is
during an interrupt. But ERET automatically clears the exclusive
monitor, so the "stxr x7" would not succeed.

-- 
Catalin

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Pranith Kumar]

On Fri, Feb 27, 2015 at 1:33 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> It's either badly formatted or I don't get it. Are the "stxr x1" and
> "stxr x7" happening on the same CPU (P0)? If yes, that's badly written
> code, not even architecturally compliant (you are not allowed other
> memory accesses between ldxr and stxr).

OK. Is that the same case with ldaxr (acquire) and stlxr (release)?
AFAIK, memory accesses between acquire and release exclusive
operations are allowed.

>
>>     The last store exclusive succeeds since the exclusive bit is set which
>> should not happen. Clearing the exclusive bit before returning from cmpxchg
>> prevents this happening.
>>
>> Now I am not sure how likely this will happen. One can argue that a cmpxchg()
>> will not happen between an external ldxr/stxr. But isn't clearing the exclusive
>> bit better?
>
> The only way cmpxchg() could happen between a different ldxr/stxr is
> during an interrupt. But ERET automatically clears the exclusive
> monitor, so the "stxr x7" would not succeed.

That makes sense. But please consider the ldaxr/stlxr case and let me know.

Thanks!
-- 
Pranith

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Mark Rutland]

On Fri, Feb 27, 2015 at 06:44:19PM +0000, Pranith Kumar wrote:
> On Fri, Feb 27, 2015 at 1:33 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > It's either badly formatted or I don't get it. Are the "stxr x1" and
> > "stxr x7" happening on the same CPU (P0)? If yes, that's badly written
> > code, not even architecturally compliant (you are not allowed other
> > memory accesses between ldxr and stxr).
> 
> OK. Is that the same case with ldaxr (acquire) and stlxr (release)?
> AFAIK, memory accesses between acquire and release exclusive
> operations are allowed.

The restriction on memory accesses in the middle of a load-exclusive
store-exclusive sequence applies to all the load/store-exclusive
variants, including ldaxr and stlxr.

Thanks,
Mark.

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Pranith Kumar]

On Fri, Feb 27, 2015 at 2:08 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Feb 27, 2015 at 06:44:19PM +0000, Pranith Kumar wrote:
>> On Fri, Feb 27, 2015 at 1:33 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > It's either badly formatted or I don't get it. Are the "stxr x1" and
>> > "stxr x7" happening on the same CPU (P0)? If yes, that's badly written
>> > code, not even architecturally compliant (you are not allowed other
>> > memory accesses between ldxr and stxr).
>>
>> OK. Is that the same case with ldaxr (acquire) and stlxr (release)?
>> AFAIK, memory accesses between acquire and release exclusive
>> operations are allowed.
>
> The restriction on memory accesses in the middle of a load-exclusive
> store-exclusive sequence applies to all the load/store-exclusive
> variants, including ldaxr and stlxr.
>

Thanks Mark. I am trying to see where this restriction is documented.
Looking at: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0802a/a64_data_transfer_alpha.html
I do not see that mentioned. The only restriction relevant is that a
stxr should use the same address as the most recent ldxr.

Could you please point me to the relevant documentation?

Thanks!
-- 
Pranith

[RFC PATCH] ARM64: cmpxchg.h: Clear the exclusive access bit on fail

[Mark Rutland]

On Fri, Feb 27, 2015 at 07:15:57PM +0000, Pranith Kumar wrote:
> On Fri, Feb 27, 2015 at 2:08 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Feb 27, 2015 at 06:44:19PM +0000, Pranith Kumar wrote:
> >> On Fri, Feb 27, 2015 at 1:33 PM, Catalin Marinas
> >> <catalin.marinas@arm.com> wrote:
> >> > It's either badly formatted or I don't get it. Are the "stxr x1" and
> >> > "stxr x7" happening on the same CPU (P0)? If yes, that's badly written
> >> > code, not even architecturally compliant (you are not allowed other
> >> > memory accesses between ldxr and stxr).
> >>
> >> OK. Is that the same case with ldaxr (acquire) and stlxr (release)?
> >> AFAIK, memory accesses between acquire and release exclusive
> >> operations are allowed.
> >
> > The restriction on memory accesses in the middle of a load-exclusive
> > store-exclusive sequence applies to all the load/store-exclusive
> > variants, including ldaxr and stlxr.
> >
> 
> Thanks Mark. I am trying to see where this restriction is documented.
> Looking at: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0802a/a64_data_transfer_alpha.html
> I do not see that mentioned. The only restriction relevant is that a
> stxr should use the same address as the most recent ldxr.
> 
> Could you please point me to the relevant documentation?

You will need to look at the latest ARMv8-A Architecture Reference
Manual [1].

Table B2-2 defines the set of Load-Exclusive and Store-Exclusive
variants, including LDAXR and STLXR.

Taking this into account, take a look at the restrictions in section
B2.10.5 "Load-Exclusive and Store-Exclusive instruction usage
restrictions". One of the bullet points nodes that software must avoid
explicit memory accesses between a Load-Exclusive instruction and the
associated Store-Exclusive.

Thanks,
Mark.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.e/index.html