| Message ID | 9359ab11b52ef7c1799337f475e1e27c0cb00e3b.1684949268.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: riscv: Add full rv32 support | expand |
On 2023-05-25 01:59:55+0800, Zhangjin Wu wrote: > rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no > __NR_pselect6 after kernel commit d4c08b9776b3 ("riscv: Use latest > system call ABI"), use __NR_pselect6_time64 instead. > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > --- > tools/include/nolibc/sys.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > index c0335a84f880..00c7197dcd50 100644 > --- a/tools/include/nolibc/sys.h > +++ b/tools/include/nolibc/sys.h > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva > struct timeval *t; > } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; > return my_syscall1(__NR_select, &arg); > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || defined(__NR_pselect6_time64)) > +#ifdef __NR_pselect6 > struct timespec t; > +#else > + struct timespec64 t; > +#define __NR_pselect6 __NR_pselect6_time64 Wouldn't this #define leak to the users of nolibc and lead to calls to pselect6_time64 with the ABI of the __NR_pselect6 if userspace is doing its own raw syscalls? > +#endif > > if (timeout) { > t.tv_sec = timeout->tv_sec; > -- > 2.25.1 >
Hi, Thomas > On 2023-05-25 01:59:55+0800, Zhangjin Wu wrote: > > rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no > > __NR_pselect6 after kernel commit d4c08b9776b3 ("riscv: Use latest > > system call ABI"), use __NR_pselect6_time64 instead. > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > --- > > tools/include/nolibc/sys.h | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > > index c0335a84f880..00c7197dcd50 100644 > > --- a/tools/include/nolibc/sys.h > > +++ b/tools/include/nolibc/sys.h > > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva > > struct timeval *t; > > } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; > > return my_syscall1(__NR_select, &arg); > > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) > > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || defined(__NR_pselect6_time64)) > > +#ifdef __NR_pselect6 > > struct timespec t; > > +#else > > + struct timespec64 t; > > +#define __NR_pselect6 __NR_pselect6_time64 > > Wouldn't this #define leak to the users of nolibc and lead to calls to > pselect6_time64 with the ABI of the __NR_pselect6 if userspace is doing > its own raw syscalls? > Yeah, it would break the user-side raw __NR_pselect6 syscall for nolibc is a header-only libc, so, it is not safe to use such method like glibc. Something like this will let the syscall call to pselect6_time64 instead of the user-required __NR_pselect6 and pass the wrong type of argument. #include "nolibc.h" // If no __NR_pselect6 defined, __NR_pselect6 = __NR_pselect6_time64 #ifdef __NR_pselect6 struct timespec t; // come here for __NR_pselect6_time64, but t is not timespec64, broken syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); #else struct timespec64 t; syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); #endif I have used something like __NR_pselect6_time3264 locally, before sending the patchset, I found a cleaner method already used in sys.h: #ifndef __NR__newselect #define __NR__newselect __NR_select #endif But I forgot the arguments mixing issue, __NR__newselect and __NR_select share the same type of arguments, but __NR_pselect6 and __NR_pselect6_time64 not, so, I will use back the old method but still need to find a better string, just like __NR__newselect, __NR__pselect6 may be used in kernel space in the future, and __NR_pselect6_time3264 is too long, what about this? #ifdef __NR_pselect6 struct timespec t; #define __NR_pselect6__ __NR_pselect6 #else struct timespec64 t; #define __NR_pselect6__ __NR_pselect6_time64 #endif Or even ___NR_pselect6? The same issue is in this patch: [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 will solve it with the same method. Thanks, Zhangjin > > > +#endif > > > > if (timeout) { > > t.tv_sec = timeout->tv_sec; > > -- > > 2.25.1 > >
On 2023-05-25 15:10:21+0800, Zhangjin Wu wrote: > Hi, Thomas > > > On 2023-05-25 01:59:55+0800, Zhangjin Wu wrote: > > > rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no > > > __NR_pselect6 after kernel commit d4c08b9776b3 ("riscv: Use latest > > > system call ABI"), use __NR_pselect6_time64 instead. > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > > --- > > > tools/include/nolibc/sys.h | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > > > index c0335a84f880..00c7197dcd50 100644 > > > --- a/tools/include/nolibc/sys.h > > > +++ b/tools/include/nolibc/sys.h > > > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva > > > struct timeval *t; > > > } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; > > > return my_syscall1(__NR_select, &arg); > > > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) > > > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || defined(__NR_pselect6_time64)) > > > +#ifdef __NR_pselect6 > > > struct timespec t; > > > +#else > > > + struct timespec64 t; > > > +#define __NR_pselect6 __NR_pselect6_time64 > > > > Wouldn't this #define leak to the users of nolibc and lead to calls to > > pselect6_time64 with the ABI of the __NR_pselect6 if userspace is doing > > its own raw syscalls? > > > > Yeah, it would break the user-side raw __NR_pselect6 syscall for nolibc is a > header-only libc, so, it is not safe to use such method like glibc. > > Something like this will let the syscall call to pselect6_time64 instead of the > user-required __NR_pselect6 and pass the wrong type of argument. > > #include "nolibc.h" // If no __NR_pselect6 defined, __NR_pselect6 = __NR_pselect6_time64 > > #ifdef __NR_pselect6 > struct timespec t; // come here for __NR_pselect6_time64, but t is not timespec64, broken > syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); > #else > struct timespec64 t; > syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); > #endif > > I have used something like __NR_pselect6_time3264 locally, before > sending the patchset, I found a cleaner method already used in sys.h: > > #ifndef __NR__newselect > #define __NR__newselect __NR_select > #endif > > But I forgot the arguments mixing issue, __NR__newselect and __NR_select > share the same type of arguments, but __NR_pselect6 and > __NR_pselect6_time64 not, so, I will use back the old method but still > need to find a better string, just like __NR__newselect, __NR__pselect6 > may be used in kernel space in the future, and __NR_pselect6_time3264 is > too long, what about this? > > #ifdef __NR_pselect6 > struct timespec t; > #define __NR_pselect6__ __NR_pselect6 > #else > struct timespec64 t; > #define __NR_pselect6__ __NR_pselect6_time64 > #endif > > Or even ___NR_pselect6? What about: #ifdef __NR_pselect6 struct timespec t; const long nr_pselect = __NR_pselect6; #else struct timespec64 t; const long nr_pselect = __NR_pselect6_time64; #endif > > The same issue is in this patch: > > [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 > > will solve it with the same method. Thanks!
> On 2023-05-25 15:10:21+0800, Zhangjin Wu wrote: > > Hi, Thomas > > > > > On 2023-05-25 01:59:55+0800, Zhangjin Wu wrote: > > > > rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no > > > > __NR_pselect6 after kernel commit d4c08b9776b3 ("riscv: Use latest > > > > system call ABI"), use __NR_pselect6_time64 instead. > > > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > > > --- > > > > tools/include/nolibc/sys.h | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > > > > index c0335a84f880..00c7197dcd50 100644 > > > > --- a/tools/include/nolibc/sys.h > > > > +++ b/tools/include/nolibc/sys.h > > > > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva > > > > struct timeval *t; > > > > } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; > > > > return my_syscall1(__NR_select, &arg); > > > > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) > > > > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || defined(__NR_pselect6_time64)) > > > > +#ifdef __NR_pselect6 > > > > struct timespec t; > > > > +#else > > > > + struct timespec64 t; > > > > +#define __NR_pselect6 __NR_pselect6_time64 > > > > > > Wouldn't this #define leak to the users of nolibc and lead to calls to > > > pselect6_time64 with the ABI of the __NR_pselect6 if userspace is doing > > > its own raw syscalls? > > > > > > > Yeah, it would break the user-side raw __NR_pselect6 syscall for nolibc is a > > header-only libc, so, it is not safe to use such method like glibc. > > > > Something like this will let the syscall call to pselect6_time64 instead of the > > user-required __NR_pselect6 and pass the wrong type of argument. > > > > #include "nolibc.h" // If no __NR_pselect6 defined, __NR_pselect6 = __NR_pselect6_time64 > > > > #ifdef __NR_pselect6 > > struct timespec t; // come here for __NR_pselect6_time64, but t is not timespec64, broken > > syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); > > #else > > struct timespec64 t; > > syscall(__NR_pselect6, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); > > #endif > > > > I have used something like __NR_pselect6_time3264 locally, before > > sending the patchset, I found a cleaner method already used in sys.h: > > > > #ifndef __NR__newselect > > #define __NR__newselect __NR_select > > #endif > > > > But I forgot the arguments mixing issue, __NR__newselect and __NR_select > > share the same type of arguments, but __NR_pselect6 and > > __NR_pselect6_time64 not, so, I will use back the old method but still > > need to find a better string, just like __NR__newselect, __NR__pselect6 > > may be used in kernel space in the future, and __NR_pselect6_time3264 is > > too long, what about this? > > > > #ifdef __NR_pselect6 > > struct timespec t; > > #define __NR_pselect6__ __NR_pselect6 > > #else > > struct timespec64 t; > > #define __NR_pselect6__ __NR_pselect6_time64 > > #endif > > > > Or even ___NR_pselect6? > > What about: > > #ifdef __NR_pselect6 > struct timespec t; > const long nr_pselect = __NR_pselect6; > #else > struct timespec64 t; > const long nr_pselect = __NR_pselect6_time64; > #endif > It looks better and cleaner, will apply this method, thanks! > > > > The same issue is in this patch: > > > > [PATCH 13/13] tools/nolibc: sys_gettimeofday: riscv: use __NR_clock_gettime64 > > > > will solve it with the same method. > And also this one: [PATCH 09/13] tools/nolibc: sys_poll: riscv: use __NR_ppoll_time64 Have tested all of them, will send a v2 later. Best regards, Zhangjin > Thanks!
On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote: > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > index c0335a84f880..00c7197dcd50 100644 > --- a/tools/include/nolibc/sys.h > +++ b/tools/include/nolibc/sys.h > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set > *wfds, fd_set *efds, struct timeva > struct timeval *t; > } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; > return my_syscall1(__NR_select, &arg); > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || > defined(__NR_pselect6_time64)) > +#ifdef __NR_pselect6 > struct timespec t; > +#else > + struct timespec64 t; > +#define __NR_pselect6 __NR_pselect6_time64 > +#endif Overriding the __NR_pselect6 constant seems wrong here, this can easily lead to more bugs, as pselect6 and pselect6_time64 are not compatible because of the different arguments, so anything using the constant after including sys.h will be broken. I would just use __kernel_timespec64 unconditionally and then use __NR_pselect6_time64 on all 32-bit architectures here, but use __NR_pselect6 on 32-bit architectures. I think we can also kill off the oldselect and newselect cases, using pselect6/pselect6_time64 unconditionally here, and no longer care about building against pre-5.1 kernel headers, at least for the copy of nolibc that ships with the kernel. Arnd
Hi, Arnd > On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote: > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > > index c0335a84f880..00c7197dcd50 100644 > > --- a/tools/include/nolibc/sys.h > > +++ b/tools/include/nolibc/sys.h > > @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set > > *wfds, fd_set *efds, struct timeva > > struct timeval *t; > > } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; > > return my_syscall1(__NR_select, &arg); > > -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) > > +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || > > defined(__NR_pselect6_time64)) > > +#ifdef __NR_pselect6 > > struct timespec t; > > +#else > > + struct timespec64 t; > > +#define __NR_pselect6 __NR_pselect6_time64 > > +#endif > > Overriding the __NR_pselect6 constant seems wrong here, this can > easily lead to more bugs, as pselect6 and pselect6_time64 are > not compatible because of the different arguments, so anything > using the constant after including sys.h will be broken. > Yes, thanks, Thomas also pointed out this issue in another reply of this message thread, it has been fixed locally with his suggestion, it looks like this: #ifdef __NR_pselect6 struct timespec t; const long nr_pselect = __NR_pselect6; #else struct timespec64 t; const long nr_pselect = __NR_pselect6_time64; #endif if (timeout) { t.tv_sec = timeout->tv_sec; t.tv_nsec = timeout->tv_usec * 1000; } return my_syscall6(nr_pselect, nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL); I have applied this method for ppoll_time64 and clock_gettime64 too, this method can save several duplicated lines for us, I have prepared v2 patches locally for them. > I would just use __kernel_timespec64 unconditionally and then > use __NR_pselect6_time64 on all 32-bit architectures here, > but use __NR_pselect6 on 32-bit architectures. > The 2nd 32-bit you mean is 64-bit? This is related to the timespec64/time64_t definitions as you commented in another reply. I will learn how to use the one from <linux/time_types.h>, it may require to clean up the existing files in tools/include/nolibc/ at first. > I think we can also kill off the oldselect and newselect > cases, using pselect6/pselect6_time64 unconditionally here, > and no longer care about building against pre-5.1 kernel > headers, at least for the copy of nolibc that ships with the > kernel. As Willy commented in another reply, users may want to copy the recent one and use them with an old kernel, even if want to drop them, a standalone patch may be preferable. Thanks very much, Zhangjin > > Arnd
On Fri, May 26, 2023, at 13:00, Zhangjin Wu wrote: >> On Wed, May 24, 2023, at 19:59, Zhangjin Wu wrote: >> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h > > I have applied this method for ppoll_time64 and clock_gettime64 too, > this method can save several duplicated lines for us, I have prepared v2 > patches locally for them. Ok, that addresses my concern about the possible bugs. >> I would just use __kernel_timespec64 unconditionally and then >> use __NR_pselect6_time64 on all 32-bit architectures here, >> but use __NR_pselect6 on 32-bit architectures. >> > > The 2nd 32-bit you mean is 64-bit? Yes, sorry for the typo. > This is related to the timespec64/time64_t definitions as you commented > in another reply. I will learn how to use the one from > <linux/time_types.h>, it may require to clean up the existing files in > tools/include/nolibc/ at first. Ok, thanks. >> I think we can also kill off the oldselect and newselect >> cases, using pselect6/pselect6_time64 unconditionally here, >> and no longer care about building against pre-5.1 kernel >> headers, at least for the copy of nolibc that ships with the >> kernel. > > As Willy commented in another reply, users may want to copy the recent > one and use them with an old kernel, even if want to drop them, a > standalone patch may be preferable. Fair enough. I checked the old versions and see that 5.1 in May 2019 was the first one to include the time64 system call definitions, though 5.6 from March 2020 was the first version that I consider fully working with time64. I don't know how common it is to copy nolibc into other projects, but requiring a three year old kernel might be a little too aggressive here. They could copy from 6.1-stable to keep the fallback (and miss rv32) if we do this, but a better cutoff may be Dec 2025 when linux-5.4 has its "projected EOL" date and one last LTS with the fallback (linux-6.16 or so) gets released. Arnd
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index c0335a84f880..00c7197dcd50 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -1041,8 +1041,13 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva struct timeval *t; } arg = { .n = nfds, .r = rfds, .w = wfds, .e = efds, .t = timeout }; return my_syscall1(__NR_select, &arg); -#elif defined(__ARCH_WANT_SYS_PSELECT6) && defined(__NR_pselect6) +#elif defined(__ARCH_WANT_SYS_PSELECT6) && (defined(__NR_pselect6) || defined(__NR_pselect6_time64)) +#ifdef __NR_pselect6 struct timespec t; +#else + struct timespec64 t; +#define __NR_pselect6 __NR_pselect6_time64 +#endif if (timeout) { t.tv_sec = timeout->tv_sec;
rv32 uses the generic include/uapi/asm-generic/unistd.h and it has no __NR_pselect6 after kernel commit d4c08b9776b3 ("riscv: Use latest system call ABI"), use __NR_pselect6_time64 instead. Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/include/nolibc/sys.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)