Re: [PATCH 3/7] powerpc: Free up four 64K PTE bits in 4K backed HPTE pages

[Ram Pai <linuxram@us.ibm.com>]

On Thu, Oct 19, 2017 at 02:25:47PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/mm/hash64_64k.c b/arch/powerpc/mm/hash64_64k.c
> > index 1a68cb1..c6c5559 100644
> > --- a/arch/powerpc/mm/hash64_64k.c
> > +++ b/arch/powerpc/mm/hash64_64k.c
> > @@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> >  	if (__rpte_sub_valid(rpte, subpg_index)) {
> >  		int ret;
> >  
> > -		hash = hpt_hash(vpn, shift, ssize);
> > -		hidx = __rpte_to_hidx(rpte, subpg_index);
> > -		if (hidx & _PTEIDX_SECONDARY)
> > -			hash = ~hash;
> > -		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
> > -		slot += hidx & _PTEIDX_GROUP_IX;
> > +		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte,
> > +					subpg_index);
> > +		ret = mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn,
> > +			MMU_PAGE_4K, MMU_PAGE_4K, ssize, flags);
> 
> This was formatted correctly before:
>   
> > -		ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
> > -						 MMU_PAGE_4K, MMU_PAGE_4K,
> > -						 ssize, flags);
> >  		/*
> > -		 *if we failed because typically the HPTE wasn't really here
> > +		 * if we failed because typically the HPTE wasn't really here
> 
> If you're fixing it up please make it "If ...".
> 
> >  		 * we try an insertion.
> >  		 */
> >  		if (ret == -1)
> > @@ -148,6 +130,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> >  	}
> >  
> >  htab_insert_hpte:
> > +
> > +	/*
> > +	 * initialize all hidx entries to invalid value,
> > +	 * the first time the PTE is about to allocate
> > +	 * a 4K hpte
> > +	 */
> 
> Should be:
> 	/*
> 	 * Initialize all hidx entries to invalid value, the first time
>          * the PTE is about to allocate a 4K HPTE.
> 	 */
> 
> > +	if (!(old_pte & H_PAGE_COMBO))
> > +		rpte.hidx = ~0x0UL;
> > +
> 
> Paul had the idea that if we biased the slot number by 1, we could make
> the "invalid" value be == 0.
> 
> That would avoid needing to that above, and also mean the value is
> correctly invalid from the get-go, which would be good IMO.
> 
> I think now that you've added the slot accessors it would be pretty easy
> to do.

I did attempt to do so, and was not getting it right. The machine went
unstable. So left it with an accessor, to be revisited at a
later point in time. That time has come... I suppose.  Shall I make it a
separate patch instead of baking it into this patch?

RP