Message ID | 20230710184022.3701346-2-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | Add clone3 support for multiple architectures | expand |
Hi Adhemerval, I have applied your patches and tested it on s390x. I've ensured that the wrapper is now used for pthread_create/posix_spawn and the testsuite runs fine. Nevertheless I have some minor comments: see below. And one more question: Is there a reason, why the wrapper is only added to s390x (64bit), but not for s390 (31bit)? Should there be an #undef __ASSUME_CLONE3? According to the kernel commit it is also available for compat-mode: "s390: wire up clone3 system call" https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=5518aed82d2abd97f8d3ec91d8ba455d939e8cd1 On 10.07.23 20:40, Adhemerval Zanella via Libc-alpha wrote: > It follows the internal signature: > > extern int clone3 (struct clone_args *__cl_args, size_t __size, > int (*__func) (void *__arg), void *__arg); > > Checked on s390x-linux-gnu. > --- > sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 +++++++++++++++++++ > sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + > 2 files changed, 82 insertions(+) > create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S > > diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S > new file mode 100644 > index 0000000000..417fdcb785 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S > @@ -0,0 +1,81 @@ > +/* The clone3 syscall wrapper. Linux/s390x version. > + Copyright (C) 2023 Free Software Foundation, Inc. > + > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <sysdep.h> > +#define _ERRNO_H 1 > +#include <bits/errno.h> > + > +/* The userland implementation is: > + int clone3 (struct clone_args *cl_args, size_t size, > + int (*func)(void *arg), void *arg); > + > + the kernel entry is: > + int clone3 (struct clone_args *cl_args, size_t size); > + > + The parameters are passed in registers from userland: > + r2: cl_args > + r3: size > + r4: func > + r5: arg */ > + > + .text > +ENTRY(__clone3) > + stg %r6, 48(%r15) Add cfi_offset (%r6,-112) > + > + /* Sanity check args. */ > + ltgr %r2, %r2 > + je error > + ltgr %r6, %r4 > + je error > + > + /* Do the system call, the kernel expects: > + r1: system call number > + r2: cl_args > + r3: size */ > + lghi %r1, SYS_ify(clone3) > + svc 0 > + ltgr %r2,%r2 /* check return code */ > + jz thread_start > + lg %r6, 48(%r15) > + jgm SYSCALL_ERROR_LABEL > + br %r14 > +error: > + lghi %r2,-EINVAL > + jg SYSCALL_ERROR_LABEL > +PSEUDO_END (__clone3) > + > + .align 4 The ENTRY-macro from sysdep.h is using an alignment of 16 bytes. Can you please also use it here? > + .type thread_start, %function > +thread_start: > + cfi_startproc > + /* Mark r14 as undefined in order to stop unwinding here. */ > + cfi_undefined (r14) > + > + /* func is in gpr 6, arg in gpr 5. */ > + lgr %r2, %r5 > + aghi %r15, -160 > + xc 0(8,%r15),0(%r15) > + basr %r14, %r6 > + > + DO_CALL (exit, 1) > + cfi_endproc > + .size thread_start, .-thread_start If you want, you can use ASM_SIZE_DIRECTIVE (thread_start) from sysdep.h here, which is also used in END-macros. > + > +libc_hidden_def (__clone3) > +weak_alias (__clone3, clone3) > diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h > index 3bde8ba19c..b099fea9c6 100644 > --- a/sysdeps/unix/sysv/linux/s390/sysdep.h > +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h > @@ -72,6 +72,7 @@ > #ifdef __s390x__ > #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" > #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" > +#define HAVE_CLONE3_WRAPPER 1 > #else > #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" > #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime" Thanks, Stefan
On 12/07/23 11:51, Stefan Liebler via Libc-alpha wrote: > Hi Adhemerval, > > I have applied your patches and tested it on s390x. I've ensured that > the wrapper is now used for pthread_create/posix_spawn and the testsuite > runs fine. Nevertheless I have some minor comments: see below. > > And one more question: > Is there a reason, why the wrapper is only added to s390x (64bit), but > not for s390 (31bit)? Should there be an #undef __ASSUME_CLONE3? > According to the kernel commit it is also available for compat-mode: > "s390: wire up clone3 system call" > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=5518aed82d2abd97f8d3ec91d8ba455d939e8cd1 Mainly because I could not test it properly: there is no easy access to s390x machine (such as on gcc build farm), there is no qemu-user support for 31-bit, and I saw strange results running s390 binaries on a s390x qemu vm. > > On 10.07.23 20:40, Adhemerval Zanella via Libc-alpha wrote: >> It follows the internal signature: >> >> extern int clone3 (struct clone_args *__cl_args, size_t __size, >> int (*__func) (void *__arg), void *__arg); >> >> Checked on s390x-linux-gnu. >> --- >> sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 +++++++++++++++++++ >> sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + >> 2 files changed, 82 insertions(+) >> create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >> >> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >> new file mode 100644 >> index 0000000000..417fdcb785 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >> @@ -0,0 +1,81 @@ >> +/* The clone3 syscall wrapper. Linux/s390x version. >> + Copyright (C) 2023 Free Software Foundation, Inc. >> + >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + <https://www.gnu.org/licenses/>. */ >> + >> +#include <sysdep.h> >> +#define _ERRNO_H 1 >> +#include <bits/errno.h> >> + >> +/* The userland implementation is: >> + int clone3 (struct clone_args *cl_args, size_t size, >> + int (*func)(void *arg), void *arg); >> + >> + the kernel entry is: >> + int clone3 (struct clone_args *cl_args, size_t size); >> + >> + The parameters are passed in registers from userland: >> + r2: cl_args >> + r3: size >> + r4: func >> + r5: arg */ >> + >> + .text >> +ENTRY(__clone3) >> + stg %r6, 48(%r15) > Add cfi_offset (%r6,-112) > > >> + >> + /* Sanity check args. */ >> + ltgr %r2, %r2 >> + je error >> + ltgr %r6, %r4 >> + je error >> + >> + /* Do the system call, the kernel expects: >> + r1: system call number >> + r2: cl_args >> + r3: size */ >> + lghi %r1, SYS_ify(clone3) >> + svc 0 >> + ltgr %r2,%r2 /* check return code */ >> + jz thread_start >> + lg %r6, 48(%r15) >> + jgm SYSCALL_ERROR_LABEL >> + br %r14 >> +error: >> + lghi %r2,-EINVAL >> + jg SYSCALL_ERROR_LABEL >> +PSEUDO_END (__clone3) >> + >> + .align 4 > The ENTRY-macro from sysdep.h is using an alignment of 16 bytes. Can you > please also use it here? > >> + .type thread_start, %function >> +thread_start: >> + cfi_startproc >> + /* Mark r14 as undefined in order to stop unwinding here. */ >> + cfi_undefined (r14) >> + >> + /* func is in gpr 6, arg in gpr 5. */ >> + lgr %r2, %r5 >> + aghi %r15, -160 >> + xc 0(8,%r15),0(%r15) >> + basr %r14, %r6 >> + >> + DO_CALL (exit, 1) >> + cfi_endproc >> + .size thread_start, .-thread_start > If you want, you can use ASM_SIZE_DIRECTIVE (thread_start) from sysdep.h > here, which is also used in END-macros. >> + >> +libc_hidden_def (__clone3) >> +weak_alias (__clone3, clone3) >> diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h >> index 3bde8ba19c..b099fea9c6 100644 >> --- a/sysdeps/unix/sysv/linux/s390/sysdep.h >> +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h >> @@ -72,6 +72,7 @@ >> #ifdef __s390x__ >> #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" >> #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" >> +#define HAVE_CLONE3_WRAPPER 1 >> #else >> #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" >> #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime" Updated patch below. From 74f79943014c4df13f2ce6d6ccd60b8b271319b3 Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Tue, 27 Sep 2022 17:00:27 -0300 Subject: [PATCH 1/9] s390x: Add the clone3 wrapper It follows the internal signature: extern int clone3 (struct clone_args *__cl_args, size_t __size, int (*__func) (void *__arg), void *__arg); Checked on s390x-linux-gnu. --- sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 82 +++++++++++++++++++ sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + 2 files changed, 83 insertions(+) create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S new file mode 100644 index 0000000000..22e07fa450 --- /dev/null +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S @@ -0,0 +1,82 @@ +/* The clone3 syscall wrapper. Linux/s390x version. + Copyright (C) 2023 Free Software Foundation, Inc. + + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <sysdep.h> +#define _ERRNO_H 1 +#include <bits/errno.h> + +/* The userland implementation is: + int clone3 (struct clone_args *cl_args, size_t size, + int (*func)(void *arg), void *arg); + + the kernel entry is: + int clone3 (struct clone_args *cl_args, size_t size); + + The parameters are passed in registers from userland: + r2: cl_args + r3: size + r4: func + r5: arg */ + + .text +ENTRY(__clone3) + stg %r6, 48(%r15) + cfi_offset (%r6, -112) + + /* Sanity check args. */ + ltgr %r2, %r2 + je error + ltgr %r6, %r4 + je error + + /* Do the system call, the kernel expects: + r1: system call number + r2: cl_args + r3: size */ + lghi %r1, SYS_ify(clone3) + svc 0 + ltgr %r2,%r2 /* check return code */ + jz thread_start + lg %r6, 48(%r15) + jgm SYSCALL_ERROR_LABEL + br %r14 +error: + lghi %r2,-EINVAL + jg SYSCALL_ERROR_LABEL +PSEUDO_END (__clone3) + + .align 16 + .type thread_start, %function +thread_start: + cfi_startproc + /* Mark r14 as undefined in order to stop unwinding here. */ + cfi_undefined (r14) + + /* func is in gpr 6, arg in gpr 5. */ + lgr %r2, %r5 + aghi %r15, -160 + xc 0(8,%r15),0(%r15) + basr %r14, %r6 + + DO_CALL (exit, 1) + cfi_endproc + ASM_SIZE_DIRECTIVE (thread_start) + +libc_hidden_def (__clone3) +weak_alias (__clone3, clone3) diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h index 3bde8ba19c..b099fea9c6 100644 --- a/sysdeps/unix/sysv/linux/s390/sysdep.h +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h @@ -72,6 +72,7 @@ #ifdef __s390x__ #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" +#define HAVE_CLONE3_WRAPPER 1 #else #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
Hi Adhemerval, I had a further look into the implementation. And we can get rid of save/restore of r6 to stack at all. Despite of r2 (return-value), the syscall does not clobber the registers. Thus we can just keep func in r4 and arg in r5 and use both directly in thread_start. Furthermore I've added a clone3.S in s390-32 directory and define HAVE_CLONE3_WRAPPER to 1 for s390x(64bit)/s390(31bit). The testsuite runs fine for both modes. Both changes are within the patch-file attached (I'm not quite sure if my mail-client corrupts an inlined patch). Bye, Stefan On 12.07.23 20:08, Adhemerval Zanella Netto wrote: > > > On 12/07/23 11:51, Stefan Liebler via Libc-alpha wrote: >> Hi Adhemerval, >> >> I have applied your patches and tested it on s390x. I've ensured that >> the wrapper is now used for pthread_create/posix_spawn and the testsuite >> runs fine. Nevertheless I have some minor comments: see below. >> >> And one more question: >> Is there a reason, why the wrapper is only added to s390x (64bit), but >> not for s390 (31bit)? Should there be an #undef __ASSUME_CLONE3? >> According to the kernel commit it is also available for compat-mode: >> "s390: wire up clone3 system call" >> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=5518aed82d2abd97f8d3ec91d8ba455d939e8cd1 > > Mainly because I could not test it properly: there is no easy access to > s390x machine (such as on gcc build farm), there is no qemu-user support > for 31-bit, and I saw strange results running s390 binaries on a s390x > qemu vm. > >> >> On 10.07.23 20:40, Adhemerval Zanella via Libc-alpha wrote: >>> It follows the internal signature: >>> >>> extern int clone3 (struct clone_args *__cl_args, size_t __size, >>> int (*__func) (void *__arg), void *__arg); >>> >>> Checked on s390x-linux-gnu. >>> --- >>> sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 +++++++++++++++++++ >>> sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + >>> 2 files changed, 82 insertions(+) >>> create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >>> >>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >>> new file mode 100644 >>> index 0000000000..417fdcb785 >>> --- /dev/null >>> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >>> @@ -0,0 +1,81 @@ >>> +/* The clone3 syscall wrapper. Linux/s390x version. >>> + Copyright (C) 2023 Free Software Foundation, Inc. >>> + >>> + This file is part of the GNU C Library. >>> + >>> + The GNU C Library is free software; you can redistribute it and/or >>> + modify it under the terms of the GNU Lesser General Public >>> + License as published by the Free Software Foundation; either >>> + version 2.1 of the License, or (at your option) any later version. >>> + >>> + The GNU C Library is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + Lesser General Public License for more details. >>> + >>> + You should have received a copy of the GNU Lesser General Public >>> + License along with the GNU C Library; if not, see >>> + <https://www.gnu.org/licenses/>. */ >>> + >>> +#include <sysdep.h> >>> +#define _ERRNO_H 1 >>> +#include <bits/errno.h> >>> + >>> +/* The userland implementation is: >>> + int clone3 (struct clone_args *cl_args, size_t size, >>> + int (*func)(void *arg), void *arg); >>> + >>> + the kernel entry is: >>> + int clone3 (struct clone_args *cl_args, size_t size); >>> + >>> + The parameters are passed in registers from userland: >>> + r2: cl_args >>> + r3: size >>> + r4: func >>> + r5: arg */ >>> + >>> + .text >>> +ENTRY(__clone3) >>> + stg %r6, 48(%r15) >> Add cfi_offset (%r6,-112) >> >> >>> + >>> + /* Sanity check args. */ >>> + ltgr %r2, %r2 >>> + je error >>> + ltgr %r6, %r4 >>> + je error >>> + >>> + /* Do the system call, the kernel expects: >>> + r1: system call number >>> + r2: cl_args >>> + r3: size */ >>> + lghi %r1, SYS_ify(clone3) >>> + svc 0 >>> + ltgr %r2,%r2 /* check return code */ >>> + jz thread_start >>> + lg %r6, 48(%r15) >>> + jgm SYSCALL_ERROR_LABEL >>> + br %r14 >>> +error: >>> + lghi %r2,-EINVAL >>> + jg SYSCALL_ERROR_LABEL >>> +PSEUDO_END (__clone3) >>> + >>> + .align 4 >> The ENTRY-macro from sysdep.h is using an alignment of 16 bytes. Can you >> please also use it here? >> >>> + .type thread_start, %function >>> +thread_start: >>> + cfi_startproc >>> + /* Mark r14 as undefined in order to stop unwinding here. */ >>> + cfi_undefined (r14) >>> + >>> + /* func is in gpr 6, arg in gpr 5. */ >>> + lgr %r2, %r5 >>> + aghi %r15, -160 >>> + xc 0(8,%r15),0(%r15) >>> + basr %r14, %r6 >>> + >>> + DO_CALL (exit, 1) >>> + cfi_endproc >>> + .size thread_start, .-thread_start >> If you want, you can use ASM_SIZE_DIRECTIVE (thread_start) from sysdep.h >> here, which is also used in END-macros. >>> + >>> +libc_hidden_def (__clone3) >>> +weak_alias (__clone3, clone3) >>> diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h >>> index 3bde8ba19c..b099fea9c6 100644 >>> --- a/sysdeps/unix/sysv/linux/s390/sysdep.h >>> +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h >>> @@ -72,6 +72,7 @@ >>> #ifdef __s390x__ >>> #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" >>> #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" >>> +#define HAVE_CLONE3_WRAPPER 1 >>> #else >>> #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" >>> #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime" > > Updated patch below. > > From 74f79943014c4df13f2ce6d6ccd60b8b271319b3 Mon Sep 17 00:00:00 2001 > From: Adhemerval Zanella <adhemerval.zanella@linaro.org> > Date: Tue, 27 Sep 2022 17:00:27 -0300 > Subject: [PATCH 1/9] s390x: Add the clone3 wrapper > > It follows the internal signature: > > extern int clone3 (struct clone_args *__cl_args, size_t __size, > int (*__func) (void *__arg), void *__arg); > > Checked on s390x-linux-gnu. > --- > sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 82 +++++++++++++++++++ > sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + > 2 files changed, 83 insertions(+) > create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S > > diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S > new file mode 100644 > index 0000000000..22e07fa450 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S > @@ -0,0 +1,82 @@ > +/* The clone3 syscall wrapper. Linux/s390x version. > + Copyright (C) 2023 Free Software Foundation, Inc. > + > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <sysdep.h> > +#define _ERRNO_H 1 > +#include <bits/errno.h> > + > +/* The userland implementation is: > + int clone3 (struct clone_args *cl_args, size_t size, > + int (*func)(void *arg), void *arg); > + > + the kernel entry is: > + int clone3 (struct clone_args *cl_args, size_t size); > + > + The parameters are passed in registers from userland: > + r2: cl_args > + r3: size > + r4: func > + r5: arg */ > + > + .text > +ENTRY(__clone3) > + stg %r6, 48(%r15) > + cfi_offset (%r6, -112) > + > + /* Sanity check args. */ > + ltgr %r2, %r2 > + je error > + ltgr %r6, %r4 > + je error > + > + /* Do the system call, the kernel expects: > + r1: system call number > + r2: cl_args > + r3: size */ > + lghi %r1, SYS_ify(clone3) > + svc 0 > + ltgr %r2,%r2 /* check return code */ > + jz thread_start > + lg %r6, 48(%r15) > + jgm SYSCALL_ERROR_LABEL > + br %r14 > +error: > + lghi %r2,-EINVAL > + jg SYSCALL_ERROR_LABEL > +PSEUDO_END (__clone3) > + > + .align 16 > + .type thread_start, %function > +thread_start: > + cfi_startproc > + /* Mark r14 as undefined in order to stop unwinding here. */ > + cfi_undefined (r14) > + > + /* func is in gpr 6, arg in gpr 5. */ > + lgr %r2, %r5 > + aghi %r15, -160 > + xc 0(8,%r15),0(%r15) > + basr %r14, %r6 > + > + DO_CALL (exit, 1) > + cfi_endproc > + ASM_SIZE_DIRECTIVE (thread_start) > + > +libc_hidden_def (__clone3) > +weak_alias (__clone3, clone3) > diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h > index 3bde8ba19c..b099fea9c6 100644 > --- a/sysdeps/unix/sysv/linux/s390/sysdep.h > +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h > @@ -72,6 +72,7 @@ > #ifdef __s390x__ > #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" > #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" > +#define HAVE_CLONE3_WRAPPER 1 > #else > #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" > #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
On 13/07/23 06:30, Stefan Liebler wrote: > Hi Adhemerval, > > I had a further look into the implementation. And we can get rid of > save/restore of r6 to stack at all. Despite of r2 (return-value), the > syscall does not clobber the registers. Thus we can just keep func in r4 > and arg in r5 and use both directly in thread_start. > > Furthermore I've added a clone3.S in s390-32 directory and define > HAVE_CLONE3_WRAPPER to 1 for s390x(64bit)/s390(31bit). > The testsuite runs fine for both modes. > > Both changes are within the patch-file attached (I'm not quite sure if > my mail-client corrupts an inlined patch). > > Bye, > Stefan LGTM, thanks for check the s390 version. I will install the patch. > > On 12.07.23 20:08, Adhemerval Zanella Netto wrote: >> >> >> On 12/07/23 11:51, Stefan Liebler via Libc-alpha wrote: >>> Hi Adhemerval, >>> >>> I have applied your patches and tested it on s390x. I've ensured that >>> the wrapper is now used for pthread_create/posix_spawn and the testsuite >>> runs fine. Nevertheless I have some minor comments: see below. >>> >>> And one more question: >>> Is there a reason, why the wrapper is only added to s390x (64bit), but >>> not for s390 (31bit)? Should there be an #undef __ASSUME_CLONE3? >>> According to the kernel commit it is also available for compat-mode: >>> "s390: wire up clone3 system call" >>> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=5518aed82d2abd97f8d3ec91d8ba455d939e8cd1 >> >> Mainly because I could not test it properly: there is no easy access to >> s390x machine (such as on gcc build farm), there is no qemu-user support >> for 31-bit, and I saw strange results running s390 binaries on a s390x >> qemu vm. >> >>> >>> On 10.07.23 20:40, Adhemerval Zanella via Libc-alpha wrote: >>>> It follows the internal signature: >>>> >>>> extern int clone3 (struct clone_args *__cl_args, size_t __size, >>>> int (*__func) (void *__arg), void *__arg); >>>> >>>> Checked on s390x-linux-gnu. >>>> --- >>>> sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 +++++++++++++++++++ >>>> sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + >>>> 2 files changed, 82 insertions(+) >>>> create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >>>> >>>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >>>> new file mode 100644 >>>> index 0000000000..417fdcb785 >>>> --- /dev/null >>>> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >>>> @@ -0,0 +1,81 @@ >>>> +/* The clone3 syscall wrapper. Linux/s390x version. >>>> + Copyright (C) 2023 Free Software Foundation, Inc. >>>> + >>>> + This file is part of the GNU C Library. >>>> + >>>> + The GNU C Library is free software; you can redistribute it and/or >>>> + modify it under the terms of the GNU Lesser General Public >>>> + License as published by the Free Software Foundation; either >>>> + version 2.1 of the License, or (at your option) any later version. >>>> + >>>> + The GNU C Library is distributed in the hope that it will be useful, >>>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>>> + Lesser General Public License for more details. >>>> + >>>> + You should have received a copy of the GNU Lesser General Public >>>> + License along with the GNU C Library; if not, see >>>> + <https://www.gnu.org/licenses/>. */ >>>> + >>>> +#include <sysdep.h> >>>> +#define _ERRNO_H 1 >>>> +#include <bits/errno.h> >>>> + >>>> +/* The userland implementation is: >>>> + int clone3 (struct clone_args *cl_args, size_t size, >>>> + int (*func)(void *arg), void *arg); >>>> + >>>> + the kernel entry is: >>>> + int clone3 (struct clone_args *cl_args, size_t size); >>>> + >>>> + The parameters are passed in registers from userland: >>>> + r2: cl_args >>>> + r3: size >>>> + r4: func >>>> + r5: arg */ >>>> + >>>> + .text >>>> +ENTRY(__clone3) >>>> + stg %r6, 48(%r15) >>> Add cfi_offset (%r6,-112) >>> >>> >>>> + >>>> + /* Sanity check args. */ >>>> + ltgr %r2, %r2 >>>> + je error >>>> + ltgr %r6, %r4 >>>> + je error >>>> + >>>> + /* Do the system call, the kernel expects: >>>> + r1: system call number >>>> + r2: cl_args >>>> + r3: size */ >>>> + lghi %r1, SYS_ify(clone3) >>>> + svc 0 >>>> + ltgr %r2,%r2 /* check return code */ >>>> + jz thread_start >>>> + lg %r6, 48(%r15) >>>> + jgm SYSCALL_ERROR_LABEL >>>> + br %r14 >>>> +error: >>>> + lghi %r2,-EINVAL >>>> + jg SYSCALL_ERROR_LABEL >>>> +PSEUDO_END (__clone3) >>>> + >>>> + .align 4 >>> The ENTRY-macro from sysdep.h is using an alignment of 16 bytes. Can you >>> please also use it here? >>> >>>> + .type thread_start, %function >>>> +thread_start: >>>> + cfi_startproc >>>> + /* Mark r14 as undefined in order to stop unwinding here. */ >>>> + cfi_undefined (r14) >>>> + >>>> + /* func is in gpr 6, arg in gpr 5. */ >>>> + lgr %r2, %r5 >>>> + aghi %r15, -160 >>>> + xc 0(8,%r15),0(%r15) >>>> + basr %r14, %r6 >>>> + >>>> + DO_CALL (exit, 1) >>>> + cfi_endproc >>>> + .size thread_start, .-thread_start >>> If you want, you can use ASM_SIZE_DIRECTIVE (thread_start) from sysdep.h >>> here, which is also used in END-macros. >>>> + >>>> +libc_hidden_def (__clone3) >>>> +weak_alias (__clone3, clone3) >>>> diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h >>>> index 3bde8ba19c..b099fea9c6 100644 >>>> --- a/sysdeps/unix/sysv/linux/s390/sysdep.h >>>> +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h >>>> @@ -72,6 +72,7 @@ >>>> #ifdef __s390x__ >>>> #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" >>>> #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" >>>> +#define HAVE_CLONE3_WRAPPER 1 >>>> #else >>>> #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" >>>> #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime" >> >> Updated patch below. >> >> From 74f79943014c4df13f2ce6d6ccd60b8b271319b3 Mon Sep 17 00:00:00 2001 >> From: Adhemerval Zanella <adhemerval.zanella@linaro.org> >> Date: Tue, 27 Sep 2022 17:00:27 -0300 >> Subject: [PATCH 1/9] s390x: Add the clone3 wrapper >> >> It follows the internal signature: >> >> extern int clone3 (struct clone_args *__cl_args, size_t __size, >> int (*__func) (void *__arg), void *__arg); >> >> Checked on s390x-linux-gnu. >> --- >> sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 82 +++++++++++++++++++ >> sysdeps/unix/sysv/linux/s390/sysdep.h | 1 + >> 2 files changed, 83 insertions(+) >> create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >> >> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >> new file mode 100644 >> index 0000000000..22e07fa450 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S >> @@ -0,0 +1,82 @@ >> +/* The clone3 syscall wrapper. Linux/s390x version. >> + Copyright (C) 2023 Free Software Foundation, Inc. >> + >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + <https://www.gnu.org/licenses/>. */ >> + >> +#include <sysdep.h> >> +#define _ERRNO_H 1 >> +#include <bits/errno.h> >> + >> +/* The userland implementation is: >> + int clone3 (struct clone_args *cl_args, size_t size, >> + int (*func)(void *arg), void *arg); >> + >> + the kernel entry is: >> + int clone3 (struct clone_args *cl_args, size_t size); >> + >> + The parameters are passed in registers from userland: >> + r2: cl_args >> + r3: size >> + r4: func >> + r5: arg */ >> + >> + .text >> +ENTRY(__clone3) >> + stg %r6, 48(%r15) >> + cfi_offset (%r6, -112) >> + >> + /* Sanity check args. */ >> + ltgr %r2, %r2 >> + je error >> + ltgr %r6, %r4 >> + je error >> + >> + /* Do the system call, the kernel expects: >> + r1: system call number >> + r2: cl_args >> + r3: size */ >> + lghi %r1, SYS_ify(clone3) >> + svc 0 >> + ltgr %r2,%r2 /* check return code */ >> + jz thread_start >> + lg %r6, 48(%r15) >> + jgm SYSCALL_ERROR_LABEL >> + br %r14 >> +error: >> + lghi %r2,-EINVAL >> + jg SYSCALL_ERROR_LABEL >> +PSEUDO_END (__clone3) >> + >> + .align 16 >> + .type thread_start, %function >> +thread_start: >> + cfi_startproc >> + /* Mark r14 as undefined in order to stop unwinding here. */ >> + cfi_undefined (r14) >> + >> + /* func is in gpr 6, arg in gpr 5. */ >> + lgr %r2, %r5 >> + aghi %r15, -160 >> + xc 0(8,%r15),0(%r15) >> + basr %r14, %r6 >> + >> + DO_CALL (exit, 1) >> + cfi_endproc >> + ASM_SIZE_DIRECTIVE (thread_start) >> + >> +libc_hidden_def (__clone3) >> +weak_alias (__clone3, clone3) >> diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h >> index 3bde8ba19c..b099fea9c6 100644 >> --- a/sysdeps/unix/sysv/linux/s390/sysdep.h >> +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h >> @@ -72,6 +72,7 @@ >> #ifdef __s390x__ >> #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" >> #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" >> +#define HAVE_CLONE3_WRAPPER 1 >> #else >> #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" >> #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S new file mode 100644 index 0000000000..417fdcb785 --- /dev/null +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S @@ -0,0 +1,81 @@ +/* The clone3 syscall wrapper. Linux/s390x version. + Copyright (C) 2023 Free Software Foundation, Inc. + + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <sysdep.h> +#define _ERRNO_H 1 +#include <bits/errno.h> + +/* The userland implementation is: + int clone3 (struct clone_args *cl_args, size_t size, + int (*func)(void *arg), void *arg); + + the kernel entry is: + int clone3 (struct clone_args *cl_args, size_t size); + + The parameters are passed in registers from userland: + r2: cl_args + r3: size + r4: func + r5: arg */ + + .text +ENTRY(__clone3) + stg %r6, 48(%r15) + + /* Sanity check args. */ + ltgr %r2, %r2 + je error + ltgr %r6, %r4 + je error + + /* Do the system call, the kernel expects: + r1: system call number + r2: cl_args + r3: size */ + lghi %r1, SYS_ify(clone3) + svc 0 + ltgr %r2,%r2 /* check return code */ + jz thread_start + lg %r6, 48(%r15) + jgm SYSCALL_ERROR_LABEL + br %r14 +error: + lghi %r2,-EINVAL + jg SYSCALL_ERROR_LABEL +PSEUDO_END (__clone3) + + .align 4 + .type thread_start, %function +thread_start: + cfi_startproc + /* Mark r14 as undefined in order to stop unwinding here. */ + cfi_undefined (r14) + + /* func is in gpr 6, arg in gpr 5. */ + lgr %r2, %r5 + aghi %r15, -160 + xc 0(8,%r15),0(%r15) + basr %r14, %r6 + + DO_CALL (exit, 1) + cfi_endproc + .size thread_start, .-thread_start + +libc_hidden_def (__clone3) +weak_alias (__clone3, clone3) diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h index 3bde8ba19c..b099fea9c6 100644 --- a/sysdeps/unix/sysv/linux/s390/sysdep.h +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h @@ -72,6 +72,7 @@ #ifdef __s390x__ #define HAVE_CLOCK_GETRES64_VSYSCALL "__kernel_clock_getres" #define HAVE_CLOCK_GETTIME64_VSYSCALL "__kernel_clock_gettime" +#define HAVE_CLONE3_WRAPPER 1 #else #define HAVE_CLOCK_GETRES_VSYSCALL "__kernel_clock_getres" #define HAVE_CLOCK_GETTIME_VSYSCALL "__kernel_clock_gettime"