HIGHMEM is broken when working in SMP V6 mode

[linux@arm.linux.org.uk (Russell King - ARM Linux)]

On Mon, Jan 24, 2011 at 10:47:36AM +0200, saeed bishara wrote:
> >> >> 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 that
> >> >
> >> > 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.

> 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?

> +			{
>  				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.

> +		{
>  			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.