From: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
To: Stefan Liebler <stli@linux.ibm.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH v2 1/4] s390x: Add the clone3 wrapper
Date: Thu, 13 Jul 2023 08:52:04 -0300 [thread overview]
Message-ID: <4591cad6-e011-5ddf-6b48-f20edf336e15@linaro.org> (raw)
In-Reply-To: <074639e3-5e12-d580-b64f-7798fcf5db6b@linux.ibm.com>
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"
next prev parent reply other threads:[~2023-07-13 11:52 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-10 18:40 [PATCH v2 0/4] Add clone3 support for multiple architectures Adhemerval Zanella
2023-07-10 18:40 ` [PATCH v2 1/4] s390x: Add the clone3 wrapper Adhemerval Zanella
2023-07-12 14:51 ` Stefan Liebler
2023-07-12 18:08 ` Adhemerval Zanella Netto
2023-07-13 9:30 ` Stefan Liebler
2023-07-13 11:52 ` Adhemerval Zanella Netto [this message]
2023-07-10 18:40 ` [PATCH v2 2/4] arm: " Adhemerval Zanella
2023-07-10 18:40 ` [PATCH v2 3/4] mips: " Adhemerval Zanella
2023-07-10 18:40 ` [PATCH v2 4/4] linux: Undef __ASSUME_CLONE3 for alpha, ia64, nios2, sh, and sparc Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4591cad6-e011-5ddf-6b48-f20edf336e15@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=stli@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).