Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2009-10-08 at 21:22 +0200, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:05:
> >
> > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > > 8xx sometimes need to load a invalid/non-present TLBs in
> > > it DTLB asm handler.
> > > These must be invalidated separaly as linux mm don't.
> >
> > not sure about the dsisr test here, what is the point ?
> 
> Without this patch I get about twice as many DTLB errors( on 2.4)

Ok, so it is useful... I would have thought that invalidating a TLB
entry that just caused a fault mostly be a nop.. well, the tlbie on 8xx
ignores the ASID so maybe it's invalidating next door process entries
but even that doesn't sound right. The TLB is so tiny on these things...

Oh well, as I said, something else to look at more closely.

> I have also noted that all my dcbst DTLB has the store bit set:
>   trap:300 address:10030b8c, dar:10030b8c,err:42000000 dcbst
> 
> Thare are comments in the kernel that dcbst wrongly
> generates TLB Errors with store set on 8xx. Is this really so?
> Should dcbst always trap as a load?

Architecturally it should, that's a known 8xx core bug.

Cheers,
Ben.

>  Jocke
> 
> >
> > Cheers,
> > Ben.
> >
> > > ---
> > >  arch/powerpc/mm/fault.c |    8 +++++++-
> > >  1 files changed, 7 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > > index 7699394..72941c7 100644
> > > --- a/arch/powerpc/mm/fault.c
> > > +++ b/arch/powerpc/mm/fault.c
> > > @@ -39,7 +39,7 @@
> > >  #include <asm/uaccess.h>
> > >  #include <asm/tlbflush.h>
> > >  #include <asm/siginfo.h>
> > > -
> > > +#include <mm/mmu_decl.h>
> > >
> > >  #ifdef CONFIG_KPROBES
> > >  static inline int notify_page_fault(struct pt_regs *regs)
> > > @@ -243,6 +243,12 @@ good_area:
> > >        goto bad_area;
> > >  #endif /* CONFIG_6xx */
> > >  #if defined(CONFIG_8xx)
> > > +   /* 8xx sometimes need to load a invalid/non-present TLBs.
> > > +    * These must be invalidated separately as linux mm don't.
> > > +    */
> > > +   if (error_code & 0x40000000) /* no translation? */
> > > +      _tlbil_va(address);
> > > +
> > >          /* The MPC8xx seems to always set 0x80000000, which is
> > >           * "undefined".  Of those that can be set, this is the only
> > >           * one which seems bad.
> >
> >
> >
> >