Re: [PATCH -next v5 1/2] riscv: kdump: Implement crashkernel=X,[high,low]

[chenjiahao (C)]

On 2023/6/4 11:50, Baoquan He wrote:
> Hi Jiahao,
>
> On 05/11/23 at 04:51pm, Chen Jiahao wrote:
> ......
> > @@ -1300,14 +1325,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_end = memblock_end_of_DRAM();
> > +	} else if (ret || !crash_size) {
> > +		/* Invalid argument value specified */
> >   		return;
> > +	}
> The parsing part looks great, while you didn't mark if it's specified
> high reservation, please see later comment why it's needed.
>
> >   
> >   	crash_size = PAGE_ALIGN(crash_size);
> >   
> >   	if (crash_base) {
> > +		fixed_base = true;
> >   		search_start = crash_base;
> >   		search_end = crash_base + crash_size;
> >   	}
> > @@ -1320,17 +1365,31 @@ static void __init reserve_crashkernel(void)
> >   	 * swiotlb can work on the crash kernel.
> >   	 */
> >   	crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > -					       search_start,
> > -					       min(search_end, (unsigned long) SZ_4G));
> > +					       search_start, search_end);
> If it's a specified high reservation, you have
> search_start = memblock_start_of_DRAM();
> search_end = memblock_end_of_DRAM();
>
> Then it attempts to search top down first time here.
>
> >   	if (crash_base == 0) {
> > -		/* Try again without restricting region to 32bit addressible memory */
> > +		if (fixed_base) {
> > +			pr_warn("crashkernel: allocating failed with given size@offset\n");
> > +			return;
> > +		}
> > +		search_end = memblock_end_of_DRAM();
> > +
> > +		/* Try again above the region of 32bit addressible memory */
> >   		crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > -						search_start, search_end);
> > +						       search_start, search_end);
> If crashkernel=,high case, the first attempt failed, here it assigns
> search_end with memblock_end_of_DRAM(). It's the exactly the same
> attempt, why is that needed? Why don't you use a local variable 'high'
> to mark the crashkernel=,hig, then judge when deciding how to adjsut the
> reservation range.
>
> Do I misunderstand the code?
>
> Thanks
> Baoquan

You are right. Here I use search_end = memblock_end_of_DRAM() for the
first attempt on "crashkernel=,high" case, but it will not distinct from
other cases if the first attempt fails.

I have read your latest refactor on Arm64, introducing the "high" flag
is a good choice, the logic gets more straightforward when handling
crashkernel=,high case and retrying.

Following that logic, here introducing and set "high" flag when parsing
cmdline, when the first attempt failed:

if fixed_base:
    failed and return;

if set high:
    search_start = memblock_start_of_DRAM();
    search_end = (unsigned long)dma32_phys_limit;
else:
    search_start = (unsigned long)dma32_phys_limit;
    search_end = memblock_end_of_DRAM();

second attempt with new {search_start, search_end}
...

This should handle "crashkernel=,high" case correctly and avoid cross
4G reservation.

Is that logic correct, or is any other problem missed?

Thanks,
Jiahao

> >   		if (crash_base == 0) {
> >   			pr_warn("crashkernel: couldn't allocate %lldKB\n",
> >   				crash_size >> 10);
> >   			return;
> >   		}
> > +
> > +		if (!crash_low_size)
> > +			crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> > +	}
> > +
> > +	if ((crash_base > dma32_phys_limit - crash_low_size) &&
> > +	    crash_low_size && reserve_crashkernel_low(crash_low_size)) {
> > +		memblock_phys_free(crash_base, crash_size);
> > +		return;
> >   	}
> >   
> >   	pr_info("crashkernel: reserved 0x%016llx - 0x%016llx (%lld MB)\n",
> > --
> > 2.31.1