2019-07-22 13:35:40

by Michael Ellerman

[permalink] [raw]
Subject: [PATCH] powerpc: Wire up clone3 syscall

Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork:
add clone3").

This requires a ppc_clone3 wrapper, in order to save the non-volatile
GPRs before calling into the generic syscall code. Otherwise we hit
the BUG_ON in CHECK_FULL_REGS in copy_thread().

Lightly tested using Christian's test code on a Power8 LE VM.

Signed-off-by: Michael Ellerman <[email protected]>
---
arch/powerpc/include/asm/unistd.h | 1 +
arch/powerpc/kernel/entry_64.S | 5 +++++
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
3 files changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index 68473c3c471c..b0720c7c3fcf 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -49,6 +49,7 @@
#define __ARCH_WANT_SYS_FORK
#define __ARCH_WANT_SYS_VFORK
#define __ARCH_WANT_SYS_CLONE
+#define __ARCH_WANT_SYS_CLONE3

#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_UNISTD_H_ */
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index d9105fcf4021..0a0b5310f54a 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -487,6 +487,11 @@ _GLOBAL(ppc_clone)
bl sys_clone
b .Lsyscall_exit

+_GLOBAL(ppc_clone3)
+ bl save_nvgprs
+ bl sys_clone3
+ b .Lsyscall_exit
+
_GLOBAL(ppc32_swapcontext)
bl save_nvgprs
bl compat_sys_swapcontext
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index f2c3bda2d39f..6886ecb590d5 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -516,3 +516,4 @@
432 common fsmount sys_fsmount
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
+435 common clone3 ppc_clone3
--
2.20.1


2019-07-22 13:37:45

by Christian Brauner

[permalink] [raw]
Subject: Re: [PATCH] powerpc: Wire up clone3 syscall

On Mon, Jul 22, 2019 at 11:22:31PM +1000, Michael Ellerman wrote:
> Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork:
> add clone3").
>
> This requires a ppc_clone3 wrapper, in order to save the non-volatile
> GPRs before calling into the generic syscall code. Otherwise we hit
> the BUG_ON in CHECK_FULL_REGS in copy_thread().
>
> Lightly tested using Christian's test code on a Power8 LE VM.
>
> Signed-off-by: Michael Ellerman <[email protected]>

Thank you, Michael!

One comment below, otherwise:

Acked-by: Christian Brauner <[email protected]>

> ---
> arch/powerpc/include/asm/unistd.h | 1 +
> arch/powerpc/kernel/entry_64.S | 5 +++++
> arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
> index 68473c3c471c..b0720c7c3fcf 100644
> --- a/arch/powerpc/include/asm/unistd.h
> +++ b/arch/powerpc/include/asm/unistd.h
> @@ -49,6 +49,7 @@
> #define __ARCH_WANT_SYS_FORK
> #define __ARCH_WANT_SYS_VFORK
> #define __ARCH_WANT_SYS_CLONE
> +#define __ARCH_WANT_SYS_CLONE3
>
> #endif /* __ASSEMBLY__ */
> #endif /* _ASM_POWERPC_UNISTD_H_ */
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index d9105fcf4021..0a0b5310f54a 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -487,6 +487,11 @@ _GLOBAL(ppc_clone)
> bl sys_clone
> b .Lsyscall_exit
>
> +_GLOBAL(ppc_clone3)
> + bl save_nvgprs
> + bl sys_clone3
> + b .Lsyscall_exit
> +
> _GLOBAL(ppc32_swapcontext)
> bl save_nvgprs
> bl compat_sys_swapcontext
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index f2c3bda2d39f..6886ecb590d5 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -516,3 +516,4 @@
> 432 common fsmount sys_fsmount
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> +435 common clone3 ppc_clone3

Note that in v5.3-rc1 there's now a comment that 435 is reserved in
there. So this will likely cause a merge conflict. You might want to
base your change off of v5.3-rc1 instead to avoid that. :)

So basically:

From 10b2e4176d712e45c7cb22af4aed4ce09818785c Mon Sep 17 00:00:00 2001
From: Michael Ellerman <[email protected]>
Date: Mon, 22 Jul 2019 23:22:31 +1000
Subject: [PATCH] powerpc: Wire up clone3 syscall

Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork:
add clone3").

This requires a ppc_clone3 wrapper, in order to save the non-volatile
GPRs before calling into the generic syscall code. Otherwise we hit
the BUG_ON in CHECK_FULL_REGS in copy_thread().

Lightly tested using Christian's test code on a Power8 LE VM.

Signed-off-by: Michael Ellerman <[email protected]>
Acked-by: Christian Brauner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/powerpc/include/asm/unistd.h | 1 +
arch/powerpc/kernel/entry_64.S | 5 +++++
arch/powerpc/kernel/syscalls/syscall.tbl | 2 +-
3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index 68473c3c471c..b0720c7c3fcf 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -49,6 +49,7 @@
#define __ARCH_WANT_SYS_FORK
#define __ARCH_WANT_SYS_VFORK
#define __ARCH_WANT_SYS_CLONE
+#define __ARCH_WANT_SYS_CLONE3

#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_UNISTD_H_ */
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index d9105fcf4021..0a0b5310f54a 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -487,6 +487,11 @@ _GLOBAL(ppc_clone)
bl sys_clone
b .Lsyscall_exit

+_GLOBAL(ppc_clone3)
+ bl save_nvgprs
+ bl sys_clone3
+ b .Lsyscall_exit
+
_GLOBAL(ppc32_swapcontext)
bl save_nvgprs
bl compat_sys_swapcontext
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 3331749aab20..6886ecb590d5 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -516,4 +516,4 @@
432 common fsmount sys_fsmount
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
-# 435 reserved for clone3
+435 common clone3 ppc_clone3
--
2.22.0

2019-07-24 05:26:44

by Arseny Solokha

[permalink] [raw]
Subject: Re: [PATCH] powerpc: Wire up clone3 syscall

Hi,

may I also ask to provide ppc_clone3 symbol also for 32-bit powerpc? Otherwise
Michael's patch breaks build for me:

powerpc-e500v2-linux-gnuspe-ld: arch/powerpc/kernel/systbl.o: in function `sys_call_table':
(.rodata+0x6cc): undefined reference to `ppc_clone3'
make: *** [Makefile:1060: vmlinux] Error 1

The patch was tested using Christian's program on a real e500 machine.

--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -597,6 +597,14 @@ ppc_clone:
stw r0,_TRAP(r1) /* register set saved */
b sys_clone

+ .globl ppc_clone3
+ppc_clone3:
+ SAVE_NVGPRS(r1)
+ lwz r0,_TRAP(r1)
+ rlwinm r0,r0,0,0,30 /* clear LSB to indicate full */
+ stw r0,_TRAP(r1) /* register set saved */
+ b sys_clone3
+
.globl ppc_swapcontext
ppc_swapcontext:
SAVE_NVGPRS(r1)

I don't think this trivial hunk deserves a separate patch submission.

Thanks,
Arseny

2019-07-24 12:16:30

by Christian Brauner

[permalink] [raw]
Subject: Re: [PATCH] powerpc: Wire up clone3 syscall

On Wed, Jul 24, 2019 at 12:25:14PM +0700, Arseny Solokha wrote:
> Hi,
>
> may I also ask to provide ppc_clone3 symbol also for 32-bit powerpc? Otherwise
> Michael's patch breaks build for me:

Makes sense. Michael, are you planning on picking this up? :)

Christian

>
> powerpc-e500v2-linux-gnuspe-ld: arch/powerpc/kernel/systbl.o: in function `sys_call_table':
> (.rodata+0x6cc): undefined reference to `ppc_clone3'
> make: *** [Makefile:1060: vmlinux] Error 1
>
> The patch was tested using Christian's program on a real e500 machine.
>
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -597,6 +597,14 @@ ppc_clone:
> stw r0,_TRAP(r1) /* register set saved */
> b sys_clone
>
> + .globl ppc_clone3
> +ppc_clone3:
> + SAVE_NVGPRS(r1)
> + lwz r0,_TRAP(r1)
> + rlwinm r0,r0,0,0,30 /* clear LSB to indicate full */
> + stw r0,_TRAP(r1) /* register set saved */
> + b sys_clone3
> +
> .globl ppc_swapcontext
> ppc_swapcontext:
> SAVE_NVGPRS(r1)
>
> I don't think this trivial hunk deserves a separate patch submission.
>
> Thanks,
> Arseny

2019-07-24 14:11:48

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH] powerpc: Wire up clone3 syscall

Christian Brauner <[email protected]> writes:
> On Wed, Jul 24, 2019 at 12:25:14PM +0700, Arseny Solokha wrote:
>> Hi,
>>
>> may I also ask to provide ppc_clone3 symbol also for 32-bit powerpc? Otherwise
>> Michael's patch breaks build for me:
>
> Makes sense. Michael, are you planning on picking this up? :)

Yes I obviously (I hope) am not going to merge a patch that breaks the
32-bit build :)

I'll send a v2.

cheers

2019-07-24 14:14:09

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH] powerpc: Wire up clone3 syscall

Christian Brauner <[email protected]> writes:
> On Mon, Jul 22, 2019 at 11:22:31PM +1000, Michael Ellerman wrote:
>> Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork:
>> add clone3").
>>
>> This requires a ppc_clone3 wrapper, in order to save the non-volatile
>> GPRs before calling into the generic syscall code. Otherwise we hit
>> the BUG_ON in CHECK_FULL_REGS in copy_thread().
>>
>> Lightly tested using Christian's test code on a Power8 LE VM.
>>
>> Signed-off-by: Michael Ellerman <[email protected]>
>
> Thank you, Michael!
>
> One comment below, otherwise:
>
> Acked-by: Christian Brauner <[email protected]>

Thanks.

...
>> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
>> index f2c3bda2d39f..6886ecb590d5 100644
>> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
>> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
>> @@ -516,3 +516,4 @@
>> 432 common fsmount sys_fsmount
>> 433 common fspick sys_fspick
>> 434 common pidfd_open sys_pidfd_open
>> +435 common clone3 ppc_clone3
>
> Note that in v5.3-rc1 there's now a comment that 435 is reserved in
> there. So this will likely cause a merge conflict. You might want to
> base your change off of v5.3-rc1 instead to avoid that. :)

Thanks for the heads-up.

My fixes branch is already based off pre-rc1, and in general Linus can
handle a trivial merge conflict like that.

But given I had to send a v2 to fix the 32-bit build (doh!), I'll move
my fixes up past rc1 once Linus has merged what's in there now, and then
do this patch based on top of rc1, so there'll be no conflict.

cheers