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

[chenjiahao (C)]

On 2023/3/20 22:36, Simon Horman wrote:
> On Tue, Mar 21, 2023 at 04:42:43AM +0800, Chen Jiahao wrote:
> > On riscv, the current crash kernel allocation logic is trying to
> > allocate within 32bit addressible memory region by default, if
> > failed, try to allocate without 4G restriction.
> >
> > In need of saving DMA zone memory while allocating a relatively large
> > crash kernel region, allocating the reserved memory top down in
> > high memory, without overlapping the DMA zone, is a mature solution.
> > Here introduce the parameter option crashkernel=X,[high,low].
> >
> > We can reserve the crash kernel from high memory above DMA zone range
> > by explicitly passing "crashkernel=X,high"; or reserve a memory range
> > below 4G with "crashkernel=X,low".
> >
> > Signed-off-by: Chen Jiahao <chenjiahao16@xxxxxxxxxx>
> Some minor nits, but I don't think there is any need to respin for them.
>
> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>

I will send another version and clean up these by the way, thanks.

Chen, Jiahao


> ...
>
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 478d6763a01a..5def2174b243 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> ...
>
> > @@ -1201,16 +1242,25 @@ 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, (unsigned long) dma32_phys_limit));
> nit: While here, you could drop the space before 'ma32_phys_limit'.
>       Or perhaps use min_t, which seems appropriate here.
>
> >   	if (crash_base == 0) {
> >   		/* Try again without restricting region to 32bit addressible memory */
> >   		crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > -						search_start, search_end);
> > +						       search_start, search_end);
> >   		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)) {
> nit: The line above should be aligned one character to the left
>       (remove one space in the indent).
>
> > +		memblock_phys_free(crash_base, crash_size);
> > +		return;
> >   	}
> >   
> >   	pr_info("crashkernel: reserved 0x%016llx - 0x%016llx (%lld MB)\n",
> ...