| Message ID | 153294521235.6959.12520498748431693809.stgit@dhcp-9-109-246-16 |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] linux-user: ppc64: don't use volatile register during safe_syscall | expand |
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~
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
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
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
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(-)