Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <tencent_79F42B5A66F98A65266F989EC9C86A69F005@qq.com>
Date:   Thu, 8 Jun 2023 06:17:22 +0800
From:   Woody Zhang <woodylab@foxmail.com>
To:     Conor Dooley <conor@kernel.org>
Cc:     Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        alexghiti@rivosinc.com
Subject: Re: [PATCH] riscv: reserve DTB before possible memblock allocation
References: <tencent_B15C0F1F3105597D0DCE7DADC96C5EB5CF0A@qq.com>
 <20230607-jogging-grudging-70dede86bc53@spud>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230607-jogging-grudging-70dede86bc53@spud>
Precedence: bulk

Hi, Conor

On Wed, Jun 07, 2023 at 07:17:28PM +0100, Conor Dooley wrote:
>+CC Alex, you should take a look at this patch.
>
>On Wed, Jun 07, 2023 at 09:35:19PM +0800, Woody Zhang wrote:
>> It's possible that early_init_fdt_scan_reserved_mem() allocates memory
>> from memblock for dynamic reserved memory in `/reserved-memory` node.
>> Any fixed reservation must be done before that to avoid potential
>> conflicts.
>> 
>> Reserve the DTB in memblock just after early scanning it.
>
>The rationale makes sense to me, I am just wondering what compelling
>reason there is to move it away from the memblock_reserve()s for the
>initd and vmlinux? Moving it above early_init_fdt_scan_reserved_mem()
>should be the sufficient minimum & would keep things together.

IMO, moving it to parse_dtb() is more reasonable as early scanning and
reservation are both subject to DTB. It can also lower the risk to
mess up the sequence in the future. BTW, it's also invoked in
setup_machine_fdt() in arm64.

Thanks,
Woody

>
>Cheers,
>Conor.
>
>> 
>> Signed-off-by: Woody Zhang <woodylab@foxmail.com>
>> ---
>>  arch/riscv/kernel/setup.c | 10 ++++++++++
>>  arch/riscv/mm/init.c      |  9 ---------
>>  2 files changed, 10 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index 36b026057503..c147fa8da929 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/console.h>
>>  #include <linux/screen_info.h>
>>  #include <linux/of_fdt.h>
>> +#include <linux/libfdt.h>
>>  #include <linux/sched/task.h>
>>  #include <linux/smp.h>
>>  #include <linux/efi.h>
>> @@ -256,6 +257,15 @@ static void __init parse_dtb(void)
>>  		pr_err("No DTB passed to the kernel\n");
>>  	}
>>  
>> +	/*
>> +	 * If DTB is built in, no need to reserve its memblock.
>> +	 * Otherwise, do reserve it but avoid using
>> +	 * early_init_fdt_reserve_self() since __pa() does
>> +	 * not work for DTB pointers that are fixmap addresses
>> +	 */
>> +	if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
>> +		memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
>> +
>>  #ifdef CONFIG_CMDLINE_FORCE
>>  	strscpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>>  	pr_info("Forcing kernel command line to: %s\n", boot_command_line);
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index c6bb966e4123..f8c9a79acd94 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -254,15 +254,6 @@ static void __init setup_bootmem(void)
>>  	 */
>>  	early_init_fdt_scan_reserved_mem();
>>  
>> -	/*
>> -	 * If DTB is built in, no need to reserve its memblock.
>> -	 * Otherwise, do reserve it but avoid using
>> -	 * early_init_fdt_reserve_self() since __pa() does
>> -	 * not work for DTB pointers that are fixmap addresses
>> -	 */
>> -	if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
>> -		memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
>> -
>>  	dma_contiguous_reserve(dma32_phys_limit);
>>  	if (IS_ENABLED(CONFIG_64BIT))
>>  		hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT);
>> -- 
>> 2.39.2
>>