Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite

[Joakim Tjernlund <joakim.tjernlund@transmode.se>]

>
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 01/10/2009 00:35:59:
> >
> >
> > > > Had a look at linus tree and there is something I don't understand.
> > > > Your fix, e0908085fc2391c85b85fb814ae1df377c8e0dcb, fixes a problem
> > > > that was introduced by 8d30c14cab30d405a05f2aaceda1e9ad57800f36 but
> > > > 8d30c14cab30d405a05f2aaceda1e9ad57800f36 was included in .31 and .31
> > > > works and top of tree does not, how can that be?
> > > >
> > > > To me it seems more likely that some mm change introduced between .31 and
> > > > top of tree is the culprit.
> > > > My patch addresses the problem described in the comment:
> > > > /* On 8xx, cache control instructions (particularly
> > > >  * "dcbst" from flush_dcache_icache) fault as write
> > > >  * operation if there is an unpopulated TLB entry
> > > >  * for the address in question. To workaround that,
> > > >  * we invalidate the TLB here, thus avoiding dcbst
> > > >  * misbehaviour.
> > > >  */
> > > > Now you are using this old fix to paper over some other bug or so I think.
> > >
> > > There is something fishy with the TLB status, looking into the mpc862 manual I
> > > don't see how it can work reliably. Need to dwell some more.
> > > Anyhow, I have incorporated some of my findings into a new patch,
> > > perhaps this will be the golden one?
> >
> > Well, I still wonder about what whole unpopulated entry business.
> >
> > >From what I can see, the TLB miss code will check _PAGE_PRESENT, and
> > when not set, it will -still- insert something into the TLB (unlike
> > all other CPU types that go straight to data access faults from there).
> >
> > So we end up populating with those unpopulated entries that will then
> > cause us to take a DSI (or ISI) instead of a TLB miss the next time
> > around ... and so we need to remove them once we do that, no ? IE. Once
> > we have actually put a valid PTE in.
> >
> > At least that's my understanding and why I believe we should always have
> > tlbil_va() in set_pte() and ptep_set_access_flags(), which boils down
> > in putting it into the 2 "filter" functions in the new code.
> >
> > Well.. actually, the ptep_set_access_flags() will already do a
> > flush_tlb_page_nohash() when the PTE is changed. So I suppose all we
> > really need here would be in set_pte_filter(), to do the same if we are
> > writing a PTE that has _PAGE_PRESENT, at least on 8xx.
> >
> > But just unconditionally doing a tlbil_va() in both the filter functions
> > shouldn't harm and also fix the problem, though Rex seems to indicate
> > that is not the case.
> >
> > Now, we -might- have something else broken on 8xx, hard to tell. You may
> > want to try to bisect, adding back the removed tlbil_va() as you go
> > backward, between .31 and upstream...
>
> Found something odd, perhaps Rex can test?
> I tested this on my old 2.4 and it worked well.

That was not quite correct, sorry.
Try this instead:

>From c5f1a70561730b8a443f7081fbd9c5b023147043 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Fri, 2 Oct 2009 14:59:21 +0200
Subject: [PATCH] powerpc, 8xx: DTLB Error must check for more errors.

DataTLBError currently does:
 if ((err & 0x02000000) == 0)
    DSI();
This won't handle a store with no valid translation.
Change this to
 if ((err & 0x48000000) != 0)
    DSI();
that is, branch to DSI if either !permission or
!translation.
---
 arch/powerpc/kernel/head_8xx.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 52ff8c5..118bb05 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -472,8 +472,8 @@ DataTLBError:
 	/* First, make sure this was a store operation.
 	*/
 	mfspr	r10, SPRN_DSISR
-	andis.	r11, r10, 0x0200	/* If set, indicates store op */
-	beq	2f
+	andis.	r11, r10, 0x4800 /* no translation, no permission. */
+	bne	2f	/* branch if either is set */

 	/* The EA of a data TLB miss is automatically stored in the MD_EPN
 	 * register.  The EA of a data TLB error is automatically stored in
--
1.6.4.4