diff mbox

[2/3] ARM 64 bit atomic operations

Message ID 20110701155557.GC5242@davesworkthinkpad
State Superseded
Headers show

Commit Message

Dr. David Alan Gilbert July 1, 2011, 3:55 p.m. UTC 
Provide fallbacks for 64bit atomics that call Linux commpage helpers
  when compiling for older machines.  The code is based on the existing
  linux-atomic.c for other sizes, however it performs an init time
  check that the kernel is new enough to provide the helper.

  This relies on Nicolas Pitre's kernel patch here:
  https://patchwork.kernel.org/patch/894932/

Comments

Richard Henderson July 1, 2011, 4:03 p.m. UTC | #1 
On 07/01/2011 08:55 AM, Dr. David Alan Gilbert wrote:
> +/* Check that the kernel has a new enough version at load */
> +void __check_for_sync8_kernelhelper (void)
> +{
> +  if (__kernel_helper_version < 5)
> +    {
> +      const char err[] = "A newer kernel is required to run this binary. (__kernel_cmpxchg64 helper)\n";
> +      /* At this point we need a way to crash with some information
> +	 for the user - I'm not sure I can rely on much else being
> +	 available at this point, so do the same as generic-morestack.c
> +	 write() and abort(). */
> +      write (2 /* stderr */, err, sizeof(err));
> +      abort ();
> +    }
> +};

Wouldn't it be better to convert the arm kernel to use a proper VDSO,
so that this error actually comes from the dynamic linker?  That's
the beauty of a true VDSO -- proper symbol resolution.


r~
Dr. David Alan Gilbert July 1, 2011, 5:07 p.m. UTC | #2 
On 1 July 2011 17:03, Richard Henderson <rth@redhat.com> wrote:
> On 07/01/2011 08:55 AM, Dr. David Alan Gilbert wrote:
>> +/* Check that the kernel has a new enough version at load */
>> +void __check_for_sync8_kernelhelper (void)
>> +{
>> +  if (__kernel_helper_version < 5)
>> +    {
>> +      const char err[] = "A newer kernel is required to run this binary. (__kernel_cmpxchg64 helper)\n";
>> +      /* At this point we need a way to crash with some information
>> +      for the user - I'm not sure I can rely on much else being
>> +      available at this point, so do the same as generic-morestack.c
>> +      write() and abort(). */
>> +      write (2 /* stderr */, err, sizeof(err));
>> +      abort ();
>> +    }
>> +};
>
> Wouldn't it be better to convert the arm kernel to use a proper VDSO,
> so that this error actually comes from the dynamic linker?  That's
> the beauty of a true VDSO -- proper symbol resolution.

Well I can't say I like the need for this check/exit - so yes if something else
could do it for me that would be great.  Having said that, I don't know
the history of the ARM commpage/lack of VDSO , neither do I know how
big a change that would be.

Let me have a chat to some of the kernel guys who might know the history.

Dave
Joseph Myers July 1, 2011, 7:38 p.m. UTC | #3 
On Fri, 1 Jul 2011, Dr. David Alan Gilbert wrote:

> +/* For write */
> +#include <unistd.h>
> +/* For abort */
> +#include <stdlib.h>

Please don't include system headers in libgcc without appropriate 
inhibit_libc checks for bootstrap purposes.  In this case, it would seem 
better just to declare the functions you need.

> +/* Check that the kernel has a new enough version at load */
> +void __check_for_sync8_kernelhelper (void)

Shouldn't this function be static?

> +{
> +  if (__kernel_helper_version < 5)
> +    {
> +      const char err[] = "A newer kernel is required to run this binary. (__kernel_cmpxchg64 helper)\n";
> +      /* At this point we need a way to crash with some information
> +	 for the user - I'm not sure I can rely on much else being
> +	 available at this point, so do the same as generic-morestack.c
> +	 write() and abort(). */
> +      write (2 /* stderr */, err, sizeof(err));

"write" is in the user's namespace in ISO C so it's not ideal to have a 
call to it.  If there isn't a reserved-namespace version, using the 
syscall directly (hardcoding the syscall number) might be better.

> +void (*__sync8_kernelhelper_inithook[]) (void) __attribute__ ((section (".init_array"))) = {
> +  &__check_for_sync8_kernelhelper
> +};

Shouldn't this also be static (marked "used" if needed)?  Though I'd have 
thought simply marking the function as a constructor would be simpler and 
better....
Dr. David Alan Gilbert July 4, 2011, 10:27 p.m. UTC | #4 
On 1 July 2011 20:38, Joseph S. Myers <joseph@codesourcery.com> wrote:

Hi Joseph,
  Thanks for your comments.

> On Fri, 1 Jul 2011, Dr. David Alan Gilbert wrote:
>
>> +/* For write */
>> +#include <unistd.h>
>> +/* For abort */
>> +#include <stdlib.h>
>
> Please don't include system headers in libgcc without appropriate
> inhibit_libc checks for bootstrap purposes.  In this case, it would seem
> better just to declare the functions you need.

OK.

>> +/* Check that the kernel has a new enough version at load */
>> +void __check_for_sync8_kernelhelper (void)
>
> Shouldn't this function be static?

Yep.

>> +{
>> +  if (__kernel_helper_version < 5)
>> +    {
>> +      const char err[] = "A newer kernel is required to run this binary. (__kernel_cmpxchg64 helper)\n";
>> +      /* At this point we need a way to crash with some information
>> +      for the user - I'm not sure I can rely on much else being
>> +      available at this point, so do the same as generic-morestack.c
>> +      write() and abort(). */
>> +      write (2 /* stderr */, err, sizeof(err));
>
> "write" is in the user's namespace in ISO C so it's not ideal to have a
> call to it.  If there isn't a reserved-namespace version, using the
> syscall directly (hardcoding the syscall number) might be better.

OK, fair enough.

>> +void (*__sync8_kernelhelper_inithook[]) (void) __attribute__ ((section (".init_array"))) = {
>> +  &__check_for_sync8_kernelhelper
>> +};
>
> Shouldn't this also be static (marked "used" if needed)?  Though I'd have
> thought simply marking the function as a constructor would be simpler and
> better....

OK, can do - I wasn't too sure if constructor would end up later in
the initialisation - I was worrying whether that might end up after a
C++ constructor that might actually use; (although I'm not actually
sure if that's more or less likely to happen with constructor v
init_array).

Dave
diff mbox

Patch

diff --git a/gcc/config/arm/linux-atomic-64bit.c b/gcc/config/arm/linux-atomic-64bit.c
new file mode 100644
index 0000000..140cc2f
--- /dev/null
+++ b/gcc/config/arm/linux-atomic-64bit.c
@@ -0,0 +1,165 @@ 
+/* 64bit Linux-specific atomic operations for ARM EABI.
+   Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+   Based on linux-atomic.c
+
+   64 bit additions david.gilbert@linaro.org
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+<http://www.gnu.org/licenses/>.  */
+
+/* 64bit helper functions for atomic operations; the compiler will
+   call these when the code is compiled for a CPU without ldrexd/strexd.
+   (If the CPU had those then the compiler inlines the operation).
+
+   These helpers require a kernel helper that's only present on newer
+   kernels; we check for that in an init section and bail out rather
+   unceremoneously.  */
+
+/* For write */
+#include <unistd.h>
+/* For abort */
+#include <stdlib.h>
+
+/* Kernel helper for compare-and-exchange.  */
+typedef int (__kernel_cmpxchg64_t) (const long long* oldval,
+					const long long* newval,
+					long long *ptr);
+#define __kernel_cmpxchg64 (*(__kernel_cmpxchg64_t *) 0xffff0f60)
+
+/* Kernel helper page version number */
+
+#define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
+
+/* Check that the kernel has a new enough version at load */
+void __check_for_sync8_kernelhelper (void)
+{
+  if (__kernel_helper_version < 5)
+    {
+      const char err[] = "A newer kernel is required to run this binary. (__kernel_cmpxchg64 helper)\n";
+      /* At this point we need a way to crash with some information
+	 for the user - I'm not sure I can rely on much else being
+	 available at this point, so do the same as generic-morestack.c
+	 write() and abort(). */
+      write (2 /* stderr */, err, sizeof(err));
+      abort ();
+    }
+};
+
+void (*__sync8_kernelhelper_inithook[]) (void) __attribute__ ((section (".init_array"))) = {
+  &__check_for_sync8_kernelhelper
+};
+
+#define HIDDEN __attribute__ ((visibility ("hidden")))
+
+#define FETCH_AND_OP_WORD64(OP, PFX_OP, INF_OP)			\
+  long long HIDDEN						\
+  __sync_fetch_and_##OP##_8 (long long *ptr, long long val)	\
+  {								\
+    int failure;						\
+    long long tmp,tmp2;						\
+								\
+    do {							\
+      tmp = *ptr;						\
+      tmp2 = PFX_OP (tmp INF_OP val);				\
+      failure = __kernel_cmpxchg64 (&tmp, &tmp2, ptr);		\
+    } while (failure != 0);					\
+								\
+    return tmp;							\
+  }
+
+FETCH_AND_OP_WORD64 (add,   , +)
+FETCH_AND_OP_WORD64 (sub,   , -)
+FETCH_AND_OP_WORD64 (or,    , |)
+FETCH_AND_OP_WORD64 (and,   , &)
+FETCH_AND_OP_WORD64 (xor,   , ^)
+FETCH_AND_OP_WORD64 (nand, ~, &)
+
+#define NAME_oldval(OP, WIDTH) __sync_fetch_and_##OP##_##WIDTH
+#define NAME_newval(OP, WIDTH) __sync_##OP##_and_fetch_##WIDTH
+
+/* Implement both __sync_<op>_and_fetch and __sync_fetch_and_<op> for
+   subword-sized quantities.  */
+
+#define OP_AND_FETCH_WORD64(OP, PFX_OP, INF_OP)			\
+  long long HIDDEN						\
+  __sync_##OP##_and_fetch_8 (long long *ptr, long long val)	\
+  {								\
+    int failure;						\
+    long long tmp,tmp2;						\
+								\
+    do {							\
+      tmp = *ptr;						\
+      tmp2 = PFX_OP (tmp INF_OP val);				\
+      failure = __kernel_cmpxchg64 (&tmp, &tmp2, ptr);		\
+    } while (failure != 0);					\
+								\
+    return tmp2;						\
+  }
+
+OP_AND_FETCH_WORD64 (add,   , +)
+OP_AND_FETCH_WORD64 (sub,   , -)
+OP_AND_FETCH_WORD64 (or,    , |)
+OP_AND_FETCH_WORD64 (and,   , &)
+OP_AND_FETCH_WORD64 (xor,   , ^)
+OP_AND_FETCH_WORD64 (nand, ~, &)
+
+long long HIDDEN
+__sync_val_compare_and_swap_8 (long long *ptr, long long oldval, long long newval)
+{
+  int failure;
+  long long actual_oldval;
+
+  while (1)
+    {
+      actual_oldval = *ptr;
+
+      if (__builtin_expect (oldval != actual_oldval, 0))
+	return actual_oldval;
+
+      failure = __kernel_cmpxchg64 (&actual_oldval, &newval, ptr);
+
+      if (__builtin_expect (!failure, 1))
+	return oldval;
+    }
+}
+
+typedef unsigned char bool;
+
+bool HIDDEN
+__sync_bool_compare_and_swap_8 (long long *ptr, long long oldval, long long newval)
+{
+  int failure = __kernel_cmpxchg64 (&oldval, &newval, ptr);
+  return (failure == 0);
+}
+
+long long HIDDEN
+__sync_lock_test_and_set_8 (long long *ptr, long long val)
+{
+  int failure;
+  long long oldval;
+
+  do {
+    oldval = *ptr;
+    failure = __kernel_cmpxchg64 (&oldval, &val, ptr);
+  } while (failure != 0);
+
+  return oldval;
+}
diff --git a/gcc/config/arm/linux-atomic.c b/gcc/config/arm/linux-atomic.c
index 57065a6..80f161d 100644
--- a/gcc/config/arm/linux-atomic.c
+++ b/gcc/config/arm/linux-atomic.c
@@ -32,8 +32,8 @@  typedef void (__kernel_dmb_t) (void);
 #define __kernel_dmb (*(__kernel_dmb_t *) 0xffff0fa0)
 
 /* Note: we implement byte, short and int versions of atomic operations using
-   the above kernel helpers, but there is no support for "long long" (64-bit)
-   operations as yet.  */
+   the above kernel helpers; see linux-atomic-64bit.c for "long long" (64-bit)
+   operations.  */
 
 #define HIDDEN __attribute__ ((visibility ("hidden")))
 
@@ -273,6 +273,7 @@  SUBWORD_TEST_AND_SET (unsigned char,  1)
     *ptr = 0;								\
   }
 
+SYNC_LOCK_RELEASE (long long,   8)
 SYNC_LOCK_RELEASE (int,   4)
 SYNC_LOCK_RELEASE (short, 2)
 SYNC_LOCK_RELEASE (char,  1)
diff --git a/gcc/config/arm/t-linux-eabi b/gcc/config/arm/t-linux-eabi
index a328a00..3814cc0 100644
--- a/gcc/config/arm/t-linux-eabi
+++ b/gcc/config/arm/t-linux-eabi
@@ -36,3 +36,4 @@  LIB1ASMFUNCS := $(filter-out _dvmd_tls,$(LIB1ASMFUNCS)) _dvmd_lnx _clear_cache
 EXTRA_MULTILIB_PARTS=crtbegin.o crtend.o crtbeginS.o crtendS.o crtbeginT.o
 
 LIB2FUNCS_STATIC_EXTRA += $(srcdir)/config/arm/linux-atomic.c
+LIB2FUNCS_STATIC_EXTRA += $(srcdir)/config/arm/linux-atomic-64bit.c