Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut <marex@denx.de>]

On 8/6/21 6:49 PM, Tom Rini wrote:
> On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
>> On 8/3/21 11:51 PM, Tom Rini wrote:
>>> On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
>>>> On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
>>>>
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
>>>>>
>>>>> While the goal is valid and there is surely unused memory in that area,
>>>>> we also have a lot of crucial things still located at the top-of-memory
>>>>> while running lmb_alloc_base. Such things are the page table (tlb_addr),
>>>>> relocated U-Boot and the active stack. Possibly more. So this patch was
>>>>> premature, we will need relocations of those things first if we want to
>>>>> use the range.
>>>>>
>>>>> Fixes booting on the IOT2050, but likely also on other boards. It got
>>>>> stuck on relocating the FDT - over the relocated U-Boot code.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> Practically,
>>>>>
>>>>> void arch_lmb_reserve(struct lmb *lmb)
>>>>> {
>>>>> 	lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr);
>>>>> }
>>>>>
>>>>> worked as well for me - but it left the stack in danger.
>>>>
>>>> I want to cycle back up to this practically part.  Marek, would changing
>>>> the arch_lmb_reserve (or possibly even making this a more global thing /
>>>> option) still let the area you want exposed on rcar3 (I assume) be
>>>> exposed ?  Or would it be covered again?  Part of the problem,
>>>> practically speaking, is that we need to ensure that as part of
>>>> (attempting and likely succeeding in) booting the OS we don't overwrite
>>>> ourself and hang.
>>
>> I think large part of the problem is the purpose of LMB is unclear at best.
>>
>> The arch_lmb_reserve() says this:
>>
>>   54 void arch_lmb_reserve(struct lmb *lmb)
>> [...]
>>   59 /*
>>   60  * Booting a (Linux) kernel image
>>   61  *
>>   62  * Allocate space for command line and board info - the
>>   63  * address should be as high as possible within the reach of
>>   64  * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused
>>   65  * memory, which means far enough below the current stack
>>   66  * pointer.
>>   67  */
>>
>> So basically reserve chunk of memory in the right place, which can be safely
>> passed to the kernel.
> 
> No.  This isn't the case.  We reserve chunks of memory away from other
> usage by U-Boot.

Then I have to wonder, why is this not called in board_init_f or 
board_init_r , but only after bootm or similar boot command was called ?

>> If that's not the case, and the LMB is used also to protect U-Boot from
>> overwriting itself, then the above comment is totally misleading and wrong
>> (and I have a feeling that is what you are trying to get on).
>>
>> And if that's the case, then what we should reserve isn't
>> stack_bottom...ram_top, but really the work areas of U-Boot only (i.e. a
>> more fine grained reservation might be needed). For starters, replacing
>> ram_top with end of u-boot would do for my use case.
> 
> I addressed this I believe by going back and explaining what the code
> base did at the time this was added, and booting up the image on the
> platform it was tested on at the time.  I think there's a case to be
> made that the comment is a bit misleading / unclear, when it was added.

I would say that is an understatement , but also see above.

> We can see when it was added CONFIG_SYS_BOOTMAPSZ being set to cover the
> start + 0x8000 or so of where ATAGS had traditionally been, and this
> function here covering where U-Boot was in memory, to avoid being
> overwritten by the device tree being relocated.
> 
>>>> An alternative that I'm not 100% sure I like would be adjusting
>>>> env_get_bootm_size() so that the case of bootm_size and bootm_low are
>>>> unset, so we figure out a value based on gd->ram_top we also take in to
>>>> account where U-Boot itself is atm and exclude that.
>>
>> I think we should really clarify the purpose of the LMB and make sure we
>> both understand it the same way.
>>
>>>> It's possible that
>>>> if we had something like that at the start of the DT world, we wouldn't
>>>> have the code to disable fdt relocation, which really feels like it's
>>>> largely been "things crash when we relocate stuff, disable relocation"
>>>> and not so much "save a little boot time, we optimized things VERY
>>>> CAREFULLY".
>>>
>>> Digging more.  The comment about "Allocate space for command line and
>>> board info" was introduced in 2d1916e48bd8 ("ARM: add flat device tree
>>> support").  I happen to have the platform this was tested on, a
>>> Beagleboard, around still and was able to boot that commit up and poke a
>>> bit.  It's effectively covering all of the running U-Boot with an LMB.
>>> You can move on to commit b485556be51d ("ARM: enable device tree for
>>> beagle").  That defines CONFIG_SYS_BOOTMAPSZ which is about start of
>>> DRAM + a bit, to reserve space there that would be questionable on
>>> ARM64.  But all of that code area has been reworked since then.
>>>
>>> So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880cd7 is
>>> right.  U-Boot needs to be protected from being overwritten as it's
>>> still alive at that point and relocating things around.  This has always
>>> been true.  This has always been related to device tree booting.
>>>
>>> What rcar is trying to do needs to be better explained first so that we
>>> can figure out the right place to make some sort of update.
>>
>> rcar might not relocate u-boot all the way to the end of DRAM, because there
>> might be some reserved memory at the end of DRAM used by one of the other
>> CPUs in the SoC, that's all.
> 
> OK, so then there isn't a problem reverting this commit for rcar?

The revert will break the use case where the other CPUs are using memory 
above U-Boot, but have a look at the following branch, it should permit 
me to parametrize the arch_lmb_reserve() better and reserve the right 
memory areas per architecture/mach/board, and even clean the 
arch_lmb_reserve up further:
https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1
So yes, pick the revert and I'll submit the four patches for likely next 
release.