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? From: Jay Lan <jlan@xxxxxxx> 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@xxxxxxx> --- 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++; } } -- To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html