On 05/11/23 at 04:47pm, chenjiahao (C) wrote:
......
> > > @@ -1163,8 +1185,12 @@ static void __init reserve_crashkernel(void)
> > > {
> > > unsigned long long crash_base = 0;
> > > unsigned long long crash_size = 0;
> > > + unsigned long long crash_low_size = 0;
> > > unsigned long search_start = memblock_start_of_DRAM();
> > > unsigned long search_end = memblock_end_of_DRAM();
> > > + unsigned long search_low_max = (unsigned long)dma32_phys_limit;
> > > + char *cmdline = boot_command_line;
> > > + bool fixed_base = false;
> > > int ret = 0;
> > > @@ -1180,14 +1206,34 @@ static void __init reserve_crashkernel(void)
> > > return;
> > > }
> > > - ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> > > + ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
> > > &crash_size, &crash_base);
> > > - if (ret || !crash_size)
> > > + if (ret == -ENOENT) {
> > > + /* Fallback to crashkernel=X,[high,low] */
> > > + ret = parse_crashkernel_high(cmdline, 0, &crash_size, &crash_base);
> > > + if (ret || !crash_size)
> > > + return;
> > > +
> > > + /*
> > > + * crashkernel=Y,low is valid only when crashkernel=X,high
> > > + * is passed.
> > > + */
> > > + ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base);
> > > + if (ret == -ENOENT)
> > > + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> > > + else if (ret)
> > > + return;
> > > +
> > > + search_start = search_low_max;
> > > + } else if (ret || !crash_size) {
> > > + /* Invalid argument value specified */
> > > return;
> > > + }
> > > crash_size = PAGE_ALIGN(crash_size);
> > > if (crash_base) {
> > > + fixed_base = true;
> > > search_start = crash_base;
> > > search_end = crash_base + crash_size;
> > > }
> > > @@ -1201,16 +1247,31 @@ static void __init reserve_crashkernel(void)
> > > */
> > > crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > > search_start,
> > > - min(search_end, (unsigned long) SZ_4G));
> > > + min(search_end, search_low_max));
> > Here, it seems not right in case crashkernel=,high is specified. In that
> > case, search_start == search_low_max, then the min(search_end,
> > search_low_max) will get search_low_max too. Then you make the fallback
> > in below code block to try to get crashkernel reservation above 4G. This
> > doesn't comply with the crashkernel=,high grammer which has been
> > implemented in other architectures.
> >
> > For crashkernel=,high, user explicitly require memory reservation above
> > 4G. Why does crashkernel=,high is needed? E.g on big end server with
> > huge memory, while the low memory under 4G is limited and precious.
> > Hence, user want to put the main crashkernel reservation above 4G to
> > contain kdump kernel/initrd and run user space program, while with few
> > low memory for pci device driver. E.g crashkernel=2G,high, it won't
> > impact much if there's huge memory above 4G and get crashkernel
> > reservation there. However, it impacts a lot if it reserves memory
> > below 4G.
> >
> > I would strongly suggest that risc-v also reserve memory from above 4G
> > for crashkernel=,high, then fallback to below 4G. That's consistent with
> > crashkernel=,high grammer.
>
> Sorry for late response.
>
> I have got the point here. So with the original implication of "crashkernel=,high",
> there is even no need to try reserving low memory under 4G. I have arranged another
> version of patchset, in which I updated the allocation logic in that case.
>
> For example, when "crashkernel=1G,high" is specified, the previous logic is like:
> alloc range: crash_size: 0x40000000 (1G), crash_base: 4G_limit,
> crash_max: 4G_limit
> alloc range high: crash_size: 0x40000000 (1G), crash_base: 4G_limit,
> crash_max: memblock_range_end
> alloc range low: low_size: 0x8000000 (128MB,default), crash_base: 0x0,
> crash_max: 4G_limit
>
> After revision, the logic is like:
> alloc range: crash_size: 0x40000000 (1G), crash_base: memblock_range_start,
> crash_max: memblock_range_end
> alloc range low: low_size: 0x8000000 (128MB,default), crash_base: 0x0,
> crash_max: 4G_limit
>
> Please let me know if there is any problem exist.
Sorry for late reply.
Hmm, it doesn't seem completely correct. I will comment in your v5
patch. Please see over there.
