Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Simon Horman]

[ 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;

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Simon Horman]

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;

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Simon Horman]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Ingo Molnar]

* 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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Simon Horman]

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

[PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch]

[Vivek Goyal]

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;
_

[PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent section

[Vivek Goyal]

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
_

[PATCH 3/5] ia64: Define elfcorehdr_addr in arch dependent section

[Vivek Goyal]

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;
_

[PATCH 4/5] powerpc: Define elfcorehdr_addr in arch dependent

[Vivek Goyal]

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

[PATCH 5/5] sh: Define elfcorehdr_addr in arch dependent section

[Vivek Goyal]

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
_

Re: [PATCH 5/5] sh: Define elfcorehdr_addr in arch dependent section

[Paul Mundt]

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>

Re: [PATCH 3/5] ia64: Define elfcorehdr_addr in arch dependent

[Simon Horman]

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

Re: [PATCH 3/5] ia64: Define elfcorehdr_addr in arch dependent

[Vivek Goyal]

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

Re: [PATCH 2/5] x86: Define elfcorehdr_addr in arch dependent

[Ingo Molnar]

* 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

Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined refe

[Eric W. Biederman]

Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re: [patch] crashdump: fix undefined refe

[Eric W. Biederman]

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

Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re:

[Simon Horman]

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

Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re:

[Vivek Goyal]

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

Re: [PATCH 1/5] Move elfcorehdr_addr out of vmcore.c (Was: Re:

[Simon Horman]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Eric W. Biederman]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Muli Ben-Yehuda]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Vivek Goyal]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Eric W. Biederman]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Vivek Goyal]

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

Re: [patch] crashdump: fix undefined reference to `elfcorehdr_addr'

[Simon Horman]

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