[PATCH] S390: Remove not needed stack frame in syscall function.

[PATCH] S390: Remove not needed stack frame in syscall function.

[Stefan Liebler]

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

Hi,

As an svc invocation does not clobber any user space registers
despite of the return value r2 and it does not need a special
stack frame. This patch gets rid of the extra frame.
We just have to save and restore r6 and r7 as those are
preserved across function calls.

Bye,
Stefan

[-- Attachment #2: 20191017_s390_syscall.patch --]
[-- Type: text/x-patch, Size: 3759 bytes --]

commit 1349ca6793627f62d0fe96d5e0891e2e0166ba90
Author: Stefan Liebler <stli@linux.ibm.com>
Date:   Thu Oct 17 13:49:03 2019 +0200

    S390: Remove not needed stack frame in syscall function.
    
    As an svc invocation does not clobber any user space registers
    despite of the return value r2 and it does not need a special
    stack frame. This patch gets rid of the extra frame.
    We just have to save and restore r6 and r7 as those are
    preserved across function calls.

diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscall.S b/sysdeps/unix/sysv/linux/s390/s390-32/syscall.S
index 69561d0b76..7ed36c0714 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/syscall.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscall.S
@@ -22,37 +22,19 @@
    more information about the value -4095 used below.*/
 
 ENTRY (syscall)
-	/* Save registers and setup stack.  */
-	stm     %r6,%r15,24(%r15)  /* save registers */
-	cfi_offset (%r15, -36)
-	cfi_offset (%r14, -40)
-	cfi_offset (%r13, -44)
-	cfi_offset (%r12, -48)
-	cfi_offset (%r11, -52)
-	cfi_offset (%r10, -56)
-	cfi_offset (%r9, -60)
-	cfi_offset (%r8, -64)
+	stm     %r6,%r7,24(%r15)  /* save registers */
 	cfi_offset (%r7, -68)
 	cfi_offset (%r6, -72)
-	lr      %r1,%r15
-	l       %r0,4(0,%r15)      /* load eos */
-	ahi     %r15,-96           /* buy stack space */
-	cfi_adjust_cfa_offset (96)
-	st      %r1,0(0,%r15)      /* store back chain */
-	st      %r0,4(0,%r15)      /* store eos */
 
 	lr     %r1,%r2             /* move syscall number */
 	lr     %r2,%r3             /* first parameter  */
 	lr     %r3,%r4             /* second parameter */
 	lr     %r4,%r5             /* third parameter  */
 	lr     %r5,%r6             /* fourth parameter */
-	l      %r6,192(%r15)       /* fifth parameter  */
-	l      %r7,196(%r15)       /* sixth parameter  */
-
+	lm     %r6,%r7,96(%r15)    /* fifth / sixth parameter  */
 	svc    0
-	l      %r15,0(%r15)        /* load back chain.  */
-	cfi_adjust_cfa_offset (-96)
-	lm     %r6,%r15,24(%r15)   /* load registers.  */
+
+	lm     %r6,%r7,24(%r15)    /* load registers.  */
 
 	lhi    %r0,-4095
 	clr    %r2,%r0             /* check R2 for error */
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/syscall.S b/sysdeps/unix/sysv/linux/s390/s390-64/syscall.S
index bbe4d79848..442f9e4878 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/syscall.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/syscall.S
@@ -22,37 +22,19 @@
    more information about the value -4095 used below.*/
 
 ENTRY (syscall)
-	/* Save registers and setup stack.  */
-	stmg	%r6,%r15,48(%r15)  /* Save registers.  */
-	cfi_offset (%r15,-40)
-	cfi_offset (%r14,-48)
-	cfi_offset (%r13,-56)
-	cfi_offset (%r12,-64)
-	cfi_offset (%r11,-72)
-	cfi_offset (%r10,-80)
-	cfi_offset (%r9,-88)
-	cfi_offset (%r8,-96)
+	stmg	%r6,%r7,48(%r15)  /* Save registers.  */
 	cfi_offset (%r7,-104)
 	cfi_offset (%r6,-112)
-	lgr	%r1,%r15
-	lg	%r0,8(%r15)	   /* Load eos.	 */
-	aghi	%r15,-160	   /* Buy stack space.	*/
-	cfi_adjust_cfa_offset (160)
-	stg	%r1,0(%r15)	   /* Store back chain.	 */
-	stg	%r0,8(%r15)	   /* Store eos.  */
 
 	lgr    %r1,%r2		   /* Move syscall number.  */
 	lgr    %r2,%r3		   /* First parameter.	*/
 	lgr    %r3,%r4		   /* Second parameter.	 */
 	lgr    %r4,%r5		   /* Third parameter.	*/
 	lgr    %r5,%r6		   /* Fourth parameter.	 */
-	lg     %r6,320(%r15)	   /* Fifth parameter.	*/
-	lg     %r7,328(%r15)	   /* Sixth parameter.	*/
-
+	lmg    %r6,%r7,160(%r15)   /* Fifth / Sixth parameter.	*/
 	svc    0
-	lg     %r15,0(%r15)        /* Load back chain.  */
-	cfi_adjust_cfa_offset (-160)
-	lmg    %r6,%r15,48(%r15)   /* Load registers.  */
+
+	lmg    %r6,%r7,48(%r15)    /* Load registers.  */
 
 	lghi   %r0,-4095
 	clgr   %r2,%r0		   /* Check R2 for error.  */

Re: [PATCH] S390: Remove not needed stack frame in syscall function.

[Florian Weimer]

* Stefan Liebler:

> As an svc invocation does not clobber any user space registers
> despite of the return value r2 and it does not need a special
> stack frame. This patch gets rid of the extra frame.
> We just have to save and restore r6 and r7 as those are
> preserved across function calls.

Looks okay to me.  Would it be possible to save r6 and r7 in
caller-saved registers not clobbered by the system call?  That might
provide another small benefit.

Thanks,
Florian

Re: [PATCH] S390: Remove not needed stack frame in syscall function.

[Stefan Liebler]

On 10/22/19 12:25 PM, Florian Weimer wrote:
> * Stefan Liebler:
> 
>> As an svc invocation does not clobber any user space registers
>> despite of the return value r2 and it does not need a special
>> stack frame. This patch gets rid of the extra frame.
>> We just have to save and restore r6 and r7 as those are
>> preserved across function calls.
> 
> Looks okay to me.  Would it be possible to save r6 and r7 in
> caller-saved registers not clobbered by the system call?  That might
> provide another small benefit.
> 
> Thanks,
> Florian
> 
The syscall itself just clobbers the return value in r2. But for its 
parameters we have to clobber r6 and r7.

According to the ABI, r0-r5 and r14 are volatile.
We need r1 for the syscall number for "svc 0", r2-r7 as parameters for 
svc and r14 is the return-address.

Thus we could use r0 for saving/restoring r6.
For r7 we have the option to either use the register save area on the 
stack-frame prepared by the caller or one of the volatile fprs. But the 
instructions for transferring gpr <-> fpr are not available with all 
architecture level sets. Thus we would need something like #ifdef / 
#else to provide alternative implementations.

Therefore I think just storing/restoring both registers at once with one 
stmg/lmg instruction is okay.

Thanks,
Stefan

Re: [PATCH] S390: Remove not needed stack frame in syscall function.

[Florian Weimer]

* Stefan Liebler:

> The syscall itself just clobbers the return value in r2. But for its
> parameters we have to clobber r6 and r7.
>
> According to the ABI, r0-r5 and r14 are volatile.
> We need r1 for the syscall number for "svc 0", r2-r7 as parameters for
> svc and r14 is the return-address.
>
> Thus we could use r0 for saving/restoring r6.
> For r7 we have the option to either use the register save area on the
> stack-frame prepared by the caller or one of the volatile fprs. But
> the instructions for transferring gpr <-> fpr are not available with
> all architecture level sets. Thus we would need something like #ifdef
> / #else to provide alternative implementations.
>
> Therefore I think just storing/restoring both registers at once with
> one stmg/lmg instruction is okay.

I see.  I didn't know the ABI details.

Thanks,
Florian

Re: [PATCH] S390: Remove not needed stack frame in syscall function.

[Stefan Liebler]

On 10/22/19 1:44 PM, Florian Weimer wrote:
> * Stefan Liebler:
> 
>> The syscall itself just clobbers the return value in r2. But for its
>> parameters we have to clobber r6 and r7.
>>
>> According to the ABI, r0-r5 and r14 are volatile.
>> We need r1 for the syscall number for "svc 0", r2-r7 as parameters for
>> svc and r14 is the return-address.
>>
>> Thus we could use r0 for saving/restoring r6.
>> For r7 we have the option to either use the register save area on the
>> stack-frame prepared by the caller or one of the volatile fprs. But
>> the instructions for transferring gpr <-> fpr are not available with
>> all architecture level sets. Thus we would need something like #ifdef
>> / #else to provide alternative implementations.
>>
>> Therefore I think just storing/restoring both registers at once with
>> one stmg/lmg instruction is okay.
> 
> I see.  I didn't know the ABI details.
> 
> Thanks,
> Florian
> 

Committed.
Thanks