On Thu, Oct 19, 2017 at 02:25:47PM +1100, Michael Ellerman wrote:
> Ram Pai <linux...@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
