kdump: use is_kdump_kernel() in sba_init()

After recent patches* is_kdump_kernel() should return 1 in the case where code is executing in a crashkernel and 0 otherwise. It is save to use outside of CONFIG_KDUMP.

* http://lkml.org/lkml/2008/7/28/445

Quoted 6 lines · Simon Horman

After recent patches* is_kdump_kernel() should return 1 in the case where code is executing in a crashkernel and 0 otherwise. It is save to use outside of CONFIG_KDUMP.

* http://lkml.org/lkml/2008/7/28/445

Hi Simon,

is_kdump_kernel() returns 1 when both the following conditions are true.

- CONFIG_CRASH_DUMP is enabled
- And kernel is booting after a panic.

If any of the above condition is not true, it returns 0.

So for the cases where, CONFIG_CRASH_DUMP=n and kernel is booting after panic, is_kdump_kernel() will still return 0.

This essentially means that any kernel which is booting after a panic, but does not have CONFIG_CRASH_DUMP enabled, will not take special actions required for booting that kernel. Until, somebody can come up with another usage of booting a kernel after a panic (apart from crash dump), I think this is good enough an assumption.

So your changes will work. I just wanted to clarify the semantics more explicitly.

After recent patches* is_kdump_kernel() should return 1 in the case where code is executing in a crashkernel and 0 otherwise. It is safe to use outside of CONFIG_CRASH_DUMP.

* http://lkml.org/lkml/2008/7/28/445

Signed-off-by Simon Horman <[email protected]>

Acked-by Vivek Goyal <[email protected]>

Index: linux-2.6/arch/ia64/hp/common/sba_iommu.c
=================================> --- linux-2.6.orig/arch/ia64/hp/common/sba_iommu.c 2008-07-29 10:57:17.000000000 +1000
+++ linux-2.6/arch/ia64/hp/common/sba_iommu.c 2008-07-29 10:57:51.000000000 +1000

(quoted diff) +1 -2

@@ -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_CRASH_DUMP) && \
-        defined(CONFIG_PROC_FS)
+#if defined(CONFIG_IA64_GENERIC)

The usage of elfcorehdr_addr has changed recently such that being set to ELFCORE_ADDR_MAX is used by is_kdump_kernel() to indicate if the code is executing in a kernel executed as a crash kernel.

However, arch/ia64/kernel/setup.c:reserve_elfcorehdr will rest elfcorehdr_addr to ELFCORE_ADDR_MAX on error, which means any subsequent calls to is_kdump_kernel() will return 0, even though they should return 1.

Ok, at this point in time there are no subsequent calls, but I think its fair to say that there is ample scope for error or at the very least confusion.

This patch add an extra state, ELFCORE_ADDR_ERR, which indicates that elfcorehdr_addr was passed on the command line, and thus execution is taking place in a crashdump kernel, but vmcore can't be used for some reason. This is tested for using is_vmcore_usable() and set using vmcore_unusable(). A subsequent patch makes use of this new code.

To summarise, the states that elfcorehdr_addr can now be in are as follows:

ELFCORE_ADDR_MAX: not a crashdump kernel
ELFCORE_ADDR_ERR: crashdump kernel but vmcore is unusable
any other value:  crash dump kernel and vmcore is usable

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/include/linux/crash_dump.h
=================================> --- linux-2.6.orig/include/linux/crash_dump.h 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/include/linux/crash_dump.h 2008-07-29 17:53:13.000000000 +1000

(quoted diff) +14 -0

@@ -8,6 +8,7 @@
 #include <linux/proc_fs.h>
 
 #define ELFCORE_ADDR_MAX	(-1ULL)
+#define ELFCORE_ADDR_ERR	(-2ULL)
 
 #ifdef CONFIG_PROC_VMCORE
 extern unsigned long long elfcorehdr_addr;

@@ -36,5 +37,30 @@ static inline int is_kdump_kernel(void)
 static inline int is_kdump_kernel(void) { return 0; }
 #endif /* CONFIG_CRASH_DUMP */
 
+#ifdef CONFIG_PROC_VMCORE
+/* is_vmcore_usable() checks if the kernel is booting after a panic and
+ * the vmcore region is usable.
+ *
+ * This makes use of the fact that due to alignment 1 is not
+ * a valid pointer, much in the vain of IS_ERR(), except
+ * dealing directly with an unsigned long long rather than a pointer.
+ */
+
+static inline int is_vmcore_usable(void)
+{
+	return is_kdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0;
+}

Quoted 58 lines · Simon Horman

The usage of elfcorehdr_addr has changed recently such that being set to ELFCORE_ADDR_MAX is used by is_kdump_kernel() to indicate if the code is executing in a kernel executed as a crash kernel.

However, arch/ia64/kernel/setup.c:reserve_elfcorehdr will rest elfcorehdr_addr to ELFCORE_ADDR_MAX on error, which means any subsequent calls to is_kdump_kernel() will return 0, even though they should return 1.

Ok, at this point in time there are no subsequent calls, but I think its fair to say that there is ample scope for error or at the very least confusion.

This patch add an extra state, ELFCORE_ADDR_ERR, which indicates that elfcorehdr_addr was passed on the command line, and thus execution is taking place in a crashdump kernel, but vmcore can't be used for some reason. This is tested for using is_vmcore_usable() and set using vmcore_unusable(). A subsequent patch makes use of this new code.

To summarise, the states that elfcorehdr_addr can now be in are as follows:

ELFCORE_ADDR_MAX: not a crashdump kernel
ELFCORE_ADDR_ERR: crashdump kernel but vmcore is unusable
any other value:  crash dump kernel and vmcore is usable

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/include/linux/crash_dump.h
=================================> > --- linux-2.6.orig/include/linux/crash_dump.h 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/include/linux/crash_dump.h 2008-07-29 17:53:13.000000000 +1000

(quoted diff) +14 -0

@@ -8,6 +8,7 @@
 #include <linux/proc_fs.h>
 
 #define ELFCORE_ADDR_MAX	(-1ULL)
+#define ELFCORE_ADDR_ERR	(-2ULL)
 
 #ifdef CONFIG_PROC_VMCORE
 extern unsigned long long elfcorehdr_addr;

@@ -36,5 +37,30 @@ static inline int is_kdump_kernel(void)
 static inline int is_kdump_kernel(void) { return 0; }
 #endif /* CONFIG_CRASH_DUMP */
 
+#ifdef CONFIG_PROC_VMCORE
+/* is_vmcore_usable() checks if the kernel is booting after a panic and
+ * the vmcore region is usable.
+ *
+ * This makes use of the fact that due to alignment 1 is not
+ * a valid pointer, much in the vain of IS_ERR(), except
+ * dealing directly with an unsigned long long rather than a pointer.
+ */
+
+static inline int is_vmcore_usable(void)
+{
+	return is_kdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0;
+}

Review static inline int is_kdump_kernel(void) · line 62

Quoted 10 lines

(quoted diff) +10 -0

+
+/* vmcore_unusable() marks the vmcore as unusable, without disturbing
+ * the logic of is_kdump_kernel()
+ */
+
+static inline void vmcore_unusable(void)
+{
+	if (!is_kdump_kernel())
+		elfcorehdr_addr = ELFCORE_ADDR_ERR;
+}

Hi Simon,

Should above condition be "if(is_kdump_kernel())" instead of "if(!is_kdump_kernel())?

I would think that you would like to mark a vmcore unusable only if, to begin with you were booting after a panic.

If we being marking vmcore_unusable in case of normal kernel (!is_kdump_kernel()), then is_kdump_kernel() will start reporting normal kernel as kdump kernel?

After recent changes setting elfcorehdr_addr to ELFCORE_ADDR_MAX will cause is_kdump_kernel() to return 0 when it should return 1. Instead use vmcore_unusable(), which has been added for this purpose.

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/arch/ia64/kernel/setup.c
=================================> --- linux-2.6.orig/arch/ia64/kernel/setup.c 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/setup.c 2008-07-29 17:50:50.000000000 +1000

(quoted diff) +0 -0

@@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
 	 * to work properly.
 	 */
 

Signature · 4 lines

-- 

-- 
Horms

Quoted 13 lines · Simon Horman

After recent changes setting elfcorehdr_addr to ELFCORE_ADDR_MAX will cause is_kdump_kernel() to return 0 when it should return 1. Instead use vmcore_unusable(), which has been added for this purpose.

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/arch/ia64/kernel/setup.c
=================================> > --- linux-2.6.orig/arch/ia64/kernel/setup.c 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/setup.c 2008-07-29 17:50:50.000000000 +1000

(quoted diff) +0 -0

@@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
 	 * to work properly.
 	 */
 

Review int __init reserve_elfcorehdr(unsigned l · line 512

Quoted 10 lines

(quoted diff) +2 -2

-	if (elfcorehdr_addr >= ELFCORE_ADDR_MAX)
+	if (!is_vmcore_usable())
 		return -EINVAL;
 
 	if ((length = vmcore_find_descriptor_size(elfcorehdr_addr)) = 0) {
-		elfcorehdr_addr = ELFCORE_ADDR_MAX;
+		vmcore_unusable();
 		return -EINVAL;
 	}
 

Hi Simon,

I had a question. I am not very sure what reserve_elfcorehdr is doing but doing something similar to reserving some memory area where elfcoreheaders are.

I see that reserve_elfcorehdr is under CONFIG_PROC_VMCORE. Will it work if CONFIG_PROC_VMCORE=n and somebody wants to use /dev/oldmem? Or reserve_elfcorehdr should be under CONFIG_CRASH_DUMP?

Quoted 27 lines · Vivek Goyal

Quoted 13 lines · Simon Horman

After recent changes setting elfcorehdr_addr to ELFCORE_ADDR_MAX will cause is_kdump_kernel() to return 0 when it should return 1. Instead use vmcore_unusable(), which has been added for this purpose.

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/arch/ia64/kernel/setup.c
=================================> > > --- linux-2.6.orig/arch/ia64/kernel/setup.c 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/setup.c 2008-07-29 17:50:50.000000000 +1000

(quoted diff) +0 -0

@@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
 	 * to work properly.
 	 */
 

Review int __init reserve_elfcorehdr(unsigned l · line 512

Quoted 10 lines

(quoted diff) +2 -2

-	if (elfcorehdr_addr >= ELFCORE_ADDR_MAX)
+	if (!is_vmcore_usable())
 		return -EINVAL;
 
 	if ((length = vmcore_find_descriptor_size(elfcorehdr_addr)) = 0) {
-		elfcorehdr_addr = ELFCORE_ADDR_MAX;
+		vmcore_unusable();
 		return -EINVAL;
 	}
 

Hi Simon,

Quoted 3 lines

I had a question. I am not very sure what reserve_elfcorehdr is doing but doing something similar to reserving some memory area where elfcoreheaders are.

Yes, that is my understanding of what it does.

Quoted 3 lines

I see that reserve_elfcorehdr is under CONFIG_PROC_VMCORE. Will it work if CONFIG_PROC_VMCORE=n and somebody wants to use /dev/oldmem? Or reserve_elfcorehdr should be under CONFIG_CRASH_DUMP?

I'm not that familiar with /dev/oldmem, but as far as I can see, read_oldmem doesn't do anything with ELF headers, so reservation by vmcore_find_descriptor_size() should not be neccessary.

Quoted 28 lines · Simon Horman

Quoted 27 lines · Vivek Goyal

Quoted 13 lines · Simon Horman

After recent changes setting elfcorehdr_addr to ELFCORE_ADDR_MAX will cause is_kdump_kernel() to return 0 when it should return 1. Instead use vmcore_unusable(), which has been added for this purpose.

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/arch/ia64/kernel/setup.c
=================================> > > > --- linux-2.6.orig/arch/ia64/kernel/setup.c 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/setup.c 2008-07-29 17:50:50.000000000 +1000

(quoted diff) +0 -0

@@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
 	 * to work properly.
 	 */
 

Review int __init reserve_elfcorehdr(unsigned l · line 512

Quoted 10 lines

(quoted diff) +2 -2

-	if (elfcorehdr_addr >= ELFCORE_ADDR_MAX)
+	if (!is_vmcore_usable())
 		return -EINVAL;
 
 	if ((length = vmcore_find_descriptor_size(elfcorehdr_addr)) = 0) {
-		elfcorehdr_addr = ELFCORE_ADDR_MAX;
+		vmcore_unusable();
 		return -EINVAL;
 	}
 

Hi Simon,

Quoted 6 lines

Quoted 3 lines

I had a question. I am not very sure what reserve_elfcorehdr is doing but doing something similar to reserving some memory area where elfcoreheaders are.

Yes, that is my understanding of what it does.

In x86 we never have to reserve that elf header area as we know that booting kernel will never overwrite in that area. (Previous kernel has modified the memory map in such a way so that elf header memory area is not even part of memory range second kernel can use.

Any idea, why do we need to reserve this area in IA64.

Quoted 8 lines

Quoted 3 lines

I see that reserve_elfcorehdr is under CONFIG_PROC_VMCORE. Will it work if CONFIG_PROC_VMCORE=n and somebody wants to use /dev/oldmem? Or reserve_elfcorehdr should be under CONFIG_CRASH_DUMP?

I'm not that familiar with /dev/oldmem, but as far as I can see, read_oldmem doesn't do anything with ELF headers, so reservation by vmcore_find_descriptor_size() should not be neccessary.

/dev/oldmem does not directly touch elfcorehdr. But it indirectly does in the sense that it is reading all the previous kernel's memory and dumping it to disk. That would include elfcorehdrs also.

Now if we need to do some kind of reservation for elf headers so that fs/vmcore.c code can read it, then same should be true for /dev/oldmem code also. Otherwise reserving elfcorehdr code should be redundant.

That's why I raised the question that why do we need to reserve that area in case of IA64. And if there is a genuine reason then probably that reason will hold valid in case of /dev/oldmem also and we might have to put is_vmcore_usable() definition also under CONFIG_CRASH_DUMP.

Quoted 56 lines · Vivek Goyal

Quoted 28 lines · Simon Horman

Quoted 27 lines · Vivek Goyal

Quoted 13 lines · Simon Horman

After recent changes setting elfcorehdr_addr to ELFCORE_ADDR_MAX will cause is_kdump_kernel() to return 0 when it should return 1. Instead use vmcore_unusable(), which has been added for this purpose.

Signed-off-by Simon Horman <[email protected]>

Index: linux-2.6/arch/ia64/kernel/setup.c
=================================> > > > > --- linux-2.6.orig/arch/ia64/kernel/setup.c 2008-07-29 17:27:43.000000000 +1000
+++ linux-2.6/arch/ia64/kernel/setup.c 2008-07-29 17:50:50.000000000 +1000

(quoted diff) +0 -0

@@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
 	 * to work properly.
 	 */
 

Review int __init reserve_elfcorehdr(unsigned l · line 512

Quoted 10 lines

(quoted diff) +2 -2

-	if (elfcorehdr_addr >= ELFCORE_ADDR_MAX)
+	if (!is_vmcore_usable())
 		return -EINVAL;
 
 	if ((length = vmcore_find_descriptor_size(elfcorehdr_addr)) = 0) {
-		elfcorehdr_addr = ELFCORE_ADDR_MAX;
+		vmcore_unusable();
 		return -EINVAL;
 	}
 

Hi Simon,

Quoted 6 lines

Quoted 3 lines

I had a question. I am not very sure what reserve_elfcorehdr is doing but doing something similar to reserving some memory area where elfcoreheaders are.

Yes, that is my understanding of what it does.

In x86 we never have to reserve that elf header area as we know that booting kernel will never overwrite in that area. (Previous kernel has modified the memory map in such a way so that elf header memory area is not even part of memory range second kernel can use.

Any idea, why do we need to reserve this area in IA64.

Quoted 8 lines

Quoted 3 lines

I see that reserve_elfcorehdr is under CONFIG_PROC_VMCORE. Will it work if CONFIG_PROC_VMCORE=n and somebody wants to use /dev/oldmem? Or reserve_elfcorehdr should be under CONFIG_CRASH_DUMP?

I'm not that familiar with /dev/oldmem, but as far as I can see, read_oldmem doesn't do anything with ELF headers, so reservation by vmcore_find_descriptor_size() should not be neccessary.

/dev/oldmem does not directly touch elfcorehdr. But it indirectly does in the sense that it is reading all the previous kernel's memory and dumping it to disk. That would include elfcorehdrs also.

Quoted 8 lines

Now if we need to do some kind of reservation for elf headers so that fs/vmcore.c code can read it, then same should be true for /dev/oldmem code also. Otherwise reserving elfcorehdr code should be redundant.

That's why I raised the question that why do we need to reserve that area in case of IA64. And if there is a genuine reason then probably that reason will hold valid in case of /dev/oldmem also and we might have to put is_vmcore_usable() definition also under CONFIG_CRASH_DUMP.

Hi Vivek,

yes, I do think that the problem is a bit deeper than my original response. I'll investigate this further.