Patchwork [4/8] 8xx: Fixup DAR from buggy dcbX instructions.

login
register
mail settings
Submitter Joakim Tjernlund
Date Oct. 11, 2009, 4:35 p.m.
Message ID <1255278912-8042-5-git-send-email-Joakim.Tjernlund@transmode.se>
Download mbox | patch
Permalink /patch/35706/
State Superseded
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Joakim Tjernlund - Oct. 11, 2009, 4:35 p.m.
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(-)
Scott Wood - Oct. 14, 2009, 5:20 p.m.
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
Joakim Tjernlund - Oct. 14, 2009, 7:05 p.m.
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.
Scott Wood - Oct. 14, 2009, 7:23 p.m.
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
Joakim Tjernlund - Oct. 14, 2009, 8:03 p.m.
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.
Scott Wood - Oct. 14, 2009, 8:22 p.m.
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
Joakim Tjernlund - Oct. 14, 2009, 9:10 p.m.
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?
Scott Wood - Oct. 14, 2009, 9:14 p.m.
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
Benjamin Herrenschmidt - Oct. 14, 2009, 9:17 p.m.
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.
Joakim Tjernlund - Oct. 14, 2009, 9:41 p.m.
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
Benjamin Herrenschmidt - Oct. 14, 2009, 9:52 p.m.
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.
Joakim Tjernlund - Oct. 14, 2009, 10:09 p.m.
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 :):)

Patch

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