Re: [PATCH] aio: fix D-cache aliasing issues

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Sun, 2013-11-17 at 20:15 -0500, John David Anglin wrote:
> On 17-Nov-13, at 7:52 PM, James Bottomley wrote:
> 
> >> +void flush_user_dcache_page(struct page *page);
> >> +
> >
> > The split into flush_kernel.. and flush_user.. is pointless.  We  
> > have no
> > use for a flush_user_.. API because it's not part of the standard  
> > linux
> > one.  Plus it's going to confuse those who come after us no end  
> > because
> > now we're different from every other architecture.
> 
> I introduced it because it seemed to be what was needed on parisc when
> we map a kernel page on PA8800/PA8900.  We need this to ensure what the
> kernel sees is up to date and also to ensure that we don't create  
> inequivalent
> aliases between the user and kernel mappings.
> 
> >
> >> #define flush_kernel_dcache_range(start,size) \
> >> 	flush_kernel_dcache_range_asm((start), (start)+(size));
> >> /* vmap range flushes and invalidates.  Architecturally, we don't  
> >> need
> >> @@ -125,18 +127,27 @@ flush_anon_page(struct vm_area_struct *vma,  
> >> struct page *page, unsigned long vma
> >> void mark_rodata_ro(void);
> >> #endif
> >>
> >> -#ifdef CONFIG_PA8X00
> >> -/* Only pa8800, pa8900 needs this */
> >> -
> >> #include <asm/kmap_types.h>
> >>
> >> #define ARCH_HAS_KMAP
> >>
> >> -void kunmap_parisc(void *addr);
> >> +static inline void kmap_parisc(struct page *page)
> >> +{
> >> +	if (parisc_requires_coherency())
> >> +		flush_user_dcache_page(page);
> >> +}
> >
> > No ... if we're genuinely moving coherency into kmap/kunmap, this  
> > has to
> > become
> >
> > 	flush_dcache_page(page);
> >
> > unconditionally.
> 
> There are two things about flush_dcache_page(page):
> 
> 1) The flush could be deferred.  I don't think we want that.

Our deferred flush logic seems a bit bogus to me.  We set a flag that
causes a kernel page flush in invalidate_kernel_vmap_range().  However,
when that function is called we checked this on a page by page bases and
if set we flush the page then we then go and do a dcache flush on the
entire range ... that's got to be wrong.  I also can't really see what
the check for mapping && !mapping_mapped() is about.  Mapping != NULL
means shared mappings with user space !mapping_mapped() means no file
backing (so they must be some form of anonymous vma).  Even if we got
the deferred logic right, I don't think we can avoid flushing in the
anon_vma case

> 2) It seems to unnecessarily call flush_kernel_dcache_page().

Remember flush on parisc is flush and invalidate.  We do need to
invalidate the kernel page in flush_dcache_page() just in case there are
any read only speculated cache lines above it.  I agree there shouldn't
be any dirty cache lines above it in kernel space.

James