[PATCH 5/5] powerpc/tm: Clarify get_tm_stackpointer() by renaming it

[Michael Ellerman]

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