RE: [patch 3/3] IA64: verify the base address of crashkernel

Zou, Nanhai
2007-03-07T02:15:20Z

> -----Original Message-----
> From: Horms [mailto:horms@verge.net.au]
> Sent: 2007Äê3ÔÂ7ÈÕ 8:50
> To: Zou, Nanhai
> Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
>
> On Tue, Mar 06, 2007 at 04:23:37PM +0800, Zou, Nanhai wrote:
> >
> > Hi Horms,
> > I feel this is over-designed.
> > I think to specify crash kernel base address in command line is only
> useful for debug, on platform like SN this feature is totally unusable.At the
> most of time, user should let kernel to decide where to reserve crashdump
> region.
> > If a user wants to put crash kernel in command line, he should know what
> he is doing.
>
> Hi Nanhai,
>
> while I do agree that perhaps these checks are a little verbose I don't
> agree that they are uneccessary. Specifying the base address is entirely
> sane on some platforms (e.g. Tiger 2). And more to the point, it is the
> only method available on some architectures, and thus its seems
> reasonable to expect that it might work sanley on ia64. It seems to me
> that it is a good idea to have some checks in place, in line with the
> checks performed when the base address is automatically determinted to
> make the behaviour (more) consistent.
>
> Ideally it would be good if there were not two code-paths relating
> to base address selection - auto and manual. Or more to the point, if
> they could share the same checks. But at this point I can't see a way to
> make the code do that.
>
> I guess in the end it comes down to how easy you want it to be for users
> mistakes to be caught. I think that currently kexec/kdump is quite
> fragile and its easy to end up with a setup that doesn't work. I think
> that changes like this one are one small step towards making a more
> robust system. Ditto for the change regarding loging success or failure
> of inserting the crashkernel region.
>
Hi,
I think even in the situation of manually pick address, user can check /proc/iomem to found the address, it is easy.
I don't see in which situation like kernel automatically determined method does not work but manually pick address would work. So I think manually pick address could only be help for debug. But I think it is good to have this feature since it only cost 2 lines of code.
I believe it is good to keep the kernel code simple and clean.
We do not need to add more than 70 lines of code to kernel only for debug print.
You can add those prints to kernel whenever you want to debug something, but put those in vanilla kernel is not a good idea.
BTW: the code logic in your updated patch is still not correct, in kdump_region_verify_rsvd_region you do not check the partly overlap situation.

Thanks
Zou Nan hai
> --
> Horms
> H: http://www.vergenet.net/~horms/
> W: http://www.valinux.co.jp/en/