[RESEND PATCH] microblaze: Fix clone syscall

[RESEND PATCH] microblaze: Fix clone syscall

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

Microblaze was assign to CLONE_BACKWARDS type where
parent tid was passed via 3rd argument.
Microblaze glibc is using 4th argument for it.

Create new CLONE_BACKWARDS3 type where stack_size is passed
via 3rd argument, parent thread id pointer via 4th,
child thread id pointer via 5th and tls value as 6th
argument

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Hi Al,

We have found this problem based on debugging timer_create()
reported by customer which wasn't cover by LTP testing.
What tool do you use for syscall testing?

IRC there was any discussion about adding syscall tests
directly to the kernel.

I am not sure if there is more elegant way how to fix this
in syscalls.h.

Thanks,
Michal

---
 arch/Kconfig             | 6 ++++++
 arch/microblaze/Kconfig  | 2 +-
 include/linux/syscalls.h | 5 +++++
 kernel/fork.c            | 6 ++++++
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 8d2ae24..1feb169 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -407,6 +407,12 @@ config CLONE_BACKWARDS2
 	help
 	  Architecture has the first two arguments of clone(2) swapped.

+config CLONE_BACKWARDS3
+	bool
+	help
+	  Architecture has tls passed as the 3rd argument of clone(2),
+	  not the 5th one.
+
 config ODD_RT_SIGACTION
 	bool
 	help
diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
index d22a4ec..4fab522 100644
--- a/arch/microblaze/Kconfig
+++ b/arch/microblaze/Kconfig
@@ -28,7 +28,7 @@ config MICROBLAZE
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_IDLE_POLL_SETUP
 	select MODULES_USE_ELF_RELA
-	select CLONE_BACKWARDS
+	select CLONE_BACKWARDS3

 config SWAP
 	def_bool n
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 4147d70..71d8931 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -802,9 +802,14 @@ asmlinkage long sys_vfork(void);
 asmlinkage long sys_clone(unsigned long, unsigned long, int __user *, int,
 	       int __user *);
 #else
+#if CONFIG_CLONE_BACKWARDS3
+asmlinkage long sys_clone(unsigned long, unsigned long, int, int __user *,
+			  int __user *, int);
+#else
 asmlinkage long sys_clone(unsigned long, unsigned long, int __user *,
 	       int __user *, int);
 #endif
+#endif

 asmlinkage long sys_execve(const char __user *filename,
 		const char __user *const __user *argv,
diff --git a/kernel/fork.c b/kernel/fork.c
index 66635c8..da6b699 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1679,6 +1679,12 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned long, clone_flags,
 		 int __user *, parent_tidptr,
 		 int __user *, child_tidptr,
 		 int, tls_val)
+#elif defined(CONFIG_CLONE_BACKWARDS3)
+SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
+		int, stack_size,
+		int __user *, parent_tidptr,
+		int __user *, child_tidptr,
+		int, tls_val)
 #else
 SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
 		 int __user *, parent_tidptr,
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Rich Felker]

On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
> Microblaze was assign to CLONE_BACKWARDS type where
> parent tid was passed via 3rd argument.
> Microblaze glibc is using 4th argument for it.
> 
> Create new CLONE_BACKWARDS3 type where stack_size is passed
> via 3rd argument, parent thread id pointer via 4th,
> child thread id pointer via 5th and tls value as 6th
> argument

I believe this also affects us in musl. What is the motivation for
making a configure option that results in there being two incompatible
syscall ABIs for the same arch? This sounds like a really bad idea...
And how was glibc successfuly using a form that mismatched the
existing kernel? Did nobody ever use/test it? I think the broken
userspace software that was already failing to work due to this
mismatch should simply be fixed rather than adding incompatible kernel
ABI variants.

Rich

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

Hi Rich,

On 07/24/2013 07:55 AM, Rich Felker wrote:
> On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
>> Microblaze was assign to CLONE_BACKWARDS type where
>> parent tid was passed via 3rd argument.
>> Microblaze glibc is using 4th argument for it.
>>
>> Create new CLONE_BACKWARDS3 type where stack_size is passed
>> via 3rd argument, parent thread id pointer via 4th,
>> child thread id pointer via 5th and tls value as 6th
>> argument
> 
> I believe this also affects us in musl. What is the motivation for
> making a configure option that results in there being two incompatible
> syscall ABIs for the same arch?
> This sounds like a really bad idea...

This patch fixes bug which was introduced by Al's patch where he moved
clone implementation from microblaze folder to generic location.
It means I am not creating two incompatible syscalls ABIs but fixing
broken one.

> And how was glibc successfuly using a form that mismatched the
> existing kernel? Did nobody ever use/test it?

We are running LTP syscall tests and there is not LTP test which
was able to find out this mismatch in clone. That's why I haven't
figure it out at that time and ACKed that origin patch.

In my email you can see that I have also asked about tools which should be used
for kernel API testing.

> I think the broken
> userspace software that was already failing to work due to this
> mismatch should simply be fixed rather than adding incompatible kernel
> ABI variants.

The incompatibility is between glibc register setup and the kernel sys_clone
register expectation which doesn't match right now.

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Rich Felker]

On Wed, Jul 24, 2013 at 08:48:27AM +0200, Michal Simek wrote:
> >> Create new CLONE_BACKWARDS3 type where stack_size is passed
> >> via 3rd argument, parent thread id pointer via 4th,
> >> child thread id pointer via 5th and tls value as 6th
> >> argument
> > 
> > I believe this also affects us in musl. What is the motivation for
> > making a configure option that results in there being two incompatible
> > syscall ABIs for the same arch?
> > This sounds like a really bad idea...
> 
> This patch fixes bug which was introduced by Al's patch where he moved
> clone implementation from microblaze folder to generic location.
> It means I am not creating two incompatible syscalls ABIs but fixing
> broken one.

So this patch is just fixing a regression in the kernel?

> > And how was glibc successfuly using a form that mismatched the
> > existing kernel? Did nobody ever use/test it?
> 
> We are running LTP syscall tests and there is not LTP test which
> was able to find out this mismatch in clone. That's why I haven't
> figure it out at that time and ACKed that origin patch.

I would think pthread_create would have broken pretty badly; I
remember early-on in porting musl to microblaze, we had the clone
arguments misordered, and it blew up badly. ;) Perhaps you could just
run some general libc/libpthread level tests to catch things like this
that are hard to measure at the syscall-test level with existing
tests.

Rich

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On 07/24/2013 08:59 AM, Rich Felker wrote:
> On Wed, Jul 24, 2013 at 08:48:27AM +0200, Michal Simek wrote:
>>>> Create new CLONE_BACKWARDS3 type where stack_size is passed
>>>> via 3rd argument, parent thread id pointer via 4th,
>>>> child thread id pointer via 5th and tls value as 6th
>>>> argument
>>>
>>> I believe this also affects us in musl. What is the motivation for
>>> making a configure option that results in there being two incompatible
>>> syscall ABIs for the same arch?
>>> This sounds like a really bad idea...
>>
>> This patch fixes bug which was introduced by Al's patch where he moved
>> clone implementation from microblaze folder to generic location.
>> It means I am not creating two incompatible syscalls ABIs but fixing
>> broken one.
> 
> So this patch is just fixing a regression in the kernel?

yes.


>>> And how was glibc successfuly using a form that mismatched the
>>> existing kernel? Did nobody ever use/test it?
>>
>> We are running LTP syscall tests and there is not LTP test which
>> was able to find out this mismatch in clone. That's why I haven't
>> figure it out at that time and ACKed that origin patch.
> 
> I would think pthread_create would have broken pretty badly; I
> remember early-on in porting musl to microblaze, we had the clone
> arguments misordered, and it blew up badly. ;) Perhaps you could just
> run some general libc/libpthread level tests to catch things like this
> that are hard to measure at the syscall-test level with existing
> tests.

David was running glibc tests and I was also running some pthreads tests
but we have seen the problem only on timer_create tests we have got from
customer.
I found that we should maybe invest our time to open posix testsuite
to get another set of tests we should run.

BTW: Where to get musl package and are there any tests we should regularly run?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Rich Felker]

On Wed, Jul 24, 2013 at 09:16:03AM +0200, Michal Simek wrote:
> BTW: Where to get musl package

Source is available from http://www.musl-libc.org/. We also have cross
compilers and binaries for a number of archs here:

  https://bitbucket.org/GregorR/musl-cross

However, microblaze was not included since the support in the upstream
GNU packages seemed to be broken/unusable. If this has changed, please
let me know. I'd really like to get microblaze support added added to
the cross-compiler builds.

> and are there any tests we should regularly run?

We have a libc-test package under development here:

  http://nsz.repo.hu/git/?p=libc-test

There's nothing musl-specific about the tests; they can also be run
against glibc, and presumably uClibc too. It's still a bit
experimental now, and still pulling in tests we had in various
non-unified test modules into a proper test framework, but it may be
useful.

Rich

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Andrew Morton]

On Wed, 24 Jul 2013 08:48:27 +0200 Michal Simek <monstr@monstr.eu> wrote:

> On 07/24/2013 07:55 AM, Rich Felker wrote:
> > On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
> >> Microblaze was assign to CLONE_BACKWARDS type where
> >> parent tid was passed via 3rd argument.
> >> Microblaze glibc is using 4th argument for it.
> >>
> >> Create new CLONE_BACKWARDS3 type where stack_size is passed
> >> via 3rd argument, parent thread id pointer via 4th,
> >> child thread id pointer via 5th and tls value as 6th
> >> argument
> > 
> > I believe this also affects us in musl. What is the motivation for
> > making a configure option that results in there being two incompatible
> > syscall ABIs for the same arch?
> > This sounds like a really bad idea...
> 
> This patch fixes bug which was introduced by Al's patch where he moved
> clone implementation from microblaze folder to generic location.

That's important information which was omitted from the changelog. 

Please identify the patch which casused this regression (SHA hash and
title), thanks.

Re: [microblaze-linux] [RESEND PATCH] microblaze: Fix clone syscall

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

Hi Andrew,

On 07/27/2013 01:08 AM, Andrew Morton wrote:
> On Wed, 24 Jul 2013 08:48:27 +0200 Michal Simek <monstr@monstr.eu> wrote:
> 
>> On 07/24/2013 07:55 AM, Rich Felker wrote:
>>> On Wed, Jul 24, 2013 at 07:34:07AM +0200, Michal Simek wrote:
>>>> Microblaze was assign to CLONE_BACKWARDS type where
>>>> parent tid was passed via 3rd argument.
>>>> Microblaze glibc is using 4th argument for it.
>>>>
>>>> Create new CLONE_BACKWARDS3 type where stack_size is passed
>>>> via 3rd argument, parent thread id pointer via 4th,
>>>> child thread id pointer via 5th and tls value as 6th
>>>> argument
>>>
>>> I believe this also affects us in musl. What is the motivation for
>>> making a configure option that results in there being two incompatible
>>> syscall ABIs for the same arch?
>>> This sounds like a really bad idea...
>>
>> This patch fixes bug which was introduced by Al's patch where he moved
>> clone implementation from microblaze folder to generic location.
> 
> That's important information which was omitted from the changelog. 
> 
> Please identify the patch which casused this regression (SHA hash and
> title), thanks.

Title is here and no problem to add it to the patch.
"microblaze: switch to generic fork/vfork/clone"
(sha1: f3268edbe6fe0ce56e62c6d6b14640aeb04864b7)
I will do v2 patch.
I see you have added v1 patch to your repo. What's the reason for that?
I expect just not to forget it, right?

Not sure if Al is on his vacation or not but will be helpful if you
can give me your ACK on that v2 patch and will send pull request directly
to Linus.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]