Follow @Openwall on Twitter for new release announcements and other news
[<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.