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@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
![]() |