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

[Tom Rini]

On Mon, Aug 09, 2021 at 07:34:34AM +0000, Ye Li wrote:
> Hi Marek,
> 
> On Sun, 2021-08-08 at 17:25 +0200, Marek Vasut wrote:
> > Caution: EXT Email
> > 
> > On 8/8/21 4:54 PM, Tom Rini wrote:
> > 
> > [...]
> > 
> > > 
> > > > 
> > > > > 
> > > > > I expect it was not simply because up
> > > > > until rather recently we didn't have any checks for "don't
> > > > > overwrite
> > > > > specific areas of memory" other than right before firing off
> > > > > the OS (and
> > > > > modify whatever memory you want to modify is a feature not a
> > > > > bug).
> > > > The LMB has been around since forever though ?
> > > Yes, LMB has been around since the PowerPC device tree days I
> > > suspect (I
> > > didn't dig that far back), but only used outside of the "don't
> > > overwrite
> > > the running U-Boot while we relocate device tree / initrd before
> > > booting
> > > OS" since 2018 or so.
> > So, are we using LMB for two different things now ?
> > 
> > > 
> > > > 
> > > > [...]
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%
> > > > > > 2F%2Fsource.denx.de%2Fu-boot%2Fcustodians%2Fu-boot-sh%2F-
> > > > > > %2Ftree%2Flmb-
> > > > > > v1&data=04%7C01%7Cye.li%40nxp.com%7Cb9bda480d0494a9249c70
> > > > > > 8d95a80c552%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6376
> > > > > > 40331407737098%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
> > > > > > JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata
> > > > > > =yhIbMHWZMjXy59BVDFVbY2owM7TNdWvk%2B3w2IHg78ok%3D&reserve
> > > > > > d=0
> > > > > > So yes, pick the revert and I'll submit the four patches for
> > > > > > likely next
> > > > > > release.
> > > > > Thanks for explaining, I'll pick up the revert patch then.
> > > > > 
> > > > > For your LMB tree, I like the initial approach but looking at
> > > > > 528915c71762 ("imx: Fix potential lmb memory overwritten by
> > > > > stack") I
> > > > > think that shows the general "4K is enough for stack we hope"
> > > > > is wrong,
> > > > > and we should do 16K instead for everyone as the default.  But
> > > > > we can
> > > > > discuss that more too once you post the whole series which
> > > > > again, I
> > > > > think is the right direction.
> > > > The IMX thing is odd indeed and raises a bigger question -- what
> > > > is the
> > > > "right" amount of stack to reserve ?
> > > It's a good question, yes.  And some more details about what
> > > exactly the
> > > NXP folks were doing to hit that would also be nice.
> > +CC Ye Li.
> 
> On i.MX8QM/QXP, we implement the ft_system_setup to update kernel FDT.
> It needs larger stack size to parse the FDT to disable nodes if the
> corresponding resources are not owned by A core.
> When we enabled the initrd relocation in u-boot, it allocates a space
> from LMB for initrd just before the SP reservation. The stack overflow
> overwrites the initrd and cause kernel issue.
> 
> The size of stack reservation actually depends on the implementation.
> There are lots of board or soc level functions in the boot sequence.
> You can't predict how much stack is needed. So providing a way that can
> adjust the size is useful.

Thanks for explaining.  It sounds like
arch/arm/mach-imx/imx8m/soc.c::the ft_system_setup() needs a comment
that it uses a lot of stack, due to how complex it is, and that
arch/arm/mach-imx/misc.c::board_lmb_reserve() should get moved, and
reworked to just reserve another few kilobytes, for the imx8m case.

-- 
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/20210809/05f7e496/attachment.sig>