[v2] powerpc64[le]: Allocate extra stack frame on syscall.S

  v1 was not working properly on hosts without scv support, so some tests were
failing in such systems. This is fixed now in this new version.

--8<--

The syscall function does not allocate the extra stack frame for scv like other
assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
offset that is used to save LR, syscall ended up using an invalid offset,
causing regressions on powerpc64. So make sure the extra stack frame is
allocated in syscall.S as well to make it consistent with other uses of
DO_CALL_SCV and avoid similar issues in the future.

Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
---
 sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
 1 file changed, 4 insertions(+)
  

On 12/15/21 8:39 AM, Matheus Castanho wrote:
> v1 was not working properly on hosts without scv support, so some tests were
> failing in such systems. This is fixed now in this new version.
> 
> --8<--
> 
> The syscall function does not allocate the extra stack frame for scv like other
> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
> offset that is used to save LR, syscall ended up using an invalid offset,
> causing regressions on powerpc64. So make sure the extra stack frame is
> allocated in syscall.S as well to make it consistent with other uses of
> DO_CALL_SCV and avoid similar issues in the future.
> 
> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
> ---
>   sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> index a29652feaf..a5497c8370 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> @@ -27,7 +27,11 @@ ENTRY (syscall)
>   	mr   r8,r9
>   #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>   	CHECK_SCV_SUPPORT r9 0f
> +	stdu r1,-SCV_FRAME_SIZE(r1)
> +	cfi_adjust_cfa_offset(SCV_FRAME_SIZE)

I think this fixes the issue, but it seems like a workaround of a 
deficiency in the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter 
with current frame size? It would avoid the need to push a dummy frame here.

>   	DO_CALL_SCV > +	addi r1,r1,SCV_FRAME_SIZE
> +	cfi_adjust_cfa_offset(-SCV_FRAME_SIZE)
>   	RET_SCV
>   	b 1f
>   #endif
>
  

Paul E Murphy <murphyp@linux.ibm.com> writes:

> On 12/15/21 8:39 AM, Matheus Castanho wrote:
>> v1 was not working properly on hosts without scv support, so some tests were
>> failing in such systems. This is fixed now in this new version.
>> --8<--
>> The syscall function does not allocate the extra stack frame for scv like
>> other
>> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
>> offset that is used to save LR, syscall ended up using an invalid offset,
>> causing regressions on powerpc64. So make sure the extra stack frame is
>> allocated in syscall.S as well to make it consistent with other uses of
>> DO_CALL_SCV and avoid similar issues in the future.
>> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
>> ---
>>   sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>>   1 file changed, 4 insertions(+)
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> index a29652feaf..a5497c8370 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> @@ -27,7 +27,11 @@ ENTRY (syscall)
>>   	mr   r8,r9
>>   #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>>   	CHECK_SCV_SUPPORT r9 0f
>> +	stdu r1,-SCV_FRAME_SIZE(r1)
>> +	cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>
> I think this fixes the issue, but it seems like a workaround of a deficiency in
> the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter with current frame
> size? It would avoid the need to push a dummy frame here.
>

That is an option. But I opted to allocate the dummy frame to make it
consistent other uses of DO_CALL_SCV. I believe this will be easier to
maintain, as there will be one less exceptional way to handle scv to
worry about.

And in the end, the benefit of not allocating the dummy frame is
probably negligible, as the overall syscall latency will be dominated by
the context switch + kernel-side handling.

--
Matheus Castanho
  

LGTM

Reviewed-by: Raphael M Zinsly <rzinsly@linux.ibm.com>

On 15/12/2021 11:39, Matheus Castanho wrote:
> v1 was not working properly on hosts without scv support, so some tests were
> failing in such systems. This is fixed now in this new version.
> 
> --8<--
> 
> The syscall function does not allocate the extra stack frame for scv like other
> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
> offset that is used to save LR, syscall ended up using an invalid offset,
> causing regressions on powerpc64. So make sure the extra stack frame is
> allocated in syscall.S as well to make it consistent with other uses of
> DO_CALL_SCV and avoid similar issues in the future.
> 
> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
> ---
>   sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> index a29652feaf..a5497c8370 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> @@ -27,7 +27,11 @@ ENTRY (syscall)
>   	mr   r8,r9
>   #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>   	CHECK_SCV_SUPPORT r9 0f
> +	stdu r1,-SCV_FRAME_SIZE(r1)
> +	cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>   	DO_CALL_SCV
> +	addi r1,r1,SCV_FRAME_SIZE
> +	cfi_adjust_cfa_offset(-SCV_FRAME_SIZE)
>   	RET_SCV
>   	b 1f
>   #endif
>
  

On 12/16/21 10:52 AM, Matheus Castanho wrote:
> 
> Paul E Murphy <murphyp@linux.ibm.com> writes:
> 
>> On 12/15/21 8:39 AM, Matheus Castanho wrote:
>>> v1 was not working properly on hosts without scv support, so some tests were
>>> failing in such systems. This is fixed now in this new version.
>>> --8<--
>>> The syscall function does not allocate the extra stack frame for scv like
>>> other
>>> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
>>> offset that is used to save LR, syscall ended up using an invalid offset,
>>> causing regressions on powerpc64. So make sure the extra stack frame is
>>> allocated in syscall.S as well to make it consistent with other uses of
>>> DO_CALL_SCV and avoid similar issues in the future.
>>> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
>>> ---
>>>    sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> index a29652feaf..a5497c8370 100644
>>> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> @@ -27,7 +27,11 @@ ENTRY (syscall)
>>>    	mr   r8,r9
>>>    #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>>>    	CHECK_SCV_SUPPORT r9 0f
>>> +	stdu r1,-SCV_FRAME_SIZE(r1)
>>> +	cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>>
>> I think this fixes the issue, but it seems like a workaround of a deficiency in
>> the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter with current frame
>> size? It would avoid the need to push a dummy frame here.
>>
> 
> That is an option. But I opted to allocate the dummy frame to make it
> consistent other uses of DO_CALL_SCV. I believe this will be easier to
> maintain, as there will be one less exceptional way to handle scv to
> worry about.
> 
> And in the end, the benefit of not allocating the dummy frame is
> probably negligible, as the overall syscall latency will be dominated by
> the context switch + kernel-side handling.

OK.  I agree with that.  LGTM.
  

Paul E Murphy <murphyp@linux.ibm.com> writes:

> On 12/16/21 10:52 AM, Matheus Castanho wrote:
>> Paul E Murphy <murphyp@linux.ibm.com> writes:
>>
>>> On 12/15/21 8:39 AM, Matheus Castanho wrote:
>>>> v1 was not working properly on hosts without scv support, so some tests were
>>>> failing in such systems. This is fixed now in this new version.
>>>> --8<--
>>>> The syscall function does not allocate the extra stack frame for scv like
>>>> other
>>>> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
>>>> offset that is used to save LR, syscall ended up using an invalid offset,
>>>> causing regressions on powerpc64. So make sure the extra stack frame is
>>>> allocated in syscall.S as well to make it consistent with other uses of
>>>> DO_CALL_SCV and avoid similar issues in the future.
>>>> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
>>>> ---
>>>>    sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> index a29652feaf..a5497c8370 100644
>>>> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> @@ -27,7 +27,11 @@ ENTRY (syscall)
>>>>    	mr   r8,r9
>>>>    #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>>>>    	CHECK_SCV_SUPPORT r9 0f
>>>> +	stdu r1,-SCV_FRAME_SIZE(r1)
>>>> +	cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>>>
>>> I think this fixes the issue, but it seems like a workaround of a deficiency in
>>> the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter with current frame
>>> size? It would avoid the need to push a dummy frame here.
>>>
>> That is an option. But I opted to allocate the dummy frame to make it
>> consistent other uses of DO_CALL_SCV. I believe this will be easier to
>> maintain, as there will be one less exceptional way to handle scv to
>> worry about.
>> And in the end, the benefit of not allocating the dummy frame is
>> probably negligible, as the overall syscall latency will be dominated by
>> the context switch + kernel-side handling.
>
> OK.  I agree with that.  LGTM.

Pushed as ae91d3df24a4a1b1f264d101a71a298bff310d14

Thanks,
Matheus Castanho