[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180928024633.GR17995@brightrain.aerifal.cx>
Date: Thu, 27 Sep 2018 22:46:33 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: riscv port for review
On Thu, Sep 27, 2018 at 10:24:04PM -0400, Rich Felker wrote:
> Pulled from here:
> https://github.com/riscv/riscv-musl/commit/6a4f4a9c774608add4b02f95322518bd2f5f51ee
>
> Attached for review.
Review inline below:
> diff --git a/arch/riscv32/atomic_arch.h b/arch/riscv32/atomic_arch.h
> new file mode 100644
> index 0000000..93c89cc
> --- /dev/null
> +++ b/arch/riscv32/atomic_arch.h
> @@ -0,0 +1,35 @@
> +#define a_barrier a_barrier
> +static inline void a_barrier()
> +{
> + __asm__ __volatile__ ("fence rw,rw" : : : "memory");
> +}
> +
> +#define a_ll a_ll
> +static inline int a_ll(volatile int *p)
> +{
> + int v;
> + __asm__ __volatile__ ("lr.w %0, (%1)" : "=&r"(v) : "r"(p));
> + return v;
> +}
> +
> +#define a_sc a_sc
> +static inline int a_sc(volatile int *p, int v)
> +{
> + int r;
> + __asm__ __volatile__ ("sc.w %0, %2, (%1)" : "=&r"(r) : "r"(p), "r"(v) : "memory");
> + return !r;
> +}
> +
> +#define a_cas a_cas
> +static inline int a_cas(volatile int *p, int t, int s)
> +{
> + int old, tmp;
> + __asm__("1: lr.w %0, %2 \n"
> + " bne %0, %3, 1f \n"
> + " sc.w %1, %4, %2 \n"
> + " bnez %1, 1b \n"
> + "1: \n"
> + : "=&r"(old), "+r"(tmp), "+A"(*p)
> + : "r"(t), "r"(s));
> + return old;
> +}
Why are both a_ll/a_sc and a_cas defined, and why is a_cas missing
barriers? Normally if a_ll/a_sc/a_barrier are defined, the top-level
atomic.h should be allowed to generate a_cas in terms of them.
> diff --git a/arch/riscv32/bits/signal.h b/arch/riscv32/bits/signal.h
> new file mode 100644
> index 0000000..8b992cc
> --- /dev/null
> +++ b/arch/riscv32/bits/signal.h
> @@ -0,0 +1,113 @@
> +#if defined(_POSIX_SOURCE) || defined(_POSIX_C_SOURCE) \
> + || defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +
> +#if defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +# define MINSIGSTKSZ 2048
> +# define SIGSTKSZ 8192
> +#endif
> +
> +/* gregs[0] holds the program counter. */
> +
> +#if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +typedef unsigned long greg_t;
> +typedef unsigned long gregset_t[32];
> +
> +struct __riscv_f_ext_state {
> + unsigned int f[32];
> + unsigned int fcsr;
> +};
> +
> +struct __riscv_d_ext_state {
> + unsigned long long f[32];
> + unsigned int fcsr;
> +};
> +
> +struct __riscv_q_ext_state {
> + unsigned long long f[64] __attribute__((aligned(16)));
> + unsigned int fcsr;
> + unsigned int reserved[3];
> +};
> +
> +union __riscv_fp_state {
> + struct __riscv_f_ext_state f;
> + struct __riscv_d_ext_state d;
> + struct __riscv_q_ext_state q;
> +};
> +
> +typedef union __riscv_fp_state fpregset_t;
> +
> +typedef struct sigcontext {
> + gregset_t gregs;
> + fpregset_t fpregs;
> +} mcontext_t;
> +
> +#else
> +typedef struct {
> + unsigned long gregs[32];
> + unsigned long long fpregs[66];
> +} mcontext_t;
> +#endif
In the namespace-safe version of mcontext_t, the names gregs and
fpregs are not valid here. They would need to be __-prefixed or in
some other reserved namespace.
> +struct sigaltstack {
> + void *ss_sp;
> + int ss_flags;
> + size_t ss_size;
> +};
> +
> +typedef struct __ucontext
> +{
> + unsigned long uc_flags;
> + struct __ucontext *uc_link;
> + stack_t uc_stack;
> + sigset_t uc_sigmask;
> + char __unused[1024 / 8 - sizeof(sigset_t)];
This is an invalid array of size zero and should just be removed.
> + mcontext_t uc_mcontext;
> +} ucontext_t;
...
> diff --git a/arch/riscv32/crt_arch.h b/arch/riscv32/crt_arch.h
> new file mode 100644
> index 0000000..65187e1
> --- /dev/null
> +++ b/arch/riscv32/crt_arch.h
> @@ -0,0 +1,18 @@
> +__asm__(
> +".text\n"
> +".global " START "\n"
> +".type " START ",%function\n"
> +START ":\n"
> +".weak __global_pointer$\n"
> +".hidden __global_pointer$\n\t"
> +".option push\n"
> +".option norelax\n\t"
> +"lla gp, __global_pointer$\n"
> +".option pop\n\t"
> +"mv a0, sp\n"
> +".weak _DYNAMIC\n"
> +".hidden _DYNAMIC\n\t"
> +"lla a1, _DYNAMIC\n\t"
> +"andi sp, sp, -16\n\t"
> +"jal " START "_c"
> +);
> diff --git a/arch/riscv32/pthread_arch.h b/arch/riscv32/pthread_arch.h
> new file mode 100644
> index 0000000..feffaa4
> --- /dev/null
> +++ b/arch/riscv32/pthread_arch.h
> @@ -0,0 +1,12 @@
> +static inline struct pthread *__pthread_self()
> +{
> + char *tp;
> + __asm__ __volatile__("mv %0, tp" : "=r"(tp));
> + return (void *)(tp - sizeof(struct pthread));
> +}
> +
> +#define TLS_ABOVE_TP
> +#define GAP_ABOVE_TP 0
> +#define TP_ADJ(p) ((char *)p + sizeof(struct pthread))
> +
> +#define MC_PC gregs[0]
> diff --git a/arch/riscv32/reloc.h b/arch/riscv32/reloc.h
> new file mode 100644
> index 0000000..d057bbe
> --- /dev/null
> +++ b/arch/riscv32/reloc.h
> @@ -0,0 +1,27 @@
> +#if defined __riscv_float_abi_soft
> +#define RISCV_FP_SUFFIX "-sf"
> +#elif defined __riscv_float_abi_single
> +#define RISCV_FP_SUFFIX "-sp"
> +#elif defined __riscv_float_abi_double
> +#define RISCV_FP_SUFFIX ""
> +#endif
> +
> +#define RISCV_LDSO_HELPER(x) "riscv" #x
> +#define RISCV_LDSO(x) RISCV_LDSO_HELPER(x)
> +
> +#define LDSO_ARCH RISCV_LDSO(__riscv_xlen) RISCV_FP_SUFFIX
Elsewhere it looks like little/big endian are both options, but I see
no endian variant here. If so this needs to be fixed.
Also what is __riscv_xlen? A predefined macro that expands to 32 or
64? Since this file is just for 32-bit it should just be hard-coded
rather than assuming a macro would expand to the token 32 and not
(31+1) or some other expression equal to 32, I think.
> diff --git a/arch/riscv32/syscall_arch.h b/arch/riscv32/syscall_arch.h
> new file mode 100644
> index 0000000..bc60d1f
> --- /dev/null
> +++ b/arch/riscv32/syscall_arch.h
> @@ -0,0 +1,78 @@
> +#define __SYSCALL_LL_E(x) \
> +((union { long long ll; long l[2]; }){ .ll = x }).l[0], \
> +((union { long long ll; long l[2]; }){ .ll = x }).l[1]
> +#define __SYSCALL_LL_O(x) 0, __SYSCALL_LL_E((x))
> +
> +#define __asm_syscall(...) \
> + __asm__ __volatile__ ("scall\n\t" \
> + : "+r"(a0) : __VA_ARGS__ : "memory"); \
> + return a0; \
> +
> +static inline long __syscall0(long n)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0");
> + __asm_syscall("r"(a7))
> +}
> +
> +static inline long __syscall1(long n, long a)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0") = a;
> + __asm_syscall("r"(a7), "0"(a0))
> +}
> +
> +static inline long __syscall2(long n, long a, long b)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0") = a;
> + register long a1 __asm__("a1") = b;
> + __asm_syscall("r"(a7), "0"(a0), "r"(a1))
> +}
> +
> +static inline long __syscall3(long n, long a, long b, long c)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0") = a;
> + register long a1 __asm__("a1") = b;
> + register long a2 __asm__("a2") = c;
> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2))
> +}
> +
> +static inline long __syscall4(long n, long a, long b, long c, long d)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0") = a;
> + register long a1 __asm__("a1") = b;
> + register long a2 __asm__("a2") = c;
> + register long a3 __asm__("a3") = d;
> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2), "r"(a3))
> +}
> +
> +static inline long __syscall5(long n, long a, long b, long c, long d, long e)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0") = a;
> + register long a1 __asm__("a1") = b;
> + register long a2 __asm__("a2") = c;
> + register long a3 __asm__("a3") = d;
> + register long a4 __asm__("a4") = e;
> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2), "r"(a3), "r"(a4))
> +}
> +
> +static inline long __syscall6(long n, long a, long b, long c, long d, long e, long f)
> +{
> + register long a7 __asm__("a7") = n;
> + register long a0 __asm__("a0") = a;
> + register long a1 __asm__("a1") = b;
> + register long a2 __asm__("a2") = c;
> + register long a3 __asm__("a3") = d;
> + register long a4 __asm__("a4") = e;
> + register long a5 __asm__("a5") = f;
> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2), "r"(a3), "r"(a4), "r"(a5))
> +}
> +
> +#define VDSO_USEFUL
> +/* We don't have a clock_gettime function.
> +#define VDSO_CGT_SYM "__vdso_clock_gettime"
> +#define VDSO_CGT_VER "LINUX_2.6" */
In that case VDSO_USEFUL might as well also be omitted for now.
> diff --git a/arch/riscv64/atomic_arch.h b/arch/riscv64/atomic_arch.h
> new file mode 100644
> index 0000000..018c7fd
> --- /dev/null
> +++ b/arch/riscv64/atomic_arch.h
> @@ -0,0 +1,66 @@
> +#define a_barrier a_barrier
> +static inline void a_barrier()
> +{
> + __asm__ __volatile__ ("fence rw,rw" : : : "memory");
> +}
> +
> +#define a_ll a_ll
> +static inline int a_ll(volatile int *p)
> +{
> + int v;
> + __asm__ __volatile__ ("lr.w %0, %1" : "=&r"(v), "+A"(*p));
> + return v;
> +}
> +
> +#define a_sc a_sc
> +static inline int a_sc(volatile int *p, int v)
> +{
> + int r;
> + __asm__ __volatile__ ("sc.w %0, %2, %1" : "=&r"(r), "+A"(*p) : "r"(v) : "memory");
> +return !r;
> +}
> +
> +#define a_cas a_cas
> +static inline int a_cas(volatile int *p, int t, int s)
> +{
> + int old, tmp;
> + __asm__("1: lr.w %0, %2 \n"
> + " bne %0, %3, 1f \n"
> + " sc.w %1, %4, %2 \n"
> + " bnez %1, 1b \n"
> + "1: \n"
> + : "=&r"(old), "+r"(tmp), "+A"(*p)
> + : "r"(t), "r"(s));
> + return old;
> +}
> +
> +#define a_ll_p a_ll_p
> +static inline void *a_ll_p(volatile void *p)
> +{
> + void *v;
> + __asm__ __volatile__ ("lr.d %0, %1" : "=&r"(v), "+A"(*(long *)p));
> + return v;
> +}
> +
> +#define a_sc_p a_sc_p
> +static inline int a_sc_p(volatile int *p, void *v)
> +{
> + int r;
> + __asm__ __volatile__ ("sc.d %0, %2, %1" : "=&r"(r), "+A"(*(long *)p) : "r"(v) : "memory");
> + return !r;
> +}
> +
> +#define a_cas_p a_cas_p
> +static inline void *a_cas_p(volatile void *p, void *t, void *s)
> +{
> + void *old;
> + int tmp;
> + __asm__("1: lr.d %0, %2 \n"
> + " bne %0, %3, 1f \n"
> + " sc.d %1, %4, %2 \n"
> + " bnez %1, 1b \n"
> + "1: \n"
> + : "=&r"(old), "+r"(tmp), "+A"(*(long *)p)
> + : "r"(t), "r"(s));
> + return old;
> +}
Same comments about cas vs ll/sc, and lack of barrier, as 32-bit version.
> diff --git a/arch/riscv64/bits/signal.h b/arch/riscv64/bits/signal.h
> new file mode 100644
> index 0000000..8b992cc
> --- /dev/null
> +++ b/arch/riscv64/bits/signal.h
> @@ -0,0 +1,113 @@
> +#if defined(_POSIX_SOURCE) || defined(_POSIX_C_SOURCE) \
> + || defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +
> +#if defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +# define MINSIGSTKSZ 2048
> +# define SIGSTKSZ 8192
> +#endif
> +
> +/* gregs[0] holds the program counter. */
> +
> +#if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +typedef unsigned long greg_t;
> +typedef unsigned long gregset_t[32];
> +
> +struct __riscv_f_ext_state {
> + unsigned int f[32];
> + unsigned int fcsr;
> +};
> +
> +struct __riscv_d_ext_state {
> + unsigned long long f[32];
> + unsigned int fcsr;
> +};
> +
> +struct __riscv_q_ext_state {
> + unsigned long long f[64] __attribute__((aligned(16)));
> + unsigned int fcsr;
> + unsigned int reserved[3];
> +};
> +
> +union __riscv_fp_state {
> + struct __riscv_f_ext_state f;
> + struct __riscv_d_ext_state d;
> + struct __riscv_q_ext_state q;
> +};
> +
> +typedef union __riscv_fp_state fpregset_t;
> +
> +typedef struct sigcontext {
> + gregset_t gregs;
> + fpregset_t fpregs;
> +} mcontext_t;
> +
> +#else
> +typedef struct {
> + unsigned long gregs[32];
> + unsigned long long fpregs[66];
> +} mcontext_t;
> +#endif
> +
> +struct sigaltstack {
> + void *ss_sp;
> + int ss_flags;
> + size_t ss_size;
> +};
> +
> +typedef struct __ucontext
> +{
> + unsigned long uc_flags;
> + struct __ucontext *uc_link;
> + stack_t uc_stack;
> + sigset_t uc_sigmask;
> + char __unused[1024 / 8 - sizeof(sigset_t)];
> + mcontext_t uc_mcontext;
> +} ucontext_t;
Same issues here as 32-bit.
> diff --git a/arch/riscv64/reloc.h b/arch/riscv64/reloc.h
> new file mode 100644
> index 0000000..8bd90dd
> --- /dev/null
> +++ b/arch/riscv64/reloc.h
> @@ -0,0 +1,27 @@
> +#if defined __riscv_float_abi_soft
> +#define RISCV_FP_SUFFIX "-sf"
> +#elif defined __riscv_float_abi_single
> +#define RISCV_FP_SUFFIX "-sp"
> +#elif defined __riscv_float_abi_double
> +#define RISCV_FP_SUFFIX ""
> +#endif
> +
> +#define RISCV_LDSO_HELPER(x) "riscv" #x
> +#define RISCV_LDSO(x) RISCV_LDSO_HELPER(x)
> +
> +#define LDSO_ARCH RISCV_LDSO(__riscv_xlen) RISCV_FP_SUFFIX
Same here.
> diff --git a/configure b/configure
> index 997e665..4d3d8b4 100755
> --- a/configure
> +++ b/configure
> @@ -322,6 +322,8 @@ microblaze*) ARCH=microblaze ;;
> or1k*) ARCH=or1k ;;
> powerpc64*) ARCH=powerpc64 ;;
> powerpc*) ARCH=powerpc ;;
> +riscv64*) ARCH=riscv64 ;;
> +riscv*) ARCH=riscv32 ;;
> sh[1-9bel-]*|sh|superh*) ARCH=sh ;;
> s390x*) ARCH=s390x ;;
> unknown) fail "$0: unable to detect target arch; try $0 --target=..." ;;
> @@ -640,6 +642,11 @@ trycppif __LITTLE_ENDIAN__ "$t" && SUBARCH=${SUBARCH}le
> trycppif _SOFT_FLOAT "$t" && fail "$0: error: soft-float not supported on powerpc64"
> fi
>
> +if test "$ARCH" = "riscv" || test "$ARCH" = "riscv64" ; then
> +trycppif "RISCVEB || _RISCVEB || __RISCVEB || __RISCVEB__" "$t" && SUBARCH=${SUBARCH}eb
Predefined macros that violate the namespace (RISCVEB) shouldn't be
defined or observed.
> +trycppif __riscv_soft_float "$t" && SUBARCH=${SUBARCH}-sf
> +fi
> +
> if test "$ARCH" = "sh" ; then
> tryflag CFLAGS_AUTO -Wa,--isa=any
> trycppif __BIG_ENDIAN__ "$t" && SUBARCH=${SUBARCH}eb
> diff --git a/crt/riscv32/crti.s b/crt/riscv32/crti.s
> new file mode 100644
> index 0000000..6916bfd
> --- /dev/null
> +++ b/crt/riscv32/crti.s
> @@ -0,0 +1,11 @@
> +.section .init
> +.global _init
> +.type _init,%function
> +_init:
> + ret
> +
> +.section .fini
> +.global _fini
> +.type _fini,%function
> +_fini:
> + ret
> diff --git a/crt/riscv32/crtn.s b/crt/riscv32/crtn.s
> new file mode 100644
> index 0000000..e69de29
It looks like these are not used, right?
> diff --git a/include/elf.h b/include/elf.h
> index c229735..ec2e8fd 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -3164,6 +3164,62 @@ enum
> #define R_BPF_NONE 0
> #define R_BPF_MAP_FD 1
>
> +#define R_RISCV_NONE 0
> +#define R_RISCV_32 1
> +#define R_RISCV_64 2
> +#define R_RISCV_RELATIVE 3
> +#define R_RISCV_COPY 4
> +#define R_RISCV_JUMP_SLOT 5
> +#define R_RISCV_TLS_DTPMOD32 6
> +#define R_RISCV_TLS_DTPMOD64 7
> +#define R_RISCV_TLS_DTPREL32 8
> +#define R_RISCV_TLS_DTPREL64 9
> +#define R_RISCV_TLS_TPREL32 10
> +#define R_RISCV_TLS_TPREL64 11
> +
> +#define R_RISCV_BRANCH 16
> +#define R_RISCV_JAL 17
> +#define R_RISCV_CALL 18
> +#define R_RISCV_CALL_PLT 19
> +#define R_RISCV_GOT_HI20 20
> +#define R_RISCV_TLS_GOT_HI20 21
> +#define R_RISCV_TLS_GD_HI20 22
> +#define R_RISCV_PCREL_HI20 23
> +#define R_RISCV_PCREL_LO12_I 24
> +#define R_RISCV_PCREL_LO12_S 25
> +#define R_RISCV_HI20 26
> +#define R_RISCV_LO12_I 27
> +#define R_RISCV_LO12_S 28
> +#define R_RISCV_TPREL_HI20 29
> +#define R_RISCV_TPREL_LO12_I 30
> +#define R_RISCV_TPREL_LO12_S 31
> +#define R_RISCV_TPREL_ADD 32
> +#define R_RISCV_ADD8 33
> +#define R_RISCV_ADD16 34
> +#define R_RISCV_ADD32 35
> +#define R_RISCV_ADD64 36
> +#define R_RISCV_SUB8 37
> +#define R_RISCV_SUB16 38
> +#define R_RISCV_SUB32 39
> +#define R_RISCV_SUB64 40
> +#define R_RISCV_GNU_VTINHERIT 41
> +#define R_RISCV_GNU_VTENTRY 42
> +#define R_RISCV_ALIGN 43
> +#define R_RISCV_RVC_BRANCH 44
> +#define R_RISCV_RVC_JUMP 45
> +#define R_RISCV_RVC_LUI 46
> +#define R_RISCV_GPREL_I 47
> +#define R_RISCV_GPREL_S 48
> +#define R_RISCV_TPREL_I 49
> +#define R_RISCV_TPREL_S 50
> +#define R_RISCV_RELAX 51
> +#define R_RISCV_SUB6 52
> +#define R_RISCV_SET6 53
> +#define R_RISCV_SET8 54
> +#define R_RISCV_SET16 55
> +#define R_RISCV_SET32 56
> +#define R_RISCV_32_PCREL 57
> +
> #ifdef __cplusplus
> }
> #endif
This should be its own patch independent of the port; I can commit it
earlier.
> diff --git a/src/thread/riscv32/syscall_cp.s b/src/thread/riscv32/syscall_cp.s
> new file mode 100644
> index 0000000..71bf6d3
> --- /dev/null
> +++ b/src/thread/riscv32/syscall_cp.s
> @@ -0,0 +1,29 @@
> +.global __cp_begin
> +.hidden __cp_begin
> +.global __cp_end
> +.hidden __cp_end
> +.global __cp_cancel
> +.hidden __cp_cancel
> +.hidden __cancel
> +.global __syscall_cp_asm
> +.hidden __syscall_cp_asm
> +.type __syscall_cp_asm, %function
> +__syscall_cp_asm:
> +__cp_begin:
> + lw t0, 0(a0)
> + bnez t0, __cp_cancel
> +
> + mv t0, a1
> + mv a0, a2
> + mv a1, a3
> + mv a2, a4
> + mv a3, a5
> + mv a4, a6
> + mv a5, a7
> + lw a6, 0(sp)
> + mv a7, t0
> + scall
> +__cp_cancel:
> + ret
> +__cp_end:
> + j __cancel
The labels here are backwards. __cp_end must point immediately after
the syscall instruction, and __cp_end needs to jump to __cancel.
> diff --git a/src/thread/riscv64/syscall_cp.s b/src/thread/riscv64/syscall_cp.s
> new file mode 100644
> index 0000000..c745b32
> --- /dev/null
> +++ b/src/thread/riscv64/syscall_cp.s
> @@ -0,0 +1,29 @@
> +.global __cp_begin
> +.hidden __cp_begin
> +.global __cp_end
> +.hidden __cp_end
> +.global __cp_cancel
> +.hidden __cp_cancel
> +.hidden __cancel
> +.global __syscall_cp_asm
> +.hidden __syscall_cp_asm
> +.type __syscall_cp_asm, %function
> +__syscall_cp_asm:
> +__cp_begin:
> + ld t0, 0(a0)
> + bnez t0, __cp_cancel
> +
> + mv t0, a1
> + mv a0, a2
> + mv a1, a3
> + mv a2, a4
> + mv a3, a5
> + mv a4, a6
> + mv a5, a7
> + ld a6, 0(sp)
> + mv a7, t0
> + scall
> +__cp_cancel:
> + ret
> +__cp_end:
> + j __cancel
> --
> 2.10.0
>
Likewise here.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
