On Sat, Jun 22, 2024 at 09:16:23PM +0300, Mike Rapoport wrote:
>(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:
>
>>From 975c5ba011330238444c82d0b171779c2156d2dc Mon Sep 17 00:00:00 2001
>From: "Mike Rapoport (IBM)" <rppt@xxxxxxxxxx>
>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@xxxxxxxxxxxx>
>Signed-off-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
Thanks Mike :-)
Reviewed-by: Wei Yang <richard.weiyang@xxxxxxxxx>
>---
> 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.
--
Wei Yang
Help you, Help me
