On Thu, Nov 07, 2013 at 06:35:54PM +0000, Maciej W. Rozycki wrote:
> We currently support no MIPS processor that has its I-cache coherent with
> the D-cache, no such processor may even exist. We apparently have two
> configurations that have fully coherent D-caches and therefore set
> cpu_has_ic_fills_f_dc, and these are the Alchemy and NetLogic processor
> families. I have checked relevant CPU documentation I was able to track
> down and in both cases the respective documents[1][2] clearly state that
> the I-cache provides no hardware coherency and whenever instructions in
> memory are modified then the I-cache has to be synchronized by software
> even though the D-caches are fully coherent.
No, cpu_has_ic_fills_f_dc doesn't mean the D-cache is coherent but rather
that the I-cache is refilled from the D-cache if there was a hit. This
means there is no need to write back the D-cache to S-cache or even memory
which is saving some time.
> Therefore we cannot ever avoid the call to flush_cache_page in
> copy_to_user_page and here is a change that reflects this observation.
> The implementation of flush_cache_page may then choose freely whether it
> needs to perform a full cache synchronization with D-cache writeback and
> invalidation requests or whether a lone I-cache invalidation will suffice.
> The c-r4k.c implementation already respects the setting of
> cpu_has_ic_fills_f_dc and avoids touching the D-cache unless necessary.
>
> The lack of I-cache synchronization is typically seen in debugging
> sessions e.g. with GDB where software breakpoints are used. When such a
> breakpoint is hit and subsequently replaced using a ptrace(2) call with
> the original instruction, the BREAK instruction previously executed
> sometimes remains in the I-cache and causes the breakpoint just removed to
> hit again regardless, resulting in a spurious SIGTRAP signal delivery that
> debuggers typically complain about (e.g. "Program received signal SIGTRAP,
> Trace/breakpoint trap" in the case of GDB). Of course the I-cache line
> containing the BREAK instruction may have since been randomly replaced, in
> which case no problem occurs.
>
> [1] "AMD Alchemy Au1200 Processor Data Book", AMD Alchemy, January, 2005,
> Publication ID: 32798A
>
> [2] "XLP Processor Family Programming Reference Manual", NetLogic
> Microsystems, Revision Level 1.10, February, 2011, Document Number
> 10724V110PM-CR (regrettably not publicly available)
>
> Signed-off-by: Maciej W. Rozycki <macro@xxxxxxxxxxxxxxxx>
> ---
> Ralf,
>
> Please apply. I've seen these SIGTRAPs in some NetLogic GDB testing and
> the removal of this cpu_has_ic_fills_f_dc condition from copy_to_user_page
> is really necessary; also the Au1200 document is very explicit about the
> requirement of I-cache invalidation in software (see Section 2.3.7.3
> "Instruction Cache Coherency").
You found a bug and yes, the fix you sent improves things a bit. But
there is also the cache on a cache coherent system where a page might
be marked for a delayed cache flush with SetPageDcacheDirty(), then
flushed by flush_cache_page() before eventually the delayed cacheflush
flushes it once more for a good meassure.
What do you think about below patch to also deal with the duplicate flushing?
Ralf
arch/mips/mm/init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/mips/mm/init.c b/arch/mips/mm/init.c
index 6b59617..80072ef 100644
--- a/arch/mips/mm/init.c
+++ b/arch/mips/mm/init.c
@@ -227,13 +227,13 @@ void copy_to_user_page(struct vm_area_struct *vma,
void *vto = kmap_coherent(page, vaddr) + (vaddr & ~PAGE_MASK);
memcpy(vto, src, len);
kunmap_coherent();
+ if (vma->vm_flags & VM_EXEC)
+ flush_cache_page(vma, vaddr, page_to_pfn(page));
} else {
memcpy(dst, src, len);
if (cpu_has_dc_aliases)
SetPageDcacheDirty(page);
}
- if ((vma->vm_flags & VM_EXEC) && !cpu_has_ic_fills_f_dc)
- flush_cache_page(vma, vaddr, page_to_pfn(page));
}
void copy_from_user_page(struct vm_area_struct *vma,
