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

[Laurent Vivier <laurent@vivier.eu>]

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