[PATCH] IA64: better calculate PT_LOAD segment size

[PATCH] IA64: better calculate PT_LOAD segment size

[Jay Lan]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

This patch combines consecutive PL_LOAD segments into one.
The end address of the last PL_LOAD segment, calculated by
adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
will be the end address of this loaded_segments[] entry.

This patch fixes the kdump kernel MCA problem caused by under-
allocation of memory and a "kdump broken on ALtix 350" problem
reported by Bernhard Walle.

Simon, this patch replaces my previous patch I submitted on the
underallocation issue.

Signed-off-by: Jay Lan <jlan@sgi.com>


[-- Attachment #2: bootmem-fix.v2 --]
[-- Type: text/plain, Size: 1372 bytes --]

---
 kexec/arch/ia64/crashdump-ia64.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: kexec-tools/kexec/arch/ia64/crashdump-ia64.c
===================================================================
--- kexec-tools.orig/kexec/arch/ia64/crashdump-ia64.c	2008-09-19 14:33:07.593344017 -0700
+++ kexec-tools/kexec/arch/ia64/crashdump-ia64.c	2008-09-19 17:39:03.732928237 -0700
@@ -86,19 +86,20 @@ static void add_loaded_segments_info(str
                 loaded_segments[loaded_segments_num].end =
 			loaded_segments[loaded_segments_num].start;
 
+		/* Consolidate consecutive PL_LOAD segments into one.
+		 * The end addr of the last PL_LOAD segment, calculated by
+		 * adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
+		 * will be the end address of this loaded_segments entry.
+		 */
 		while (i < ehdr->e_phnum) {
 			phdr = &ehdr->e_phdr[i];
 	                if (phdr->p_type != PT_LOAD)
 	                        break;
-			if (loaded_segments[loaded_segments_num].end !=
-				phdr->p_paddr & ~(ELF_PAGE_SIZE-1))
-				break;
-			loaded_segments[loaded_segments_num].end +=
-				(phdr->p_memsz + ELF_PAGE_SIZE - 1) &
-				~(ELF_PAGE_SIZE - 1);
+			loaded_segments[loaded_segments_num].end =
+				(phdr->p_paddr + phdr->p_memsz +
+				ELF_PAGE_SIZE - 1) & ~(ELF_PAGE_SIZE - 1);
 			i++;
 		}
-
 		loaded_segments_num++;
 	}
 }

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] IA64: better calculate PT_LOAD segment size

[Simon Horman]

On Fri, Sep 19, 2008 at 07:17:05PM -0700, Jay Lan wrote:
> This patch combines consecutive PL_LOAD segments into one.
> The end address of the last PL_LOAD segment, calculated by
> adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
> will be the end address of this loaded_segments[] entry.
> 
> This patch fixes the kdump kernel MCA problem caused by under-
> allocation of memory and a "kdump broken on ALtix 350" problem
> reported by Bernhard Walle.
> 
> Simon, this patch replaces my previous patch I submitted on the
> underallocation issue.
> 
> Signed-off-by: Jay Lan <jlan@sgi.com>
> 

The logic-bug portion of this fix I am fine with.
As you pointed out previously & has higher precedence
than += which makes the following bogus:

                        loaded_segments[loaded_segments_num].end +=
                                (phdr->p_memsz + ELF_PAGE_SIZE - 1) &
                                ~(ELF_PAGE_SIZE - 1)

The segment merging portion I am less clear about.
Why are there gaps in the first place? If these
multiple PT_LOAD segments are really one segment,
why aren't they being fed in as one segment?

> ---
>  kexec/arch/ia64/crashdump-ia64.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> Index: kexec-tools/kexec/arch/ia64/crashdump-ia64.c
> ===================================================================
> --- kexec-tools.orig/kexec/arch/ia64/crashdump-ia64.c	2008-09-19 14:33:07.593344017 -0700
> +++ kexec-tools/kexec/arch/ia64/crashdump-ia64.c	2008-09-19 17:39:03.732928237 -0700
> @@ -86,19 +86,20 @@ static void add_loaded_segments_info(str
>                  loaded_segments[loaded_segments_num].end =
>  			loaded_segments[loaded_segments_num].start;
>  
> +		/* Consolidate consecutive PL_LOAD segments into one.
> +		 * The end addr of the last PL_LOAD segment, calculated by
> +		 * adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
> +		 * will be the end address of this loaded_segments entry.
> +		 */
>  		while (i < ehdr->e_phnum) {
>  			phdr = &ehdr->e_phdr[i];
>  	                if (phdr->p_type != PT_LOAD)
>  	                        break;
> -			if (loaded_segments[loaded_segments_num].end !=
> -				phdr->p_paddr & ~(ELF_PAGE_SIZE-1))
> -				break;
> -			loaded_segments[loaded_segments_num].end +=
> -				(phdr->p_memsz + ELF_PAGE_SIZE - 1) &
> -				~(ELF_PAGE_SIZE - 1);
> +			loaded_segments[loaded_segments_num].end =
> +				(phdr->p_paddr + phdr->p_memsz +
> +				ELF_PAGE_SIZE - 1) & ~(ELF_PAGE_SIZE - 1);
>  			i++;
>  		}
> -
>  		loaded_segments_num++;
>  	}
>  }


-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] IA64: better calculate PT_LOAD segment size

[Jay Lan]

Simon Horman wrote:
> On Fri, Sep 19, 2008 at 07:17:05PM -0700, Jay Lan wrote:
>> This patch combines consecutive PL_LOAD segments into one.
>> The end address of the last PL_LOAD segment, calculated by
>> adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
>> will be the end address of this loaded_segments[] entry.
>>
>> This patch fixes the kdump kernel MCA problem caused by under-
>> allocation of memory and a "kdump broken on ALtix 350" problem
>> reported by Bernhard Walle.
>>
>> Simon, this patch replaces my previous patch I submitted on the
>> underallocation issue.
>>
>> Signed-off-by: Jay Lan <jlan@sgi.com>
>>
> 
> The logic-bug portion of this fix I am fine with.
> As you pointed out previously & has higher precedence
> than += which makes the following bogus:
> 
>                         loaded_segments[loaded_segments_num].end +=
>                                 (phdr->p_memsz + ELF_PAGE_SIZE - 1) &
>                                 ~(ELF_PAGE_SIZE - 1)
> 
> The segment merging portion I am less clear about.
> Why are there gaps in the first place? 

I think there is still a bug in the kernel.

> If these
> multiple PT_LOAD segments are really one segment,
> why aren't they being fed in as one segment?

I searched a little... My guess is that it was done so that
the program headers provided more information for kexec
and crash? And thus put it back to simulate a boot loader?
I really do not know why it was done. Maybe Vivek or Eric
would know?

The current comparison part of code is basically a no-op.
I think we can simplify the code here.

Regards,
jay

> 
>> ---
>>  kexec/arch/ia64/crashdump-ia64.c |   15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> Index: kexec-tools/kexec/arch/ia64/crashdump-ia64.c
>> ===================================================================
>> --- kexec-tools.orig/kexec/arch/ia64/crashdump-ia64.c	2008-09-19 14:33:07.593344017 -0700
>> +++ kexec-tools/kexec/arch/ia64/crashdump-ia64.c	2008-09-19 17:39:03.732928237 -0700
>> @@ -86,19 +86,20 @@ static void add_loaded_segments_info(str
>>                  loaded_segments[loaded_segments_num].end =
>>  			loaded_segments[loaded_segments_num].start;
>>  
>> +		/* Consolidate consecutive PL_LOAD segments into one.
>> +		 * The end addr of the last PL_LOAD segment, calculated by
>> +		 * adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
>> +		 * will be the end address of this loaded_segments entry.
>> +		 */
>>  		while (i < ehdr->e_phnum) {
>>  			phdr = &ehdr->e_phdr[i];
>>  	                if (phdr->p_type != PT_LOAD)
>>  	                        break;
>> -			if (loaded_segments[loaded_segments_num].end !=
>> -				phdr->p_paddr & ~(ELF_PAGE_SIZE-1))
>> -				break;
>> -			loaded_segments[loaded_segments_num].end +=
>> -				(phdr->p_memsz + ELF_PAGE_SIZE - 1) &
>> -				~(ELF_PAGE_SIZE - 1);
>> +			loaded_segments[loaded_segments_num].end =
>> +				(phdr->p_paddr + phdr->p_memsz +
>> +				ELF_PAGE_SIZE - 1) & ~(ELF_PAGE_SIZE - 1);
>>  			i++;
>>  		}
>> -
>>  		loaded_segments_num++;
>>  	}
>>  }
> 
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec