[PATCH v2 0/8] Initial implementation of kdump for ARM

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

Hello,

This is re-send of the series. I didn't receive any comments on this but we
found few minor things during in-house review which are incorporated.

Changes to v1:
	- corrected kernel-doc for crash_setup_regs()
	- check return value of ioremap() in copy_oldmem_page()

Changes to RFC version:
	- crash_setup_regs() uses Russell's suggestion for saving registers
	- copy_oldmem_page() code is changed to use ioremap()
	- there is no need for special 'mem=' parameters

I've tested this on N900 and beagleboard.

Thanks,
Mw

Mika Westerberg (8):
  arm: kdump: reserve memory for crashkernel
  arm: kdump: implement crash_setup_regs()
  arm: kdump: implement machine_crash_shutdown()
  arm: kdump: skip indirection page when crashing
  arm: kdump: implement copy_oldmem_page()
  arm: allow passing an ELF64 header to elf_check_arch()
  arm: kdump: add support for elfcorehdr= parameter
  arm: kdump: add CONFIG_CRASH_DUMP Kconfig option

 arch/arm/Kconfig                  |   12 ++++++
 arch/arm/include/asm/elf.h        |    4 +-
 arch/arm/include/asm/kexec.h      |   22 +++++++++-
 arch/arm/kernel/Makefile          |    1 +
 arch/arm/kernel/crash_dump.c      |   60 +++++++++++++++++++++++++++++
 arch/arm/kernel/elf.c             |    6 ++-
 arch/arm/kernel/machine_kexec.c   |    4 ++
 arch/arm/kernel/relocate_kernel.S |    6 +++
 arch/arm/kernel/setup.c           |   76 +++++++++++++++++++++++++++++++++++++
 9 files changed, 184 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm/kernel/crash_dump.c

[PATCH v2 1/8] arm: kdump: reserve memory for crashkernel

[Mika Westerberg]

Implemented ARM support for command line option "crashkernel=size at start" which
allows user to reserve some memory for a dump capture kernel.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/setup.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index c91c77b..076454f 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -19,6 +19,7 @@
 #include <linux/seq_file.h>
 #include <linux/screen_info.h>
 #include <linux/init.h>
+#include <linux/kexec.h>
 #include <linux/root_dev.h>
 #include <linux/cpu.h>
 #include <linux/interrupt.h>
@@ -661,6 +662,55 @@ static int __init customize_machine(void)
 }
 arch_initcall(customize_machine);
 
+#ifdef CONFIG_KEXEC
+static inline unsigned long long get_total_mem(void)
+{
+	unsigned long total;
+
+	total = max_low_pfn - min_low_pfn;
+	return total << PAGE_SHIFT;
+}
+
+/**
+ * reserve_crashkernel() - reserves memory are for crash kernel
+ *
+ * This function reserves memory area given in "crashkernel=" kernel command
+ * line parameter. The memory reserved is used by a dump capture kernel when
+ * primary kernel is crashing.
+ */
+static void __init reserve_crashkernel(void)
+{
+	unsigned long long crash_size, crash_base;
+	unsigned long long total_mem;
+	int ret;
+
+	total_mem = get_total_mem();
+	ret = parse_crashkernel(boot_command_line, total_mem,
+				&crash_size, &crash_base);
+	if (ret)
+		return;
+
+	ret = reserve_bootmem(crash_base, crash_size, BOOTMEM_EXCLUSIVE);
+	if (ret < 0) {
+		printk(KERN_WARNING "crashkernel reservation failed - "
+		       "memory is in use (0x%lx)\n", (unsigned long)crash_base);
+		return;
+	}
+
+	printk(KERN_INFO "Reserving %ldMB of memory@%ldMB "
+	       "for crashkernel (System RAM: %ldMB)\n",
+	       (unsigned long)(crash_size >> 20),
+	       (unsigned long)(crash_base >> 20),
+	       (unsigned long)(total_mem >> 20));
+
+	crashk_res.start = crash_base;
+	crashk_res.end = crash_base + crash_size - 1;
+	insert_resource(&iomem_resource, &crashk_res);
+}
+#else
+static inline void reserve_crashkernel(void) {}
+#endif /* CONFIG_KEXEC */
+
 void __init setup_arch(char **cmdline_p)
 {
 	struct tag *tags = (struct tag *)&init_tags;
@@ -720,6 +770,7 @@ void __init setup_arch(char **cmdline_p)
 #ifdef CONFIG_SMP
 	smp_init_cpus();
 #endif
+	reserve_crashkernel();
 
 	cpu_init();
 	tcm_init();
-- 
1.5.6.5

[PATCH v2 2/8] arm: kdump: implement crash_setup_regs()

[Mika Westerberg]

Implement machine specific function crash_setup_regs() which is responsible for
storing machine state when crash occured.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/include/asm/kexec.h |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kexec.h b/arch/arm/include/asm/kexec.h
index df15a0d..8ec9ef5 100644
--- a/arch/arm/include/asm/kexec.h
+++ b/arch/arm/include/asm/kexec.h
@@ -19,10 +19,26 @@
 
 #ifndef __ASSEMBLY__
 
-struct kimage;
-/* Provide a dummy definition to avoid build failures. */
+/**
+ * crash_setup_regs() - save registers for the panic kernel
+ * @newregs: registers are saved here
+ * @oldregs: registers to be saved (may be %NULL)
+ *
+ * Function copies machine registers from @oldregs to @newregs. If @oldregs is
+ * %NULL then current registers are stored there.
+ */
 static inline void crash_setup_regs(struct pt_regs *newregs,
-                                        struct pt_regs *oldregs) { }
+				    struct pt_regs *oldregs)
+{
+	if (oldregs) {
+		memcpy(newregs, oldregs, sizeof(*newregs));
+	} else {
+		__asm__ __volatile__ ("stmia %0, {r0 - r15}"
+				      : : "r" (&newregs->ARM_r0));
+		__asm__ __volatile__ ("mrs %0, cpsr"
+				      : "=r" (newregs->ARM_cpsr));
+	}
+}
 
 #endif /* __ASSEMBLY__ */
 
-- 
1.5.6.5

[PATCH v2 3/8] arm: kdump: implement machine_crash_shutdown()

[Mika Westerberg]

Implement function machine_crash_shutdown() which disables IRQs and saves
machine state to ELF notes structure.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/machine_kexec.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 598ca61..81e9898 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -43,6 +43,10 @@ void machine_shutdown(void)
 
 void machine_crash_shutdown(struct pt_regs *regs)
 {
+	local_irq_disable();
+	crash_save_cpu(regs, smp_processor_id());
+
+	printk(KERN_INFO "Loading crashdump kernel...\n");
 }
 
 void machine_kexec(struct kimage *image)
-- 
1.5.6.5

[PATCH v2 4/8] arm: kdump: skip indirection page when crashing

[Mika Westerberg]

When we are crashing there is no indirection page in place. Only control page is
present.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/relocate_kernel.S |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
index 61930eb..fd26f8d 100644
--- a/arch/arm/kernel/relocate_kernel.S
+++ b/arch/arm/kernel/relocate_kernel.S
@@ -10,6 +10,12 @@ relocate_new_kernel:
 	ldr	r0,kexec_indirection_page
 	ldr	r1,kexec_start_address
 
+	/*
+	 * If there is no indirection page (we are doing crashdumps)
+	 * skip any relocation.
+	 */
+	cmp	r0, #0
+	beq	2f
 
 0:	/* top, read another word for the indirection page */
 	ldr	r3, [r0],#4
-- 
1.5.6.5

[PATCH v2 5/8] arm: kdump: implement copy_oldmem_page()

[Mika Westerberg]

This function is used by vmcore code to read a page from the old kernel memory.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/Makefile     |    1 +
 arch/arm/kernel/crash_dump.c |   60 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/kernel/crash_dump.c

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 26d302c..ea023c6 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_ARM_UNWIND)	+= unwind.o
 obj-$(CONFIG_HAVE_TCM)		+= tcm.o
+obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 
 obj-$(CONFIG_CRUNCH)		+= crunch.o crunch-bits.o
 AFLAGS_crunch-bits.o		:= -Wa,-mcpu=ep9312
diff --git a/arch/arm/kernel/crash_dump.c b/arch/arm/kernel/crash_dump.c
new file mode 100644
index 0000000..cd3b853
--- /dev/null
+++ b/arch/arm/kernel/crash_dump.c
@@ -0,0 +1,60 @@
+/*
+ * arch/arm/kernel/crash_dump.c
+ *
+ * Copyright (C) 2010 Nokia Corporation.
+ * Author: Mika Westerberg
+ *
+ * This code is taken from arch/x86/kernel/crash_dump_64.c
+ *   Created by: Hariprasad Nellitheertha (hari at in.ibm.com)
+ *   Copyright (C) IBM Corporation, 2004. All rights reserved
+ *
+ * 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.
+ */
+
+#include <linux/errno.h>
+#include <linux/crash_dump.h>
+#include <linux/uaccess.h>
+#include <linux/io.h>
+
+/* stores the physical address of elf header of crash image */
+unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX;
+
+/**
+ * copy_oldmem_page() - copy one page from old kernel memory
+ * @pfn: page frame number to be copied
+ * @buf: buffer where the copied page is placed
+ * @csize: number of bytes to copy
+ * @offset: offset in bytes into the page
+ * @userbuf: if set, @buf is int he user address space
+ *
+ * This function copies one page from old kernel memory into buffer pointed by
+ * @buf. If @buf is in userspace, set @userbuf to %1. Returns number of bytes
+ * copied or negative error in case of failure.
+ */
+ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
+			 size_t csize, unsigned long offset,
+			 int userbuf)
+{
+	void *vaddr;
+
+	if (!csize)
+		return 0;
+
+	vaddr = ioremap(pfn << PAGE_SHIFT, PAGE_SIZE);
+	if (!vaddr)
+		return -ENOMEM;
+
+	if (userbuf) {
+		if (copy_to_user(buf, vaddr + offset, csize)) {
+			iounmap(vaddr);
+			return -EFAULT;
+		}
+	} else {
+		memcpy(buf, vaddr + offset, csize);
+	}
+
+	iounmap(vaddr);
+	return csize;
+}
-- 
1.5.6.5

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Mika Westerberg]

This is needed to shut following compiler warning when CONFIG_PROC_VMCORE is
enabled:

fs/proc/vmcore.c: In function 'parse_crash_elf64_headers':
fs/proc/vmcore.c:500: warning: passing argument 1 of 'elf_check_arch' from
incompatible pointer type

ELF32 and ELF64 headers have common fields of same size (namely e_ident and
e_machine) which are checked in arm_elf_check_arch().

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/include/asm/elf.h |    4 ++--
 arch/arm/kernel/elf.c      |    6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/elf.h b/arch/arm/include/asm/elf.h
index bff0564..aa71815 100644
--- a/arch/arm/include/asm/elf.h
+++ b/arch/arm/include/asm/elf.h
@@ -92,8 +92,8 @@ struct elf32_hdr;
 /*
  * This is used to ensure we don't load something for the wrong architecture.
  */
-extern int elf_check_arch(const struct elf32_hdr *);
-#define elf_check_arch elf_check_arch
+extern int arm_elf_check_arch(const struct elf32_hdr *);
+#define elf_check_arch(x) arm_elf_check_arch((const struct elf32_hdr *)(x))
 
 extern int arm_elf_read_implies_exec(const struct elf32_hdr *, int);
 #define elf_read_implies_exec(ex,stk) arm_elf_read_implies_exec(&(ex), stk)
diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index d4a0da1..14e501b 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -4,11 +4,13 @@
 #include <linux/binfmts.h>
 #include <linux/elf.h>
 
-int elf_check_arch(const struct elf32_hdr *x)
+int arm_elf_check_arch(const struct elf32_hdr *x)
 {
 	unsigned int eflags;
 
 	/* Make sure it's an ARM executable */
+	if (x->e_ident[EI_CLASS] != ELF_CLASS)
+		return 0;
 	if (x->e_machine != EM_ARM)
 		return 0;
 
@@ -35,7 +37,7 @@ int elf_check_arch(const struct elf32_hdr *x)
 	}
 	return 1;
 }
-EXPORT_SYMBOL(elf_check_arch);
+EXPORT_SYMBOL(arm_elf_check_arch);
 
 void elf_set_personality(const struct elf32_hdr *x)
 {
-- 
1.5.6.5

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Russell King - ARM Linux]

On Wed, May 05, 2010 at 09:54:18AM +0300, Mika Westerberg wrote:
> This is needed to shut following compiler warning when CONFIG_PROC_VMCORE is
> enabled:
> 
> fs/proc/vmcore.c: In function 'parse_crash_elf64_headers':
> fs/proc/vmcore.c:500: warning: passing argument 1 of 'elf_check_arch' from
> incompatible pointer type
> 
> ELF32 and ELF64 headers have common fields of same size (namely e_ident and
> e_machine) which are checked in arm_elf_check_arch().

This patch is bogus - and shows the dangers of throwing casts into C code
to shut up warnings without first analysing the code.

Our elf_check_arch() uses:
	e_machine
	e_entry
	e_flags
thusly:
        if (x->e_machine != EM_ARM)
        if (x->e_entry & 1) {
        } else if (x->e_entry & 3)
        eflags = x->e_flags;

Now, the Elf32 header looks like this:

typedef struct elf32_hdr{
  unsigned char e_ident[EI_NIDENT];	/* 0x00 - 0x0F */
  Elf32_Half    e_type;			/* 0x10 - 0x11 */
  Elf32_Half    e_machine;		/* 0x12 - 0x13 */
  Elf32_Word    e_version;		/* 0x14 - 0x17 */
  Elf32_Addr    e_entry;		/* 0x18 - 0x1b */
  Elf32_Off     e_phoff;		/* 0x1c - 0x1f */
  Elf32_Off     e_shoff;		/* 0x20 - 0x23 */
  Elf32_Word    e_flags;		/* 0x24 - 0x27 */

and Elf64 header:

typedef struct elf64_hdr {
  unsigned char e_ident[EI_NIDENT];	/* 0x00 - 0x0F */
  Elf64_Half e_type;			/* 0x10 - 0x11 */
  Elf64_Half e_machine;			/* 0x12 - 0x13 */
  Elf64_Word e_version;			/* 0x14 - 0x17 */
  Elf64_Addr e_entry;			/* 0x18 - 0x1f */
  Elf64_Off e_phoff;			/* 0x20 - 0x27 */
  Elf64_Off e_shoff;			/* 0x28 - 0x2f */
  Elf64_Word e_flags;			/* 0x30 - 0x33 */

Notice that e_entry and e_flags are different sizes and/or different
offsets, so ARMs elf_check_arch can not work with elf64 headers.  So
with an ELF64 header, accessing e_flags will result in actually
accessing the top half of the 64-bit e_phoff, and accessing 32-bit
e_entry will get us the lower half of the 64-bit e_entry.

Now, here's the question: why does this crashkernel stuff want to
parse a 64-bit ELF header on a 32-bit only platform where the crashing
kernel will never generate a 64-bit ELF core file?

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Mika Westerberg]

On Mon, May 10, 2010 at 01:20:36PM +0200, ext Russell King - ARM Linux wrote:
> On Wed, May 05, 2010 at 09:54:18AM +0300, Mika Westerberg wrote:
> > This is needed to shut following compiler warning when CONFIG_PROC_VMCORE is
> > enabled:
> > 
> > fs/proc/vmcore.c: In function 'parse_crash_elf64_headers':
> > fs/proc/vmcore.c:500: warning: passing argument 1 of 'elf_check_arch' from
> > incompatible pointer type
> > 
> > ELF32 and ELF64 headers have common fields of same size (namely e_ident and
> > e_machine) which are checked in arm_elf_check_arch().
> 
> This patch is bogus - and shows the dangers of throwing casts into C code
> to shut up warnings without first analysing the code.
> 
> Our elf_check_arch() uses:
> 	e_machine
> 	e_entry
> 	e_flags
> thusly:
>         if (x->e_machine != EM_ARM)
>         if (x->e_entry & 1) {
>         } else if (x->e_entry & 3)
>         eflags = x->e_flags;
> 
> Now, the Elf32 header looks like this:
> 
> typedef struct elf32_hdr{
>   unsigned char e_ident[EI_NIDENT];	/* 0x00 - 0x0F */
>   Elf32_Half    e_type;			/* 0x10 - 0x11 */
>   Elf32_Half    e_machine;		/* 0x12 - 0x13 */
>   Elf32_Word    e_version;		/* 0x14 - 0x17 */
>   Elf32_Addr    e_entry;		/* 0x18 - 0x1b */
>   Elf32_Off     e_phoff;		/* 0x1c - 0x1f */
>   Elf32_Off     e_shoff;		/* 0x20 - 0x23 */
>   Elf32_Word    e_flags;		/* 0x24 - 0x27 */
> 
> and Elf64 header:
> 
> typedef struct elf64_hdr {
>   unsigned char e_ident[EI_NIDENT];	/* 0x00 - 0x0F */
>   Elf64_Half e_type;			/* 0x10 - 0x11 */
>   Elf64_Half e_machine;			/* 0x12 - 0x13 */
>   Elf64_Word e_version;			/* 0x14 - 0x17 */
>   Elf64_Addr e_entry;			/* 0x18 - 0x1f */
>   Elf64_Off e_phoff;			/* 0x20 - 0x27 */
>   Elf64_Off e_shoff;			/* 0x28 - 0x2f */
>   Elf64_Word e_flags;			/* 0x30 - 0x33 */
> 
> Notice that e_entry and e_flags are different sizes and/or different
> offsets, so ARMs elf_check_arch can not work with elf64 headers.  So
> with an ELF64 header, accessing e_flags will result in actually
> accessing the top half of the 64-bit e_phoff, and accessing 32-bit
> e_entry will get us the lower half of the 64-bit e_entry.

Thanks for comments.

I believe that when passing ELF64 header, it fails in following checks:

	/* Make sure it's an ARM executable */
	if (x->e_ident[EI_CLASS] != ELF_CLASS)
		return 0;
	if (x->e_machine != EM_ARM)
		return 0;

ELF_CLASS is defined in arch/arm/include/asm/elf.h:

#define ELF_CLASS       ELFCLASS32

So if class is different than ELFCLASS32 it returns 0 and never even try to
access other fields, right?

> Now, here's the question: why does this crashkernel stuff want to
> parse a 64-bit ELF header on a 32-bit only platform where the crashing
> kernel will never generate a 64-bit ELF core file?

I really don't know but fs/proc/vmcore.c is coded in such way that it supports
both types of ELF headers. It however, passes the header to elf_check_arch()
which in our case should fail if it is something else than ELF32 header.

Thanks,
MW

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Russell King - ARM Linux]

On Mon, May 10, 2010 at 03:09:20PM +0300, Mika Westerberg wrote:
> I believe that when passing ELF64 header, it fails in following checks:
> 
> 	/* Make sure it's an ARM executable */
> 	if (x->e_ident[EI_CLASS] != ELF_CLASS)
> 		return 0;
> 	if (x->e_machine != EM_ARM)
> 		return 0;
> 
> ELF_CLASS is defined in arch/arm/include/asm/elf.h:
> 
> #define ELF_CLASS       ELFCLASS32
> 
> So if class is different than ELFCLASS32 it returns 0 and never even try to
> access other fields, right?

Yes.

> > Now, here's the question: why does this crashkernel stuff want to
> > parse a 64-bit ELF header on a 32-bit only platform where the crashing
> > kernel will never generate a 64-bit ELF core file?
> 
> I really don't know but fs/proc/vmcore.c is coded in such way that it supports
> both types of ELF headers. It however, passes the header to elf_check_arch()
> which in our case should fail if it is something else than ELF32 header.

There's other arches which want elf_check_arch to be a function call, so
I think my question needs to be looked at more closely - and possibly
the code changed such that we don't end up with this situation.

Maybe a cleaner solution would be for vmcore.c to split its calls to
elf_check_arch() - to be elf32_check_arch() and elf64_check_arch() ?
Platforms where it's just a macro can define both to be elf_check_arch()
but those where only one flavour is supported should define the unsupported
flavour to zero - which incidentally would allow the compiler to optimize
away the unnecessary parts of parse_crash_elf*_headers().

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Mika Westerberg]

On Mon, May 10, 2010 at 02:21:56PM +0200, ext Russell King - ARM Linux wrote:
> 
> > > Now, here's the question: why does this crashkernel stuff want to
> > > parse a 64-bit ELF header on a 32-bit only platform where the crashing
> > > kernel will never generate a 64-bit ELF core file?
> > 
> > I really don't know but fs/proc/vmcore.c is coded in such way that it supports
> > both types of ELF headers. It however, passes the header to elf_check_arch()
> > which in our case should fail if it is something else than ELF32 header.
> 
> There's other arches which want elf_check_arch to be a function call, so
> I think my question needs to be looked at more closely - and possibly
> the code changed such that we don't end up with this situation.

I quickly checked and it seems that only one arch in addition to ARM wants this
to be a function:

arch/frv/include/asm/elf.h:
...
extern int elf_check_arch(const struct elf32_hdr *hdr);

and they don't (yet) support kdump.

> Maybe a cleaner solution would be for vmcore.c to split its calls to
> elf_check_arch() - to be elf32_check_arch() and elf64_check_arch() ?

True. However, there already is another macro in vmcore.c:
vmcore_elf_check_arch() which is used by 64-bit code. So adding 32- and 64-bit
versions of elf_check_arch() might be overkill.

Also elf_check_arch() is used outside vmcore.c as well.

> Platforms where it's just a macro can define both to be elf_check_arch()
> but those where only one flavour is supported should define the unsupported
> flavour to zero - which incidentally would allow the compiler to optimize
> away the unnecessary parts of parse_crash_elf*_headers().

I agree but changing these needs to be performed on every arch and it might
cause regressions (at least because it is hard to test on archs that I don't
have). So I'm not sure if it is worth a risk.

It's up to you, I can implement it this way also if you don't accept the current
version of the patch.

Thanks again,
MW

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Mika Westerberg]

On Mon, May 10, 2010 at 02:21:56PM +0200, ext Russell King - ARM Linux wrote:
> On Mon, May 10, 2010 at 03:09:20PM +0300, Mika Westerberg wrote:
[...]
> > I really don't know but fs/proc/vmcore.c is coded in such way that it supports
> > both types of ELF headers. It however, passes the header to elf_check_arch()
> > which in our case should fail if it is something else than ELF32 header.
> 
> There's other arches which want elf_check_arch to be a function call, so
> I think my question needs to be looked at more closely - and possibly
> the code changed such that we don't end up with this situation.
> 
> Maybe a cleaner solution would be for vmcore.c to split its calls to
> elf_check_arch() - to be elf32_check_arch() and elf64_check_arch() ?
> Platforms where it's just a macro can define both to be elf_check_arch()
> but those where only one flavour is supported should define the unsupported
> flavour to zero - which incidentally would allow the compiler to optimize
> away the unnecessary parts of parse_crash_elf*_headers().

Russell,

I noticed that you applied all the kdump patches except this and the
CONFIG_CRASH_DUMP patch. Thanks.

Should I update this patch as you describe above? So that we don't need to
perform any casting but just have elf_check_arch() separated into 32- and 64-bit
versions. Or is there something else preventing these 2 patches to be merged?

Thanks,
MW

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Lei Wen]

Hi Mika,

I have tried your patch, and it works perfect.
But there is something  inconvenient  that I have to modify the
PHYS_OFFSET and mach/Makefile.boot
manually if I want to get a kdump capable kernel.

How about add additional configuration like CONFIG_RELOCATABLE
CONFIG_PHYSICAL_STARTas x86?
This would do great help for formal usage.

Thanks,
Lei


On Fri, Jul 16, 2010 at 4:14 PM, Mika Westerberg
<ext-mika.1.westerberg@nokia.com> wrote:
> On Mon, May 10, 2010 at 02:21:56PM +0200, ext Russell King - ARM Linux wrote:
>> On Mon, May 10, 2010 at 03:09:20PM +0300, Mika Westerberg wrote:
> [...]
>> > I really don't know but fs/proc/vmcore.c is coded in such way that it supports
>> > both types of ELF headers. It however, passes the header to elf_check_arch()
>> > which in our case should fail if it is something else than ELF32 header.
>>
>> There's other arches which want elf_check_arch to be a function call, so
>> I think my question needs to be looked at more closely - and possibly
>> the code changed such that we don't end up with this situation.
>>
>> Maybe a cleaner solution would be for vmcore.c to split its calls to
>> elf_check_arch() - to be elf32_check_arch() and elf64_check_arch() ?
>> Platforms where it's just a macro can define both to be elf_check_arch()
>> but those where only one flavour is supported should define the unsupported
>> flavour to zero - which incidentally would allow the compiler to optimize
>> away the unnecessary parts of parse_crash_elf*_headers().
>
> Russell,
>
> I noticed that you applied all the kdump patches except this and the
> CONFIG_CRASH_DUMP patch. Thanks.
>
> Should I update this patch as you describe above? So that we don't need to
> perform any casting but just have elf_check_arch() separated into 32- and 64-bit
> versions. Or is there something else preventing these 2 patches to be merged?
>
> Thanks,
> MW
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH v2 6/8] arm: allow passing an ELF64 header to elf_check_arch()

[Mika Westerberg]

Hi,

On Wed, Aug 25, 2010 at 04:40:45AM +0200, ext Lei Wen wrote:
> 
> I have tried your patch, and it works perfect.
> But there is something  inconvenient  that I have to modify the
> PHYS_OFFSET and mach/Makefile.boot
> manually if I want to get a kdump capable kernel.

Yeah, it's a bit annoying. I remember that I had some plans to make it
configurable but never implemented it.

> How about add additional configuration like CONFIG_RELOCATABLE
> CONFIG_PHYSICAL_STARTas x86?
> This would do great help for formal usage.

You are right, something like that is eventually needed. I'll try to find some
time to look into that and cook up some solution.

Thanks,
MW

[PATCH v2 7/8] arm: kdump: add support for elfcorehdr= parameter

[Mika Westerberg]

This parameter is used by primary kernel to pass address of vmcore header to the
dump capture kernel.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/setup.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 076454f..25a1664 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -20,6 +20,7 @@
 #include <linux/screen_info.h>
 #include <linux/init.h>
 #include <linux/kexec.h>
+#include <linux/crash_dump.h>
 #include <linux/root_dev.h>
 #include <linux/cpu.h>
 #include <linux/interrupt.h>
@@ -711,6 +712,30 @@ static void __init reserve_crashkernel(void)
 static inline void reserve_crashkernel(void) {}
 #endif /* CONFIG_KEXEC */
 
+/*
+ * Note: elfcorehdr_addr is not just limited to vmcore. It is also used by
+ * is_kdump_kernel() to determine if we are booting after a panic. Hence
+ * ifdef it under CONFIG_CRASH_DUMP and not CONFIG_PROC_VMCORE.
+ */
+
+#ifdef CONFIG_CRASH_DUMP
+/*
+ * elfcorehdr= specifies the location of elf core header stored by the crashed
+ * kernel. This option will be passed by kexec loader to the capture kernel.
+ */
+static int __init setup_elfcorehdr(char *arg)
+{
+	char *end;
+
+	if (!arg)
+		return -EINVAL;
+
+	elfcorehdr_addr = memparse(arg, &end);
+	return end > arg ? 0 : -EINVAL;
+}
+early_param("elfcorehdr", setup_elfcorehdr);
+#endif /* CONFIG_CRASH_DUMP */
+
 void __init setup_arch(char **cmdline_p)
 {
 	struct tag *tags = (struct tag *)&init_tags;
-- 
1.5.6.5

[PATCH v2 8/8] arm: kdump: add CONFIG_CRASH_DUMP Kconfig option

[Mika Westerberg]

Add CONFIG_CRASH_DUMP configuration option which is used by dump capture
kernels. Dump capture kernel must be loaded into different physical address
than the primary kernel. This means that PHYS_OFFSET must be different and
it also should match the 'crashkernel=size at start' value passed to primary
kernel command line.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/Kconfig |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 92622eb..bfc7128 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1467,6 +1467,18 @@ config ATAGS_PROC
 	  Should the atags used to boot the kernel be exported in an "atags"
 	  file in procfs. Useful with kexec.
 
+config CRASH_DUMP
+	bool "Build kdump crash kernel (EXPERIMENTAL)"
+	depends on EXPERIMENTAL
+	help
+	  Build a kernel suitable for use as kdump capture kernel. This should
+	  be set only on dump capture kernels. Note that dump capture kernel
+	  must be loaded into different physical address than the primary kernel
+	  (e.g set PHYS_OFFSET and related mach/Makefile.boot parameters
+	  to match value given in 'crashkernel=size at start').
+
+	  For more details see Documentation/kdump/kdump.txt
+
 endmenu
 
 menu "CPU Power Management"
-- 
1.5.6.5

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

On Wed, May 05, 2010 at 08:54:12AM +0200, Westerberg Mika.1 (EXT-Nixu/Helsinki) wrote:
> 
> This is re-send of the series. I didn't receive any comments on this but we
> found few minor things during in-house review which are incorporated.
> 
> Changes to v1:
> 	- corrected kernel-doc for crash_setup_regs()
> 	- check return value of ioremap() in copy_oldmem_page()
> 
> Changes to RFC version:
> 	- crash_setup_regs() uses Russell's suggestion for saving registers
> 	- copy_oldmem_page() code is changed to use ioremap()
> 	- there is no need for special 'mem=' parameters
> 
> I've tested this on N900 and beagleboard.

Hi Russell,

Sorry to bother you with this but I wasn't sure what is going to happen with
these patches. Are you planning to take them or do you want me to change
something?

I already submitted these to your patch system as I didn't receive much
comments (not sure was this the right thing to do, however).

In addition, I tested these on ep93xx based Sim.One board and they seem to work
there as well.

Thanks,
MW

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

On Tue, May 25, 2010 at 11:19:37AM +0300, Mika Westerberg wrote:
> On Wed, May 05, 2010 at 08:54:12AM +0200, Westerberg Mika.1 (EXT-Nixu/Helsinki) wrote:
> > 
> > This is re-send of the series. I didn't receive any comments on this but we
> > found few minor things during in-house review which are incorporated.
> > 
> > Changes to v1:
> > 	- corrected kernel-doc for crash_setup_regs()
> > 	- check return value of ioremap() in copy_oldmem_page()
> > 
> > Changes to RFC version:
> > 	- crash_setup_regs() uses Russell's suggestion for saving registers
> > 	- copy_oldmem_page() code is changed to use ioremap()
> > 	- there is no need for special 'mem=' parameters
> > 
> > I've tested this on N900 and beagleboard.
> 
> Hi Russell,
> 
> Sorry to bother you with this but I wasn't sure what is going to happen with
> these patches. Are you planning to take them or do you want me to change
> something?
> 
> I already submitted these to your patch system as I didn't receive much
> comments (not sure was this the right thing to do, however).
> 
> In addition, I tested these on ep93xx based Sim.One board and they seem to work
> there as well.

Ping..

Do you have any plans for these patches? The reason I'm asking is that there are
also userspace tools (namely makedumpfile and crash) which are needed for
post-mortem analysis, and they depend on kernel support. Without support in
mainline it makes little sense to port these to ARM.

As ARM is going to use LMB I know that patch 6116/1 "kdump: reserve memory for
crashkernel" needs to be changed to use LMB instead of bootmem but that change
should be quite simple.

So do you think I should rework these patches or may they be included in .36? It
would be nice to know if any changes are needed.

Thanks again,
MW

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

Hi,

I have a question regarding these patches. It seems to me that the kexec 
will be done with the MMU left on for the ARMv7 case. Looking at the 
code in the cpu_arm926_reset() routine in arch/arm/mm/proc-arm926.S this 
is not the case there. Instead, the MMU is turned off and the last 
instruction that is fetched using the virtual address mapping is a jump 
to a physical address.

Shouldn't this be handled as consistently as possible for the different 
ARM sub-archs?

Regards,
Per Fransson

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

Hi,

On Fri, Jul 02, 2010 at 02:48:36PM +0200, Per Fransson wrote:
>
> I have a question regarding these patches. It seems to me that the kexec  
> will be done with the MMU left on for the ARMv7 case. Looking at the  
> code in the cpu_arm926_reset() routine in arch/arm/mm/proc-arm926.S this  
> is not the case there. Instead, the MMU is turned off and the last  
> instruction that is fetched using the virtual address mapping is a jump  
> to a physical address.

This seems to be the case for both ARMv6 and ARMv7. I don't know
why the MMU is not switched off there. Anyway, I've been testing
this on ARMv7 and at least for panic kernel it works even when MMU
is left on (didn't check but maybe the decompressor disables that
before jumping to the kernel).

> Shouldn't this be handled as consistently as possible for the different  
> ARM sub-archs?

Yes, I guess. I'm still wondering why this hasn't been fixed already?

Regards,
MW

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/05/2010 10:28 AM, Mika Westerberg wrote:
>>
>> I have a question regarding these patches. It seems to me that the kexec
>> will be done with the MMU left on for the ARMv7 case. Looking at the
>> code in the cpu_arm926_reset() routine in arch/arm/mm/proc-arm926.S this
>> is not the case there. Instead, the MMU is turned off and the last
>> instruction that is fetched using the virtual address mapping is a jump
>> to a physical address.
>
> This seems to be the case for both ARMv6 and ARMv7. I don't know
> why the MMU is not switched off there. Anyway, I've been testing
> this on ARMv7 and at least for panic kernel it works even when MMU
> is left on (didn't check but maybe the decompressor disables that
> before jumping to the kernel).
>

In machine_kexec an identity mapping is set up for the user space part 
of the virtual address space, through a call to setup_mm_for_reboot(). I 
believe this mapping is only necessary because the MMU is kept on and if 
the kexec is done to facilitate the collection of a dump it would be 
nice if a large part of the page table for the crashing context has not 
been corrupted. Don't you agree?

>> Shouldn't this be handled as consistently as possible for the different
>> ARM sub-archs?
>
> Yes, I guess. I'm still wondering why this hasn't been fixed already?

Can't help you there, I'm afraid.

Regards,
Per

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Russell King - ARM Linux]

On Mon, Jul 05, 2010 at 12:01:06PM +0200, Per Fransson wrote:
> In machine_kexec an identity mapping is set up for the user space part  
> of the virtual address space, through a call to setup_mm_for_reboot(). I  
> believe this mapping is only necessary because the MMU is kept on and if  
> the kexec is done to facilitate the collection of a dump it would be  
> nice if a large part of the page table for the crashing context has not  
> been corrupted. Don't you agree?

No.  The identity mapping is there so that we can safely disable the MMU
and jump to the intended address.

There is an implementation defined delay between writing the control
register and the point in the instruction stream that the effect of that
write is seen.

The missing MMU disable for AMRv6 and ARMv7 is an oversight which needs
to be resolved.

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/05/2010 12:18 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 05, 2010 at 12:01:06PM +0200, Per Fransson wrote:
>> In machine_kexec an identity mapping is set up for the user space part
>> of the virtual address space, through a call to setup_mm_for_reboot(). I
>> believe this mapping is only necessary because the MMU is kept on and if
>> the kexec is done to facilitate the collection of a dump it would be
>> nice if a large part of the page table for the crashing context has not
>> been corrupted. Don't you agree?
>
> No.  The identity mapping is there so that we can safely disable the MMU
> and jump to the intended address.
>
> There is an implementation defined delay between writing the control
> register and the point in the instruction stream that the effect of that
> write is seen.
>

But the identity mapping is only set up for user space part. It needs to 
be set up around the code which turns off the MMU for the case when 
there is no delay at all. And it would still be nice to not corrupt the 
page table of the crashing context.

> The missing MMU disable for AMRv6 and ARMv7 is an oversight which needs
> to be resolved.
>

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

On Mon, Jul 05, 2010 at 12:34:15PM +0200, Per Fransson wrote:
> But the identity mapping is only set up for user space part. It needs to  
> be set up around the code which turns off the MMU for the case when  
> there is no delay at all. And it would still be nice to not corrupt the  
> page table of the crashing context.

Yeah, with the current version, user-space mappings of the crashing
process are gone.

If they are really needed, then maybe we could do similar than x86
and set up temporary page tables and do the identity mapping there?
This way the crashing process' page tables would remain untouched.

Regards,
MW

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/05/2010 01:31 PM, Mika Westerberg wrote:
> On Mon, Jul 05, 2010 at 12:34:15PM +0200, Per Fransson wrote:
>> But the identity mapping is only set up for user space part. It needs to
>> be set up around the code which turns off the MMU for the case when
>> there is no delay at all. And it would still be nice to not corrupt the
>> page table of the crashing context.
>
> Yeah, with the current version, user-space mappings of the crashing
> process are gone.
>
> If they are really needed, then maybe we could do similar than x86
> and set up temporary page tables and do the identity mapping there?
> This way the crashing process' page tables would remain untouched.
>

We could move the MMU disabling to the relocate_new_kernel() routine. 
Before jumping there the two (if we are unlucky) page table entries (for 
1 MB sections) that this routine occupies could be put into registers 
and an identity mapping be set up for just those entries. Once we reach 
relocate_new_kernel():

1) the MMU is turned off

2) relocate_new_kernel does it's stuff (not much if a crashkernel area 
is used)

3) the entries are restored, once we are sure the disabling has taken effect

Regards,
Per

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Russell King - ARM Linux]

On Mon, Jul 05, 2010 at 12:34:15PM +0200, Per Fransson wrote:
> On 07/05/2010 12:18 PM, Russell King - ARM Linux wrote:
>> On Mon, Jul 05, 2010 at 12:01:06PM +0200, Per Fransson wrote:
>>> In machine_kexec an identity mapping is set up for the user space part
>>> of the virtual address space, through a call to setup_mm_for_reboot(). I
>>> believe this mapping is only necessary because the MMU is kept on and if
>>> the kexec is done to facilitate the collection of a dump it would be
>>> nice if a large part of the page table for the crashing context has not
>>> been corrupted. Don't you agree?
>>
>> No.  The identity mapping is there so that we can safely disable the MMU
>> and jump to the intended address.
>>
>> There is an implementation defined delay between writing the control
>> register and the point in the instruction stream that the effect of that
>> write is seen.
>>
>
> But the identity mapping is only set up for user space part.

That's because we're normally calling back into the boot loader.

> It needs to  
> be set up around the code which turns off the MMU for the case when  
> there is no delay at all.

There is always a delay as the ARM architecture is pipelined.  By the
time the instruction which writes to the control register has got to
the writeback stage, the following instruction has long since been
fetched from memory, and has probably been decoded and partially
executed.

> And it would still be nice to not corrupt the page table of the
> crashing context.

What you're then asking for is the crashing kernel to allocate 16K of
memory to setup some page tables, and then context switch to that table
so that the MMU can be turned off.

That's never going to be anywhere near reliable.

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/05/2010 03:55 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 05, 2010 at 12:34:15PM +0200, Per Fransson wrote:
>> On 07/05/2010 12:18 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jul 05, 2010 at 12:01:06PM +0200, Per Fransson wrote:
>>>> In machine_kexec an identity mapping is set up for the user space part
>>>> of the virtual address space, through a call to setup_mm_for_reboot(). I
>>>> believe this mapping is only necessary because the MMU is kept on and if
>>>> the kexec is done to facilitate the collection of a dump it would be
>>>> nice if a large part of the page table for the crashing context has not
>>>> been corrupted. Don't you agree?
>>>
>>> No.  The identity mapping is there so that we can safely disable the MMU
>>> and jump to the intended address.
>>>
>>> There is an implementation defined delay between writing the control
>>> register and the point in the instruction stream that the effect of that
>>> write is seen.
>>>
>>
>> But the identity mapping is only set up for user space part.
>
> That's because we're normally calling back into the boot loader.
>

True

>> It needs to
>> be set up around the code which turns off the MMU for the case when
>> there is no delay at all.
>
> There is always a delay as the ARM architecture is pipelined.  By the
> time the instruction which writes to the control register has got to
> the writeback stage, the following instruction has long since been
> fetched from memory, and has probably been decoded and partially
> executed.
>

I'm sure you are right. Although I've actually come across kexec not 
working for this very reason, when running under Qemu. By that I'm not 
saying the kernel should necessarily provide work-arounds for emulator 
inaccuracies.

>> And it would still be nice to not corrupt the page table of the
>> crashing context.
>
> What you're then asking for is the crashing kernel to allocate 16K of
> memory to setup some page tables, and then context switch to that table
> so that the MMU can be turned off.
>
> That's never going to be anywhere near reliable.
>

Well, I had another suggestion in my last mail.

Regards,
Per

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Russell King - ARM Linux]

On Mon, Jul 05, 2010 at 04:05:56PM +0200, Per Fransson wrote:
> Well, I had another suggestion in my last mail.

Which is going to need per-CPU type support in order to achieve.  Do
we really want to implement relocate_new_kernel for each of the 24
CPU types which we currently support?

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/05/2010 04:19 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 05, 2010 at 04:05:56PM +0200, Per Fransson wrote:
>> Well, I had another suggestion in my last mail.
> 
> Which is going to need per-CPU type support in order to achieve.  Do
> we really want to implement relocate_new_kernel for each of the 24
> CPU types which we currently support?
> 

Perhaps not. Couldn't the variable delay be macrofied then, i.e. a macro like

MMU_DISABLED_DELAY_AND_EXEC(<asm instr>)

which would do nothing for an appropriate amount of time and put the instruction in the last MMU mapped slot?


Regards,
Per

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Nicolas Pitre]

On Mon, 5 Jul 2010, Per Fransson wrote:

> On 07/05/2010 04:19 PM, Russell King - ARM Linux wrote:
> > On Mon, Jul 05, 2010 at 04:05:56PM +0200, Per Fransson wrote:
> >> Well, I had another suggestion in my last mail.
> > 
> > Which is going to need per-CPU type support in order to achieve.  Do
> > we really want to implement relocate_new_kernel for each of the 24
> > CPU types which we currently support?
> > 
> 
> Perhaps not. Couldn't the variable delay be macrofied then, i.e. a 
> macro like
> 
> MMU_DISABLED_DELAY_AND_EXEC(<asm instr>)
> 
> which would do nothing for an appropriate amount of time and put the 
> instruction in the last MMU mapped slot?

This delay is not the same across various ARM implementations, and I 
don't think it is even reliably predictable and stable on some 
implementations.


Nicolas

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Russell King - ARM Linux]

On Mon, Jul 05, 2010 at 05:37:11PM +0200, Per Fransson wrote:
> On 07/05/2010 04:19 PM, Russell King - ARM Linux wrote:
> > On Mon, Jul 05, 2010 at 04:05:56PM +0200, Per Fransson wrote:
> >> Well, I had another suggestion in my last mail.
> > 
> > Which is going to need per-CPU type support in order to achieve.  Do
> > we really want to implement relocate_new_kernel for each of the 24
> > CPU types which we currently support?
> > 
> 
> Perhaps not. Couldn't the variable delay be macrofied then, i.e. a
> macro like
> 
> MMU_DISABLED_DELAY_AND_EXEC(<asm instr>)
> 
> which would do nothing for an appropriate amount of time and put the
> instruction in the last MMU mapped slot?

There is no "appropriate amount of time" - some CPUs depend on many
effects that a "3 nops and you'll be fine" approach doesn't work.

Also, what about kernels that support multiple different CPU types.

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/05/2010 08:14 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 05, 2010 at 05:37:11PM +0200, Per Fransson wrote:
>> On 07/05/2010 04:19 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jul 05, 2010 at 04:05:56PM +0200, Per Fransson wrote:
>>>> Well, I had another suggestion in my last mail.
>>>
>>> Which is going to need per-CPU type support in order to achieve.  Do
>>> we really want to implement relocate_new_kernel for each of the 24
>>> CPU types which we currently support?
>>>
>>
>> Perhaps not. Couldn't the variable delay be macrofied then, i.e. a
>> macro like
>>
>> MMU_DISABLED_DELAY_AND_EXEC(<asm instr>)
>>
>> which would do nothing for an appropriate amount of time and put the
>> instruction in the last MMU mapped slot?
>
> There is no "appropriate amount of time" - some CPUs depend on many
> effects that a "3 nops and you'll be fine" approach doesn't work.
>
> Also, what about kernels that support multiple different CPU types.
>

Ok, I see what you're both saying about there not always being an 
"appropriate amount of time". But surely there must be a minimum number 
of instructions after which we can be sure the MMU will be off? Can't we 
set up the identity mapping for only the entry containing 
relocate_new_kernel() and then add enough NOPs at the start of that 
routine to cover all implementations? That way only one entry in the L1 
table is over-written while keeping the MMU handling code in the 
different arch/arm/mm/proc-<cpu_or_arch>.S?

Also, couldn't the L1 page table entry in question be saved for 
posterity in a variable inside the kernel before the table is modified, 
together with another variable to hold information on the index in the 
table the entry came from.

Regards,
Per

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

On Tue, Jul 06, 2010 at 10:30:53AM +0200, ext Per Fransson wrote:
> 
> Can't we set up the identity mapping for only the entry containing
> relocate_new_kernel() and then add enough NOPs at the start of that routine to
> cover all implementations? That way only one entry in the L1 table is
> over-written while keeping the MMU handling code in the different
> arch/arm/mm/proc-<cpu_or_arch>.S?

Did you mean something like the patch at the end of this mail? It doesn't turn
off the MMU but sets up the identity mapping for the control page only. I
quickly tested it on our platform and it seems to work:

crash> bt
PID: 1505   TASK: ddc51d40  CPU: 0   COMMAND: "sh"
 #0 [<c00890b0>] (crash_kexec) from [<c037ecd0>]
 #1 [<c037ecd0>] (panic) from [<c0030544>]
 #2 [<c0030544>] (die) from [<c0032630>]
 #3 [<c0032630>] (__do_kernel_fault) from [<c0032804>]
 #4 [<c0032804>] (do_page_fault) from [<c002c2b4>]
 #5 [<c002c2b4>] (do_DataAbort) from [<c002ca6c>]
    pc : [<c0252d74>]    lr : [<c0252f54>]    psr: 600000d3
    sp : ddca1f18  ip : 00005d75  fp : bede761c
    r10: 00000000  r9 : ddca0000  r8 : 00000007
    r7 : 00000063  r6 : 00000000  r5 : 60000053  r4 : c04e0a10
    r3 : 00000001  r2 : 00000000  r1 : 00000000  r0 : 00000063
    Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM
 #6 [<c002ca6c>] (__dabt_svc) from [<c0252f54>]
 #7 [<c0252d74>] (sysrq_handle_crash) from [<c0252f54>]
 #8 [<c0252f54>] (__handle_sysrq) from [<c0253050>]
 #9 [<c0253050>] (write_sysrq_trigger) from [<c010e9bc>]
#10 [<c010e9bc>] (proc_reg_write) from [<c00ca0a0>]
#11 [<c00ca0a0>] (vfs_write) from [<c00ca1f4>]
#12 [<c00ca1f4>] (sys_write) from [<c002cf40>]

crash> ps -a 1505
PID: 1505   TASK: ddc51d40  CPU: 0   COMMAND: "sh"
ARG: -sh 
ENV: TERM=linux
     PATH=/sbin:/usr/sbin:/bin:/usr/bin
     USER=root
     LOGNAME=root
     HOME=/root
     SHELL=/bin/sh

So we can at least access the user stack.

However, I'm not sure what is happening with these kdump patches. If they are
going in at some point maybe we can do this stuff later on as separate patches
or should post a new version of the patches?

> Also, couldn't the L1 page table entry in question be saved for 
> posterity in a variable inside the kernel before the table is modified, 
> together with another variable to hold information on the index in the 
> table the entry came from.

Yeah, I think that we want to have full user-space mappings for post-mortem
analysis.

Regards,
MW

>From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
Subject: [PATCH] ARM: kexec: make identity mapping for control page only

With current implementation we setup 1:1 mapping for all user-space pages in
order to softboot to a new kernel. This has a drawback that we cannot access
those pages later on during post-mortem analysis.

This patch changes kexec to setup identity mapping for only the control page
(this takes 2 PMD entries) and leaves rest of the mappings intact.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/machine_kexec.c |   42 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 81e9898..518c1ad 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -7,6 +7,7 @@
 #include <linux/delay.h>
 #include <linux/reboot.h>
 #include <linux/io.h>
+#include <asm/cputype.h>
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
 #include <asm/mmu_context.h>
@@ -16,8 +17,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;
@@ -49,6 +48,43 @@ void machine_crash_shutdown(struct pt_regs *regs)
 	printk(KERN_INFO "Loading crashdump kernel...\n");
 }
 
+/**
+ * setup_identity_mapping() - sets up 1:1 identity mapping for a control page
+ * @phys: physical address of the control page
+ *
+ * Sets up necessary 1:1 identity mapping for user-space pages covering kexec
+ * control page. Other user-space mappings are kept intact.
+ *
+ * TODO: We could save these 2 PMD entries and restore them in
+ *       relocate_new_kernel() when we are sure that the MMU is off.
+ *       This would allow us to have complete user-space mappings
+ *       for post-mortem analysis.
+ */
+static void setup_identity_mapping(unsigned long phys)
+{
+	unsigned long pmdval = phys & PMD_MASK;
+	pgd_t *pgd;
+	pmd_t *pmd;
+
+	/*
+	 * We need to access to user-mode page tables here. For kernel threads
+	 * we don't have any user-mode mappings so we use the context that we
+	 * "borrowed".
+	 */
+	pgd = pgd_offset(current->active_mm, phys);
+	pmd = pmd_offset(pgd, phys);
+
+	pmdval |= PMD_SECT_AP_WRITE | PMD_SECT_AP_READ | PMD_TYPE_SECT;
+	if (cpu_architecture() <= CPU_ARCH_ARMv5TEJ && !cpu_is_xscale())
+		pmdval |= PMD_BIT4;
+
+	pmd[0] = __pmd(pmdval);
+	pmd[1] = __pmd(pmdval + (1 << (PGDIR_SHIFT - 1)));
+
+	flush_pmd_entry(pmd);
+	local_flush_tlb_all();
+}
+
 void machine_kexec(struct kimage *image)
 {
 	unsigned long page_list;
@@ -79,6 +115,6 @@ void machine_kexec(struct kimage *image)
 	printk(KERN_INFO "Bye!\n");
 
 	cpu_proc_fin();
-	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
+	setup_identity_mapping(reboot_code_buffer_phys);
 	cpu_reset(reboot_code_buffer_phys);
 }
-- 
1.5.6.5

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/07/2010 09:29 AM, Mika Westerberg wrote:
> On Tue, Jul 06, 2010 at 10:30:53AM +0200, ext Per Fransson wrote:
>>
>> Can't we set up the identity mapping for only the entry containing
>> relocate_new_kernel() and then add enough NOPs at the start of that routine to
>> cover all implementations? That way only one entry in the L1 table is
>> over-written while keeping the MMU handling code in the different
>> arch/arm/mm/proc-<cpu_or_arch>.S?
>
> Did you mean something like the patch at the end of this mail? It doesn't turn
> off the MMU but sets up the identity mapping for the control page only. I
> quickly tested it on our platform and it seems to work:
>
> crash>  bt
...
> So we can at least access the user stack.
>

Yes, that's what I had in mind. A delay will have to be introduced at 
the start of relocate_new_kernel as well. And we have to make sure that 
it's not possible for this code to straddle two L1 page table entries, 
which might be the case already, I don't know. Finally, the overwritten 
entry needs to be stored somewhere before cleaning the caches.

> However, I'm not sure what is happening with these kdump patches. If they are
> going in at some point maybe we can do this stuff later on as separate patches
> or should post a new version of the patches?
>

I'm also interested in knowing whether there's a chance of these patches 
going in soon.

Is the solution for saving the user-space MMU mapping we've sketched 
here good enough? If yes, we might as well try to do it all in one go, 
as far as I'm concerned. Maybe a new version of the patches is needed 
anyway to fix the inconsistencies between CPUs with regards to the MMU? 
On the other hand, if there's a chance of getting these patches in 
quicker as they stand, it would be nice to seize the opportunity for 
kdump/ARM to get a footing in mainline.

>> Also, couldn't the L1 page table entry in question be saved for
>> posterity in a variable inside the kernel before the table is modified,
>> together with another variable to hold information on the index in the
>> table the entry came from.
>
> Yeah, I think that we want to have full user-space mappings for post-mortem
> analysis.
>
> Regards,
> MW
>
>> From: Mika Westerberg<ext-mika.1.westerberg@nokia.com>
> Subject: [PATCH] ARM: kexec: make identity mapping for control page only
>
> With current implementation we setup 1:1 mapping for all user-space pages in
> order to softboot to a new kernel. This has a drawback that we cannot access
> those pages later on during post-mortem analysis.
>
> This patch changes kexec to setup identity mapping for only the control page
> (this takes 2 PMD entries) and leaves rest of the mappings intact.
>

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Mika Westerberg]

On Thu, Jul 08, 2010 at 10:52:42AM +0200, ext Per Fransson wrote:
> 
> Yes, that's what I had in mind. A delay will have to be introduced at 
> the start of relocate_new_kernel as well. And we have to make sure that 
> it's not possible for this code to straddle two L1 page table entries, 
> which might be the case already, I don't know. Finally, the overwritten 
> entry needs to be stored somewhere before cleaning the caches.

Hi,

Now that I (hopefully) understand this little bit better, I made a small change
to kexec code according what you proposed. It, however creates identity mapping
for the cpu_reset() function instead of relocate_new_kernel(). At least if I
understand ARMv7 specs correctly, it is recommended to run the code which
disables/enables the MMU with VA == PA. I also tried to just disable the MMU but
if I'm not running with VA == PA, it hangs.

I'm not sure whether the MMU disabling code is correct, should we do something
else before switching the MMU off? In OMAP3 it seems to work as is.

Regards,
MW

Subject: ARM: kexec: create identity mapping for cpu_reset

With current implementation we setup 1:1 mapping for all user-space pages in
order to softboot to a new kernel. This has a drawback that we cannot access
those pages later on during post-mortem analysis. We also leave MMU on when
calling the secondary kernel.

This patch makes identity mapping for the cpu_reset() function only. This way we
can be sure that we are running with VA == PA when the MMU is disabled.

relocate_new_kernel() restores the trashed 2 PMD entries which can be then used
for post-mortem analysis.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 arch/arm/kernel/machine_kexec.c   |   62 ++++++++++++++++++++++++++++++++++---
 arch/arm/kernel/relocate_kernel.S |   26 +++++++++++++++
 arch/arm/mm/proc-v7.S             |   18 +++++++++++
 3 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 81e9898..4cfad60 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -7,6 +7,7 @@
 #include <linux/delay.h>
 #include <linux/reboot.h>
 #include <linux/io.h>
+#include <asm/cputype.h>
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
 #include <asm/mmu_context.h>
@@ -16,12 +17,13 @@
 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;
 extern unsigned long kexec_boot_atags;
+extern unsigned long kexec_pmd_addr;
+extern unsigned long kexec_pmd_val0;
+extern unsigned long kexec_pmd_val1;
 
 /*
  * Provide a dummy crash_notes definition while crash dump arrives to arm.
@@ -49,12 +51,59 @@ void machine_crash_shutdown(struct pt_regs *regs)
 	printk(KERN_INFO "Loading crashdump kernel...\n");
 }
 
+/**
+ * setup_identity_mapping() - set up identity mapping for given address
+ * @paddr: physical address which is mapped
+ *
+ * This function sets up indentity mapping for the given CPU reset function. We
+ * do the worst case and allocate 2 PMD entries. This is due the fact that
+ * cpu_reset() might be split into subsequent sections. Original PMD entries are
+ * placed in @kexec_pmd_val0 and @kexec_pmd_val1, and address of the first PMD
+ * is placed in @kexec_pmd_addr.
+ */
+static void setup_identity_mapping(unsigned long paddr)
+{
+	unsigned long pmdval = paddr & SECTION_MASK;
+	pgd_t *pgd;
+	pmd_t *pmd;
+
+	/*
+	 * We need to access to user-mode page tables here. For kernel threads
+	 * we don't have any user-mode mappings so we use the context that we
+	 * "borrowed".
+	 */
+	pgd = pgd_offset(current->active_mm, paddr);
+	pmd = pmd_offset(pgd, paddr);
+
+	/*
+	 * Store the both original PMD entries. These are restored later on by
+	 * relocate_new_kernel().
+	 */
+	kexec_pmd_addr = __pa(pmd);
+	kexec_pmd_val0 = pmd[0];
+	kexec_pmd_val1 = pmd[1];
+
+	pmdval |= PMD_SECT_AP_WRITE | PMD_SECT_AP_READ | PMD_TYPE_SECT;
+	if (cpu_architecture() <= CPU_ARCH_ARMv5TEJ && !cpu_is_xscale())
+		pmdval |= PMD_BIT4;
+
+	/*
+	 * Place identity mapping for the 2 sections.
+	 */
+	pmd[0] = __pmd(pmdval);
+	pmd[1] = __pmd(pmdval + (1 << (PGDIR_SHIFT - 1)));
+
+	flush_pmd_entry(pmd);
+}
+
 void machine_kexec(struct kimage *image)
 {
 	unsigned long page_list;
 	unsigned long reboot_code_buffer_phys;
 	void *reboot_code_buffer;
+	void (*reset_fn)(unsigned long);
 
+	reset_fn = (void (*)(unsigned long))__pa(cpu_reset);
 
 	page_list = image->head & PAGE_MASK;
 
@@ -69,16 +118,19 @@ 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;
 
+	setup_identity_mapping(__pa(cpu_reset));
+	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");
 
 	cpu_proc_fin();
-	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
-	cpu_reset(reboot_code_buffer_phys);
+
+	/* call the CPU reset function through the identity mapping */
+	(*reset_fn)(reboot_code_buffer_phys);
 }
diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
index fd26f8d..028f889 100644
--- a/arch/arm/kernel/relocate_kernel.S
+++ b/arch/arm/kernel/relocate_kernel.S
@@ -11,6 +11,17 @@ relocate_new_kernel:
 	ldr	r1,kexec_start_address
 
 	/*
+	 * First restore the 2 PMD entries that were thrashed when identity
+	 * mapping was created for CPU reset function. These are needed for
+	 * possible post-mortem analysis.
+	 */
+	ldr	r2, kexec_pmd_addr
+	ldr	r3, kexec_pmd_val0
+	ldr	r4, kexec_pmd_val1
+	str	r3, [r2], #4
+	str	r4, [r2]
+
+	/*
 	 * If there is no indirection page (we are doing crashdumps)
 	 * skip any relocation.
 	 */
@@ -76,6 +87,21 @@ kexec_mach_type:
 kexec_boot_atags:
 	.long	0x0
 
+/*
+ * machine_kexec() changes user-space mappings for cpu_reset() function. The 2
+ * original values are stored here, and will be restored when
+ * relocate_new_kernel is called (with MMU off).
+ */
+	.globl kexec_pmd_addr
+	.globl kexec_pmd_val0
+	.globl kexec_pmd_val1
+kexec_pmd_addr:
+	.long	0x0
+kexec_pmd_val0:
+	.long	0x0
+kexec_pmd_val1:
+	.long	0x0
+
 relocate_new_kernel_end:
 
 	.globl relocate_new_kernel_size
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 7aaf88a..f5092cb 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -62,10 +62,28 @@ ENDPROC(cpu_v7_proc_fin)
  *	same state as it would be if it had been reset, and branch
  *	to what would be the reset vector.
  *
+ *	This function should be called only when VA == PA (e.g make indentity
+ *	mapping for the function and jump into __pa(cpu_v7_reset)).
+ *
  *	- loc   - location to jump to for soft reset
  */
 	.align	5
 ENTRY(cpu_v7_reset)
+#ifdef CONFIG_MMU
+	mcr	p15, 0, ip, c8, c7, 0	@ invalidate I & D TLBs
+#endif
+	mrc	p15, 0, ip, c1, c0
+	bic	ip, ip, #0x0001
+	mcr	p15, 0, ip, c1, c0	@ turn MMU off
+
+	/*
+	 * Now provide a small delay which should guarantee that MMU is really
+	 * switched off.
+	 */
+	nop; nop; nop
+	nop; nop; nop
+
+	/* and jump to the reset address */
 	mov	pc, r0
 ENDPROC(cpu_v7_reset)
 
-- 
1.5.6.5

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Simon Horman]

On Mon, Jul 05, 2010 at 02:55:52PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 05, 2010 at 12:34:15PM +0200, Per Fransson wrote:
> > On 07/05/2010 12:18 PM, Russell King - ARM Linux wrote:
> >> On Mon, Jul 05, 2010 at 12:01:06PM +0200, Per Fransson wrote:
> >>> In machine_kexec an identity mapping is set up for the user space part
> >>> of the virtual address space, through a call to setup_mm_for_reboot(). I
> >>> believe this mapping is only necessary because the MMU is kept on and if
> >>> the kexec is done to facilitate the collection of a dump it would be
> >>> nice if a large part of the page table for the crashing context has not
> >>> been corrupted. Don't you agree?
> >>
> >> No.  The identity mapping is there so that we can safely disable the MMU
> >> and jump to the intended address.
> >>
> >> There is an implementation defined delay between writing the control
> >> register and the point in the instruction stream that the effect of that
> >> write is seen.
> >>
> >
> > But the identity mapping is only set up for user space part.
> 
> That's because we're normally calling back into the boot loader.
> 
> > It needs to  
> > be set up around the code which turns off the MMU for the case when  
> > there is no delay at all.
> 
> There is always a delay as the ARM architecture is pipelined.  By the
> time the instruction which writes to the control register has got to
> the writeback stage, the following instruction has long since been
> fetched from memory, and has probably been decoded and partially
> executed.
> 
> > And it would still be nice to not corrupt the page table of the
> > crashing context.
> 
> What you're then asking for is the crashing kernel to allocate 16K of
> memory to setup some page tables, and then context switch to that table
> so that the MMU can be turned off.
> 
> That's never going to be anywhere near reliable.

Can this be addresses by allocating the memory at the time that kdump
is loaded rather than when it is executed (because of a crash)?

IIRC, x86 uses a double page table approach and it is done reliably.

[PATCH v2 0/8] Initial implementation of kdump for ARM

[Per Fransson]

On 07/09/2010 05:38 AM, Simon Horman wrote:
>>> And it would still be nice to not corrupt the page table of the
>>> crashing context.
>>
>> What you're then asking for is the crashing kernel to allocate 16K of
>> memory to setup some page tables, and then context switch to that table
>> so that the MMU can be turned off.
>>
>> That's never going to be anywhere near reliable.
>
> Can this be addresses by allocating the memory at the time that kdump
> is loaded rather than when it is executed (because of a crash)?
>
> IIRC, x86 uses a double page table approach and it is done reliably.
>
>

We could do that.

Admittedly, this is getting very "hacky", but if we want to set up a new 
L1 page table in which we really only need one entry (for the code 
running until we're sure the MMU is off), we could set up this one entry 
all by itself placed at just the right offset within a page and then set 
the PGD to point at the start of that page (since we're talking about 
mappings for 0x00000000-0x40000000). The rest of the page could still be 
used for other stuff - and even if this turns out to be more trouble 
than it's worth, the other 12KB do not need to be allocated.

However, I still think we could work within the old L1 table and:

1) only modify one entry

2) save that entry before overwriting it

Regards,
Per