On 3/28/07, Vivek Goyal <[EMAIL PROTECTED]> wrote: > On Wed, Mar 28, 2007 at 03:55:24PM +0900, Magnus Damm wrote: > > > > This means that we will end up with many architecture-specific > > differences that may not be needed but they are there because no one > > really had enough knowledge or time to see the big picture. > > > > So, I think we should solve this by a simple and well documented fix > > now. And long-term we can try to share the /proc/kcore code between > > ia64 and x86_64. > > > > Probably we can share that code later, but atleast for the time being > we should either replicate that kcore reading code or right now perform > the step of calculating the virtual address by adding physical address > in arch dependent section.
Yes, putting things in the arch-dependent code sounds like the best choice. I'm a bit reluctant to duplicate the kcore code - that is more of a feature than a fix IMO. We want to be at least as good as the last kexec-tools release and currently it looks like we are unsuccessful with that mission on ia64. > > >I think in arch dependent code you should set > > >kern_vaddr=LOAD_OFFSET + kernel_physical start. > > > > Right. Patches are welcome. =) > > > > I don't understand that what's the point of giving the review comments > on a particular patch if a person is not willing to change the patch. So > only way to give review comments is that one should also send a patch > otherwise don't talk about it. :-( First of all, I really appreciate your comments Vivek. Secondly, the patch was just my stab at fixing up the original patch written by Jonathan. I see this as an iterative process so if anyone has any better patch/idea then I'd be happy to go with that instead. And I may of course come up with a better alternative myself - thanks to your comments - but I'm unfortunately a bit low on time. And that's the reason why I asked for patches. Sorry if that sounded like I wasn't willing to change the patch. > It should not be that big a patch that we should keep it a future item > and right now put a patch which is not intitutive at all. Right. Let's come up with a small arch-specific patch that we all feel good about. =) > For kexec-tools-testing there are no hard deadlines for any release. If > we delay it by a day or two just becuase we can put a better patch which > everybody can understand down the line, I will definitely go by that. But > the argument, I understand that we are not doing the right thing but > we will still do it and if somebody has objections, he should provide a > patch, I am not sure I understand that. You are right that there are no hard deadlines for the kexec-tools-testing release. The only limitation for me is that I have my last working day in the office on Friday and I sort of wanted to have the release done by then. I think we can have both when it comes to long-term and short-term. Duplicating the /proc/kcore code is wrong IMO, if we should add /proc/kcore support for IA64 then it should be done correctly - ie the same code base should be used for x86_64. This will unfortunately take longer time than a few days so someone else than me will have to work on that. I propose that we postpone the /proc/kcore change and go with a simple IA64-specific for now. But OTOH, ia64 kdump seems to be broken at the moment. So adding /proc/kcore support will not make things any worse. Except we have more lines of duplicated code... > Why I am pressing so hard for this change is that I am more concerned > about that at least logic of something should be clear. By looking at > the patch, one can't make any guesses about what kern_offset field > is and why it is required. I agree that it's important to keep sane logic and by that I realize that my patch may be a step in the wrong direction. I saw it as a simple fix to unbreak ia64 and get the release out of the door. The best solution is probably to keep on using kern_vaddr_start and perform the offset calculation in the crashdump-ia64.c. Jonathan, if I repost a better patch, do you have time to test? Thanks! / magnus _______________________________________________ fastboot mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/fastboot
