BookE pmap_enter() bug?

[Alan Cox]

Marcel Moolenaar wrote:
> On Jun 1, 2010, at 9:21 AM, Alan Cox wrote:
>
>   
>> I've been reviewing the various pmap_enter() implementations and came across the following bit of code in the BookE pmap_enter():
>>
>>       if (prot & VM_PROT_EXECUTE) {
>>           flags |= PTE_SX;
>>           if (!su)
>>               flags |= PTE_UX;
>>
>>           /*
>>            * Check existing flags for execute permissions: if we
>>            * are turning execute permissions on, icache should
>>            * be flushed.
>>            */
>>           if ((flags & (PTE_UX | PTE_SX)) == 0)
>>               sync++;
>>       }
>>
>>
>> This will never flush the instruction cache because the new flags, not the old flags, are being used.  I suspect that the attached change should be made.
>>     
>
> *snip*
>
> You're right.
>
> I'm not aware of icache incoherency problems on BookE, so this raises
> the question as to whether we actually need to worry about syncing the
> icache here. But that's ortogonal...
>
>   

I'm also skeptical of the following, which is a few lines earlier:

            /*
             * The PTE_MODIFIED flag could be set by underlying
             * TLB misses since we last read it (above), possibly
             * other CPUs could update it so we check in the PTE
             * directly rather than rely on that saved local flags
             * copy.
             */
            if (PTE_ISMODIFIED(pte))
                vm_page_dirty(m);


I don't see how this completely addresses the race.  Why can't another 
processor update the PTE immediately after this test and before the PTE 
is finally updated a few lines later?


Alan