| Message ID | 1447390652-28355-5-git-send-email-mikey@neuling.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote: > get_tm_stackpointer() gets the userspace stack pointer which the > signal frame will be written from pt_regs. To do this it performs a > tm_reclaim to access the checkpointed stack pointer. > > Doing a tm_reclaim is an important operation which changes the machine > state. Doing this in the wrong state or an attempt to do it twice it can > result in a TM Bad Thing exception. Hence we should make it clearer > that this function is making this change. > > This patch renames this function to make it clearer what's happening > when calling this function. No functional change. Urgh. It's really not very pretty. > @@ -1001,7 +1001,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset, > > /* Set up Signal Frame */ > /* Put a Real Time Context onto stack */ > - rt_sf = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*rt_sf), 1); > + rt_sf = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs), > + sizeof(*rt_sf), 1); > addr = rt_sf; > if (unlikely(rt_sf == NULL)) > goto badframe; I think partly we just have the wrong abstraction here. We try to hide TM inside get_tm_stackpointer(), but then we confused ourselves by hiding the reclaim in there. And all three call sites then *also* have an #ifdef CONFIG_PPC_TRANSACTIONAL_MEM block in the function, so we failed at hiding TM anyway. I wonder if we'd be better off relying more on the compiler to elide code behind an always false boolean, something like: int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs) { struct rt_sigframe __user *frame; unsigned long sp, newsp = 0; bool tm_active; long err = 0; tm_active = msr_tm_active(regs->msr); if (tm_active) sp = tm_reclaim_sp(regs); else sp = regs->gpr[1]; frame = get_sigframe(ksig, sp, sizeof(*frame), 0); if (unlikely(frame == NULL)) goto badframe; ... -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM - if (MSR_TM_ACTIVE(regs->msr)) { + if (tm_active) { /* The ucontext_t passed to userland points to the second * ucontext_t (for transactional state) with its uc_link ptr. */ err |= __put_user(&frame->uc_transact, &frame->uc.uc_link); err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext, We'd need to move more of the __put_user() logic into a helper, because uc_transact doesn't exist when TM is disabled. cheers
diff --git a/arch/powerpc/include/asm/signal.h b/arch/powerpc/include/asm/signal.h index 9322c28..0139dc2 100644 --- a/arch/powerpc/include/asm/signal.h +++ b/arch/powerpc/include/asm/signal.h @@ -5,6 +5,6 @@ #include <uapi/asm/signal.h> #include <uapi/asm/ptrace.h> -extern unsigned long get_tm_stackpointer(struct pt_regs *regs); +extern unsigned long get_tm_stackpointer_and_reclaim(struct pt_regs *regs); #endif /* _ASM_POWERPC_SIGNAL_H */ diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index a1b41d1..264a39c 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -555,7 +555,7 @@ static void tm_reclaim_thread(struct thread_struct *thr, * Use the current MSR TM suspended bit to track if we have * checkpointed state outstanding. * On signal delivery, we'd normally reclaim the checkpointed - * state to obtain stack pointer (see:get_tm_stackpointer()). + * state to obtain stack pointer (see:get_tm_stackpointer_and_reclaim()). * This will then directly return to userspace without going * through __switch_to(). However, if the stack frame is bad, * we need to exit this thread which calls __switch_to() which diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c index cf8c7e4..b3de865 100644 --- a/arch/powerpc/kernel/signal.c +++ b/arch/powerpc/kernel/signal.c @@ -162,7 +162,7 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags) user_enter(); } -unsigned long get_tm_stackpointer(struct pt_regs *regs) +unsigned long get_tm_stackpointer_and_reclaim(struct pt_regs *regs) { /* When in an active transaction that takes a signal, we need to be * careful with the stack. It's possible that the stack has moved back diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c index 5b519c7..b2c3f8c 100644 --- a/arch/powerpc/kernel/signal_32.c +++ b/arch/powerpc/kernel/signal_32.c @@ -1001,7 +1001,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset, /* Set up Signal Frame */ /* Put a Real Time Context onto stack */ - rt_sf = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*rt_sf), 1); + rt_sf = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs), + sizeof(*rt_sf), 1); addr = rt_sf; if (unlikely(rt_sf == NULL)) goto badframe; @@ -1424,7 +1425,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset, struct pt_regs *regs unsigned long tramp; /* Set up Signal Frame */ - frame = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*frame), 1); + frame = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs), + sizeof(*frame), 1); if (unlikely(frame == NULL)) goto badframe; sc = (struct sigcontext __user *) &frame->sctx; diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c index b320689..96264be 100644 --- a/arch/powerpc/kernel/signal_64.c +++ b/arch/powerpc/kernel/signal_64.c @@ -731,7 +731,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs unsigned long newsp = 0; long err = 0; - frame = get_sigframe(ksig, get_tm_stackpointer(regs), sizeof(*frame), 0); + frame = get_sigframe(ksig, get_tm_stackpointer_and_reclaim(regs), + sizeof(*frame), 0); if (unlikely(frame == NULL)) goto badframe;
get_tm_stackpointer() gets the userspace stack pointer which the signal frame will be written from pt_regs. To do this it performs a tm_reclaim to access the checkpointed stack pointer. Doing a tm_reclaim is an important operation which changes the machine state. Doing this in the wrong state or an attempt to do it twice it can result in a TM Bad Thing exception. Hence we should make it clearer that this function is making this change. This patch renames this function to make it clearer what's happening when calling this function. No functional change. Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/include/asm/signal.h | 2 +- arch/powerpc/kernel/process.c | 2 +- arch/powerpc/kernel/signal.c | 2 +- arch/powerpc/kernel/signal_32.c | 6 ++++-- arch/powerpc/kernel/signal_64.c | 3 ++- 5 files changed, 9 insertions(+), 6 deletions(-)