Re: [PATCH -V5 08/25] powerpc: Decode the pte-lp-encoding bits correctly.

[David Gibson <dwg@au1.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 5036 bytes --]

On Wed, Apr 10, 2013 at 01:41:16PM +0530, Aneesh Kumar K.V wrote:
> David Gibson <dwg@au1.ibm.com> writes:
> 
> > On Thu, Apr 04, 2013 at 11:27:46AM +0530, Aneesh Kumar K.V wrote:
> >> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> >> 
> >> We look at both the segment base page size and actual page size and store
> >> the pte-lp-encodings in an array per base page size.
> >> 
> >> We also update all relevant functions to take actual page size argument
> >> so that we can use the correct PTE LP encoding in HPTE. This should also
> >> get the basic Multiple Page Size per Segment (MPSS) support. This is needed
> >> to enable THP on ppc64.
> >> 
> 
> ....
> 
> >> +static inline int hpte_actual_psize(struct hash_pte *hptep, int psize)
> >> +{
> >> +	int i, shift;
> >> +	unsigned int mask;
> >> +	/* Look at the 8 bit LP value */
> >> +	unsigned int lp = (hptep->r >> LP_SHIFT) & ((1 << LP_BITS) - 1);
> >> +
> >> +	if (!(hptep->v & HPTE_V_VALID))
> >> +		return -1;
> >
> > Folding the validity check into the size check seems confusing to me.
> 
> We do end up with invalid hpte with which we call
> hpte_actual_psize. So that check is needed. I can either move to caller,
> but then i will have to replicate it in all the call sites.
> 
> 
> >> +	/* First check if it is large page */
> >> +	if (!(hptep->v & HPTE_V_LARGE))
> >> +		return MMU_PAGE_4K;
> >> +
> >> +	/* start from 1 ignoring MMU_PAGE_4K */
> >> +	for (i = 1; i < MMU_PAGE_COUNT; i++) {
> >> +		/* valid entries have a shift value */
> >> +		if (!mmu_psize_defs[i].shift)
> >> +			continue;
> >
> > Isn't this check redundant with the one below?
> 
> Yes. I guess we can safely assume that if penc is valid then we do
> support that specific large page.
> 
> I will drop this and keep the penc check. That is more correct check
> 
> >> +		/* invalid penc */
> >> +		if (mmu_psize_defs[psize].penc[i] == -1)
> >> +			continue;
> >> +		/*
> >> +		 * encoding bits per actual page size
> >> +		 *        PTE LP     actual page size
> >> +		 *    rrrr rrrz		>=8KB
> >> +		 *    rrrr rrzz		>=16KB
> >> +		 *    rrrr rzzz		>=32KB
> >> +		 *    rrrr zzzz		>=64KB
> >> +		 * .......
> >> +		 */
> >> +		shift = mmu_psize_defs[i].shift - LP_SHIFT;
> >> +		if (shift > LP_BITS)
> >> +			shift = LP_BITS;
> >> +		mask = (1 << shift) - 1;
> >> +		if ((lp & mask) == mmu_psize_defs[psize].penc[i])
> >> +			return i;
> >> +	}
> >
> > Shouldn't we have a BUG() or something here.  If we get here we've
> > somehow created a PTE with LP bits we can't interpret, yes?
> 
> I don't know. Is BUG() the right thing to do ? 

Well, it's a situation that should never occur, and it's not clear
what we can do to fix it if it does, so, yeah, I think BUG() is appropriate.

> >> +	return -1;
> >> +}
> >> +
> >>  static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> >>  				 unsigned long vpn, int psize, int ssize,
> >>  				 int local)
> >> @@ -251,6 +294,7 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> >>  	struct hash_pte *hptep = htab_address + slot;
> >>  	unsigned long hpte_v, want_v;
> >>  	int ret = 0;
> >> +	int actual_psize;
> >>  
> >>  	want_v = hpte_encode_avpn(vpn, psize, ssize);
> >>  
> >> @@ -260,9 +304,13 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> >>  	native_lock_hpte(hptep);
> >>  
> >>  	hpte_v = hptep->v;
> >> -
> >> +	actual_psize = hpte_actual_psize(hptep, psize);
> >> +	if (actual_psize < 0) {
> >> +		native_unlock_hpte(hptep);
> >> +		return -1;
> >> +	}
> >
> > Wouldn't it make more sense to only do the psize lookup once you've
> > found a matching hpte?
> 
> But we need to do psize lookup even if V_COMPARE fail, because we want
> to do tlbie in both the case.

Ah, yes.  Sorry, misunderstood what this code was doing.

[snip]
> >> @@ -388,19 +444,26 @@ static void hpte_decode(struct hash_pte *hpte, unsigned long slot,
> >>  		penc = LP_MASK(i+1) >> LP_SHIFT;
> >>  		for (size = 0; size < MMU_PAGE_COUNT; size++) {
> >
> >>  
> >> -			/* 4K pages are not represented by LP */
> >> -			if (size == MMU_PAGE_4K)
> >> -				continue;
> >> -
> >>  			/* valid entries have a shift value */
> >>  			if (!mmu_psize_defs[size].shift)
> >>  				continue;
> >> +			for (a_size = 0; a_size < MMU_PAGE_COUNT; a_size++) {
> >
> > Can't you resize hpte_actual_psize() here instead of recoding the
> > lookup?
> 
> I thought about that, but re-coding avoided some repeated check. But
> then, if I follow your review comments of avoiding hpte valid check etc, may
> be I can reuse the hpte_actual_psize. Will try this. 

hpte_decode() is only used in the kexec() path so some repeated simple
tests don't really matter.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]