[PATCH 0/7] Fix Keystone 2 physical address switch

[PATCH 0/7] Fix Keystone 2 physical address switch

[Russell King - ARM Linux]

For some time, the Keystone 2 physical address switch has been
tainting the kernel ever since we decided that it was not safe.

This series re-works the address switch to be compliant architecturally.

There's still a niggle in the keystone2 code, where we set the
arch_virt_to_idmap function pointer in keystone_pv_fixup() - something
I'd ideally like to avoid.

I'm in two minds about how much of this code should be in SoC specific
code, and how much should be in generic code - on one hand, I don't
want to give a green light to this kind of sillyness by encouraging
it, but I'd rather not end up with multiple implementations of this.

 arch/arm/include/asm/mach/arch.h  |   2 +-
 arch/arm/kernel/setup.c           |   7 +-
 arch/arm/mach-keystone/keystone.c |  41 +++++------
 arch/arm/mm/Kconfig               |   4 ++
 arch/arm/mm/Makefile              |   1 +
 arch/arm/mm/mmu.c                 | 148 +++++++++++++++-----------------------
 arch/arm/mm/nommu.c               |   9 ---
 arch/arm/mm/pv-fixup-asm.S        |  88 +++++++++++++++++++++++
 8 files changed, 174 insertions(+), 126 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/6] ARM: keystone2: move platform notifier initialisation into platform init

[Russell King]

We ideally want the init_meminfo function to do nothing but return the
delta to be applied to PHYS_OFFSET - it should do nothing else.  As we
can detect in platform init code whether we are running in the high
physical address space, move the platform notifier initialisation
entirely into the platform init code.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-keystone/keystone.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index 06620875813a..3d58a8f4dc7e 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -27,7 +27,6 @@
 
 #include "keystone.h"
 
-static struct notifier_block platform_nb;
 static unsigned long keystone_dma_pfn_offset __read_mostly;
 
 static int keystone_platform_notifier(struct notifier_block *nb,
@@ -49,11 +48,18 @@ static int keystone_platform_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static struct notifier_block platform_nb = {
+	.notifier_call = keystone_platform_notifier,
+};
+
 static void __init keystone_init(void)
 {
-	keystone_pm_runtime_init();
-	if (platform_nb.notifier_call)
+	if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) {
+		keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
+						   KEYSTONE_LOW_PHYS_START);
 		bus_register_notifier(&platform_bus_type, &platform_nb);
+	}
+	keystone_pm_runtime_init();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
@@ -96,9 +102,6 @@ static void __init keystone_init_meminfo(void)
 
 	/* Populate the arch idmap hook */
 	arch_virt_to_idmap = keystone_virt_to_idmap;
-	platform_nb.notifier_call = keystone_platform_notifier;
-	keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
-						KEYSTONE_LOW_PHYS_START);
 
 	pr_info("Switching to high address space at 0x%llx\n", (u64)offset);
 }
-- 
1.8.3.1

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Russell King]

Make the init_meminfo function return the offset to be applied to the
phys-to-virt translation constants.  This allows us to move the update
into generic code, along with the requirements for this update.

This avoids platforms having to know the details of the phys-to-virt
translation support.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/mach/arch.h  |  2 +-
 arch/arm/mach-keystone/keystone.c | 27 ++++++++++-----------------
 arch/arm/mm/mmu.c                 | 26 ++++++++++++++++++++++----
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 0406cb3f1af7..e881913f7c3e 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -51,7 +51,7 @@ struct machine_desc {
 	bool			(*smp_init)(void);
 	void			(*fixup)(struct tag *, char **);
 	void			(*dt_fixup)(void);
-	void			(*init_meminfo)(void);
+	long long		(*init_meminfo)(void);
 	void			(*reserve)(void);/* reserve mem blocks	*/
 	void			(*map_io)(void);/* IO mapping function	*/
 	void			(*init_early)(void);
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index 3d58a8f4dc7e..baa0fbc9803a 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -68,11 +68,9 @@ static phys_addr_t keystone_virt_to_idmap(unsigned long x)
 	return (phys_addr_t)(x) - CONFIG_PAGE_OFFSET + KEYSTONE_LOW_PHYS_START;
 }
 
-static void __init keystone_init_meminfo(void)
+static long long __init keystone_init_meminfo(void)
 {
-	bool lpae = IS_ENABLED(CONFIG_ARM_LPAE);
-	bool pvpatch = IS_ENABLED(CONFIG_ARM_PATCH_PHYS_VIRT);
-	phys_addr_t offset = PHYS_OFFSET - KEYSTONE_LOW_PHYS_START;
+	long long offset;
 	phys_addr_t mem_start, mem_end;
 
 	mem_start = memblock_start_of_DRAM();
@@ -81,29 +79,24 @@ static void __init keystone_init_meminfo(void)
 	/* nothing to do if we are running out of the <32-bit space */
 	if (mem_start >= KEYSTONE_LOW_PHYS_START &&
 	    mem_end   <= KEYSTONE_LOW_PHYS_END)
-		return;
-
-	if (!lpae || !pvpatch) {
-		pr_crit("Enable %s%s%s to run outside 32-bit space\n",
-		      !lpae ? __stringify(CONFIG_ARM_LPAE) : "",
-		      (!lpae && !pvpatch) ? " and " : "",
-		      !pvpatch ? __stringify(CONFIG_ARM_PATCH_PHYS_VIRT) : "");
-	}
+		return 0;
 
 	if (mem_start < KEYSTONE_HIGH_PHYS_START ||
 	    mem_end   > KEYSTONE_HIGH_PHYS_END) {
 		pr_crit("Invalid address space for memory (%08llx-%08llx)\n",
-		      (u64)mem_start, (u64)mem_end);
+		        (u64)mem_start, (u64)mem_end);
+		return 0;
 	}
 
-	offset += KEYSTONE_HIGH_PHYS_START;
-	__pv_phys_pfn_offset = PFN_DOWN(offset);
-	__pv_offset = (offset - PAGE_OFFSET);
+	offset = KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START;
 
 	/* Populate the arch idmap hook */
 	arch_virt_to_idmap = keystone_virt_to_idmap;
 
-	pr_info("Switching to high address space at 0x%llx\n", (u64)offset);
+	pr_info("Switching to high address space at 0x%llx\n",
+	        (u64)PHYS_OFFSET + (u64)offset);
+
+	return offset;
 }
 
 static const char *const keystone_match[] __initconst = {
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 4e6ef896c619..71563fdf298c 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1387,7 +1387,7 @@ static void __init map_lowmem(void)
 	}
 }
 
-#ifdef CONFIG_ARM_LPAE
+#if defined(CONFIG_ARM_LPAE) && defined(CONFIG_ARM_PATCH_PHYS_VIRT)
 /*
  * early_paging_init() recreates boot time page table setup, allowing machines
  * to switch over to a high (>4G) address space on LPAE systems
@@ -1397,6 +1397,7 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 {
 	pmdval_t pmdprot = procinfo->__cpu_mm_mmu_flags;
 	unsigned long map_start, map_end;
+	long long offset;
 	pgd_t *pgd0, *pgdk;
 	pud_t *pud0, *pudk, *pud_start;
 	pmd_t *pmd0, *pmdk;
@@ -1419,7 +1420,13 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 	pudk = pud_offset(pgdk, map_start);
 	pmdk = pmd_offset(pudk, map_start);
 
-	mdesc->init_meminfo();
+	offset = mdesc->init_meminfo();
+	if (offset == 0)
+		return;
+
+	/* Re-set the phys pfn offset, and the pv offset */
+	__pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET;
+	__pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset);
 
 	/* Run the patch stub to update the constants */
 	fixup_pv_table(&__pv_table_begin,
@@ -1502,8 +1509,19 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 void __init early_paging_init(const struct machine_desc *mdesc,
 			      struct proc_info_list *procinfo)
 {
-	if (mdesc->init_meminfo)
-		mdesc->init_meminfo();
+	long long offset;
+
+	if (!mdesc->init_meminfo)
+		return;
+
+	offset = mdesc->init_meminfo();
+	if (offset == 0)
+		return;
+
+	pr_crit("Physical address space modification is only to support Keystone2.\n");
+	pr_crit("Please enable ARM_LPAE and ARM_PATCH_PHYS_VIRT support to use this\n");
+	pr_crit("feature. Your kernel may crash now, have a good day.\n");
+	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
 }
 
 #endif
-- 
1.8.3.1

[PATCH 3/6] ARM: keystone2: move address space switch printk to generic code

[Russell King]

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-keystone/keystone.c | 3 ---
 arch/arm/mm/mmu.c                 | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index baa0fbc9803a..af8c92bc8188 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -93,9 +93,6 @@ static long long __init keystone_init_meminfo(void)
 	/* Populate the arch idmap hook */
 	arch_virt_to_idmap = keystone_virt_to_idmap;
 
-	pr_info("Switching to high address space at 0x%llx\n",
-	        (u64)PHYS_OFFSET + (u64)offset);
-
 	return offset;
 }
 
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 71563fdf298c..657b50085942 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1424,6 +1424,9 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 	if (offset == 0)
 		return;
 
+	pr_info("Switching physical address space to 0x%08llx\n",
+		(u64)PHYS_OFFSET + offset);
+
 	/* Re-set the phys pfn offset, and the pv offset */
 	__pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET;
 	__pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset);
-- 
1.8.3.1

[PATCH 4/6] ARM: keystone2: rename init_meminfo to pv_fixup

[Russell King]

The init_meminfo() method is not about initialising meminfo - it's about
fixing up the physical to virtual translation so that we use a different
physical address space, possibly above the 4GB physical address space.
Therefore, the name "init_meminfo()" is confusing.

Rename it to pv_fixup() instead.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/mach/arch.h  | 2 +-
 arch/arm/mach-keystone/keystone.c | 4 ++--
 arch/arm/mm/mmu.c                 | 8 ++++----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index e881913f7c3e..cb3a40717edd 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -51,7 +51,7 @@ struct machine_desc {
 	bool			(*smp_init)(void);
 	void			(*fixup)(struct tag *, char **);
 	void			(*dt_fixup)(void);
-	long long		(*init_meminfo)(void);
+	long long		(*pv_fixup)(void);
 	void			(*reserve)(void);/* reserve mem blocks	*/
 	void			(*map_io)(void);/* IO mapping function	*/
 	void			(*init_early)(void);
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index af8c92bc8188..e288010522f9 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -68,7 +68,7 @@ static phys_addr_t keystone_virt_to_idmap(unsigned long x)
 	return (phys_addr_t)(x) - CONFIG_PAGE_OFFSET + KEYSTONE_LOW_PHYS_START;
 }
 
-static long long __init keystone_init_meminfo(void)
+static long long __init keystone_pv_fixup(void)
 {
 	long long offset;
 	phys_addr_t mem_start, mem_end;
@@ -108,5 +108,5 @@ DT_MACHINE_START(KEYSTONE, "Keystone")
 	.smp		= smp_ops(keystone_smp_ops),
 	.init_machine	= keystone_init,
 	.dt_compat	= keystone_match,
-	.init_meminfo   = keystone_init_meminfo,
+	.pv_fixup	= keystone_pv_fixup,
 MACHINE_END
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 657b50085942..4eab87eab5aa 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1404,7 +1404,7 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 	phys_addr_t phys;
 	int i;
 
-	if (!(mdesc->init_meminfo))
+	if (!mdesc->pv_fixup)
 		return;
 
 	/* remap kernel code and data */
@@ -1420,7 +1420,7 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 	pudk = pud_offset(pgdk, map_start);
 	pmdk = pmd_offset(pudk, map_start);
 
-	offset = mdesc->init_meminfo();
+	offset = mdesc->pv_fixup();
 	if (offset == 0)
 		return;
 
@@ -1514,10 +1514,10 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 {
 	long long offset;
 
-	if (!mdesc->init_meminfo)
+	if (!mdesc->pv_fixup)
 		return;
 
-	offset = mdesc->init_meminfo();
+	offset = mdesc->pv_fixup();
 	if (offset == 0)
 		return;
 
-- 
1.8.3.1

[PATCH 5/6] ARM: re-implement physical address space switching

[Russell King]

Re-implement the physical address space switching to be architecturally
complaint.  This involves flushing the caches, disabling the MMU, and
only then updating the page tables.  Once that is complete, the system
can be brought back up again.

Since we disable the MMU, we need to do the update in assembly code.
Luckily, the entries which need updating are fairly trivial, and are
all setup by the early assembly code.  We can merely adjust each entry
by the delta required.

Not only does htis fix the code to be architecturally compliant, but it
fixes a couple of bugs too:

1. The original code would only ever update the first L2 entry covering
   a fraction of the kernel; the remainder were left untouched.
2. The L2 entries covering the DTB blob were likewise untouched.

This solution fixes up all entries.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mm/Kconfig        |   4 ++
 arch/arm/mm/Makefile       |   1 +
 arch/arm/mm/mmu.c          | 119 +++++++++++++--------------------------------
 arch/arm/mm/pv-fixup-asm.S |  88 +++++++++++++++++++++++++++++++++
 4 files changed, 128 insertions(+), 84 deletions(-)
 create mode 100644 arch/arm/mm/pv-fixup-asm.S

diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index b7644310236b..cc873e74643a 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -624,6 +624,10 @@ config ARM_LPAE
 
 	  If unsure, say N.
 
+config ARM_PV_FIXUP
+	def_bool y
+	depends on ARM_LPAE && ARM_PATCH_PHYS_VIRT && ARCH_KEYSTONE
+
 config ARCH_PHYS_ADDR_T_64BIT
 	def_bool ARM_LPAE
 
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index 19df9bd9a79f..290ec5056b3b 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)		+= proc-syms.o
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
+obj-$(CONFIG_ARM_PV_FIXUP)	+= pv-fixup-asm.o
 
 obj-$(CONFIG_CPU_ABRT_NOMMU)	+= abort-nommu.o
 obj-$(CONFIG_CPU_ABRT_EV4)	+= abort-ev4.o
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 4eab87eab5aa..addb7987c714 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1387,7 +1387,11 @@ static void __init map_lowmem(void)
 	}
 }
 
-#if defined(CONFIG_ARM_LPAE) && defined(CONFIG_ARM_PATCH_PHYS_VIRT)
+#ifdef CONFIG_ARM_PV_FIXUP
+extern unsigned long __atags_pointer;
+typedef void pgtables_remap(long long offset, unsigned long pgd, void *bdata);
+pgtables_remap lpae_pgtables_remap_asm;
+
 /*
  * early_paging_init() recreates boot time page table setup, allowing machines
  * to switch over to a high (>4G) address space on LPAE systems
@@ -1395,35 +1399,30 @@ static void __init map_lowmem(void)
 void __init early_paging_init(const struct machine_desc *mdesc,
 			      struct proc_info_list *procinfo)
 {
-	pmdval_t pmdprot = procinfo->__cpu_mm_mmu_flags;
-	unsigned long map_start, map_end;
+	pgtables_remap *lpae_pgtables_remap;
+	unsigned long pa_pgd;
+	unsigned int cr;
 	long long offset;
-	pgd_t *pgd0, *pgdk;
-	pud_t *pud0, *pudk, *pud_start;
-	pmd_t *pmd0, *pmdk;
-	phys_addr_t phys;
-	int i;
+	void *boot_data;
 
 	if (!mdesc->pv_fixup)
 		return;
 
-	/* remap kernel code and data */
-	map_start = init_mm.start_code & PMD_MASK;
-	map_end   = ALIGN(init_mm.brk, PMD_SIZE);
-
-	/* get a handle on things... */
-	pgd0 = pgd_offset_k(0);
-	pud_start = pud0 = pud_offset(pgd0, 0);
-	pmd0 = pmd_offset(pud0, 0);
-
-	pgdk = pgd_offset_k(map_start);
-	pudk = pud_offset(pgdk, map_start);
-	pmdk = pmd_offset(pudk, map_start);
-
 	offset = mdesc->pv_fixup();
 	if (offset == 0)
 		return;
 
+	/*
+	 * Get the address of the remap function in the 1:1 identity
+	 * mapping setup by the early page table assembly code.  We
+	 * must get this prior to the pv update.  The following barrier
+	 * ensures that this is complete before we fixup any P:V offsets.
+	 */
+	lpae_pgtables_remap = (pgtables_remap *)(unsigned long)__pa(lpae_pgtables_remap_asm);
+	pa_pgd = __pa(swapper_pg_dir);
+	boot_data = __va(__atags_pointer);
+	barrier();
+
 	pr_info("Switching physical address space to 0x%08llx\n",
 		(u64)PHYS_OFFSET + offset);
 
@@ -1436,75 +1435,27 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 		(&__pv_table_end - &__pv_table_begin) << 2);
 
 	/*
-	 * Cache cleaning operations for self-modifying code
-	 * We should clean the entries by MVA but running a
-	 * for loop over every pv_table entry pointer would
-	 * just complicate the code.
-	 */
-	flush_cache_louis();
-	dsb(ishst);
-	isb();
-
-	/*
-	 * FIXME: This code is not architecturally compliant: we modify
-	 * the mappings in-place, indeed while they are in use by this
-	 * very same code.  This may lead to unpredictable behaviour of
-	 * the CPU.
-	 *
-	 * Even modifying the mappings in a separate page table does
-	 * not resolve this.
-	 *
-	 * The architecture strongly recommends that when a mapping is
-	 * changed, that it is changed by first going via an invalid
-	 * mapping and back to the new mapping.  This is to ensure that
-	 * no TLB conflicts (caused by the TLB having more than one TLB
-	 * entry match a translation) can occur.  However, doing that
-	 * here will result in unmapping the code we are running.
-	 */
-	pr_warn("WARNING: unsafe modification of in-place page tables - tainting kernel\n");
-	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
-
-	/*
-	 * Remap level 1 table.  This changes the physical addresses
-	 * used to refer to the level 2 page tables to the high
-	 * physical address alias, leaving everything else the same.
-	 */
-	for (i = 0; i < PTRS_PER_PGD; pud0++, i++) {
-		set_pud(pud0,
-			__pud(__pa(pmd0) | PMD_TYPE_TABLE | L_PGD_SWAPPER));
-		pmd0 += PTRS_PER_PMD;
-	}
-
-	/*
-	 * Remap the level 2 table, pointing the mappings at the high
-	 * physical address alias of these pages.
-	 */
-	phys = __pa(map_start);
-	do {
-		*pmdk++ = __pmd(phys | pmdprot);
-		phys += PMD_SIZE;
-	} while (phys < map_end);
-
-	/*
-	 * Ensure that the above updates are flushed out of the cache.
-	 * This is not strictly correct; on a system where the caches
-	 * are coherent with each other, but the MMU page table walks
-	 * may not be coherent, flush_cache_all() may be a no-op, and
-	 * this will fail.
+	 * We changing not only the virtual to physical mapping, but
+	 * also the physical addresses used to access memory.  We need
+	 * to flush all levels of cache in the system with caching
+	 * disabled to ensure that all data is written back.  We do this
+	 * with caching and write buffering disabled to ensure that
+	 * nothing is speculatively prefetched.
 	 */
+	cr = get_cr();
+	set_cr(cr & ~(CR_I | CR_C | CR_W));
 	flush_cache_all();
 
 	/*
-	 * Re-write the TTBR values to point them@the high physical
-	 * alias of the page tables.  We expect __va() will work on
-	 * cpu_get_pgd(), which returns the value of TTBR0.
+	 * Fixup the page tables - this must be in the idmap region as
+	 * we need to disable the MMU to do this safely, and hence it
+	 * needs to be assembly.  It's fairly simple, as we're using the
+	 * temporary tables setup by the initial assembly code.
 	 */
-	cpu_switch_mm(pgd0, &init_mm);
-	cpu_set_ttbr(1, __pa(pgd0) + TTBR1_OFFSET);
+	lpae_pgtables_remap(offset, pa_pgd, boot_data);
 
-	/* Finally flush any stale TLB values. */
-	local_flush_bp_all();
-	local_flush_tlb_all();
+	/* Re-enable the caches */
+	set_cr(cr);
 }
 
 #else
diff --git a/arch/arm/mm/pv-fixup-asm.S b/arch/arm/mm/pv-fixup-asm.S
new file mode 100644
index 000000000000..1867f3e43016
--- /dev/null
+++ b/arch/arm/mm/pv-fixup-asm.S
@@ -0,0 +1,88 @@
+/*
+ *  Copyright (C) 2015 Russell King
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This assembly is required to safely remap the physical address space
+ * for Keystone 2
+ */
+#include <linux/linkage.h>
+#include <asm/asm-offsets.h>
+#include <asm/cp15.h>
+#include <asm/memory.h>
+#include <asm/pgtable.h>
+
+	.section ".idmap.text", "ax"
+
+#define L1_ORDER 3
+#define L2_ORDER 3
+
+ENTRY(lpae_pgtables_remap_asm)
+	stmfd	sp!, {r4-r8, lr}
+
+	mrc	p15, 0, r8, c1, c0, 0		@ read control reg
+	bic	ip, r8, #CR_M			@ disable caches and MMU
+	mcr	p15, 0, ip, c1, c0, 0
+	dsb
+	isb
+
+	/* Update level 2 entries covering the kernel */
+	ldr	r6, =(_end - 1)
+	add	r7, r2, #0x1000
+	add	r6, r7, r6, lsr #SECTION_SHIFT - L2_ORDER
+	add	r7, r7, #PAGE_OFFSET >> (SECTION_SHIFT - L2_ORDER)
+1:	ldrd	r4, [r7]
+	adds	r4, r4, r0
+	adc	r5, r5, r1
+	strd	r4, [r7], #1 << L2_ORDER
+	cmp	r7, r6
+	bls	1b
+
+	/* Update level 2 entries for the boot data */
+	add	r7, r2, #0x1000
+	add	r7, r7, r3, lsr #SECTION_SHIFT - L2_ORDER
+	bic	r7, r7, #(1 << L2_ORDER) - 1
+	ldrd	r4, [r7]
+	adds	r4, r4, r0
+	adc	r5, r5, r1
+	strd	r4, [r7], #1 << L2_ORDER
+	ldrd	r4, [r7]
+	adds	r4, r4, r0
+	adc	r5, r5, r1
+	strd	r4, [r7]
+
+	/* Update level 1 entries */
+	mov	r6, #4
+	mov	r7, r2
+2:	ldrd	r4, [r7]
+	adds	r4, r4, r0
+	adc	r5, r5, r1
+	strd	r4, [r7], #1 << L1_ORDER
+	subs	r6, r6, #1
+	bne	2b
+
+	mrrc	p15, 0, r4, r5, c2		@ read TTBR0
+	adds	r4, r4, r0			@ update physical address
+	adc	r5, r5, r1
+	mcrr	p15, 0, r4, r5, c2		@ write back TTBR0
+	mrrc	p15, 1, r4, r5, c2		@ read TTBR1
+	adds	r4, r4, r0			@ update physical address
+	adc	r5, r5, r1
+	mcrr	p15, 1, r4, r5, c2		@ write back TTBR1
+
+	dsb
+
+	mov	ip, #0
+	mcr	p15, 0, ip, c7, c5, 0		@ I+BTB cache invalidate
+	mcr	p15, 0, ip, c8, c7, 0		@ local_flush_tlb_all()
+	dsb
+	isb
+
+	mcr	p15, 0, r8, c1, c0, 0		@ re-enable MMU
+	dsb
+	isb
+
+	ldmfd	sp!, {r4-r8, pc}
+ENDPROC(lpae_pgtables_remap_asm)
-- 
1.8.3.1

[PATCH 6/6] ARM: cleanup early_paging_init() calling

[Russell King]

Eliminate the needless nommu version of this function, and get rid of
the proc_info_list structure argument - we no longer need this in order
to fix up the page table entries.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/setup.c | 7 ++++---
 arch/arm/mm/mmu.c       | 6 ++----
 arch/arm/mm/nommu.c     | 9 ---------
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 6c777e908a24..979c1c5fe96a 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -75,8 +75,7 @@ __setup("fpe=", fpe_setup);
 
 extern void init_default_cache_policy(unsigned long);
 extern void paging_init(const struct machine_desc *desc);
-extern void early_paging_init(const struct machine_desc *,
-			      struct proc_info_list *);
+extern void early_paging_init(const struct machine_desc *);
 extern void sanity_check_meminfo(void);
 extern enum reboot_mode reboot_mode;
 extern void setup_dma_zone(const struct machine_desc *desc);
@@ -936,7 +935,9 @@ void __init setup_arch(char **cmdline_p)
 
 	parse_early_param();
 
-	early_paging_init(mdesc, lookup_processor_type(read_cpuid_id()));
+#ifdef CONFIG_MMU
+	early_paging_init(mdesc);
+#endif
 	setup_dma_zone(mdesc);
 	sanity_check_meminfo();
 	arm_memblock_init(mdesc);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index addb7987c714..b2e96bc9d74a 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1396,8 +1396,7 @@ pgtables_remap lpae_pgtables_remap_asm;
  * early_paging_init() recreates boot time page table setup, allowing machines
  * to switch over to a high (>4G) address space on LPAE systems
  */
-void __init early_paging_init(const struct machine_desc *mdesc,
-			      struct proc_info_list *procinfo)
+void __init early_paging_init(const struct machine_desc *mdesc)
 {
 	pgtables_remap *lpae_pgtables_remap;
 	unsigned long pa_pgd;
@@ -1460,8 +1459,7 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 
 #else
 
-void __init early_paging_init(const struct machine_desc *mdesc,
-			      struct proc_info_list *procinfo)
+void __init early_paging_init(const struct machine_desc *mdesc)
 {
 	long long offset;
 
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index a014dfacd5ca..afd7e05d95f1 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -304,15 +304,6 @@ void __init sanity_check_meminfo(void)
 }
 
 /*
- * early_paging_init() recreates boot time page table setup, allowing machines
- * to switch over to a high (>4G) address space on LPAE systems
- */
-void __init early_paging_init(const struct machine_desc *mdesc,
-			      struct proc_info_list *procinfo)
-{
-}
-
-/*
  * paging_init() sets up the page tables, initialises the zone memory
  * maps, and sets up the zero page, bad page and bad page tables.
  */
-- 
1.8.3.1

[PATCH 5/6] ARM: re-implement physical address space switching

[Thomas Petazzoni]

Dear Russell King,

On Wed, 08 Apr 2015 10:45:30 +0100, Russell King wrote:
> Re-implement the physical address space switching to be architecturally
> complaint.  This involves flushing the caches, disabling the MMU, and

Nit: s/complaint/compliant/.

But since I'm not going to write an e-mail just for such a small nit,
here is an hopefully more interesting question: do you think it would
be possible to use this logic for my Armada 370/XP/375/38x case, where
I need some specific page table configuration to be able to use HW I/O
coherency in non-SMP configurations?

If you remember the discussion we had, the primary issue is that the
page tables are built in arch/arm/kernel/head.S, before we even know
which SoC we are running on (and the CPU ID available from CP15 is not
sufficient to determine whether the SoC supports HW I/O coherency). If
we can get into the SoC-specific code, and at this point decide whether
and how the page tables should be rebuilt (which is apparently what
happens in Keystone), then it would be also useful for our case I
believe.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Grygorii.Strashko@linaro.org]

Hi Russell,

On 04/08/2015 12:45 PM, Russell King wrote:
> Make the init_meminfo function return the offset to be applied to the
> phys-to-virt translation constants.  This allows us to move the update
> into generic code, along with the requirements for this update.


I have a question (or may be proposition) regarding this patch.

Could it be reasonable if .init_meminfo() will return new PHYS offset
of RAM (new start address), instead of translation's offset?

Additional comments are below, which were done only from Keystone 2 point of view.

> 
> This avoids platforms having to know the details of the phys-to-virt
> translation support.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>   arch/arm/include/asm/mach/arch.h  |  2 +-
>   arch/arm/mach-keystone/keystone.c | 27 ++++++++++-----------------
>   arch/arm/mm/mmu.c                 | 26 ++++++++++++++++++++++----
>   3 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
> index 0406cb3f1af7..e881913f7c3e 100644
> --- a/arch/arm/include/asm/mach/arch.h
> +++ b/arch/arm/include/asm/mach/arch.h
> @@ -51,7 +51,7 @@ struct machine_desc {
>   	bool			(*smp_init)(void);
>   	void			(*fixup)(struct tag *, char **);
>   	void			(*dt_fixup)(void);
> -	void			(*init_meminfo)(void);
> +	long long		(*init_meminfo)(void);
>   	void			(*reserve)(void);/* reserve mem blocks	*/
>   	void			(*map_io)(void);/* IO mapping function	*/
>   	void			(*init_early)(void);
> diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
> index 3d58a8f4dc7e..baa0fbc9803a 100644
> --- a/arch/arm/mach-keystone/keystone.c
> +++ b/arch/arm/mach-keystone/keystone.c
> @@ -68,11 +68,9 @@ static phys_addr_t keystone_virt_to_idmap(unsigned long x)
>   	return (phys_addr_t)(x) - CONFIG_PAGE_OFFSET + KEYSTONE_LOW_PHYS_START;
>   }
>   
> -static void __init keystone_init_meminfo(void)
> +static long long __init keystone_init_meminfo(void)
>   {
> -	bool lpae = IS_ENABLED(CONFIG_ARM_LPAE);
> -	bool pvpatch = IS_ENABLED(CONFIG_ARM_PATCH_PHYS_VIRT);
> -	phys_addr_t offset = PHYS_OFFSET - KEYSTONE_LOW_PHYS_START;
> +	long long offset;
>   	phys_addr_t mem_start, mem_end;
>   
>   	mem_start = memblock_start_of_DRAM();
> @@ -81,29 +79,24 @@ static void __init keystone_init_meminfo(void)
>   	/* nothing to do if we are running out of the <32-bit space */
>   	if (mem_start >= KEYSTONE_LOW_PHYS_START &&
>   	    mem_end   <= KEYSTONE_LOW_PHYS_END)
> -		return;
> -
> -	if (!lpae || !pvpatch) {
> -		pr_crit("Enable %s%s%s to run outside 32-bit space\n",
> -		      !lpae ? __stringify(CONFIG_ARM_LPAE) : "",
> -		      (!lpae && !pvpatch) ? " and " : "",
> -		      !pvpatch ? __stringify(CONFIG_ARM_PATCH_PHYS_VIRT) : "");
> -	}
> +		return 0;
>   
>   	if (mem_start < KEYSTONE_HIGH_PHYS_START ||
>   	    mem_end   > KEYSTONE_HIGH_PHYS_END) {
>   		pr_crit("Invalid address space for memory (%08llx-%08llx)\n",
> -		      (u64)mem_start, (u64)mem_end);
> +		        (u64)mem_start, (u64)mem_end);
> +		return 0;
>   	}
>   
> -	offset += KEYSTONE_HIGH_PHYS_START;
> -	__pv_phys_pfn_offset = PFN_DOWN(offset);
> -	__pv_offset = (offset - PAGE_OFFSET);
> +	offset = KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START;

New PHYS offset of RAM  can be returned here

 offset = KEYSTONE_HIGH_PHYS_START;

>   
>   	/* Populate the arch idmap hook */
>   	arch_virt_to_idmap = keystone_virt_to_idmap;
>   
> -	pr_info("Switching to high address space at 0x%llx\n", (u64)offset);
> +	pr_info("Switching to high address space at 0x%llx\n",
> +	        (u64)PHYS_OFFSET + (u64)offset);

Change can be dropped

> +
> +	return offset;
>   }
>   
>   static const char *const keystone_match[] __initconst = {
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 4e6ef896c619..71563fdf298c 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1387,7 +1387,7 @@ static void __init map_lowmem(void)
>   	}
>   }
>   
> -#ifdef CONFIG_ARM_LPAE
> +#if defined(CONFIG_ARM_LPAE) && defined(CONFIG_ARM_PATCH_PHYS_VIRT)
>   /*
>    * early_paging_init() recreates boot time page table setup, allowing machines
>    * to switch over to a high (>4G) address space on LPAE systems
> @@ -1397,6 +1397,7 @@ void __init early_paging_init(const struct machine_desc *mdesc,
>   {
>   	pmdval_t pmdprot = procinfo->__cpu_mm_mmu_flags;
>   	unsigned long map_start, map_end;
> +	long long offset;
>   	pgd_t *pgd0, *pgdk;
>   	pud_t *pud0, *pudk, *pud_start;
>   	pmd_t *pmd0, *pmdk;
> @@ -1419,7 +1420,13 @@ void __init early_paging_init(const struct machine_desc *mdesc,
>   	pudk = pud_offset(pgdk, map_start);
>   	pmdk = pmd_offset(pudk, map_start);
>   

If I understand PV patching code right, the PHYS_OFFSET == 0x8000 0000 (32 bit address)
at this moment and its value is adjusted by Asm code. So, this value
could be considered as old PHYS_OFFSET.

PAGE_OFFSET == 0xC000 0000 (defconfig)

> -	mdesc->init_meminfo();
> +	offset = mdesc->init_meminfo();
> +	if (offset == 0)
> +		return;

Here now: 
offset == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) - 0x8000 0000 (KEYSTONE_LOW_PHYS_START)
offset == 0x7 8000 0000

in turn KEYSTONE_LOW_PHYS_START == (old)PHYS_OFFSET 


> +
> +	/* Re-set the phys pfn offset, and the pv offset */
> +	__pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET;

So, here now:

__pv_offset = 0x8000 0000 + 0x7 8000 0000 - 0xC000 0000;

__pv_offset == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) - 0xC000 0000

code if offset will eq (new)PHYS_OFFSET == KEYSTONE_HIGH_PHYS_START:

  __pv_offset = offset - PAGE_OFFSET;

> +	__pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset);

Here
(old)PHYS_OFFSET + offset == 0x8000 0000 + 0x7 8000 0000 == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START)

Code if offset will eq (new)PHYS_OFFSET == KEYSTONE_HIGH_PHYS_START:
  
  __pv_phys_pfn_offset = PFN_DOWN(offset);

>   
>   	/* Run the patch stub to update the constants */
>   	fixup_pv_table(&__pv_table_begin,
> @@ -1502,8 +1509,19 @@ void __init early_paging_init(const struct machine_desc *mdesc,
>   void __init early_paging_init(const struct machine_desc *mdesc,
>   			      struct proc_info_list *procinfo)
>   {
> -	if (mdesc->init_meminfo)
> -		mdesc->init_meminfo();
> +	long long offset;
> +
> +	if (!mdesc->init_meminfo)
> +		return;
> +
> +	offset = mdesc->init_meminfo();
> +	if (offset == 0)
> +		return;
> +
> +	pr_crit("Physical address space modification is only to support Keystone2.\n");
> +	pr_crit("Please enable ARM_LPAE and ARM_PATCH_PHYS_VIRT support to use this\n");
> +	pr_crit("feature. Your kernel may crash now, have a good day.\n");
> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>   }
>   
>   #endif
> 

-- 
regards,
-grygorii

[PATCH 0/7] Fix Keystone 2 physical address switch

[santosh shilimkar]

Russell,

On 4/8/2015 2:44 AM, Russell King - ARM Linux wrote:
> For some time, the Keystone 2 physical address switch has been
> tainting the kernel ever since we decided that it was not safe.
>
> This series re-works the address switch to be compliant architecturally.
>
Thanks a lot for doing it. Will have a look at the series.

> There's still a niggle in the keystone2 code, where we set the
> arch_virt_to_idmap function pointer in keystone_pv_fixup() - something
> I'd ideally like to avoid.
>
> I'm in two minds about how much of this code should be in SoC specific
> code, and how much should be in generic code - on one hand, I don't
> want to give a green light to this kind of sillyness by encouraging
> it, but I'd rather not end up with multiple implementations of this.
>
Either way you decide, its fine by me. Actually there is just one user
at this point of time but so is the case for the address switch code.
I couldn't come up with more generic than the function pointer last
time.

Regards,
Santosh

[PATCH 5/6] ARM: re-implement physical address space switching

[santosh shilimkar]

On 4/8/2015 7:34 AM, Thomas Petazzoni wrote:
> Dear Russell King,
>
> On Wed, 08 Apr 2015 10:45:30 +0100, Russell King wrote:
>> Re-implement the physical address space switching to be architecturally
>> complaint.  This involves flushing the caches, disabling the MMU, and
>
> Nit: s/complaint/compliant/.
>
> But since I'm not going to write an e-mail just for such a small nit,
> here is an hopefully more interesting question: do you think it would
> be possible to use this logic for my Armada 370/XP/375/38x case, where
> I need some specific page table configuration to be able to use HW I/O
> coherency in non-SMP configurations?
>
> If you remember the discussion we had, the primary issue is that the
> page tables are built in arch/arm/kernel/head.S, before we even know
> which SoC we are running on (and the CPU ID available from CP15 is not
> sufficient to determine whether the SoC supports HW I/O coherency). If
> we can get into the SoC-specific code, and at this point decide whether
> and how the page tables should be rebuilt (which is apparently what
> happens in Keystone), then it would be also useful for our case I
> believe.
>
Well Keystone does switch the page tables to use entirely different
memory but what you are talking is, you want to control the page
table attributes for certain pages(memory area). Head.s page tables
are just for boot up. The actual page tables creation happens in
paging_init().

Not sure if you just need a special MT_* area which could take
care of your case but am missing details so no sure.

Regards,
Santosh

[PATCH 5/6] ARM: re-implement physical address space switching

[Mark Rutland]

Hi Russell,

On Wed, Apr 08, 2015 at 10:45:30AM +0100, Russell King wrote:
> Re-implement the physical address space switching to be architecturally
> complaint.  This involves flushing the caches, disabling the MMU, and
> only then updating the page tables.  Once that is complete, the system
> can be brought back up again.
> 
> Since we disable the MMU, we need to do the update in assembly code.
> Luckily, the entries which need updating are fairly trivial, and are
> all setup by the early assembly code.  We can merely adjust each entry
> by the delta required.
> 
> Not only does htis fix the code to be architecturally compliant, but it
> fixes a couple of bugs too:

Nit: s/htis/this/

[...]

> -       /*
> -        * Ensure that the above updates are flushed out of the cache.
> -        * This is not strictly correct; on a system where the caches
> -        * are coherent with each other, but the MMU page table walks
> -        * may not be coherent, flush_cache_all() may be a no-op, and
> -        * this will fail.
> +        * We changing not only the virtual to physical mapping, but
> +        * also the physical addresses used to access memory.  We need
> +        * to flush all levels of cache in the system with caching
> +        * disabled to ensure that all data is written back.  We do this
> +        * with caching and write buffering disabled to ensure that
> +        * nothing is speculatively prefetched.
>          */
> +       cr = get_cr();
> +       set_cr(cr & ~(CR_I | CR_C | CR_W));

SCTLR[3] (CR_W) is RAO/SBOP in VMSAv7. I don't think we should clear it
here for ARMv7.

To the best of my knowledge, the page table walkers aren't affected by
SCTLR.C, and use the attributes in TTBR{0,1} or TTBCR when translation
is active (i.e. when SCTLR.M is set). So at this point they can make
cacheable accesses to the page tables (and allocate into the caches) in
the background...

>         flush_cache_all();

...meaning this flush may not leave the caches empty, at least for
memory regions containing page tables. Stale lines could mask updates
made in lpae_pgtables_remap_asm.

I think that the cache flush needs to be performed after both
SCTLR.{C,M} are cleared in lpae_pgtables_remap_asm, just before the page
table updates. So long as the relevant pieces of kernel text are
initially clean to the PoC, we shouldn't need to flush anything
beforehand.


> 
>         /*
> -        * Re-write the TTBR values to point them at the high physical
> -        * alias of the page tables.  We expect __va() will work on
> -        * cpu_get_pgd(), which returns the value of TTBR0.
> +        * Fixup the page tables - this must be in the idmap region as
> +        * we need to disable the MMU to do this safely, and hence it
> +        * needs to be assembly.  It's fairly simple, as we're using the
> +        * temporary tables setup by the initial assembly code.
>          */
> -       cpu_switch_mm(pgd0, &init_mm);
> -       cpu_set_ttbr(1, __pa(pgd0) + TTBR1_OFFSET);
> +       lpae_pgtables_remap(offset, pa_pgd, boot_data);
> 
> -       /* Finally flush any stale TLB values. */
> -       local_flush_bp_all();
> -       local_flush_tlb_all();
> +       /* Re-enable the caches */
> +       set_cr(cr);

That being the case there's no reason to restore M and C separately on
the return path either.

[...]

> +ENTRY(lpae_pgtables_remap_asm)
> +       stmfd   sp!, {r4-r8, lr}
> +
> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
> +       mcr     p15, 0, ip, c1, c0, 0
> +       dsb
> +       isb

Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
point to an idmap/physical address)?

I don't see why we need a DSB after the write to the SCTLR.

[...]

> +       dsb
> +       isb
> +
> +       mcr     p15, 0, r8, c1, c0, 0           @ re-enable MMU
> +       dsb
> +       isb

Similarly, isn't the last DSB redundant?

Thanks,
Mark.

[PATCH 5/6] ARM: re-implement physical address space switching

[Russell King - ARM Linux]

On Wed, Apr 08, 2015 at 06:36:56PM +0100, Mark Rutland wrote:
> Hi Russell,
> 
> On Wed, Apr 08, 2015 at 10:45:30AM +0100, Russell King wrote:
> > -       /*
> > -        * Ensure that the above updates are flushed out of the cache.
> > -        * This is not strictly correct; on a system where the caches
> > -        * are coherent with each other, but the MMU page table walks
> > -        * may not be coherent, flush_cache_all() may be a no-op, and
> > -        * this will fail.
> > +        * We changing not only the virtual to physical mapping, but
> > +        * also the physical addresses used to access memory.  We need
> > +        * to flush all levels of cache in the system with caching
> > +        * disabled to ensure that all data is written back.  We do this
> > +        * with caching and write buffering disabled to ensure that
> > +        * nothing is speculatively prefetched.
> >          */
> > +       cr = get_cr();
> > +       set_cr(cr & ~(CR_I | CR_C | CR_W));
> 
> SCTLR[3] (CR_W) is RAO/SBOP in VMSAv7. I don't think we should clear it
> here for ARMv7.

I was in two minds about that - I guess as we're expecting to only run
this on ARMv7 CPUs, we can omit clearing the CR_W, but I'd need to add
a comment saying that it's ARMv7 only.

> To the best of my knowledge, the page table walkers aren't affected by
> SCTLR.C, and use the attributes in TTBR{0,1} or TTBCR when translation
> is active (i.e. when SCTLR.M is set). So at this point they can make
> cacheable accesses to the page tables (and allocate into the caches) in
> the background...

We had better clear those bits too then.

> I think that the cache flush needs to be performed after both
> SCTLR.{C,M} are cleared in lpae_pgtables_remap_asm, just before the page
> table updates. So long as the relevant pieces of kernel text are
> initially clean to the PoC, we shouldn't need to flush anything
> beforehand.

To that I say... no bloody way in hell, even once hell has frozen
over.  It took almost a _day_ to get this much working, much of it
was attempting to use cache flushing functions after the MMU had
been turned off.

If it was possible to make it work, I'd have done so.  It isn't, so
please forget the idea.

> > +ENTRY(lpae_pgtables_remap_asm)
> > +       stmfd   sp!, {r4-r8, lr}
> > +
> > +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
> > +       bic     ip, r8, #CR_M                   @ disable caches and MMU
> > +       mcr     p15, 0, ip, c1, c0, 0
> > +       dsb
> > +       isb
> 
> Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
> point to an idmap/physical address)?
> 
> I don't see why we need a DSB after the write to the SCTLR.
> 
> [...]
> 
> > +       dsb
> > +       isb
> > +
> > +       mcr     p15, 0, r8, c1, c0, 0           @ re-enable MMU
> > +       dsb
> > +       isb
> 
> Similarly, isn't the last DSB redundant?

I've really no idea.  All I know is that the above works.  I'm rather
sick of trying to read the ARM ARM and not understanding exactly what
ISB and DSB actually do.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Russell King - ARM Linux]

On Wed, Apr 08, 2015 at 05:56:49PM +0300, Grygorii.Strashko at linaro.org wrote:
> Hi Russell,
> 
> On 04/08/2015 12:45 PM, Russell King wrote:
> > Make the init_meminfo function return the offset to be applied to the
> > phys-to-virt translation constants.  This allows us to move the update
> > into generic code, along with the requirements for this update.
> 
> 
> I have a question (or may be proposition) regarding this patch.
> 
> Could it be reasonable if .init_meminfo() will return new PHYS offset
> of RAM (new start address), instead of translation's offset?

I'm not that interested in such a change, for the simple reason that
what we mostly want to know is what the _difference_ between what's
already in the page tables, and what we want to update them to.

The page table update is a case of merely adding the delta to each
page table entry.

If we were to want to pass the actual new physical address, we would
have to have the fixup code mask out the old address, and insert the
new address, keeping track of the offset into the kernel's mapping.
We'd also have to do something weirder for the DT mapping too.

Using an offset makes the page table update rather trivial and easy.

I actually did start off with passing the new physical address
initially, and I changed to this way because it simplified the
assembly code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 5/6] ARM: re-implement physical address space switching

[Russell King - ARM Linux]

On Wed, Apr 08, 2015 at 04:34:28PM +0200, Thomas Petazzoni wrote:
> But since I'm not going to write an e-mail just for such a small nit,
> here is an hopefully more interesting question: do you think it would
> be possible to use this logic for my Armada 370/XP/375/38x case, where
> I need some specific page table configuration to be able to use HW I/O
> coherency in non-SMP configurations?

This implementation only handles LPAE, and it only knows to add an
offset to each LPAE page table descriptor.  So, adapting this code
to that application isn't going to be easy, because you're talking
about setting and/or clearing some bits.

So, I think it would take a re-implementation of it to make it work.
Once the review comments have been addressed, at least this gives an
idea how to address the coherency issue there.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Russell King - ARM Linux]

On Wed, Apr 08, 2015 at 05:56:49PM +0300, Grygorii.Strashko at linaro.org wrote:
> > +
> > +	/* Re-set the phys pfn offset, and the pv offset */
> > +	__pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET;
> 
> So, here now:
> 
> __pv_offset = 0x8000 0000 + 0x7 8000 0000 - 0xC000 0000;
> 
> __pv_offset == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) - 0xC000 0000
> 
> code if offset will eq (new)PHYS_OFFSET == KEYSTONE_HIGH_PHYS_START:
> 
>   __pv_offset = offset - PAGE_OFFSET;

BTW, this could simply be changed to:

	__pv_offset += offset;

keeping the existing "delta" implementation.

> > +	__pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset);

and this to:

	__pv_phys_pfn_offset += PFN_DOWN(offset);

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Grygorii.Strashko@linaro.org]

Hi Russell,

On 04/08/2015 09:00 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 08, 2015 at 05:56:49PM +0300, Grygorii.Strashko at linaro.org wrote:
>> Hi Russell,
>>
>> On 04/08/2015 12:45 PM, Russell King wrote:
>>> Make the init_meminfo function return the offset to be applied to the
>>> phys-to-virt translation constants.  This allows us to move the update
>>> into generic code, along with the requirements for this update.
>>
>>
>> I have a question (or may be proposition) regarding this patch.
>>
>> Could it be reasonable if .init_meminfo() will return new PHYS offset
>> of RAM (new start address), instead of translation's offset?
> 
> I'm not that interested in such a change, for the simple reason that
> what we mostly want to know is what the _difference_ between what's
> already in the page tables, and what we want to update them to.
> 
> The page table update is a case of merely adding the delta to each
> page table entry.
> 
> If we were to want to pass the actual new physical address, we would
> have to have the fixup code mask out the old address, and insert the
> new address, keeping track of the offset into the kernel's mapping.
> We'd also have to do something weirder for the DT mapping too.
> 
> Using an offset makes the page table update rather trivial and easy.
> 
> I actually did start off with passing the new physical address
> initially, and I changed to this way because it simplified the
> assembly code.
> 

Ok, I understand the point, but still wish to try this proposition
(even if I'll be punished)).


First, the current commit description confused me a bit and that was the main
reason why I commented here. Commit message said:
 "Make the init_meminfo function return the offset to be applied to the
  phys-to-virt translation constants"

 but actual return value can't be applied to 
 "phys-to-virt translation constants" without modification.
 The offset to be applied to "phys-to-virt translation constants" should be
 (NEW_PHYS_OFFSET - PAGE_OFFSET) as per commit dc21af9.

 But now, the .init_meminfo() assumed to return offset between NEW and OLD
 RAM starting addresses : (NEW_PHYS_OFFSET - OLD_PHYS_OFFSET). 
 And, It looks like this offset actually needed only for lpae_pgtables_remap().

 I think, We can get all data needed for physical address switching and pgtables
 updating code from inside early_paging_init() except the NEW_PHYS_OFFSET -
 - new starting physical address of the RAM and, therefore, It seems reasonable
 to get it as return value of .pv_fixup().

 Therefore, I did a patch/diff on top of your series to illustrate this
 (no changes in the assembly code) - K2HK board boots.

Regardless of will you accept this change or not - I've tested boot on K2HK board
with yours patches (applied on top of k4.0-rc7), so
 Tested-by: Grygorii Strashko <grygorii.strashko@linaro.org>

-- 
regards,
-grygorii

----------->
>From ac22a2f5f163f09a7d51314fdaea231b98c22ec7 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@linaro.org>
Date: Thu, 9 Apr 2015 17:29:57 +0300
Subject: [PATCH] ARM: update .pv_fixup() to return new start phys address of ram

Update .pv_fixup() to return new start phys address of RAM instead
of offset between new and old RAM addresses. This will make code
a bit clear and can be done because three parameters need to be known
for proper physical address switching in early_paging_init():
 - OLD_PHYS_OFFSET
 - NEW_PHYS_OFFSET
 - PAGE_OFFSET (constant)

These parameters are used for calculation:
 -  __pv_offset = NEW_PHYS_OFFSET - PAGE_OFFSET which is
    used by phys-to-virt translation routines

 -  (NEW_PHYS_OFFSET - OLD_PHYS_OFFSET) offset which is
    used by pgtables updating routine lpae_pgtables_remap().

The OLD_PHYS_OFFSET == PHYS_OFFSET and can be saved at the
beginning of early_paging_init(). So, only NEW_PHYS_OFFSET is
unknown from inside early_paging_init() and, therefore, need
to be provided by platform code.

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 arch/arm/include/asm/mach/arch.h  |  2 +-
 arch/arm/mach-keystone/keystone.c |  4 ++--
 arch/arm/mm/mmu.c                 | 18 ++++++++++--------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index cb3a407..84acaba 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -51,7 +51,7 @@ struct machine_desc {
 	bool			(*smp_init)(void);
 	void			(*fixup)(struct tag *, char **);
 	void			(*dt_fixup)(void);
-	long long		(*pv_fixup)(void);
+	phys_addr_t		(*pv_fixup)(void);
 	void			(*reserve)(void);/* reserve mem blocks	*/
 	void			(*map_io)(void);/* IO mapping function	*/
 	void			(*init_early)(void);
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index e288010..f3816b1 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -68,7 +68,7 @@ static phys_addr_t keystone_virt_to_idmap(unsigned long x)
 	return (phys_addr_t)(x) - CONFIG_PAGE_OFFSET + KEYSTONE_LOW_PHYS_START;
 }
 
-static long long __init keystone_pv_fixup(void)
+static phys_addr_t __init keystone_pv_fixup(void)
 {
 	long long offset;
 	phys_addr_t mem_start, mem_end;
@@ -88,7 +88,7 @@ static long long __init keystone_pv_fixup(void)
 		return 0;
 	}
 
-	offset = KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START;
+	offset = KEYSTONE_HIGH_PHYS_START;
 
 	/* Populate the arch idmap hook */
 	arch_virt_to_idmap = keystone_virt_to_idmap;
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index b2e96bc..1d6c119 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1401,14 +1401,15 @@ void __init early_paging_init(const struct machine_desc *mdesc)
 	pgtables_remap *lpae_pgtables_remap;
 	unsigned long pa_pgd;
 	unsigned int cr;
-	long long offset;
+	phys_addr_t new_phys_offset;
+	phys_addr_t old_phys_offset = PHYS_OFFSET;
 	void *boot_data;
 
 	if (!mdesc->pv_fixup)
 		return;
 
-	offset = mdesc->pv_fixup();
-	if (offset == 0)
+	new_phys_offset = mdesc->pv_fixup();
+	if (new_phys_offset == 0)
 		return;
 
 	/*
@@ -1422,12 +1423,12 @@ void __init early_paging_init(const struct machine_desc *mdesc)
 	boot_data = __va(__atags_pointer);
 	barrier();
 
-	pr_info("Switching physical address space to 0x%08llx\n",
-		(u64)PHYS_OFFSET + offset);
+	pr_info("Switching physical address space to %pa\n",
+		&new_phys_offset);
 
 	/* Re-set the phys pfn offset, and the pv offset */
-	__pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET;
-	__pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset);
+	__pv_offset = new_phys_offset - PAGE_OFFSET;
+	__pv_phys_pfn_offset = PFN_DOWN(new_phys_offset);
 
 	/* Run the patch stub to update the constants */
 	fixup_pv_table(&__pv_table_begin,
@@ -1451,7 +1452,8 @@ void __init early_paging_init(const struct machine_desc *mdesc)
 	 * needs to be assembly.  It's fairly simple, as we're using the
 	 * temporary tables setup by the initial assembly code.
 	 */
-	lpae_pgtables_remap(offset, pa_pgd, boot_data);
+	lpae_pgtables_remap((new_phys_offset - old_phys_offset),
+			    pa_pgd, boot_data);
 
 	/* Re-enable the caches */
 	set_cr(cr);
-- 
1.9.1

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Russell King - ARM Linux]

On Thu, Apr 09, 2015 at 05:51:14PM +0300, Grygorii.Strashko at linaro.org wrote:
> @@ -1401,14 +1401,15 @@ void __init early_paging_init(const struct machine_desc *mdesc)
>  	pgtables_remap *lpae_pgtables_remap;
>  	unsigned long pa_pgd;
>  	unsigned int cr;
> -	long long offset;
> +	phys_addr_t new_phys_offset;
> +	phys_addr_t old_phys_offset = PHYS_OFFSET;
>  	void *boot_data;
>  
>  	if (!mdesc->pv_fixup)
>  		return;
>  
> -	offset = mdesc->pv_fixup();
> -	if (offset == 0)
> +	new_phys_offset = mdesc->pv_fixup();
> +	if (new_phys_offset == 0)

"0" is a special magic value here.  The advantage of the delta approach
is that "0" is obviously equivalent to "no change".

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[Grygorii.Strashko@linaro.org]

On 04/09/2015 06:49 PM, Russell King - ARM Linux wrote:
> On Thu, Apr 09, 2015 at 05:51:14PM +0300, Grygorii.Strashko at linaro.org wrote:
>> @@ -1401,14 +1401,15 @@ void __init early_paging_init(const struct machine_desc *mdesc)
>>   	pgtables_remap *lpae_pgtables_remap;
>>   	unsigned long pa_pgd;
>>   	unsigned int cr;
>> -	long long offset;
>> +	phys_addr_t new_phys_offset;
>> +	phys_addr_t old_phys_offset = PHYS_OFFSET;
>>   	void *boot_data;
>>
>>   	if (!mdesc->pv_fixup)
>>   		return;
>>
>> -	offset = mdesc->pv_fixup();
>> -	if (offset == 0)
>> +	new_phys_offset = mdesc->pv_fixup();
>> +	if (new_phys_offset == 0)
>
> "0" is a special magic value here.  The advantage of the delta approach
> is that "0" is obviously equivalent to "no change".
>

can "-1"  be used?



-- 
regards,
-grygorii

[PATCH 0/7] Fix Keystone 2 physical address switch

[Russell King - ARM Linux]

On Wed, Apr 08, 2015 at 10:21:47AM -0700, santosh shilimkar wrote:
> Russell,
> 
> On 4/8/2015 2:44 AM, Russell King - ARM Linux wrote:
> >For some time, the Keystone 2 physical address switch has been
> >tainting the kernel ever since we decided that it was not safe.
> >
> >This series re-works the address switch to be compliant architecturally.
> >
> Thanks a lot for doing it. Will have a look at the series.

I think given where we are, it's already too late to get it into this
merge window _unless_ Linus does -rc8 this weekend.  I don't classify
this as a "fix" which would be mergable during the window or during
-rc after, but more as a correctness update.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/7] Fix Keystone 2 physical address switch

[santosh shilimkar]

On 4/9/2015 9:21 AM, Russell King - ARM Linux wrote:
> On Wed, Apr 08, 2015 at 10:21:47AM -0700, santosh shilimkar wrote:
>> Russell,
>>
>> On 4/8/2015 2:44 AM, Russell King - ARM Linux wrote:
>>> For some time, the Keystone 2 physical address switch has been
>>> tainting the kernel ever since we decided that it was not safe.
>>>
>>> This series re-works the address switch to be compliant architecturally.
>>>
>> Thanks a lot for doing it. Will have a look at the series.
>
> I think given where we are, it's already too late to get it into this
> merge window _unless_ Linus does -rc8 this weekend.  I don't classify
> this as a "fix" which would be mergable during the window or during
> -rc after, but more as a correctness update.
>
Right. We can get them in next one.

Regards,
Santosh

[PATCH 1/6] ARM: keystone2: move platform notifier initialisation into platform init

[santosh shilimkar]

On 4/8/2015 2:45 AM, Russell King wrote:
> We ideally want the init_meminfo function to do nothing but return the
> delta to be applied to PHYS_OFFSET - it should do nothing else.  As we
> can detect in platform init code whether we are running in the high
> physical address space, move the platform notifier initialisation
> entirely into the platform init code.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

[PATCH 2/6] ARM: keystone2: move update of the phys-to-virt constants into generic code

[santosh shilimkar]

On 4/8/2015 2:45 AM, Russell King wrote:
> Make the init_meminfo function return the offset to be applied to the
> phys-to-virt translation constants.  This allows us to move the update
> into generic code, along with the requirements for this update.
>
> This avoids platforms having to know the details of the phys-to-virt
> translation support.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>   arch/arm/include/asm/mach/arch.h  |  2 +-
>   arch/arm/mach-keystone/keystone.c | 27 ++++++++++-----------------
>   arch/arm/mm/mmu.c                 | 26 ++++++++++++++++++++++----
>   3 files changed, 33 insertions(+), 22 deletions(-)
>
You already identified couple of improvements on offset calculations.
Apart from that, patch look fine by me.

Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

[PATCH 3/6] ARM: keystone2: move address space switch printk to generic code

[santosh shilimkar]

On 4/8/2015 2:45 AM, Russell King wrote:
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>   arch/arm/mach-keystone/keystone.c | 3 ---
>   arch/arm/mm/mmu.c                 | 3 +++
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

[PATCH 4/6] ARM: keystone2: rename init_meminfo to pv_fixup

[santosh shilimkar]

On 4/8/2015 2:45 AM, Russell King wrote:
> The init_meminfo() method is not about initialising meminfo - it's about
> fixing up the physical to virtual translation so that we use a different
> physical address space, possibly above the 4GB physical address space.
> Therefore, the name "init_meminfo()" is confusing.
>
Indeed.

> Rename it to pv_fixup() instead.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

[PATCH 5/6] ARM: re-implement physical address space switching

[santosh shilimkar]

On 4/8/2015 10:55 AM, Russell King - ARM Linux wrote:
> On Wed, Apr 08, 2015 at 06:36:56PM +0100, Mark Rutland wrote:
>> Hi Russell,
>>
>> On Wed, Apr 08, 2015 at 10:45:30AM +0100, Russell King wrote:
>>> -       /*
>>> -        * Ensure that the above updates are flushed out of the cache.
>>> -        * This is not strictly correct; on a system where the caches
>>> -        * are coherent with each other, but the MMU page table walks
>>> -        * may not be coherent, flush_cache_all() may be a no-op, and
>>> -        * this will fail.
>>> +        * We changing not only the virtual to physical mapping, but
>>> +        * also the physical addresses used to access memory.  We need
>>> +        * to flush all levels of cache in the system with caching
>>> +        * disabled to ensure that all data is written back.  We do this
>>> +        * with caching and write buffering disabled to ensure that
>>> +        * nothing is speculatively prefetched.
>>>           */
>>> +       cr = get_cr();
>>> +       set_cr(cr & ~(CR_I | CR_C | CR_W));
>>
>> SCTLR[3] (CR_W) is RAO/SBOP in VMSAv7. I don't think we should clear it
>> here for ARMv7.
>
> I was in two minds about that - I guess as we're expecting to only run
> this on ARMv7 CPUs, we can omit clearing the CR_W, but I'd need to add
> a comment saying that it's ARMv7 only.
>
Yep. We can do away without the CR_W change.

>> To the best of my knowledge, the page table walkers aren't affected by
>> SCTLR.C, and use the attributes in TTBR{0,1} or TTBCR when translation
>> is active (i.e. when SCTLR.M is set). So at this point they can make
>> cacheable accesses to the page tables (and allocate into the caches) in
>> the background...
>
> We had better clear those bits too then.
>
>> I think that the cache flush needs to be performed after both
>> SCTLR.{C,M} are cleared in lpae_pgtables_remap_asm, just before the page
>> table updates. So long as the relevant pieces of kernel text are
>> initially clean to the PoC, we shouldn't need to flush anything
>> beforehand.
>
> To that I say... no bloody way in hell, even once hell has frozen
> over.  It took almost a _day_ to get this much working, much of it
> was attempting to use cache flushing functions after the MMU had
> been turned off.
>
> If it was possible to make it work, I'd have done so.  It isn't, so
> please forget the idea.
>
I fully agree. I gone through the same pane while incorporating Will's
comment on similar lines last time.

>>> +ENTRY(lpae_pgtables_remap_asm)
>>> +       stmfd   sp!, {r4-r8, lr}
>>> +
>>> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
>>> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
>>> +       mcr     p15, 0, ip, c1, c0, 0
>>> +       dsb
>>> +       isb
>>
>> Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
>> point to an idmap/physical address)?
>>
>> I don't see why we need a DSB after the write to the SCTLR.
>>
dsb can be moved up after stmfd but leaving as above should be fine
as well.

>> [...]
>>
>>> +       dsb
>>> +       isb
>>> +
>>> +       mcr     p15, 0, r8, c1, c0, 0           @ re-enable MMU
>>> +       dsb
>>> +       isb
>>
>> Similarly, isn't the last DSB redundant?
>
This dsb probably can be dropped but I leave that to Russell
to decide. That one extra instruction doesn't hurt much.

Regards,
Santosh

> I've really no idea.  All I know is that the above works.  I'm rather
> sick of trying to read the ARM ARM and not understanding exactly what
> ISB and DSB actually do.
>

[PATCH 6/6] ARM: cleanup early_paging_init() calling

[santosh shilimkar]

On 4/8/2015 2:45 AM, Russell King wrote:
> Eliminate the needless nommu version of this function, and get rid of
> the proc_info_list structure argument - we no longer need this in order
> to fix up the page table entries.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>   arch/arm/kernel/setup.c | 7 ++++---
>   arch/arm/mm/mmu.c       | 6 ++----
>   arch/arm/mm/nommu.c     | 9 ---------
>   3 files changed, 6 insertions(+), 16 deletions(-)
>
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

[PATCH 5/6] ARM: re-implement physical address space switching

[Mark Rutland]

Hi,

> >> To the best of my knowledge, the page table walkers aren't affected by
> >> SCTLR.C, and use the attributes in TTBR{0,1} or TTBCR when translation
> >> is active (i.e. when SCTLR.M is set). So at this point they can make
> >> cacheable accesses to the page tables (and allocate into the caches) in
> >> the background...
> >
> > We had better clear those bits too then.
> >
> >> I think that the cache flush needs to be performed after both
> >> SCTLR.{C,M} are cleared in lpae_pgtables_remap_asm, just before the page
> >> table updates. So long as the relevant pieces of kernel text are
> >> initially clean to the PoC, we shouldn't need to flush anything
> >> beforehand.
> >
> > To that I say... no bloody way in hell, even once hell has frozen
> > over.  It took almost a _day_ to get this much working, much of it
> > was attempting to use cache flushing functions after the MMU had
> > been turned off.
> >
> > If it was possible to make it work, I'd have done so.  It isn't, so
> > please forget the idea.
> >
> I fully agree. I gone through the same pane while incorporating Will's
> comment on similar lines last time.

I'm surprised that it's so painful to get that working. I don't have a
system I could test this on, so unfortunately I can't offer much help
there.

> >>> +ENTRY(lpae_pgtables_remap_asm)
> >>> +       stmfd   sp!, {r4-r8, lr}
> >>> +
> >>> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
> >>> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
> >>> +       mcr     p15, 0, ip, c1, c0, 0
> >>> +       dsb
> >>> +       isb
> >>
> >> Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
> >> point to an idmap/physical address)?
> >>
> >> I don't see why we need a DSB after the write to the SCTLR.
> >>
> dsb can be moved up after stmfd but leaving as above should be fine
> as well.

I don't think that it's safe to leave it where it is. Currently the
STMFD could be reordered w.r.t. the cp15 accesses, and hence the write
may occur with translation disabled (and would go to the wrong physical
address).

We need to ensure that the STMFD is executed before the MCR potentially
changes the execution context. The DSB will ensure that in addition to
ensuring completion of the write (i.e. it isn't left sat in a write
buffer).

> >> [...]
> >>
> >>> +       dsb
> >>> +       isb
> >>> +
> >>> +       mcr     p15, 0, r8, c1, c0, 0           @ re-enable MMU
> >>> +       dsb
> >>> +       isb
> >>
> >> Similarly, isn't the last DSB redundant?
> >
> This dsb probably can be dropped but I leave that to Russell
> to decide. That one extra instruction doesn't hurt much.

I don't see that it adds anything to the ISB given the DSB; ISB prior to
the write to the SCTLR -- there's nothing it would ensure completion of
that the first DSB won't already have.

Mark.

[PATCH 5/6] ARM: re-implement physical address space switching

[santosh shilimkar]

On 4/15/2015 5:07 AM, Mark Rutland wrote:
> Hi,
>
[..]

>>>>> +ENTRY(lpae_pgtables_remap_asm)
>>>>> +       stmfd   sp!, {r4-r8, lr}
>>>>> +
>>>>> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
>>>>> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
>>>>> +       mcr     p15, 0, ip, c1, c0, 0
>>>>> +       dsb
>>>>> +       isb
>>>>
>>>> Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
>>>> point to an idmap/physical address)?
>>>>
>>>> I don't see why we need a DSB after the write to the SCTLR.
>>>>
>> dsb can be moved up after stmfd but leaving as above should be fine
>> as well.
>
> I don't think that it's safe to leave it where it is. Currently the
> STMFD could be reordered w.r.t. the cp15 accesses, and hence the write
> may occur with translation disabled (and would go to the wrong physical
> address).
>
> We need to ensure that the STMFD is executed before the MCR potentially
> changes the execution context. The DSB will ensure that in addition to
> ensuring completion of the write (i.e. it isn't left sat in a write
> buffer).
>
I see your point. Thanks for expanding it.

Regards,
Santosh

[PATCH 5/6] ARM: re-implement physical address space switching

[Mark Rutland]

Hi,

> >>>>> +ENTRY(lpae_pgtables_remap_asm)
> >>>>> +       stmfd   sp!, {r4-r8, lr}
> >>>>> +
> >>>>> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
> >>>>> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
> >>>>> +       mcr     p15, 0, ip, c1, c0, 0
> >>>>> +       dsb
> >>>>> +       isb
> >>>>
> >>>> Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
> >>>> point to an idmap/physical address)?
> >>>>
> >>>> I don't see why we need a DSB after the write to the SCTLR.
> >>>>
> >> dsb can be moved up after stmfd but leaving as above should be fine
> >> as well.
> >
> > I don't think that it's safe to leave it where it is. Currently the
> > STMFD could be reordered w.r.t. the cp15 accesses, and hence the write
> > may occur with translation disabled (and would go to the wrong physical
> > address).
> >
> > We need to ensure that the STMFD is executed before the MCR potentially
> > changes the execution context. The DSB will ensure that in addition to
> > ensuring completion of the write (i.e. it isn't left sat in a write
> > buffer).
> >
> I see your point. Thanks for expanding it.

It turns out that I was incorrect in my assertion, and the reordering I
suggested above can't happen. The ARMv7 ARM states:

	Any direct write to a system control register is guaranteed not
	to affect any instruction that appears, in program
	order, before the instruction that performed the direct write

Which means that the STMFD cannot be affected by the later cp15 write to
the SCTLR, and so the DSB does not need to be moved before the MCR.

I apologise for adding to the confusion there.

Mark.

[PATCH 5/6] ARM: re-implement physical address space switching

[Russell King - ARM Linux]

On Thu, Apr 23, 2015 at 12:24:58PM +0100, Mark Rutland wrote:
> Hi,
> 
> > >>>>> +ENTRY(lpae_pgtables_remap_asm)
> > >>>>> +       stmfd   sp!, {r4-r8, lr}
> > >>>>> +
> > >>>>> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
> > >>>>> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
> > >>>>> +       mcr     p15, 0, ip, c1, c0, 0
> > >>>>> +       dsb
> > >>>>> +       isb
> > >>>>
> > >>>> Shouldn't the DSB be between the STMFD and the MCR (given the SP doesn't
> > >>>> point to an idmap/physical address)?
> > >>>>
> > >>>> I don't see why we need a DSB after the write to the SCTLR.
> > >>>>
> > >> dsb can be moved up after stmfd but leaving as above should be fine
> > >> as well.
> > >
> > > I don't think that it's safe to leave it where it is. Currently the
> > > STMFD could be reordered w.r.t. the cp15 accesses, and hence the write
> > > may occur with translation disabled (and would go to the wrong physical
> > > address).
> > >
> > > We need to ensure that the STMFD is executed before the MCR potentially
> > > changes the execution context. The DSB will ensure that in addition to
> > > ensuring completion of the write (i.e. it isn't left sat in a write
> > > buffer).
> > >
> > I see your point. Thanks for expanding it.
> 
> It turns out that I was incorrect in my assertion, and the reordering I
> suggested above can't happen. The ARMv7 ARM states:
> 
> 	Any direct write to a system control register is guaranteed not
> 	to affect any instruction that appears, in program
> 	order, before the instruction that performed the direct write
> 
> Which means that the STMFD cannot be affected by the later cp15 write to
> the SCTLR, and so the DSB does not need to be moved before the MCR.
> 
> I apologise for adding to the confusion there.

So does this mean this patch gets an ack now?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 5/6] ARM: re-implement physical address space switching

[Mark Rutland]

Hi Russell,

> > It turns out that I was incorrect in my assertion, and the reordering I
> > suggested above can't happen. The ARMv7 ARM states:
> > 
> > 	Any direct write to a system control register is guaranteed not
> > 	to affect any instruction that appears, in program
> > 	order, before the instruction that performed the direct write
> > 
> > Which means that the STMFD cannot be affected by the later cp15 write to
> > the SCTLR, and so the DSB does not need to be moved before the MCR.
> > 
> > I apologise for adding to the confusion there.
> 
> So does this mean this patch gets an ack now?

I assumed there was going to be a respin for the CR_W change?

There's also the dodginess w.r.t. the page table walkers that I can't
see is solvable short of disabling the MMU prior to the flush, though I
understand you've NAKed that approach.

Thanks,
Mark.

[PATCH 5/6] ARM: re-implement physical address space switching

[Russell King - ARM Linux]

On Wed, May 06, 2015 at 11:37:37AM +0100, Mark Rutland wrote:
> Hi Russell,
> 
> > > It turns out that I was incorrect in my assertion, and the reordering I
> > > suggested above can't happen. The ARMv7 ARM states:
> > > 
> > > 	Any direct write to a system control register is guaranteed not
> > > 	to affect any instruction that appears, in program
> > > 	order, before the instruction that performed the direct write
> > > 
> > > Which means that the STMFD cannot be affected by the later cp15 write to
> > > the SCTLR, and so the DSB does not need to be moved before the MCR.
> > > 
> > > I apologise for adding to the confusion there.
> > 
> > So does this mean this patch gets an ack now?
> 
> I assumed there was going to be a respin for the CR_W change?
> 
> There's also the dodginess w.r.t. the page table walkers that I can't
> see is solvable short of disabling the MMU prior to the flush, though I
> understand you've NAKed that approach.

Why?  Are you saying that after:

+       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
+       bic     ip, r8, #CR_M                   @ disable caches and MMU
+       mcr     p15, 0, ip, c1, c0, 0
+       dsb
+       isb

the page table walkers are still actively walking the page table?

That to me sounds like a hardware bug.  The point of this is to shut
down the MMU, _then_ update the page tables, and _then_ to re-enable
the MMU to explicitly avoid problems with the page table walkers.

>From what you're saying, this is just not possible, and we should
just throw Keystone SoCs in the bin...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 5/6] ARM: re-implement physical address space switching

[Mark Rutland]

On Wed, May 06, 2015 at 12:33:13PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 06, 2015 at 11:37:37AM +0100, Mark Rutland wrote:
> > Hi Russell,
> > 
> > > > It turns out that I was incorrect in my assertion, and the reordering I
> > > > suggested above can't happen. The ARMv7 ARM states:
> > > > 
> > > > 	Any direct write to a system control register is guaranteed not
> > > > 	to affect any instruction that appears, in program
> > > > 	order, before the instruction that performed the direct write
> > > > 
> > > > Which means that the STMFD cannot be affected by the later cp15 write to
> > > > the SCTLR, and so the DSB does not need to be moved before the MCR.
> > > > 
> > > > I apologise for adding to the confusion there.
> > > 
> > > So does this mean this patch gets an ack now?
> > 
> > I assumed there was going to be a respin for the CR_W change?
> > 
> > There's also the dodginess w.r.t. the page table walkers that I can't
> > see is solvable short of disabling the MMU prior to the flush, though I
> > understand you've NAKed that approach.
> 
> Why? 

I was on about the pre-assembly portion:

	cr = get_cr();
	set_cr(cr & ~(CR_I | CR_C | CR_W));
	flush_cache_all();

With the MMU on at this point the page table walkers can race with the
set/way maintenance. It also relies on the compiler not making stack
accesses between the SCTLR write and the completion of flush_cache_all,
which is likely but not guranteed.

So this won't necessarily flush out the data it seems to be intended to.

> Are you saying that after:
> 
> +       mrc     p15, 0, r8, c1, c0, 0           @ read control reg
> +       bic     ip, r8, #CR_M                   @ disable caches and MMU
> +       mcr     p15, 0, ip, c1, c0, 0
> +       dsb
> +       isb
> 
> the page table walkers are still actively walking the page table?
> 
> That to me sounds like a hardware bug.  The point of this is to shut
> down the MMU, _then_ update the page tables, and _then_ to re-enable
> the MMU to explicitly avoid problems with the page table walkers.

I agree that after this point it would be a bug for the page table
walkers to make cacheable accesses.

Thanks,
Mark.

[PATCH 5/6] ARM: re-implement physical address space switching

[Russell King - ARM Linux]

On Wed, May 06, 2015 at 04:33:38PM +0100, Mark Rutland wrote:
> On Wed, May 06, 2015 at 12:33:13PM +0100, Russell King - ARM Linux wrote:
> > On Wed, May 06, 2015 at 11:37:37AM +0100, Mark Rutland wrote:
> > > Hi Russell,
> > > 
> > > > > It turns out that I was incorrect in my assertion, and the reordering I
> > > > > suggested above can't happen. The ARMv7 ARM states:
> > > > > 
> > > > > 	Any direct write to a system control register is guaranteed not
> > > > > 	to affect any instruction that appears, in program
> > > > > 	order, before the instruction that performed the direct write
> > > > > 
> > > > > Which means that the STMFD cannot be affected by the later cp15 write to
> > > > > the SCTLR, and so the DSB does not need to be moved before the MCR.
> > > > > 
> > > > > I apologise for adding to the confusion there.
> > > > 
> > > > So does this mean this patch gets an ack now?
> > > 
> > > I assumed there was going to be a respin for the CR_W change?
> > > 
> > > There's also the dodginess w.r.t. the page table walkers that I can't
> > > see is solvable short of disabling the MMU prior to the flush, though I
> > > understand you've NAKed that approach.
> > 
> > Why? 
> 
> I was on about the pre-assembly portion:
> 
> 	cr = get_cr();
> 	set_cr(cr & ~(CR_I | CR_C | CR_W));
> 	flush_cache_all();
> 
> With the MMU on at this point the page table walkers can race with the
> set/way maintenance. It also relies on the compiler not making stack
> accesses between the SCTLR write and the completion of flush_cache_all,
> which is likely but not guranteed.

Yes, I suppose you're right, but there's really no other way to do
this other than coding up CPU specific code.

What we'd need to do is to code up a specific assembly routine which
disables the I and C control register flags, flushes the cache, jumps
to the physical alias and then disables the MMU all without touching
memory.  That's far too much effort.  Like I say, maybe we should just
bin Keystone for this crap...

I'm not interested in trying to "fix" this code any further - as I
said earlier, it took quite a while to get this code working on
Keystone, which is really all we care about.  Anyone else who copies
the abortion that TI made in Keystone should be shot. :)

While the community has the luxury of saying "we don't like it, it's
TI's problem" - which is what was done when the problems were first
pointed out by Will, the net result is that this problem became my
problem to try and sort out.

So, if you have a better idea how to fix this, I'm all ears.

What I'm certain of though is that this is an improvement over what's
in the kernel today, and so should go in irrespective of whether it's
totally bullet-proof or not.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 5/6] ARM: re-implement physical address space switching

[Mark Rutland]

Hi Russell,

> > 	cr = get_cr();
> > 	set_cr(cr & ~(CR_I | CR_C | CR_W));
> > 	flush_cache_all();
> > 
> > With the MMU on at this point the page table walkers can race with the
> > set/way maintenance. It also relies on the compiler not making stack
> > accesses between the SCTLR write and the completion of flush_cache_all,
> > which is likely but not guranteed.
> 
> Yes, I suppose you're right, but there's really no other way to do
> this other than coding up CPU specific code.
> 
> What we'd need to do is to code up a specific assembly routine which
> disables the I and C control register flags, flushes the cache, jumps
> to the physical alias and then disables the MMU all without touching
> memory.  That's far too much effort.  Like I say, maybe we should just
> bin Keystone for this crap...
> 
> I'm not interested in trying to "fix" this code any further - as I
> said earlier, it took quite a while to get this code working on
> Keystone, which is really all we care about.  Anyone else who copies
> the abortion that TI made in Keystone should be shot. :)
> 
> While the community has the luxury of saying "we don't like it, it's
> TI's problem" - which is what was done when the problems were first
> pointed out by Will, the net result is that this problem became my
> problem to try and sort out.
> 
> So, if you have a better idea how to fix this, I'm all ears.
>
> What I'm certain of though is that this is an improvement over what's
> in the kernel today, and so should go in irrespective of whether it's
> totally bullet-proof or not.

I don't disagree with that. :)

w.r.t. "better" ideas, my initial thought was that we could move the
SCTLR.{C,M} clearing into lpae_pgtables_remap_asm, along with a call to
v7_flush_dcache_all (as it should be in the physical mapping). So long
as the kernel text was initially clean to the PoC I'd expect that to
work, but I understood from your initial reply you'd tried that and
something went wrong.

Perhaps we can look into that later.

Thanks,
Mark.

[PATCH 5/6] ARM: re-implement physical address space switching

[Will Deacon]

On Wed, May 06, 2015 at 05:14:06PM +0100, Mark Rutland wrote:
> > > 	cr = get_cr();
> > > 	set_cr(cr & ~(CR_I | CR_C | CR_W));
> > > 	flush_cache_all();
> > > 
> > > With the MMU on at this point the page table walkers can race with the
> > > set/way maintenance. It also relies on the compiler not making stack
> > > accesses between the SCTLR write and the completion of flush_cache_all,
> > > which is likely but not guranteed.
> > 
> > Yes, I suppose you're right, but there's really no other way to do
> > this other than coding up CPU specific code.
> > 
> > What we'd need to do is to code up a specific assembly routine which
> > disables the I and C control register flags, flushes the cache, jumps
> > to the physical alias and then disables the MMU all without touching
> > memory.  That's far too much effort.  Like I say, maybe we should just
> > bin Keystone for this crap...
> > 
> > I'm not interested in trying to "fix" this code any further - as I
> > said earlier, it took quite a while to get this code working on
> > Keystone, which is really all we care about.  Anyone else who copies
> > the abortion that TI made in Keystone should be shot. :)
> > 
> > While the community has the luxury of saying "we don't like it, it's
> > TI's problem" - which is what was done when the problems were first
> > pointed out by Will, the net result is that this problem became my
> > problem to try and sort out.
> > 
> > So, if you have a better idea how to fix this, I'm all ears.
> >
> > What I'm certain of though is that this is an improvement over what's
> > in the kernel today, and so should go in irrespective of whether it's
> > totally bullet-proof or not.
> 
> I don't disagree with that. :)
> 
> w.r.t. "better" ideas, my initial thought was that we could move the
> SCTLR.{C,M} clearing into lpae_pgtables_remap_asm, along with a call to
> v7_flush_dcache_all (as it should be in the physical mapping). So long
> as the kernel text was initially clean to the PoC I'd expect that to
> work, but I understood from your initial reply you'd tried that and
> something went wrong.
> 
> Perhaps we can look into that later.

I think a comment summarising the conclusion of this thread above the
cache flush and rmk's choice of adjective to describe the keystone SoC
would be sufficient.

Will