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

[Helge Deller <deller@gmx.de>]

* James Bottomley <James.Bottomley@HansenPartnership.com>:
> On Sun, 2013-11-17 at 22:23 +0100, Helge Deller wrote:
> > On 11/16/2013 09:09 PM, Benjamin LaHaise wrote:
> > > On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote:
> > >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote:
> > >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
> > >>>> When a user page mapping is released via kunmap*() functions, the D-cache needs
> > >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
> > >>>>
> > >>>> This patch fixes aio on the parisc platform (and probably others).
> > >>>
> > >>> This should be flush_kernel_dcache_page().  flush_dcache_page() is for
> > >>> full coherency but for unmap, we know the page was coherent going in and
> > >>> may have been modified by the kernel, so only the kernel view needs to
> > >>> be sync'd.  Technically, by the kernel API, the flush should be done
> > >>> *before* unmapping.  This would have mattered on parisc until we did
> > >>> flush via tmpalias which means we no-longer care if the mapping for the
> > >>> flush exists or not because we always recreate it via the tmpalias
> > >>> pages.
> > >>
> > >> On ARM, flush_kernel_dcache_page() actually assumes that the page is
> > >> mapped.  It avoids double flushing of highmem pages by not flushing
> > >> in those cases where kunmap_atomic() already takes care of flushing.
> > > 
> > > Helge -- are you going to resubmit a version of this patch that makes the 
> > > recommended change?
> > 
> > Sure, I'll do. May need some time for testing the various machine types though.
> > Maybe in the end you can drop my patch since we might be able to fix it in the 
> > parisc arch code.
> > 
> > @James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800)
> > works out of the box (and it uses the flush_kernel_dcache_page() function which
> > is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on
> > the other machines too?
> 
> Well, this is why I think our in-kernel API (not the parisc one) for
> kmap/kunmap is faulty.  I can't see any valid use of kmap/kunmap where
> you don't need to flush. 

Maybe adding a flag to kmap(), e.g. READ_ONLY, WRITE_ONLY or similiar? 

> Either you kmapped for reading, in which case
> you need to flush user space before the kernel access, or you kmapped
> for writing, in which case you need to flush user space just before
> mapping (eliminate any dirty user line) and the kernel cache before
> unmap (so the modification becomes visible in main memory).

That's exactly what Dave told me in a private mail earlier.

Below is a patch to the parisc arch code which is originally from Dave.
I only changed kunmap_parisc() to always call
flush_kernel_dcache_page_addr(addr) - independend if we are on a PA8800/8900
CPU or not - which is what you proposed in your original mail.
And yes, this flush_kernel_dcache_page() fixes the aio problems
on all my machines (32- and 64bit kernels, PA7300LC, PA8700, ...).


>  I could see
> an argument that flushing all the user mappings is expensive so only do
> it if necessary, but I think it looks to be always necessary: even if
> you're writing only, which means purging the lines above the user
> mappings, most hardware will panic (HPMC) if it sees inequivalent
> aliases with clashing dirty lines.



diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index f0e2784..ce6d605 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -43,6 +43,8 @@ static inline void flush_kernel_dcache_page(struct page *page)
 	flush_kernel_dcache_page_addr(page_address(page));
 }
 
+void flush_user_dcache_page(struct page *page);
+
 #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);
+}
+
+static inline void kunmap_parisc(void *addr)
+{
+	/* always flush kernel dcache, independed of
+	 * parisc_requires_coherency(). Needed on CPUs < PA8800/PA8900 */
+	flush_kernel_dcache_page_addr(addr);
+}
 
 static inline void *kmap(struct page *page)
 {
 	might_sleep();
+	kmap_parisc(page);
 	return page_address(page);
 }
 
@@ -148,6 +159,7 @@ static inline void kunmap(struct page *page)
 static inline void *kmap_atomic(struct page *page)
 {
 	pagefault_disable();
+	kmap_parisc(page);
 	return page_address(page);
 }
 
@@ -160,7 +172,6 @@ static inline void __kunmap_atomic(void *addr)
 #define kmap_atomic_prot(page, prot)	kmap_atomic(page)
 #define kmap_atomic_pfn(pfn)	kmap_atomic(pfn_to_page(pfn))
 #define kmap_atomic_to_page(ptr)	virt_to_page(ptr)
-#endif
 
 #endif /* _PARISC_CACHEFLUSH_H */
 
diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index c035673..a7ee7b7 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -283,7 +283,7 @@ __flush_cache_page(struct vm_area_struct *vma, unsigned long vmaddr,
 	preempt_enable();
 }
 
-void flush_dcache_page(struct page *page)
+void flush_user_dcache_page(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 	struct vm_area_struct *mpnt;
@@ -291,13 +291,6 @@ void flush_dcache_page(struct page *page)
 	unsigned long addr, old_addr = 0;
 	pgoff_t pgoff;
 
-	if (mapping && !mapping_mapped(mapping)) {
-		set_bit(PG_dcache_dirty, &page->flags);
-		return;
-	}
-
-	flush_kernel_dcache_page(page);
-
 	if (!mapping)
 		return;
 
@@ -332,6 +325,20 @@ void flush_dcache_page(struct page *page)
 	}
 	flush_dcache_mmap_unlock(mapping);
 }
+EXPORT_SYMBOL(flush_user_dcache_page);
+
+void flush_dcache_page(struct page *page)
+{
+	struct address_space *mapping = page_mapping(page);
+
+	if (mapping && !mapping_mapped(mapping)) {
+		set_bit(PG_dcache_dirty, &page->flags);
+		return;
+	}
+
+	flush_kernel_dcache_page(page);
+	flush_user_dcache_page(page);
+}
 EXPORT_SYMBOL(flush_dcache_page);
 
 /* Defined in arch/parisc/kernel/pacache.S */
@@ -413,16 +420,6 @@ void copy_user_page(void *vto, void *vfrom, unsigned long vaddr,
 }
 EXPORT_SYMBOL(copy_user_page);
 
-#ifdef CONFIG_PA8X00
-
-void kunmap_parisc(void *addr)
-{
-	if (parisc_requires_coherency())
-		flush_kernel_dcache_page_addr(addr);
-}
-EXPORT_SYMBOL(kunmap_parisc);
-#endif
-
 void purge_tlb_entries(struct mm_struct *mm, unsigned long addr)
 {
 	unsigned long flags;