Message ID | 1498783490-23675-1-git-send-email-gromero@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Thanks Gustavo for the patch. On Thu, Jun 29, 2017 at 08:39:23PM -0400, Gustavo Romero wrote: > Currently tm_reclaim() can return with a corrupted vs0 (fp0) or vs32 (v0) > due to the fact vs0 is used to save FPSCR and vs32 is used to save VSCR. > > Later, we recheckpoint trusting that the live state of FP and VEC are ok > depending on the MSR.FP and MSR.VEC bits, i.e. if MSR.FP is enabled that > means the FP registers checkpointed before entering are correct and so > after a treclaim. we can trust the FP live state, and similarly on VEC regs. > However if tm_reclaim() does not return a sane state then tm_recheckpoint() > will recheckpoint a corrupted instate back to the checkpoint area. > > That commit fixes the corruption by restoring vs0 and vs32 from the > ckfp_state and ckvr_state after they are used to save FPSCR and VSCR, > respectively. > > The effect of the issue described above is observed, for instance, once a > VSX unavailable exception is caught in the middle of a transaction with > MSR.FP = 1 or MSR.VEC = 1. If MSR.FP = 1, then after getting back to user > space FP state is corrupted. If MSR.VEC = 1, then VEC state is corrupted. > > The issue does occur if MSR.FP = 0 and MSR.VEC = 0 because ckfp_state and > ckvr_state are both copied from fp_state and vr_state, respectively, and > on recheckpointing both states will be restored from these thread > structures and not from the live state. Just complementing what Gustavo said, currently the tm_recheckpoint() function grabs the values to be re-checkpoint from two places, depending on the MSR configuration. If VEC or FP are disabled on MSR argument when tm_recheckpoint() is called, then it restorese the values from the thread ckvr/ckfp area and recheckpoint these values into processor transactional area. On the other side, if VEC or FP are disabled, it does not restore the vectors and fp registers from the thread ckvec/ckfp area, and it will recheckpoint what is currently on the live registers. If the registers changed after the reclaim, we will send these new values recheckpointed, and later on userspace when the transaction fails. This second scenario is what is causing the error reported on this email thread. In fact, I am wondering if we can hit another hidden bug that changes the fp and vec values between the tm_reclaim() and tm_recheckpoint() invokations, as for example, an interrupt that calls memcpy() in this small mean time. That said, I am wondering if we shouldn't always rely on thread ckfp and ckvec during tm_recheckpoint() (and never rely on the live registers). This should simplify the reclaim/recheckpoint mechanism, and make it less error prone.
On Thu, 2017-06-29 at 20:44 -0400, Gustavo Romero wrote: > Currently tm_reclaim() can return with a corrupted vs0 (fp0) or vs32 (v0) > due to the fact vs0 is used to save FPSCR and vs32 is used to save VSCR. > > Later, we recheckpoint trusting that the live state of FP and VEC are ok > depending on the MSR.FP and MSR.VEC bits, i.e. if MSR.FP is enabled that > means the FP registers checkpointed before entering are correct and so > after a treclaim. we can trust the FP live state, and similarly on VEC regs. > However if tm_reclaim() does not return a sane state then tm_recheckpoint() > will recheckpoint a corrupted instate back to the checkpoint area. > > That commit fixes the corruption by restoring vs0 and vs32 from the > ckfp_state and ckvr_state after they are used to save FPSCR and VSCR, > respectively. > > The effect of the issue described above is observed, for instance, once a > VSX unavailable exception is caught in the middle of a transaction with > MSR.FP = 1 or MSR.VEC = 1. If MSR.FP = 1, then after getting back to user > space FP state is corrupted. If MSR.VEC = 1, then VEC state is corrupted. > > The issue does occur if MSR.FP = 0 and MSR.VEC = 0 because ckfp_state and > ckvr_state are both copied from fp_state and vr_state, respectively, and > on recheckpointing both states will be restored from these thread > structures and not from the live state. > > The issue does no occur also if MSR.FP = 1 and MSR.VEC = 1 because it > implies MSR.VSX = 1 and in that case the VSX unavailable exception does not > happen in the middle of the transactional block. > > Finally, that commit also fixes the MSR used to check if FP or VEC bit are > enabled once we are in tm_reclaim_thread(). Checking ckpt_regs.msr is not > correct because giveup_all(), which copies regs->msr to ckpt_regs.msr, was > not called before and so the ckpt_regs.msr at that point is not valid, i.e. > it does not reflect the MSR state in userspace. > > No regression was observed on powerpc/tm selftests after this fix. > > Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > arch/powerpc/kernel/process.c | 15 ++++++++------- > arch/powerpc/kernel/tm.S | 16 ++++++++++++++++ > 2 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 2ad725e..df8e348 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -870,21 +870,22 @@ static void tm_reclaim_thread(struct thread_struct *thr, > * state is the same as the live state. We need to copy the > * live state to the checkpointed state so that when the > * transaction is restored, the checkpointed state is correct > - * and the aborted transaction sees the correct state. We use > - * ckpt_regs.msr here as that's what tm_reclaim will use to > - * determine if it's going to write the checkpointed state or > - * not. So either this will write the checkpointed registers, > - * or reclaim will. Similarly for VMX. > + * and the aborted transaction sees the correct state. So > + * either this will write the checkpointed registers, or > + * reclaim will. Similarly for VMX. > */ > - if ((thr->ckpt_regs.msr & MSR_FP) == 0) > + if ((thr->regs->msr & MSR_FP) == 0) > memcpy(&thr->ckfp_state, &thr->fp_state, > sizeof(struct thread_fp_state)); > - if ((thr->ckpt_regs.msr & MSR_VEC) == 0) > + if ((thr->regs->msr & MSR_VEC) == 0) > memcpy(&thr->ckvr_state, &thr->vr_state, > sizeof(struct thread_vr_state)); > So the name of that variable is horrifying - I don't know why it is called that I expect to save 'space' but its not helping anyone. Poor variable names not withstanding I believe the original code is correct. What the chkp_regs.msr means here is, the MSR that the thread had when it when it came off the processor. The reason for this is that giveup_fpu (and friends) will modify thread.regs->msr when doing the giveup so when reclaiming we can't trust it to know what the live state really was. check_if_tm_restore_required() copies the thread.regs->msr into the checkpointed structure so we know if the thread was really using FP/VMX/VSX or not. check_if_tm_restore_required() is called before any giveup operation. I do wonder if it would make more sense for the giveup_all() below to be above. I don't think there are any code pathes that can get here without having already done a giveup_all() but, perhaps it is possible. I don't think it will hurt to move it up, it feels more correct > giveup_all(container_of(thr, struct task_struct, thread)); > > + /* After giveup_all(), ckpt_regs.msr contains the same value > + * of regs->msr when we entered in kernel space. > + */ > tm_reclaim(thr, thr->ckpt_regs.msr, cause); > } > > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S > index 3a2d041..67b56af 100644 > --- a/arch/powerpc/kernel/tm.S > +++ b/arch/powerpc/kernel/tm.S > @@ -259,9 +259,18 @@ _GLOBAL(tm_reclaim) > > addi r7, r3, THREAD_CKVRSTATE > SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 transact vr state */ > + > + /* Corrupting v0 (vs32). Should restore it later. */ > mfvscr v0 > li r6, VRSTATE_VSCR > stvx v0, r7, r6 > + > + /* Restore v0 (vs32) from ckvr_state.vr[0], otherwise we might > + * recheckpoint the wrong live value. > + */ > + lxvx vs32, 0, r7 > + xxswapd vs32, vs32 Yes good find! I feel like the crux of the problem is that we don't always tm_recheckpoint() with the same msr as we tm_reclaimed() with, is THIS the core of the problem? In the traps.c case it is an optimisation, perhaps a pointless one, if I had spare time I would benchmark if it is worth it. This code absolutely can't hurt and does solve a real problem. Perhaps use the macro, I think what you want is: /* r0 evaluates to literal zero pp 489 ISA 3.0 */ LXVD2X_ROT(32, r0, r7) > + > dont_backup_vec: > mfspr r0, SPRN_VRSAVE > std r0, THREAD_CKVRSAVE(r3) > @@ -272,9 +281,16 @@ dont_backup_vec: > addi r7, r3, THREAD_CKFPSTATE > SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */ > > + /* Corrupting fr0 (vs0). Should restore it later. */ > mffs fr0 > stfd fr0,FPSTATE_FPSCR(r7) > > + /* Restore fr0 (vs32) from ckfp_state.fpr[0], otherwise we might > + * recheckpoint the wrong live value. > + */ > + lxvx vs0, 0, r7 > + xxswapd vs0, vs0 /* r0 evaluates to literal zero pp 489 ISA 3.0 */ LXVD2X_ROT(0, r0, r7) > + > dont_backup_fp: > > /* TM regs, incl TEXASR -- these live in thread_struct. Note they've
On Fri, 2017-06-30 at 13:41 -0300, Breno Leitao wrote: > Thanks Gustavo for the patch. > > On Thu, Jun 29, 2017 at 08:39:23PM -0400, Gustavo Romero wrote: > > Currently tm_reclaim() can return with a corrupted vs0 (fp0) or vs32 (v0) > > due to the fact vs0 is used to save FPSCR and vs32 is used to save VSCR. > > > > Later, we recheckpoint trusting that the live state of FP and VEC are ok > > depending on the MSR.FP and MSR.VEC bits, i.e. if MSR.FP is enabled that > > means the FP registers checkpointed before entering are correct and so > > after a treclaim. we can trust the FP live state, and similarly on VEC regs. > > However if tm_reclaim() does not return a sane state then tm_recheckpoint() > > will recheckpoint a corrupted instate back to the checkpoint area. > > > > That commit fixes the corruption by restoring vs0 and vs32 from the > > ckfp_state and ckvr_state after they are used to save FPSCR and VSCR, > > respectively. > > > > The effect of the issue described above is observed, for instance, once a > > VSX unavailable exception is caught in the middle of a transaction with > > MSR.FP = 1 or MSR.VEC = 1. If MSR.FP = 1, then after getting back to user > > space FP state is corrupted. If MSR.VEC = 1, then VEC state is corrupted. > > > > The issue does occur if MSR.FP = 0 and MSR.VEC = 0 because ckfp_state and > > ckvr_state are both copied from fp_state and vr_state, respectively, and > > on recheckpointing both states will be restored from these thread > > structures and not from the live state. > > Just complementing what Gustavo said, currently the tm_recheckpoint() > function grabs the values to be re-checkpoint from two places, depending > on the MSR configuration. > > If VEC or FP are disabled on MSR argument when tm_recheckpoint() is > called, then it restorese the values from the thread ckvr/ckfp area and > recheckpoint these values into processor transactional area. > > On the other side, if VEC or FP are disabled, it does not restore the > vectors and fp registers from the thread ckvec/ckfp area, and it will > recheckpoint what is currently on the live registers. If the registers > changed after the reclaim, we will send these new values recheckpointed, > and later on userspace when the transaction fails. > > This second scenario is what is causing the error reported on this > email thread. In fact, I am wondering if we can hit another hidden bug that > changes the fp and vec values between the tm_reclaim() and > tm_recheckpoint() invokations, as for example, an interrupt that calls > memcpy() in this small mean time. > > That said, I am wondering if we shouldn't always rely on thread ckfp and > ckvec during tm_recheckpoint() (and never rely on the live registers). This > should simplify the reclaim/recheckpoint mechanism, and make it less > error prone. I think you're absolutely correct - its a mess. Personally I think that the assembly functions should do the bare minimum. So the two cases that you describe which are handled in assembly could easily be handled in C either before the call to the assembly or after. Cyril
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 2ad725e..df8e348 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -870,21 +870,22 @@ static void tm_reclaim_thread(struct thread_struct *thr, * state is the same as the live state. We need to copy the * live state to the checkpointed state so that when the * transaction is restored, the checkpointed state is correct - * and the aborted transaction sees the correct state. We use - * ckpt_regs.msr here as that's what tm_reclaim will use to - * determine if it's going to write the checkpointed state or - * not. So either this will write the checkpointed registers, - * or reclaim will. Similarly for VMX. + * and the aborted transaction sees the correct state. So + * either this will write the checkpointed registers, or + * reclaim will. Similarly for VMX. */ - if ((thr->ckpt_regs.msr & MSR_FP) == 0) + if ((thr->regs->msr & MSR_FP) == 0) memcpy(&thr->ckfp_state, &thr->fp_state, sizeof(struct thread_fp_state)); - if ((thr->ckpt_regs.msr & MSR_VEC) == 0) + if ((thr->regs->msr & MSR_VEC) == 0) memcpy(&thr->ckvr_state, &thr->vr_state, sizeof(struct thread_vr_state)); giveup_all(container_of(thr, struct task_struct, thread)); + /* After giveup_all(), ckpt_regs.msr contains the same value + * of regs->msr when we entered in kernel space. + */ tm_reclaim(thr, thr->ckpt_regs.msr, cause); } diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S index 3a2d041..67b56af 100644 --- a/arch/powerpc/kernel/tm.S +++ b/arch/powerpc/kernel/tm.S @@ -259,9 +259,18 @@ _GLOBAL(tm_reclaim) addi r7, r3, THREAD_CKVRSTATE SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 transact vr state */ + + /* Corrupting v0 (vs32). Should restore it later. */ mfvscr v0 li r6, VRSTATE_VSCR stvx v0, r7, r6 + + /* Restore v0 (vs32) from ckvr_state.vr[0], otherwise we might + * recheckpoint the wrong live value. + */ + lxvx vs32, 0, r7 + xxswapd vs32, vs32 + dont_backup_vec: mfspr r0, SPRN_VRSAVE std r0, THREAD_CKVRSAVE(r3) @@ -272,9 +281,16 @@ dont_backup_vec: addi r7, r3, THREAD_CKFPSTATE SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */ + /* Corrupting fr0 (vs0). Should restore it later. */ mffs fr0 stfd fr0,FPSTATE_FPSCR(r7) + /* Restore fr0 (vs32) from ckfp_state.fpr[0], otherwise we might + * recheckpoint the wrong live value. + */ + lxvx vs0, 0, r7 + xxswapd vs0, vs0 + dont_backup_fp: /* TM regs, incl TEXASR -- these live in thread_struct. Note they've