Re: [patch 2/3] kdump: add is_vmcore_usable() and vmcore_unusable()

Re: [patch 2/3] kdump: add is_vmcore_usable() and vmcore_unusable()

[Vivek Goyal]

On Tue, Jul 29, 2008 at 06:12:37PM +1000, Simon Horman wrote:
> 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 <horms@verge.net.au>
> 
> 
> 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
> @@ -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;
> +}
> +
> +/* 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?

Thanks
Vivek

Re: [patch 2/3] kdump: add is_vmcore_usable() and vmcore_unusable()

[Simon Horman]

On Tue, Jul 29, 2008 at 11:15:33AM -0400, Vivek Goyal wrote:
> On Tue, Jul 29, 2008 at 06:12:37PM +1000, Simon Horman wrote:
> > 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 <horms@verge.net.au>
> > 
> > 
> > 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
> > @@ -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;
> > +}
> > +
> > +/* 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?

Yes, you are correct. Sorry for the silly mistake. I'll repost with

if (!is_kdump_kernel())
	...

-- 
Horms

Re: [patch 2/3] kdump: add is_vmcore_usable() and vmcore_unusable()

[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 <horms@verge.net.au>

--- 

 fs/proc/vmcore.c           |    2 +-
 include/linux/crash_dump.h |   26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

Wed, 30 Jul 2008 09:10:37 +1000
* Fixed logic bug in vmcore_unusable().
  if (!is_kdump_kernel()) is correct
  if (is_kdump_kernel()) is not

Index: linux-2.6/include/linux/crash_dump.h
===================================================================
--- linux-2.6.orig/include/linux/crash_dump.h	2008-07-30 09:05:50.000000000 +1000
+++ linux-2.6/include/linux/crash_dump.h	2008-07-30 09:08:58.000000000 +1000
@@ -8,6 +8,7 @@
 #include <linux/proc_fs.h>
 
 #define ELFCORE_ADDR_MAX	(-1ULL)
+#define ELFCORE_ADDR_ERR	(-2ULL)
 
 extern unsigned long long elfcorehdr_addr;
 
@@ -42,5 +43,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;
+}
+
+/* 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;
+}
+#endif /* CONFIG_PROC_VMCORE */
+
 extern unsigned long saved_max_pfn;
 #endif /* LINUX_CRASHDUMP_H */
Index: linux-2.6/fs/proc/vmcore.c
===================================================================
--- linux-2.6.orig/fs/proc/vmcore.c	2008-07-30 09:05:50.000000000 +1000
+++ linux-2.6/fs/proc/vmcore.c	2008-07-30 09:07:07.000000000 +1000
@@ -650,7 +650,7 @@ static int __init vmcore_init(void)
 	int rc = 0;
 
 	/* If elfcorehdr= has been passed in cmdline, then capture the dump.*/
-	if (!(elfcorehdr_addr < ELFCORE_ADDR_MAX))
+	if (!(is_vmcore_usable()))
 		return rc;
 	rc = parse_crash_elf_headers();
 	if (rc) {

Re: [patch 1/3] kdump: use is_kdump_kernel() in sba_init()

[Simon Horman]

On Tue, Jul 29, 2008 at 10:51:56AM -0400, Vivek Goyal wrote:
> On Tue, Jul 29, 2008 at 06:12:36PM +1000, Simon Horman wrote:
> > 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.

Hi Vivek,

sorry if my explanation was confused. The scheme that you describe
above is what I am working with too.

-- 
Horms

Re: [patch 1/3] kdump: use is_kdump_kernel() in sba_init()

[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 safe to use outside of CONFIG_CRASH_DUMP.

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

Signed-off-by: Simon Horman <horms@verge.net.au>
Acked-by: Vivek Goyal <vgoyal@redhat.com>

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
@@ -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)
 	/* 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 (is_kdump_kernel()) {
 		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 1/3] kdump: use is_kdump_kernel() in sba_init()

[Vivek Goyal]

On Wed, Jul 30, 2008 at 09:15:10AM +1000, Simon Horman wrote:
> 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 <horms@verge.net.au>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> 
> 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
> @@ -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)
>  	/* 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 (is_kdump_kernel()) {
>  		if (swiotlb_late_init_with_default_size(64 * (1<<20)) != 0)
>  			panic("Unable to initialize software I/O TLB:"
>  				  " Try machvec=dig boot option");

Looks good to me.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

Re: [patch 3/3] kdump: use is_vmcore_usable() and vmcore_unusable() in reserve_elfcorehdr()

[Simon Horman]

On Wed, Jul 30, 2008 at 09:01:31AM -0400, Vivek Goyal wrote:
> On Tue, Jul 29, 2008 at 06:12:38PM +1000, Simon Horman wrote:
> > 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 <horms@verge.net.au>
> > 
> > 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
> > @@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
> >  	 * to work properly.
> >  	 */
> >  
> > -	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.

Yes, that is my understanding of what it does.

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

On a related note, I wonder if some consolitation of
read_oldmem() (used by /dev/oldmem) and read_from_oldmem()
(used by /proc/vmcore) can be done.

-- 
Horms

Re: [patch 3/3] kdump: use is_vmcore_usable() and vmcore_unusable() in reserve_elfcorehdr()

[Vivek Goyal]

On Thu, Jul 31, 2008 at 10:48:43AM +1000, Simon Horman wrote:
> On Wed, Jul 30, 2008 at 09:01:31AM -0400, Vivek Goyal wrote:
> > On Tue, Jul 29, 2008 at 06:12:38PM +1000, Simon Horman wrote:
> > > 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 <horms@verge.net.au>
> > > 
> > > 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
> > > @@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
> > >  	 * to work properly.
> > >  	 */
> > >  
> > > -	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.
> 
> 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.

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

Thanks
Vivek

Re: [patch 3/3] kdump: use is_vmcore_usable() and vmcore_unusable() in reserve_elfcorehdr()

[Simon Horman]

On Thu, Jul 31, 2008 at 09:21:52AM -0400, Vivek Goyal wrote:
> On Thu, Jul 31, 2008 at 10:48:43AM +1000, Simon Horman wrote:
> > On Wed, Jul 30, 2008 at 09:01:31AM -0400, Vivek Goyal wrote:
> > > On Tue, Jul 29, 2008 at 06:12:38PM +1000, Simon Horman wrote:
> > > > 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 <horms@verge.net.au>
> > > > 
> > > > 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
> > > > @@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
> > > >  	 * to work properly.
> > > >  	 */
> > > >  
> > > > -	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.
> > 
> > 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.
> 
> > > 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.

Hi Vivek,

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

-- 
Horms

Re: [patch 3/3] kdump: use is_vmcore_usable() and vmcore_unusable() in reserve_elfcorehdr()

[Simon Horman]

On Fri, Aug 01, 2008 at 02:08:01PM +1000, Simon Horman wrote:
> On Thu, Jul 31, 2008 at 09:21:52AM -0400, Vivek Goyal wrote:
> > On Thu, Jul 31, 2008 at 10:48:43AM +1000, Simon Horman wrote:
> > > On Wed, Jul 30, 2008 at 09:01:31AM -0400, Vivek Goyal wrote:
> > > > On Tue, Jul 29, 2008 at 06:12:38PM +1000, Simon Horman wrote:
> > > > > 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 <horms@verge.net.au>
> > > > > 
> > > > > 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
> > > > > @@ -502,11 +502,11 @@ int __init reserve_elfcorehdr(unsigned l
> > > > >  	 * to work properly.
> > > > >  	 */
> > > > >  
> > > > > -	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.
> > > 
> > > 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.
> > 
> > > > 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.
> 
> Hi Vivek,
> 
> yes, I do think that the problem is a bit deeper than my original response.
> I'll investigate this further.

Hi Vivek,

sorry for not getting back to you sooner. I think that the short answer
to this question is that the code should be protected by
CONFIG_CRASH_DUMP instead of CONFIG_PROC_VMCORE. This should ensure
that the memory where the elfheader lies is not used by the crashkernel,
regardless of if CONFIG_PROC_VMCORE is in effect or not.

I will make a patch for this ASAP.