[Qemu-devel] [PATCH v3] linux-user: ppc64: don't use volatile register during safe_syscall

[Qemu-devel] [PATCH v3] linux-user: ppc64: don't use volatile register during safe_syscall

[Shivaprasad G Bhat]

r11 is a volatile register on PPC as per calling conventions.
The safe_syscall code uses it to check if the signal_pending
is set during the safe_syscall. When a syscall is interrupted
on return from signal handling, the r11 might be corrupted
before we retry the syscall leading to a crash. The registers
r0-r13 are not to be used here as they have
volatile/designated/reserved usages.

Change the code to use r14 which is non-volatile.
Use SP+16 which is a slot for LR, for save/restore of previous value
of r14. SP+16 can be used, as LR is preserved across the syscall.

Steps to reproduce:
On PPC host, issue `qemu-x86_64 /usr/bin/cc -E -`
Attempt Ctrl-C, the issue is reproduced.

Reference:
https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG
https://openpowerfoundation.org/wp-content/uploads/2016/03/ABI64BitOpenPOWERv1.1_16July2015_pub4.pdf

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
---
v2: https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05102.html
Changes from v2:
   Added code to store and restore r14 register. 

v1: https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05089.html
Changes from v1:
   Fixed the commit message as suggested

 linux-user/host/ppc64/safe-syscall.inc.S |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S
index d30050a67c..ca85da13bd 100644
--- a/linux-user/host/ppc64/safe-syscall.inc.S
+++ b/linux-user/host/ppc64/safe-syscall.inc.S
@@ -49,7 +49,8 @@ safe_syscall_base:
 	 *               and returns the result in r3
 	 * Shuffle everything around appropriately.
 	 */
-	mr	11, 3	/* signal_pending */
+	std     14, 16(1) /* Preserve r14 in SP+16 */
+	mr	14, 3	/* signal_pending */
 	mr	0, 4	/* syscall number */
 	mr	3, 5	/* syscall arguments */
 	mr	4, 6
@@ -67,11 +68,12 @@ safe_syscall_base:
 	 */
 safe_syscall_start:
 	/* if signal_pending is non-zero, don't do the call */
-	lwz	12, 0(11)
+	lwz	12, 0(14)
 	cmpwi	0, 12, 0
 	bne-	0f
 	sc
 safe_syscall_end:
+	ld 14, 16(1) /* restore r14 to its original value */
 	/* code path when we did execute the syscall */
 	bnslr+
 
@@ -81,6 +83,7 @@ safe_syscall_end:
 
 	/* code path when we didn't execute the syscall */
 0:	addi	3, 0, -TARGET_ERESTARTSYS
+	ld 14, 16(1) /* restore r14 to its orginal value */
 	blr
 	.cfi_endproc
 

Re: [Qemu-devel] [PATCH v3] linux-user: ppc64: don't use volatile register during safe_syscall

[Richard Henderson]

On 07/30/2018 06:09 AM, Shivaprasad G Bhat wrote:
> r11 is a volatile register on PPC as per calling conventions.
> The safe_syscall code uses it to check if the signal_pending
> is set during the safe_syscall. When a syscall is interrupted
> on return from signal handling, the r11 might be corrupted
> before we retry the syscall leading to a crash. The registers
> r0-r13 are not to be used here as they have
> volatile/designated/reserved usages.
> 
> Change the code to use r14 which is non-volatile.
> Use SP+16 which is a slot for LR, for save/restore of previous value
> of r14. SP+16 can be used, as LR is preserved across the syscall.
> 
> Steps to reproduce:
> On PPC host, issue `qemu-x86_64 /usr/bin/cc -E -`
> Attempt Ctrl-C, the issue is reproduced.
> 
> Reference:
> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG
> https://openpowerfoundation.org/wp-content/uploads/2016/03/ABI64BitOpenPOWERv1.1_16July2015_pub4.pdf
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>

This does fix the bug, so
Tested-by: Richard Henderson <richard.henderson@linaro.org>

But we can do slightly better:

> @@ -49,7 +49,8 @@ safe_syscall_base:
>  	 *               and returns the result in r3
>  	 * Shuffle everything around appropriately.
>  	 */
> -	mr	11, 3	/* signal_pending */
> +	std     14, 16(1) /* Preserve r14 in SP+16 */

Above this context, we have a .cfi_startproc directive, which indicates we are
providing unwind information for this function (trivial up to this point).

To preserve that, you need to add

	.cfi_offset 14, 16

right here, indicating r14 is saved 16 bytes "up" the stack frame.

Since this portion of the stack frame is allocated by the caller, this saved
value remains valid through the end of the function, so we do not need to do
anything at the points where the value is restored.

>  safe_syscall_end:
> +	ld 14, 16(1) /* restore r14 to its original value */
>  	/* code path when we did execute the syscall */

Swap these two lines.

With those two changes,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v3] linux-user: ppc64: don't use volatile register during safe_syscall

[Laurent Vivier]

Le 30/07/2018 à 14:44, Richard Henderson a écrit :
> On 07/30/2018 06:09 AM, Shivaprasad G Bhat wrote:
>> r11 is a volatile register on PPC as per calling conventions.
>> The safe_syscall code uses it to check if the signal_pending
>> is set during the safe_syscall. When a syscall is interrupted
>> on return from signal handling, the r11 might be corrupted
>> before we retry the syscall leading to a crash. The registers
>> r0-r13 are not to be used here as they have
>> volatile/designated/reserved usages.
>>
>> Change the code to use r14 which is non-volatile.
>> Use SP+16 which is a slot for LR, for save/restore of previous value
>> of r14. SP+16 can be used, as LR is preserved across the syscall.
>>
>> Steps to reproduce:
>> On PPC host, issue `qemu-x86_64 /usr/bin/cc -E -`
>> Attempt Ctrl-C, the issue is reproduced.
>>
>> Reference:
>> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG
>> https://openpowerfoundation.org/wp-content/uploads/2016/03/ABI64BitOpenPOWERv1.1_16July2015_pub4.pdf
>>
>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> 
> This does fix the bug, so
> Tested-by: Richard Henderson <richard.henderson@linaro.org>
> 
> But we can do slightly better:
> 
>> @@ -49,7 +49,8 @@ safe_syscall_base:
>>  	 *               and returns the result in r3
>>  	 * Shuffle everything around appropriately.
>>  	 */
>> -	mr	11, 3	/* signal_pending */
>> +	std     14, 16(1) /* Preserve r14 in SP+16 */
> 
> Above this context, we have a .cfi_startproc directive, which indicates we are
> providing unwind information for this function (trivial up to this point).
> 
> To preserve that, you need to add
> 
> 	.cfi_offset 14, 16
> 
> right here, indicating r14 is saved 16 bytes "up" the stack frame.
> 
> Since this portion of the stack frame is allocated by the caller, this saved
> value remains valid through the end of the function, so we do not need to do
> anything at the points where the value is restored.
> 
>>  safe_syscall_end:
>> +	ld 14, 16(1) /* restore r14 to its original value */
>>  	/* code path when we did execute the syscall */
> 
> Swap these two lines.
> 
> With those two changes,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Tested-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>

I think this patch should go into the next -rc because it fixes a lot of
failures in the LTP on ppc64 host (hundreds of failure...).

David, if you want, I can take it through the linux-user tree.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v3] linux-user: ppc64: don't use volatile register during safe_syscall

[David Gibson]

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

On Mon, 30 Jul 2018 21:16:35 +0200
Laurent Vivier <laurent@vivier.eu> wrote:

> Le 30/07/2018 à 14:44, Richard Henderson a écrit :
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> 
> Tested-by: Laurent Vivier <laurent@vivier.eu>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> I think this patch should go into the next -rc because it fixes a lot of
> failures in the LTP on ppc64 host (hundreds of failure...).
> 
> David, if you want, I can take it through the linux-user tree.

That'd be great, thanks.

> 
> Thanks,
> Laurent


-- 
David Gibson <dgibson@redhat.com>
Principal Software Engineer, Virtualization, Red Hat

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