Re: [PATCH 2/7] powerpc: introduce pte_get_hash_gslot() helper

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

On Wed, Sep 13, 2017 at 07:32:57PM +1000, Balbir Singh wrote:
> On Sat, Sep 9, 2017 at 8:44 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > Introduce pte_get_hash_gslot()() which returns the slot number of the
> > HPTE in the global hash table.
> >
> > This function will come in handy as we work towards re-arranging the
> > PTE bits in the later patches.
> >
> > Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/asm/book3s/64/hash.h |    3 +++
> >  arch/powerpc/mm/hash_utils_64.c           |   18 ++++++++++++++++++
> >  2 files changed, 21 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
> > index f884520..060c059 100644
> > --- a/arch/powerpc/include/asm/book3s/64/hash.h
> > +++ b/arch/powerpc/include/asm/book3s/64/hash.h
> > @@ -166,6 +166,9 @@ static inline int hash__pte_none(pte_t pte)
> >         return (pte_val(pte) & ~H_PTE_NONE_MASK) == 0;
> >  }
> >
> > +unsigned long pte_get_hash_gslot(unsigned long vpn, unsigned long shift,
> > +               int ssize, real_pte_t rpte, unsigned int subpg_index);
> > +
> >  /* This low level function performs the actual PTE insertion
> >   * Setting the PTE depends on the MMU type and other factors. It's
> >   * an horrible mess that I'm not going to try to clean up now but
> > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> > index 67ec2e9..e68f053 100644
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -1591,6 +1591,24 @@ static inline void tm_flush_hash_page(int local)
> >  }
> >  #endif
> >
> > +/*
> > + * return the global hash slot, corresponding to the given
> > + * pte, which contains the hpte.
> 
> Does this work with native/guest page tables? I guess both.

Yes. it is supposed to work with native as well as guest page tables.
The code has been this way for ages. This patch encapsulate
the logic in a standalone function.

> The comment sounds trivial, could you please elaborate more.
> Looking at the code, it seems like given a real pte, we use
> the hash value and hidx to figure out the slot value in the global
> slot information. This uses information in the software page
> tables. Is that correct?

Yes. This uses information passed to it by the caller, the information
is expected to be derived from linux page table.

> Do we have to consider validity and
> present state here or is that guaranteed?

This function's job is to do the math and return the global slot based
on the input. It will return the calculated value regardless of the validity of
its inputs.

Its the the callers' job to validate the pte and ensure that it is hashed,
before meaningfully using the return value of the this function.

> 
> > + */
> > +unsigned long pte_get_hash_gslot(unsigned long vpn, unsigned long shift,
> > +               int ssize, real_pte_t rpte, unsigned int subpg_index)
> > +{
> > +       unsigned long hash, slot, hidx;
> > +
> > +       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;
> > +       return slot;
> > +}
> > +
> >  /* WARNING: This is called from hash_low_64.S, if you change this prototype,
> >   *          do not forget to update the assembly call site !
> >   */
> 
> Balbir Singh.

-- 
Ram Pai