On Fri, Jan 22, 2016 at 4:02 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
> On Fri, Jan 22, 2016 at 03:30:06PM +0100, Manuel Lauss wrote:
>> On Fri, Jan 22, 2016 at 1:19 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
>> > On Fri, Jan 22, 2016 at 01:06:14PM +0100, Manuel Lauss wrote:
>> >> Hi James,
>> >>
>> >> On Fri, Jan 22, 2016 at 11:58 AM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
>> >> > It is still necessary to handle icache coherency in flush_cache_range()
>> >> > and copy_to_user_page() when the icache fills from the dcache, even
>> >> > though the dcache does not need to be written back. However when this
>> >> > handling was added in commit 2eaa7ec286db ("[MIPS] Handle I-cache
>> >> > coherency in flush_cache_range()"), it did not do any icache flushing
>> >> > when it fills from dcache.
>> >> >
>> >> > Therefore fix r4k_flush_cache_range() to run
>> >> > local_r4k_flush_cache_range() without taking into account whether icache
>> >> > fills from dcache, so that the icache coherency gets handled. Checks are
>> >> > also added in local_r4k_flush_cache_range() so that the dcache blast
>> >> > doesn't take place when icache fills from dcache.
>> >> >
>> >> > A test to mmap a page PROT_READ|PROT_WRITE, modify code in it, and
>> >> > mprotect it to VM_READ|VM_EXEC (similar to case described in above
>> >> > commit) can hit this case quite easily to verify the fix.
>> >> >
>> >> > A similar check was added in commit f8829caee311 ("[MIPS] Fix aliasing
>> >> > bug in copy_to_user_page / copy_from_user_page"), so also fix
>> >> > copy_to_user_page() similarly, to call flush_cache_page() without taking
>> >> > into account whether icache fills from dcache, since flush_cache_page()
>> >> > already takes that into account to avoid performing a dcache flush.
>> >> >
>> >> > Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
>> >> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
>> >> > Cc: Leonid Yegoshin <leonid.yegoshin@xxxxxxxxxx>
>> >> > Cc: Manuel Lauss <manuel.lauss@xxxxxxxxx>
>> >> > Cc: linux-mips@xxxxxxxxxxxxxx
>> >> > ---
>> >> > arch/mips/mm/c-r4k.c | 11 +++++++++--
>> >> > arch/mips/mm/init.c | 2 +-
>> >> > 2 files changed, 10 insertions(+), 3 deletions(-)
>> >>
>> >>
>> >> I did some light testing on Alchemy and see no problems so far.
>> >> If it matters: Tested-by: Manuel Lauss <manuel.lauss@xxxxxxxxx>
>> >
>> > Thanks Manuel.
>> >
>> > FWIW, attached is the test program I mentioned, which hits the first
>> > part of this patch (flush_cache_range) via mprotect(2) and checks if
>> > icache seems to have been flushed (tested on mips64r6, but should be
>> > portable).
>>
>> With the patch it takes almost 3 times as long to finish the test, but
>
> Interesting... I suppose at least it brings alchemy in line with
> behaviour on other cores.
>
>> it does fix
>> occasional (5 out of 20 runs) failures.
>
> How big is your icache?
icache has16kB
> With 64KB icache it fails fairly consistently for me, but wouldn't
> surprise me if smaller caches would make it much harder to hit,
> especially as it only tests the first cache line in the page.
Thanks!
Manuel
