(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@xxxxxxxxx>
> > > > > > CC: Paul Mackerras <paulus@xxxxxxxxxx>
> > > > > > CC: Tejun Heo <tj@xxxxxxxxxx>
> > > > > > CC: Mike Rapoport <rppt@xxxxxxxxxx>
> > > > > >
> > > > >
> > > > > 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@xxxxxxxxxxxxxxxxxxx) (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:
