[PATCH v3 09/31] arm64: Cache maintenance routines

Hi Simon,

On Fri, Sep 07, 2012 at 08:35:42PM +0100, Simon Baatz wrote:

On Fri, Sep 07, 2012 at 05:26:44PM +0100, Catalin Marinas wrote:

quoted

+#define ARCH_HAS_FLUSH_ANON_PAGE
+static inline void flush_anon_page(struct vm_area_struct *vma,
+                      struct page *page, unsigned long vmaddr)
+{
+     extern void __flush_anon_page(struct vm_area_struct *vma,
+                             struct page *, unsigned long);
+     if (PageAnon(page))
+             __flush_anon_page(vma, page, vmaddr);

__flush_anon_page() does nothing. Shouldn't this be removed as well?

Yes, good point.

quoted

+void __flush_dcache_page(struct address_space *mapping, struct page *page)
+{
+     __flush_dcache_area(page_address(page), PAGE_SIZE);
+}
+
+void __sync_icache_dcache(pte_t pte)
+{
+     unsigned long pfn;
+     struct page *page;
+
+     pfn = pte_pfn(pte);
+     if (!pfn_valid(pfn))
+             return;
+
+     page = pfn_to_page(pfn);
+     if (!test_and_set_bit(PG_dcache_clean, &page->flags))
+             __flush_dcache_page(NULL, page);
+     __flush_icache_all();
+}
+
+/*
+ * Ensure cache coherency between kernel mapping and userspace mapping of this
+ * page.
+ */
+void flush_dcache_page(struct page *page)
+{
+     struct address_space *mapping;
+
+     /*
+      * The zero page is never written to, so never has any dirty cache
+      * lines, and therefore never needs to be flushed.
+      */
+     if (page == ZERO_PAGE(0))
+             return;
+
+     mapping = page_mapping(page);
+
+     if (mapping && !mapping_mapped(mapping))
+             clear_bit(PG_dcache_clean, &page->flags);
+     else {
+             __flush_dcache_page(mapping, page);
+             if (mapping)
+                     __flush_icache_all();

Is this necessary to ensure I/D coherency? Then, I would have
expected

                if (mapping) {
                        __flush_dcache_page(mapping, page);
                        __flush_icache_all();
                }

similar to __sync_icache_dcache() above.

We don't want to do additional flushing if !mapping_mapped() as the page
isn't mapped in user space. In this case we defer the flushing until
__sync_icache_dcache().

The other case is for anonymous pages where mapping == NULL. Here we
don't defer the D-cache flush and do it directly. The I-cache, if
needed, is handled later in __sync_icache_dcache(). This was based on
the idea that this case is mainly for the args/env page which is mapped
shortly after anyway, so not worth deferring. On AArch64, I don't think
it makes any difference. Maybe a slight improvement (at least in
clarity) in flush_dcache_page():

	if (mapping && mapping_mapped(mapping)) {
		__flush_dcache_page(page);
		__flush_icache_all();
		set_bit(PG_dcache_clean, &page->flags);
	} else {
		clear_bit(PG_dcache_clean, &page->flags);
	}

In this case the anonymous page flushing is deferred to
__sync_icache_dcache().

What is the reason why the D-cache flush is done in different
cases than the following I-cache flush?

For __sync_icache_dcache(), we need to handle the situation where the
page mapped into user space has been cleaned (D-cache) but there may be
stale data in the I-cache. I think this can only happen with an
ASID-tagged VIVT I-cache configuration (which is allowed on AArch64) if
an existing page has been unmapped and the same virtual address remapped
(withing the same mm context) to a different page that had been cleaned
previously. We could optimise the __sync_icache_dcache() as below:

	if (!test_and_set_bit(PG_dcache_clean, &page->flags)) {
		__flush_dcache_page(page);
		__flush_icache_all();
	} else if (icache_is_aivivt()) {
		__flush_icache_all();
	}

quoted

+             set_bit(PG_dcache_clean, &page->flags);
+     }
+}
+EXPORT_SYMBOL(flush_dcache_page);
+
+void __flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vmaddr)
+{
+}

Note that the __flush_dcache_page(mapping, page) in
flush_dcache_page() above is called when page is an anonymous page
(since mapping == NULL in this case).  If the call to
__flush_dcache_page() is right above, it should be needed
here as well?

flush_anon_page() is called when the kernel needs to access an anonymous
page. Given that the D-cache behaves like a PIPT, there is no need for
additional flushing here. The flush_dcache_page() call was based on the
assumption that such page needs flushing anyway and it's not worth
deferring. But the code may be easier to understand as I suggested above
(and slightly more optimal for the VIPT I-cache case).

It looks like any other architecture does something different here.

-- 
Catalin