| Message ID | 1253847827.7103.504.camel@pasglop (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> > > > 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 :-) > > 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. > > Cheers, > Ben. > > diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c > index 5304093..7a8e676 100644 > --- a/arch/powerpc/mm/pgtable.c > +++ b/arch/powerpc/mm/pgtable.c > @@ -170,6 +170,16 @@ struct page * maybe_pte_to_page(pte_t pte) > > static pte_t set_pte_filter(pte_t pte, unsigned long addr) > { > +#ifdef CONFIG_8xx > + /* 8xx has a weird concept of "unpopulated" entries. When we take > + * a TLB miss for a non-valid PTE, we insert such an entry which > + * causes a page fault the next time around. This entry must now > + * be kicked out or we'll just fault again > + */ > + /* 8xx doesn't care about PID, size or ind args */ > + _tlbil_va(addr, 0, 0, 0); > +#endif /* CONFIG_8xx */ > + The main problem with 8xx it does not update the DAR register in the TLB Miss/Fault handlers for cache instructions :( It on old bug that was found only some years ago. I think the old comment is correct though, as I recall it was Marcelo that found the problem and added the workaround. Jocke
On Fri, 2009-09-25 at 10:31 +0200, Joakim Tjernlund wrote: > > The main problem with 8xx it does not update the DAR register in > the TLB Miss/Fault handlers for cache instructions :( It on old bug > that was found only some years ago. > > I think the old comment is correct though, as I recall it was Marcelo > that found the problem and added the workaround. But the TLB needs flushing on more than just the cache instructions, no ? IE. We take a TLB miss, there's no valid PTE, we put one of those "unpopulated" entries in and get into the page fault, at which point we do a set_pte, we -still- need to do an invalidation to get rid of the unpopulated entry so it gets a new TLB miss no ? Without that, it's just going to fault over and over again... In any case, I think flushing unconditionally the target address isn't going to hurt since we are just changing its PTE anyways. As for the DAR problem, I'm not sure whether we really need a workaround since I haven't seem much people complaining about it so far :-) Can you educate me more on the problem ? Can it be fixed without bloating those handlers to oblivion ? Cheers, Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 25/09/2009 11:47:34: > > On Fri, 2009-09-25 at 10:31 +0200, Joakim Tjernlund wrote: > > > > The main problem with 8xx it does not update the DAR register in > > the TLB Miss/Fault handlers for cache instructions :( It on old bug > > that was found only some years ago. > > > > I think the old comment is correct though, as I recall it was Marcelo > > that found the problem and added the workaround. > > But the TLB needs flushing on more than just the cache instructions, > no ? > > IE. We take a TLB miss, there's no valid PTE, we put one of those > "unpopulated" entries in and get into the page fault, at which point we > do a set_pte, we -still- need to do an invalidation to get rid of the > unpopulated entry so it gets a new TLB miss no ? Without that, it's just > going to fault over and over again... I don't know enough about 8xx in 2.6 as we still use 2.4 for 8xx to say for sure. > > In any case, I think flushing unconditionally the target address isn't > going to hurt since we are just changing its PTE anyways. > > As for the DAR problem, I'm not sure whether we really need a workaround > since I haven't seem much people complaining about it so far :-) I did some years ago on 2.4 but no one cared enough :( The drawbacks of not handling this problem is that you will have to very carful to use cache instructions and user space must be especially compiled to omit using them in optimizations. > > Can you educate me more on the problem ? Can it be fixed without > bloating those handlers to oblivion ? Yes, I fixed it for myself but the fix was never accepted. Currently only TLB Error depends on DAR so what I did was to tag DAR with an impossible value and test for that value in the TLB Error handler. If it matched I branched to a subroutine the did instruction decoding in assembler to get at registers used and calculate DAR, then return to the TLB error handler. In hindsight it would have been better to do this work in handle_page_fault. I am attaching my old head_8xx.S for 2.4 Jocke (See attached file: head_8xx.S)
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c index 5304093..7a8e676 100644 --- a/arch/powerpc/mm/pgtable.c +++ b/arch/powerpc/mm/pgtable.c @@ -170,6 +170,16 @@ struct page * maybe_pte_to_page(pte_t pte) static pte_t set_pte_filter(pte_t pte, unsigned long addr) { +#ifdef CONFIG_8xx + /* 8xx has a weird concept of "unpopulated" entries. When we take + * a TLB miss for a non-valid PTE, we insert such an entry which + * causes a page fault the next time around. This entry must now + * be kicked out or we'll just fault again + */ + /* 8xx doesn't care about PID, size or ind args */ + _tlbil_va(addr, 0, 0, 0); +#endif /* CONFIG_8xx */ + pte = __pte(pte_val(pte) & ~_PAGE_HPTEFLAGS); if (pte_looks_normal(pte) && !(cpu_has_feature(CPU_FTR_COHERENT_ICACHE) || cpu_has_feature(CPU_FTR_NOEXECUTE))) { @@ -177,17 +187,6 @@ static pte_t set_pte_filter(pte_t pte, unsigned long addr) 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); -#endif /* CONFIG_8xx */ flush_dcache_icache_page(pg); set_bit(PG_arch_1, &pg->flags); }