* [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() @ 2017-07-06 8:46 Oliver O'Halloran 2017-07-10 11:50 ` Michael Ellerman 2017-07-11 12:48 ` Michael Ellerman 0 siblings, 2 replies; 6+ messages in thread From: Oliver O'Halloran @ 2017-07-06 8:46 UTC (permalink / raw) To: linuxppc-dev; +Cc: Oliver O'Halloran The workaround for the CELL timebase bug does not correctly mark cr0 as being clobbered. This can result in GCC making some poor^W completely broken optimisations. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- arch/powerpc/include/asm/reg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h index 7e50e47375d6..a3b6575c7842 100644 --- a/arch/powerpc/include/asm/reg.h +++ b/arch/powerpc/include/asm/reg.h @@ -1303,7 +1303,7 @@ static inline void msr_check_and_clear(unsigned long bits) " .llong 0\n" \ ".previous" \ : "=r" (rval) \ - : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL)); \ + : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL) : "cr0"); \ rval;}) #else #define mftb() ({unsigned long rval; \ -- 2.9.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() 2017-07-06 8:46 [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() Oliver O'Halloran @ 2017-07-10 11:50 ` Michael Ellerman 2017-07-10 13:43 ` Segher Boessenkool 2017-07-11 4:02 ` Oliver 2017-07-11 12:48 ` Michael Ellerman 1 sibling, 2 replies; 6+ messages in thread From: Michael Ellerman @ 2017-07-10 11:50 UTC (permalink / raw) To: Oliver O'Halloran, linuxppc-dev; +Cc: Oliver O'Halloran Oliver O'Halloran <oohall@gmail.com> writes: > The workaround for the CELL timebase bug does not correctly mark cr0 as > being clobbered. This can result in GCC making some poor^W completely > broken optimisations. Fruit 'n oats, how did we never notice that? Luck I guess. Or subtle breakage that no one could pin down :E I'll tag it for stable. Your change log is not entirely fair, it's not GCC's fault, we changed register state without telling it, so it's on us :) I'll reword it a bit here. cheers > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h > index 7e50e47375d6..a3b6575c7842 100644 > --- a/arch/powerpc/include/asm/reg.h > +++ b/arch/powerpc/include/asm/reg.h > @@ -1303,7 +1303,7 @@ static inline void msr_check_and_clear(unsigned long bits) > " .llong 0\n" \ > ".previous" \ > : "=r" (rval) \ > - : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL)); \ > + : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL) : "cr0"); \ > rval;}) > #else > #define mftb() ({unsigned long rval; \ > -- > 2.9.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() 2017-07-10 11:50 ` Michael Ellerman @ 2017-07-10 13:43 ` Segher Boessenkool 2017-07-11 2:33 ` Michael Ellerman 2017-07-11 4:02 ` Oliver 1 sibling, 1 reply; 6+ messages in thread From: Segher Boessenkool @ 2017-07-10 13:43 UTC (permalink / raw) To: Michael Ellerman; +Cc: Oliver O'Halloran, linuxppc-dev Hi! On Mon, Jul 10, 2017 at 09:50:06PM +1000, Michael Ellerman wrote: > Oliver O'Halloran <oohall@gmail.com> writes: > > > The workaround for the CELL timebase bug does not correctly mark cr0 as > > being clobbered. This can result in GCC making some poor^W completely > > broken optimisations. > > Fruit 'n oats, how did we never notice that? Luck I guess. Or subtle > breakage that no one could pin down :E GCC does not use cr0 before it has used cr7, cr5, cr6, cr1 (unless some instruction _requires_ cr0, like record form ("dot") instructions, which until recently were disabled when targetting Cell), so it isn't too easy to hit the problem here. Maybe Oliver used a very new GCC? Or he was unlucky. > I'll tag it for stable. > > Your change log is not entirely fair, it's not GCC's fault, we changed > register state without telling it, so it's on us :) I'll reword it a > bit here. Yep, GCC works fine here, GIGO etc. It isn't feasible to make GCC warn for most inline asm problems, either (we do not parse the mnemonics in the asm; this is a fundamental part of the design; you must express all constraints as, well, constraints). Segher ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() 2017-07-10 13:43 ` Segher Boessenkool @ 2017-07-11 2:33 ` Michael Ellerman 0 siblings, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2017-07-11 2:33 UTC (permalink / raw) To: Segher Boessenkool; +Cc: Oliver O'Halloran, linuxppc-dev Segher Boessenkool <segher@kernel.crashing.org> writes: > Hi! > > On Mon, Jul 10, 2017 at 09:50:06PM +1000, Michael Ellerman wrote: >> Oliver O'Halloran <oohall@gmail.com> writes: >> >> > The workaround for the CELL timebase bug does not correctly mark cr0 as >> > being clobbered. This can result in GCC making some poor^W completely >> > broken optimisations. >> >> Fruit 'n oats, how did we never notice that? Luck I guess. Or subtle >> breakage that no one could pin down :E > > GCC does not use cr0 before it has used cr7, cr5, cr6, cr1 (unless some > instruction _requires_ cr0, like record form ("dot") instructions, which > until recently were disabled when targetting Cell), so it isn't too easy > to hit the problem here. Maybe Oliver used a very new GCC? Or he was > unlucky. Aha. I think he actually hit the bug elsewhere and then noticed it here also? And yeah I think it was a dot instruction that triggered it. >> I'll tag it for stable. >> >> Your change log is not entirely fair, it's not GCC's fault, we changed >> register state without telling it, so it's on us :) I'll reword it a >> bit here. > > Yep, GCC works fine here, GIGO etc. It isn't feasible to make GCC warn > for most inline asm problems, either (we do not parse the mnemonics in > the asm; this is a fundamental part of the design; you must express all > constraints as, well, constraints). Ack. It's a terrible terrible interface, but that's the price we pay for wanting to insert arbitrary bits of asm inside an otherwise compiled language :) When I get the time I might go through our inline asm and convert all the cmpwi x,z to cmpwi cr0,x,z, so at least the cr0 usage is a little more obvious. cheers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() 2017-07-10 11:50 ` Michael Ellerman 2017-07-10 13:43 ` Segher Boessenkool @ 2017-07-11 4:02 ` Oliver 1 sibling, 0 replies; 6+ messages in thread From: Oliver @ 2017-07-11 4:02 UTC (permalink / raw) To: Michael Ellerman; +Cc: linuxppc-dev On Mon, Jul 10, 2017 at 9:50 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: > Oliver O'Halloran <oohall@gmail.com> writes: > >> The workaround for the CELL timebase bug does not correctly mark cr0 as >> being clobbered. This can result in GCC making some poor^W completely >> broken optimisations. > > Fruit 'n oats, how did we never notice that? Luck I guess. Or subtle > breakage that no one could pin down :E Dumb luck probably. The workaround is inside a feature section which depends on CPU_FTR_CELL_TB_BUG so you would only ever see a problem when actually running on Cell (can't be that many people...). CR0 being volatile across function calls probably helped mask it too. > I'll tag it for stable. Good idea. > Your change log is not entirely fair, it's not GCC's fault, we changed > register state without telling it, so it's on us :) I'll reword it a > bit here. Yeah that was probably a bit harsh. > > cheers > >> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h >> index 7e50e47375d6..a3b6575c7842 100644 >> --- a/arch/powerpc/include/asm/reg.h >> +++ b/arch/powerpc/include/asm/reg.h >> @@ -1303,7 +1303,7 @@ static inline void msr_check_and_clear(unsigned long bits) >> " .llong 0\n" \ >> ".previous" \ >> : "=r" (rval) \ >> - : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL)); \ >> + : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL) : "cr0"); \ >> rval;}) >> #else >> #define mftb() ({unsigned long rval; \ >> -- >> 2.9.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: powerpc/asm: Mark cr0 as clobbered in mftb() 2017-07-06 8:46 [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() Oliver O'Halloran 2017-07-10 11:50 ` Michael Ellerman @ 2017-07-11 12:48 ` Michael Ellerman 1 sibling, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2017-07-11 12:48 UTC (permalink / raw) To: Oliver O'Halloran, linuxppc-dev; +Cc: Oliver O'Halloran On Thu, 2017-07-06 at 08:46:43 UTC, Oliver O'Halloran wrote: > The workaround for the CELL timebase bug does not correctly mark cr0 as > being clobbered. This can result in GCC making some poor^W completely > broken optimisations. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/2400fd822f467cb4c886c879d8ad99 cheers ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-11 12:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-06 8:46 [PATCH] powerpc/asm: Mark cr0 as clobbered in mftb() Oliver O'Halloran 2017-07-10 11:50 ` Michael Ellerman 2017-07-10 13:43 ` Segher Boessenkool 2017-07-11 2:33 ` Michael Ellerman 2017-07-11 4:02 ` Oliver 2017-07-11 12:48 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).