* [patch] crashdump: fix undefined reference to `elfcorehdr_addr' @ 2008-07-26 9:25 Ingo Molnar 2008-07-26 10:10 ` Andrew Morton 0 siblings, 1 reply; 28+ messages in thread From: Ingo Molnar @ 2008-07-26 9:25 UTC (permalink / raw) To: Andrew Morton, Linus Torvalds; +Cc: linux-kernel please apply before -rc1. Ingo -----------> >From 72db7cba50b6a05825f8a287f74002cc38f04fb7 Mon Sep 17 00:00:00 2001 From: Ingo Molnar <mingo@elte.hu> Date: Sat, 26 Jul 2008 11:22:33 +0200 Subject: [PATCH] crashdump: fix undefined reference to `elfcorehdr_addr' fix build bug introduced by 95b68dec0d5 "calgary iommu: use the first kernels TCE tables in kdump": arch/x86/kernel/built-in.o: In function `calgary_iommu_init': (.init.text+0x8399): undefined reference to `elfcorehdr_addr' arch/x86/kernel/built-in.o: In function `calgary_iommu_init': (.init.text+0x856c): undefined reference to `elfcorehdr_addr' arch/x86/kernel/built-in.o: In function `detect_calgary': (.init.text+0x8c68): undefined reference to `elfcorehdr_addr' arch/x86/kernel/built-in.o: In function `detect_calgary': (.init.text+0x8d0c): undefined reference to `elfcorehdr_addr' make elfcorehdr_addr a generally available symbol. Signed-off-by: Ingo Molnar <mingo@elte.hu> --- include/linux/crash_dump.h | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h index 6cd39a9..025e4f5 100644 --- a/include/linux/crash_dump.h +++ b/include/linux/crash_dump.h @@ -8,7 +8,13 @@ #include <linux/proc_fs.h> #define ELFCORE_ADDR_MAX (-1ULL) + +#ifdef CONFIG_PROC_VMCORE extern unsigned long long elfcorehdr_addr; +#else +static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; +#endif + extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, unsigned long, int); extern const struct file_operations proc_vmcore_operations; ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-26 9:25 [patch] crashdump: fix undefined reference to `elfcorehdr_addr' Ingo Molnar @ 2008-07-26 10:10 ` Andrew Morton 2008-07-27 23:45 ` Simon Horman 0 siblings, 1 reply; 28+ messages in thread From: Andrew Morton @ 2008-07-26 10:10 UTC (permalink / raw) To: Ingo Molnar Cc: Linus Torvalds, linux-kernel, Chandru, Muli Ben-Yehuda, Vivek Goyal, kexec On Sat, 26 Jul 2008 11:25:19 +0200 Ingo Molnar <mingo@elte.hu> wrote: > > please apply before -rc1. > > Ingo > > -----------> > >From 72db7cba50b6a05825f8a287f74002cc38f04fb7 Mon Sep 17 00:00:00 2001 > From: Ingo Molnar <mingo@elte.hu> > Date: Sat, 26 Jul 2008 11:22:33 +0200 > Subject: [PATCH] crashdump: fix undefined reference to `elfcorehdr_addr' > > fix build bug introduced by 95b68dec0d5 "calgary iommu: use the first > kernels TCE tables in kdump": > > arch/x86/kernel/built-in.o: In function `calgary_iommu_init': > (.init.text+0x8399): undefined reference to `elfcorehdr_addr' > arch/x86/kernel/built-in.o: In function `calgary_iommu_init': > (.init.text+0x856c): undefined reference to `elfcorehdr_addr' > arch/x86/kernel/built-in.o: In function `detect_calgary': > (.init.text+0x8c68): undefined reference to `elfcorehdr_addr' > arch/x86/kernel/built-in.o: In function `detect_calgary': > (.init.text+0x8d0c): undefined reference to `elfcorehdr_addr' > > make elfcorehdr_addr a generally available symbol. > > Signed-off-by: Ingo Molnar <mingo@elte.hu> > --- > include/linux/crash_dump.h | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > index 6cd39a9..025e4f5 100644 > --- a/include/linux/crash_dump.h > +++ b/include/linux/crash_dump.h > @@ -8,7 +8,13 @@ > #include <linux/proc_fs.h> > > #define ELFCORE_ADDR_MAX (-1ULL) > + > +#ifdef CONFIG_PROC_VMCORE > extern unsigned long long elfcorehdr_addr; > +#else > +static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; > +#endif > + > extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, > unsigned long, int); > extern const struct file_operations proc_vmcore_operations; spose that'll fix it. But it seems odd that is_kdump_kernel() will return false if CONFIG_PROC_VMCORE=n, CONFIG_CRASH_DUMP=y. I mean, it's still a crashdump kernel, is it not? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-26 10:10 ` Andrew Morton @ 2008-07-27 23:45 ` Simon Horman 2008-07-28 1:51 ` Simon Horman 0 siblings, 1 reply; 28+ messages in thread From: Simon Horman @ 2008-07-27 23:45 UTC (permalink / raw) To: Andrew Morton Cc: Ingo Molnar, Muli Ben-Yehuda, Chandru, kexec, linux-kernel, Vivek Goyal, Linus Torvalds On Sat, Jul 26, 2008 at 03:10:34AM -0700, Andrew Morton wrote: > On Sat, 26 Jul 2008 11:25:19 +0200 Ingo Molnar <mingo@elte.hu> wrote: > > > > > please apply before -rc1. > > > > Ingo > > > > -----------> > > >From 72db7cba50b6a05825f8a287f74002cc38f04fb7 Mon Sep 17 00:00:00 2001 > > From: Ingo Molnar <mingo@elte.hu> > > Date: Sat, 26 Jul 2008 11:22:33 +0200 > > Subject: [PATCH] crashdump: fix undefined reference to `elfcorehdr_addr' > > > > fix build bug introduced by 95b68dec0d5 "calgary iommu: use the first > > kernels TCE tables in kdump": > > > > arch/x86/kernel/built-in.o: In function `calgary_iommu_init': > > (.init.text+0x8399): undefined reference to `elfcorehdr_addr' > > arch/x86/kernel/built-in.o: In function `calgary_iommu_init': > > (.init.text+0x856c): undefined reference to `elfcorehdr_addr' > > arch/x86/kernel/built-in.o: In function `detect_calgary': > > (.init.text+0x8c68): undefined reference to `elfcorehdr_addr' > > arch/x86/kernel/built-in.o: In function `detect_calgary': > > (.init.text+0x8d0c): undefined reference to `elfcorehdr_addr' > > > > make elfcorehdr_addr a generally available symbol. > > > > Signed-off-by: Ingo Molnar <mingo@elte.hu> > > --- > > include/linux/crash_dump.h | 6 ++++++ > > 1 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > > index 6cd39a9..025e4f5 100644 > > --- a/include/linux/crash_dump.h > > +++ b/include/linux/crash_dump.h > > @@ -8,7 +8,13 @@ > > #include <linux/proc_fs.h> > > > > #define ELFCORE_ADDR_MAX (-1ULL) > > + > > +#ifdef CONFIG_PROC_VMCORE > > extern unsigned long long elfcorehdr_addr; > > +#else > > +static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; > > +#endif > > + > > extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, > > unsigned long, int); > > extern const struct file_operations proc_vmcore_operations; > > spose that'll fix it. But it seems odd that is_kdump_kernel() will > return false if CONFIG_PROC_VMCORE=n, CONFIG_CRASH_DUMP=y. I mean, > it's still a crashdump kernel, is it not? Perhaps is_kdump_kernel() ought to be renamed kernel_has_vmcore(). To my mind, is_kdump_kernel() should really look something like this: #ifdef CONFIG_CRASH_DUMP static inline int is_kdump_kernel(void) { return 1; } #else static inline int is_kdump_kernel(void) { return 0; } #endif But that can probably just be handled by any relevant code using CONFIG_CRASH_DUMP as necessary. -- Horms ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-27 23:45 ` Simon Horman @ 2008-07-28 1:51 ` Simon Horman 2008-07-28 2:45 ` Simon Horman ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Simon Horman @ 2008-07-28 1:51 UTC (permalink / raw) To: Andrew Morton Cc: Muli Ben-Yehuda, Chandru, kexec, linux-kernel, Vivek Goyal, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, Eric W. Biederman, linux-ia64 [ Updated Vivek's email address to his vgoyal@redhat.com in CC list Added Terry Loftin, Tony Luck, Erik Biedermann and linux-ia64 to CC list ] On Mon, Jul 28, 2008 at 09:45:31AM +1000, Simon Horman wrote: > > > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > > > index 6cd39a9..025e4f5 100644 > > > --- a/include/linux/crash_dump.h > > > +++ b/include/linux/crash_dump.h > > > @@ -8,7 +8,13 @@ > > > #include <linux/proc_fs.h> > > > > > > #define ELFCORE_ADDR_MAX (-1ULL) > > > + > > > +#ifdef CONFIG_PROC_VMCORE > > > extern unsigned long long elfcorehdr_addr; > > > +#else > > > +static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; > > > +#endif > > > + > > > extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, > > > unsigned long, int); > > > extern const struct file_operations proc_vmcore_operations; > > > > spose that'll fix it. But it seems odd that is_kdump_kernel() will > > return false if CONFIG_PROC_VMCORE=n, CONFIG_CRASH_DUMP=y. I mean, > > it's still a crashdump kernel, is it not? > > Perhaps is_kdump_kernel() ought to be renamed kernel_has_vmcore(). > > To my mind, is_kdump_kernel() should really look something like this: > > #ifdef CONFIG_CRASH_DUMP > static inline int is_kdump_kernel(void) { return 1; } > #else > static inline int is_kdump_kernel(void) { return 0; } > #endif > > But that can probably just be handled by any relevant code > using CONFIG_CRASH_DUMP as necessary. Hi, I started looking into a simple fix to change the name of the is_kdump_kernel() to kernel_has_vmcore(), which is what the code in its current incarnatation does. This also lead to cleaning the usage of elfcorehdr_addr, which is in the folloing messy state after recent changes. #ifdef CONFIG_PROC_VMCORE * Declared non-static include/linux/crash_dump.h * Initialised in fs/proc/vmcore.c #else * Declared and initialised as static in include/linux/crash_dump.h * Only used by is_kdump_kernel() which is a static function also in include/linux/crash_dump.h #endif Howerver, in the course of doing this I came to thinking that actually this code won't solve the problem at hand in the case where CONFIG_CRASH_DUMP is defined but CONFIG_PROC_VMCORE is not. Or in other words, what happens if the calgary initialisation code runs in a kdump kernel that does not have CONFIG_PROC_VMCORE ? A similar problem appears to exist in arch/ia64/hp/common/sba_iommu.c:sba_init(), which currently doesn't compile if CONFIG_CRASH_DUMP is set but CONFIG_PROC_VMCORE is not. The compilation issue could be solved by using kernel_has_vmcore() (as per the patch below) instead of checking elfcorehdr_addr directly, but does it actually lead to working code? There has long been a strong aversion to providing the second kernel with flags like im_in_kexec or im_in_kdump, as its felt that this kind of problem is better handled by making sure that the hardware is in a sensible state before leaving the first-kernel. But this is arguably more reasonable in the kexec case than the kdump case. If there really is a need for kdump kernels to know that they are booting a kdumping system, then I propose one of the following: 1) Always parse the elfcorehdr kernel command line option and set elfcorehdr_addr accordingly - currently this is only done if CONFIG_PROC_VMCORE is set. This is nice as it won't need any modifications to kexec-tools nor any command line bloat. A minor difficulty is working out where to initialise elfcorehdr_addr. Sometimes in include/linux/crash_dump.h and sometimes in fs/proc/vmcore.c seems horrible to me. Another problem is that would be alive and well in code that really only uses it to check if kdump was activated or not - a minor naming issue. 2) Add a new kernel command line option, perhaps in_kdump This is bloat to get around elfcorehdr_addr initialisation and naming awkwardness above. 3) Make select CONFIG_PROC_VMCORE when CONFIG_CRASH_DUMP is selected, or perhaps even just remove CONFIG_PROC_VMCORE and only use CONFIG_CRASH_DUMP instead. The effect would be the same either way. Pro: One less thing to be confused about Con: Bloat for people who want kdump without vmcore. I wonder what usage case that is. -- Horms Index: linux-2.6/arch/x86/kernel/pci-calgary_64.c =================================================================== --- linux-2.6.orig/arch/x86/kernel/pci-calgary_64.c 2008-07-28 10:10:31.000000000 +1000 +++ linux-2.6/arch/x86/kernel/pci-calgary_64.c 2008-07-28 10:14:24.000000000 +1000 @@ -801,7 +801,7 @@ static int __init calgary_setup_tar(stru tbl = pci_iommu(dev->bus); tbl->it_base = (unsigned long)bus_info[dev->bus->number].tce_space; - if (is_kdump_kernel()) + if (kernel_has_vmcore()) calgary_init_bitmap_from_tce_table(tbl); else tce_free(tbl, 0, tbl->it_size); @@ -1184,7 +1184,7 @@ static int __init calgary_init(void) return ret; /* Purely for kdump kernel case */ - if (is_kdump_kernel()) + if (kernel_has_vmcore()) get_tce_space_from_tar(); do { @@ -1438,7 +1438,7 @@ void __init detect_calgary(void) return; } - specified_table_size = determine_tce_table_size((is_kdump_kernel() ? + specified_table_size = determine_tce_table_size((kernel_has_vmcore() ? saved_max_pfn : max_pfn) * PAGE_SIZE); for (bus = 0; bus < MAX_PHB_BUS_NUM; bus++) { @@ -1461,7 +1461,7 @@ void __init detect_calgary(void) * If it is kdump kernel, find and use tce tables * from first kernel, else allocate tce tables here */ - if (!is_kdump_kernel()) { + if (!kernel_has_vmcore()) { tbl = alloc_tce_table(); if (!tbl) goto cleanup; Index: linux-2.6/fs/proc/vmcore.c =================================================================== --- linux-2.6.orig/fs/proc/vmcore.c 2008-07-28 09:51:30.000000000 +1000 +++ linux-2.6/fs/proc/vmcore.c 2008-07-28 09:51:53.000000000 +1000 @@ -32,8 +32,7 @@ static size_t elfcorebuf_sz; /* Total size of vmcore file. */ static u64 vmcore_size; -/* Stores the physical address of elf header of crash image. */ -unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; +unsigned long long elfcorehdr_addr; struct proc_dir_entry *proc_vmcore = NULL; Index: linux-2.6/include/linux/crash_dump.h =================================================================== --- linux-2.6.orig/include/linux/crash_dump.h 2008-07-28 09:46:29.000000000 +1000 +++ linux-2.6/include/linux/crash_dump.h 2008-07-28 10:43:03.000000000 +1000 @@ -11,8 +11,13 @@ #ifdef CONFIG_PROC_VMCORE extern unsigned long long elfcorehdr_addr; + +static inline int kernel_has_vmcore(void) +{ + return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; +} #else -static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; +static inline int kernel_has_vmcore(void) { return 0; } #endif extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, @@ -28,12 +33,6 @@ extern struct proc_dir_entry *proc_vmcor #define vmcore_elf_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) -static inline int is_kdump_kernel(void) -{ - return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; -} -#else /* !CONFIG_CRASH_DUMP */ -static inline int is_kdump_kernel(void) { return 0; } #endif /* CONFIG_CRASH_DUMP */ extern unsigned long saved_max_pfn; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 1:51 ` Simon Horman @ 2008-07-28 2:45 ` Simon Horman 2008-07-28 3:40 ` Simon Horman 2008-07-28 5:39 ` [patch] crashdump: fix undefined reference to `elfcorehdr_addr' Eric W. Biederman 2008-07-28 13:31 ` Vivek Goyal 2 siblings, 1 reply; 28+ messages in thread From: Simon Horman @ 2008-07-28 2:45 UTC (permalink / raw) To: Andrew Morton Cc: Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Vivek Goyal The rfc patch I made to rename is_kdump_kernel to kernel_has_vmcore, which I appended to my previous post should have looked more like the following. Although, as I noted in my previous post, its more a starting point for discussion than a solution to the problem at hand. Index: linux-2.6/arch/x86/kernel/pci-calgary_64.c =================================================================== --- linux-2.6.orig/arch/x86/kernel/pci-calgary_64.c 2008-07-28 10:10:31.000000000 +1000 +++ linux-2.6/arch/x86/kernel/pci-calgary_64.c 2008-07-28 10:14:24.000000000 +1000 @@ -801,7 +801,7 @@ static int __init calgary_setup_tar(stru tbl = pci_iommu(dev->bus); tbl->it_base = (unsigned long)bus_info[dev->bus->number].tce_space; - if (is_kdump_kernel()) + if (kernel_has_vmcore()) calgary_init_bitmap_from_tce_table(tbl); else tce_free(tbl, 0, tbl->it_size); @@ -1184,7 +1184,7 @@ static int __init calgary_init(void) return ret; /* Purely for kdump kernel case */ - if (is_kdump_kernel()) + if (kernel_has_vmcore()) get_tce_space_from_tar(); do { @@ -1438,7 +1438,7 @@ void __init detect_calgary(void) return; } - specified_table_size = determine_tce_table_size((is_kdump_kernel() ? + specified_table_size = determine_tce_table_size((kernel_has_vmcore() ? saved_max_pfn : max_pfn) * PAGE_SIZE); for (bus = 0; bus < MAX_PHB_BUS_NUM; bus++) { @@ -1461,7 +1461,7 @@ void __init detect_calgary(void) * If it is kdump kernel, find and use tce tables * from first kernel, else allocate tce tables here */ - if (!is_kdump_kernel()) { + if (!kernel_has_vmcore()) { tbl = alloc_tce_table(); if (!tbl) goto cleanup; Index: linux-2.6/fs/proc/vmcore.c =================================================================== --- linux-2.6.orig/fs/proc/vmcore.c 2008-07-28 09:51:30.000000000 +1000 +++ linux-2.6/fs/proc/vmcore.c 2008-07-28 09:51:53.000000000 +1000 @@ -32,8 +32,7 @@ static size_t elfcorebuf_sz; /* Total size of vmcore file. */ static u64 vmcore_size; -/* Stores the physical address of elf header of crash image. */ -unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; +unsigned long long elfcorehdr_addr; struct proc_dir_entry *proc_vmcore = NULL; Index: linux-2.6/include/linux/crash_dump.h =================================================================== --- linux-2.6.orig/include/linux/crash_dump.h 2008-07-28 09:46:29.000000000 +1000 +++ linux-2.6/include/linux/crash_dump.h 2008-07-28 10:43:03.000000000 +1000 @@ -11,8 +11,13 @@ #ifdef CONFIG_PROC_VMCORE extern unsigned long long elfcorehdr_addr; + +static inline int kernel_has_vmcore(void) +{ + return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; +} #else -static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; +static inline int kernel_has_vmcore(void) { return 0; } #endif extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, @@ -28,12 +33,6 @@ extern struct proc_dir_entry *proc_vmcor #define vmcore_elf_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) -static inline int is_kdump_kernel(void) -{ - return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; -} -#else /* !CONFIG_CRASH_DUMP */ -static inline int is_kdump_kernel(void) { return 0; } #endif /* CONFIG_CRASH_DUMP */ extern unsigned long saved_max_pfn; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 2:45 ` Simon Horman @ 2008-07-28 3:40 ` Simon Horman 2008-07-28 12:48 ` Ingo Molnar 2008-07-28 21:10 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Vivek Goyal 0 siblings, 2 replies; 28+ messages in thread From: Simon Horman @ 2008-07-28 3:40 UTC (permalink / raw) To: Andrew Morton Cc: Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Vivek Goyal On Mon, Jul 28, 2008 at 12:45:28PM +1000, Simon Horman wrote: > The rfc patch I made to rename is_kdump_kernel to kernel_has_vmcore, > which I appended to my previous post should have looked more like the > following. Although, as I noted in my previous post, its more a starting > point for discussion than a solution to the problem at hand. Sorry, one more time. I forgot to quilt refresh. Index: linux-2.6/arch/x86/kernel/pci-calgary_64.c =================================================================== --- linux-2.6.orig/arch/x86/kernel/pci-calgary_64.c 2008-07-28 10:10:31.000000000 +1000 +++ linux-2.6/arch/x86/kernel/pci-calgary_64.c 2008-07-28 10:14:24.000000000 +1000 @@ -801,7 +801,7 @@ static int __init calgary_setup_tar(stru tbl = pci_iommu(dev->bus); tbl->it_base = (unsigned long)bus_info[dev->bus->number].tce_space; - if (is_kdump_kernel()) + if (kernel_has_vmcore()) calgary_init_bitmap_from_tce_table(tbl); else tce_free(tbl, 0, tbl->it_size); @@ -1184,7 +1184,7 @@ static int __init calgary_init(void) return ret; /* Purely for kdump kernel case */ - if (is_kdump_kernel()) + if (kernel_has_vmcore()) get_tce_space_from_tar(); do { @@ -1438,7 +1438,7 @@ void __init detect_calgary(void) return; } - specified_table_size = determine_tce_table_size((is_kdump_kernel() ? + specified_table_size = determine_tce_table_size((kernel_has_vmcore() ? saved_max_pfn : max_pfn) * PAGE_SIZE); for (bus = 0; bus < MAX_PHB_BUS_NUM; bus++) { @@ -1461,7 +1461,7 @@ void __init detect_calgary(void) * If it is kdump kernel, find and use tce tables * from first kernel, else allocate tce tables here */ - if (!is_kdump_kernel()) { + if (!kernel_has_vmcore()) { tbl = alloc_tce_table(); if (!tbl) goto cleanup; Index: linux-2.6/fs/proc/vmcore.c =================================================================== --- linux-2.6.orig/fs/proc/vmcore.c 2008-07-28 09:51:30.000000000 +1000 +++ linux-2.6/fs/proc/vmcore.c 2008-07-28 09:51:53.000000000 +1000 @@ -32,8 +32,7 @@ static size_t elfcorebuf_sz; /* Total size of vmcore file. */ static u64 vmcore_size; -/* Stores the physical address of elf header of crash image. */ -unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; +unsigned long long elfcorehdr_addr; struct proc_dir_entry *proc_vmcore = NULL; Index: linux-2.6/include/linux/crash_dump.h =================================================================== --- linux-2.6.orig/include/linux/crash_dump.h 2008-07-28 09:46:29.000000000 +1000 +++ linux-2.6/include/linux/crash_dump.h 2008-07-28 12:11:57.000000000 +1000 @@ -9,12 +9,6 @@ #define ELFCORE_ADDR_MAX (-1ULL) -#ifdef CONFIG_PROC_VMCORE -extern unsigned long long elfcorehdr_addr; -#else -static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; -#endif - extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, unsigned long, int); extern const struct file_operations proc_vmcore_operations; @@ -28,13 +22,18 @@ extern struct proc_dir_entry *proc_vmcor #define vmcore_elf_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) -static inline int is_kdump_kernel(void) -{ - return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; -} -#else /* !CONFIG_CRASH_DUMP */ -static inline int is_kdump_kernel(void) { return 0; } #endif /* CONFIG_CRASH_DUMP */ extern unsigned long saved_max_pfn; #endif /* LINUX_CRASHDUMP_H */ + +#ifdef CONFIG_PROC_VMCORE +extern unsigned long long elfcorehdr_addr; + +static inline int kernel_has_vmcore(void) +{ + return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; +} +#else +static inline int kernel_has_vmcore(void) { return 0; } +#endif Index: linux-2.6/arch/ia64/hp/common/sba_iommu.c =================================================================== --- linux-2.6.orig/arch/ia64/hp/common/sba_iommu.c 2008-07-28 11:51:36.000000000 +1000 +++ linux-2.6/arch/ia64/hp/common/sba_iommu.c 2008-07-28 12:11:27.000000000 +1000 @@ -2070,14 +2070,13 @@ sba_init(void) if (!ia64_platform_is("hpzx1") && !ia64_platform_is("hpzx1_swiotlb")) return 0; -#if defined(CONFIG_IA64_GENERIC) && defined(CONFIG_PROC_VMCORE) && \ - defined(CONFIG_PROC_FS) +#if defined(CONFIG_IA64_GENERIC) /* If we are booting a kdump kernel, the sba_iommu will * cause devices that were not shutdown properly to MCA * as soon as they are turned back on. Our only option for * a successful kdump kernel boot is to use the swiotlb. */ - if (elfcorehdr_addr < ELFCORE_ADDR_MAX) { + if (kernel_has_vmcore()) { if (swiotlb_late_init_with_default_size(64 * (1<<20)) != 0) panic("Unable to initialize software I/O TLB:" " Try machvec=dig boot option"); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 3:40 ` Simon Horman @ 2008-07-28 12:48 ` Ingo Molnar 2008-07-29 0:35 ` Simon Horman 2008-07-28 21:10 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Vivek Goyal 1 sibling, 1 reply; 28+ messages in thread From: Ingo Molnar @ 2008-07-28 12:48 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Linus Torvalds, Vivek Goyal * Simon Horman <horms@verge.net.au> wrote: > On Mon, Jul 28, 2008 at 12:45:28PM +1000, Simon Horman wrote: > > The rfc patch I made to rename is_kdump_kernel to kernel_has_vmcore, > > which I appended to my previous post should have looked more like the > > following. Although, as I noted in my previous post, its more a starting > > point for discussion than a solution to the problem at hand. > > Sorry, one more time. I forgot to quilt refresh. doesnt apply cleanly to latest -git: Hunk #1 FAILED at 2070. 1 out of 1 hunk FAILED -- saving rejects to file arch/ia64/hp/common/sba_iommu.c.rej due to a crossing change i guess. Also, i guess this should go via -mm as it touches fs/proc/vmcore.c and include/linux/crash_dump.h. The x86 bits look good to me. Acked-by: Ingo Molnar <mingo@elte.hu> Ingo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 12:48 ` Ingo Molnar @ 2008-07-29 0:35 ` Simon Horman 0 siblings, 0 replies; 28+ messages in thread From: Simon Horman @ 2008-07-29 0:35 UTC (permalink / raw) To: Ingo Molnar Cc: Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Andrew Morton, Linus Torvalds, Vivek Goyal On Mon, Jul 28, 2008 at 02:48:56PM +0200, Ingo Molnar wrote: > > * Simon Horman <horms@verge.net.au> wrote: > > > On Mon, Jul 28, 2008 at 12:45:28PM +1000, Simon Horman wrote: > > > The rfc patch I made to rename is_kdump_kernel to kernel_has_vmcore, > > > which I appended to my previous post should have looked more like the > > > following. Although, as I noted in my previous post, its more a starting > > > point for discussion than a solution to the problem at hand. > > > > Sorry, one more time. I forgot to quilt refresh. > > doesnt apply cleanly to latest -git: > > Hunk #1 FAILED at 2070. > 1 out of 1 hunk FAILED -- saving rejects to file arch/ia64/hp/common/sba_iommu.c.rej > > due to a crossing change i guess. Also, i guess this should go via -mm > as it touches fs/proc/vmcore.c and include/linux/crash_dump.h. The x86 > bits look good to me. > > Acked-by: Ingo Molnar <mingo@elte.hu> Sorry about the messiness there. Vivek has a different patch set that renders most of this irrelevant. I'll fix up the arch/ia64/hp/common/sba_iommu.c portion to match his changes so that he can include it in his sereis / it can be included after his series. -- Horms ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') 2008-07-28 3:40 ` Simon Horman 2008-07-28 12:48 ` Ingo Molnar @ 2008-07-28 21:10 ` Vivek Goyal 2008-07-28 21:11 ` [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section Vivek Goyal ` (2 more replies) 1 sibling, 3 replies; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 21:10 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev Hi All, How does following series of patches look like. I have moved elfcorehdr_addr out of vmcore.c and pushed it to arch dependent section of crash dump to make sure that it can be worked with even when CONFIG_PROC_VMCORE is disabled and CONFIG_CRASH_DUMP is enabled. I tested it on x86_64. Compile tested it on i386 and ppc64. ia64 and sh versions are completely untested. Thanks Vivek o elfcorehdr_addr is used by not only the code under CONFIG_PROC_VMCORE but also by the code which is not inside CONFIG_PROC_VMCORE. For example, is_kdump_kernel() is used by powerpc code to determine if kernel is booting after a panic then use previous kernel's TCE table. So even if CONFIG_PROC_VMCORE is not set in second kernel, one should be able to correctly determine that we are booting after a panic and setup calgary iommu accordingly. o So remove the assumption that elfcorehdr_addr is under CONFIG_PROC_VMCORE. o Move definition of elfcorehdr_addr to arch dependent crash files. (Unfortunately crash dump does not have an arch independent file otherwise that would have been the best place). o kexec.c is not the right place as one can Have CRASH_DUMP enabled in second kernel without KEXEC being enabled. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/proc/vmcore.c | 3 --- include/linux/crash_dump.h | 14 ++++++++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff -puN fs/proc/vmcore.c~remove-elfcore-hdr-addr-definition-vmcore fs/proc/vmcore.c --- linux-2.6.27-pre-rc1/fs/proc/vmcore.c~remove-elfcore-hdr-addr-definition-vmcore 2008-07-28 09:19:50.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/fs/proc/vmcore.c 2008-07-28 09:20:10.000000000 -0400 @@ -32,9 +32,6 @@ static size_t elfcorebuf_sz; /* Total size of vmcore file. */ static u64 vmcore_size; -/* Stores the physical address of elf header of crash image. */ -unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; - struct proc_dir_entry *proc_vmcore = NULL; /* Reads a page from the oldmem device from given offset. */ diff -puN include/linux/crash_dump.h~remove-elfcore-hdr-addr-definition-vmcore include/linux/crash_dump.h --- linux-2.6.27-pre-rc1/include/linux/crash_dump.h~remove-elfcore-hdr-addr-definition-vmcore 2008-07-28 12:00:44.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/include/linux/crash_dump.h 2008-07-28 12:00:56.000000000 -0400 @@ -9,11 +9,7 @@ #define ELFCORE_ADDR_MAX (-1ULL) -#ifdef CONFIG_PROC_VMCORE extern unsigned long long elfcorehdr_addr; -#else -static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; -#endif extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, unsigned long, int); @@ -28,6 +24,16 @@ extern struct proc_dir_entry *proc_vmcor #define vmcore_elf_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) +/* + * is_kdump_kernel() checks whether this kernel is booting after a panic of + * previous kernel or not. This is determined by checking if previous kernel + * has passed the elf core header address on command line. + * + * This is not just a test if CONFIG_CRASH_DUMP is enabled or not. It will + * return 1 if CONFIG_CRASH_DUMP=y and if kernel is booting after a panic of + * previous kernel. + */ + static inline int is_kdump_kernel(void) { return (elfcorehdr_addr != ELFCORE_ADDR_MAX) ? 1 : 0; _ ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:10 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Vivek Goyal @ 2008-07-28 21:11 ` Vivek Goyal 2008-07-28 21:13 ` [PATCH 3/5] ia64: " Vivek Goyal 2008-07-31 15:29 ` [PATCH 2/5] x86: " Ingo Molnar 2008-07-28 22:37 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Eric W. Biederman 2008-07-28 22:47 ` Eric W. Biederman 2 siblings, 2 replies; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 21:11 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev o Move elfcorehdr_addr definition in arch dependent crash dump file. This is equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or not. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- arch/x86/kernel/crash_dump_32.c | 3 +++ arch/x86/kernel/crash_dump_64.c | 3 +++ arch/x86/kernel/setup.c | 8 +++++++- 3 files changed, 13 insertions(+), 1 deletion(-) diff -puN arch/x86/kernel/setup.c~fix-elfcorehdr_addr-parsing-x86 arch/x86/kernel/setup.c --- linux-2.6.27-pre-rc1/arch/x86/kernel/setup.c~fix-elfcorehdr_addr-parsing-x86 2008-07-28 12:01:35.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/arch/x86/kernel/setup.c 2008-07-28 12:01:35.000000000 -0400 @@ -558,7 +558,13 @@ static void __init reserve_standard_io_r } -#ifdef CONFIG_PROC_VMCORE +/* + * 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. diff -puN arch/x86/kernel/crash_dump_32.c~fix-elfcorehdr_addr-parsing-x86 arch/x86/kernel/crash_dump_32.c --- linux-2.6.27-pre-rc1/arch/x86/kernel/crash_dump_32.c~fix-elfcorehdr_addr-parsing-x86 2008-07-28 12:01:35.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/arch/x86/kernel/crash_dump_32.c 2008-07-28 12:01:35.000000000 -0400 @@ -13,6 +13,9 @@ static void *kdump_buf_page; +/* 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 "oldmem" * @pfn: page frame number to be copied diff -puN arch/x86/kernel/crash_dump_64.c~fix-elfcorehdr_addr-parsing-x86 arch/x86/kernel/crash_dump_64.c --- linux-2.6.27-pre-rc1/arch/x86/kernel/crash_dump_64.c~fix-elfcorehdr_addr-parsing-x86 2008-07-28 12:01:35.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/arch/x86/kernel/crash_dump_64.c 2008-07-28 12:01:35.000000000 -0400 @@ -11,6 +11,9 @@ #include <asm/uaccess.h> #include <asm/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 "oldmem" * @pfn: page frame number to be copied _ ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/5] ia64: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:11 ` [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section Vivek Goyal @ 2008-07-28 21:13 ` Vivek Goyal 2008-07-28 21:14 ` [PATCH 4/5] powerpc: " Vivek Goyal 2008-07-29 4:42 ` [PATCH 3/5] ia64: " Simon Horman 2008-07-31 15:29 ` [PATCH 2/5] x86: " Ingo Molnar 1 sibling, 2 replies; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 21:13 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev o Move elfcorehdr_addr definition in arch dependent crash dump file. This is equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or not. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- arch/ia64/kernel/setup.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff -puN arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 arch/ia64/kernel/setup.c --- linux-2.6.27-pre-rc1/arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 2008-07-28 12:16:40.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/arch/ia64/kernel/setup.c 2008-07-28 12:16:40.000000000 -0400 @@ -478,7 +478,12 @@ static __init int setup_nomca(char *s) } early_param("nomca", setup_nomca); -#ifdef CONFIG_PROC_VMCORE +/* + * 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. */ @@ -491,7 +496,9 @@ static int __init parse_elfcorehdr(char return 0; } early_param("elfcorehdr", parse_elfcorehdr); +#endif +#ifdef CONFIG_PROC_VMCORE int __init reserve_elfcorehdr(unsigned long *start, unsigned long *end) { unsigned long length; _ ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/5] powerpc: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:13 ` [PATCH 3/5] ia64: " Vivek Goyal @ 2008-07-28 21:14 ` Vivek Goyal 2008-07-28 21:15 ` [PATCH 5/5] sh: " Vivek Goyal 2008-07-29 4:42 ` [PATCH 3/5] ia64: " Simon Horman 1 sibling, 1 reply; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 21:14 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev o Move elfcorehdr_addr definition in arch dependent crash dump file. This is equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or not. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- arch/powerpc/kernel/crash_dump.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff -puN arch/powerpc/kernel/crash_dump.c~fix-elfcorehdr_addr-parsing-ppc64 arch/powerpc/kernel/crash_dump.c --- linux-2.6.27-pre-rc1/arch/powerpc/kernel/crash_dump.c~fix-elfcorehdr_addr-parsing-ppc64 2008-07-28 12:14:22.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/arch/powerpc/kernel/crash_dump.c 2008-07-28 12:14:22.000000000 -0400 @@ -27,6 +27,9 @@ #define DBG(fmt...) #endif +/* Stores the physical address of elf header of crash image. */ +unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; + void __init reserve_kdump_trampoline(void) { lmb_reserve(0, KDUMP_RESERVE_LIMIT); @@ -66,7 +69,11 @@ void __init setup_kdump_trampoline(void) DBG(" <- setup_kdump_trampoline()\n"); } -#ifdef CONFIG_PROC_VMCORE +/* + * 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. + */ static int __init parse_elfcorehdr(char *p) { if (p) @@ -75,7 +82,6 @@ static int __init parse_elfcorehdr(char return 1; } __setup("elfcorehdr=", parse_elfcorehdr); -#endif static int __init parse_savemaxmem(char *p) { _ ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/5] sh: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:14 ` [PATCH 4/5] powerpc: " Vivek Goyal @ 2008-07-28 21:15 ` Vivek Goyal 2008-07-29 14:18 ` Paul Mundt 0 siblings, 1 reply; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 21:15 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev o Move elfcorehdr_addr definition in arch dependent crash dump file. This is equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or not. o I don't see sh setup code parsing the command line for elfcorehdr_addr. I am wondering how does vmcore interface work on sh. Anyway, I am atleast defining elfcoredhr_addr so that compilation is not broken on sh. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- arch/sh/kernel/crash_dump.c | 3 +++ 1 file changed, 3 insertions(+) diff -puN arch/sh/kernel/crash_dump.c~fix-elfcorehdr_addr-sh arch/sh/kernel/crash_dump.c --- linux-2.6.27-pre-rc1/arch/sh/kernel/crash_dump.c~fix-elfcorehdr_addr-sh 2008-07-28 12:17:12.000000000 -0400 +++ linux-2.6.27-pre-rc1-root/arch/sh/kernel/crash_dump.c 2008-07-28 12:17:12.000000000 -0400 @@ -10,6 +10,9 @@ #include <linux/io.h> #include <asm/uaccess.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 "oldmem" * @pfn: page frame number to be copied _ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] sh: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:15 ` [PATCH 5/5] sh: " Vivek Goyal @ 2008-07-29 14:18 ` Paul Mundt 0 siblings, 0 replies; 28+ messages in thread From: Paul Mundt @ 2008-07-29 14:18 UTC (permalink / raw) To: Vivek Goyal Cc: Simon Horman, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mackerras, linuxppc-dev On Mon, Jul 28, 2008 at 05:15:14PM -0400, Vivek Goyal wrote: > o Move elfcorehdr_addr definition in arch dependent crash dump file. This is > equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of > CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be > used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or > not. > > o I don't see sh setup code parsing the command line for elfcorehdr_addr. I > am wondering how does vmcore interface work on sh. Anyway, I am atleast > defining elfcoredhr_addr so that compilation is not broken on sh. > Hmm, you are correct, it seems like it was either lost in a merge somewhere or I simply neglected to check it in it when I was testing this stuff initially. Thanks for noticing! > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> Acked-by: Paul Mundt <lethal@linux-sh.org> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] ia64: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:13 ` [PATCH 3/5] ia64: " Vivek Goyal 2008-07-28 21:14 ` [PATCH 4/5] powerpc: " Vivek Goyal @ 2008-07-29 4:42 ` Simon Horman 2008-07-29 13:53 ` Vivek Goyal 1 sibling, 1 reply; 28+ messages in thread From: Simon Horman @ 2008-07-29 4:42 UTC (permalink / raw) To: Vivek Goyal Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev On Mon, Jul 28, 2008 at 05:13:14PM -0400, Vivek Goyal wrote: > > o Move elfcorehdr_addr definition in arch dependent crash dump file. This is > equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of > CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be > used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or > not. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > > arch/ia64/kernel/setup.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff -puN arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 arch/ia64/kernel/setup.c > --- linux-2.6.27-pre-rc1/arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 2008-07-28 12:16:40.000000000 -0400 > +++ linux-2.6.27-pre-rc1-root/arch/ia64/kernel/setup.c 2008-07-28 12:16:40.000000000 -0400 > @@ -478,7 +478,12 @@ static __init int setup_nomca(char *s) > } > early_param("nomca", setup_nomca); > > -#ifdef CONFIG_PROC_VMCORE > +/* > + * 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. > */ > @@ -491,7 +496,9 @@ static int __init parse_elfcorehdr(char > return 0; > } > early_param("elfcorehdr", parse_elfcorehdr); > +#endif > > +#ifdef CONFIG_PROC_VMCORE > int __init reserve_elfcorehdr(unsigned long *start, unsigned long *end) > { > unsigned long length; > _ Hi Vivek, I think that you also need the following in arch/ia64/kernel/crash_dump.c. With this change your code compiles on ia64. Signed-off-by: Simon Horman <horms@verge.net.au> Index: linux-2.6/arch/ia64/kernel/crash_dump.c =================================================================== --- linux-2.6.orig/arch/ia64/kernel/crash_dump.c 2008-07-29 14:06:57.000000000 +1000 +++ linux-2.6/arch/ia64/kernel/crash_dump.c 2008-07-29 14:09:55.000000000 +1000 @@ -8,10 +8,14 @@ #include <linux/errno.h> #include <linux/types.h> +#include <linux/crash_dump.h> #include <asm/page.h> #include <asm/uaccess.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 "oldmem" * @pfn: page frame number to be copied ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] ia64: Define elfcorehdr_addr in arch dependent section 2008-07-29 4:42 ` [PATCH 3/5] ia64: " Simon Horman @ 2008-07-29 13:53 ` Vivek Goyal 0 siblings, 0 replies; 28+ messages in thread From: Vivek Goyal @ 2008-07-29 13:53 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev On Tue, Jul 29, 2008 at 02:42:43PM +1000, Simon Horman wrote: > On Mon, Jul 28, 2008 at 05:13:14PM -0400, Vivek Goyal wrote: > > > > o Move elfcorehdr_addr definition in arch dependent crash dump file. This is > > equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of > > CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be > > used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or > > not. > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > > > arch/ia64/kernel/setup.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff -puN arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 arch/ia64/kernel/setup.c > > --- linux-2.6.27-pre-rc1/arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 2008-07-28 12:16:40.000000000 -0400 > > +++ linux-2.6.27-pre-rc1-root/arch/ia64/kernel/setup.c 2008-07-28 12:16:40.000000000 -0400 > > @@ -478,7 +478,12 @@ static __init int setup_nomca(char *s) > > } > > early_param("nomca", setup_nomca); > > > > -#ifdef CONFIG_PROC_VMCORE > > +/* > > + * 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. > > */ > > @@ -491,7 +496,9 @@ static int __init parse_elfcorehdr(char > > return 0; > > } > > early_param("elfcorehdr", parse_elfcorehdr); > > +#endif > > > > +#ifdef CONFIG_PROC_VMCORE > > int __init reserve_elfcorehdr(unsigned long *start, unsigned long *end) > > { > > unsigned long length; > > _ > > Hi Vivek, > > I think that you also need the following in arch/ia64/kernel/crash_dump.c. > With this change your code compiles on ia64. > > Signed-off-by: Simon Horman <horms@verge.net.au> > > Thanks Simon. I had done these changes locally but somehow forgot to include the changes in patches. I will include these changes in my next posting of consolidated single patch. Thanks Vivek ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section 2008-07-28 21:11 ` [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section Vivek Goyal 2008-07-28 21:13 ` [PATCH 3/5] ia64: " Vivek Goyal @ 2008-07-31 15:29 ` Ingo Molnar 1 sibling, 0 replies; 28+ messages in thread From: Ingo Molnar @ 2008-07-31 15:29 UTC (permalink / raw) To: Vivek Goyal Cc: Simon Horman, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev * Vivek Goyal <vgoyal@redhat.com> wrote: > o Move elfcorehdr_addr definition in arch dependent crash dump file. This is > equivalent to defining elfcorehdr_addr under CONFIG_CRASH_DUMP instead of > CONFIG_PROC_VMCORE. This is needed by is_kdump_kernel() which can be > used irrespective of the fact whether CONFIG_PROC_VMCORE is enabled or > not. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > > arch/x86/kernel/crash_dump_32.c | 3 +++ > arch/x86/kernel/crash_dump_64.c | 3 +++ > arch/x86/kernel/setup.c | 8 +++++++- > 3 files changed, 13 insertions(+), 1 deletion(-) the x86 bits look fine to me. Acked-by: Ingo Molnar <mingo@elte.hu> Ingo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') 2008-07-28 21:10 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Vivek Goyal 2008-07-28 21:11 ` [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section Vivek Goyal @ 2008-07-28 22:37 ` Eric W. Biederman 2008-07-28 22:47 ` Eric W. Biederman 2 siblings, 0 replies; 28+ messages in thread From: Eric W. Biederman @ 2008-07-28 22:37 UTC (permalink / raw) To: Vivek Goyal Cc: Simon Horman, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Eric W. Biederman, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') 2008-07-28 21:10 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Vivek Goyal 2008-07-28 21:11 ` [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section Vivek Goyal 2008-07-28 22:37 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Eric W. Biederman @ 2008-07-28 22:47 ` Eric W. Biederman 2008-07-29 1:22 ` Simon Horman 2 siblings, 1 reply; 28+ messages in thread From: Eric W. Biederman @ 2008-07-28 22:47 UTC (permalink / raw) To: Vivek Goyal Cc: Simon Horman, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev Vivek Goyal <vgoyal@redhat.com> writes: > Hi All, > > How does following series of patches look like. I have moved > elfcorehdr_addr out of vmcore.c and pushed it to arch dependent section > of crash dump to make sure that it can be worked with even when > CONFIG_PROC_VMCORE is disabled and CONFIG_CRASH_DUMP is enabled. > > I tested it on x86_64. Compile tested it on i386 and ppc64. ia64 and > sh versions are completely untested. Given the current state of the code: Acked-by: "Eric W. Biederman" <ebiederm@xmission.com> To process a kernel crash dump we pass the kernel elfcorehdr option, so testing to see if it was passed seems reasonable. In general I think this method of handling the problems with kdump is too brittle to live, but in the case of iommus we certainly need to do something different, and unfortunately iommus were not common on x86 when the original code was merged so we have not handled them well. Eric ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') 2008-07-28 22:47 ` Eric W. Biederman @ 2008-07-29 1:22 ` Simon Horman 2008-07-29 2:28 ` Vivek Goyal 0 siblings, 1 reply; 28+ messages in thread From: Simon Horman @ 2008-07-29 1:22 UTC (permalink / raw) To: Eric W. Biederman Cc: Vivek Goyal, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev On Mon, Jul 28, 2008 at 03:47:41PM -0700, Eric W. Biederman wrote: > Vivek Goyal <vgoyal@redhat.com> writes: > > > Hi All, > > > > How does following series of patches look like. I have moved > > elfcorehdr_addr out of vmcore.c and pushed it to arch dependent section > > of crash dump to make sure that it can be worked with even when > > CONFIG_PROC_VMCORE is disabled and CONFIG_CRASH_DUMP is enabled. > > > > I tested it on x86_64. Compile tested it on i386 and ppc64. ia64 and > > sh versions are completely untested. > > Given the current state of the code: > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > To process a kernel crash dump we pass the kernel elfcorehdr option, > so testing to see if it was passed seems reasonable. > > In general I think this method of handling the problems with kdump is > too brittle to live, but in the case of iommus we certainly need to do > something different, and unfortunately iommus were not common on x86 > when the original code was merged so we have not handled them well. Agreed, however these patches look like they really ought to be merged into a single patch for the sake of bisect. As things stand, applying the first patch will break the build on each architecture with an architecture specific until the latter is applied. -- Horms ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') 2008-07-29 1:22 ` Simon Horman @ 2008-07-29 2:28 ` Vivek Goyal 2008-07-29 3:26 ` Simon Horman 0 siblings, 1 reply; 28+ messages in thread From: Vivek Goyal @ 2008-07-29 2:28 UTC (permalink / raw) To: Simon Horman Cc: Eric W. Biederman, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev On Tue, Jul 29, 2008 at 11:22:48AM +1000, Simon Horman wrote: > On Mon, Jul 28, 2008 at 03:47:41PM -0700, Eric W. Biederman wrote: > > Vivek Goyal <vgoyal@redhat.com> writes: > > > > > Hi All, > > > > > > How does following series of patches look like. I have moved > > > elfcorehdr_addr out of vmcore.c and pushed it to arch dependent section > > > of crash dump to make sure that it can be worked with even when > > > CONFIG_PROC_VMCORE is disabled and CONFIG_CRASH_DUMP is enabled. > > > > > > I tested it on x86_64. Compile tested it on i386 and ppc64. ia64 and > > > sh versions are completely untested. > > > > Given the current state of the code: > > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > > > To process a kernel crash dump we pass the kernel elfcorehdr option, > > so testing to see if it was passed seems reasonable. > > > > In general I think this method of handling the problems with kdump is > > too brittle to live, but in the case of iommus we certainly need to do > > something different, and unfortunately iommus were not common on x86 > > when the original code was merged so we have not handled them well. > > Agreed, however these patches look like they really ought to be merged > into a single patch for the sake of bisect. As things stand, applying > the first patch will break the build on each architecture with an > architecture specific until the latter is applied. That's a good point. I was not very sure because changes were in different arches and I broke the patch. At the same time changes are really miniscule in each arch. So, for the sake of not breaking compilation for git-bisect, I will generate a single patch tomorrow. (Until and unless somebody has an objection). Thanks Vivek > > -- > Horms ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') 2008-07-29 2:28 ` Vivek Goyal @ 2008-07-29 3:26 ` Simon Horman 0 siblings, 0 replies; 28+ messages in thread From: Simon Horman @ 2008-07-29 3:26 UTC (permalink / raw) To: Vivek Goyal Cc: Eric W. Biederman, Andrew Morton, Muli Ben-Yehuda, Tony Luck, linux-ia64, Chandru, kexec, linux-kernel, Terry Loftin, Ingo Molnar, Linus Torvalds, Paul Mundt, Paul Mackerras, linuxppc-dev On Mon, Jul 28, 2008 at 10:28:22PM -0400, Vivek Goyal wrote: > On Tue, Jul 29, 2008 at 11:22:48AM +1000, Simon Horman wrote: > > On Mon, Jul 28, 2008 at 03:47:41PM -0700, Eric W. Biederman wrote: > > > Vivek Goyal <vgoyal@redhat.com> writes: > > > > > > > Hi All, > > > > > > > > How does following series of patches look like. I have moved > > > > elfcorehdr_addr out of vmcore.c and pushed it to arch dependent section > > > > of crash dump to make sure that it can be worked with even when > > > > CONFIG_PROC_VMCORE is disabled and CONFIG_CRASH_DUMP is enabled. > > > > > > > > I tested it on x86_64. Compile tested it on i386 and ppc64. ia64 and > > > > sh versions are completely untested. > > > > > > Given the current state of the code: > > > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > > > > > To process a kernel crash dump we pass the kernel elfcorehdr option, > > > so testing to see if it was passed seems reasonable. > > > > > > In general I think this method of handling the problems with kdump is > > > too brittle to live, but in the case of iommus we certainly need to do > > > something different, and unfortunately iommus were not common on x86 > > > when the original code was merged so we have not handled them well. > > > > Agreed, however these patches look like they really ought to be merged > > into a single patch for the sake of bisect. As things stand, applying > > the first patch will break the build on each architecture with an > > architecture specific until the latter is applied. > > That's a good point. I was not very sure because changes were in > different arches and I broke the patch. At the same time changes are > really miniscule in each arch. I guessed that was why you split them up. But really the per-arch change is very small. > So, for the sake of not breaking compilation for git-bisect, I will > generate a single patch tomorrow. (Until and unless somebody has an > objection). For combiled patch: Acked-by: Simon Horman <horms@verge.net.au> -- Horms ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 1:51 ` Simon Horman 2008-07-28 2:45 ` Simon Horman @ 2008-07-28 5:39 ` Eric W. Biederman 2008-07-28 6:24 ` Muli Ben-Yehuda 2008-07-28 13:31 ` Vivek Goyal 2 siblings, 1 reply; 28+ messages in thread From: Eric W. Biederman @ 2008-07-28 5:39 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Chandru, kexec, linux-kernel, Vivek Goyal, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, Eric W. Biederman, linux-ia64 Simon Horman <horms@verge.net.au> writes: > > Hi, > > I started looking into a simple fix to change the name of > the is_kdump_kernel() to kernel_has_vmcore(), which is what > the code in its current incarnatation does. > > This also lead to cleaning the usage of elfcorehdr_addr, > which is in the folloing messy state after recent changes. > > #ifdef CONFIG_PROC_VMCORE > * Declared non-static include/linux/crash_dump.h > * Initialised in fs/proc/vmcore.c > #else > * Declared and initialised as static in include/linux/crash_dump.h > * Only used by is_kdump_kernel() which is a static function > also in include/linux/crash_dump.h > #endif > > > Howerver, in the course of doing this I came to thinking that actually > this code won't solve the problem at hand in the case where > CONFIG_CRASH_DUMP is defined but CONFIG_PROC_VMCORE is not. > Or in other words, what happens if the calgary initialisation code > runs in a kdump kernel that does not have CONFIG_PROC_VMCORE ? > > A similar problem appears to exist in > arch/ia64/hp/common/sba_iommu.c:sba_init(), which currently doesn't > compile if CONFIG_CRASH_DUMP is set but CONFIG_PROC_VMCORE is not. The > compilation issue could be solved by using kernel_has_vmcore() (as per > the patch below) instead of checking elfcorehdr_addr directly, but does > it actually lead to working code? > > There has long been a strong aversion to providing the second > kernel with flags like im_in_kexec or im_in_kdump, as its felt > that this kind of problem is better handled by making sure that the > hardware is in a sensible state before leaving the first-kernel. > But this is arguably more reasonable in the kexec case than the > kdump case. That and because we can generally solve the specific problem with a general feature. Something we can enable/disable on the command line if needed. Right now this is especially interesting as on several architectures distros are not building special kdump kernels but have a single kernel binary that works in both cases. Skimming through your patches this is a case we really do need to implement and handle cleanly. Currently we leave DMA running in the kexec on panic case. We avoid problems by only running out of a reserved area of memory. As as general strategy that is fine. However we have not implemented that strategy in the case of IOMMUs. And we are having trouble with IOMMUs. My hunch is that we should implement options to reserve a section of the iommu and to tell to the iommu to use the previously reserved section. Although turning iommus off altogether and simply using swiotlb may be acceptable. In which case we should just force usage of the swiotlb on the command line in /sbin/kexec. Eric ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 5:39 ` [patch] crashdump: fix undefined reference to `elfcorehdr_addr' Eric W. Biederman @ 2008-07-28 6:24 ` Muli Ben-Yehuda 2008-07-28 13:44 ` Vivek Goyal 0 siblings, 1 reply; 28+ messages in thread From: Muli Ben-Yehuda @ 2008-07-28 6:24 UTC (permalink / raw) To: Eric W. Biederman Cc: Simon Horman, Andrew Morton, Chandru, kexec, linux-kernel, Vivek Goyal, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, linux-ia64 On Sun, Jul 27, 2008 at 10:39:30PM -0700, Eric W. Biederman wrote: > Simon Horman <horms@verge.net.au> writes: > > > There has long been a strong aversion to providing the second > > kernel with flags like im_in_kexec or im_in_kdump, as its felt > > that this kind of problem is better handled by making sure that > > the hardware is in a sensible state before leaving the > > first-kernel. But this is arguably more reasonable in the kexec > > case than the kdump case. > > That and because we can generally solve the specific problem with a > general feature. Something we can enable/disable on the command > line if needed. Right now this is especially interesting as on > several architectures distros are not building special kdump kernels > but have a single kernel binary that works in both cases. > > Skimming through your patches this is a case we really do need to > implement and handle cleanly. > > Currently we leave DMA running in the kexec on panic case. We avoid > problems by only running out of a reserved area of memory. > > As as general strategy that is fine. However we have not > implemented that strategy in the case of IOMMUs. And we are having > trouble with IOMMUs. > > My hunch is that we should implement options to reserve a section of > the iommu and to tell to the iommu to use the previously reserved > section. Although turning iommus off altogether and simply using > swiotlb may be acceptable. In which case we should just force usage > of the swiotlb on the command line in /sbin/kexec. With an isolation-capable IOMMU (such as Calgary, VT-d and AMD's IOMMU) on the I/O path, as long as we want to keep DMAs running and going through to memory, we need to keep the IOMMU running, with the same set of permissions and translation tables. If we don't mind DMAs failing, we need to gracefully handle IOMMU DMA faults (where possible---Calgary can't do it at the moment). What we do instead with Calgary (c.f., the patch that instigated this discussion) is a hack, we "reinitialize" the IOMMU with the old IOMMU's translation tables so that DMAs continue working. My preference would be to have stopped all DMAs in the old kernel, which would've made this nastiness go away. Can you explain in simple words why we can't or won't do that? Cheers, Muli ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 6:24 ` Muli Ben-Yehuda @ 2008-07-28 13:44 ` Vivek Goyal 2008-07-28 19:12 ` Eric W. Biederman 0 siblings, 1 reply; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 13:44 UTC (permalink / raw) To: Muli Ben-Yehuda Cc: Eric W. Biederman, Simon Horman, Andrew Morton, Chandru, kexec, linux-kernel, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, linux-ia64 On Mon, Jul 28, 2008 at 09:24:46AM +0300, Muli Ben-Yehuda wrote: > On Sun, Jul 27, 2008 at 10:39:30PM -0700, Eric W. Biederman wrote: > > Simon Horman <horms@verge.net.au> writes: > > > > > There has long been a strong aversion to providing the second > > > kernel with flags like im_in_kexec or im_in_kdump, as its felt > > > that this kind of problem is better handled by making sure that > > > the hardware is in a sensible state before leaving the > > > first-kernel. But this is arguably more reasonable in the kexec > > > case than the kdump case. > > > > That and because we can generally solve the specific problem with a > > general feature. Something we can enable/disable on the command > > line if needed. Right now this is especially interesting as on > > several architectures distros are not building special kdump kernels > > but have a single kernel binary that works in both cases. > > > > Skimming through your patches this is a case we really do need to > > implement and handle cleanly. > > > > Currently we leave DMA running in the kexec on panic case. We avoid > > problems by only running out of a reserved area of memory. > > > > As as general strategy that is fine. However we have not > > implemented that strategy in the case of IOMMUs. And we are having > > trouble with IOMMUs. > > > > My hunch is that we should implement options to reserve a section of > > the iommu and to tell to the iommu to use the previously reserved > > section. Although turning iommus off altogether and simply using > > swiotlb may be acceptable. In which case we should just force usage > > of the swiotlb on the command line in /sbin/kexec. > > With an isolation-capable IOMMU (such as Calgary, VT-d and AMD's > IOMMU) on the I/O path, as long as we want to keep DMAs running and > going through to memory, we need to keep the IOMMU running, with the > same set of permissions and translation tables. If we don't mind DMAs > failing, we need to gracefully handle IOMMU DMA faults (where > possible---Calgary can't do it at the moment). What we do instead with > Calgary (c.f., the patch that instigated this discussion) is a hack, > we "reinitialize" the IOMMU with the old IOMMU's translation tables so > that DMAs continue working. > Hi Muli, Agree, using old kernel's TCE tables is a hack. As Eric pointed out, is there a reason why swiotlb will not work here? (I guess using swiotlb will mean disabling iommu and that will again fault if DMA is going on). So one of the solutions, as eric suggested, will be to reserve some entries in first kernel and then pass that info to second kernel and let second kernel use thos entries for setting up DMA and let the DMA's of first kernel run untouched. This patch is effectively doing that (using previous kernel's TCE table), except the fact that there are no gurantees that there are free table entries when kdump kernel wants to perform a DMA of its own. Should probably work though in most of the cases. > My preference would be to have stopped all DMAs in the old kernel, > which would've made this nastiness go away. Can you explain in simple > words why we can't or won't do that? > Is there a reliable way of stopping all ongoing DMAs after kernel crash? Thanks Vivek ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 13:44 ` Vivek Goyal @ 2008-07-28 19:12 ` Eric W. Biederman 0 siblings, 0 replies; 28+ messages in thread From: Eric W. Biederman @ 2008-07-28 19:12 UTC (permalink / raw) To: Vivek Goyal Cc: Muli Ben-Yehuda, Simon Horman, Andrew Morton, Chandru, kexec, linux-kernel, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, linux-ia64 Vivek Goyal <vgoyal@redhat.com> writes: > On Mon, Jul 28, 2008 at 09:24:46AM +0300, Muli Ben-Yehuda wrote: >> >> With an isolation-capable IOMMU (such as Calgary, VT-d and AMD's >> IOMMU) on the I/O path, as long as we want to keep DMAs running and >> going through to memory, we need to keep the IOMMU running, with the >> same set of permissions and translation tables. If we don't mind DMAs >> failing, we need to gracefully handle IOMMU DMA faults (where >> possible---Calgary can't do it at the moment). What we do instead with >> Calgary (c.f., the patch that instigated this discussion) is a hack, >> we "reinitialize" the IOMMU with the old IOMMU's translation tables so >> that DMAs continue working. >> > > Hi Muli, > > Agree, using old kernel's TCE tables is a hack. As Eric pointed out, > is there a reason why swiotlb will not work here? (I guess using > swiotlb will mean disabling iommu and that will again fault if > DMA is going on). > > So one of the solutions, as eric suggested, will be to reserve some > entries in first kernel and then pass that info to second kernel and > let second kernel use thos entries for setting up DMA and let the > DMA's of first kernel run untouched. > > This patch is effectively doing that (using previous kernel's TCE table), > except the fact that there are no gurantees that there are free table > entries when kdump kernel wants to perform a DMA of its own. Should > probably work though in most of the cases. > >> My preference would be to have stopped all DMAs in the old kernel, >> which would've made this nastiness go away. Can you explain in simple >> words why we can't or won't do that? > > Is there a reliable way of stopping all ongoing DMAs after kernel crash? When I investigated this there was no reliable way to stop DMAs from devices, in a generic way, and a callback to each device in the system in the kexec on panic path is less reliable then simply avoiding running out of regions where those DMAs are running. So it would be quite reasonable to drop all in flight DMA at the iommu. We do need a way to allow the drivers in the kernel running after the panic to use DMA. Which is where the idea of reserving a region of the iommu comes from. Eric ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 1:51 ` Simon Horman 2008-07-28 2:45 ` Simon Horman 2008-07-28 5:39 ` [patch] crashdump: fix undefined reference to `elfcorehdr_addr' Eric W. Biederman @ 2008-07-28 13:31 ` Vivek Goyal 2008-07-29 0:33 ` Simon Horman 2 siblings, 1 reply; 28+ messages in thread From: Vivek Goyal @ 2008-07-28 13:31 UTC (permalink / raw) To: Simon Horman Cc: Andrew Morton, Muli Ben-Yehuda, Chandru, kexec, linux-kernel, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, Eric W. Biederman, linux-ia64 On Mon, Jul 28, 2008 at 11:51:19AM +1000, Simon Horman wrote: > [ Updated Vivek's email address to his vgoyal@redhat.com in CC list > Added Terry Loftin, Tony Luck, Erik Biedermann and linux-ia64 to CC list ] > > On Mon, Jul 28, 2008 at 09:45:31AM +1000, Simon Horman wrote: > > > > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > > > > index 6cd39a9..025e4f5 100644 > > > > --- a/include/linux/crash_dump.h > > > > +++ b/include/linux/crash_dump.h > > > > @@ -8,7 +8,13 @@ > > > > #include <linux/proc_fs.h> > > > > > > > > #define ELFCORE_ADDR_MAX (-1ULL) > > > > + > > > > +#ifdef CONFIG_PROC_VMCORE > > > > extern unsigned long long elfcorehdr_addr; > > > > +#else > > > > +static const unsigned long long elfcorehdr_addr = ELFCORE_ADDR_MAX; > > > > +#endif > > > > + > > > > extern ssize_t copy_oldmem_page(unsigned long, char *, size_t, > > > > unsigned long, int); > > > > extern const struct file_operations proc_vmcore_operations; > > > > > > spose that'll fix it. But it seems odd that is_kdump_kernel() will > > > return false if CONFIG_PROC_VMCORE=n, CONFIG_CRASH_DUMP=y. I mean, > > > it's still a crashdump kernel, is it not? > > > > Perhaps is_kdump_kernel() ought to be renamed kernel_has_vmcore(). > > > > To my mind, is_kdump_kernel() should really look something like this: > > > > #ifdef CONFIG_CRASH_DUMP > > static inline int is_kdump_kernel(void) { return 1; } > > #else > > static inline int is_kdump_kernel(void) { return 0; } > > #endif > > > > But that can probably just be handled by any relevant code > > using CONFIG_CRASH_DUMP as necessary. > > Hi, > > I started looking into a simple fix to change the name of > the is_kdump_kernel() to kernel_has_vmcore(), which is what > the code in its current incarnatation does. > > This also lead to cleaning the usage of elfcorehdr_addr, > which is in the folloing messy state after recent changes. > > #ifdef CONFIG_PROC_VMCORE > * Declared non-static include/linux/crash_dump.h > * Initialised in fs/proc/vmcore.c > #else > * Declared and initialised as static in include/linux/crash_dump.h > * Only used by is_kdump_kernel() which is a static function > also in include/linux/crash_dump.h > #endif > > > Howerver, in the course of doing this I came to thinking that actually > this code won't solve the problem at hand in the case where > CONFIG_CRASH_DUMP is defined but CONFIG_PROC_VMCORE is not. > Or in other words, what happens if the calgary initialisation code > runs in a kdump kernel that does not have CONFIG_PROC_VMCORE ? > > A similar problem appears to exist in > arch/ia64/hp/common/sba_iommu.c:sba_init(), which currently doesn't > compile if CONFIG_CRASH_DUMP is set but CONFIG_PROC_VMCORE is not. The > compilation issue could be solved by using kernel_has_vmcore() (as per > the patch below) instead of checking elfcorehdr_addr directly, but does > it actually lead to working code? > > There has long been a strong aversion to providing the second > kernel with flags like im_in_kexec or im_in_kdump, as its felt > that this kind of problem is better handled by making sure that the > hardware is in a sensible state before leaving the first-kernel. > But this is arguably more reasonable in the kexec case than the > kdump case. > > > If there really is a need for kdump kernels to know that they are > booting a kdumping system, then I propose one of the following: > > 1) Always parse the elfcorehdr kernel command line option > and set elfcorehdr_addr accordingly - currently this is only > done if CONFIG_PROC_VMCORE is set. > > This is nice as it won't need any modifications to kexec-tools > nor any command line bloat. > > A minor difficulty is working out where to initialise elfcorehdr_addr. > Sometimes in include/linux/crash_dump.h and sometimes in > fs/proc/vmcore.c seems horrible to me. > > Another problem is that would be alive and well in > code that really only uses it to check if kdump was activated or not > - a minor naming issue. > Hi Simon, There are some kernel bits (like iommu initialization patch), which need to take special action if they are booting after a kexec on panic (Generally we are referring it to booting into kdump kernel) and that's why the notion is_kdump_kernel(). To me, is_kdump_kernel() symbolizes whether I am booting after kexec on panic and not just the fact if CONFIG_CRASH_DUMP is enabled or not in this kernel. So I would think that lets not rename it to kernel_has_vmcore(), instead lets write few lines of comments before function is_kdump_kernel() to clarify its meaning. Secondly, we are using elfcorehdr_addr to determine whether this kernel is booting after a panic so elfcorehdr_addr is not just limited to CONFIG_PROC_VMCORE now and we should probably pull it out of fs/proc/vmcore.c. How about declaring and initializing this variable in kernel/kexec.c under CONFIG_CRASH_DUMP and always parse elfcorehdr_addr irrespective of the setting of CONFIG_PROC_VMCORE? > 2) Add a new kernel command line option, perhaps in_kdump > > This is bloat to get around elfcorehdr_addr initialisation and > naming awkwardness above. > > 3) Make select CONFIG_PROC_VMCORE when CONFIG_CRASH_DUMP is selected, > or perhaps even just remove CONFIG_PROC_VMCORE and only use > CONFIG_CRASH_DUMP instead. The effect would be the same either way. > > Pro: One less thing to be confused about > > Con: Bloat for people who want kdump without vmcore. > I wonder what usage case that is. Argument was people can use /dev/oldmem and not use /proc/vmcore. So far I don't know anybody who uses /dev/oldmem to capture dump and not /proc/vmcore. Thanks Vivek ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr' 2008-07-28 13:31 ` Vivek Goyal @ 2008-07-29 0:33 ` Simon Horman 0 siblings, 0 replies; 28+ messages in thread From: Simon Horman @ 2008-07-29 0:33 UTC (permalink / raw) To: Vivek Goyal Cc: Andrew Morton, Muli Ben-Yehuda, Chandru, kexec, linux-kernel, Ingo Molnar, Linus Torvalds, Terry Loftin, Tony Luck, Eric W. Biederman, linux-ia64 On Mon, Jul 28, 2008 at 09:31:10AM -0400, Vivek Goyal wrote: > On Mon, Jul 28, 2008 at 11:51:19AM +1000, Simon Horman wrote: > > [ Updated Vivek's email address to his vgoyal@redhat.com in CC list > > Added Terry Loftin, Tony Luck, Erik Biedermann and linux-ia64 to CC list ] [snip] > > 1) Always parse the elfcorehdr kernel command line option > > and set elfcorehdr_addr accordingly - currently this is only > > done if CONFIG_PROC_VMCORE is set. > > > > This is nice as it won't need any modifications to kexec-tools > > nor any command line bloat. > > > > A minor difficulty is working out where to initialise elfcorehdr_addr. > > Sometimes in include/linux/crash_dump.h and sometimes in > > fs/proc/vmcore.c seems horrible to me. > > > > Another problem is that would be alive and well in > > code that really only uses it to check if kdump was activated or not > > - a minor naming issue. > > > > Hi Simon, > > There are some kernel bits (like iommu initialization patch), which need to > take special action if they are booting after a kexec on panic (Generally we > are referring it to booting into kdump kernel) and that's why the notion > is_kdump_kernel(). > > To me, is_kdump_kernel() symbolizes whether I am booting after kexec on > panic and not just the fact if CONFIG_CRASH_DUMP is enabled or not in this > kernel. > > So I would think that lets not rename it to kernel_has_vmcore(), instead > lets write few lines of comments before function is_kdump_kernel() to > clarify its meaning. > > Secondly, we are using elfcorehdr_addr to determine whether this kernel > is booting after a panic so elfcorehdr_addr is not just limited to > CONFIG_PROC_VMCORE now and we should probably pull it out of > fs/proc/vmcore.c. How about declaring and initializing this variable in > kernel/kexec.c under CONFIG_CRASH_DUMP and always parse elfcorehdr_addr > irrespective of the setting of CONFIG_PROC_VMCORE? Agreed. Like Eric I think that this is a reasonable solution given the current state of things. I'll reply to your patches after looking at them a bit more closely. > > 2) Add a new kernel command line option, perhaps in_kdump > > > > This is bloat to get around elfcorehdr_addr initialisation and > > naming awkwardness above. > > > > 3) Make select CONFIG_PROC_VMCORE when CONFIG_CRASH_DUMP is selected, > > or perhaps even just remove CONFIG_PROC_VMCORE and only use > > CONFIG_CRASH_DUMP instead. The effect would be the same either way. > > > > Pro: One less thing to be confused about > > > > Con: Bloat for people who want kdump without vmcore. > > I wonder what usage case that is. > > Argument was people can use /dev/oldmem and not use /proc/vmcore. So far > I don't know anybody who uses /dev/oldmem to capture dump and not > /proc/vmcore. What I was getting at is that frangly the three variables CONFIG_KEXEC, CONFIG_CRASH_DUMP and CONFIG_PROC_VMCORE seem to confuse people. I've seen them used incorrectly several times now. So if there is a way to simplyfy things, even slightly, I think that would be a good idea. But if there isn't, so be it. -- Horms ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2008-07-31 15:31 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-07-26 9:25 [patch] crashdump: fix undefined reference to `elfcorehdr_addr' Ingo Molnar 2008-07-26 10:10 ` Andrew Morton 2008-07-27 23:45 ` Simon Horman 2008-07-28 1:51 ` Simon Horman 2008-07-28 2:45 ` Simon Horman 2008-07-28 3:40 ` Simon Horman 2008-07-28 12:48 ` Ingo Molnar 2008-07-29 0:35 ` Simon Horman 2008-07-28 21:10 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Vivek Goyal 2008-07-28 21:11 ` [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section Vivek Goyal 2008-07-28 21:13 ` [PATCH 3/5] ia64: " Vivek Goyal 2008-07-28 21:14 ` [PATCH 4/5] powerpc: " Vivek Goyal 2008-07-28 21:15 ` [PATCH 5/5] sh: " Vivek Goyal 2008-07-29 14:18 ` Paul Mundt 2008-07-29 4:42 ` [PATCH 3/5] ia64: " Simon Horman 2008-07-29 13:53 ` Vivek Goyal 2008-07-31 15:29 ` [PATCH 2/5] x86: " Ingo Molnar 2008-07-28 22:37 ` [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr') Eric W. Biederman 2008-07-28 22:47 ` Eric W. Biederman 2008-07-29 1:22 ` Simon Horman 2008-07-29 2:28 ` Vivek Goyal 2008-07-29 3:26 ` Simon Horman 2008-07-28 5:39 ` [patch] crashdump: fix undefined reference to `elfcorehdr_addr' Eric W. Biederman 2008-07-28 6:24 ` Muli Ben-Yehuda 2008-07-28 13:44 ` Vivek Goyal 2008-07-28 19:12 ` Eric W. Biederman 2008-07-28 13:31 ` Vivek Goyal 2008-07-29 0:33 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).