[PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
Gustavo Romero
gromero at linux.vnet.ibm.com
Sat Jan 18 07:57:20 AEDT 2020
Hi Gustavo,
Thanks for fixing that TM issue.
On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:
> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
> Cc: stable at vger.kernel.org # v3.9
> Signed-off-by: Gustavo Luiz Duarte <gustavold at linux.ibm.com>
> ---
> arch/powerpc/kernel/signal.c | 17 +++++++++++++++--
> arch/powerpc/kernel/signal_32.c | 24 ++++++++++--------------
> arch/powerpc/kernel/signal_64.c | 20 ++++++++------------
> 3 files changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index e6c30cee6abf..1660be1061ac 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
> * normal/non-checkpointed stack pointer.
> */
>
> + unsigned long ret = tsk->thread.regs->gpr[1];
> +
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> BUG_ON(tsk != current);
>
> if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
> + preempt_disable();
> tm_reclaim_current(TM_CAUSE_SIGNAL);
> if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
> - return tsk->thread.ckpt_regs.gpr[1];
> + ret = tsk->thread.ckpt_regs.gpr[1];
> +
> + /* If we treclaim, we must immediately clear the current
> + * thread's TM bits. Otherwise we might be preempted and have
> + * the live MSR[TS] changed behind our back
> + * (tm_recheckpoint_new_task() would recheckpoint).
> + * Besides, we enter the signal handler in non-transactional
> + * state.
> + */
> + tsk->thread.regs->msr &= ~MSR_TS_MASK;
> + preempt_enable();
> }
> #endif
> - return tsk->thread.regs->gpr[1];
> + return ret;
> }
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 98600b276f76..132a092cd170 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
> */
> static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> - struct mcontext __user *tm_frame, int sigret)
> + struct mcontext __user *tm_frame, int sigret,
> + unsigned long msr)
> {
> - unsigned long msr = regs->msr;
> -
> WARN_ON(tm_suspend_disabled);
>
> - /* Remove TM bits from thread's MSR. The MSR in the sigcontext
> - * just indicates to userland that we were doing a transaction, but we
> - * don't want to return in transactional state. This also ensures
> - * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
> - */
> - regs->msr &= ~MSR_TS_MASK;
> -
> /* Save both sets of general registers */
> if (save_general_regs(¤t->thread.ckpt_regs, frame)
> || save_general_regs(regs, tm_frame))
> @@ -912,6 +904,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
> int sigret;
> unsigned long tramp;
> struct pt_regs *regs = tsk->thread.regs;
> + /* Save the thread's msr before get_tm_stackpointer() changes it */
> + unsigned long msr = regs->msr;
>
> BUG_ON(tsk != current);
>
> @@ -944,13 +938,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> tm_frame = &rt_sf->uc_transact.uc_mcontext;
> - if (MSR_TM_ACTIVE(regs->msr)) {
> + if (MSR_TM_ACTIVE(msr)) {
> if (__put_user((unsigned long)&rt_sf->uc_transact,
> &rt_sf->uc.uc_link) ||
> __put_user((unsigned long)tm_frame,
> &rt_sf->uc_transact.uc_regs))
> goto badframe;
> - if (save_tm_user_regs(regs, frame, tm_frame, sigret))
> + if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
> goto badframe;
> }
> else
> @@ -1369,6 +1363,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
> int sigret;
> unsigned long tramp;
> struct pt_regs *regs = tsk->thread.regs;
> + /* Save the thread's msr before get_tm_stackpointer() changes it */
> + unsigned long msr = regs->msr;
>
> BUG_ON(tsk != current);
>
> @@ -1402,9 +1398,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> tm_mctx = &frame->mctx_transact;
> - if (MSR_TM_ACTIVE(regs->msr)) {
> + if (MSR_TM_ACTIVE(msr)) {
> if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
> - sigret))
> + sigret, msr))
> goto badframe;
> }
> else
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 117515564ec7..e5b5f9738056 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> struct sigcontext __user *tm_sc,
> struct task_struct *tsk,
> - int signr, sigset_t *set, unsigned long handler)
> + int signr, sigset_t *set, unsigned long handler,
> + unsigned long msr)
> {
> /* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
> * process never used altivec yet (MSR_VEC is zero in pt_regs of
> @@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
> #endif
> struct pt_regs *regs = tsk->thread.regs;
> - unsigned long msr = tsk->thread.regs->msr;
> long err = 0;
>
> BUG_ON(tsk != current);
>
> - BUG_ON(!MSR_TM_ACTIVE(regs->msr));
> + BUG_ON(!MSR_TM_ACTIVE(msr));
>
> WARN_ON(tm_suspend_disabled);
>
> @@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> */
> msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
>
> - /* Remove TM bits from thread's MSR. The MSR in the sigcontext
> - * just indicates to userland that we were doing a transaction, but we
> - * don't want to return in transactional state. This also ensures
> - * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
> - */
> - regs->msr &= ~MSR_TS_MASK;
> -
> #ifdef CONFIG_ALTIVEC
> err |= __put_user(v_regs, &sc->v_regs);
> err |= __put_user(tm_v_regs, &tm_sc->v_regs);
> @@ -824,6 +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;
> + /* Save the thread's msr before get_tm_stackpointer() changes it */
> + unsigned long msr = regs->msr;
>
> BUG_ON(tsk != current);
>
> @@ -841,7 +836,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> 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(regs->msr)) {
> + if (MSR_TM_ACTIVE(msr)) {
> /* The ucontext_t passed to userland points to the second
> * ucontext_t (for transactional state) with its uc_link ptr.
> */
> @@ -849,7 +844,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
> &frame->uc_transact.uc_mcontext,
> tsk, ksig->sig, NULL,
> - (unsigned long)ksig->ka.sa.sa_handler);
> + (unsigned long)ksig->ka.sa.sa_handler,
> + msr);
> } else
> #endif
> {
>
The change needs to be improved in case CONFIG_PPC_TRANSACTIONAL_MEM=n, like:
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 132a092cd170..1b090a76b444 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -904,8 +904,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
int sigret;
unsigned long tramp;
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
BUG_ON(tsk != current);
@@ -1363,8 +1365,10 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
int sigret;
unsigned long tramp;
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
BUG_ON(tsk != current);
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index e5b5f9738056..84ed2e77ef9c 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -817,8 +817,10 @@ 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
BUG_ON(tsk != current);
or -Werror=unused-variable will catch it like:
/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_rt_signal32':
/linux/arch/powerpc/kernel/signal_32.c:908:16: error: unused variable 'msr' [-Werror=unused-variable]
908 | unsigned long msr = regs->msr;
| ^~~
/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
/linux/arch/powerpc/kernel/signal_32.c:1367:16: error: unused variable 'msr' [-Werror=unused-variable]
1367 | unsigned long msr = regs->msr;
|
Feel free to send a v2 only after Mikey's review.
Otherwise, LGTM.
Reviewed-by: Gustavo Romero <gromero at linux.ibm.com>
Best regards,
Gustavo
More information about the Linuxppc-dev
mailing list