| Message ID | 20090925211848.GA3371@compile2.chatsunix.int.mrv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> > Thus spake Benjamin Herrenschmidt (benh@kernel.crashing.org): > > > > > > I think there's more finishyness to 8xx than we thought. IE. That > > > tlbil_va might have more reasons to be there than what the comment > > > seems to advertize. Can you try to move it even higher up ? IE. > > > Unconditionally at the beginning of set_pte_filter ? > > > > > > Also, if that doesn't help, can you try putting one in > > > set_access_flags_filter() just below ? > > > > Ok, I got a refresher on the whole concept of "unpopulated TLB entries" > > on 8xx, and that's damn scary. I think what mislead me initially is that > > the comment around the workaround is simply not properly describing the > > extent of the problem :-) > > Oh boy, that sounds bad. Where is a good place to read about this? > > > So I'm not going to make the 8xx TLB miss code sane, that's beyond what > > I'm prepare to do with it, but I suspect that this should fix it (on top > > of upstream). Let me know if that's enough or if we also need to put > > one of these in ptep_set_access_flags(). > > > > Please let me know if that works for you. > > Putting the tlbil_va() in the top of set_pte_filter() doesn't work - it > hangs on boot before it even prints any messages to the console. > > However, adding tlbil_va() to ptep_set_access_flags() as you suggested > makes everything happy. I need to test it some more, but it looks good > so far. Below is what I am testing now. 8xx, is getting very hacky and I suspect that the only long term fix is add code to trap the cache instructions in TLB error/miss and fixup the exception in page fault handler. This will also have the added benefit on being able to use the cache instructions in both kernel and user space like any other ppc arch. Jocke
On Sun, 2009-09-27 at 15:22 +0200, Joakim Tjernlund wrote: > > However, adding tlbil_va() to ptep_set_access_flags() as you suggested > > makes everything happy. I need to test it some more, but it looks good > > so far. Below is what I am testing now. > > 8xx, is getting very hacky and I suspect that the only long term fix is > add code to trap the cache instructions in TLB error/miss and fixup the > exception in page fault handler. This will also have the added benefit on being able > to use the cache instructions in both kernel and user space like any other > ppc arch. First I'd like to understand exactly what's happening today, since it makes little sense :-) I suppose I'll have to get myself some 8xx doco and understand how the bloody MMU works. Then, I saw your old patch and it's -very- invasive. If we can get away with a one liner just adding tlbil_va in the right place, I think I'm happy to stick with it until somebody comes up with a real good reason to do more :-) 8xx is on life support and has been around for long enough without people feeling the need overall to work around that problem so I'm tempted to keep the status-quo here. Cheers, Ben
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 28/09/2009 05:21:00: > > On Sun, 2009-09-27 at 15:22 +0200, Joakim Tjernlund wrote: > > > However, adding tlbil_va() to ptep_set_access_flags() as you suggested > > > makes everything happy. I need to test it some more, but it looks good > > > so far. Below is what I am testing now. > > > > 8xx, is getting very hacky and I suspect that the only long term fix is > > add code to trap the cache instructions in TLB error/miss and fixup the > > exception in page fault handler. This will also have the added benefit on being able > > to use the cache instructions in both kernel and user space like any other > > ppc arch. > > First I'd like to understand exactly what's happening today, since > it makes little sense :-) I suppose I'll have to get myself some > 8xx doco and understand how the bloody MMU works. hmm, I recall that the reason for the currect "force a TLB error" procedure was to fix a problem Montavista had, possibly Tom Rini. He fixed the problem differently at first but later it was changed to what is in use today. If you have access to the old linuxppc-embedded you will find a lot of info in a thread with subject "[PATCH 2.6.14] mm: 8xx MM fix for" Also I made a ref that I cannot access anymore: http://ozlabs.org/pipermail/linuxppc-embedded/2005-January/016382.html > > Then, I saw your old patch and it's -very- invasive. If we can get away > with a one liner just adding tlbil_va in the right place, I think I'm Yes, my patch is invasive but that is because I did it in asm. If you do it in the page fault handler, it will be much easier. In principle you would only have to tag DAR and test for the tag value in asm then branch to the page fault handler. Once there it is easy to decode the insns. > happy to stick with it until somebody comes up with a real good reason > to do more :-) 8xx is on life support and has been around for long yeah, I figured that too but Freescale seems to be cranking out new variants still.
On Mon, 2009-09-28 at 09:22 +0200, Joakim Tjernlund wrote: > > > happy to stick with it until somebody comes up with a real good > reason > > to do more :-) 8xx is on life support and has been around for long > > yeah, I figured that too but Freescale seems to be cranking out new > variants still. Ugh ? And still not fixing the major core bugs ? Grmbl... Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 28/09/2009 09:34:46: > > On Mon, 2009-09-28 at 09:22 +0200, Joakim Tjernlund wrote: > > > > > happy to stick with it until somebody comes up with a real good > > reason > > > to do more :-) 8xx is on life support and has been around for long > > > > yeah, I figured that too but Freescale seems to be cranking out new > > variants still. > > Ugh ? And still not fixing the major core bugs ? Grmbl... yes, I even had them chase down the problem so they know exactly where it is :( I don't think it is in any errata though, but I haven't looked for a few years.
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 28/09/2009 05:21:00: > > > > On Sun, 2009-09-27 at 15:22 +0200, Joakim Tjernlund wrote: > > > > However, adding tlbil_va() to ptep_set_access_flags() as you suggested > > > > makes everything happy. I need to test it some more, but it looks good > > > > so far. Below is what I am testing now. > > > > > > 8xx, is getting very hacky and I suspect that the only long term fix is > > > add code to trap the cache instructions in TLB error/miss and fixup the > > > exception in page fault handler. This will also have the added benefit on being able > > > to use the cache instructions in both kernel and user space like any other > > > ppc arch. > > > > First I'd like to understand exactly what's happening today, since > > it makes little sense :-) I suppose I'll have to get myself some > > 8xx doco and understand how the bloody MMU works. > > hmm, I recall that the reason for the currect "force a TLB error" > procedure was to fix a problem Montavista had, possibly Tom Rini. > He fixed the problem differently at first but later it > was changed to what is in use today. > > If you have access to the old linuxppc-embedded you will find a lot > of info in a thread with subject "[PATCH 2.6.14] mm: 8xx MM fix for" > Also I made a ref that I cannot access anymore: > http://ozlabs.org/pipermail/linuxppc-embedded/2005-January/016382.html Found a link to the original problem http://www.mail-archive.com/linuxppc-embedded@ozlabs.org/msg03376.html seems like I had something to do with the solution that is haunting 8xx now :( Basically 8xx should revert back to the fix suggested in the above link.
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c index 5304093..aef552a 100644 --- a/arch/powerpc/mm/pgtable.c +++ b/arch/powerpc/mm/pgtable.c @@ -176,18 +176,19 @@ static pte_t set_pte_filter(pte_t pte, unsigned long addr) struct page *pg = maybe_pte_to_page(pte); if (!pg) return pte; - if (!test_bit(PG_arch_1, &pg->flags)) { #ifdef CONFIG_8xx - /* On 8xx, cache control instructions (particularly - * "dcbst" from flush_dcache_icache) fault as write - * operation if there is an unpopulated TLB entry - * for the address in question. To workaround that, - * we invalidate the TLB here, thus avoiding dcbst - * misbehaviour. - */ - /* 8xx doesn't care about PID, size or ind args */ - _tlbil_va(addr, 0, 0, 0); + /* On 8xx, cache control instructions (particularly + * "dcbst" from flush_dcache_icache) fault as write + * operation if there is an unpopulated TLB entry + * for the address in question. To workaround that, + * we invalidate the TLB here, thus avoiding dcbst + * misbehaviour. + */ + /* 8xx doesn't care about PID, size or ind args */ + _tlbil_va(addr, 0, 0, 0); #endif /* CONFIG_8xx */ + + if (!test_bit(PG_arch_1, &pg->flags)) { flush_dcache_icache_page(pg); set_bit(PG_arch_1, &pg->flags); } @@ -308,6 +309,12 @@ int ptep_set_access_flags(struct vm_area_struct *vma, unsigned long address, int changed; entry = set_access_flags_filter(entry, vma, dirty); changed = !pte_same(*(ptep), entry); + +#ifdef CONFIG_8xx + /* 8xx doesn't care about PID, size or ind args */ + _tlbil_va(address, 0, 0, 0); +#endif /* CONFIG_8xx */ + if (changed) { if (!(vma->vm_flags & VM_HUGETLB)) assert_pte_locked(vma->vm_mm, address);