Re: [PATCH 2/4] MIPS: Flush highmem pages in __flush_dcache_page

[Paul Burton]

On Tue, Mar 29, 2016 at 10:35:43AM +0200, Ralf Baechle wrote:
> On Tue, Mar 01, 2016 at 02:37:57AM +0000, Paul Burton wrote:
> 
> > When flush_dcache_page is called on an executable page, that page is
> > about to be provided to userland & we can presume that the icache
> > contains no valid entries for its address range. However if the icache
> > does not fill from the dcache then we cannot presume that the pages
> > content has been written back as far as the memories that the dcache
> > will fill from (ie. L2 or further out).
> > 
> > This was being done for lowmem pages, but not for highmem which can lead
> > to icache corruption. Fix this by mapping highmem pages & flushing their
> > content from the dcache in __flush_dcache_page before providing the page
> > to userland, just as is done for lowmem pages.
> > 
> > Signed-off-by: Paul Burton <paul.burton@xxxxxxxxxx>
> > Cc: Lars Persson <lars.persson@xxxxxxxx>
> > ---
> > 
> >  arch/mips/mm/cache.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
> > index 3f159ca..5a67d8c 100644
> > --- a/arch/mips/mm/cache.c
> > +++ b/arch/mips/mm/cache.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/mm.h>
> >  
> >  #include <asm/cacheflush.h>
> > +#include <asm/highmem.h>
> >  #include <asm/processor.h>
> >  #include <asm/cpu.h>
> >  #include <asm/cpu-features.h>
> > @@ -83,8 +84,6 @@ void __flush_dcache_page(struct page *page)
> >  	struct address_space *mapping = page_mapping(page);
> >  	unsigned long addr;
> >  
> > -	if (PageHighMem(page))
> > -		return;
> >  	if (mapping && !mapping_mapped(mapping)) {
> >  		SetPageDcacheDirty(page);
> >  		return;
> > @@ -95,8 +94,15 @@ void __flush_dcache_page(struct page *page)
> >  	 * case is for exec env/arg pages and those are %99 certainly going to
> >  	 * get faulted into the tlb (and thus flushed) anyways.
> >  	 */
> > -	addr = (unsigned long) page_address(page);
> > +	if (PageHighMem(page))
> > +		addr = (unsigned long)kmap_atomic(page);
> > +	else
> > +		addr = (unsigned long)page_address(page);
> > +
> >  	flush_data_cache_page(addr);
> > +
> > +	if (PageHighMem(page))
> > +		__kunmap_atomic((void *)addr);
> >  }
> >  
> >  EXPORT_SYMBOL(__flush_dcache_page);
> 
> I don't see how this should work with cache aliases.  If the page is unmapped
> kmap_atomic will pick a deterministic address only under some circumstances,
> kmap won't.  As the result the wrong cache way will be flushed out, I think.
> 
>   Ralf

Hi Ralf,

None of the systems I tested this on have cache aliases, and highmem on
systems with cache aliases is currently unsupported to the extent that
we BUG_ON such cases in flush_kernel_dcache_page.

So hopefully you won't require that this code making highmem without
aliases also fix the currently-unsupported highmem with aliases case?

Thanks,
    Paul