svn commit: r221550 - head/sys/powerpc/conf

[Andreas Tobler]

On 08.05.11 00:17, Attilio Rao wrote:
> 2011/5/6 Attilio Rao<attilio at freebsd.org>:
>> 2011/5/6 Nathan Whitehorn<nwhitehorn at freebsd.org>:
>>> Author: nwhitehorn
>>> Date: Fri May  6 20:43:02 2011
>>> New Revision: 221550
>>> URL: http://svn.freebsd.org/changeset/base/221550
>>>
>>> Log:
>>>   SMP has worked perfectly for a very long time on 32-bit PowerPC on both
>>>   UP and SMP hardware. Enable it in GENERIC.
>>>
>>
>> While working on largeSMP, I think there is a breakage in atomic.h.
>> More specifically, atomic_store_rel_long() (and related functions) are
>> not going to properly work because powerpc defines them as:
>>
>> atomic_store_rel_long ->  atomic_store_rel_32(volatile u_int *p, u_int v)
>>
>> while this should really follow the long arguments.
>>
>> This happens because powerpc doesn't follow the other architectures
>> semantic on defining the "similar" atomic operations.
>> Other arches define an hardcode version of _type version of the
>> function and than make a macro the _32 (or whatever) version.
>> In other words this is what they do:
>>
>> void
>> atomic_store_rel_32()
>> {
>> ...
>> }
>>
>> #define atomic_store_rel_int atomic_store_rel_32
>>
>> which si clearly dangerous for cases as reported above. Maybe that
>> could be fixed by passing sized types, rather than simply int or long
>> in numbered version, but I'd really prefer to follow the semantic by
>> other architectures and then have:
>>
>> void
>> atomic_store_rel_int()
>> {
>> ...
>> }
>>
>> #define atomic_store_rel_32 atomic_store_rel_int
>>
>> I fixed the ATOMIC_STORE_LOAD case in my code, because I needed it,
>> but the final cleanup is much bigger.
>> I can make a patch tomorrow if you can test it.
>>
>
> Can you please test and review this patch?:
> http://www.freebsd.org/~attilio/largeSMP/atomic-powerpc.diff
>
> Unfortunately I'm having issues with the toolchains in atm, so I can't
> really neither test compile it.

I built kernel and world on both, on a 32-bit system and on a 64-bit 
system with 32-bit compat support.

The 32-bit world failed due to type punning issues from umtx.h:121ff

Attached my try to workaround these issues. With my try I can build 
both, kernel and world. Right now I have a world running with it (32-bit).
I do not know if my try is correct. Even less after reading the comments 
from Bruce.
But I think if it is on a somehow correct way, then we need something 
similar for the other _long functions on 32-bit where we go from the 
u_long to u_int. (e.g, atomic_add_long etc.)

I'm ready for more testing.

Gruss,
Andreas
-------------- next part --------------
--- atomic.h.attilio	2011-05-08 11:00:42.000000000 +0200
+++ atomic.h.andreas	2011-05-08 14:01:00.000000000 +0200
@@ -651,16 +651,27 @@
 #define	atomic_cmpset_rel_ptr(dst, old, new)				\
 	atomic_cmpset_rel_long((volatile u_long *)(dst), (u_long)(old),	\
 	    (u_long)(new))
-#else
-#define	atomic_cmpset_long(dst, old, new)				\
-	atomic_cmpset_int((volatile u_int *)(dst), (u_int)(old),	\
-	    (u_int)(new))
-#define	atomic_cmpset_acq_long(dst, old, new)				\
-	atomic_cmpset_acq_int((volatile u_int *)(dst), (u_int)(old),	\
-	    (u_int)(new))
-#define	atomic_cmpset_rel_long(dst, old, new)				\
-	atomic_cmpset_rel_int((volatile u_int *)(dst), (u_int)(old),	\
-	    (u_int)(new))
+#else  /* __powerpc64__  */
+static __inline int
+atomic_cmpset_long(volatile u_long *dst, u_long old, u_long new)
+{
+	return (atomic_cmpset_int((volatile u_int *)dst, (u_int)old, 
+				  (u_int)new));
+}
+
+static __inline int
+atomic_cmpset_acq_long(volatile u_long *dst, u_long old, u_long new)
+{
+	return (atomic_cmpset_acq_int((volatile u_int *)dst, (u_int)old, 
+				      (u_int)new));
+}
+
+static __inline int
+atomic_cmpset_rel_long(volatile u_long *dst, u_long old, u_long new)
+{
+	return (atomic_cmpset_rel_int((volatile u_int *)dst, (u_int)old, 
+				      (u_int)new));
+}
 
 #define	atomic_cmpset_ptr(dst, old, new)				\
 	atomic_cmpset_int((volatile u_int *)(dst), (u_int)(old),	\
@@ -671,7 +682,7 @@
 #define	atomic_cmpset_rel_ptr(dst, old, new)				\
 	atomic_cmpset_rel_int((volatile u_int *)(dst), (u_int)(old),	\
 	    (u_int)(new))
-#endif
+#endif  /* __powerpc64__  */
 
 static __inline u_int
 atomic_fetchadd_int(volatile u_int *p, u_int v)