Re: [PATCH] TI: include: env: ti_common: move fdtoverlay_addr_r to fix ramdisk size

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Tom,

On lun., oct. 14, 2024 at 18:02, Tom Rini <trini@konsulko.com> wrote:

> On Fri, Oct 11, 2024 at 05:39:53PM +0200, Mattijs Korpershoek wrote:
>> Hi Judith,
>> 
>> Thank you for sending this patch.
>> 
>> On jeu., oct. 10, 2024 at 19:03, Judith Mendez <jm@ti.com> wrote:
>> 
>> > From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> >
>> > When booting Android with adtbo_idx set, we observe the
>> > following crash:
>> 
>> In upstream/master, this is no such thing as adtbo_idx.
>> 
>> This is only present in TI's U-Boot fork, via commit [1]
>> 
>> [1] https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=1c6cf852b8b8f869d2c5e39eb071ec3025bf739a
>> 
>> >
>> >    ** Booting bootflow 'mmc@fa10000.bootdev.whole' with android
>> >    ## Booting Android Image at 0x82000000 ...
>> >    Kernel load addr 0x92000000 size 20195 KiB
>> >    Kernel extra command line: console=ttyS2,115200 cma=768M 8250.
>> > 	nr_uarts=10 printk.devkmsg=on init=/init quiet firmware_class.
>> > 	path=/vendor/firmware mem_sleep_default=deep bootconfig
>> >    RAM disk load addr 0x88080000 size 16901 KiB
>> >    "Synchronous Abort" handler, esr 0x96000005, far 0x155b104c8
>> >    elr: 0000000080808560 lr : 0000000080808558 (reloc)
>> >    elr: 00000000ffebf560 lr : 00000000ffebf558
>> >    x0 : 00000000fff99000 x1 : 00000000fff9553c
>> >    x2 : 000000000000000a x3 : 0000000002800000
>> >    x4 : 0000000002800000 x5 : 0000000000000020
>> >
>> > This happens because the memory at fdtoverlay_addr_r is bogus.
>> > In fact:
>> >
>> > => printenv fdtoverlay_addr_r
>> > fdtoverlay_addr_r=0x89000000
>> >
>> > And the ramdisk address range is:
>> > [0x88080000; 0x88080000 + 16901 KiB]
>> >
>> > Which is equal to:
>> > [0x88080000; 0x89101400]
>> >
>> > So, if we represent the addresses:
>> > fdtaddr             0x88000000
>> > ramdisk             0x88080000
>> > fdtoverlay_addr_r   0x89000000
>> > ramisk (end)        0x89101400
>> >
>> > We see that fdtoverlay_addr_r in fact has been overridden by
>> > the Android ramdisk.
>> >
>> > The maximum ramdisk size is 0x1080000 (15,5 MiB) and a compressed
>> > Android vendor ramdisk is 15MiB:
>> > $ file vendor_ramdisk.img
>> > vendor_ramdisk.img: LZ4 compressed data (v0.1-v0.9)
>> > $ du -sh vendor_ramdisk.img
>> > 15M     vendor_ramdisk.img
>> >
>> > When it gets decompressed, it uses 16.5MiB, exceeding the
>> > maximum ramdisk size.
>> >
>> > Increase the maximum ramdisk size to 20.5MiB by moving
>> > fdtoverlay_addr_r higher up in the address space to fix the crash.
>> 
>> I do think that reserving more room between the fdtoverlay_addr_r and
>> the ramdisk address is probably a good idea for Linux as well, but I
>> think that the commit message might need a rewrite ?
>
> Sorry for the slow reply here. This is I think a case where someone
> needs to step back and think about all of the addresses again, and how
> much gap should be between areas, along with just how big some areas can
> grow to be. There should be some upper reasonable bound on overlays, and
> it will be safer long term I believe to have that above the device tree
> and below the ramdisk. Placing things above the ramdisk always results
> in pain as there's always some new larger image for some valid use case.

ACK.

>
> -- 
> Tom