Re: [PATCH] arm64: Do not defer reserve_crashkernel() for platforms with no DMA memory zones

[Vijay Balakrishna]

On 2/17/2022 2:49 AM, nicolas saenz julienne wrote:
> On Wed, 2022-02-16 at 16:04 -0800, Vijay Balakrishna wrote:
> > The following patches resulted in deferring crash kernel reservation to
> > mem_init(), mainly aimed at platforms with DMA memory zones (no IOMMU),
> > in particular Raspberry Pi 4.
> >
> > commit 1a8e1cef7603 ("arm64: use both ZONE_DMA and ZONE_DMA32")
> > commit 8424ecdde7df ("arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges")
> > commit 0a30c53573b0 ("arm64: mm: Move reserve_crashkernel() into mem_init()")
..
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index acfae9b41cc8..e7faf5edccfc 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -517,7 +517,7 @@ static void __init map_mem(pgd_t *pgdp)
> >   	 */
> >   	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
> >   
> > -	if (can_set_direct_map() || crash_mem_map || IS_ENABLED(CONFIG_KFENCE))
> > +	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
> >   		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >   
> >   	/*
> > @@ -528,6 +528,14 @@ static void __init map_mem(pgd_t *pgdp)
> >   	 */
> >   	memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
> >   
> > +#ifdef CONFIG_KEXEC_CORE
> > +	if (crash_mem_map && !crashk_res.end)
> > +		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>
> Using IS_ENABLED(ZONE_DMA/DMA32) instead of '!crashk_res.end' would be more
> efficient and a bit more explicit IMO.

Sure, I will make change in a follow up submission.

> >   	/* map all the memory banks */
> >   	for_each_mem_range(i, &start, &end) {
> >   		if (start >= end)
> > @@ -554,6 +562,20 @@ static void __init map_mem(pgd_t *pgdp)
> >   	__map_memblock(pgdp, kernel_start, kernel_end,
> >   		       PAGE_KERNEL, NO_CONT_MAPPINGS);
> >   	memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
> > +#ifdef CONFIG_KEXEC_CORE
> > +	/*
> > +	 * Use page-level mappings here so that we can shrink the region
> > +	 * in page granularity and put back unused memory to buddy system
> > +	 * through /sys/kernel/kexec_crash_size interface.
> > +	 */
> > +	if (crashk_res.end) {
>
> Same here.

Yes.

> > +		__map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
> > +			       PAGE_KERNEL,
> > +			       NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> > +		memblock_clear_nomap(crashk_res.start,
> > +				     resource_size(&crashk_res));
> > +	}
> > +#endif
>
> Now, I carefully reviewed the patch and it seems to be doing the right thing.
> But even while knowlegable on the topic, it took a good amount of effort to
> untangle the possible code paths. I suspect it's going to be painful to
> maintain. I'd suggest at least introducing a comment explaining the situation.

I appreciate your review.  Yes, it took a good amount of time for me 
(new here) too and glad for your notice.  Let me take a shot at 
explaining in my next revision.
> If there approach if deemed acceptable, I'll test is on the RPi4.

Please, your testing on RPi4 would be valuable.

Thanks,
Vijay

> Regards,
> Nicolas