LinuxPPC-Dev Archive on lore.kernel.org

* Re: signal/syscall/swapcontext fixes
From: David Woodhouse @ 2006-03-07 13:09 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev
In-Reply-To: <17421.32092.442696.49291@cargo.ozlabs.ibm.com>

On Tue, 2006-03-07 at 23:32 +1100, Paul Mackerras wrote:
> * The TIF_SAVE_NVGPRS mechanism was only being used by swapcontext, so
>   I removed it and put swapcontext back the way it was in 2.6.15, with
>   an assembler stub to save the non-volatile GPRs before calling the C
>   code.  The save_general_regs functions now WARN_ON(!FULL_REGS(regs)).

OK. As observed, I'd mostly obsoleted TIF_SAVE_NVGPRS when I implemented
TIF_RESTORE_SIGMASK.

The 64-bit version (bl .save_nvgprs) is prettier than the 32-bit version
(doing it longhand), and they're still gratuitously different. We should
probably fix that.

> * 32-bit wasn't testing the _TIF_NOERROR bit in the syscall fast exit
>   path, so it was only doing anything with it once it saw some other
>   bit being set.

OK.

> * 32-bit wasn't doing the call to ptrace_notify in the syscall exit
>   path when the _TIF_SINGLESTEP bit was set.

Yeah. That was historical (arch/ppc never did this) but nonetheless
still wrong. Without it, we never stop on the instruction immediately
following the 'sc', when we're supposed to be single-stepping.

On a similar note, we should also do a ptrace stop when we deliver
signals, to ensure that the debugger gets a stop _on_ the first
instruction of the handler. Once upon a time there was a stop in
handle_rt_signal() and handle_signal(), but doing it that way gave a
_double_ stop when we ended up in a signal handler from sigsuspend(),
because the syscall stop you just fixed for ppc32 happened too.

> * _TIF_RESTOREALL was in both _TIF_USER_WORK_MASK and
>   _TIF_PERSYSCALL_MASK, which is odd since _TIF_RESTOREALL is only set
>   by system calls.  I took it out of _TIF_USER_WORK_MASK.

Right.

> * On 64-bit, _TIF_RESTOREALL wasn't causing the non-volatile registers
>   to be restored (unless perhaps a signal was delivered or the syscall
>   was traced or single-stepped).  Thus the non-volatile registers
>   weren't restored on exit from a signal handler.  We probably got
>   away with it mostly because signal handlers written in C wouldn't
>   alter the non-volatile registers.

Hm, yes. The idea behind TIF_RESTOREALL was just to prevent r3 and the
flags from getting stomped on -- I didn't look at the nvgprs at that
point, and I'm not sure if the nvgprs were being restored on ppc32
before my changes. They could well have been on ppc64 -- this was
another area in which the two were different.

> * On 32-bit I simplified the code and made it more like 64-bit by
>   making the syscall exit path jump to ret_from_except to handle
>   preemption and signal delivery.

OK, good. It would actually be nice if we could split a bunch of this
code out into a shared file and use the PPC_LL, PPC_STL, etc. macros.

> * 32-bit was calling do_signal unnecessarily when _TIF_RESTOREALL was
>   set - but I think because of that 32-bit was actually restoring the
>   non-volatile registers on exit from a signal handler.

OK.

> * I changed the order of enabling interrupts and saving the
>   non-volatile registers before calling do_syscall_trace_leave; now we
>   enable interrupts first.

OK.

> Any comments before I send this off to Linus to go in 2.6.16?

Looks good.

On a purely cosmetic note I'm not sure about this though -- it was
slightly easier to follow when it was more explicit:
-       andi.   r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_RESTOREALL|_TIF_SAVE_NVGPRS|_TIF_NOERROR|_TIF_RESTORE_SIGMASK)
+       andi.   r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)

-- 
dwmw2

^ permalink raw reply