| Message ID | f1ddc3bc4b970a480367a45cdca51e879cb14df0.1401852531.git.sam.bobroff@au1.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Sam, Comments inline .. On Wed, 2014-06-04 at 13:33 +1000, Sam Bobroff wrote: > Correct the DSCR SPR becoming temporarily corrupted when a task is > context switched when within a transaction. It is corrected when > the transaction is aborted (which will happen after a context switch) > but if the task has suspended (TSUSPEND) the transaction the incorrect > value can be seen. I don't quite follow this description. How is it corrected when the transaction is aborted, and when does that usually happen? If that happens the task can't ever see the corrupted value? To hit the suspended case, the task starts a transaction, suspends it, is then context switched out and back in, and at that point it can see the wrong value? > The problem is caused by saving a thread's DSCR after it has already > been reverted to the CPU's default value: > > __switch_to() calls __switch_to_tm() > which calls tm_reclaim_task() > which calls tm_reclaim_thread() > which calls tm_reclaim() where the DSCR is reset Where the DSCR is set to DSCR_DEFAULT ? Or now PACA_DSCR since your previous patches? Could we instead fix the bug there by reverting to the thread's DSCR value? > __switch_to() calls _switch > _switch() saves the DSCR to thread.dscr > > The fix is to treat the DSCR similarly to the TAR and save it early > in __switch_to(). > > The program below will expose the problem: Can you drop this in tools/testing/selftests/powerpc/tm ? You'll need to create that directory, you can ape the Makefile from the pmu directory, it should be fairly obvious. See the pmu tests for how to integrate with the test harness etc., or bug me if it's not straight forward. > diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h > index 2737f46..3efd0e5 100644 > --- a/arch/powerpc/include/asm/switch_to.h > +++ b/arch/powerpc/include/asm/switch_to.h > @@ -16,13 +16,15 @@ struct thread_struct; > extern struct task_struct *_switch(struct thread_struct *prev, > struct thread_struct *next); > #ifdef CONFIG_PPC_BOOK3S_64 > -static inline void save_tar(struct thread_struct *prev) > +static inline void save_early_sprs(struct thread_struct *prev) > { > if (cpu_has_feature(CPU_FTR_ARCH_207S)) > prev->tar = mfspr(SPRN_TAR); > + if (cpu_has_feature(CPU_FTR_DSCR)) > + prev->dscr = mfspr(SPRN_DSCR); > } Are we going to end up saving more SPRs in this code? What makes the TAR & DSCR special vs everything else? The nice thing about doing this in asm is it's nop'ed out for cpus that don't have the DSCR. What does the generated code for this look like? > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index 662c6dd..a107f4a 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -432,12 +432,6 @@ BEGIN_FTR_SECTION > std r24,THREAD_VRSAVE(r3) > END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC) > #endif /* CONFIG_ALTIVEC */ > -#ifdef CONFIG_PPC64 > -BEGIN_FTR_SECTION > - mfspr r25,SPRN_DSCR > - std r25,THREAD_DSCR(r3) > -END_FTR_SECTION_IFSET(CPU_FTR_DSCR) > -#endif > and. r0,r0,r22 > beq+ 1f > andc r22,r22,r0 cheers
On Wed, 2014-06-04 at 17:31 +1000, Michael Ellerman wrote: > Hi Sam, > > Comments inline .. Ditto.... > > On Wed, 2014-06-04 at 13:33 +1000, Sam Bobroff wrote: > > Correct the DSCR SPR becoming temporarily corrupted when a task is > > context switched when within a transaction. It is corrected when > > the transaction is aborted (which will happen after a context switch) > > but if the task has suspended (TSUSPEND) the transaction the incorrect > > value can be seen. > > I don't quite follow this description. How is it corrected when the transaction > is aborted, and when does that usually happen? If that happens the task can't > ever see the corrupted value? > > To hit the suspended case, the task starts a transaction, suspends it, is then > context switched out and back in, and at that point it can see the wrong value? Yep, that's it and it's corrupted until the transaction is rolled back (normally at the tresume). At the tresume it gets rolled back to the checkpointed value at tbegin and is no longer corrupt. > > The problem is caused by saving a thread's DSCR afterNo it's lost at that point as we've not saved it and it was overwritten when we did the treclaim. it has already > > been reverted to the CPU's default value: > > > > __switch_to() calls __switch_to_tm() > > which calls tm_reclaim_task() > > which calls tm_reclaim_thread() > > which calls tm_reclaim() where the DSCR is reset > > Where the DSCR is set to DSCR_DEFAULT ? Or now PACA_DSCR since your previous > patches? > > Could we instead fix the bug there by reverting to the thread's DSCR value? We really need to save it earlier, before the treclaim which will override it. > > __switch_to() calls _switch > > _switch() saves the DSCR to thread.dscrTBEGIN > > > > The fix is to treat the DSCR similarly to the TAR and save it early > > in __switch_to(). > > > > The program below will expose the problem: > > > Can you drop this in tools/testing/selftests/powerpc/tm ? > > You'll need to create that directory, you can ape the Makefile from the pmu > directory, it should be fairly obvious. See the pmu tests for how to integrate > with the test harness etc., or bug me if it's not straight forward. > > > > diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h > > index 2737f46..3efd0e5 100644 > > --- a/arch/powerpc/include/asm/switch_to.h > > +++ b/arch/powerpc/include/asm/switch_to.h > > @@ -16,13 +16,15 @@ struct thread_struct; > > extern struct task_struct *_switch(struct thread_struct *prev, > > struct thread_struct *next); > > #ifdef CONFIG_PPC_BOOK3S_64 > > -static inline void save_tar(struct thread_struct *prev) > > +static inline void save_early_sprs(struct thread_struct *prev) > > { > > if (cpu_has_feature(CPU_FTR_ARCH_207S)) > > prev->tar = mfspr(SPRN_TAR); > > + if (cpu_has_feature(CPU_FTR_DSCR)) > > + prev->dscr = mfspr(SPRN_DSCR); > > } > > Are we going to end up saving more SPRs in this code? What makes the TAR & DSCR > special vs everything else? There are only a limited set of SPRs that TM checkpoints. The full list is CR, LR, CTR, FPSCR, AMR, PPR, VRSAVE, VSCR, DSCR, and TAR. http://www.scribd.com/doc/142877680/PowerISA-v2-07#outer_page_826 CR, LR, CTR, PPR are handled really early in the exception handler FPSCR, VSCR are done in the FP/VMX/VSX code. AMR we don't care about. That just leaves the DSCR and the TAR for here.... ... and the VRSAVE. Sam: did you have a patch to save that one early too? I think we talked about it but forgot, or did we decide that it's always broken anyway so we don't care? :-D Mikey > The nice thing about doing this in asm is it's nop'ed out for cpus that don't > have the DSCR. What does the generated code for this look like? > > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > > index 662c6dd..a107f4a 100644 > > --- a/arch/powerpc/kernel/entry_64.S > > +++ b/arch/powerpc/kernel/entry_64.S > > @@ -432,12 +432,6 @@ BEGIN_FTR_SECTION > > std r24,THREAD_VRSAVE(r3) > > END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC) > > #endif /* CONFIG_ALTIVEC */ > > -#ifdef CONFIG_PPC64 > > -BEGIN_FTR_SECTION > > - mfspr r25,SPRN_DSCR > > - std r25,THREAD_DSCR(r3) > > -END_FTR_SECTION_IFSET(CPU_FTR_DSCR) > > -#endif > > and. r0,r0,r22 > > beq+ 1f > > andc r22,r22,r0 > > > cheers > >
On 04/06/14 20:03, Michael Neuling wrote: > On Wed, 2014-06-04 at 17:31 +1000, Michael Ellerman wrote: >> Hi Sam, >> >> Comments inline .. > > Ditto.... Responses inline... >> On Wed, 2014-06-04 at 13:33 +1000, Sam Bobroff wrote: >>> Correct the DSCR SPR becoming temporarily corrupted when a task is >>> context switched when within a transaction. It is corrected when >>> the transaction is aborted (which will happen after a context switch) >>> but if the task has suspended (TSUSPEND) the transaction the incorrect >>> value can be seen. >> >> I don't quite follow this description. How is it corrected when the transaction >> is aborted, and when does that usually happen? If that happens the task can't >> ever see the corrupted value? >> >> To hit the suspended case, the task starts a transaction, suspends it, is then >> context switched out and back in, and at that point it can see the wrong value? > > Yep, that's it and it's corrupted until the transaction is rolled back > (normally at the tresume). At the tresume it gets rolled back to the > checkpointed value at tbegin and is no longer corrupt. > I'll re-work the explanation to be clearer about how it becomes corrupt and how it is corrected. >>> The problem is caused by saving a thread's DSCR afterNo it's lost at that point as we've not saved it and it was overwritten when we did the treclaim. it has already >>> been reverted to the CPU's default value: >>> >>> __switch_to() calls __switch_to_tm() >>> which calls tm_reclaim_task() >>> which calls tm_reclaim_thread() >>> which calls tm_reclaim() where the DSCR is reset >> >> Where the DSCR is set to DSCR_DEFAULT ? Or now PACA_DSCR since your previous >> patches? >> >> Could we instead fix the bug there by reverting to the thread's DSCR value? > > We really need to save it earlier, before the treclaim which will > override it. I'll try to improve this explanation as well. >>> __switch_to() calls _switch >>> _switch() saves the DSCR to thread.dscrTBEGIN >>> >>> The fix is to treat the DSCR similarly to the TAR and save it early >>> in __switch_to(). >>> >>> The program below will expose the problem: >> >> >> Can you drop this in tools/testing/selftests/powerpc/tm ? >> >> You'll need to create that directory, you can ape the Makefile from the pmu >> directory, it should be fairly obvious. See the pmu tests for how to integrate >> with the test harness etc., or bug me if it's not straight forward. Will do :-) >>> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h >>> index 2737f46..3efd0e5 100644 >>> --- a/arch/powerpc/include/asm/switch_to.h >>> +++ b/arch/powerpc/include/asm/switch_to.h >>> @@ -16,13 +16,15 @@ struct thread_struct; >>> extern struct task_struct *_switch(struct thread_struct *prev, >>> struct thread_struct *next); >>> #ifdef CONFIG_PPC_BOOK3S_64 >>> -static inline void save_tar(struct thread_struct *prev) >>> +static inline void save_early_sprs(struct thread_struct *prev) >>> { >>> if (cpu_has_feature(CPU_FTR_ARCH_207S)) >>> prev->tar = mfspr(SPRN_TAR); >>> + if (cpu_has_feature(CPU_FTR_DSCR)) >>> + prev->dscr = mfspr(SPRN_DSCR); >>> } >> >> Are we going to end up saving more SPRs in this code? What makes the TAR & DSCR >> special vs everything else? > > There are only a limited set of SPRs that TM checkpoints. The full list > is CR, LR, CTR, FPSCR, AMR, PPR, VRSAVE, VSCR, DSCR, and TAR. > > http://www.scribd.com/doc/142877680/PowerISA-v2-07#outer_page_826 > > CR, LR, CTR, PPR are handled really early in the exception handler > > FPSCR, VSCR are done in the FP/VMX/VSX code. > > AMR we don't care about. > > That just leaves the DSCR and the TAR for here.... > > ... and the VRSAVE. Sam: did you have a patch to save that one early > too? I think we talked about it but forgot, or did we decide that it's > always broken anyway so we don't care? :-D I thought we'd decided that VRSAVE was already probably broken ;-) I haven't tested VRSAVE yet so we don't know if it's actually getting corrupted in this situation (although it seems likely), and from a quick look at the code it's not being treated like DSCR or TAR at the moment so I would need to investigate it separately. > Mikey > >> The nice thing about doing this in asm is it's nop'ed out for cpus that don't >> have the DSCR. What does the generated code for this look like? >> >>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S >>> index 662c6dd..a107f4a 100644 >>> --- a/arch/powerpc/kernel/entry_64.S >>> +++ b/arch/powerpc/kernel/entry_64.S >>> @@ -432,12 +432,6 @@ BEGIN_FTR_SECTION >>> std r24,THREAD_VRSAVE(r3) >>> END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC) >>> #endif /* CONFIG_ALTIVEC */ >>> -#ifdef CONFIG_PPC64 >>> -BEGIN_FTR_SECTION >>> - mfspr r25,SPRN_DSCR >>> - std r25,THREAD_DSCR(r3) >>> -END_FTR_SECTION_IFSET(CPU_FTR_DSCR) >>> -#endif >>> and. r0,r0,r22 >>> beq+ 1f >>> andc r22,r22,r0 >> I wondered a little about this as well. The C code calls this function: static inline int cpu_has_feature(unsigned long feature) { return (CPU_FTRS_ALWAYS & feature) || (CPU_FTRS_POSSIBLE & cur_cpu_spec->cpu_features & feature); } And if I'm extracting it correctly, it ends up like this (17 == SPRN_DSCR): 0x0000000000000bac <__switch_to+108>: rldicl. r8,r9,20,63 0x0000000000000bb0 <__switch_to+112>: beq 0xbc0 <__switch_to+128> 0x0000000000000bb4 <__switch_to+116>: mfspr r9,17 0x0000000000000bb8 <__switch_to+120>: std r9,4280(r31) 0x0000000000000bbc <__switch_to+124>: ld r9,16(r10) I don't think I can comment on it's performance myself; I'll wait for someone more knowledgeable :-) Converting this to ASM could be out of scope for this fix, but it doesn't seem like it would be hard. (I assume we'd want to convert both the TAR and DSCR parts.) >> cheers Cheers, Sam.
diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h index 2737f46..3efd0e5 100644 --- a/arch/powerpc/include/asm/switch_to.h +++ b/arch/powerpc/include/asm/switch_to.h @@ -16,13 +16,15 @@ struct thread_struct; extern struct task_struct *_switch(struct thread_struct *prev, struct thread_struct *next); #ifdef CONFIG_PPC_BOOK3S_64 -static inline void save_tar(struct thread_struct *prev) +static inline void save_early_sprs(struct thread_struct *prev) { if (cpu_has_feature(CPU_FTR_ARCH_207S)) prev->tar = mfspr(SPRN_TAR); + if (cpu_has_feature(CPU_FTR_DSCR)) + prev->dscr = mfspr(SPRN_DSCR); } #else -static inline void save_tar(struct thread_struct *prev) {} +static inline void save_early_sprs(struct thread_struct *prev) {} #endif extern void enable_kernel_fp(void); diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index 662c6dd..a107f4a 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -432,12 +432,6 @@ BEGIN_FTR_SECTION std r24,THREAD_VRSAVE(r3) END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC) #endif /* CONFIG_ALTIVEC */ -#ifdef CONFIG_PPC64 -BEGIN_FTR_SECTION - mfspr r25,SPRN_DSCR - std r25,THREAD_DSCR(r3) -END_FTR_SECTION_IFSET(CPU_FTR_DSCR) -#endif and. r0,r0,r22 beq+ 1f andc r22,r22,r0 diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index e247898..8d2065e 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -771,15 +771,15 @@ struct task_struct *__switch_to(struct task_struct *prev, WARN_ON(!irqs_disabled()); - /* Back up the TAR across context switches. + /* Back up the TAR and DSCR across context switches. * Note that the TAR is not available for use in the kernel. (To * provide this, the TAR should be backed up/restored on exception * entry/exit instead, and be in pt_regs. FIXME, this should be in * pt_regs anyway (for debug).) - * Save the TAR here before we do treclaim/trecheckpoint as these - * will change the TAR. + * Save the TAR and DSCR here before we do treclaim/trecheckpoint as + * these will change them. */ - save_tar(&prev->thread); + save_early_sprs(&prev->thread); __switch_to_tm(prev);
Correct the DSCR SPR becoming temporarily corrupted when a task is context switched when within a transaction. It is corrected when the transaction is aborted (which will happen after a context switch) but if the task has suspended (TSUSPEND) the transaction the incorrect value can be seen. The problem is caused by saving a thread's DSCR after it has already been reverted to the CPU's default value: __switch_to() calls __switch_to_tm() which calls tm_reclaim_task() which calls tm_reclaim_thread() which calls tm_reclaim() where the DSCR is reset __switch_to() calls _switch _switch() saves the DSCR to thread.dscr The fix is to treat the DSCR similarly to the TAR and save it early in __switch_to(). The program below will expose the problem: #include <inttypes.h> #include <stdio.h> #include <stdlib.h> #include <assert.h> #include <asm/tm.h> #define TBEGIN ".long 0x7C00051D ;" #define TEND ".long 0x7C00055D ;" #define TCHECK ".long 0x7C00059C ;" #define TSUSPEND ".long 0x7C0005DD ;" #define TRESUME ".long 0x7C2005DD ;" #define SPRN_TEXASR 0x82 #define SPRN_DSCR 0x03 int main(void) { uint64_t i = 0, rv, dscr1 = 1, dscr2, texasr; for (;;) { rv = 1; asm __volatile__ ( "ld 3, %[dscr1];" "mtspr %[sprn_dscr], 3;" TBEGIN "beq 1f;" TSUSPEND "2: ;" TCHECK "bc 4, 0, 2b;" "mfspr 3, %[sprn_dscr];" "std 3, %[dscr2];" "mfspr 3, %[sprn_texasr];" "std 3, %[texasr];" TRESUME TEND "li %[rv], 0;" "1: ;" : [rv]"=r"(rv), [dscr2]"=m"(dscr2), [texasr]"=m"(texasr) : [dscr1]"m"(dscr1) , [sprn_dscr]"i"(SPRN_DSCR), [sprn_texasr]"i"(SPRN_TEXASR) : "memory", "r3" ); assert(rv); if ((texasr >> 56) == TM_CAUSE_RESCHED) { putchar('!'); fflush(stdout); i++; } else { putchar('.'); fflush(stdout); } if (dscr2 != dscr1) { printf("\n==== DSCR incorrect: 0x%lx (expecting 0x%lx)\n", dscr2, dscr1); exit(EXIT_FAILURE); } if (i > 10) { printf("\n==== DSCR TM context switching seems OK.\n"); exit(EXIT_SUCCESS); } } } Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> --- arch/powerpc/include/asm/switch_to.h | 6 ++++-- arch/powerpc/kernel/entry_64.S | 6 ------ arch/powerpc/kernel/process.c | 8 ++++---- 3 files changed, 8 insertions(+), 12 deletions(-)