pte_update and 64-bit PTEs on PPC32?

[Kumar Gala]

On Apr 6, 2005, at 1:53 AM, Benjamin Herrenschmidt wrote:

> On Wed, 2005-04-06 at 01:51 -0500, Kumar Gala wrote:
>  > Paul,
>  >
> > I've tracked down a bug I've been having to the fact that pte_update
> > assumes a pte is a unsigned long.  I need to look into what the exact
>  > implications this has.  I was wondering what the thoughts were with
> > respect to how this is suppose to work properly on 440 with its 
> 64-bit
> > pte?  I'm looking at a 64-bit pte for some Freescale book-e parts as 
> we
> > move to 36-bit physical address support.
>  >
> > The problem I found was ptep_get_and_clear() would return back only a
> > 32-bit value and thus we loose any information in the upper 
> 32-bits.  I
> > found the call in sys_mprotect ... -> change_pte_range ->
> > ptep_get_and_clear()
> >
> > Will provide some update on this tomorrow.
>
> It's quite important for the flags to all be together in a single 32
>  bits entity so that atomic operations can be done on them. The RPN
>  should be able to extend beyond the initial 32 bits provided we are
>  careful about the way we manipulate the PTEs. When setting a PTE, we
>  should always first set the "other" part, then the PTE present bit 
> last
> or a CPU would possibly get a stale PTE. The problem with that scheme 
> is
> that I can see possible races on dual page faults trying to fill in the
>  same PTE if we drop the page table lock (christoph lameter stuff) but 
> it
>  should work for us now.

Ben, I agree with you about having the flags in a single word so we can 
lock them properly.  In the short term it appears that the issue I'm 
running into is explicit with ptep_get_and_clear():

static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned 
long addr,
                                        pte_t *ptep)
{
         return __pte(pte_update(ptep, ~_PAGE_HASHPTE, 0));
}

It appears that we should be returning the pte that was passed in, 
before its modified?  (seems a little silly to me, why bother, the 
caller could do this -- i've posted to lkml on the issue?).  Anyways, 
since pte_update only returns the lower 32-bits the wrong thing 
happens.  The following seems to be a better implementation of 
ptep_get_and_clear() for ppc32 which resolves my issue:

static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned 
long addr,
                                        pte_t *ptep)
{
         pte_t tmp = *ptep;
         pte_update(ptep, ~_PAGE_HASHPTE, 0);
         return tmp;
}

If we are ok with this I'll send a patch upstream for it.  I'd like to 
still discuss how to make this all proper long term.  Currently, 
ptep_get_and_clear was the only user of pte_update that used the return 
value for anything but flags.  One change would be for it to return 
just the flags portion of the pte it was given.  Another would be for 
us to implement a proper 64-bit pte version of pte_update.

- kumar