[PATCH] 8xx: avoid icbi misbehaviour in __flush_dcache_icache_phys

[Marcelo Tosatti]

Hi Anton,

On Thu, Jul 21, 2005 at 01:34:53AM +0200, Anton Wöllert wrote:
> Marcelo Tosatti wrote:
> >Can you please test the following which is equivalent to your 
> >suggest changes, the only difference being the location, contained 
> >inside flush_dcache_icache_page(). After confirmation that it works
> >we can send up to the kernel maintainers.
> 
> of course, i can do that :).
> 
> >We should still try to understand why this is happening, possibly 
> >matching it to a published CPU errata bug, or inform Freescale
> >otherwise.
> 
> i think so too. as far as i understand it, the thing with the previous 
> __flush_dcache_icache&tlbie patch was that __flush_dcache_icache should 
> try to call dcbst on every address on a cache-line-boundary that could 
> reference the page to give the cpu a hint, so that the next write-access 
> to this page will be more effective, right? but while trying to do the 
> dcbst, the TLB is looked up for the addresses for a faster translation. 
> but here the old TLB-Entry exists, that says invalid Page or something 
> like that, right? So it should be invalidated before, right?

Actually the dcbst instruction flushes data from cache to main memory.
Quoting MPC860UM.pdf:

Data Cache Block Store (dcbst)

The effective address is computed, translated, and checked for protection
violations as defined in the PowerPC architecture. This instruction is 
treated as a load with respect to address translation and memory protection.
If the address hits in the cache and the cache block is in the
unmodified-valid state, no action is taken. If the address hits in the cache
and the cache block is in the modified-valid state, the modified block is 
written back to memory and the cache block is placed in the unmodified-valid
state.

> 	Question, normally the dcbst should cause a TLB-Error-Exception, 
> 	that than should do reload the TLB-Entry automatically, doesn't this work 
> because of the dcb*-errata (btw. where can i found that?). 

I dont think that this one bug which the TLB invalidate worksaround, namely 
dcbst misbehaviour with invalid (zeroed) TLB, is covered by Freescale
errata (I'm looking at Errata revision A.2).

> And the __flush_dcache_icache should do a dcbst 4096/16=256 times, but there are 
> just 64 cache-lines à 16Byte (1Kb DCache), so shouldn't be 3/4 of that 
> routine redundant? and also, as it seems to me, the whole dcache will be 
> *flushed* or better dcbst'd trough this routine. is that right?
> please correct me!

See the dcbst description above, the cache line entry is only flushed if
the address hits in cache _and_ is the modified state.

> so, now the __flush_dcache_icache_phys(). this is just called, when the 
> memory-mapping of the vma is not same as the one of that from the 
> current process. this should be the case with ptrace. couldn't now be 
> done the same as above? maybe no because the address is the virtual 
> address as seen from the ptraced process? and so that will *flush* the 
> pages from the ptraced process (that doesn't exists yet?, because they 
> were just created for the tracing process?). 

Since the 8xx cache is physically indexed and tagged, you can flush memory
by either its virtual address or its physical address - virtual accesses 
need to perform v->p translation with help from the MMU.

On the ptrace case the kernel must do a physical flush because the ptraceing 
process does not have an equivalent virtual->physical mapping in its own 
address space. ie. equivalent virtual addresses from the ptracing and 
ptrace'd processes are obviously not physically equivalent. 

The change you proposed flushes the page via its kernel virtual map: 
page_address(page), which is similar to doing a physical flush. Only 
difference being that on 8xx "icbi" goes nuts when using the physical 
flush, as you reported.

> so the pte, that was created for the tracing process is used. from that, 
> the physical address is calculated (how is this done, whats the magic 
> about the pfn-stuff etc, i didn't really understand that :( ).

Actually the page being faulted in by the ptrace'ing process is a page
of the _ptraced_ process, that is, one process is faulting in pages 
of another process.

> now  __flush_dcache_icache_phys is used, because the [d|i]cache uses physical 
> address tags and so the data-mmu could be turned off to flush these entries.
> so to understand the things further, i have to understand which 
> addresses are used with dcbst and icbi. there is 
> page=pfn_to_page(pte_pfn(pte)) and (page_to_pfn(page) << PAGE_SHIFT). 
> but what are these values or where do they point. any hints to the 
> pfn&pte stuff? unfortunately i didn't have much time the last days to 
> understand this more deeply... 

PFN = Page Frame Number. This value is the page identifier by page size units. 

page_to_pfn(page) << PAGE_SHIFT means:

Get the pfn of the page and multiply it by page size ("<< PAGE_SHIFT"). 

Returns address of page in bytes, not in page units.

> >On 8xx, in the case where a pagefault happens for a process who's not
> >the owner of the vma in question (ptrace for instance), the flush 
> >operation is performed via the physical address.
> >
> >Unfortunately, that results in a strange, unexplainable "icbi" 
> >instruction fault, most likely due to a CPU bug (see oops below).
> 
> where do you know, that "icbi" is the bad?

What instruction resides at __flush_dcache_icache_phys+0x38 ? 

>From what I could see that is "icbi".