| Message ID | ec5af2ae25264eddce4b50380bfd24f9490eca75.1684949268.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tools/nolibc: riscv: Add full rv32 support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
On 2023-05-25 01:57:24+0800, Zhangjin Wu wrote: > rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no > __NR_ppoll after kernel commit d4c08b9776b3 ("riscv: Use latest system > call ABI"), use __NR_ppoll_time64 instead. > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > --- > tools/include/nolibc/std.h | 1 + > tools/include/nolibc/sys.h | 7 ++++++- > tools/include/nolibc/types.h | 6 ++++++ > 3 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/tools/include/nolibc/std.h b/tools/include/nolibc/std.h > index 83c0b0cb9564..221385c0e823 100644 > --- a/tools/include/nolibc/std.h > +++ b/tools/include/nolibc/std.h > @@ -32,6 +32,7 @@ typedef signed long off_t; > typedef signed long blksize_t; > typedef signed long blkcnt_t; > typedef signed long time_t; > +typedef long long time64_t; > typedef long long loff_t; > > #endif /* _NOLIBC_STD_H */ > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > index 0ff77c0a06d7..08d38175bd7b 100644 > --- a/tools/include/nolibc/sys.h > +++ b/tools/include/nolibc/sys.h > @@ -923,8 +923,13 @@ int pivot_root(const char *new, const char *old) > static __attribute__((unused)) > int sys_poll(struct pollfd *fds, int nfds, int timeout) > { > -#if defined(__NR_ppoll) > +#if defined(__NR_ppoll) || defined(__NR_ppoll_time64) > +#ifdef __NR_ppoll > struct timespec t; > +#else > + struct timespec64 t; > +#define __NR_ppoll __NR_ppoll_time64 > +#endif > > if (timeout >= 0) { > t.tv_sec = timeout / 1000; > diff --git a/tools/include/nolibc/types.h b/tools/include/nolibc/types.h > index 15b0baffd336..ee914391439c 100644 > --- a/tools/include/nolibc/types.h > +++ b/tools/include/nolibc/types.h > @@ -203,6 +203,12 @@ struct stat { > time_t st_ctime; /* time of last status change */ > }; > > +/* needed by time64 syscalls */ > +struct timespec64 { > + time64_t tv_sec; /* seconds */ > + long tv_nsec; /* nanoseconds */ > +}; A question to you and Willy, as it's also done the same for other types: What is the advantage of custom definitions over using the one from the kernel (maybe via a typedef). From linux/time_types.h: struct __kernel_timespec { __kernel_time64_t tv_set; long long tv_nsec; }; > + > /* WARNING, it only deals with the 4096 first majors and 256 first minors */ > #define makedev(major, minor) ((dev_t)((((major) & 0xfff) << 8) | ((minor) & 0xff))) > #define major(dev) ((unsigned int)(((dev) >> 8) & 0xfff)) > -- > 2.25.1 >
On Fri, May 26, 2023, at 09:15, Thomas Weißschuh wrote: > On 2023-05-25 01:57:24+0800, Zhangjin Wu wrote: >> rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no >> >> +/* needed by time64 syscalls */ >> +struct timespec64 { >> + time64_t tv_sec; /* seconds */ >> + long tv_nsec; /* nanoseconds */ >> +}; > > A question to you and Willy, as it's also done the same for other types: > > What is the advantage of custom definitions over using the one from the > kernel (maybe via a typedef). > > From linux/time_types.h: > > struct __kernel_timespec { > __kernel_time64_t tv_set; > long long tv_nsec; > }; I agree the __kernel_* types are what we should be using when interacting with system calls directly, that is definitely what they are intended for. I would go further here and completely drop support for 32-bit time_t/off_t and derived types in nolibc. Unfortunately, the kernel's include/uapi/linux/time.h header still defines the old types, this is one of the last remnants the time32 syscalls definitions in the kernel headers, and this already conflicts with the glibc and musl definitions, so anything that includes this header is broken on real systems. I think it makes most sense for nolibc to just use the linux/time_types.h header instead and use something like #define timespec __kernel_timespec #define itimerspec __kernel_itimerspec typedef __kernel_time64_t time_t; /* timeval is only provided for users, not compatible with syscalls */ struct timeval { __kernel_time64_t tv_sec; __s64 tv_nsec; }; so we can drop all the fallbacks for old 32-bit targets. This also allows running with CONFIG_COMPAT_32BIT_TIME disabled. Arnd
Hi, Arnd, Thomas, Willy > On Fri, May 26, 2023, at 09:15, Thomas Wei=C3=9Fschuh wrote: > > On 2023-05-25 01:57:24+0800, Zhangjin Wu wrote: > >> > >> +/* needed by time64 syscalls */ > >> +struct timespec64 { > >> + time64_t tv_sec; /* seconds */ > >> + long tv_nsec; /* nanoseconds */ > >> +}; > > > > A question to you and Willy, as it's also done the same for other types: > > > > What is the advantage of custom definitions over using the one from the > > kernel (maybe via a typedef). > > > > From linux/time_types.h: > > > > struct __kernel_timespec { > > __kernel_time64_t tv_set; > > long long tv_nsec; > > }; > > I agree the __kernel_* types are what we should be using when > interacting with system calls directly, that is definitely what > they are intended for. > > I would go further here and completely drop support for 32-bit > time_t/off_t and derived types in nolibc. Unfortunately, the > kernel's include/uapi/linux/time.h header still defines the > old types, this is one of the last remnants the time32 syscalls > definitions in the kernel headers, and this already conflicts > with the glibc and musl definitions, so anything that includes > this header is broken on real systems. I think it makes most > sense for nolibc to just use the linux/time_types.h header > instead and use something like > > #define timespec __kernel_timespec > #define itimerspec __kernel_itimerspec > typedef __kernel_time64_t time_t; > /* timeval is only provided for users, not compatible with syscalls */ > struct timeval { __kernel_time64_t tv_sec; __s64 tv_nsec; }; > > so we can drop all the fallbacks for old 32-bit targets. This > also allows running with CONFIG_COMPAT_32BIT_TIME disabled. Just a status update ... I'm working on the pure time64 and 64bit off_t syscalls support, it almost worked (tested on rv32/64, arm32/64), thanks very much for your suggestions. It includes: * Based on linux/types.h and * Use 64bit off_t * Use 64bit time_t * the new std.h looks like this typedef uint32_t __kernel_dev_t; typedef __kernel_dev_t dev_t; typedef __kernel_ulong_t ino_t; typedef __kernel_mode_t mode_t; typedef __kernel_pid_t pid_t; typedef __kernel_uid32_t uid_t; typedef __kernel_gid32_t gid_t; typedef __kernel_loff_t off_t; typedef __kernel_time64_t time_t; typedef uint32_t nlink_t; typedef uint64_t blksize_t; typedef uint64_t blkcnt_t; * Use __kernel_timespec as timespec * Use 64bit time_t based struct timeval * Disable gettimeofday syscall completely for 32bit platforms * And disable the gettimeofday_bad1/2 test case too * Remove the oldselect and newslect path completely * The new types.h looks this: /* always use time64 structs in user-space even on 32bit platforms */ #define timespec __kernel_timespec #define itimerspec __kernel_itimerspec /* timeval is only provided for users, not compatible with syscalls */ struct __timeval64 { __kernel_time64_t tv_sec; /* seconds */ __s64 tv_usec; /* microseconds */ }; /* override the 32bit version of struct timeval in linux/time.h */ #define timeval __timeval64 /* itimerval is only provided for users, not compatible with syscalls */ struct __itimerval64 { struct __timeval64 it_interval; /* timer interval */ struct __timeval64 it_value; /* current value */ }; /* override the 32bit version of struct itimerval in linux/time.h */ #define itimerval __itimerval64 * Use __NR_*time64 for all 32bit platforms * Use __NR_pselect6/ppoll/clock_gettime only for 64bit platforms * New sizeof tests added to verify off_t, time_t, timespec, itimerspec... CASE_TEST(sizeof_time_t); EXPECT_EQ(1, 8, sizeof(time_t)); break; CASE_TEST(sizeof_timespec); EXPECT_EQ(1, 16, sizeof(struct timespec)); break; #ifdef NOLIBC CASE_TEST(sizeof_itimerspec); EXPECT_EQ(1, 32, sizeof(struct itimerspec)); break; #endif CASE_TEST(sizeof_timeval); EXPECT_EQ(1, 16, sizeof(struct timeval)); break; CASE_TEST(sizeof_itimerval); EXPECT_EQ(1, 32, sizeof(struct itimerval)); break; CASE_TEST(sizeof_off_t); EXPECT_EQ(1, 8, sizeof(off_t)); break; @Arnd, the above timeval/itimerval definitions are used to override the ones from linux/time.h to avoid such error: error: redefinition of ‘struct timeval’ nolibc/sysroot/riscv/include/types.h:225:8: error: redefinition of ‘struct timeval’ 225 | struct timeval { | ^~~~~~~ In file included from nolibc/sysroot/riscv/include/types.h:11, from nolibc/sysroot/riscv/include/nolibc.h:98, from nolibc/sysroot/riscv/include/errno.h:26, from nolibc/sysroot/riscv/include/stdio.h:14, from tools/testing/selftests/nolibc/nolibc-test.c:12: nolibc/sysroot/riscv/include/linux/time.h:16:8: note: originally defined here 16 | struct timeval { @Arnd, As you commented in another reply, is it time for us to update include/uapi/linux/time.h together and let it provide time64 timeval/itimerval instead of the old ones? perhaps some libc's are still using them. Or perhaps we can add a switch like __ARCH_WANT_TIME32_SYSCALLS, add a __ARCH_WANT_TIME32_STRUCTS and simply bind it with __ARCH_WANT_TIME32_SYSCALLS? About the above ugly override code, What's your suggestion in v2? ;-) Best regards, Zhangjin > > Arnd
On Sun, May 28, 2023, at 10:25, Zhangjin Wu wrote: >> On Fri, May 26, 2023, at 09:15, Thomas Wei=C3=9Fschuh wrote: > * Use __NR_*time64 for all 32bit platforms > * Use __NR_pselect6/ppoll/clock_gettime only for 64bit platforms > * New sizeof tests added to verify off_t, time_t, timespec, itimerspec... > > CASE_TEST(sizeof_time_t); EXPECT_EQ(1, 8, > sizeof(time_t)); break; > CASE_TEST(sizeof_timespec); EXPECT_EQ(1, 16, > sizeof(struct timespec)); break; > #ifdef NOLIBC > CASE_TEST(sizeof_itimerspec); EXPECT_EQ(1, 32, > sizeof(struct itimerspec)); break; > #endif > CASE_TEST(sizeof_timeval); EXPECT_EQ(1, 16, > sizeof(struct timeval)); break; > CASE_TEST(sizeof_itimerval); EXPECT_EQ(1, 32, > sizeof(struct itimerval)); break; > CASE_TEST(sizeof_off_t); EXPECT_EQ(1, 8, > sizeof(off_t)); break; > > > @Arnd, the above timeval/itimerval definitions are used to override the ones > from linux/time.h to avoid such error: > > error: redefinition of ‘struct timeval’ > > nolibc/sysroot/riscv/include/types.h:225:8: error: redefinition of > ‘struct timeval’ > 225 | struct timeval { > | ^~~~~~~ > In file included from nolibc/sysroot/riscv/include/types.h:11, > from nolibc/sysroot/riscv/include/nolibc.h:98, > from nolibc/sysroot/riscv/include/errno.h:26, > from nolibc/sysroot/riscv/include/stdio.h:14, > from > tools/testing/selftests/nolibc/nolibc-test.c:12: > nolibc/sysroot/riscv/include/linux/time.h:16:8: note: originally > defined here > 16 | struct timeval { > > @Arnd, As you commented in another reply, is it time for us to update > include/uapi/linux/time.h together and let it provide time64 timeval/itimerval > instead of the old ones? perhaps some libc's are still using them. It's hard to know if anything is using it until we try. On the other hand, we also know that anything still relying on it is going to be increasingly broken on 32-bit architectures over as we get closer to y2038, so we can just try removing these here to see what happens. > Or perhaps we can add a switch like __ARCH_WANT_TIME32_SYSCALLS, add a > __ARCH_WANT_TIME32_STRUCTS and simply bind it with __ARCH_WANT_TIME32_SYSCALLS? I don't like that idea: __ARCH_WANT_TIME32_SYSCALLS tells us that an architeture still provides those syscalls for compatibility, so that is architecture specific, but __ARCH_WANT_TIME32_STRUCTS is not something that is an architecture specific decision at all, it's only needed for old source code. > About the above ugly override code, What's your suggestion in v2? ;-) Can you maybe just remove the "#include <linux/time.h>" from all include/uapi/ and nolibc headers so it just never gets seen? Unfortunately I see the header contains a few other definitions, which might have to get moved out of the way, possibly to linux/time_types.h. Arnd
Hi Zhangjin, On Sun, May 28, 2023 at 04:25:09PM +0800, Zhangjin Wu wrote: > Just a status update ... > > I'm working on the pure time64 and 64bit off_t syscalls support, it almost > worked (tested on rv32/64, arm32/64), thanks very much for your suggestions. > > It includes: > > * Based on linux/types.h and > * Use 64bit off_t > * Use 64bit time_t > * the new std.h looks like this > > typedef uint32_t __kernel_dev_t; > > typedef __kernel_dev_t dev_t; > typedef __kernel_ulong_t ino_t; > typedef __kernel_mode_t mode_t; > typedef __kernel_pid_t pid_t; > typedef __kernel_uid32_t uid_t; > typedef __kernel_gid32_t gid_t; > typedef __kernel_loff_t off_t; > typedef __kernel_time64_t time_t; > typedef uint32_t nlink_t; > typedef uint64_t blksize_t; > typedef uint64_t blkcnt_t; > > * Use __kernel_timespec as timespec > * Use 64bit time_t based struct timeval > * Disable gettimeofday syscall completely for 32bit platforms > * And disable the gettimeofday_bad1/2 test case too When you say "disable", you mean "remap", right ? Or do you mean "break in 2023 code that was expected to break only in 2038 after the hardware supporting it no longer exists" ? Thanks, Willy
On Sun, May 28, 2023, at 12:29, Willy Tarreau wrote: > On Sun, May 28, 2023 at 04:25:09PM +0800, Zhangjin Wu wrote: >> >> * Use __kernel_timespec as timespec >> * Use 64bit time_t based struct timeval >> * Disable gettimeofday syscall completely for 32bit platforms >> * And disable the gettimeofday_bad1/2 test case too > > When you say "disable", you mean "remap", right ? Or do you mean > "break in 2023 code that was expected to break only in 2038 after clock_gettime() has been supported for a very long time, so both time() and gettimeofday() can be trivial wrappers around that. Nothing really should be using the timezone argument, so I'd just ignore that in nolibc. (it's a little trickier for /sbin/init setting the initial timezone, but I hope we can ignore that here). clock_gettime() as a function call that takes a timespec argument in turn should be a wrapper around either sys_clock_gettime64 (on 32-bit architectures) or sys_clock_gettime_old() (on 64-bit architectures, or as a fallback on old 32-bit kernels after clock_gettime64 fails). On normal libc implementations, the low-level sys_clock_gettime64() and sys_clock_gettime_old(), whatever they are named, would call vdso first and then fall back to the syscall, but I don't think that's necessary for nolibc. I'd define them the same as the kernel, with sys_clock_gettime64() taking a __kernel_timespec, and sys_clock_gettime_old() takeing a __kernel_old_timespec. Arnd
On Sun, May 28, 2023 at 12:55:47PM +0200, Arnd Bergmann wrote: > On Sun, May 28, 2023, at 12:29, Willy Tarreau wrote: > > On Sun, May 28, 2023 at 04:25:09PM +0800, Zhangjin Wu wrote: > >> > >> * Use __kernel_timespec as timespec > >> * Use 64bit time_t based struct timeval > >> * Disable gettimeofday syscall completely for 32bit platforms > >> * And disable the gettimeofday_bad1/2 test case too > > > > When you say "disable", you mean "remap", right ? Or do you mean > > "break in 2023 code that was expected to break only in 2038 after > > clock_gettime() has been supported for a very long time, so both > time() and gettimeofday() can be trivial wrappers around that. OK, that's what I wanted to clarify. I understood "drop" in the sense of, well, "drop" :-) > Nothing really should be using the timezone argument, so I'd > just ignore that in nolibc. (it's a little trickier for /sbin/init > setting the initial timezone, but I hope we can ignore that here). Yes I'm fine with this approach. > clock_gettime() as a function call that takes a timespec argument > in turn should be a wrapper around either sys_clock_gettime64 (on > 32-bit architectures) or sys_clock_gettime_old() (on 64-bit > architectures, or as a fallback on old 32-bit kernels after > clock_gettime64 fails). Sounds good to me. > On normal libc implementations, the low-level > sys_clock_gettime64() and sys_clock_gettime_old(), whatever > they are named, would call vdso first and then fall back > to the syscall, but I don't think that's necessary for nolibc. Indeed, we don't exploit the VDSO here since it's essentially useful for performance and that's not what we're seeking. > I'd define them the same as the kernel, with > sys_clock_gettime64() taking a __kernel_timespec, and > sys_clock_gettime_old() takeing a __kernel_old_timespec. Sounds good, thanks Arnd! Willy
diff --git a/tools/include/nolibc/std.h b/tools/include/nolibc/std.h index 83c0b0cb9564..221385c0e823 100644 --- a/tools/include/nolibc/std.h +++ b/tools/include/nolibc/std.h @@ -32,6 +32,7 @@ typedef signed long off_t; typedef signed long blksize_t; typedef signed long blkcnt_t; typedef signed long time_t; +typedef long long time64_t; typedef long long loff_t; #endif /* _NOLIBC_STD_H */ diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 0ff77c0a06d7..08d38175bd7b 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -923,8 +923,13 @@ int pivot_root(const char *new, const char *old) static __attribute__((unused)) int sys_poll(struct pollfd *fds, int nfds, int timeout) { -#if defined(__NR_ppoll) +#if defined(__NR_ppoll) || defined(__NR_ppoll_time64) +#ifdef __NR_ppoll struct timespec t; +#else + struct timespec64 t; +#define __NR_ppoll __NR_ppoll_time64 +#endif if (timeout >= 0) { t.tv_sec = timeout / 1000; diff --git a/tools/include/nolibc/types.h b/tools/include/nolibc/types.h index 15b0baffd336..ee914391439c 100644 --- a/tools/include/nolibc/types.h +++ b/tools/include/nolibc/types.h @@ -203,6 +203,12 @@ struct stat { time_t st_ctime; /* time of last status change */ }; +/* needed by time64 syscalls */ +struct timespec64 { + time64_t tv_sec; /* seconds */ + long tv_nsec; /* nanoseconds */ +}; + /* WARNING, it only deals with the 4096 first majors and 256 first minors */ #define makedev(major, minor) ((dev_t)((((major) & 0xfff) << 8) | ((minor) & 0xff))) #define major(dev) ((unsigned int)(((dev) >> 8) & 0xfff))
rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no __NR_ppoll after kernel commit d4c08b9776b3 ("riscv: Use latest system call ABI"), use __NR_ppoll_time64 instead. Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/include/nolibc/std.h | 1 + tools/include/nolibc/sys.h | 7 ++++++- tools/include/nolibc/types.h | 6 ++++++ 3 files changed, 13 insertions(+), 1 deletion(-)