Re: [PATCH] powerpc/book3e-64: use a separate TLB handler when linear map is bolted

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

On Fri, 2011-06-17 at 11:32 -0500, Scott Wood wrote:
> On Fri, 17 Jun 2011 12:00:50 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Does this completely replace your previous series of 7 patches ? (IE.
> > Should I ditch them in patchwork ?) Or does it apply on top of them ?
> 
> It replaces them.
> 
> > >  #define SET_IVOR(vector_number, vector_offset)	\
> > > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> > > index a73668a..9d9e444 100644
> > > --- a/arch/powerpc/include/asm/mmu_context.h
> > > +++ b/arch/powerpc/include/asm/mmu_context.h
> > > @@ -54,6 +54,7 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
> > >  	/* 64-bit Book3E keeps track of current PGD in the PACA */
> > >  #ifdef CONFIG_PPC_BOOK3E_64
> > >  	get_paca()->pgd = next->pgd;
> > > +	get_paca()->extlb[0][EX_TLB_PGD / 8] = (unsigned long)next->pgd;
> > >  #endif
> > >  	/* Nothing else to do if we aren't actually switching */
> > >  	if (prev == next)
> > > @@ -110,6 +111,7 @@ static inline void enter_lazy_tlb(struct mm_struct *mm,
> > >  	/* 64-bit Book3E keeps track of current PGD in the PACA */
> > >  #ifdef CONFIG_PPC_BOOK3E_64
> > >  	get_paca()->pgd = NULL;
> > > +	get_paca()->extlb[0][EX_TLB_PGD / 8] = 0;
> > >  #endif
> > >  }
> > 
> > Why do you keep a copy of the pgd there since it's in the PACA already
> > and you have r13 setup in your handlers ?
> 
> This cache line is already being used by the TLB miss handler.  The cache
> line containing paca->pgd isn't otherwise used there.

Can't you just re-org the PACA instead ? (with a comment) ? Or at least
if you want to keep it that way, put the cache line explanation in a
comment somewhere.

> > > +.macro tlb_prolog_bolted addr
> > > +	mtspr	SPRN_SPRG_TLB_SCRATCH,r13
> > > +	mfspr	r13,SPRN_SPRG_PACA
> > > +	std	r10,PACA_EXTLB+EX_TLB_R10(r13)
> > > +	mfcr	r10
> > > +	std	r11,PACA_EXTLB+EX_TLB_R11(r13)
> > > +	mfspr	r11,SPRN_SPRG_TLB_SCRATCH
> > 
> > Do you need that ? Can't you leave r13 in scratch the whole way and
> > just pop it out in the error case when branching to DSI/ISI ? The only
> > thing is that TLB_SCRATCH needs to be saved/restored by
> > crit/debug/mcheck but thats worth saving cycles in the TLB miss handler
> > no ?
> 
> Sounds right... I'll try it.
> 
> > > +	mfspr	r11,SPRN_ESR
> > > +
> > > +	srdi	r15,r16,60		/* get region */
> > > +	rldicl.	r10,r16,64-PGTABLE_EADDR_SIZE,PGTABLE_EADDR_SIZE+4
> > > +	bne-	dtlb_miss_fault_bolted
> > 
> > Ok so I'm not familiar with your pipeline here, but wouldn't it be
> > better to move the srdi to after the bne above and make it srdi., thus
> > avoiding the compare below ?
> > 
> > > +	rlwinm	r10,r11,32-19,27,27
> > > +	rlwimi	r10,r11,32-16,19,19
> > > +	cmpwi	r15,0
> > > +	ori	r10,r10,_PAGE_PRESENT
> > > +	oris	r11,r10,_PAGE_ACCESSED@h
> 
> The cmpwi will run in parallel with the rlwimi above on e5500, but
> changing it shouldn't hurt, and it's one less instruction to sit in the
> icache...

Cheers,
Ben.