Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <a22c9bed-3cc3-e2c0-7eb2-1a19d64af574@linux.microsoft.com>
Date:   Thu, 17 Feb 2022 10:26:14 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH] arm64: Do not defer reserve_crashkernel() for platforms
 with no DMA memory zones
Content-Language: en-US
To:     nicolas saenz julienne <nsaenz@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>
Cc:     Anshuman Khandual <anshuman.khandual@arm.com>,
        Mike Rapoport <rppt@kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Pavel Tatashin <pasha.tatashin@soleen.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <1645056294-6509-1-git-send-email-vijayb@linux.microsoft.com>
 <ed4f4589f322bd3871f825239985410b535df30e.camel@kernel.org>
From:   Vijay Balakrishna <vijayb@linux.microsoft.com>
In-Reply-To: <ed4f4589f322bd3871f825239985410b535df30e.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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