On Sat, 31 Oct 2009, Benjamin Herrenschmidt wrote: > Hi folks ! > > So I have a little problem on powerpc ... :-)
Thanks a lot for running this by us. > > Due to the way I'm attempting to do my I$/D$ coherency on embedded > processors, I basically need to "filter out" _PAGE_EXEC in set_pte_at() > if the page isn't clean (PG_arch_1) and the set_pte_at() isn't caused by > an exec fault. etc... > > The problem with that approach (current upstream) is that the generic > code tends not to read back the PTE, and thus still carries around a PTE > value that doesn't match what was actually written. > > For example, we end up with update_mmu_cache() called with an "entry" > argument that has _PAGE_EXEC set while we really didn't write it into > the page tables. This will be problematic when we finally add preloading > directly into the TLB on those processors. There's at least one other > fishy case where huetlbfs would carry the PTE value around and later do > the wrong thing because pte_same() with the loaded one failed. I've not looked to see if there are more such issues in arch/powerpc itself, but those instances you mention are the only ones I managed to find: uses of update_mmu_cache() and that hugetlb_cow() one. The hugetlb_cow() one involves not set_pte_at() but set_huge_pte_at(), so you'd want to change that too? And presumably set_pte_at_notify()? It all seems a lot of tedium, when so very few places are interested in the pte after they've set it. > > What do you suggest we do here ? Among the options at hand: > > - Ugly but would probably "just work" with the last amount of changes: > we could make set_pte_at() be a macro on powerpc that modifies it's PTE > value argument :-) (I -did- warn it was ugly !) I'm not keen on that one :) > > - Another one slightly less bad that would require more work but mostly > mechanical arch header updates would be to make set_pte_at() return the > new value of the PTE, and thus change the callsites to something like: > > entry = set_pte_at(mm, addr, ptep, entry) I prefer that, but it still seems more trouble than it's worth. And though I prefer it to set_pte_at(mm, addr, ptep, &entry) (which would anyway complicate many of the callsites), it might unnecessarily increase the codesize for all architectures (depends on whether gcc notices entry isn't used afterwards anyway). > > - Any other idea ? We could use another PTE bit (_PAGE_HWEXEC), in > fact, we used to, but we are really short on PTE bits nowadays and I > freed that one up to get _PAGE_SPECIAL... _PAGE_EXEC is trivial to > "recover" from ptep_set_access_flags() on an exec fault or from the VM > prot. No, please don't go ransacking your PTE for a sparish bit. You're being a very good citizen to want to bring this so forcefully to the attention of any user of set_pte_at(); but given how few care, and the other such functions you'd want to change too, am I being disgracefully lazy to suggest that you simply change the occasional update_mmu_cache(vma, address, pte); to /* powerpc's set_pte_at might have adjusted the pte */ update_mmu_cache(vma, address, *ptep); ? Which would make no difference to those architectures whose update_mmu_cache() is an empty macro. And fix the mm/hugetlb.c instance in a similar way? Hugh _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev