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

[Tom Rini]

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 at 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 at 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.

> 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.
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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210806/cc18511e/attachment.sig>