diff mbox series

[v2,1/4] s390x: Add the clone3 wrapper

Message ID 20230710184022.3701346-2-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Add clone3 support for multiple architectures |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 warning Patch failed to apply

Commit Message

Adhemerval Zanella Netto July 10, 2023, 6:40 p.m. UTC 
  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

Comments

Stefan Liebler July 12, 2023, 2:51 p.m. UTC | #1 
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
Adhemerval Zanella Netto July 12, 2023, 6:08 p.m. UTC | #2 
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"
Stefan Liebler July 13, 2023, 9:30 a.m. UTC | #3 
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"
Adhemerval Zanella Netto July 13, 2023, 11:52 a.m. UTC | #4 
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 mbox series

Patch

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"