[PATCH v2] ARM: kexec: selective MMU identity mapping

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Per Fransson]

When restarting using the kernel kexec functionality the MMU
needs to be turned off. Any code which does this needs to use
identity mapped addresses to get reliable results. In the ARM
kexec case this identity mapping is done:

- using the page table of the current task

- for all addresses normally used by user space,
  i.e. 0x00000000-PAGE_OFFSET

If kexec is used at a kernel crash to collect a core dump this
means that we lose important information.

This is what this patches does:

* Actually turns off the MMU, which has been omitted by mistake

* Sets up a more selective identity mapping

* Restores the old mapping once the MMU is off

Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
---
v2 changes:

* now uses (modified versions of) the identity mapping functions in idmap.c
  as they look in 2.6.38-rc1. Some pud-level code has been added there in
  linux-next.

 arch/arm/include/asm/pgtable.h    |    4 +++-
 arch/arm/kernel/machine_kexec.c   |   26 +++++++++++++++++++++-----
 arch/arm/kernel/relocate_kernel.S |   23 +++++++++++++++++++++++
 arch/arm/kernel/smp.c             |    7 ++++---
 arch/arm/mm/idmap.c               |   20 +++++++++++++++-----
 arch/arm/mm/proc-v7.S             |    4 ++++
 include/linux/kexec.h             |    6 +++++-
 7 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index ebcb643..08dd591 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -458,6 +458,7 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 #define kern_addr_valid(addr)	(1)
 
 #include <asm-generic/pgtable.h>
+#include <linux/kexec.h>
 
 /*
  * We provide our own arch_get_unmapped_area to cope with VIPT caches.
@@ -473,7 +474,8 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 
 #define pgtable_cache_init() do { } while (0)
 
-void identity_mapping_add(pgd_t *, unsigned long, unsigned long);
+void identity_mapping_add(pgd_t *, unsigned long, unsigned long,
+			  struct kexec_mmu_ent *);
 void identity_mapping_del(pgd_t *, unsigned long, unsigned long);
 
 #endif /* !__ASSEMBLY__ */
diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 30ead13..bd53684 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -16,8 +16,6 @@
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned int relocate_new_kernel_size;
 
-extern void setup_mm_for_reboot(char mode);
-
 extern unsigned long kexec_start_address;
 extern unsigned long kexec_indirection_page;
 extern unsigned long kexec_mach_type;
@@ -79,9 +77,9 @@ void machine_kexec(struct kimage *image)
 {
 	unsigned long page_list;
 	unsigned long reboot_code_buffer_phys;
+	unsigned long cpu_reset_phys;
 	void *reboot_code_buffer;
 
-
 	page_list = image->head & PAGE_MASK;
 
 	/* we need both effective and real address here */
@@ -95,18 +93,36 @@ void machine_kexec(struct kimage *image)
 	kexec_mach_type = machine_arch_type;
 	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
 
+	/* Identity map the code which turns off the mmu (cpu_reset) and
+	   the code which will be executed immediately afterwards
+	   (relocate_new_kernel).
+	   Store the old entries so they can be restored. */
+	/* cpu_reset cannot be used directly when MULTI_CPU is true, see
+	   cpu-multi32.h, instead processor.reset will have to be used */
+#ifdef MULTI_CPU
+	cpu_reset_phys = virt_to_phys(processor.reset);
+#else
+	cpu_reset_phys = virt_to_phys(cpu_reset);
+#endif
+
+	identity_mapping_add(current->active_mm->pgd, cpu_reset_phys,
+			     ALIGN(cpu_reset_phys, PGDIR_SIZE)+PGDIR_SIZE,
+			     &kexec_mmu_ents[0]);
+	identity_mapping_add(current->active_mm->pgd, reboot_code_buffer_phys,
+			     ALIGN(reboot_code_buffer_phys, PGDIR_SIZE)
+			     + PGDIR_SIZE, &kexec_mmu_ents[2]);
+	local_flush_tlb_all();
+
 	/* copy our kernel relocation code to the control code page */
 	memcpy(reboot_code_buffer,
 	       relocate_new_kernel, relocate_new_kernel_size);
 
-
 	flush_icache_range((unsigned long) reboot_code_buffer,
 			   (unsigned long) reboot_code_buffer + KEXEC_CONTROL_PAGE_SIZE);
 	printk(KERN_INFO "Bye!\n");
 
 	local_irq_disable();
 	local_fiq_disable();
-	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
 	flush_cache_all();
 	outer_flush_all();
 	outer_disable();
diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
index 9cf4cbf..a5e155e 100644
--- a/arch/arm/kernel/relocate_kernel.S
+++ b/arch/arm/kernel/relocate_kernel.S
@@ -7,6 +7,24 @@
 	.globl relocate_new_kernel
 relocate_new_kernel:
 
+	/* We get here when the MMU is in a transitional state.
+	   Wait for the virtual address mapping to wear off before
+	   overwriting the identity mappings (set up for the sake
+	   of MMU disabling) with the previous mappings. */
+	ldr	r0, =100
+0:
+	subs	r0, r0, #1
+	beq	0b
+
+	adr	r0, kexec_mmu_ents
+	.rept 4
+	ldr	r1, [r0], #4
+	ldr	r2, [r0], #4
+	str	r2, [r1], #4
+	ldr	r2, [r0], #4
+	str	r2, [r1], #4
+	.endr
+
 	ldr	r0,kexec_indirection_page
 	ldr	r1,kexec_start_address
 
@@ -69,6 +87,11 @@ kexec_start_address:
 kexec_indirection_page:
 	.long	0x0
 
+	.globl kexec_mmu_ents
+kexec_mmu_ents:
+	.space 4*12, 0
+
+
 	.globl kexec_mach_type
 kexec_mach_type:
 	.long	0x0
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 4539ebc..607b6a8 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -93,10 +93,11 @@ int __cpuinit __cpu_up(unsigned int cpu)
 
 	if (PHYS_OFFSET != PAGE_OFFSET) {
 #ifndef CONFIG_HOTPLUG_CPU
-		identity_mapping_add(pgd, __pa(__init_begin), __pa(__init_end));
+		identity_mapping_add(pgd, __pa(__init_begin), __pa(__init_end),
+				     NULL);
 #endif
-		identity_mapping_add(pgd, __pa(_stext), __pa(_etext));
-		identity_mapping_add(pgd, __pa(_sdata), __pa(_edata));
+		identity_mapping_add(pgd, __pa(_stext), __pa(_etext), NULL);
+		identity_mapping_add(pgd, __pa(_sdata), __pa(_edata), NULL);
 	}
 
 	/*
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index 5729944..d5d313d 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -1,14 +1,19 @@
 #include <linux/kernel.h>
+#include <linux/kexec.h>
 
 #include <asm/cputype.h>
 #include <asm/pgalloc.h>
 #include <asm/pgtable.h>
 
 static void idmap_add_pmd(pgd_t *pgd, unsigned long addr, unsigned long end,
-	unsigned long prot)
+			  unsigned long prot, struct kexec_mmu_ent *store)
 {
 	pmd_t *pmd = pmd_offset(pgd, addr);
-
+	if (store) {
+		pmd_t *pmd_store = pmd_offset(&store->store, addr);
+		pmd_store[0] = pmd[0];
+		pmd_store[1] = pmd[1];
+	}
 	addr = (addr & PMD_MASK) | prot;
 	pmd[0] = __pmd(addr);
 	addr += SECTION_SIZE;
@@ -16,7 +21,8 @@ static void idmap_add_pmd(pgd_t *pgd, unsigned long addr, unsigned long end,
 	flush_pmd_entry(pmd);
 }
 
-void identity_mapping_add(pgd_t *pgd, unsigned long addr, unsigned long end)
+void identity_mapping_add(pgd_t *pgd, unsigned long addr, unsigned long end,
+			  struct kexec_mmu_ent *store)
 {
 	unsigned long prot, next;
 
@@ -27,7 +33,11 @@ void identity_mapping_add(pgd_t *pgd, unsigned long addr, unsigned long end)
 	pgd += pgd_index(addr);
 	do {
 		next = pgd_addr_end(addr, end);
-		idmap_add_pmd(pgd, addr, next, prot);
+		idmap_add_pmd(pgd, addr, next, prot, store);
+		if (store) {
+			store->ptr = (pgd_t *)virt_to_phys(pgd);
+			store++;
+		}
 	} while (pgd++, addr = next, addr != end);
 }
 
@@ -62,6 +72,6 @@ void setup_mm_for_reboot(char mode)
 	 * we don't have any user-mode mappings so we use the context that we
 	 * "borrowed".
 	 */
-	identity_mapping_add(current->active_mm->pgd, 0, TASK_SIZE);
+	identity_mapping_add(current->active_mm->pgd, 0, TASK_SIZE, NULL);
 	local_flush_tlb_all();
 }
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 0c1172b..762bb43 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -61,6 +61,10 @@ ENDPROC(cpu_v7_proc_fin)
  */
 	.align	5
 ENTRY(cpu_v7_reset)
+	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses
+	mrc	p15, 0, ip, c1, c0, 0		@ ctrl register
+	bic	ip, ip, #0x0001			@ ...............m
+	mcr	p15, 0, ip, c1, c0, 0		@ ctrl register
 	mov	pc, r0
 ENDPROC(cpu_v7_reset)
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 03e8e8d..6a1345b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -106,7 +106,11 @@ struct kimage {
 #endif
 };
 
-
+struct kexec_mmu_ent {
+	pgd_t *ptr;
+	pgd_t store;
+};
+extern struct kexec_mmu_ent kexec_mmu_ents[4];
 
 /* kexec interface functions */
 extern void machine_kexec(struct kimage *image);
-- 
1.7.3.2

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Nicolas Pitre]

On Wed, 2 Feb 2011, Per Fransson wrote:

> When restarting using the kernel kexec functionality the MMU
> needs to be turned off. Any code which does this needs to use
> identity mapped addresses to get reliable results. In the ARM
> kexec case this identity mapping is done:
> 
> - using the page table of the current task
> 
> - for all addresses normally used by user space,
>   i.e. 0x00000000-PAGE_OFFSET
> 
> If kexec is used at a kernel crash to collect a core dump this
> means that we lose important information.

setup_mm_for_reboot is rather heavy handed indeed.

> This is what this patches does:
> 
> * Actually turns off the MMU, which has been omitted by mistake

Please make this into a patch of its own.

> * Sets up a more selective identity mapping
> 
> * Restores the old mapping once the MMU is off
> 
> Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>

More comments below.

> ---
> v2 changes:
> 
> * now uses (modified versions of) the identity mapping functions in idmap.c
>   as they look in 2.6.38-rc1. Some pud-level code has been added there in
>   linux-next.
> 
>  arch/arm/include/asm/pgtable.h    |    4 +++-
>  arch/arm/kernel/machine_kexec.c   |   26 +++++++++++++++++++++-----
>  arch/arm/kernel/relocate_kernel.S |   23 +++++++++++++++++++++++
>  arch/arm/kernel/smp.c             |    7 ++++---
>  arch/arm/mm/idmap.c               |   20 +++++++++++++++-----
>  arch/arm/mm/proc-v7.S             |    4 ++++
>  include/linux/kexec.h             |    6 +++++-
>  7 files changed, 75 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index ebcb643..08dd591 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -458,6 +458,7 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>  #define kern_addr_valid(addr)	(1)
>  
>  #include <asm-generic/pgtable.h>
> +#include <linux/kexec.h>
>  
>  /*
>   * We provide our own arch_get_unmapped_area to cope with VIPT caches.
> @@ -473,7 +474,8 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>  
>  #define pgtable_cache_init() do { } while (0)
>  
> -void identity_mapping_add(pgd_t *, unsigned long, unsigned long);
> +void identity_mapping_add(pgd_t *, unsigned long, unsigned long,
> +			  struct kexec_mmu_ent *);

I would try to avoid contaminating such a generic function with kexec 
specific structures.

>  void identity_mapping_del(pgd_t *, unsigned long, unsigned long);
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index 30ead13..bd53684 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -16,8 +16,6 @@
>  extern const unsigned char relocate_new_kernel[];
>  extern const unsigned int relocate_new_kernel_size;
>  
> -extern void setup_mm_for_reboot(char mode);
> -
>  extern unsigned long kexec_start_address;
>  extern unsigned long kexec_indirection_page;
>  extern unsigned long kexec_mach_type;
> @@ -79,9 +77,9 @@ void machine_kexec(struct kimage *image)
>  {
>  	unsigned long page_list;
>  	unsigned long reboot_code_buffer_phys;
> +	unsigned long cpu_reset_phys;
>  	void *reboot_code_buffer;
>  
> -
>  	page_list = image->head & PAGE_MASK;
>  
>  	/* we need both effective and real address here */
> @@ -95,18 +93,36 @@ void machine_kexec(struct kimage *image)
>  	kexec_mach_type = machine_arch_type;
>  	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
>  
> +	/* Identity map the code which turns off the mmu (cpu_reset) and
> +	   the code which will be executed immediately afterwards
> +	   (relocate_new_kernel).
> +	   Store the old entries so they can be restored. */
> +	/* cpu_reset cannot be used directly when MULTI_CPU is true, see
> +	   cpu-multi32.h, instead processor.reset will have to be used */
> +#ifdef MULTI_CPU
> +	cpu_reset_phys = virt_to_phys(processor.reset);
> +#else
> +	cpu_reset_phys = virt_to_phys(cpu_reset);
> +#endif

Why not always using processor.reset in all cases instead?

> +
> +	identity_mapping_add(current->active_mm->pgd, cpu_reset_phys,
> +			     ALIGN(cpu_reset_phys, PGDIR_SIZE)+PGDIR_SIZE,
> +			     &kexec_mmu_ents[0]);
> +	identity_mapping_add(current->active_mm->pgd, reboot_code_buffer_phys,
> +			     ALIGN(reboot_code_buffer_phys, PGDIR_SIZE)
> +			     + PGDIR_SIZE, &kexec_mmu_ents[2]);
> +	local_flush_tlb_all();
> +
>  	/* copy our kernel relocation code to the control code page */
>  	memcpy(reboot_code_buffer,
>  	       relocate_new_kernel, relocate_new_kernel_size);
>  
> -
>  	flush_icache_range((unsigned long) reboot_code_buffer,
>  			   (unsigned long) reboot_code_buffer + KEXEC_CONTROL_PAGE_SIZE);
>  	printk(KERN_INFO "Bye!\n");
>  
>  	local_irq_disable();
>  	local_fiq_disable();
> -	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
>  	flush_cache_all();
>  	outer_flush_all();
>  	outer_disable();
> diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
> index 9cf4cbf..a5e155e 100644
> --- a/arch/arm/kernel/relocate_kernel.S
> +++ b/arch/arm/kernel/relocate_kernel.S
> @@ -7,6 +7,24 @@
>  	.globl relocate_new_kernel
>  relocate_new_kernel:
>  
> +	/* We get here when the MMU is in a transitional state.
> +	   Wait for the virtual address mapping to wear off before
> +	   overwriting the identity mappings (set up for the sake
> +	   of MMU disabling) with the previous mappings. */
> +	ldr	r0, =100
> +0:
> +	subs	r0, r0, #1
> +	beq	0b

Two problems here:

1) This will never loop as you load r0 with 100, decrement it once, and 
   then test for zero to branch back, which at this point it won't.

2) There is certainly a better architecturally defined way to ensure the 
   MMU is actually disabled, such as reading back the control register 
   and queuing a data dependency on the register it was read into?

In any case, given that the loop in 1) above is broken, we can assume 
this wasn't needed anyway in your testing.

> +
> +	adr	r0, kexec_mmu_ents
> +	.rept 4
> +	ldr	r1, [r0], #4
> +	ldr	r2, [r0], #4
> +	str	r2, [r1], #4
> +	ldr	r2, [r0], #4
> +	str	r2, [r1], #4
> +	.endr

This is burying knowledge about the pmd implementation a bit too deep.  
If the pmd implementation changes, such as with the A15 page table 
format, then this may easily be missed.  Better create a generic copy 
list with <data_size><data_dest_addr><data...> and zero terminate it.  
This would also allow for a variable number of pmds to restore, as right 
now your code won't work as expected if the identity mapping is crossing 
a pmd boundary.

> +
>  	ldr	r0,kexec_indirection_page
>  	ldr	r1,kexec_start_address
>  
> @@ -69,6 +87,11 @@ kexec_start_address:
>  kexec_indirection_page:
>  	.long	0x0
>  
> +	.globl kexec_mmu_ents
> +kexec_mmu_ents:
> +	.space 4*12, 0
> +
> +
>  	.globl kexec_mach_type
>  kexec_mach_type:
>  	.long	0x0
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4539ebc..607b6a8 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -93,10 +93,11 @@ int __cpuinit __cpu_up(unsigned int cpu)
>  
>  	if (PHYS_OFFSET != PAGE_OFFSET) {
>  #ifndef CONFIG_HOTPLUG_CPU
> -		identity_mapping_add(pgd, __pa(__init_begin), __pa(__init_end));
> +		identity_mapping_add(pgd, __pa(__init_begin), __pa(__init_end),
> +				     NULL);
>  #endif
> -		identity_mapping_add(pgd, __pa(_stext), __pa(_etext));
> -		identity_mapping_add(pgd, __pa(_sdata), __pa(_edata));
> +		identity_mapping_add(pgd, __pa(_stext), __pa(_etext), NULL);
> +		identity_mapping_add(pgd, __pa(_sdata), __pa(_edata), NULL);
>  	}
>  
>  	/*
> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> index 5729944..d5d313d 100644
> --- a/arch/arm/mm/idmap.c
> +++ b/arch/arm/mm/idmap.c
> @@ -1,14 +1,19 @@
>  #include <linux/kernel.h>
> +#include <linux/kexec.h>
>  
>  #include <asm/cputype.h>
>  #include <asm/pgalloc.h>
>  #include <asm/pgtable.h>
>  
>  static void idmap_add_pmd(pgd_t *pgd, unsigned long addr, unsigned long end,
> -	unsigned long prot)
> +			  unsigned long prot, struct kexec_mmu_ent *store)
>  {
>  	pmd_t *pmd = pmd_offset(pgd, addr);
> -
> +	if (store) {
> +		pmd_t *pmd_store = pmd_offset(&store->store, addr);
> +		pmd_store[0] = pmd[0];
> +		pmd_store[1] = pmd[1];
> +	}
>  	addr = (addr & PMD_MASK) | prot;
>  	pmd[0] = __pmd(addr);
>  	addr += SECTION_SIZE;
> @@ -16,7 +21,8 @@ static void idmap_add_pmd(pgd_t *pgd, unsigned long addr, unsigned long end,
>  	flush_pmd_entry(pmd);
>  }
>  
> -void identity_mapping_add(pgd_t *pgd, unsigned long addr, unsigned long end)
> +void identity_mapping_add(pgd_t *pgd, unsigned long addr, unsigned long end,
> +			  struct kexec_mmu_ent *store)
>  {
>  	unsigned long prot, next;
>  
> @@ -27,7 +33,11 @@ void identity_mapping_add(pgd_t *pgd, unsigned long addr, unsigned long end)
>  	pgd += pgd_index(addr);
>  	do {
>  		next = pgd_addr_end(addr, end);
> -		idmap_add_pmd(pgd, addr, next, prot);
> +		idmap_add_pmd(pgd, addr, next, prot, store);
> +		if (store) {
> +			store->ptr = (pgd_t *)virt_to_phys(pgd);
> +			store++;
> +		}
>  	} while (pgd++, addr = next, addr != end);
>  }
>  
> @@ -62,6 +72,6 @@ void setup_mm_for_reboot(char mode)
>  	 * we don't have any user-mode mappings so we use the context that we
>  	 * "borrowed".
>  	 */
> -	identity_mapping_add(current->active_mm->pgd, 0, TASK_SIZE);
> +	identity_mapping_add(current->active_mm->pgd, 0, TASK_SIZE, NULL);
>  	local_flush_tlb_all();
>  }
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index 0c1172b..762bb43 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -61,6 +61,10 @@ ENDPROC(cpu_v7_proc_fin)
>   */
>  	.align	5
>  ENTRY(cpu_v7_reset)
> +	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses
> +	mrc	p15, 0, ip, c1, c0, 0		@ ctrl register
> +	bic	ip, ip, #0x0001			@ ...............m
> +	mcr	p15, 0, ip, c1, c0, 0		@ ctrl register
>  	mov	pc, r0
>  ENDPROC(cpu_v7_reset)
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 03e8e8d..6a1345b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -106,7 +106,11 @@ struct kimage {
>  #endif
>  };
>  
> -
> +struct kexec_mmu_ent {
> +	pgd_t *ptr;
> +	pgd_t store;
> +};
> +extern struct kexec_mmu_ent kexec_mmu_ents[4];
>  
>  /* kexec interface functions */
>  extern void machine_kexec(struct kimage *image);
> -- 
> 1.7.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Russell King - ARM Linux]

On Wed, Feb 02, 2011 at 03:43:29PM +0100, Per Fransson wrote:
> When restarting using the kernel kexec functionality the MMU
> needs to be turned off. Any code which does this needs to use
> identity mapped addresses to get reliable results. In the ARM
> kexec case this identity mapping is done:
> 
> - using the page table of the current task
> 
> - for all addresses normally used by user space,
>   i.e. 0x00000000-PAGE_OFFSET
> 
> If kexec is used at a kernel crash to collect a core dump this
> means that we lose important information.
> 
> This is what this patches does:
> 
> * Actually turns off the MMU, which has been omitted by mistake
> 
> * Sets up a more selective identity mapping
> 
> * Restores the old mapping once the MMU is off
> 
> Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
> ---
> v2 changes:
> 
> * now uses (modified versions of) the identity mapping functions in idmap.c
>   as they look in 2.6.38-rc1. Some pud-level code has been added there in
>   linux-next.

Thanks.  As this been tested with ARMv4/v5 CPUs as well?

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Nicolas Pitre]

On Wed, 2 Feb 2011, Russell King - ARM Linux wrote:

> On Wed, Feb 02, 2011 at 03:43:29PM +0100, Per Fransson wrote:
> > When restarting using the kernel kexec functionality the MMU
> > needs to be turned off. Any code which does this needs to use
> > identity mapped addresses to get reliable results. In the ARM
> > kexec case this identity mapping is done:
> > 
> > - using the page table of the current task
> > 
> > - for all addresses normally used by user space,
> >   i.e. 0x00000000-PAGE_OFFSET
> > 
> > If kexec is used at a kernel crash to collect a core dump this
> > means that we lose important information.
> > 
> > This is what this patches does:
> > 
> > * Actually turns off the MMU, which has been omitted by mistake
> > 
> > * Sets up a more selective identity mapping
> > 
> > * Restores the old mapping once the MMU is off
> > 
> > Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
> > ---
> > v2 changes:
> > 
> > * now uses (modified versions of) the identity mapping functions in idmap.c
> >   as they look in 2.6.38-rc1. Some pud-level code has been added there in
> >   linux-next.
> 
> Thanks.  As this been tested with ARMv4/v5 CPUs as well?

I have doubts about the effectiveness of setup_mm_for_reboot() on any 
machine where physical RAM starts at 0xc0000000 or above, such as on 
SA1100.


Nicolas

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Russell King - ARM Linux]

On Wed, Feb 02, 2011 at 05:10:37PM -0500, Nicolas Pitre wrote:
> On Wed, 2 Feb 2011, Russell King - ARM Linux wrote:
> 
> > On Wed, Feb 02, 2011 at 03:43:29PM +0100, Per Fransson wrote:
> > > When restarting using the kernel kexec functionality the MMU
> > > needs to be turned off. Any code which does this needs to use
> > > identity mapped addresses to get reliable results. In the ARM
> > > kexec case this identity mapping is done:
> > > 
> > > - using the page table of the current task
> > > 
> > > - for all addresses normally used by user space,
> > >   i.e. 0x00000000-PAGE_OFFSET
> > > 
> > > If kexec is used at a kernel crash to collect a core dump this
> > > means that we lose important information.
> > > 
> > > This is what this patches does:
> > > 
> > > * Actually turns off the MMU, which has been omitted by mistake
> > > 
> > > * Sets up a more selective identity mapping
> > > 
> > > * Restores the old mapping once the MMU is off
> > > 
> > > Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
> > > ---
> > > v2 changes:
> > > 
> > > * now uses (modified versions of) the identity mapping functions in idmap.c
> > >   as they look in 2.6.38-rc1. Some pud-level code has been added there in
> > >   linux-next.
> > 
> > Thanks.  As this been tested with ARMv4/v5 CPUs as well?
> 
> I have doubts about the effectiveness of setup_mm_for_reboot() on any 
> machine where physical RAM starts at 0xc0000000 or above, such as on 
> SA1100.

It's known to work on Assabet.  It works on SA1100 because the kernel
mapping is already a 1:1 mapping.

What setup_mm_for_reboot() is doing on Assabet though is making the
flash available for cpu_reset(0) to be able to call, not making the
kernel code for cpu_reset() available for calling.

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Nicolas Pitre]

On Wed, 2 Feb 2011, Russell King - ARM Linux wrote:

> On Wed, Feb 02, 2011 at 05:10:37PM -0500, Nicolas Pitre wrote:
> > On Wed, 2 Feb 2011, Russell King - ARM Linux wrote:
> > 
> > > On Wed, Feb 02, 2011 at 03:43:29PM +0100, Per Fransson wrote:
> > > > When restarting using the kernel kexec functionality the MMU
> > > > needs to be turned off. Any code which does this needs to use
> > > > identity mapped addresses to get reliable results. In the ARM
> > > > kexec case this identity mapping is done:
> > > > 
> > > > - using the page table of the current task
> > > > 
> > > > - for all addresses normally used by user space,
> > > >   i.e. 0x00000000-PAGE_OFFSET
> > > > 
> > > > If kexec is used at a kernel crash to collect a core dump this
> > > > means that we lose important information.
> > > > 
> > > > This is what this patches does:
> > > > 
> > > > * Actually turns off the MMU, which has been omitted by mistake
> > > > 
> > > > * Sets up a more selective identity mapping
> > > > 
> > > > * Restores the old mapping once the MMU is off
> > > > 
> > > > Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
> > > > ---
> > > > v2 changes:
> > > > 
> > > > * now uses (modified versions of) the identity mapping functions in idmap.c
> > > >   as they look in 2.6.38-rc1. Some pud-level code has been added there in
> > > >   linux-next.
> > > 
> > > Thanks.  As this been tested with ARMv4/v5 CPUs as well?
> > 
> > I have doubts about the effectiveness of setup_mm_for_reboot() on any 
> > machine where physical RAM starts at 0xc0000000 or above, such as on 
> > SA1100.
> 
> It's known to work on Assabet.  It works on SA1100 because the kernel
> mapping is already a 1:1 mapping.
D'oh.

> What setup_mm_for_reboot() is doing on Assabet though is making the
> flash available for cpu_reset(0) to be able to call, not making the
> kernel code for cpu_reset() available for calling.

Right.  So if RAM is located at 0xd0000000 instead then this won't work 
as intended.  And overwriting the entire user space is not the best 
thing to do for kexec anyway.


Nicolas

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Russell King - ARM Linux]

On Wed, Feb 02, 2011 at 05:27:40PM -0500, Nicolas Pitre wrote:
> On Wed, 2 Feb 2011, Russell King - ARM Linux wrote:
> 
> > It's known to work on Assabet.  It works on SA1100 because the kernel
> > mapping is already a 1:1 mapping.
> D'oh.
> 
> > What setup_mm_for_reboot() is doing on Assabet though is making the
> > flash available for cpu_reset(0) to be able to call, not making the
> > kernel code for cpu_reset() available for calling.
> 
> Right.  So if RAM is located at 0xd0000000 instead then this won't work 
> as intended.

It'll work as intended for cpu resetting, except for v6 and v7 as they
don't have the necessary code in their cpu_reset() function to work,
and secondly perversely, v6 and v7 CPUs stall the pipeline when turning
the MMU off, while previous CPUs didn't.  Meanwhile, many other
instructions which did stall the pipeline no longer do.

> And overwriting the entire user space is not the best thing to do for
> kexec anyway.

Probably not.  As I see it, having RAM located above PAGE_OFFSET in phys
address space, but not being a 1:1 mapping, will always be a problem.
What if it ends up overwriting the L2 cache controller mapping?

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Mika Westerberg]

Hi,

Few questions, see below.

On Wed, Feb 02, 2011 at 03:43:29PM +0100, Per Fransson wrote:
> When restarting using the kernel kexec functionality the MMU
> needs to be turned off. Any code which does this needs to use
> identity mapped addresses to get reliable results. In the ARM
> kexec case this identity mapping is done:

[...]

> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index 0c1172b..762bb43 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -61,6 +61,10 @@ ENDPROC(cpu_v7_proc_fin)
>   */
>  	.align	5
>  ENTRY(cpu_v7_reset)
> +	sub	pc, pc, #PAGE_OFFSET+4		@ go to physical addresses

How the above is going to work if PHYS_OFFSET is something else than zero? Also
does this work with Thumb-2 kernels?

> +	mrc	p15, 0, ip, c1, c0, 0		@ ctrl register
> +	bic	ip, ip, #0x0001			@ ...............m
> +	mcr	p15, 0, ip, c1, c0, 0		@ ctrl register
>  	mov	pc, r0
>  ENDPROC(cpu_v7_reset)
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 03e8e8d..6a1345b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -106,7 +106,11 @@ struct kimage {
>  #endif
>  };
>  
> -
> +struct kexec_mmu_ent {
> +	pgd_t *ptr;
> +	pgd_t store;
> +};
> +extern struct kexec_mmu_ent kexec_mmu_ents[4];
>  
>  /* kexec interface functions */
>  extern void machine_kexec(struct kimage *image);

Is it good idea to place these in a generic header as they are only used in ARM?

Thanks,
MW

[PATCH v2] ARM: kexec: selective MMU identity mapping

[Per Fransson]

Hi,

Ok, I've tried to summarize your comments:

* Break out turning off the MMU.

  - Not sure about this one, the changes rely on each other anyway.
But if you insist...

* Don't add a kexec specific structure as argument to identity_mapping_add().

  - How about a separate 'store_mapping_for(<address to store for>,
<where to store>)' function?
    Could it be placed idmap.c as well?

* Always use processor.reset, regardless of #ifdef MULTI_CPU

  - Yes

* beq -> bneq in relocate_new_kernel()

  - errare humanum est =o)

* better way to check for the MMU actually being off?

  - Will reading the M bit of the System Control Register do?

* Prettier, more general copy lists for restoring the page table entries.

  - Are there any such pre-existing types, e.g.

        struct {
            size_t howmuch;
            void * towhere;
            char what[0];
        }

    or something similar?

* The old 0x0-PAGE_OFFSET 1-1 mapping doesn't cover the physical
addresses needed if these are
  higher, e.g. 0xd0000000--. Thus it will never work.

  - My patch will identity map the needed addresses regardless of
whether they are located in the
    user or kernel space ranges. This at least *could* work, if we are
lucky and it doesn't coincide
    with the mappings for the code we still need to run before turning
off the MMU.

* The jump to physical addresses in cpu_reset() needs to account for
PHYS_OFFSET!=0

  - Yes

* Move ARM specific struct definitions to ARM specific header

  - Yes

* Tested on ARMv4/v5 CPUs?

  - No, sorry, there are none available to me. They should work like
they used to if they don't use
    any other code than the cpu_reset() and relocate_new_kernel()
functions once the MMU has been
    turned off.

Regards,
Per