Re: [Patch v3] mm/memblock: remove empty dummy entry

[Mike Rapoport <rppt@kernel.org>]

(added microblaze maintainer)

On Fri, Jun 21, 2024 at 09:11:59PM -0700, Guenter Roeck wrote:
> On 6/21/24 16:06, Wei Yang wrote:
> > On Thu, Jun 20, 2024 at 07:34:06PM -0700, Guenter Roeck wrote:
> > > On 6/20/24 18:07, Wei Yang wrote:
> > > > On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Apr 05, 2024 at 01:58:21AM +0000, Wei Yang wrote:
> > > > > > The dummy entry is introduced in the initial implementation of lmb in
> > > > > > commit 7c8c6b9776fb ("powerpc: Merge lmb.c and make MM initialization
> > > > > > use it.").
> > > > > > 
> > > > > > As the comment says the empty dummy entry is to simplify the code.
> > > > > > 
> > > > > > 	/* Create a dummy zero size LMB which will get coalesced away later.
> > > > > >            * This simplifies the lmb_add() code below...
> > > > > >            */
> > > > > > 
> > > > > > While current code is reimplemented by Tejun in commit 784656f9c680
> > > > > > ("memblock: Reimplement memblock_add_region()"). This empty dummy entry
> > > > > > seems not benefit the code any more.
> > > > > > 
> > > > > > Let's remove it.
> > > > > > 
> > > > > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > > > > > CC: Paul Mackerras <paulus@ozlabs.org>
> > > > > > CC: Tejun Heo <tj@kernel.org>
> > > > > > CC: Mike Rapoport <rppt@kernel.org>
> > > > > > 
> > > > > 
> > > > > With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
> > > > > fixes the problem.
> > > 
> > > This is a silent failure. There is no console output, so the image crashes
> > > before it gets to that point.
> > > 
> > > Building microblaze:petalogix-s3adsp1800:initrd ... running ................R............. failed (silent)
> > > ------------
> > > qemu log:
> > > qemu-system-microblaze: terminating on signal 15 from pid 2343410 (/bin/bash)
> > 
> > Would you mind add kernel parameter memblock=debug without the commit applied?
> > If my understanding is correct, you can bootup without this commit, right?
> 
> Yes. With this change on top of linux-next:
> 
> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> index 3827dc76edd8..1d3edac064ee 100644
> --- a/arch/microblaze/mm/init.c
> +++ b/arch/microblaze/mm/init.c
> @@ -195,7 +195,9 @@ asmlinkage void __init mmu_init(void)
> 
>         if (!memblock.reserved.cnt) {

I tried to understand what this was supposed to check, but this test was
there from day 1, so git archaeology didn't help :(

Anyway, it's perfectly fine to have any number of memblock reservations
here or no at all.

early_init_devtree() is called before mmu_init() and it sets up
memblock.memory via early_init_dt_scan() and even allows memblock
allocations. So the check for !memblock.reserved.cnt here seems wrong.

>                 pr_emerg("Error memory count\n");
> +#if 0
>                 machine_restart(NULL);
> +#endif
>         }
> 
> the log starts with:
> 
> random: crng init done
> Ramdisk addr 0x51923788,
> FDT at 0x51f43d88
> Error memory count
> MEMBLOCK configuration:
>  memory size = 0x10000000 reserved size = 0x015bb600
>  memory.cnt  = 0x1
>  memory[0x0]    [0x50000000-0x5fffffff], 0x10000000 bytes flags: 0x0
>  reserved.cnt  = 0x3
>  reserved[0x0]  [0x50000000-0x50f5cfff], 0x00f5d000 bytes flags: 0x0
>  reserved[0x1]  [0x510c0000-0x510fffff], 0x00040000 bytes flags: 0x0
>  reserved[0x2]  [0x51923788-0x51f41d87], 0x0061e600 bytes flags: 0x0
> Linux version 6.10.0-rc4-next-20240620-dirty (groeck@server.roeck-us.net) (microblaze-linux-gcc (GCC) 11.4.0, GNU ld (GNU Binutils) 2.40) #1 Fri Jun 21 21:00:40 PDT 2024
> setup_memory: max_mapnr: 0x10000
> setup_memory: min_low_pfn: 0x50000
> setup_memory: max_low_pfn: 0x60000
> setup_memory: max_pfn: 0x60000
> memblock_alloc_try_nid: 4096 bytes align=0x1000 nid=-1 from=0x00000000 max_addr=0x51100000 pte_alloc_one_kernel+0x50/0x64
> memblock_reserve: [0x510bf000-0x510bffff] memblock_alloc_range_nid+0x104/0x1d4
> 
> Guenter

I think simply removing the check for !memblock.reserved.cnt is the right
thing to do here:

From 975c5ba011330238444c82d0b171779c2156d2dc Mon Sep 17 00:00:00 2001
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Date: Sat, 22 Jun 2024 20:46:36 +0300
Subject: [PATCH] microblaze: don't treat zero reserved memory regions as error

Before commit 721f4a6526da ("mm/memblock: remove empty dummy entry") the
check for non-zero of memblock.reserved.cnt in mmu_init() would always
be true either because  memblock.reserved.cnt is initialized to 1 or
because there were memory reservations earlier.

The removal of dummy empty entry in memblock caused this check to fail
because now memblock.reserved.cnt is initialized to 0.

Remove the check for non-zero of memblock.reserved.cnt because it's
perfectly fine to have an empty memblock.reserved array that early in
boot.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
 arch/microblaze/mm/init.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
index 3827dc76edd8..4520c5741579 100644
--- a/arch/microblaze/mm/init.c
+++ b/arch/microblaze/mm/init.c
@@ -193,11 +193,6 @@ asmlinkage void __init mmu_init(void)
 {
 	unsigned int kstart, ksize;
 
-	if (!memblock.reserved.cnt) {
-		pr_emerg("Error memory count\n");
-		machine_restart(NULL);
-	}
-
 	if ((u32) memblock.memory.regions[0].size < 0x400000) {
 		pr_emerg("Memory must be greater than 4MB\n");
 		machine_restart(NULL);
-- 
2.43.0


-- 
Sincerely yours,
Mike.