HIGHMEM is broken when working in SMP V6 mode

HIGHMEM is broken when working in SMP V6 mode

[saeed bishara]

Hi,
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:


[   24.460000] =================================
[   24.460000] [ INFO: inconsistent lock state ]
[   24.460000] 2.6.35.9 #102
[   24.460000] ---------------------------------
[   24.460000] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[   24.460000] rc/1370 [HC0[0]:SC0[0]:HE1:SE1] takes:
[   24.460000]  (kmap_lock){+.?...}, at: [<800ffb64>] kmap_high+0x24/0x1ec
[   24.460000] {IN-SOFTIRQ-W} state was registered at:
[   24.460000]   [<800b1f60>] mark_lock+0x26c/0x65c
[   24.460000]   [<800b5614>] __lock_acquire+0x6e4/0xa20
[   24.460000]   [<800b5a40>] lock_acquire+0xf0/0x110
[   24.460000]   [<804bc97c>] _raw_spin_lock_irqsave+0x50/0x64
[   24.460000]   [<800fffec>] kmap_high_get+0x20/0x88
[   24.460000]   [<8003fc40>] __flush_dcache_page+0x4c/0x120
[   24.460000]   [<8003ff64>] flush_dcache_page+0x68/0x88
[   24.460000]   [<8049a710>] xdr_partial_copy_from_skb+0x168/0x1dc
[   24.460000]   [<8049a7f4>] csum_partial_copy_to_xdr+0x70/0x154
[   24.460000]   [<8049d200>] xs_udp_data_ready+0x114/0x234
[   24.460000]   [<80418a44>] sock_queue_rcv_skb+0x200/0x23c
[   24.460000]   [<8044da24>] ip_queue_rcv_skb+0x5c/0x64
[   24.460000]   [<8046b340>] __udp_queue_rcv_skb+0xf4/0x1f0
[   24.460000]   [<8046cd48>] udp_queue_rcv_skb+0x240/0x3e0
[   24.460000]   [<8046d69c>] __udp4_lib_rcv+0x680/0x96c
[   24.460000]   [<8046d9a8>] udp_rcv+0x20/0x28
[   24.460000]   [<80446e8c>] ip_local_deliver+0x1c8/0x354
[   24.460000]   [<80447764>] ip_rcv+0x74c/0x7b8
[   24.460000]   [<80424868>] __netif_receive_skb+0x2a4/0x2f8
[   24.460000]   [<80425558>] netif_receive_skb+0xec/0x114
[   24.460000]   [<80425644>] napi_skb_finish+0x40/0x58
[   24.460000]   [<80425b98>] napi_gro_receive+0x38/0x3c
[   24.460000]   [<8038ecec>] e1000_receive_skb+0x50/0x54
[   24.460000]   [<80393a7c>] e1000_clean_rx_irq+0x20c/0x2d0
[   24.460000]   [<803924bc>] e1000_clean+0x80/0x208
[   24.460000]   [<804288ec>] net_rx_action+0x90/0x1f4
[   24.460000]   [<8008b238>] __do_softirq+0x110/0x20c
[   24.460000]   [<8008b830>] irq_exit+0x60/0x68
[   24.460000]   [<80034094>] asm_do_IRQ+0x94/0xc8
[   24.460000]   [<804bd458>] __irq_svc+0x38/0x120
[   24.460000]   [<804b0af4>] rest_init+0xc0/0xf4
[   24.460000]   [<80008e34>] start_kernel+0x27c/0x2dc
[   24.460000]   [<00008080>] 0x8080
[   24.460000] irq event stamp: 605
[   24.460000] hardirqs last  enabled at (604): [<804bd05c>]
_raw_spin_unlock_irqrestore+0x4c/0x54
[   24.460000] hardirqs last disabled at (605): [<804bc8f4>]
_raw_spin_lock_irq+0x28/0x60
[   24.460000] softirqs last  enabled at (547): [<8049f438>]
rpc_wake_up_next+0x1ac/0x1c0
[   24.460000] softirqs last disabled at (545): [<804bcac0>]
_raw_spin_lock_bh+0x24/0x5c
[   24.460000]
[   24.460000] other info that might help us debug this:
[   24.460000] 2 locks held by rc/1370:
[   24.460000]  #0:  (&p->cred_guard_mutex){+.+.+.}, at: [<8011f988>]
prepare_bprm_creds+0x34/0x68
[   24.460000]  #1:  (kmap_lock){+.?...}, at: [<800ffb64>] kmap_high+0x24/0x1ec
[   24.460000]
[   24.460000] stack backtrace:
[   24.460000] Backtrace:
[   24.460000] [<80038d68>] (dump_backtrace+0x0/0x114) from
[<80038ec0>] (dump_stack+0x20/0x24)
[   24.460000]  r6:c01c5460 r5:805b104c r4:00000001
[   24.460000] [<80038ea0>] (dump_stack+0x0/0x24) from [<800b1cb0>]
(print_usage_bug+0x170/0x1b4)
[   24.460000] [<800b1b40>] (print_usage_bug+0x0/0x1b4) from
[<800b2024>] (mark_lock+0x330/0x65c)
[   24.460000] [<800b1cf4>] (mark_lock+0x0/0x65c) from [<800b23b4>]
(mark_held_locks+0x64/0x8c)
[   24.460000] [<800b2350>] (mark_held_locks+0x0/0x8c) from
[<800b253c>] (trace_hardirqs_on_caller+0x160/0x21c)
[   24.460000]  r8:80baa4b8 r7:80baa4e4 r6:00000000 r5:8008ac24 r4:c01c5460
[   24.460000] [<800b23dc>] (trace_hardirqs_on_caller+0x0/0x21c) from
[<800b260c>] (trace_hardirqs_on+0x14/0x18)
[   24.460000]  r5:c15a1e2c r4:8003a41c
[   24.460000] [<800b25f8>] (trace_hardirqs_on+0x0/0x18) from
[<8008ac24>] (on_each_cpu+0x3c/0x48)
[   24.460000] [<8008abe8>] (on_each_cpu+0x0/0x48) from [<8003a408>]
(flush_tlb_kernel_range+0x4c/0x60)
[   24.460000]  r6:00000000 r5:00000800 r4:81c445cc
[   24.460000] [<8003a3bc>] (flush_tlb_kernel_range+0x0/0x60) from
[<800ffb28>] (flush_all_zero_pkmaps+0xb8/0xd0)
[   24.460000] [<800ffa70>] (flush_all_zero_pkmaps+0x0/0xd0) from
[<800ffbbc>] (kmap_high+0x7c/0x1ec)
[   24.460000] [<800ffb40>] (kmap_high+0x0/0x1ec) from [<80041ce4>]
(kmap+0x58/0x70)
[   24.460000] [<80041c8c>] (kmap+0x0/0x70) from [<8011f218>]
(copy_strings+0x1a4/0x330)
[   24.460000]  r4:c11c1000
[   24.460000] [<8011f074>] (copy_strings+0x0/0x330) from [<8011f3f0>]
(copy_strings_kernel+0x4c/0x84)
[   24.460000] [<8011f3a4>] (copy_strings_kernel+0x0/0x84) from
[<80120c2c>] (do_execve+0x17c/0x2d4)
[   24.460000]  r4:00000080
[   24.460000] [<80120ab0>] (do_execve+0x0/0x2d4) from [<80038700>]
(sys_execve+0x44/0x64)
[   24.460000] [<800386bc>] (sys_execve+0x0/0x64) from [<80034ca0>]
(ret_fast_syscall+0x0/0x3c)
[   24.460000]  r7:0000000b r6:00032580 r5:000325fc r4:00000002

HIGHMEM is broken when working in SMP V6 mode

[Russell King - ARM Linux]

On Sun, Jan 23, 2011 at 04:38:01PM +0200, saeed bishara wrote:
> Hi,
> 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.

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.

The kmap code sets up and tears down mappings with IRQs disabled because
of the need to work with the DMA API.  This means that it when it wants
to do a TLB operation (to flush out old mappings) its calling context is
incompatible with what's required to broadcast that operation in
software.

There is no solution to this.  SMP without hardware TLB broadcast is
incompatible with the requirements of highmem.

HIGHMEM is broken when working in SMP V6 mode

[saeed bishara]

On Sun, Jan 23, 2011 at 4:56 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Jan 23, 2011 at 04:38:01PM +0200, saeed bishara wrote:
>> Hi,
>> 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?
saeed

>
> The kmap code sets up and tears down mappings with IRQs disabled because
> of the need to work with the DMA API. ?This means that it when it wants
> to do a TLB operation (to flush out old mappings) its calling context is
> incompatible with what's required to broadcast that operation in
> software.
>
> There is no solution to this. ?SMP without hardware TLB broadcast is
> incompatible with the requirements of highmem.
>

HIGHMEM is broken when working in SMP V6 mode

[Russell King - ARM Linux]

On Sun, Jan 23, 2011 at 06:34:24PM +0200, saeed bishara wrote:
> On Sun, Jan 23, 2011 at 4:56 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Jan 23, 2011 at 04:38:01PM +0200, saeed bishara wrote:
> >> Hi,
> >> 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.

HIGHMEM is broken when working in SMP V6 mode

[saeed bishara]

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

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

HIGHMEM is broken when working in SMP V6 mode

[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.

HIGHMEM is broken when working in SMP V6 mode

[saeed bishara]

On Mon, Jan 24, 2011 at 11:19 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> 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?
no I'm not disabling DMA cache maintenance, this is how the code looks like:
#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())
#endif
                        {
                                pte_t saved_pte;
                                vaddr = kmap_high_l1_vipt(page, &saved_pte);
                                op(vaddr + offset, len, dir);
                                kunmap_high_l1_vipt(page, saved_pte);
                        }

so I'm doing that cache maintenance using new mapping by kmap_high_l1_vipt.

saeed

HIGHMEM is broken when working in SMP V6 mode

[Nicolas Pitre]

On Mon, 24 Jan 2011, 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.

This looks like it should work.

The reason for kmap_high_get() is to ensure that the currently kmap'd 
page usage count does not decrease to zero while we're using its 
existing virtual mapping in an atomic context.  With a VIVT cache this 
is essential to do, but with a VIPT cache this is only an optimization 
so not to pay the price of establishing a second mapping if an existing 
one can be used.

However your patch is ugly.  I'd suggest the following instead:

diff --git a/arch/arm/include/asm/highmem.h b/arch/arm/include/asm/highmem.h
index 7080e2c..be535ad 100644
--- a/arch/arm/include/asm/highmem.h
+++ b/arch/arm/include/asm/highmem.h
@@ -19,13 +19,37 @@
 
 extern pte_t *pkmap_page_table;
 
-#define ARCH_NEEDS_KMAP_HIGH_GET
-
 extern void *kmap_high(struct page *page);
-extern void *kmap_high_get(struct page *page);
 extern void kunmap_high(struct page *page);
 
 /*
+ * The reason for kmap_high_get() is to ensure that the currently kmap'd
+ * page usage count does not decrease to zero while we're using its
+ * existing virtual mapping in an atomic context.  With a VIVT cache this
+ * is essential to do, but with a VIPT cache this is only an optimization
+ * so not to pay the price of establishing a second mapping if an existing
+ * one can be used.  However, on platforms without hardware TLB maintainence
+ * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
+ * the locking involved must also disable IRQs which is incompatible with
+ * the IPI mechanism used by global TLB operations.
+ */
+#define ARCH_NEEDS_KMAP_HIGH_GET
+#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
+#undef ARCH_NEEDS_KMAP_HIGH_GET
+#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
+#error "The sum of feature in your kernel config cannot be supported together"
+#endif
+
+#ifdef ARCH_NEEDS_KMAP_HIGH_GET
+extern void *kmap_high_get(struct page *page);
+#else
+static inline void *kmap_high_get(struct page *page)
+{
+	return NULL;
+}
+#endif
+
+/*
  * The following functions are already defined by <linux/highmem.h>
  * when CONFIG_HIGHMEM is not set.
  */
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 3c67e92..ff7b43b 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -827,16 +827,6 @@ static void __init sanity_check_meminfo(void)
 			 * rather difficult.
 			 */
 			reason = "with VIPT aliasing cache";
-		} else 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";
 		}
 		if (reason) {
 			printk(KERN_CRIT "HIGHMEM is not supported %s, ignoring high memory\n",

HIGHMEM is broken when working in SMP V6 mode

[saeed bishara]

On Mon, Jan 24, 2011 at 9:58 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Mon, 24 Jan 2011, 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.
>
> This looks like it should work.
>
> The reason for kmap_high_get() is to ensure that the currently kmap'd
> page usage count does not decrease to zero while we're using its
> existing virtual mapping in an atomic context. ?With a VIVT cache this
> is essential to do, but with a VIPT cache this is only an optimization
> so not to pay the price of establishing a second mapping if an existing
> one can be used.
>
> However your patch is ugly. ?I'd suggest the following instead:
>
> diff --git a/arch/arm/include/asm/highmem.h b/arch/arm/include/asm/highmem.h
> index 7080e2c..be535ad 100644
> --- a/arch/arm/include/asm/highmem.h
> +++ b/arch/arm/include/asm/highmem.h
> @@ -19,13 +19,37 @@
>
> ?extern pte_t *pkmap_page_table;
>
> -#define ARCH_NEEDS_KMAP_HIGH_GET
> -
> ?extern void *kmap_high(struct page *page);
> -extern void *kmap_high_get(struct page *page);
> ?extern void kunmap_high(struct page *page);
>
> ?/*
> + * The reason for kmap_high_get() is to ensure that the currently kmap'd
> + * page usage count does not decrease to zero while we're using its
> + * existing virtual mapping in an atomic context. ?With a VIVT cache this
> + * is essential to do, but with a VIPT cache this is only an optimization
> + * so not to pay the price of establishing a second mapping if an existing
> + * one can be used. ?However, on platforms without hardware TLB maintainence
> + * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
> + * the locking involved must also disable IRQs which is incompatible with
> + * the IPI mechanism used by global TLB operations.
> + */
> +#define ARCH_NEEDS_KMAP_HIGH_GET
> +#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
> +#undef ARCH_NEEDS_KMAP_HIGH_GET
> +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
> +#error "The sum of feature in your kernel config cannot be supported together"
> +#endif
#endif is missing here.
> +
> +#ifdef ARCH_NEEDS_KMAP_HIGH_GET
> +extern void *kmap_high_get(struct page *page);
> +#else
> +static inline void *kmap_high_get(struct page *page)
> +{
> + ? ? ? return NULL;
> +}
> +#endif
> +
> +/*
> ?* The following functions are already defined by <linux/highmem.h>
> ?* when CONFIG_HIGHMEM is not set.
> ?*/
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 3c67e92..ff7b43b 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -827,16 +827,6 @@ static void __init sanity_check_meminfo(void)
> ? ? ? ? ? ? ? ? ? ? ? ? * rather difficult.
> ? ? ? ? ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ? ? ? ? ?reason = "with VIPT aliasing cache";
> - ? ? ? ? ? ? ? } else 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";
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?if (reason) {
> ? ? ? ? ? ? ? ? ? ? ? ?printk(KERN_CRIT "HIGHMEM is not supported %s, ignoring high memory\n",
>
This patch worked for me, I could boot from sata with DMA and run some
data integrity test while using large part of high memory.
saeed

HIGHMEM is broken when working in SMP V6 mode

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 02:58:07PM -0500, Nicolas Pitre wrote:
>  /*
> + * The reason for kmap_high_get() is to ensure that the currently kmap'd
> + * page usage count does not decrease to zero while we're using its
> + * existing virtual mapping in an atomic context.  With a VIVT cache this
> + * is essential to do, but with a VIPT cache this is only an optimization
> + * so not to pay the price of establishing a second mapping if an existing
> + * one can be used.  However, on platforms without hardware TLB maintainence
> + * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
> + * the locking involved must also disable IRQs which is incompatible with
> + * the IPI mechanism used by global TLB operations.
> + */
> +#define ARCH_NEEDS_KMAP_HIGH_GET
> +#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
> +#undef ARCH_NEEDS_KMAP_HIGH_GET
> +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
> +#error "The sum of feature in your kernel config cannot be supported together"
> +#endif

This is wrong.  Take a moment to consider a kernel supporting an ARMv6
VIPT aliasing cache CPU and ARMv7 SMP.  Don't we need kmap_high_get()
for ARMv6 VIPT aliasing cache?

The effect of this is that dma_cache_maint_page() will create new mappings
which could be have the wrong colour - and therefore the subsequent cache
maintainence will not have the desired effect.

I think this may break OMAP.

HIGHMEM is broken when working in SMP V6 mode

[Nicolas Pitre]

On Thu, 27 Jan 2011, Russell King - ARM Linux wrote:

> On Mon, Jan 24, 2011 at 02:58:07PM -0500, Nicolas Pitre wrote:
> >  /*
> > + * The reason for kmap_high_get() is to ensure that the currently kmap'd
> > + * page usage count does not decrease to zero while we're using its
> > + * existing virtual mapping in an atomic context.  With a VIVT cache this
> > + * is essential to do, but with a VIPT cache this is only an optimization
> > + * so not to pay the price of establishing a second mapping if an existing
> > + * one can be used.  However, on platforms without hardware TLB maintainence
> > + * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
> > + * the locking involved must also disable IRQs which is incompatible with
> > + * the IPI mechanism used by global TLB operations.
> > + */
> > +#define ARCH_NEEDS_KMAP_HIGH_GET
> > +#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
> > +#undef ARCH_NEEDS_KMAP_HIGH_GET
> > +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
> > +#error "The sum of feature in your kernel config cannot be supported together"
> > +#endif
> 
> This is wrong.  Take a moment to consider a kernel supporting an ARMv6
> VIPT aliasing cache CPU and ARMv7 SMP.  Don't we need kmap_high_get()
> for ARMv6 VIPT aliasing cache?

We don't support highmem on aliasing VIPT.  Highmem gets disabled at run 
time on aliasing VIPT platforms.


Nicolas

HIGHMEM is broken when working in SMP V6 mode

[Russell King - ARM Linux]

On Thu, Jan 27, 2011 at 01:40:37PM -0500, Nicolas Pitre wrote:
> On Thu, 27 Jan 2011, Russell King - ARM Linux wrote:
> 
> > On Mon, Jan 24, 2011 at 02:58:07PM -0500, Nicolas Pitre wrote:
> > >  /*
> > > + * The reason for kmap_high_get() is to ensure that the currently kmap'd
> > > + * page usage count does not decrease to zero while we're using its
> > > + * existing virtual mapping in an atomic context.  With a VIVT cache this
> > > + * is essential to do, but with a VIPT cache this is only an optimization
> > > + * so not to pay the price of establishing a second mapping if an existing
> > > + * one can be used.  However, on platforms without hardware TLB maintainence
> > > + * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
> > > + * the locking involved must also disable IRQs which is incompatible with
> > > + * the IPI mechanism used by global TLB operations.
> > > + */
> > > +#define ARCH_NEEDS_KMAP_HIGH_GET
> > > +#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
> > > +#undef ARCH_NEEDS_KMAP_HIGH_GET
> > > +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
> > > +#error "The sum of feature in your kernel config cannot be supported together"
> > > +#endif
> > 
> > This is wrong.  Take a moment to consider a kernel supporting an ARMv6
> > VIPT aliasing cache CPU and ARMv7 SMP.  Don't we need kmap_high_get()
> > for ARMv6 VIPT aliasing cache?
> 
> We don't support highmem on aliasing VIPT.  Highmem gets disabled at run 
> time on aliasing VIPT platforms.

Correct.  But a kernel which has this configuration:

CONFIG_SMP=y
CONFIG_HIGHMEM=y
CONFIG_CPU_V6=y
CONFIG_CPU_V7=y
CONFIG_CPU_32v6=y
CONFIG_CPU_32v7=y
CONFIG_CPU_CACHE_V6=y
CONFIG_CPU_CACHE_VIPT=y
CONFIG_CPU_CACHE_V7=y
CONFIG_CPU_CACHE_VIPT=y
CONFIG_CPU_TLB_V6=y
CONFIG_CPU_TLB_V7=y

This disables ARCH_NEEDS_KMAP_HIGH_GET in this kernel, which can be used on
ARMv6 VIPT non-aliasing, VIPT aliasing, as well as SMP stuff.

If we don't need ARCH_NEEDS_KMAP_HIGH_GET on V6 and V7, we should just not
enable it for those at all.

HIGHMEM is broken when working in SMP V6 mode

[Nicolas Pitre]

On Thu, 27 Jan 2011, Russell King - ARM Linux wrote:

> On Thu, Jan 27, 2011 at 01:40:37PM -0500, Nicolas Pitre wrote:
> > On Thu, 27 Jan 2011, Russell King - ARM Linux wrote:
> > 
> > > On Mon, Jan 24, 2011 at 02:58:07PM -0500, Nicolas Pitre wrote:
> > > >  /*
> > > > + * The reason for kmap_high_get() is to ensure that the currently kmap'd
> > > > + * page usage count does not decrease to zero while we're using its
> > > > + * existing virtual mapping in an atomic context.  With a VIVT cache this
> > > > + * is essential to do, but with a VIPT cache this is only an optimization
> > > > + * so not to pay the price of establishing a second mapping if an existing
> > > > + * one can be used.  However, on platforms without hardware TLB maintainence
> > > > + * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
> > > > + * the locking involved must also disable IRQs which is incompatible with
> > > > + * the IPI mechanism used by global TLB operations.
> > > > + */
> > > > +#define ARCH_NEEDS_KMAP_HIGH_GET
> > > > +#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
> > > > +#undef ARCH_NEEDS_KMAP_HIGH_GET
> > > > +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
> > > > +#error "The sum of feature in your kernel config cannot be supported together"
> > > > +#endif
> > > 
> > > This is wrong.  Take a moment to consider a kernel supporting an ARMv6
> > > VIPT aliasing cache CPU and ARMv7 SMP.  Don't we need kmap_high_get()
> > > for ARMv6 VIPT aliasing cache?
> > 
> > We don't support highmem on aliasing VIPT.  Highmem gets disabled at run 
> > time on aliasing VIPT platforms.
> 
> Correct.  But a kernel which has this configuration:
> 
> CONFIG_SMP=y
> CONFIG_HIGHMEM=y
> CONFIG_CPU_V6=y
> CONFIG_CPU_V7=y
> CONFIG_CPU_32v6=y
> CONFIG_CPU_32v7=y
> CONFIG_CPU_CACHE_V6=y
> CONFIG_CPU_CACHE_VIPT=y
> CONFIG_CPU_CACHE_V7=y
> CONFIG_CPU_CACHE_VIPT=y
> CONFIG_CPU_TLB_V6=y
> CONFIG_CPU_TLB_V7=y
> 
> This disables ARCH_NEEDS_KMAP_HIGH_GET in this kernel, which can be used on
> ARMv6 VIPT non-aliasing, VIPT aliasing, as well as SMP stuff.

Yes.

On a VIPT aliasing target, highmem gets disabled at run time.  So no 
issue there.

Otherwise, on VIPT, it is not essential to have 
ARCH_NEEDS_KMAP_HIGH_GET.

> If we don't need ARCH_NEEDS_KMAP_HIGH_GET on V6 and V7, we should just not
> enable it for those at all.

As I said in the patch comment, ARCH_NEEDS_KMAP_HIGH_GET is still a nice 
optimization on VIPT. It avoids the creation of a second mapping and TLB 
usage when an existing mapping from the pkmap array can be reused right 
away.  But if our kernel config indicates that we might run on a target 
lacking hw TLB broadcast then we have to compromize and give up on that 
optimization.


Nicolas