Simon Horman wrote:
> On Tue, Oct 21, 2008 at 09:52:27AM -0700, Jay Lan wrote:
>> Simon Horman wrote:
>>
>> Hi Simon,
>>
>> Just got back from vacation. Sorry for late response.
>>
>>> This bug was discovered by Jay Lan and he also proposed this fix, however
>>> thee is some discussion about what if any related changes should be made at
>>> the same time.
>>>
>>> The bug comes about because the break statment was never executed because
>>> the if clause would bever be true because the if clause will never be true
>>> because & has higher precedence than !=.
>>>
>>> My position on this is that with the if logic fixed, as per this patch, the
>>> break statment and the rest of the while() loop makes sense and should work
>>> as intended.
>>>
>>> As I understand it, Jay's position is that the code should be simplified,
>>> after all it never worked as intended.
>>>
>>> There is a related kernel bug that lead Jay to discover this problem.
>>> The kernel bug has been resolved by Tony Luck and was
>>> included in Linus's tree between 2.6.27-rc8 and 2.6.27-rc9 as
>>> "[IA64] Put the space for cpu0 per-cpu area into .data section".
>>>
>>> Now that the kernel bug is out of the way, I am providing this patch to
>>> continue discussion on what to do on the kexec-tools side of things. I do
>>> not intend to apply this patch until there is some conclusion in the
>>> discussion between Jay and myself.
>> I think this patch is not right for two reasons:
>> 1) The if-statement below has never proved the correctness of
>> its intent because the 'break' statement never got executed
>> due to a logic error.
>> if (loaded_segments[loaded_segments_num].end !=
>> (phdr->p_paddr & ~(ELF_PAGE_SIZE-1)))
>> break;
>> 2) With your patch in my testing, the kdump kernel boot hung
>> even earlier in a PAL_CALL that was not returned to the kernel.
>> I understand that my test case was based on a kernel without
>> Tony's latest fix, but that was the only situation we can
>> see the if-statement becomes true. I do not know any other
>> way to make a memory gap happen. However, when it happens,
>> your patch only makes kdump kenrel boot hang earlier.
>>
>> I still root for my patch because the kdump kernel would boot
>> correctly even if a memory gap indeed happened. ;) However,
>> if you do not feel comfortable with my patch, i think the best
>> alternative is to take out the if-statement above completely.
>
> Point taken, just to clarify, this is the patch you would like merged?
Hi Simon,
Yes. This is the patch that i would like merged.
Thanks,
jay
>
>
> From: Jay Lan <jlan at sgi.com>
>
> IA64: better calculate PT_LOAD segment size
>
> 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 at sgi.com>
>
> ---
> 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++;
> }
> }
>
