[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 6 May 2024 16:58:19 -0400
From: Rich Felker <dalias@...c.org>
To: Max Filippov <jcmvbkbc@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [RFC v3 1/1] xtensa: add port
On Mon, May 06, 2024 at 11:01:12AM -0700, Max Filippov wrote:
> Signed-off-by: Max Filippov <jcmvbkbc@...il.com>
> ---
> Changes v2->v3:
> - fix semid_ds::sem_nsems type
> - drop store to GOT entry at offset 12 from arch/xtensa/crt_arch.h
> - add alignment to all assembly function entry points
> - add .literal_position directive to vfork
Can you elaborate on what .literal_position does here?
> diff --git a/arch/xtensa/bits/alltypes.h.in b/arch/xtensa/bits/alltypes.h.in
> new file mode 100644
> index 000000000000..24f4d20995af
> --- /dev/null
> +++ b/arch/xtensa/bits/alltypes.h.in
> @@ -0,0 +1,27 @@
> +#define _REDIR_TIME64 1
I don't know why I didn't notice or comment on this before, but unless
there's existing ABI you're trying to maintain, it doesn't make sense
to define _REDIR_TIME64 on new archs. That is just for compatibility
with an old time32 ABI, and since you're not adding an
arch/xtensa/arch.mk file including compat sources, that wouldn't even
get built. I think this line should probably be removed, and then
__dlsym_time64.S can be removed too and a compat-type stat struct with
the time32 fields in it does not need to be used, but instead the
arch-generic version of stat can be used.
> diff --git a/arch/xtensa/bits/ioctl.h b/arch/xtensa/bits/ioctl.h
> new file mode 100644
> index 000000000000..f30e3a699bf8
> --- /dev/null
> +++ b/arch/xtensa/bits/ioctl.h
> @@ -0,0 +1,219 @@
> +#define _IOC(a,b,c,d) ( ((a)<<30) | ((b)<<8) | (c) | ((d)<<16) )
> +#define _IOC_NONE 0U
> +#define _IOC_READ 2U
> +#define _IOC_WRITE 1U
> +
> +#define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
> +#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
> +#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
> +#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
> +
> +#define FIOCLEX _IO('f', 1)
> +#define FIONCLEX _IO('f', 2)
> +#define FIOASYNC _IOW('f', 125, int)
> +#define FIONBIO _IOW('f', 126, int)
> +#define FIONREAD _IOR('f', 127, int)
> +#define TIOCINQ FIONREAD
> +#define FIOQSIZE _IOR('f', 128, loff_t)
loff_t may not be defined here. You could just write long long.
> +#define TCGETS 0x5401
> +#define TCSETS 0x5402
> +#define TCSETSW 0x5403
> +#define TCSETSF 0x5404
> +
> +#define TCGETA 0x80127417 /* _IOR('t', 23, struct termio) */
> +#define TCSETA 0x40127418 /* _IOW('t', 24, struct termio) */
> +#define TCSETAW 0x40127419 /* _IOW('t', 25, struct termio) */
> +#define TCSETAF 0x4012741C /* _IOW('t', 28, struct termio) */
Rather than expanding these out manually where the type is missing,
you can use char[N] where N is the expected size. That's what's done
on other archs.
> diff --git a/arch/xtensa/bits/ipcstat.h b/arch/xtensa/bits/ipcstat.h
> new file mode 100644
> index 000000000000..4f4fcb0c5b74
> --- /dev/null
> +++ b/arch/xtensa/bits/ipcstat.h
> @@ -0,0 +1 @@
> +#define IPC_STAT 0x102
Looks good. FYI this is needed even once _REDIR_TIME64 is removed.
It's not about ABI compat but about the kernel and user ipc struct
types not matching and needing translation.
> diff --git a/arch/xtensa/bits/posix.h b/arch/xtensa/bits/posix.h
> new file mode 100644
> index 000000000000..30a38714f36d
> --- /dev/null
> +++ b/arch/xtensa/bits/posix.h
> @@ -0,0 +1,2 @@
> +#define _POSIX_V6_ILP32_OFFBIG 1
> +#define _POSIX_V7_ILP32_OFFBIG 1
If I get around to clearning up upstream before the port is merged,
this file might be obsolete and able to be dropped.
> diff --git a/arch/xtensa/bits/reg.h b/arch/xtensa/bits/reg.h
> new file mode 100644
> index 000000000000..0192a2931bd7
> --- /dev/null
> +++ b/arch/xtensa/bits/reg.h
> @@ -0,0 +1,2 @@
> +#undef __WORDSIZE
> +#define __WORDSIZE 32
This one too.
> diff --git a/arch/xtensa/bits/stat.h b/arch/xtensa/bits/stat.h
> new file mode 100644
> index 000000000000..31d92fec1232
> --- /dev/null
> +++ b/arch/xtensa/bits/stat.h
> @@ -0,0 +1,23 @@
> +/* copied from kernel definition, but with padding replaced
> + * by the corresponding correctly-sized userspace types. */
> +
> +struct stat {
> + dev_t st_dev;
> + ino_t st_ino;
> + mode_t st_mode;
> + nlink_t st_nlink;
> + uid_t st_uid;
> + gid_t st_gid;
> + dev_t st_rdev;
> + off_t st_size;
> + blksize_t st_blksize;
> + long __st_padding1;
> + blkcnt_t st_blocks;
> + struct {
> + long tv_sec;
> + long tv_nsec;
> + } __st_atim32, __st_mtim32, __st_ctim32;
> + struct timespec st_atim;
> + struct timespec st_mtim;
> + struct timespec st_ctim;
> +};
I'm also planning to add an arch/generic/stat.h that can probably be
used once time32-compat is not an issue.
> diff --git a/arch/xtensa/bits/stdint.h b/arch/xtensa/bits/stdint.h
> new file mode 100644
> index 000000000000..d1b2712199ac
> --- /dev/null
> +++ b/arch/xtensa/bits/stdint.h
> @@ -0,0 +1,20 @@
> +typedef int32_t int_fast16_t;
> +typedef int32_t int_fast32_t;
> +typedef uint32_t uint_fast16_t;
> +typedef uint32_t uint_fast32_t;
> +
> +#define INT_FAST16_MIN INT32_MIN
> +#define INT_FAST32_MIN INT32_MIN
> +
> +#define INT_FAST16_MAX INT32_MAX
> +#define INT_FAST32_MAX INT32_MAX
> +
> +#define UINT_FAST16_MAX UINT32_MAX
> +#define UINT_FAST32_MAX UINT32_MAX
> +
> +#define INTPTR_MIN INT32_MIN
> +#define INTPTR_MAX INT32_MAX
> +#define UINTPTR_MAX UINT32_MAX
> +#define PTRDIFF_MIN INT32_MIN
> +#define PTRDIFF_MAX INT32_MAX
> +#define SIZE_MAX UINT32_MAX
This might end up being droppable too.
> diff --git a/arch/xtensa/bits/syscall.h.in b/arch/xtensa/bits/syscall.h.in
> new file mode 100644
> index 000000000000..ec3135e12b07
> --- /dev/null
> +++ b/arch/xtensa/bits/syscall.h.in
> @@ -0,0 +1,407 @@
> [...]
> +#define __NR_gettimeofday 192
> +#define __NR_settimeofday 193
The time32 syscalls need to be renamed with _time32. See commits
5a105f19b5aae79dd302899e634b6b18b3dcd0d6,
2cae9f59da6106b4545da85b33d1e206a1e4c1e7, and
b4712ba445a5cb589d1ac37785c29164cd3cf1f9.
> diff --git a/arch/xtensa/reloc.h b/arch/xtensa/reloc.h
> new file mode 100644
> index 000000000000..cd7a455a2d9c
> --- /dev/null
> +++ b/arch/xtensa/reloc.h
> @@ -0,0 +1,32 @@
> +#if __FDPIC__
> +#define ABI_SUFFIX "-fdpic"
> +#else
> +#define ABI_SUFFIX ""
> +#endif
> +
> +#define LDSO_ARCH "xtensa" ABI_SUFFIX
The ldso name is still missing endianness, if it's intended that both
be supported. It needs to completely identify the ABI whenever there
are incompatible ABI variants.
> diff --git a/crt/xtensa/crti.S b/crt/xtensa/crti.S
> new file mode 100644
> index 000000000000..20d910820288
> --- /dev/null
> +++ b/crt/xtensa/crti.S
> @@ -0,0 +1,23 @@
> +.section .init
> +.global _init
> +.type _init, @function
> +.align 4
> +_init:
> + addi sp, sp, -16
> + s32i a0, sp, 0
> +#ifdef __FDPIC__
> + s32i a12, sp, 4
> + mov a12, a11
> +#endif
> +
> +.section .fini
> +.global _fini
> +.type _fini, @function
> +.align 4
> +_fini:
> + addi sp, sp, -16
> + s32i a0, sp, 0
> +#ifdef __FDPIC__
> + s32i a12, sp, 4
> + mov a12, a11
> +#endif
> diff --git a/crt/xtensa/crtn.S b/crt/xtensa/crtn.S
> new file mode 100644
> index 000000000000..656edaa7f146
> --- /dev/null
> +++ b/crt/xtensa/crtn.S
> @@ -0,0 +1,15 @@
> +.section .init
> +#ifdef __FDPIC__
> + l32i a12, sp, 4
> +#endif
> + l32i a0, sp, 0
> + addi sp, sp, 16
> + ret
> +
> +.section .fini
> +#ifdef __FDPIC__
> + l32i a12, sp, 4
> +#endif
> + l32i a0, sp, 0
> + addi sp, sp, 16
> + ret
It may be okay to omit these on new archs, as init/fini arrays should
always be used instead. (There was the bug with them where the ARM
folks broke and disabled them, but that should be fixed along with the
toolchain work.)
> diff --git a/src/internal/xtensa/syscall.s b/src/internal/xtensa/syscall.s
> new file mode 100644
> index 000000000000..3112bc3890bc
> --- /dev/null
> +++ b/src/internal/xtensa/syscall.s
> @@ -0,0 +1,14 @@
> +.global __syscall
> +.hidden __syscall
> +.type __syscall,@function
> +.align 4
> +__syscall:
> + mov a8, a3
> + mov a3, a4
> + mov a4, a5
> + mov a5, a6
> + mov a6, a8
> + mov a8, a7
> + l32i a9, a1, 0
> + syscall
> + ret
I don't think this is needed or used anywhere, is it? Most archs don't
have a file like this.
> diff --git a/src/thread/xtensa/__unmapself.s b/src/thread/xtensa/__unmapself.s
> new file mode 100644
> index 000000000000..8f007e84ec43
> --- /dev/null
> +++ b/src/thread/xtensa/__unmapself.s
> @@ -0,0 +1,9 @@
> +.global __unmapself
> +.type __unmapself,@function
> +.align 4
> +__unmapself:
> + mov a6, a2
> + movi a2, 81 # SYS_munmap
> + syscall
> + movi a2, 118 # SYS_exit
> + syscall
I think we established this was okay, yes?
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.
