[-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Sachin Sant]

Today's next 20091117 build failed on s390 with

arch/s390/kernel/compat_wrapper.S: Assembler messages:
arch/s390/kernel/compat_wrapper.S:1871: Error: operand out of range (164 is not between 0 and 15)
arch/s390/kernel/compat_wrapper.S:1871: Error: junk at end of line: `(%r15)'
make[1]: *** [arch/s390/kernel/compat_wrapper.o] Error 1

The code in question was added by commit 9db3031ac785b068eb4636465eebb7b346c48dbb

fanotify: sys_fanotify_mark declartion

I had applied the following patch to get past another build break reported
against yesterday's next (20091116)

http://marc.info/?l=linux-next&m=125844478708886&w=2 <http://marc.info/?l=linux-next&m=125844478708886&w=2>

Thanks
-Sachin

-- 

---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Tue, Nov 17, 2009 at 05:36:19PM +0530, Sachin Sant wrote:
> Today's next 20091117 build failed on s390 with
> 
> arch/s390/kernel/compat_wrapper.S: Assembler messages:
> arch/s390/kernel/compat_wrapper.S:1871: Error: operand out of range (164 is not between 0 and 15)
> arch/s390/kernel/compat_wrapper.S:1871: Error: junk at end of line: `(%r15)'
> make[1]: *** [arch/s390/kernel/compat_wrapper.o] Error 1
> 
> The code in question was added by commit 9db3031ac785b068eb4636465eebb7b346c48dbb
> 
> fanotify: sys_fanotify_mark declartion

Hmm, the compat wrapper for sys_fanotify_mark is more broken (besides the
fact that it doesn't compile).
The same is true for sys_fanotify_init.

I thought it was general consensus that new syscalls should be wired up only
on x86 and let arch maintainers wire them up on their architectures.
Besides that a simple C source needs to be delivered so it can be easily
tested on each architecture.

Especially we want to avoid subtle sign extension bugs like we have them here.
Probably gone unnoticed if there wouldn't be a compile bug.

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Eric Paris]

On Tue, 2009-11-17 at 13:52 +0100, Heiko Carstens wrote:
> On Tue, Nov 17, 2009 at 05:36:19PM +0530, Sachin Sant wrote:
> > Today's next 20091117 build failed on s390 with
> > 
> > arch/s390/kernel/compat_wrapper.S: Assembler messages:
> > arch/s390/kernel/compat_wrapper.S:1871: Error: operand out of range (164 is not between 0 and 15)
> > arch/s390/kernel/compat_wrapper.S:1871: Error: junk at end of line: `(%r15)'
> > make[1]: *** [arch/s390/kernel/compat_wrapper.o] Error 1
> > 
> > The code in question was added by commit 9db3031ac785b068eb4636465eebb7b346c48dbb
> > 
> > fanotify: sys_fanotify_mark declartion
> 
> Hmm, the compat wrapper for sys_fanotify_mark is more broken (besides the
> fact that it doesn't compile).
> The same is true for sys_fanotify_init.
> 
> I thought it was general consensus that new syscalls should be wired up only
> on x86 and let arch maintainers wire them up on their architectures.
> Besides that a simple C source needs to be delivered so it can be easily
> tested on each architecture.
> 
> Especially we want to avoid subtle sign extension bugs like we have them here.
> Probably gone unnoticed if there wouldn't be a compile bug.

I'll un-wire tonight or fix it if you tell me how.  I guess my code was
supposed to use l instead of or as in sys_fallocate_wrapper() instead of
copying sys32_lookup_dcookie_wrapper().

Simple C source is available, but without s390 syscall definitions at
people.redhat.com/~eparis/fanotify

touch /tmp/tmp
./fanotify /tmp/tmp &
^Z
echo hello > /tmp/tmp
fg
^C

-Eric

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Tue, Nov 17, 2009 at 08:40:41AM -0500, Eric Paris wrote:
> On Tue, 2009-11-17 at 13:52 +0100, Heiko Carstens wrote:
> > On Tue, Nov 17, 2009 at 05:36:19PM +0530, Sachin Sant wrote:
> > > Today's next 20091117 build failed on s390 with
> > > 
> > > arch/s390/kernel/compat_wrapper.S: Assembler messages:
> > > arch/s390/kernel/compat_wrapper.S:1871: Error: operand out of range (164 is not between 0 and 15)
> > > arch/s390/kernel/compat_wrapper.S:1871: Error: junk at end of line: `(%r15)'
> > > make[1]: *** [arch/s390/kernel/compat_wrapper.o] Error 1
> > > 
> > > The code in question was added by commit 9db3031ac785b068eb4636465eebb7b346c48dbb
> > > 
> > > fanotify: sys_fanotify_mark declartion
> > 
> > Hmm, the compat wrapper for sys_fanotify_mark is more broken (besides the
> > fact that it doesn't compile).
> > The same is true for sys_fanotify_init.
> > 
> > I thought it was general consensus that new syscalls should be wired up only
> > on x86 and let arch maintainers wire them up on their architectures.
> > Besides that a simple C source needs to be delivered so it can be easily
> > tested on each architecture.
> > 
> > Especially we want to avoid subtle sign extension bugs like we have them here.
> > Probably gone unnoticed if there wouldn't be a compile bug.
> 
> I'll un-wire tonight or fix it if you tell me how.  I guess my code was
> supposed to use l instead of or as in sys_fallocate_wrapper() instead of
> copying sys32_lookup_dcookie_wrapper().

Yes, also some places should have used lgfr instead of llgfr for proper sign
extension. But please, just drop the s390 bits from your patch.
Its easier and less painful for us to do it ourselves instead of reviewing
and fixing these things. (No offence intended!).

> Simple C source is available, but without s390 syscall definitions at
> people.redhat.com/~eparis/fanotify
> 
> touch /tmp/tmp
> ./fanotify /tmp/tmp &
> ^Z
> echo hello > /tmp/tmp
> fg
> ^C

Ok, thanks!

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Eric Paris]

On Tue, 2009-11-17 at 14:55 +0100, Heiko Carstens wrote:

> Yes, also some places should have used lgfr instead of llgfr for proper sign
> extension. But please, just drop the s390 bits from your patch.
> Its easier and less painful for us to do it ourselves instead of reviewing
> and fixing these things. (No offence intended!).

dropped and won't show up in -next tomorrow.

This what I thought it should be and would love to read if you say it's
right....

diff --git a/arch/s390/include/asm/unistd.h b/arch/s390/include/asm/unistd.h
index cb5232d..8962161 100644
--- a/arch/s390/include/asm/unistd.h
+++ b/arch/s390/include/asm/unistd.h
@@ -269,7 +269,9 @@
 #define	__NR_pwritev		329
 #define __NR_rt_tgsigqueueinfo	330
 #define __NR_perf_event_open	331
-#define NR_syscalls 332
+#define __NR_fanotify_init	332
+#define __NR_fanotify_mark	333
+#define NR_syscalls 334
 
 /* 
  * There are some system calls that are not present on 64 bit, some
diff --git a/arch/s390/kernel/compat_wrapper.S b/arch/s390/kernel/compat_wrapper.S
index cbd9901..c81723d 100644
--- a/arch/s390/kernel/compat_wrapper.S
+++ b/arch/s390/kernel/compat_wrapper.S
@@ -1855,3 +1855,20 @@ sys32_execve_wrapper:
 	llgtr	%r3,%r3			# compat_uptr_t *
 	llgtr	%r4,%r4			# compat_uptr_t *
 	jg	sys32_execve		# branch to system call
+
+	.globl	sys_fanotify_init_wrapper
+sys_fanotify_init_wrapper:
+	llgfr	%r2,%r2			# unsigned int
+	llgfr	%r3,%r3			# unsigned int
+	llgfr	%r4,%r4			# unsigned int
+	jg	sys_fanotify_init	# branch to system call
+
+	.globl	sys32_fanotify_mark_wrapper
+sys32_fanotify_mark_wrapper:
+	lgfr	%r2,%r2			# int
+	llgfr	%r3,%r3			# unsigned int
+	lgfr	%r4,%r4			# int
+	llgtr	%r5,%r5			# char *
+	sllg	%r6,%r6,32		# get high word of 64bit mask
+	l	%r6,164(%r15)		# get low word of 64bit mask
+	jg	sys_fanotify_mark
diff --git a/arch/s390/kernel/syscalls.S b/arch/s390/kernel/syscalls.S
index 30eca07..cc781f1 100644
--- a/arch/s390/kernel/syscalls.S
+++ b/arch/s390/kernel/syscalls.S
@@ -340,3 +340,5 @@ SYSCALL(sys_preadv,sys_preadv,compat_sys_preadv_wrapper)
 SYSCALL(sys_pwritev,sys_pwritev,compat_sys_pwritev_wrapper)
 SYSCALL(sys_rt_tgsigqueueinfo,sys_rt_tgsigqueueinfo,compat_sys_rt_tgsigqueueinfo_wrapper) /* 330 */
 SYSCALL(sys_perf_event_open,sys_perf_event_open,sys_perf_event_open_wrapper)
+SYSCALL(sys_fanotify_init,sys_fanotify_init,sys_fanotify_init_wrapper)
+SYSCALL(sys_fanotify_mark,sys_fanotify_mark,sys32_fanotify_mark_wrapper)

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Tue, Nov 17, 2009 at 10:23:56AM -0500, Eric Paris wrote:
> On Tue, 2009-11-17 at 14:55 +0100, Heiko Carstens wrote:
> 
> > Yes, also some places should have used lgfr instead of llgfr for proper sign
> > extension. But please, just drop the s390 bits from your patch.
> > Its easier and less painful for us to do it ourselves instead of reviewing
> > and fixing these things. (No offence intended!).
> 
> dropped and won't show up in -next tomorrow.
> 
> This what I thought it should be and would love to read if you say it's
> right....

Yes, it is except for one thing:

> diff --git a/arch/s390/include/asm/unistd.h b/arch/s390/include/asm/unistd.h
> index cb5232d..8962161 100644
> --- a/arch/s390/include/asm/unistd.h
> +++ b/arch/s390/include/asm/unistd.h
> @@ -269,7 +269,9 @@
>  #define	__NR_pwritev		329
>  #define __NR_rt_tgsigqueueinfo	330
>  #define __NR_perf_event_open	331
> -#define NR_syscalls 332
> +#define __NR_fanotify_init	332
> +#define __NR_fanotify_mark	333
> +#define NR_syscalls 334
> 
>  /* 
>   * There are some system calls that are not present on 64 bit, some
> diff --git a/arch/s390/kernel/compat_wrapper.S b/arch/s390/kernel/compat_wrapper.S
> index cbd9901..c81723d 100644
> --- a/arch/s390/kernel/compat_wrapper.S
> +++ b/arch/s390/kernel/compat_wrapper.S
> @@ -1855,3 +1855,20 @@ sys32_execve_wrapper:
>  	llgtr	%r3,%r3			# compat_uptr_t *
>  	llgtr	%r4,%r4			# compat_uptr_t *
>  	jg	sys32_execve		# branch to system call
> +
> +	.globl	sys_fanotify_init_wrapper
> +sys_fanotify_init_wrapper:
> +	llgfr	%r2,%r2			# unsigned int
> +	llgfr	%r3,%r3			# unsigned int
> +	llgfr	%r4,%r4			# unsigned int
> +	jg	sys_fanotify_init	# branch to system call

The third argument an int and hence it should have been lgfr ;)

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Eric Paris]

On Tue, 2009-11-17 at 16:50 +0100, Heiko Carstens wrote:
> On Tue, Nov 17, 2009 at 10:23:56AM -0500, Eric Paris wrote:
> > On Tue, 2009-11-17 at 14:55 +0100, Heiko Carstens wrote:
> > 
> > > Yes, also some places should have used lgfr instead of llgfr for proper sign
> > > extension. But please, just drop the s390 bits from your patch.
> > > Its easier and less painful for us to do it ourselves instead of reviewing
> > > and fixing these things. (No offence intended!).
> > 
> > dropped and won't show up in -next tomorrow.
> > 
> > This what I thought it should be and would love to read if you say it's
> > right....
> 
> Yes, it is except for one thing:
> 
> > diff --git a/arch/s390/include/asm/unistd.h b/arch/s390/include/asm/unistd.h
> > index cb5232d..8962161 100644
> > --- a/arch/s390/include/asm/unistd.h
> > +++ b/arch/s390/include/asm/unistd.h
> > @@ -269,7 +269,9 @@
> >  #define	__NR_pwritev		329
> >  #define __NR_rt_tgsigqueueinfo	330
> >  #define __NR_perf_event_open	331
> > -#define NR_syscalls 332
> > +#define __NR_fanotify_init	332
> > +#define __NR_fanotify_mark	333
> > +#define NR_syscalls 334
> > 
> >  /* 
> >   * There are some system calls that are not present on 64 bit, some
> > diff --git a/arch/s390/kernel/compat_wrapper.S b/arch/s390/kernel/compat_wrapper.S
> > index cbd9901..c81723d 100644
> > --- a/arch/s390/kernel/compat_wrapper.S
> > +++ b/arch/s390/kernel/compat_wrapper.S
> > @@ -1855,3 +1855,20 @@ sys32_execve_wrapper:
> >  	llgtr	%r3,%r3			# compat_uptr_t *
> >  	llgtr	%r4,%r4			# compat_uptr_t *
> >  	jg	sys32_execve		# branch to system call
> > +
> > +	.globl	sys_fanotify_init_wrapper
> > +sys_fanotify_init_wrapper:
> > +	llgfr	%r2,%r2			# unsigned int
> > +	llgfr	%r3,%r3			# unsigned int
> > +	llgfr	%r4,%r4			# unsigned int
> > +	jg	sys_fanotify_init	# branch to system call
> 
> The third argument an int and hence it should have been lgfr ;)

It should have been an unsigned int, just noticed it today (because of
this discussion) and fixed it.  You'll see in a patch which does so in
-next tomorrow.

I'll let this settle a bit and come back to linux-s390 in a week or two
to ask you guys to write/review some hooks!

-Eric

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Tue, Nov 17, 2009 at 10:57:45AM -0500, Eric Paris wrote:
> On Tue, 2009-11-17 at 16:50 +0100, Heiko Carstens wrote:
> > On Tue, Nov 17, 2009 at 10:23:56AM -0500, Eric Paris wrote:
> > > On Tue, 2009-11-17 at 14:55 +0100, Heiko Carstens wrote:
> > The third argument an int and hence it should have been lgfr ;)
> 
> It should have been an unsigned int, just noticed it today (because of
> this discussion) and fixed it.  You'll see in a patch which does so in
> -next tomorrow.
> 
> I'll let this settle a bit and come back to linux-s390 in a week or two
> to ask you guys to write/review some hooks!

We will be triggered automatically as soon as your patch gets merged.
The build system emits a warning for each new syscall which isn't wired up
and present for x86.

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Tue, Nov 17, 2009 at 10:23:56AM -0500, Eric Paris wrote:
> On Tue, 2009-11-17 at 14:55 +0100, Heiko Carstens wrote:
> 
> > Yes, also some places should have used lgfr instead of llgfr for proper sign
> > extension. But please, just drop the s390 bits from your patch.
> > Its easier and less painful for us to do it ourselves instead of reviewing
> > and fixing these things. (No offence intended!).
> 
> dropped and won't show up in -next tomorrow.
> 
> This what I thought it should be and would love to read if you say it's
> right....
> 

[...]

> +sys32_fanotify_mark_wrapper:
> +	lgfr	%r2,%r2			# int
> +	llgfr	%r3,%r3			# unsigned int
> +	lgfr	%r4,%r4			# int
> +	llgtr	%r5,%r5			# char *
> +	sllg	%r6,%r6,32		# get high word of 64bit mask
> +	l	%r6,164(%r15)		# get low word of 64bit mask
> +	jg	sys_fanotify_mark

Oh wait, I have to correct myself:

With

long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
     	 	       int fd, const char  __user *pathname,
                       u64 mask);

we have a 64 bit type as 5th argument. That doesn't work for syscalls
on 32 bit s390.
I just simplify the reason for this: on 32 bit long longs will be passed via
two consecutive registers _unless_ the first register would be r6 (which is
the case here). In that case the whole 64 bits would be passed on the stack.
Our glibc syscall code will always put the contents of the first parameter
stack slot into register r7, so we have six registers for parameter passing
(r2-r7). So with the 64 bit value put into two stack slots we would miss
the second part of the 5th argument.

Please note that other architectures (I think at least arm and powerpc) put
64 bit values into even/odd register pairs and add padding if the first free
available register is an odd one. So any of the following interfaces should
work for all architectures:

long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
     	 	       int fd, const char  __user *pathname,
                       u32 mask_high, u32 mask_low);

long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
                       u64 mask,
     	 	       int fd, const char  __user *pathname);

long sys_fanotify_mark(u64 mask,
     		       int fanotify_fd, unsigned int flags,
     	 	       int fd, const char  __user *pathname);

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Russell King]

On Wed, Nov 18, 2009 at 08:04:18AM +0100, Heiko Carstens wrote:
> On Tue, Nov 17, 2009 at 10:23:56AM -0500, Eric Paris wrote:
> With
> 
> long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>      	 	       int fd, const char  __user *pathname,
>                        u64 mask);
> 
> we have a 64 bit type as 5th argument. That doesn't work for syscalls
> on 32 bit s390.

Note that the above works on ARM, since we end up with the following
register allocation:

r0: fanotify_fd
r1: flags
r2: fd
r3: pathname
r4,r5: mask

since 'mask' is an even,odd pair, and the arguments all fit within r0 - r5
inclusive.

> Please note that other architectures (I think at least arm and powerpc) put
> 64 bit values into even/odd register pairs and add padding if the first free
> available register is an odd one. So any of the following interfaces should
> work for all architectures:

Indeed.

Since we're going around the loop of re-organizing the argument order of
syscalls, the question which needs asking is: what's happening with HPA's
idea of having a set of per-arch rules and generating the interfaces
according to those rules?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Ralf Baechle]

On Wed, Nov 18, 2009 at 08:04:18AM +0100, Heiko Carstens wrote:

> Please note that other architectures (I think at least arm and powerpc) put
> 64 bit values into even/odd register pairs and add padding if the first free
> available register is an odd one. So any of the following interfaces should
> work for all architectures:
> 
> long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>      	 	       int fd, const char  __user *pathname,
>                        u32 mask_high, u32 mask_low);
> 
> long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>                        u64 mask,
>      	 	       int fd, const char  __user *pathname);
> 
> long sys_fanotify_mark(u64 mask,
>      		       int fanotify_fd, unsigned int flags,
>      	 	       int fd, const char  __user *pathname);

Correct - but the splitting is unnecessary pain for some platforms like
64-bit userland on 64-bit MIPS where the 64-bit argument would be passed
in a single register so I have preference for the 2nd or 3rd suggestion.

The 1st prototype has the advantage of avoiding the need for a compat
wrapper which otherwise would look like:

long compat_sys_fanotify_mark(int fanotify_fd, unsigned int flags,
                       u32 a2, u32 a3,
     	 	       int fd, const char  __user *pathname);
{
	/* assuming 2nd suggested prototype from above */
	return sys_fanotify_mark(fd, flags,
				 merge64(a2, a3),
				 fd, pathname);
}

I'd prefer to see the compat code carrying the burden.

  Ralf

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Eric Paris]

On Wed, 2009-11-18 at 08:04 +0100, Heiko Carstens wrote:
> On Tue, Nov 17, 2009 at 10:23:56AM -0500, Eric Paris wrote:
> > On Tue, 2009-11-17 at 14:55 +0100, Heiko Carstens wrote:
> > 
> > > Yes, also some places should have used lgfr instead of llgfr for proper sign
> > > extension. But please, just drop the s390 bits from your patch.
> > > Its easier and less painful for us to do it ourselves instead of reviewing
> > > and fixing these things. (No offence intended!).
> > 
> > dropped and won't show up in -next tomorrow.
> > 
> > This what I thought it should be and would love to read if you say it's
> > right....
> > 
> 
> [...]
> 
> > +sys32_fanotify_mark_wrapper:
> > +	lgfr	%r2,%r2			# int
> > +	llgfr	%r3,%r3			# unsigned int
> > +	lgfr	%r4,%r4			# int
> > +	llgtr	%r5,%r5			# char *
> > +	sllg	%r6,%r6,32		# get high word of 64bit mask
> > +	l	%r6,164(%r15)		# get low word of 64bit mask
> > +	jg	sys_fanotify_mark
> 
> Oh wait, I have to correct myself:
> 
> With
> 
> long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>      	 	       int fd, const char  __user *pathname,
>                        u64 mask);
> 
> we have a 64 bit type as 5th argument. That doesn't work for syscalls
> on 32 bit s390.
> I just simplify the reason for this: on 32 bit long longs will be passed via
> two consecutive registers _unless_ the first register would be r6 (which is
> the case here). In that case the whole 64 bits would be passed on the stack.
> Our glibc syscall code will always put the contents of the first parameter
> stack slot into register r7, so we have six registers for parameter passing
> (r2-r7). So with the 64 bit value put into two stack slots we would miss
> the second part of the 5th argument.

asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);

sys_fallocate_wrapper:
        lgfr    %r2,%r2                 # int
        lgfr    %r3,%r3                 # int
        sllg    %r4,%r4,32              # get high word of 64bit loff_t
        lr      %r4,%r5                 # get low word of 64bit loff_t
        sllg    %r5,%r6,32              # get high word of 64bit loff_t
        l       %r5,164(%r15)           # get low word of 64bit loff_t
        jg      sys_fallocate

Does this work?  It's basically the same thing, right?  I'm willing to
hear "that's fine you are clueless"   Just saw it and hoping that we
have everything right....

-Eric

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Wed, Nov 18, 2009 at 11:02:57AM -0500, Eric Paris wrote:
> 
> asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);
> 
> sys_fallocate_wrapper:
>         lgfr    %r2,%r2                 # int
>         lgfr    %r3,%r3                 # int
>         sllg    %r4,%r4,32              # get high word of 64bit loff_t
>         lr      %r4,%r5                 # get low word of 64bit loff_t
>         sllg    %r5,%r6,32              # get high word of 64bit loff_t
>         l       %r5,164(%r15)           # get low word of 64bit loff_t
>         jg      sys_fallocate
> 
> Does this work?  It's basically the same thing, right?  I'm willing to
> hear "that's fine you are clueless"   Just saw it and hoping that we
> have everything right....

It works, because for 32 bit s390 it's not the interface above but this one:
(see arch/s390/kernel/sys_s390.c):

/*
 * This is a wrapper to call sys_fallocate(). For 31 bit s390 the last
 * 64 bit argument "len" is split into the upper and lower 32 bits. The
 * system call wrapper in the user space loads the value to %r6/%r7.
 * The code in entry.S keeps the values in %r2 - %r6 where they are and
 * stores %r7 to 96(%r15). But the standard C linkage requires that
 * the whole 64 bit value for len is stored on the stack and doesn't
 * use %r6 at all. So s390_fallocate has to convert the arguments from
 *   %r2: fd, %r3: mode, %r4/%r5: offset, %r6/96(%r15)-99(%r15): len
 * to
 *   %r2: fd, %r3: mode, %r4/%r5: offset, 96(%r15)-103(%r15): len
 */
SYSCALL_DEFINE(s390_fallocate)(int fd, int mode, loff_t offset,
			       u32 len_high, u32 len_low)
{
	return sys_fallocate(fd, mode, offset, ((u64)len_high << 32) | len_low);
}
#ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
asmlinkage long SyS_s390_fallocate(long fd, long mode, loff_t offset,
				   long len_high, long len_low)
{
	return SYSC_s390_fallocate((int) fd, (int) mode, offset,
				   (u32) len_high, (u32) len_low);
}
SYSCALL_ALIAS(sys_s390_fallocate, SyS_s390_fallocate);
#endif

We (or better: Martin ;) had to define a special s390 fallocate system call
because the "real" system call interface doesn't work on s390 for the same
reason like your proposed system call.

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Martin Schwidefsky]

On Wed, 18 Nov 2009 11:02:57 -0500
Eric Paris <eparis@redhat.com> wrote:

> On Wed, 2009-11-18 at 08:04 +0100, Heiko Carstens wrote:
> > Oh wait, I have to correct myself:
> > 
> > With
> > 
> > long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
> >      	 	       int fd, const char  __user *pathname,
> >                        u64 mask);
> > 
> > we have a 64 bit type as 5th argument. That doesn't work for syscalls
> > on 32 bit s390.
> > I just simplify the reason for this: on 32 bit long longs will be passed via
> > two consecutive registers _unless_ the first register would be r6 (which is
> > the case here). In that case the whole 64 bits would be passed on the stack.
> > Our glibc syscall code will always put the contents of the first parameter
> > stack slot into register r7, so we have six registers for parameter passing
> > (r2-r7). So with the 64 bit value put into two stack slots we would miss
> > the second part of the 5th argument.
> 
> asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);
> 
> sys_fallocate_wrapper:
>         lgfr    %r2,%r2                 # int
>         lgfr    %r3,%r3                 # int
>         sllg    %r4,%r4,32              # get high word of 64bit loff_t
>         lr      %r4,%r5                 # get low word of 64bit loff_t
>         sllg    %r5,%r6,32              # get high word of 64bit loff_t
>         l       %r5,164(%r15)           # get low word of 64bit loff_t
>         jg      sys_fallocate
> 
> Does this work?  It's basically the same thing, right?  I'm willing to
> hear "that's fine you are clueless"   Just saw it and hoping that we
> have everything right....

Ok, we need the full version of the story..
The 32 bit ELF ABI specifies that the 32 bit registers %r2 to %r6 are
used for parameter passing. 64 bit values are passed as registers pairs
with the first register an even numbered register. The effect of that
rule is that parameter registers may be skipped or that the whole 64 bit
value is passed on the stack. Examples:

fn(int a, int b, long long c)
a is passed in %r2, b is passed in %r3, c is passed in %r4/%r5.

fn(int a, long long b, int c)
a is passed in %r2, b is passed in %r4/%r5, c is passed in %r6, %r3 is
skipped.

fn(int a, int b, int c, int d, long long e)
a is passed in %r2, b is passed in %r3, c is passed in %r4, d is passed
in %r5, e is passed on the stack, %r6 is skipped.

The second fact to understand is how the system call arguments are
passed. The original system call ABI used the same calling conventions
as the ELF ABI. That is only registers %r2 to %r6 are used. Now futex
came along with 6 parameters. We did not want to use the user process
stack to pass the parameters because that would require a
copy_from_user which is expensive. Instead we tricked a little bit. The
6th parameter is passed by glue code in glibc in register %r7 (no user
copy). The code in entry.S stores %r7 to the beginning of the pt_regs
structure:

struct pt_regs
{
        unsigned long args[1];
	...
};

The C function that implements a system call with 6 32-bit parameters
expects 5 parameters in registers, the 6th is located on the stack. The
args element of pt_regs "happens" to be at the same offset where the C
function is looking for the first overflow argument (= the 6th
parameter).

Now consider a system call with an overflowing 64 bit parameter. The
glue code in glibc could be hacked in a way that the 64 bit value is
split into %r6 and %r7. But the system call function is just a C
function. It follows the ELF ABI and expects the 64 bit argument on the
stack. It would take two 32 bit overflow registers in pt_regs to make
one 64 bit parameter. With the current code that won't work. We would
need a wrapper function in the kernel to untangle this parameter mess.

The avoid all this all 64 bit parameter have to be placed at positions
where no register is skipped because of the even/odd rule and where it
is not affected by the %r7 trick (= may not be the last parameter).
Easy, no?

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Heiko Carstens]

On Wed, Nov 18, 2009 at 06:34:00PM +0100, Martin Schwidefsky wrote:
> used for parameter passing. 64 bit values are passed as registers pairs
> with the first register an even numbered register. The effect of that
> rule is that parameter registers may be skipped or that the whole 64 bit
> value is passed on the stack. Examples:

Erm, did I miss something? The 32 bit ABI doesn't say anything about
even/odd register pairs for 64 bit values. Also gcc doesn't generate
such code:

> fn(int a, long long b, int c)
> a is passed in %r2, b is passed in %r4/%r5, c is passed in %r6, %r3 is
> skipped.

extern void fn(int a, long long b, int c);

void bla(void)
{
	fn(0, 1, 2);
}

00000000 <bla>:
   0:   90 ef f0 38             stm     %r14,%r15,56(%r15)
   4:   a7 fa ff a0             ahi     %r15,-96
   8:   a7 28 00 00             lhi     %r2,0
   c:   a7 38 00 00             lhi     %r3,0
  10:   a7 48 00 01             lhi     %r4,1
  14:   a7 58 00 02             lhi     %r5,2
  18:   c0 e5 00 00 00 00       brasl   %r14,18 <bla+0x18>
  1e:   98 ef f0 98             lm      %r14,%r15,152(%r15)
  22:   07 fe                   br      %r14

Not that it really matters, since other archs enforce the rule anyway.

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Martin Schwidefsky]

On Wed, 18 Nov 2009 19:41:56 +0100
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Wed, Nov 18, 2009 at 06:34:00PM +0100, Martin Schwidefsky wrote:
> > used for parameter passing. 64 bit values are passed as registers pairs
> > with the first register an even numbered register. The effect of that
> > rule is that parameter registers may be skipped or that the whole 64 bit
> > value is passed on the stack. Examples:
> 
> Erm, did I miss something? The 32 bit ABI doesn't say anything about
> even/odd register pairs for 64 bit values. Also gcc doesn't generate
> such code:

Odd, I can remember that there has been a rule like that. It is indeed
not in the ABI, seems like we dropped it/never added it. There are quite
a few 32 bit instruction that operate on 64 bit values in even/odd
registers which makes it beneficial to pass 64 bit values in a way that
they already are located in even/odd and not in odd/even registers. 

> > fn(int a, long long b, int c)
> > a is passed in %r2, b is passed in %r4/%r5, c is passed in %r6, %r3 is
> > skipped.
> 
> extern void fn(int a, long long b, int c);
> 
> void bla(void)
> {
> 	fn(0, 1, 2);
> }
> 
> 00000000 <bla>:
>    0:   90 ef f0 38             stm     %r14,%r15,56(%r15)
>    4:   a7 fa ff a0             ahi     %r15,-96
>    8:   a7 28 00 00             lhi     %r2,0
>    c:   a7 38 00 00             lhi     %r3,0
>   10:   a7 48 00 01             lhi     %r4,1
>   14:   a7 58 00 02             lhi     %r5,2
>   18:   c0 e5 00 00 00 00       brasl   %r14,18 <bla+0x18>
>   1e:   98 ef f0 98             lm      %r14,%r15,152(%r15)
>   22:   07 fe                   br      %r14
> 
> Not that it really matters, since other archs enforce the rule anyway.

Perhaps that is what confused me. Anyway, the thing to remember here is
that passing a long long as the last parameter which would end up in
%r6/%r7 requires a system call wrapper for the standard 32 bit system
call for a 32 bit kernel. So please don't do that.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [-next Nov 17] s390 build break(arch/s390/kernel/compat_wrapper.S)

[Eric Paris]

On Wed, 2009-11-18 at 08:04 +0100, Heiko Carstens wrote:


> long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>                        u64 mask,
>      	 	       int fd, const char  __user *pathname);

I'm willing to go with this interface if that works the best for most
arches.  I'll queue it up for -next tomorrow.

-Eric