Re: [PATCH v2] POWERPC: Allow 32-bit pgtable code to support 36-bit physical

[Scott Wood <scottwood@freescale.com>]

Becky Bruce wrote:
> On Aug 28, 2008, at 11:07 AM, Scott Wood wrote:
> 
>> Becky Bruce wrote:
>>> I'm pretty sure I went through this in great detail at one point
>>> and concluded that I did in fact need the lwarx/stwcx.  IIRC, it
>>> has to do with other non-set_pte_at writers not necessarily
>>> holding the page table lock. FYI, the existing 32-bit PTE code is
>>> doing atomic updates as well.
>> 
>> But will those updates happen if there isn't already a valid PTE?
> 
> I understand what you're saying, I've been here before :)  However, I
>  was never able to convince myself that it's safe without the 
> lwarx/stwcx.  There's hashing code that wanks around with the HASHPTE
>  bit doing a RMW without holding any lock (other than lwarx/stwcx-ing
> the PTE itself).

OK.  I was concerned not just about efficiency, but of the safety of the 
  "stw" write if there were other modifications going on (even if the 
set_pte_at stwcx fails, the other updater could have lwarxed an 
succesfully stwcxed after the stw and ended up with a mixed PTE), but it 
may not be an issue depending on the nature of the updates.

>>> About PTE_ATOMIC_UPDATES, I didn't add that in because hashed
>>> page table implementations require atomic updates.
>> 
>> Right, I misread it and thought it was being used for non-hashed 
>> implementations as well.
>> 
>> What happens if you enable 64-bit PTEs on a 603-ish CPU?  The
>> kconfig seems to allow it.
> 
> Don't do that :)  That's why the help is there in the Kconfig. 

People will do it anyway -- and there's multiplatform to consider.

> Otherwise, I have to list out every 74xx part that supports 36-bit 
> physical addressing.  In any event, nothing interesting will happen 
> other than that you'll waste some space.  The kernel boots fine with
> a non-36b physical u-boot and small amounts of RAM.

My concern was the generic code trying to use 64-bit PTEs, and the 603 
TLB miss handlers continuing to assume that the PTEs are 32-bit, and 
loading the wrong thing.

Wasted space alone is an acceptable consequence of turning on things you 
don't need. :-)

> I'm still not sure where you're going with this - I can remove 44x
> from the conditional part, but we still have to deal with e500 and
> 6xx.

You still need it in "depends" (in the absence of a "PHYS_64BIT_CAPABLE" 
or some such), but not "bool '...' if".  It's not a big deal, just a pet 
peeve.

> In which case you're now setting this in different places for difft
> plats, making it potentially harder to read.  Unless you're
> suggesting allowing the selection of PHYS_64BIT on any platform

No, unless the code for all platforms makes it safe to do so.

-Scott