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. Regards, jay > > Cc: Jay Lan <jlan at sgi.com> > Signed-off-by: Simon Horman <horms at verge.net.au> > > Index: kexec-tools/kexec/arch/ia64/crashdump-ia64.c > =================================================================== > --- kexec-tools.orig/kexec/arch/ia64/crashdump-ia64.c 2008-10-08 17:31:42.000000000 +1100 > +++ kexec-tools/kexec/arch/ia64/crashdump-ia64.c 2008-10-08 17:32:08.000000000 +1100 > @@ -91,7 +91,7 @@ static void add_loaded_segments_info(str > if (phdr->p_type != PT_LOAD) > break; > if (loaded_segments[loaded_segments_num].end != > - phdr->p_paddr & ~(ELF_PAGE_SIZE-1)) > + (phdr->p_paddr & ~(ELF_PAGE_SIZE-1))) > break; > loaded_segments[loaded_segments_num].end += > (phdr->p_memsz + ELF_PAGE_SIZE - 1) & >
