| Message ID | 1255278912-8042-5-git-send-email-Joakim.Tjernlund@transmode.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
On Sun, Oct 11, 2009 at 06:35:08PM +0200, Joakim Tjernlund wrote: > This is an assembler version to fixup DAR not being set > by dcbX, icbi instructions. There are two versions, one > uses selfmodifing code, the other uses a > jump table but is much bigger(default). > --- > arch/powerpc/kernel/head_8xx.S | 146 +++++++++++++++++++++++++++++++++++++++- > 1 files changed, 145 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S > index 093176c..9839e79 100644 > --- a/arch/powerpc/kernel/head_8xx.S > +++ b/arch/powerpc/kernel/head_8xx.S > @@ -494,7 +494,8 @@ DataTLBError: > > mfspr r10, SPRN_DAR > cmpwi cr0, r10, 0x00f0 > - beq- 2f /* must be a buggy dcbX, icbi insn. */ > + beq- FixDAR /* must be a buggy dcbX, icbi insn. */ > +DARFix: /* Return from dcbx instruction bug workaround, r10 holds value of DAR */ Both FixDAR and DARFix? Could we make the labels a little clearer? > +/* This is the procedure to calculate the data EA for buggy dcbx,dcbi instructions > + * by decoding the registers used by the dcbx instruction and adding them. > + * DAR is set to the calculated address and r10 also holds the EA on exit. > + */ How often does this happen? Could we just do it in C code after saving all the registers, and avoid the self modifying stuff (or the big switch statement equivalent)? -Scott
Scott Wood <scottwood@freescale.com> wrote on 14/10/2009 19:20:03: > > On Sun, Oct 11, 2009 at 06:35:08PM +0200, Joakim Tjernlund wrote: > > This is an assembler version to fixup DAR not being set > > by dcbX, icbi instructions. There are two versions, one > > uses selfmodifing code, the other uses a > > jump table but is much bigger(default). > > --- > > arch/powerpc/kernel/head_8xx.S | 146 +++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 145 insertions(+), 1 deletions(-) > > > > diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S > > index 093176c..9839e79 100644 > > --- a/arch/powerpc/kernel/head_8xx.S > > +++ b/arch/powerpc/kernel/head_8xx.S > > @@ -494,7 +494,8 @@ DataTLBError: > > > > mfspr r10, SPRN_DAR > > cmpwi cr0, r10, 0x00f0 > > - beq- 2f /* must be a buggy dcbX, icbi insn. */ > > + beq- FixDAR /* must be a buggy dcbX, icbi insn. */ > > +DARFix: /* Return from dcbx instruction bug workaround, r10 holds value of DAR */ > > Both FixDAR and DARFix? Could we make the labels a little clearer? Yes, need to come up with better names :) > > > +/* This is the procedure to calculate the data EA for buggy dcbx,dcbi instructions > > + * by decoding the registers used by the dcbx instruction and adding them. > > + * DAR is set to the calculated address and r10 also holds the EA on exit. > > + */ > > How often does this happen? Could we just do it in C code after saving all > the registers, and avoid the self modifying stuff (or the big switch > statement equivalent)? I had some problems with the C-version. I got lots of extra TLB errors for the same address so I am not confident it will work in the long run. BTW, you could add a test and printk in do_page_fault on address 0x000000f0. if that ever hits there is a problem with dcbX fixup.
Joakim Tjernlund wrote: > BTW, you could add a test and printk in do_page_fault on address 0x000000f0. > if that ever hits there is a problem with dcbX fixup. It doesn't get any 0xf0 faults. FWIW, I'm not seeing the segfault any more, but I still get the lockup. -Scott
Scott Wood <scottwood@freescale.com> wrote on 14/10/2009 21:23:02: > Joakim Tjernlund wrote: > > BTW, you could add a test and printk in do_page_fault on address 0x000000f0. > > if that ever hits there is a problem with dcbX fixup. > > It doesn't get any 0xf0 faults. > > FWIW, I'm not seeing the segfault any more, but I still get the lockup. Have you reverted 8xx: start using dcbX instructions in various copy routines ? After that you could stick a b DataAccess directly in the DTLB error handler to skip and dcbX fixups.
Joakim Tjernlund wrote: > Scott Wood <scottwood@freescale.com> wrote on 14/10/2009 21:23:02: >> Joakim Tjernlund wrote: >>> BTW, you could add a test and printk in do_page_fault on address 0x000000f0. >>> if that ever hits there is a problem with dcbX fixup. >> It doesn't get any 0xf0 faults. >> >> FWIW, I'm not seeing the segfault any more, but I still get the lockup. > > Have you reverted > 8xx: start using dcbX instructions in various copy routines ? > > After that you could stick a > b DataAccess > > directly in the DTLB error handler to skip and dcbX fixups. With that, I don't see the hard lockup, but things get stuck during bootup with everything idle. I see this even if I revert everything but the "invalidate non present TLBs" patch, and I was seeing similar things sometimes with the other tlbil_va hacks. I think there's something else going on in the 2.6 8xx code that needs to be fixed before we can tell what the impact of these patches is. I'll look into it. -Scott
Scott Wood <scottwood@freescale.com> wrote on 14/10/2009 22:22:25: > > Joakim Tjernlund wrote: > > Scott Wood <scottwood@freescale.com> wrote on 14/10/2009 21:23:02: > >> Joakim Tjernlund wrote: > >>> BTW, you could add a test and printk in do_page_fault on address 0x000000f0. > >>> if that ever hits there is a problem with dcbX fixup. > >> It doesn't get any 0xf0 faults. > >> > >> FWIW, I'm not seeing the segfault any more, but I still get the lockup. > > > > Have you reverted > > 8xx: start using dcbX instructions in various copy routines ? > > > > After that you could stick a > > b DataAccess > > > > directly in the DTLB error handler to skip and dcbX fixups. > > With that, I don't see the hard lockup, but things get stuck during You needed both to loose the hard lockup? I would think it should be enough to revert the "various copy routines" stuff? I figure that these routines aren't working in 8xx for other reasons since they haven't been used on 8xx since at least early 2.4. > bootup with everything idle. I see this even if I revert everything but > the "invalidate non present TLBs" patch, and I was seeing similar things > sometimes with the other tlbil_va hacks. OK, something else is up. > > I think there's something else going on in the 2.6 8xx code that needs > to be fixed before we can tell what the impact of these patches is. > I'll look into it. Great because I am really out of ideas. Perhaps back down to 2.6.30 and test from there?
Joakim Tjernlund wrote: >> With that, I don't see the hard lockup, but things get stuck during > > You needed both to loose the hard lockup? I would think > it should be enough to revert the "various copy routines" stuff? No, but when I just reverted the patch and didn't change the TLB error handler, I got some other weirdness (assertion failure in some userspace program). It may have been coincidental, though. >> I think there's something else going on in the 2.6 8xx code that needs >> to be fixed before we can tell what the impact of these patches is. >> I'll look into it. > > Great because I am really out of ideas. Perhaps back down to 2.6.30 and test > from there? I think the last working version was a little older than that -- and it's quite possible that there was underlying badness even earlier that just recently got exposed. I think I want to just debug it and find out what's really going on. -Scott
On Wed, 2009-10-14 at 16:14 -0500, Scott Wood wrote: > > I think the last working version was a little older than that -- and it's quite > possible that there was underlying badness even earlier that just recently got > exposed. I think I want to just debug it and find out what's really going on. That would be good :-) I've been itching to do that but without HW it's not trivial :-) Cheers, Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 14/10/2009 23:17:09: > > On Wed, 2009-10-14 at 16:14 -0500, Scott Wood wrote: > > > > I think the last working version was a little older than that -- and it's quite > > possible that there was underlying badness even earlier that just recently got > > exposed. I think I want to just debug it and find out what's really going on. > > That would be good :-) > > I've been itching to do that but without HW it's not trivial :-) Meanwhile, how about the tlb asm you promised me? :) It will be a challenge I think since you only have 2 GPRs I guess it would be possible to stash yet another reg since it will fit in the cache line already used by the TLB handlers. Jocke
On Wed, 2009-10-14 at 23:41 +0200, Joakim Tjernlund wrote: > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 14/10/2009 23:17:09: > > > > On Wed, 2009-10-14 at 16:14 -0500, Scott Wood wrote: > > > > > > I think the last working version was a little older than that -- and it's quite > > > possible that there was underlying badness even earlier that just recently got > > > exposed. I think I want to just debug it and find out what's really going on. > > > > That would be good :-) > > > > I've been itching to do that but without HW it's not trivial :-) > > Meanwhile, how about the tlb asm you promised me? :) > It will be a challenge I think since you only have 2 GPRs > I guess it would be possible to stash yet another reg since it > will fit in the cache line already used by the TLB handlers. Let's just get it working first :-) Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 14/10/2009 23:52:10: > > On Wed, 2009-10-14 at 23:41 +0200, Joakim Tjernlund wrote: > > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 14/10/2009 23:17:09: > > > > > > On Wed, 2009-10-14 at 16:14 -0500, Scott Wood wrote: > > > > > > > > I think the last working version was a little older than that -- and it's quite > > > > possible that there was underlying badness even earlier that just recently got > > > > exposed. I think I want to just debug it and find out what's really going on. > > > > > > That would be good :-) > > > > > > I've been itching to do that but without HW it's not trivial :-) > > > > Meanwhile, how about the tlb asm you promised me? :) > > It will be a challenge I think since you only have 2 GPRs > > I guess it would be possible to stash yet another reg since it > > will fit in the cache line already used by the TLB handlers. > > Let's just get it working first :-) Chicken :):)
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S index 093176c..9839e79 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -494,7 +494,8 @@ DataTLBError: mfspr r10, SPRN_DAR cmpwi cr0, r10, 0x00f0 - beq- 2f /* must be a buggy dcbX, icbi insn. */ + beq- FixDAR /* must be a buggy dcbX, icbi insn. */ +DARFix: /* Return from dcbx instruction bug workaround, r10 holds value of DAR */ mfspr r11, SPRN_DSISR andis. r11, r11, 0x4800 /* !translation or protection */ @@ -604,6 +605,149 @@ DataTLBError: . = 0x2000 +/* This is the procedure to calculate the data EA for buggy dcbx,dcbi instructions + * by decoding the registers used by the dcbx instruction and adding them. + * DAR is set to the calculated address and r10 also holds the EA on exit. + */ +#define NO_SELF_MODIFYING_CODE /* define if you don't want to use self modifying code */ + nop /* A few nops to make the modified_instr: space below cache line aligned */ + nop +139: /* fetch instruction from userspace memory */ + DO_8xx_CPU6(0x3780, r3) + mtspr SPRN_MD_EPN, r10 + mfspr r11, SPRN_M_TWB /* Get level 1 table entry address */ + lwz r11, 0(r11) /* Get the level 1 entry */ + tophys (r11, r11) + DO_8xx_CPU6(0x3b80, r3) + mtspr SPRN_MD_TWC, r11 /* Load pte table base address */ + mfspr r11, SPRN_MD_TWC /* ....and get the pte address */ + lwz r11, 0(r11) /* Get the pte */ + /* concat physical page address(r11) and page offset(r10) */ + rlwimi r11, r10, 0, 20, 31 + b 140f +FixDAR: /* Entry point for dcbx workaround. */ + /* fetch instruction from memory. */ + mfspr r10, SPRN_SRR0 + andis. r11, r10, 0x8000 + tophys (r11, r10) + beq- 139b /* Branch if user space address */ +140: lwz r11,0(r11) +#ifdef CONFIG_8xx_CPU6 + lwz r3, 8(r0) /* restore r3 from memory */ +#endif +#ifndef NO_SELF_MODIFYING_CODE + andis. r10,r11,0x1f /* test if reg RA is r0 */ + li r10,modified_instr@l + dcbtst r0,r10 /* touch for store */ + rlwinm r11,r11,0,0,20 /* Zero lower 10 bits */ + oris r11,r11,640 /* Transform instr. to a "add r10,RA,RB" */ + ori r11,r11,532 + stw r11,0(r10) /* store add/and instruction */ + dcbf 0,r10 /* flush new instr. to memory. */ + icbi 0,r10 /* invalidate instr. cache line */ + lwz r11, 4(r0) /* restore r11 from memory */ + mfspr r10, SPRN_M_TW /* restore r10 from M_TW */ + isync /* Wait until new instr is loaded from memory */ +modified_instr: + .space 4 /* this is where the add/and instr. is stored */ + bne+ 143f + subf r10,r0,r10 /* r10=r10-r0, only if reg RA is r0 */ +143: mtdar r10 /* store faulting EA in DAR */ + b DARFix /* Go back to normal TLB handling */ +#else + mfctr r10 + mtdar r10 /* save ctr reg in DAR */ + rlwinm r10, r11, 24, 24, 28 /* offset into jump table for reg RB */ + addi r10, r10, 150f@l /* add start of table */ + mtctr r10 /* load ctr with jump address */ + xor r10, r10, r10 /* sum starts at zero */ + bctr /* jump into table */ +150: + add r10, r10, r0 + b 151f + add r10, r10, r1 + b 151f + add r10, r10, r2 + b 151f + add r10, r10, r3 + b 151f + add r10, r10, r4 + b 151f + add r10, r10, r5 + b 151f + add r10, r10, r6 + b 151f + add r10, r10, r7 + b 151f + add r10, r10, r8 + b 151f + add r10, r10, r9 + b 151f + add r10, r10, r10 + b 151f + add r10, r10, r11 + b 151f + add r10, r10, r12 + b 151f + add r10, r10, r13 + b 151f + add r10, r10, r14 + b 151f + add r10, r10, r15 + b 151f + add r10, r10, r16 + b 151f + add r10, r10, r17 + b 151f + add r10, r10, r18 + b 151f + add r10, r10, r19 + b 151f + mtctr r11 /* r10 needs special handling */ + b 154f + mtctr r11 /* r11 needs special handling */ + b 153f + add r10, r10, r22 + b 151f + add r10, r10, r23 + b 151f + add r10, r10, r24 + b 151f + add r10, r10, r25 + b 151f + add r10, r10, r25 + b 151f + add r10, r10, r27 + b 151f + add r10, r10, r28 + b 151f + add r10, r10, r29 + b 151f + add r10, r10, r30 + b 151f + add r10, r10, r31 +151: + rlwinm. r11,r11,19,24,28 /* offset into jump table for reg RA */ + beq 152f /* if reg RA is zero, don't add it */ + addi r11, r11, 150b@l /* add start of table */ + mtctr r11 /* load ctr with jump address */ + rlwinm r11,r11,0,16,10 /* make sure we don't execute this more than once */ + bctr /* jump into table */ +152: + mfdar r11 + mtctr r11 /* restore ctr reg from DAR */ + mtdar r10 /* save fault EA to DAR */ + b DARFix /* Go back to normal TLB handling */ + + /* special handling for r10,r11 since these are modified already */ +153: lwz r11, 4(r0) /* load r11 from memory */ + b 155f +154: mfspr r11, SPRN_M_TW /* load r10 from M_TW */ +155: add r10, r10, r11 /* add it */ + mfctr r11 /* restore r11 */ + b 151b +#endif + .globl giveup_fpu giveup_fpu: blr