Re: [PATCH 8/9] RISC-V: User-facing API

[Palmer Dabbelt]

On Tue, 11 Jul 2017 06:55:28 PDT (-0700), hch@xxxxxxxxxxxxx wrote:
> On Tue, Jul 11, 2017 at 02:22:15PM +0100, Will Deacon wrote:
>> The problem is that by supporting these hypothetical designs that can't do
>> atomics, you hurt sensible designs that *can* do the atomics because you
>> force them to take an additional indirection that could otherwise be
>> avoided.
>
> Agreed.  But the new patchset seems to remove it already, so I guess
> we're fine on the kernel side.  Now we just need to make sure the
> glibc API doesn't use any indirections.
>
> Note that it might make sense to emit these for very low end nommu
> designs.  Maybe even running Linux, but in that case they'll just need
> a special non-standard ABI for very limited use cases.

glibc has never used these calls on machines with the A extension.  They're
only used in one specific header file to emulate cmpxchg, and they're guarded
by something like "#ifdef riscv_atomic".  Here's the glibc code (from a
slightly older version, glibc is also in submission so everything's a bit of a
mess there too) for reference

  /* If the A (atomic) extension is not present, we need help from the
     kernel to do atomic accesses.  Linux provides two system calls for
     this purpose.  RISCV_ATOMIC_CMPXCHG will perform an atomic compare
     and exchange operation for a 32-bit value.  RISCV_ATOMIC_CMPXCHG64
     will do the same for a 64-bit value. */

  #include <sys/syscall.h>
  #include <sysdep.h>

  #define __HAVE_64B_ATOMICS (__riscv_xlen >= 64)
  #define USE_ATOMIC_COMPILER_BUILTINS 0

  #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
    (abort (), (__typeof (*mem)) 0)

  #define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
    (abort (), (__typeof (*mem)) 0)

  /* The only basic operation needed is compare and exchange.  */
  #define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
    ({									      \
      INTERNAL_SYSCALL_DECL (__err);					      \
      (__typeof (*mem)) INTERNAL_SYSCALL (sysriscv, __err, 4,		      \
  		      RISCV_ATOMIC_CMPXCHG, mem, oldval, newval);	      \
    })

  #define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
    ({									      \
      INTERNAL_SYSCALL_DECL (__err);					      \
      (__typeof (*mem)) INTERNAL_SYSCALL (sysriscv, __err, 4,		      \
  		      RISCV_ATOMIC_CMPXCHG64, mem, oldval, newval);	      \
    })

We originally had these as a special Kconfig option, but then it was pointed
out that user binaries built on non-A systems wouldn't run on A systems.  That
seemed like a bad idea, so we just enabled it everywhere.

I think we should just table this discussion for now: we can always add the
system calls back in if people build non-A Linux systems.  We'll mark our glibc
port as requiring the A extension and delete the dead code there so nothing
knows about the syscalls.

Thanks!