Re: [PATCH 1/2] MIPS: c-r4k: Sync icache when it fills from dcache

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]

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@imgtec.com> 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@imgtec.com> 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@imgtec.com>
> >> > Cc: Ralf Baechle <ralf@linux-mips.org>
> >> > Cc: Leonid Yegoshin <leonid.yegoshin@imgtec.com>
> >> > Cc: Manuel Lauss <manuel.lauss@gmail.com>
> >> > Cc: linux-mips@linux-mips.org
> >> > ---
> >> >  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@gmail.com>
> >
> > 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?

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.

> The --loop3 test always fails, with or
> without the patch:
> 
> db1300 ~ # ./mprotect-test --loop2
> Initial mprotect SUCCESS
> Looped { mprotect RW, modify, mprotect RX, test } SUCCESS
> mprotect.c:214: Looped mprotect(2) PROT_READ|PROT_WRITE|PROT_EXEC
> didn't sync icache, ret=ffffffff, expected 0

Yes, thats expected. Generic code detects that flags haven't actually
changed and doesn't do anything, so icache never gets flushed. I
disabled but didn't remove that loop.

Thanks a lot for testing!

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]