[Cbe-oss-dev] [PATCH] 64K page support for kexec

[Luke Browning]

On Tue, 2007-04-24 at 07:50 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-04-23 at 14:53 -0300, Luke Browning wrote:
> > This patch fixes a couple of kexec problems related to 64K page 
> > support in the kernel.  kexec issues a tlbie for each pte.  The 
> > parameters for the tlbie are the page size and the virtual address.
> > Support was missing for the computation of these two parameters
> > for 64K pages.  This patch adds that support.  
> > 
> > Signed-off-by: Luke Browning <lukebrowning at us.ibm.com>
> 
> Acked-by: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> 
> BTW. Your mailer is still mangling patches.... (wrapping them). Can't
> you disable word wrap ? If you're using evolution, make sure to use the
> "preformat" style, and do an "insert text file", that should keep
> patches intact.

I am using evolution.  where is the "preformat" option?

> A couple of minor things that shouldn't block accepting the patch:
> 
> > -static unsigned long slot2va(unsigned long hpte_v, unsigned long slot)
> > +static unsigned long slot2va(unsigned long hpte_v, unsigned long slot,
> > int psize)
> >  {
> >  	unsigned long avpn = HPTE_V_AVPN_VAL(hpte_v);
> >  	unsigned long va;
> >  
> >  	va = avpn << 23;
> >  
> > -	if (! (hpte_v & HPTE_V_LARGE)) {
> > +	if (psize != MMU_PAGE_16M) {
> >  		unsigned long vpi, pteg;
> 
> Maybe should be a test wether page shift > 23 ? One day we might
> actually use the 16G pages option too :-) Though that would mean using
> 1T segment, which means a different hashing too... that's something we
> haven't looked properly into yet. (We have had some patches for some
> times but there are issues we haven't fully solved yet).

Might be better to wait on this.  

> > +	if (pa & LP_MASK(0)) {
> > +		for (i = 0; i < LP_BITS; i++) {
> > +			if ((pa & LP_MASK(i+1)) == LP_MASK(i+1))
> > +				break;
> > +		}
> > +		penc = LP_MASK(i+1) >> LP_SHIFT;
> > +		for (i = MMU_PAGE_COUNT - 1; i > 0; i--) {
> > +			if (!mmu_psize_defs[i].shift)
> > +				continue;
> > +			if (penc == mmu_psize_defs[i].penc)
> > +				break;
> > +		}
> > +	}
> > +	else {
> > +		i = MMU_PAGE_16M;
> > +	}
> > +
> > +	return i;
> > +}
> 
> So you didn't like my idea of using a single loop ? Oh well, that should
> do I suppose. One minor coding style nit, I would have preferred
> something like
> 
> 	if (!(pa & LP_MASK(0)))
> 		return MMU_PAGE_16M;
> 
> 	for (...)
> 
> To limit the right shift of the whole function, but that's a detail.
> 
> >  		if (hpte_v & HPTE_V_VALID) {
> > +			if (!(hpte_v & HPTE_V_LARGE))
> > +				psize = MMU_PAGE_4K;
> > +			else
> > +				psize = hpte_decode_lpsize(hptep->r);
> 
> Another minor coding style nit, but maybe it would have been nicer to
> have the 4K page also dealt within  hpte_decode_lpsize() ? Since the
> function is likely to inline it, that will make no difference in
> generated code but keeps the logic for getting the psize completely
> within a single function...
> > +			vpn = slot2va(hpte_v, slot, psize);
> >  			hptep->v = 0;
> > -			__tlbie(slot2va(hpte_v, slot), MMU_PAGE_4K);
> > +			__tlbie(vpn, psize);
> >  		}
> >  	}
> 
> Cheers,
> Ben.
> 

I was pressed for time and wanted to get the patch into the hands of the
developers trying to generate the crash file. There are still other
problems occurring, but the dump kernel boots now.  I will look at your
coding suggestions now.  

Thanks for the feedback.

Luke