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

Zou, Nanhai
2007-03-07T04:50:12Z

> -----Original Message-----
> From: Horms [mailto:horms@verge.net.au]
> Sent: 2007年3月7日 11:46
> 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 Wed, Mar 07, 2007 at 10:15:20AM +0800, Zou, Nanhai wrote:
> > > -----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.
>
> I think that the manual option is also important because it maintains
> feature-compatibility with other architectures. I don't consider it
> a hack that might work purely for the purposes of debugging.
>
I don't understand why we need to maintain compatibility with other architectures here. Manfully choose may confuse user, XXX@16M may work on one arch,but not on another arch. Other architectures need manually choose crash kernel region simply because they do not support kernel automatically choose feature.

I keep the XXX@YYY format to just make kdump script compatible, do that distributions does not need to maintain different kdump scripts for different arches.

> With regards to 70 lines of extra code, it can probably be consolidated
> a bit - for insance I think that the ckeck contained in
> kdump_region_verify_rsvd_region() is more important than the other
> checks which I guess could be disposed of if the length of the code
> really is a problem. But in any case the new code is almost entirely in
> __init. So I don't really see that it is a big concern.
>
> As for the partly-overlaped case in kdump_region_verify_rsvd_region().
> I'm not entirely sure what you are getting at there. Are you talking
> about an optimisation to the check, or a correctness problem?
>
(reserve_region.start < base && reserve_region.end < base + size)
or
(reserve_region.end > base && reserve_region.end > base + size) will pass the check

Zou Nan hai

> --
> Horms
> H: http://www.vergenet.net/~horms/
> W: http://www.valinux.co.jp/en/
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html