On 09/18/15 at 03:11pm, Petr Tesarik wrote:
> On Fri, 18 Sep 2015 19:39:02 +0800
> Minfei Huang <mhuang at redhat.com> wrote:
>
> > On 09/18/15 at 08:03am, Petr Tesarik wrote:
> > > Hello,
> > >
> > > There may be more than one crash kernel regions on x86. However, the kexec
> > > syscall checks that target address is within crashk_res boundaries. Looking
> > > at the logic in arch/x86/kernel/setup.c, there are only two possible layouts:
> > >
> > > 1. crashk_res is below 4G, and there is only one region,
> > > 2. crashk_res is above 4G, and crashk_low_res is below 4G
> > >
> > > In either case, kexec-tools must pick the highest region.
> > >
> > > Currently, kexec-tools picks the largest region. If high reservation is
> > > smaller than low, kexec(2) returns -EADDRNOTAVAIL, and kexec prints out
> > > this error message:
> > >
> > > kexec_load failed: Cannot assign requested address
> > >
> > > Signed-off-by: Petr Tesarik <ptesarik at suse.com>
> > > ---
> > > kexec/arch/i386/crashdump-x86.c | 12 ++----------
> > > 1 file changed, 2 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > index 63959b7..2710a9e 100644
> > > --- a/kexec/arch/i386/crashdump-x86.c
> > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > @@ -1034,16 +1034,8 @@ int get_max_crash_kernel_limit(uint64_t *start, uint64_t *end)
> > > if (!crash_reserved_mem_nr)
> > > return -1;
> > >
> > > - for (i = crash_reserved_mem_nr - 1; i >= 0; i--) {
> > > - sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> > > - if (sz <= sz_max)
> > > - continue;
> > > - sz_max = sz;
> > > - idx = i;
> > > - }
> > > -
> > > - *start = crash_reserved_mem[idx].start;
> > > - *end = crash_reserved_mem[idx].end;
> > > + *start = crash_reserved_mem[crash_reserved_mem_nr - 1].start;
> > > + *end = crash_reserved_mem[crash_reserved_mem_nr - 1].end;
> > >
> > > return 0;
> >
> > It is not a proper function name for what it does in your patch. How
>
> True. It should be now called something like get_crashk_res_location().
> I'm going to change that.
>
> > about get_reserved_mem_limit()? Also it is appreciative, if you can
> > comment it more detail.
>
> I'm not sure what would be more detail. Regarding the motivation, I
> have already documented why it fails and what are the symptoms of
> failure. Regarding the implementation, the code speaks for itself, IMO.
>
> Can you explain what kind of detail you're missing, please?
Sorry, What I mean is to comment above the function why we choose the
region crash_reserved_mem[crash_reserved_mem_nr - 1].
Thanks
Minfei
