[PATCH] highmem: fix highmem for xtensa

[PATCH] highmem: fix highmem for xtensa

[Max Filippov]

Fixmap on xtensa grows upwards, i.e. bigger fixmap entry index
corresponds to a higher virtual address. This was lost in highmem
generalization resulting in the following runtime warnings:

 WARNING: CPU: 0 PID: 18 at mm/highmem.c:494 kunmap_local_indexed+0x45/0x54
 Modules linked in:
 CPU: 0 PID: 18 Comm: kworker/u2:0 Not tainted 5.10.0-rc3-next-20201113 #1
 Call Trace:
   __warn+0x8f/0xc8
   warn_slowpath_fmt+0x35/0x70
   kunmap_local_indexed+0x45/0x54
   handle_mm_fault+0x325/0xbe0
   __get_user_pages.part.61+0x131/0x22c
   __get_user_pages+0x44/0x60
   __get_user_pages_remote+0xe8/0x290
   get_user_pages_remote+0x24/0x40
   get_arg_page+0x50/0x78
   copy_string_kernel+0x5c/0x120
   kernel_execve+0x76/0xc8
   call_usermodehelper_exec_async+0xc8/0x10c
   ret_from_kernel_thread+0xc/0x18

Fix it by adding __ARCH_HAS_POSITIVE_FIXMAP macro and implementing
vaddr_in_fixmap and fixmap_pte primitives differently depending on
whether it is defined or not.

Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 629ed3f7dad2 ("xtensa/mm/highmem: Switch to generic kmap atomic")
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 arch/xtensa/include/asm/fixmap.h |  2 ++
 mm/highmem.c                     | 29 ++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/arch/xtensa/include/asm/fixmap.h b/arch/xtensa/include/asm/fixmap.h
index 92049b61c351..66787b5f13d6 100644
--- a/arch/xtensa/include/asm/fixmap.h
+++ b/arch/xtensa/include/asm/fixmap.h
@@ -51,6 +51,8 @@ enum fixed_addresses {
 #define __fix_to_virt(x)	(FIXADDR_START + ((x) << PAGE_SHIFT))
 #define __virt_to_fix(x)	(((x) - FIXADDR_START) >> PAGE_SHIFT)
 
+#define __ARCH_HAS_POSITIVE_FIXMAP
+
 #ifndef __ASSEMBLY__
 /*
  * 'index to address' translation. If anyone tries to use the idx
diff --git a/mm/highmem.c b/mm/highmem.c
index 54bd233846c9..af27ed8d6a97 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -434,6 +434,26 @@ static inline void kmap_high_unmap_local(unsigned long vaddr)
 #endif
 }
 
+static inline bool vaddr_in_fixmap(unsigned long addr)
+{
+#ifdef __ARCH_HAS_POSITIVE_FIXMAP
+	return addr <= __fix_to_virt(FIX_KMAP_END) &&
+		addr >= __fix_to_virt(FIX_KMAP_BEGIN);
+#else
+	return addr >= __fix_to_virt(FIX_KMAP_END) &&
+		addr <= __fix_to_virt(FIX_KMAP_BEGIN);
+#endif
+}
+
+static pte_t *fixmap_pte(pte_t *kmap_pte, int idx)
+{
+#ifdef __ARCH_HAS_POSITIVE_FIXMAP
+	return kmap_pte + idx;
+#else
+	return kmap_pte - idx;
+#endif
+}
+
 static inline int kmap_local_calc_idx(int idx)
 {
 	return idx + KM_MAX_IDX * smp_processor_id();
@@ -457,9 +477,9 @@ void *__kmap_local_pfn_prot(unsigned long pfn, pgprot_t prot)
 	preempt_disable();
 	idx = arch_kmap_local_map_idx(kmap_local_idx_push(), pfn);
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
-	BUG_ON(!pte_none(*(kmap_pte - idx)));
+	BUG_ON(!pte_none(*(fixmap_pte(kmap_pte, idx))));
 	pteval = pfn_pte(pfn, prot);
-	set_pte_at(&init_mm, vaddr, kmap_pte - idx, pteval);
+	set_pte_at(&init_mm, vaddr, fixmap_pte(kmap_pte, idx), pteval);
 	arch_kmap_local_post_map(vaddr, pteval);
 	preempt_enable();
 
@@ -489,8 +509,7 @@ void kunmap_local_indexed(void *vaddr)
 	pte_t *kmap_pte = kmap_get_pte();
 	int idx;
 
-	if (addr < __fix_to_virt(FIX_KMAP_END) ||
-	    addr > __fix_to_virt(FIX_KMAP_BEGIN)) {
+	if (!vaddr_in_fixmap(addr)) {
 		WARN_ON_ONCE(addr < PAGE_OFFSET);
 
 		/* Handle mappings which were obtained by kmap_high_get() */
@@ -503,7 +522,7 @@ void kunmap_local_indexed(void *vaddr)
 	WARN_ON_ONCE(addr != __fix_to_virt(FIX_KMAP_BEGIN + idx));
 
 	arch_kmap_local_pre_unmap(addr);
-	pte_clear(&init_mm, addr, kmap_pte - idx);
+	pte_clear(&init_mm, addr, fixmap_pte(kmap_pte, idx));
 	arch_kmap_local_post_unmap(addr);
 	kmap_local_idx_pop();
 	preempt_enable();
-- 
2.20.1

Re: [PATCH] highmem: fix highmem for xtensa

[Thomas Gleixner]

Max,

On Fri, Nov 13 2020 at 04:23, Max Filippov wrote:
> Fixmap on xtensa grows upwards, i.e. bigger fixmap entry index
> corresponds to a higher virtual address. This was lost in highmem
> generalization resulting in the following runtime warnings:

Sorry for not noticing.

> Fix it by adding __ARCH_HAS_POSITIVE_FIXMAP macro and implementing
> vaddr_in_fixmap and fixmap_pte primitives differently depending on
> whether it is defined or not.

What's wrong with just doing the obvious and making the fixmap defines
the other way round?

Thanks,

        tglx
---

diff --git a/arch/xtensa/include/asm/fixmap.h b/arch/xtensa/include/asm/fixmap.h
index 92049b61c351..ee17e46f3b44 100644
--- a/arch/xtensa/include/asm/fixmap.h
+++ b/arch/xtensa/include/asm/fixmap.h
@@ -37,8 +37,8 @@
 enum fixed_addresses {
 #ifdef CONFIG_HIGHMEM
 	/* reserved pte's for temporary kernel mappings */
-	FIX_KMAP_BEGIN,
-	FIX_KMAP_END = FIX_KMAP_BEGIN +
+	FIX_KMAP_END,
+	FIX_KMAP_BEGIN = FIX_KMAP_END +
 		(KM_MAX_IDX * NR_CPUS * DCACHE_N_COLORS) - 1,
 #endif
 	__end_of_fixed_addresses

Re: [PATCH] highmem: fix highmem for xtensa

[Max Filippov]

Hi Thomas,

On Fri, Nov 13, 2020 at 5:40 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, Nov 13 2020 at 04:23, Max Filippov wrote:
> > Fixmap on xtensa grows upwards, i.e. bigger fixmap entry index
> > corresponds to a higher virtual address. This was lost in highmem
> > generalization resulting in the following runtime warnings:
>
> Sorry for not noticing.
>
> > Fix it by adding __ARCH_HAS_POSITIVE_FIXMAP macro and implementing
> > vaddr_in_fixmap and fixmap_pte primitives differently depending on
> > whether it is defined or not.
>
> What's wrong with just doing the obvious and making the fixmap defines
> the other way round?

It becomes really awkward when we get to support high memory with
aliasing data cache: we must think about the actual virtual addresses
assigned to pages and it feels much simpler when it's done this way.

-- 
Thanks.
-- Max

Re: [PATCH] highmem: fix highmem for xtensa

[Thomas Gleixner]

Max,

On Fri, Nov 13 2020 at 05:50, Max Filippov wrote:
> On Fri, Nov 13, 2020 at 5:40 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Fri, Nov 13 2020 at 04:23, Max Filippov wrote:
>> > Fixmap on xtensa grows upwards, i.e. bigger fixmap entry index
>> > corresponds to a higher virtual address. This was lost in highmem
>> > generalization resulting in the following runtime warnings:
>>
>> Sorry for not noticing.
>>
>> > Fix it by adding __ARCH_HAS_POSITIVE_FIXMAP macro and implementing
>> > vaddr_in_fixmap and fixmap_pte primitives differently depending on
>> > whether it is defined or not.
>>
>> What's wrong with just doing the obvious and making the fixmap defines
>> the other way round?
>
> It becomes really awkward when we get to support high memory with
> aliasing data cache: we must think about the actual virtual addresses
> assigned to pages and it feels much simpler when it's done this way.

Feeling are not really a technical argument. Is there any functional
difference which matters?

Thanks,

        tglx

Re: [PATCH] highmem: fix highmem for xtensa

[Max Filippov]

On Fri, Nov 13, 2020 at 6:36 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, Nov 13 2020 at 05:50, Max Filippov wrote:
> > On Fri, Nov 13, 2020 at 5:40 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> What's wrong with just doing the obvious and making the fixmap defines
> >> the other way round?
> >
> > It becomes really awkward when we get to support high memory with
> > aliasing data cache: we must think about the actual virtual addresses
> > assigned to pages and it feels much simpler when it's done this way.
>
> Feeling are not really a technical argument. Is there any functional
> difference which matters?

arch_kmap_local_map_idx must produce index based on type and
pfn that will be translated to virtual address with the same color this
page would've had if it was in the low memory. With positive fixmap
the formula is: (type * (number of cache colors)) + (color of the pfn).
With negative fixmap there must be additional +1 and -1 in it.

-- 
Thanks.
-- Max

Re: [PATCH] highmem: fix highmem for xtensa

[Thomas Gleixner]

Max,

On Fri, Nov 13 2020 at 08:34, Max Filippov wrote:
> On Fri, Nov 13, 2020 at 6:36 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Fri, Nov 13 2020 at 05:50, Max Filippov wrote:
>> > On Fri, Nov 13, 2020 at 5:40 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> What's wrong with just doing the obvious and making the fixmap defines
>> >> the other way round?
>> >
>> > It becomes really awkward when we get to support high memory with
>> > aliasing data cache: we must think about the actual virtual addresses
>> > assigned to pages and it feels much simpler when it's done this way.
>>
>> Feeling are not really a technical argument. Is there any functional
>> difference which matters?
>
> arch_kmap_local_map_idx must produce index based on type and
> pfn that will be translated to virtual address with the same color this
> page would've had if it was in the low memory. With positive fixmap
> the formula is: (type * (number of cache colors)) + (color of the pfn).
> With negative fixmap there must be additional +1 and -1 in it.

I agree that the top down mechanics is not really intuitive, but that
does not justify the ifdeffery in the generic code.

xtensa can just use the generic fix_to_virt/virt_to_fix mechanics. All
it needs is to adjust the mapping defines and to adjust the color offset
to

        NR_COLORS - color
        
which is not an unreasonable ask. As a side effect all highmem inflicted
systems which do not have the cache aliasing problem can just use the
generic code as is. See untested patch below.

It builds for some configs, but the smp_lx200_defconfig (which has the
aliasing) it fails to build even without this patch (highmem.o at least
builds).

Toolchain is the one from https://mirrors.edge.kernel.org/pub/tools/crosstool/

Thanks,

        tglx
---
Subject: xtensa/mm/highmem: Make generic kmap_atomic() work correctly
From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 13 Nov 2020 21:25:12 +0100

The conversion to the generic kmap_atomic() implementation missed the fact
that xtensa's fixmap works bottom up while all other implementations work
top down. There is no real reason why xtensa needs to work that way.

Cure it by:

  - Using the generic fix_to_virt()/virt_to_fix() functions which work top
    down
  - Adjusting the mapping defines
  - Using the generic index calculation for the non cache aliasing case
  - Making the cache colour offset reverse so the effective index is correct

While at it, remove the outdated and misleading comment above the fixmap
enum which originates from the initial copy&pasta of this code from i386.

Reported-by: Max Filippov <jcmvbkbc@gmail.com>
Fixes: 629ed3f7dad2 ("xtensa/mm/highmem: Switch to generic kmap atomic")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/xtensa/include/asm/fixmap.h  |   55 ++++----------------------------------
 arch/xtensa/include/asm/highmem.h |   15 ++++++----
 arch/xtensa/mm/highmem.c          |   18 +++++++-----
 arch/xtensa/mm/init.c             |    4 +-
 arch/xtensa/mm/mmu.c              |    3 +-
 5 files changed, 31 insertions(+), 64 deletions(-)

--- a/arch/xtensa/include/asm/fixmap.h
+++ b/arch/xtensa/include/asm/fixmap.h
@@ -17,63 +17,22 @@
 #include <linux/threads.h>
 #include <linux/pgtable.h>
 #include <asm/kmap_size.h>
-#endif
 
-/*
- * Here we define all the compile-time 'special' virtual
- * addresses. The point is to have a constant address at
- * compile time, but to set the physical address only
- * in the boot process. We allocate these special addresses
- * from the start of the consistent memory region upwards.
- * Also this lets us do fail-safe vmalloc(), we
- * can guarantee that these special addresses and
- * vmalloc()-ed addresses never overlap.
- *
- * these 'compile-time allocated' memory buffers are
- * fixed-size 4k pages. (or larger if used with an increment
- * higher than 1) use fixmap_set(idx,phys) to associate
- * physical memory with fixmap indices.
- */
+/* The map slots for temporary mappings via kmap_atomic/local(). */
 enum fixed_addresses {
-#ifdef CONFIG_HIGHMEM
-	/* reserved pte's for temporary kernel mappings */
 	FIX_KMAP_BEGIN,
 	FIX_KMAP_END = FIX_KMAP_BEGIN +
 		(KM_MAX_IDX * NR_CPUS * DCACHE_N_COLORS) - 1,
-#endif
 	__end_of_fixed_addresses
 };
 
-#define FIXADDR_TOP     (XCHAL_KSEG_CACHED_VADDR - PAGE_SIZE)
+#define FIXADDR_END     (XCHAL_KSEG_CACHED_VADDR - PAGE_SIZE)
 #define FIXADDR_SIZE	(__end_of_fixed_addresses << PAGE_SHIFT)
-#define FIXADDR_START	((FIXADDR_TOP - FIXADDR_SIZE) & PMD_MASK)
-
-#define __fix_to_virt(x)	(FIXADDR_START + ((x) << PAGE_SHIFT))
-#define __virt_to_fix(x)	(((x) - FIXADDR_START) >> PAGE_SHIFT)
+/* Enforce that FIXADDR_START is PMD aligned to handle cache aliasing */
+#define FIXADDR_START	((FIXADDR_END - FIXADDR_SIZE) & PMD_MASK)
+#define FIXADDR_TOP	(FIXADDR_START + FIXADDR_SIZE - PAGE_SIZE)
 
-#ifndef __ASSEMBLY__
-/*
- * 'index to address' translation. If anyone tries to use the idx
- * directly without translation, we catch the bug with a NULL-deference
- * kernel oops. Illegal ranges of incoming indices are caught too.
- */
-static __always_inline unsigned long fix_to_virt(const unsigned int idx)
-{
-	/* Check if this memory layout is broken because fixmap overlaps page
-	 * table.
-	 */
-	BUILD_BUG_ON(FIXADDR_START <
-		     TLBTEMP_BASE_1 + TLBTEMP_SIZE);
-	BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
-	return __fix_to_virt(idx);
-}
-
-static inline unsigned long virt_to_fix(const unsigned long vaddr)
-{
-	BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
-	return __virt_to_fix(vaddr);
-}
-
-#endif
+#include <asm-generic/fixmap.h>
 
+#endif /* CONFIG_HIGHMEM */
 #endif
--- a/arch/xtensa/include/asm/highmem.h
+++ b/arch/xtensa/include/asm/highmem.h
@@ -12,6 +12,7 @@
 #ifndef _XTENSA_HIGHMEM_H
 #define _XTENSA_HIGHMEM_H
 
+#ifdef CONFIG_HIGHMEM
 #include <linux/wait.h>
 #include <linux/pgtable.h>
 #include <asm/cacheflush.h>
@@ -58,6 +59,13 @@ static inline wait_queue_head_t *get_pkm
 {
 	return pkmap_map_wait_arr + color;
 }
+
+enum fixed_addresses kmap_local_map_idx(int type, unsigned long pfn);
+#define arch_kmap_local_map_idx		kmap_local_map_idx
+
+enum fixed_addresses kmap_local_unmap_idx(int type, unsigned long addr);
+#define arch_kmap_local_unmap_idx	kmap_local_unmap_idx
+
 #endif
 
 extern pte_t *pkmap_page_table;
@@ -67,15 +75,10 @@ static inline void flush_cache_kmaps(voi
 	flush_cache_all();
 }
 
-enum fixed_addresses kmap_local_map_idx(int type, unsigned long pfn);
-#define arch_kmap_local_map_idx		kmap_local_map_idx
-
-enum fixed_addresses kmap_local_unmap_idx(int type, unsigned long addr);
-#define arch_kmap_local_unmap_idx	kmap_local_unmap_idx
-
 #define arch_kmap_local_post_unmap(vaddr)	\
 	local_flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE)
 
 void kmap_init(void);
 
+#endif /* CONFIG_HIGHMEM */
 #endif
--- a/arch/xtensa/mm/highmem.c
+++ b/arch/xtensa/mm/highmem.c
@@ -23,16 +23,16 @@ static void __init kmap_waitqueues_init(
 	for (i = 0; i < ARRAY_SIZE(pkmap_map_wait_arr); ++i)
 		init_waitqueue_head(pkmap_map_wait_arr + i);
 }
-#else
-static inline void kmap_waitqueues_init(void)
-{
-}
-#endif
 
 static inline enum fixed_addresses kmap_idx(int type, unsigned long color)
 {
-	return (type + KM_MAX_IDX * smp_processor_id()) * DCACHE_N_COLORS +
-		color;
+	int idx = (type + KM_MAX_IDX * smp_processor_id()) * DCACHE_N_COLORS;
+
+	/*
+	 * The fixmap operates top down, so the color offset needs to be
+	 * reverse as well.
+	 */
+	return idx + DCACHE_N_COLORS - color;
 }
 
 enum fixed_addresses kmap_local_map_idx(int type, unsigned long pfn)
@@ -45,6 +45,10 @@ enum fixed_addresses kmap_local_unmap_id
 	return kmap_idx(type, DCACHE_ALIAS(addr));
 }
 
+#else
+static inline void kmap_waitqueues_init(void) { }
+#endif
+
 void __init kmap_init(void)
 {
 	/* Check if this memory layout is broken because PKMAP overlaps
--- a/arch/xtensa/mm/init.c
+++ b/arch/xtensa/mm/init.c
@@ -147,8 +147,8 @@ void __init mem_init(void)
 #ifdef CONFIG_HIGHMEM
 		PKMAP_BASE, PKMAP_BASE + LAST_PKMAP * PAGE_SIZE,
 		(LAST_PKMAP*PAGE_SIZE) >> 10,
-		FIXADDR_START, FIXADDR_TOP,
-		(FIXADDR_TOP - FIXADDR_START) >> 10,
+		FIXADDR_START, FIXADDR_END,
+		(FIXADDR_END - FIXADDR_START) >> 10,
 #endif
 		PAGE_OFFSET, PAGE_OFFSET +
 		(max_low_pfn - min_low_pfn) * PAGE_SIZE,
--- a/arch/xtensa/mm/mmu.c
+++ b/arch/xtensa/mm/mmu.c
@@ -52,7 +52,8 @@ static void * __init init_pmd(unsigned l
 
 static void __init fixedrange_init(void)
 {
-	init_pmd(__fix_to_virt(0), __end_of_fixed_addresses);
+	BUILD_BUG_ON(FIXADDR_START < TLBTEMP_BASE_1 + TLBTEMP_SIZE);
+	init_pmd(FIXADDR_START, __end_of_fixed_addresses);
 }
 #endif
 

Re: [PATCH] highmem: fix highmem for xtensa

[Max Filippov]

Hi Thomas,

On Fri, Nov 13, 2020 at 2:34 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> I agree that the top down mechanics is not really intuitive, but that
> does not justify the ifdeffery in the generic code.

But then maybe xtensa did the right thing where everyone else just
copied the not really intuitive implementation? If nobody else cares
then maybe generic fix_to_virt/virt_to_fix can be changed for positive
indexing?

> xtensa can just use the generic fix_to_virt/virt_to_fix mechanics. All
> it needs is to adjust the mapping defines and to adjust the color offset
> to
>
>         NR_COLORS - color
>
> which is not an unreasonable ask. As a side effect all highmem inflicted
> systems which do not have the cache aliasing problem can just use the
> generic code as is. See untested patch below.

Thanks. I'll test this patch and post the result.
But still this change doesn't look like a step in the right direction to me:
I can't find the reason why fixmap must be indexed backwards.

> It builds for some configs, but the smp_lx200_defconfig (which has the
> aliasing) it fails to build even without this patch (highmem.o at least
> builds).
>
> Toolchain is the one from https://mirrors.edge.kernel.org/pub/tools/crosstool/

xtensa toolchain must match the selected CPU core. For smp_lx200_defconfig
the toolchain is available here:
https://github.com/foss-xtensa/toolchain/releases/download/2020.07/x86_64-2020.07-xtensa-test_mmuhifi_c3-elf.tar.gz

-- 
Thanks.
-- Max

Re: [PATCH] highmem: fix highmem for xtensa

[Thomas Gleixner]

Max,

On Mon, Nov 16 2020 at 06:47, Max Filippov wrote:
> On Fri, Nov 13, 2020 at 2:34 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> I agree that the top down mechanics is not really intuitive, but that
>> does not justify the ifdeffery in the generic code.
>
> But then maybe xtensa did the right thing where everyone else just
> copied the not really intuitive implementation? If nobody else cares
> then maybe generic fix_to_virt/virt_to_fix can be changed for positive
> indexing?

Which requires to change 9 architectures instead of one for a feature
which is barely used nowadays and which we rather want to get rid of.

Thanks,

        tglx