HIGHMEM is broken when working in SMP V6 mode
From: Russell King - ARM Linux <hidden>
Date: 2011-01-24 09:19:57
On Mon, Jan 24, 2011 at 10:47:36AM +0200, saeed bishara wrote:
quoted
quoted
quoted
quoted
I've port 2.6.35 to SMP system that runs in V6 mode, this system doesn't support TLB operations broadcasting by hw, so it uses IPI messages for that. ?when enabling DEBUG_LOCKDEP, I got the following error message while booting the system from NFS:You've bypassed this check: ? ? ? ? ? ? ? ?if (is_smp() && tlb_ops_need_broadcast()) { ? ? ? ? ? ? ? ? ? ? ? ?/* ? ? ? ? ? ? ? ? ? ? ? ? * kmap_high needs to occasionally flush TLB entries, ? ? ? ? ? ? ? ? ? ? ? ? * however, if the TLB entries need to be broadcast ? ? ? ? ? ? ? ? ? ? ? ? * we may deadlock: ? ? ? ? ? ? ? ? ? ? ? ? * ?kmap_high(irqs off)->flush_all_zero_pkmaps-> ? ? ? ? ? ? ? ? ? ? ? ? * ?flush_tlb_kernel_range->smp_call_function_many ? ? ? ? ? ? ? ? ? ? ? ? * ? (must not be called with irqs off) ? ? ? ? ? ? ? ? ? ? ? ? */ ? ? ? ? ? ? ? ? ? ? ? ?reason = "without hardware TLB ops broadcasting"; ? ? ? ? ? ? ? ?} so you lose. ?There's reasons why such checks are put in. ?We can not support SMP and highmem on systems which do not have TLB broadcasting. That's not because the code doesn't support it, it's because there are deadlocks which will occur.thanks, I missed thatquoted
The fact is that it is unsafe to send IPIs with IRQs disabled, which means you can't IPI a TLB operation and wait for it to complete with IRQs disabled.as I understand it, the lock_kmap() started to disable IRQs in order to support the vivt and vipt caches, but in SMP (at least in my case), the caches are PIPT, so I think I can do the following: 1. undef ?the ?ARCH_NEEDS_KMAP_HIGH_GET 2. use page_address instead of kmap_high_get() do you think it will work?Definitely not. ?We use kmap_high_get() so that we can ensure that we've flushed data out of the PIPT cache for highmem pages. ?highmem pages which are unmapped do not have a valid page_address() but may have PIPT cache lines associated with them. So no, I don't think it'll be safe.ok, what about the following patch, the idea is to use only the kmap_high_l1_vipt when doing cache maintenance.
You're really not listening.
quoted hunk ↗ jump to hunk
diff --git a/arch/arm/include/asm/highmem.h b/arch/arm/include/asm/highmem.h index feb988a..457998c 100644 --- a/arch/arm/include/asm/highmem.h +++ b/arch/arm/include/asm/highmem.h@@ -19,7 +19,9 @@ extern pte_t *pkmap_page_table; +#ifndef CONFIG_SMP #define ARCH_NEEDS_KMAP_HIGH_GET +#endif extern void *kmap_high(struct page *page); extern void *kmap_high_get(struct page *page);diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 9e7742f..d22366b 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c@@ -459,12 +459,15 @@ static void dma_cache_maint_page(struct page*page, unsigned long offset, } len = PAGE_SIZE - offset; } +#ifdef ARCH_NEEDS_KMAP_HIGH_GET vaddr = kmap_high_get(page); if (vaddr) { vaddr += offset; op(vaddr, len, dir); kunmap_high(page); - } else if (cache_is_vipt()) { + } else if (cache_is_vipt()) +#endif
So you're disabling DMA cache maintainence, making DMA support *unsafe* on your platform. You'll get filesystem corruption and other crap like that. Maybe you don't care for users data?
quoted hunk ↗ jump to hunk
+ { pte_t saved_pte; vaddr = kmap_high_l1_vipt(page, &saved_pte); op(vaddr + offset, len, dir);diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c index c6844cb..7f96b2c 100644 --- a/arch/arm/mm/flush.c +++ b/arch/arm/mm/flush.c@@ -161,11 +161,15 @@ void __flush_dcache_page(struct address_space*mapping, struct page *page) if (!PageHighMem(page)) { __cpuc_flush_dcache_area(page_address(page), PAGE_SIZE); } else { - void *addr = kmap_high_get(page); + void *addr; +#ifdef ARCH_NEEDS_KMAP_HIGH_GET + addr = kmap_high_get(page); if (addr) { __cpuc_flush_dcache_area(addr, PAGE_SIZE); kunmap_high(page); - } else if (cache_is_vipt()) { + } else if (cache_is_vipt()) +#endif
I suggest you read the commit comments in 7e5a69e83. Not only that but this can lead to I/D cache incoherency, leading to segfaults and illegal instruction exceptions from userspace programs.
quoted hunk ↗ jump to hunk
+ { pte_t saved_pte; addr = kmap_high_l1_vipt(page, &saved_pte); __cpuc_flush_dcache_area(addr, PAGE_SIZE);diff --git a/arch/arm/mm/highmem.c b/arch/arm/mm/highmem.c index 6ab2440..7493a79 100644 --- a/arch/arm/mm/highmem.c +++ b/arch/arm/mm/highmem.c@@ -57,7 +57,11 @@ void *kmap_atomic(struct page *page, enum km_type type) kmap = NULL; else #endif +#ifdef ARCH_NEEDS_KMAP_HIGH_GET kmap = kmap_high_get(page); +#else + kmap = NULL; +#endif if (kmap) return kmap;
So I doubt you'll be able to get this to work reliably, even if you disabled all DMA support for your platform. I really think you're wasting your time.