[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMo8BfJdH3rcTDZAF58AScjXnEXLDQo0PotsYVU4tNWAy75Eow@mail.gmail.com>
Date: Mon, 6 May 2024 14:47:45 -0700
From: Max Filippov <jcmvbkbc@...il.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [RFC v3 1/1] xtensa: add port
On Mon, May 6, 2024 at 1:57 PM Rich Felker <dalias@...c.org> wrote:
>
> 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?
It's an xtensa-specific assembler directive that indicates where
the assembler may place literals when --text-section-literals
and --auto-litpools options are used. vfork implementation has
two movi instructions that are relaxed into literal loads and thus
may need this hint. I've discovered that it's missing when building
the toolchain with other target options produced build error in
vfork.s
> > 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.
Ok.
> > 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.
Ok.
> > +#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.
These lines are copied from the linux header.
> > 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.
Correct.
> > 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.
Ok.
> > 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.
Ok.
> > 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.
For each xtensa core there's only one fixed endianness and code
built for one xtensa core is not supposed to be used for any other
core, so it's not an issue, right?
> > 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.)
I'll check.
> > 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.
It must be coming from the initial xtensa port from ~2016.
I'll remove it.
> > 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?
Correct.
--
Thanks.
-- Max
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.
