[3/3] mips: remove register spill

Message ID 20201128081817.15463-4-huangpei@loongson.cn
State Superseded
Headers
Series [1/3] mips: add hp-timing support for MIPS R2 |

Commit Message

Huang Pei Nov. 28, 2020, 8:18 a.m. UTC
  Before Linux/MIPS 2.6.36, kernel expected setting syscall number(aka
"li v0, #sys_number") right precedes "syscall", so the kernel syscall
restart sequence can use CP0 EPC - 4 to restart the syscall, because
kernel DID NOT save v0 during syscall handling. Linux 2.6.36 canceled
this restriction.

See sysdeps/unix/sysv/linux/mips/{mips32/sysdep.h,mips64/sysdep.h,sysdep.h}

Since glibc-2.24 the minimum kernel version is 3.2(much higer than
2.6.36), I think it is OK to remove the ugly register spill in
syscall.S just because of the old convention

Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 sysdeps/unix/sysv/linux/mips/mips64/syscall.S | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
  

Comments

Adhemerval Zanella Nov. 30, 2020, 2:22 p.m. UTC | #1
On 28/11/2020 05:18, Huang Pei wrote:
> Before Linux/MIPS 2.6.36, kernel expected setting syscall number(aka
> "li v0, #sys_number") right precedes "syscall", so the kernel syscall
> restart sequence can use CP0 EPC - 4 to restart the syscall, because
> kernel DID NOT save v0 during syscall handling. Linux 2.6.36 canceled
> this restriction.
> 
> See sysdeps/unix/sysv/linux/mips/{mips32/sysdep.h,mips64/sysdep.h,sysdep.h}
> 
> Since glibc-2.24 the minimum kernel version is 3.2(much higer than
> 2.6.36), I think it is OK to remove the ugly register spill in
> syscall.S just because of the old convention
> 
> Signed-off-by: Huang Pei <huangpei@loongson.cn>

We do not use SCO, but rather Copyright assignment.	

The rest of the patch looks ok.

> ---
>  sysdeps/unix/sysv/linux/mips/mips64/syscall.S | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
> index aab1f389aa..089524a40b 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
> @@ -27,14 +27,9 @@
>  
>  	.text
>  NESTED (syscall, SZREG, ra)
> -	.mask 0x00010000, -2 * SZREG
> +	.mask 0x00000000, 0
>  	.fmask 0x00000000, 0
> -	PTR_ADDIU sp, -2 * SZREG
> -	cfi_adjust_cfa_offset (2 * SZREG)
> -	REG_S s0, (sp)
> -	cfi_rel_offset (s0, 0)
> -
> -	move s0, a0
> +	move v0, a0
>  	move a0, a1		/* shift arg1 - arg7.  */
>  	move a1, a2
>  	move a2, a3
> @@ -43,13 +38,8 @@ NESTED (syscall, SZREG, ra)
>  	move a5, a6
>  	move a6, a7
>  
> -	move v0, s0		/* Syscall number -> v0 */
>  	syscall			/* Do the system call.  */
>  
> -	REG_L s0, (sp)
> -	cfi_restore (s0)
> -	PTR_ADDIU sp, 2 * SZREG
> -	cfi_adjust_cfa_offset (-2 * 2 * SZREG)
>  	bne a3, zero, L(error)
>  
>  	ret
>
  
Huang Pei Dec. 1, 2020, 9:39 a.m. UTC | #2
hi,

On Mon, Nov 30, 2020 at 11:22:03AM -0300, Adhemerval Zanella wrote:
> 
> 
> On 28/11/2020 05:18, Huang Pei wrote:
> > Before Linux/MIPS 2.6.36, kernel expected setting syscall number(aka
> > "li v0, #sys_number") right precedes "syscall", so the kernel syscall
> > restart sequence can use CP0 EPC - 4 to restart the syscall, because
> > kernel DID NOT save v0 during syscall handling. Linux 2.6.36 canceled
> > this restriction.
> > 
> > See sysdeps/unix/sysv/linux/mips/{mips32/sysdep.h,mips64/sysdep.h,sysdep.h}
> > 
> > Since glibc-2.24 the minimum kernel version is 3.2(much higer than
> > 2.6.36), I think it is OK to remove the ugly register spill in
> > syscall.S just because of the old convention
> > 
> > Signed-off-by: Huang Pei <huangpei@loongson.cn>
> 
> We do not use SCO, but rather Copyright assignment.	
> 
> The rest of the patch looks ok.
> 

I would like to combine the Patch 2/3 and Patch 3/3 into one patch,
since Patch 2/3 only fix sp alignment, but the key point is folllowing syscall
restart convention, any advice?

> > ---
> >  sysdeps/unix/sysv/linux/mips/mips64/syscall.S | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
> > index aab1f389aa..089524a40b 100644
> > --- a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
> > +++ b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
> > @@ -27,14 +27,9 @@
> >  
> >  	.text
> >  NESTED (syscall, SZREG, ra)
> > -	.mask 0x00010000, -2 * SZREG
> > +	.mask 0x00000000, 0
> >  	.fmask 0x00000000, 0
> > -	PTR_ADDIU sp, -2 * SZREG
> > -	cfi_adjust_cfa_offset (2 * SZREG)
> > -	REG_S s0, (sp)
> > -	cfi_rel_offset (s0, 0)
> > -
> > -	move s0, a0
> > +	move v0, a0
> >  	move a0, a1		/* shift arg1 - arg7.  */
> >  	move a1, a2
> >  	move a2, a3
> > @@ -43,13 +38,8 @@ NESTED (syscall, SZREG, ra)
> >  	move a5, a6
> >  	move a6, a7
> >  
> > -	move v0, s0		/* Syscall number -> v0 */
> >  	syscall			/* Do the system call.  */
> >  
> > -	REG_L s0, (sp)
> > -	cfi_restore (s0)
> > -	PTR_ADDIU sp, 2 * SZREG
> > -	cfi_adjust_cfa_offset (-2 * 2 * SZREG)
> >  	bne a3, zero, L(error)
> >  
> >  	ret
> >
  
Adhemerval Zanella Dec. 1, 2020, 11:58 a.m. UTC | #3
On 01/12/2020 06:39, Huang Pei wrote:
> hi,
> 
> On Mon, Nov 30, 2020 at 11:22:03AM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 28/11/2020 05:18, Huang Pei wrote:
>>> Before Linux/MIPS 2.6.36, kernel expected setting syscall number(aka
>>> "li v0, #sys_number") right precedes "syscall", so the kernel syscall
>>> restart sequence can use CP0 EPC - 4 to restart the syscall, because
>>> kernel DID NOT save v0 during syscall handling. Linux 2.6.36 canceled
>>> this restriction.
>>>
>>> See sysdeps/unix/sysv/linux/mips/{mips32/sysdep.h,mips64/sysdep.h,sysdep.h}
>>>
>>> Since glibc-2.24 the minimum kernel version is 3.2(much higer than
>>> 2.6.36), I think it is OK to remove the ugly register spill in
>>> syscall.S just because of the old convention
>>>
>>> Signed-off-by: Huang Pei <huangpei@loongson.cn>
>>
>> We do not use SCO, but rather Copyright assignment.	
>>
>> The rest of the patch looks ok.
>>
> 
> I would like to combine the Patch 2/3 and Patch 3/3 into one patch,
> since Patch 2/3 only fix sp alignment, but the key point is folllowing syscall
> restart convention, any advice?

Send a v2 with the patches merged and the hp-inline fix.
  
Maciej W. Rozycki Dec. 4, 2020, 11:03 a.m. UTC | #4
On Tue, 1 Dec 2020, Adhemerval Zanella via Libc-alpha wrote:

> > I would like to combine the Patch 2/3 and Patch 3/3 into one patch,
> > since Patch 2/3 only fix sp alignment, but the key point is folllowing syscall
> > restart convention, any advice?
> 
> Send a v2 with the patches merged and the hp-inline fix.

 I would defer the removal of the old syscall restart convention in this 
single place only.  I don't think we need to introduce such a mess.

 Instead an audit should be made and all the places adjusted with a single 
change.  That would include at least MIPS16 wrappers, and IIRC some 
microMIPS support code, and then the comment I referred previously needs 
to be adjusted accordingly (maybe removed altogether even).

  Maciej
  
Adhemerval Zanella Dec. 4, 2020, 12:33 p.m. UTC | #5
On 04/12/2020 08:03, Maciej W. Rozycki wrote:
> On Tue, 1 Dec 2020, Adhemerval Zanella via Libc-alpha wrote:
> 
>>> I would like to combine the Patch 2/3 and Patch 3/3 into one patch,
>>> since Patch 2/3 only fix sp alignment, but the key point is folllowing syscall
>>> restart convention, any advice?
>>
>> Send a v2 with the patches merged and the hp-inline fix.
> 
>  I would defer the removal of the old syscall restart convention in this 
> single place only.  I don't think we need to introduce such a mess.
> 
>  Instead an audit should be made and all the places adjusted with a single 
> change.  That would include at least MIPS16 wrappers, and IIRC some 
> microMIPS support code, and then the comment I referred previously needs 
> to be adjusted accordingly (maybe removed altogether even).
> 
>   Maciej

Alright, it seems better indeed.
  

Patch

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
index aab1f389aa..089524a40b 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
+++ b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
@@ -27,14 +27,9 @@ 
 
 	.text
 NESTED (syscall, SZREG, ra)
-	.mask 0x00010000, -2 * SZREG
+	.mask 0x00000000, 0
 	.fmask 0x00000000, 0
-	PTR_ADDIU sp, -2 * SZREG
-	cfi_adjust_cfa_offset (2 * SZREG)
-	REG_S s0, (sp)
-	cfi_rel_offset (s0, 0)
-
-	move s0, a0
+	move v0, a0
 	move a0, a1		/* shift arg1 - arg7.  */
 	move a1, a2
 	move a2, a3
@@ -43,13 +38,8 @@  NESTED (syscall, SZREG, ra)
 	move a5, a6
 	move a6, a7
 
-	move v0, s0		/* Syscall number -> v0 */
 	syscall			/* Do the system call.  */
 
-	REG_L s0, (sp)
-	cfi_restore (s0)
-	PTR_ADDIU sp, 2 * SZREG
-	cfi_adjust_cfa_offset (-2 * 2 * SZREG)
 	bne a3, zero, L(error)
 
 	ret