Re: [RFC PATCH -V2 08/21] powerpc: Decode the pte-lp-encoding bits correctly.

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Paul Mackerras <paulus@samba.org> writes:

> On Thu, Feb 21, 2013 at 10:17:15PM +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.
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>
> This needs more than 2 lines of patch description.  In fact what
> you're doing is adding general mixed page-size segment (MPSS)
> support.  Doing this should mean that you can also get rid of the
> MMU_PAGE_64K_AP value from the list in asm/mmu.h.
>

Can you elaborate on Admixed pages ? I was not able to find more info on
that. 


>>  struct mmu_psize_def
>>  {
>>  	unsigned int	shift;	/* number of bits */
>> -	unsigned int	penc;	/* HPTE encoding */
>> +	unsigned int	penc[MMU_PAGE_COUNT];	/* HPTE encoding */
>
> I guess this is reasonable, though adding space for 14 page size
> encodings seems a little bit over the top.  Also, you don't seem to
> have any way to indicate which encodings are valid, since 0 is a valid
> encoding.  Maybe you need to add a valid bit higher up to indicate
> which page sizes are valid.
>

how about penc = { [0 ... MMU_PAGE_COUNT] = -1 } ?, that would make sure
we set all the bits for invalid entries. 


>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 71d0c90..d2c9932 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1515,7 +1515,12 @@ static void kvmppc_add_seg_page_size(struct kvm_ppc_one_seg_page_size **sps,
>>  	(*sps)->page_shift = def->shift;
>>  	(*sps)->slb_enc = def->sllp;
>>  	(*sps)->enc[0].page_shift = def->shift;
>> -	(*sps)->enc[0].pte_enc = def->penc;
>> +	/*
>> +	 * FIXME!!
>> +	 * This is returned to user space. Do we need to
>> +	 * return details of MPSS here ?
>
> Yes, we do, probably a separate entry for each valid base/actual page
> size pair.
>

Ok will do new entries to enc for valid actual page size supported.

for 16MB actual page size 

enc[1].page_shift = 24
enc[1].page_shift = x

should this be a seperate patch ?

>> +static inline int hpte_actual_psize(struct hash_pte *hptep, int psize)
>> +{
>> +	unsigned int mask;
>> +	int i, penc, shift;
>> +	/* Look at the 8 bit LP value */
>> +	unsigned int lp = (hptep->r >> LP_SHIFT) & ((1 << (LP_BITS + 1)) - 1);
>
> Why LP_BITS + 1 here?  You seem to be extracting and comparing 9 bits
> rather than 8.  Why is that?
>

My mistake.  Will fix

>> @@ -395,12 +422,13 @@ static void hpte_decode(struct hash_pte *hpte, unsigned long slot,
>>  			/* valid entries have a shift value */
>>  			if (!mmu_psize_defs[size].shift)
>>  				continue;
>> -
>> -			if (penc == mmu_psize_defs[size].penc)
>> -				break;
>> +			for (a_size = 0; a_size < MMU_PAGE_COUNT; a_size++)
>> +				if (penc == mmu_psize_defs[size].penc[a_size])
>> +					goto out;
>
> I think this will get false matches due to unused/invalid entries
> in mmu_psize_defs[size].penc[] containing 0.

Will set the invalid value to 0xff. 

-aneesh