| Message ID | 1541508028-31865-14-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | New TM Model | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
On Tue, 2018-11-06 at 10:40 -0200, Breno Leitao wrote: > Currently the signal context restore code enables the bit on the MSR > register without restoring the TM SPRs, which can cause undesired side > effects. > > This is not correct because if TM is enabled in MSR, it means the TM SPR > registers are valid and updated, which is not correct here. In fact, the > live registers may contain previous' thread SPRs. > > Functions check if the register values are valid or not through looking > if the facility is enabled at MSR, as MSR[TM] set means that the TM SPRs > are hot. > > When just enabling MSR[TM] without updating the live SPRs, this can cause a > crash, since current TM SPR from previous thread will be saved on the > current thread, and might not have TEXASR[FS] set, for example. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > arch/powerpc/kernel/signal_64.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index 487c3b6aa2e3..156b90e8ee78 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -478,8 +478,18 @@ static long restore_tm_sigcontexts(struct task_struct > *tsk, > * happened whilst in the signal handler and load_tm overflowed, > * disabling the TM bit. In either case we can end up with an illegal > * TM state leading to a TM Bad Thing when we return to userspace. > + * > + * Every time MSR_TM is enabled, mainly for the b) case, the TM SPRs > + * must be restored in the live registers, since the live registers > + * could contain garbage and later we want to read from live, since > + * MSR_TM is enabled, and MSR[TM] is what is used to check if the > + * TM SPRs live registers are valid or not. > */ > - regs->msr |= MSR_TM; > + if ((regs->msr & MSR_TM) == 0) { > + regs->msr |= MSR_TM; > + tm_enable(); > + tm_restore_sprs(&tsk->thread); > + } I'm wondering if we should put the save/restore TM registers in the early entry/exit code too. We'd need to add the check on msr[tm]/load_tm. Distributing the SPR save/restore throughout the kernel is just going to lead us to similar problems that we are having now with reclaim/recheckpoint. Mikey > > /* pull in MSR LE from user context */ > regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c index 487c3b6aa2e3..156b90e8ee78 100644 --- a/arch/powerpc/kernel/signal_64.c +++ b/arch/powerpc/kernel/signal_64.c @@ -478,8 +478,18 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, * happened whilst in the signal handler and load_tm overflowed, * disabling the TM bit. In either case we can end up with an illegal * TM state leading to a TM Bad Thing when we return to userspace. + * + * Every time MSR_TM is enabled, mainly for the b) case, the TM SPRs + * must be restored in the live registers, since the live registers + * could contain garbage and later we want to read from live, since + * MSR_TM is enabled, and MSR[TM] is what is used to check if the + * TM SPRs live registers are valid or not. */ - regs->msr |= MSR_TM; + if ((regs->msr & MSR_TM) == 0) { + regs->msr |= MSR_TM; + tm_enable(); + tm_restore_sprs(&tsk->thread); + } /* pull in MSR LE from user context */ regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
Currently the signal context restore code enables the bit on the MSR register without restoring the TM SPRs, which can cause undesired side effects. This is not correct because if TM is enabled in MSR, it means the TM SPR registers are valid and updated, which is not correct here. In fact, the live registers may contain previous' thread SPRs. Functions check if the register values are valid or not through looking if the facility is enabled at MSR, as MSR[TM] set means that the TM SPRs are hot. When just enabling MSR[TM] without updating the live SPRs, this can cause a crash, since current TM SPR from previous thread will be saved on the current thread, and might not have TEXASR[FS] set, for example. Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/signal_64.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)