crashdump: fix undefined reference to `elfcorehdr_addr'

From 72db7cba50b6a05825f8a287f74002cc38f04fb7 Mon Sep 17 00:00:00 2001

please apply before -rc1.

Ingo

----------->

Quoted 1 line

From 72db7cba50b6a05825f8a287f74002cc38f04fb7 Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
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 <[email protected]>

---
 include/linux/crash_dump.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/linux/crash_dump.h +0 -0

index 6cd39a9..025e4f5 100644
@@ -8,7 +8,13 @@
 #include <linux/proc_fs.h>
 
 #define ELFCORE_ADDR_MAX	(-1ULL)

Quoted 37 lines · On Sat, 26 Jul 2008 11:25:19 +0200 Ingo Molnar <[email protected]> wrote:

please apply before -rc1.

Ingo

----------->

Quoted 1 line

From 72db7cba50b6a05825f8a287f74002cc38f04fb7 Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
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 <[email protected]>

---
 include/linux/crash_dump.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/linux/crash_dump.h +0 -0

index 6cd39a9..025e4f5 100644
@@ -8,7 +8,13 @@
 #include <linux/proc_fs.h>
 

include/linux/crash_dump.h line 20

Quoted 11 lines

include/linux/crash_dump.h +6 -0

 #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?

Quoted 7 lines

Quoted 7 lines

include/linux/crash_dump.h +0 -0

index 6cd39a9..025e4f5 100644
@@ -8,7 +8,13 @@
 #include <linux/proc_fs.h>
 

Quoted 15 lines

include/linux/crash_dump.h line 20

Quoted 11 lines

include/linux/crash_dump.h +6 -0

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

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.

Quoted 4 lines · 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.

Sorry, one more time. I forgot to quilt refresh.

Quoted 7 lines · Simon Horman

Quoted 4 lines · 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.

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

---

 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

arch/ia64/kernel/setup.c renamed from arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 +8 -1

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

_

Quoted 40 lines · 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 <[email protected]>

---

 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

arch/ia64/kernel/setup.c renamed from arch/ia64/kernel/setup.c~fix-elfcorehdr_addr-parsing-ia64 +8 -1

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

Quoted 1 line

_

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 <[email protected]>

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 <[email protected]>

---

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

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.

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

Given the current state of the code:

Acked-by "Eric W. Biederman" <[email protected]>

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.

Quoted 15 lines · Eric W. Biederman

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

Given the current state of the code:

Acked-by "Eric W. Biederman" <[email protected]>

Quoted 7 lines

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.

Signature · 2 lines

-- 
Horms

Quoted 24 lines · Simon Horman

Quoted 22 lines · Eric W. Biederman

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

Given the current state of the code:

Acked-by "Eric W. Biederman" <[email protected]>

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.

Quoted 4 lines

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

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.

Quoted 6 lines · Simon Horman

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.

Quoted 25 lines · Eric W. Biederman

Quoted 6 lines · Simon Horman

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.

Quoted 5 lines

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?

Quoted 11 lines · Muli Ben-Yehuda

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.

Quoted 3 lines

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?

[ Updated Vivek's email address to his [email protected] in CC list
Added Terry Loftin, Tony Luck, Erik Biedermann and linux-ia64 to CC list ]

Quoted 27 lines · Simon Horman

Quoted 7 lines

Quoted 7 lines

include/linux/crash_dump.h +0 -0

index 6cd39a9..025e4f5 100644
@@ -8,7 +8,13 @@
 #include <linux/proc_fs.h>
 

Quoted 15 lines

include/linux/crash_dump.h line 20

Quoted 11 lines

include/linux/crash_dump.h +6 -0

 #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:

Quoted 8 lines

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

Quoted 2 lines · Simon Horman

[ Updated Vivek's email address to his [email protected] in CC list
Added Terry Loftin, Tony Luck, Erik Biedermann and linux-ia64 to CC list ]

Quoted 7 lines

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.

Quoted 8 lines

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?

Quoted 9 lines

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.

Quoted 4 lines

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.