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

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

[Judith Mendez]

From: Mattijs Korpershoek <mkorpershoek@baylibre.com>

When booting Android with adtbo_idx set, we observe the
following crash:

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

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Judith Mendez <jm@ti.com>
---
 include/env/ti/ti_common.env | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/env/ti/ti_common.env b/include/env/ti/ti_common.env
index 7029d12bf20..a15af79d9c3 100644
--- a/include/env/ti/ti_common.env
+++ b/include/env/ti/ti_common.env
@@ -3,7 +3,7 @@ kernel_addr_r=0x82000000
 fdtaddr=0x88000000
 dtboaddr=0x89000000
 fdt_addr_r=0x88000000
-fdtoverlay_addr_r=0x89000000
+fdtoverlay_addr_r=0x89500000
 rdaddr=0x88080000
 ramdisk_addr_r=0x88080000
 scriptaddr=0x80000000

base-commit: a404065479be2c1fe1167c3c91367e8194a69d1b
-- 
2.46.2

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

[Mattijs Korpershoek]

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 ?

>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  include/env/ti/ti_common.env | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/env/ti/ti_common.env b/include/env/ti/ti_common.env
> index 7029d12bf20..a15af79d9c3 100644
> --- a/include/env/ti/ti_common.env
> +++ b/include/env/ti/ti_common.env
> @@ -3,7 +3,7 @@ kernel_addr_r=0x82000000
>  fdtaddr=0x88000000
>  dtboaddr=0x89000000
>  fdt_addr_r=0x88000000
> -fdtoverlay_addr_r=0x89000000
> +fdtoverlay_addr_r=0x89500000
>  rdaddr=0x88080000
>  ramdisk_addr_r=0x88080000
>  scriptaddr=0x80000000
>
> base-commit: a404065479be2c1fe1167c3c91367e8194a69d1b
> -- 
> 2.46.2

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Mattijs Korpershoek]

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