[PATCH v3 4/8] powerpc/signal64: Remove TM ifdefery in middle of if/else block

[Christophe Leroy]

Le 09/01/2021 à 04:25, Christopher M. Riedl a écrit :
> Rework the messy ifdef breaking up the if-else for TM similar to
> commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").
> 
> Unlike that commit for ppc32, the ifdef can't be removed entirely since
> uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.
> 
> Signed-off-by: Christopher M. Riedl <cmr at codefail.de>
> ---
>   arch/powerpc/kernel/signal_64.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index b211a8ea4f6e..dd3787f67a78 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
>   	struct pt_regs *regs = current_pt_regs();
>   	struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
>   	sigset_t set;
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	unsigned long msr;
> -#endif
>   
>   	/* Always make any pending restarted system calls return -EINTR */
>   	current->restart_block.fn = do_no_restart_syscall;
> @@ -762,10 +760,12 @@ SYSCALL_DEFINE0(rt_sigreturn)
>   	 * restore_tm_sigcontexts.
>   	 */
>   	regs->msr &= ~MSR_TS_MASK;
> +#endif
>   
>   	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
>   		goto badframe;

This means you are doing that __get_user() even when msr is not used. That should be avoided.

>   	if (MSR_TM_ACTIVE(msr)) {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   		/* We recheckpoint on return. */
>   		struct ucontext __user *uc_transact;
>   
> @@ -778,9 +778,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>   		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
>   					   &uc_transact->uc_mcontext))
>   			goto badframe;
> -	} else
>   #endif
> -	{
> +	} else {
>   		/*
>   		 * Fall through, for non-TM restore
>   		 *
> @@ -818,10 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	unsigned long newsp = 0;
>   	long err = 0;
>   	struct pt_regs *regs = tsk->thread.regs;
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	/* Save the thread's msr before get_tm_stackpointer() changes it */
> -	unsigned long msr = regs->msr;
> -#endif
> +	unsigned long msr __maybe_unused = regs->msr;

I don't thing __maybe_unused() is the right solution.

I think MSR_TM_ACTIVE() should be fixed instead, either by changing it into a static inline 
function, or doing something similar to 
https://github.com/linuxppc/linux/commit/05a4ab823983d9136a460b7b5e0d49ee709a6f86

>   
>   	frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
>   	if (!access_ok(frame, sizeof(*frame)))
> @@ -836,8 +833,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	/* Create the ucontext.  */
>   	err |= __put_user(0, &frame->uc.uc_flags);
>   	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +
>   	if (MSR_TM_ACTIVE(msr)) {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   		/* The ucontext_t passed to userland points to the second
>   		 * ucontext_t (for transactional state) with its uc_link ptr.
>   		 */
> @@ -847,9 +845,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   					    tsk, ksig->sig, NULL,
>   					    (unsigned long)ksig->ka.sa.sa_handler,
>   					    msr);
> -	} else
>   #endif
> -	{
> +	} else {
>   		err |= __put_user(0, &frame->uc.uc_link);
>   		prepare_setup_sigcontext(tsk, 1);
>   		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> 

Christophe