Initial implementation of kdump for ARM

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

Quoted 9 lines · 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().

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?

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?

Quoted 3 lines

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.

Quoted 5 lines

Quoted 4 lines

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?

Quoted 3 lines

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

On Mon, May 10, 2010 at 03:09:20PM +0300, Mika Westerberg wrote:

Quoted 3 lines

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

Quoted 1 line · ext Russell King - ARM Linux

On Mon, May 10, 2010 at 03:09:20PM +0300, Mika Westerberg wrote:

[...]

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

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.

Quoted 8 lines · On Wed, May 05, 2010 at 08:54:12AM +0200, Westerberg Mika.1…

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()

Quoted 6 lines

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.

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?

Quoted 7 lines

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

Quoted 2 lines

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?

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?

Quoted 6 lines · Per Fransson

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.

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.

Quoted 4 lines · Per Fransson

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.

Quoted 11 lines · Russell King - ARM Linux

Quoted 6 lines · Per Fransson

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.

Quoted 4 lines

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.

Quoted 12 lines · Per Fransson

Quoted 11 lines · Russell King - ARM Linux

Quoted 6 lines · Per Fransson

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 1 line · Per Fransson

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?

Quoted 7 lines · Russell King - ARM Linux

Quoted 1 line · Per Fransson

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?

Quoted 7 lines · Russell King - ARM Linux

Quoted 1 line · Per Fransson

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?

Quoted 14 lines · Per Fransson

Quoted 7 lines · Russell King - ARM Linux

Quoted 1 line · Per Fransson

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

Quoted 2 lines

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.

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.

From: Mika Westerberg <[email protected]>

Quoted 6 lines · ext Per Fransson

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.

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?

Quoted 4 lines

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

Quoted 1 line

From: Mika Westerberg<[email protected]>

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.

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.

Quoted 12 lines · Per Fransson

Quoted 11 lines · Russell King - ARM Linux

Quoted 6 lines · Per Fransson

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 1 line

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.