[PATCH 1/3] doc: environment: Drop u-boot_addr_r

[PATCH 1/3] doc: environment: Drop u-boot_addr_r

[Tom Rini]

This variable is never set nor explained why it would be set, drop it.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 doc/usage/environment.rst | 1 -
 1 file changed, 1 deletion(-)

diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index 28a8952b7528..a3f23d4e4e22 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -390,7 +390,6 @@ in U-Boot code.
 ================= ============== ================ ==============
 Image             File Name      RAM Address      Flash Location
 ================= ============== ================ ==============
-u-boot            u-boot         u-boot_addr_r    u-boot_addr
 Linux kernel      bootfile       kernel_addr_r    kernel_addr
 device tree blob  fdtfile        fdt_addr_r       fdt_addr
 ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
-- 
2.25.1

[PATCH 2/3] doc: environment: Expand on fdt_addr, initrd_addr and loadaddr

[Tom Rini]

- Explain why fdt_addr and initrd_addr should not be set to disable
  relocation normally.
- Provide some advice on the typical loadaddr default value.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 doc/usage/environment.rst | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index a3f23d4e4e22..a9a4702632d2 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -204,7 +204,9 @@ fdt_high
     to work it must reside in writable memory, have
     sufficient padding on the end of it for u-boot to
     add the information it needs into it, and the memory
-    must be accessible by the kernel.
+    must be accessible by the kernel.  This usage is strongly discouraged
+    however as it also stops U-Boot from ensuring the device tree starting
+    address is properly aligned and a misaligned tree will cause OS failures.
 
 fdtcontroladdr
     if set this is the address of the control flattened
@@ -240,14 +242,21 @@ initrd_high
     memory. In this case U-Boot will NOT COPY the
     ramdisk at all. This may be useful to reduce the
     boot time on your system, but requires that this
-    feature is supported by your Linux kernel.
+    feature is supported by your Linux kernel.  This usage however requires
+    that the user ensure that there will be no overlap with other parts of the
+    iamge such as the Linux kernel BSS.  It should not be enabled by default
+    and only done as part of optimizing a deployment.
 
 ipaddr
     IP address; needed for tftpboot command
 
 loadaddr
     Default load address for commands like "bootp",
-    "rarpboot", "tftpboot", "loadb" or "diskboot"
+    "rarpboot", "tftpboot", "loadb" or "diskboot".  Note that the optimal
+    default values here will vary between architectures.  On 32bit ARM for
+    example, some offset from start of memory is used as the Linux kernel
+    zImage has a self decompressor and it's best if we stay out of where that
+    will be working.
 
 loads_echo
     see CONFIG_LOADS_ECHO
-- 
2.25.1

[PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Tom Rini]

Start by elaborating on what some of our constraints tend to be with
image location values, and document where these external constraints
come from.  Provide a new subsection, an example based on the TI ARMv7
OMAP2PLUS families of chips, that gives sample values and explains why
we use these particular values.  This is based on what is in
include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
values referenced in this document now and not some of the further
additions that are less generic.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index a9a4702632d2..f70ccd6a58ee 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
 ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
 ================= ============== ================ ==============
 
+When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
+`ramdisk_addr_r` there are several constraints to keep in mind. When booting
+Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
+the requirements for booting all ARM platforms, including both alignment and
+where within memory various things must be.  These guidelines tend to also be
+correct for other OSes and unless specifically contradicted by documentation
+specific to another architecture, are good rules to follow for other
+architectures as well.
+
+Example Image locations
+^^^^^^^^^^^^^^^^^^^^^^^
+
+If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
+example for the above listed variables, we would do::
+
+    loadaddr=0x82000000
+    kernel_addr_r=${loadaddr}
+    fdt_addr_r=0x88000000
+    ramdisk_addr_r=0x88080000
+    bootm_size=0x10000000
+
+To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
+buffer from the start of memory as our default load address, and so where the
+kernel would also be loaded to.  This will hopefully allow for us to have the
+whole of the compressed kernel image exist in memory above where the whole of
+the decompressed kernel image will be, and allow for a quicker boot.  Next, we
+say that the device tree will be placed at 128MiB offset from the start of
+memory.  This is suggested by the kernel documment as it is exceedingly
+unlikely to be overwritten by the kernel itself given other architectural
+constraints.  We then allow for the device tree to be up to 512KiB in size
+before placing the ramdisk in memory.  We then say that everything should be
+within the first 256MiB of memory so that U-Boot can relocate things as needed
+to ensure proper alignment.  We pick 256MiB as our value here because we know
+there are very few platforms on in this family with less memory.  It could be
+as high as 768MiB and still ensure that everything would be visible to the
+kernel, but again we go with what we assume is the safest assumption.
 
 Automatically updated variables
 -------------------------------
@@ -472,3 +508,6 @@ Implementation
 --------------
 
 See :doc:`../develop/environment` for internal development details.
+
+.. _`Booting ARM Linux`: https://www.kernel.org/doc/html/latest/arm/booting.html
+.. _`Booting AArch64 Linux`: https://www.kernel.org/doc/html/latest/arm64/booting.html
-- 
2.25.1

Re: [PATCH 1/3] doc: environment: Drop u-boot_addr_r

[Simon Glass]

On Mon, 20 Jun 2022 at 08:31, Tom Rini <trini@konsulko.com> wrote:
>
> This variable is never set nor explained why it would be set, drop it.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  doc/usage/environment.rst | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/3] doc: environment: Expand on fdt_addr, initrd_addr and loadaddr

[Simon Glass]

On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
>
> - Explain why fdt_addr and initrd_addr should not be set to disable
>   relocation normally.
> - Provide some advice on the typical loadaddr default value.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  doc/usage/environment.rst | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Simon Glass]

On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
>
> Start by elaborating on what some of our constraints tend to be with
> image location values, and document where these external constraints
> come from.  Provide a new subsection, an example based on the TI ARMv7
> OMAP2PLUS families of chips, that gives sample values and explains why
> we use these particular values.  This is based on what is in
> include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
> DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
> values referenced in this document now and not some of the further
> additions that are less generic.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> index a9a4702632d2..f70ccd6a58ee 100644
> --- a/doc/usage/environment.rst
> +++ b/doc/usage/environment.rst
> @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
>  ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
>  ================= ============== ================ ==============
>
> +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
> +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
> +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
> +the requirements for booting all ARM platforms, including both alignment and
> +where within memory various things must be.  These guidelines tend to also be
> +correct for other OSes and unless specifically contradicted by documentation
> +specific to another architecture, are good rules to follow for other
> +architectures as well.
> +
> +Example Image locations
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
> +example for the above listed variables, we would do::
> +
> +    loadaddr=0x82000000
> +    kernel_addr_r=${loadaddr}
> +    fdt_addr_r=0x88000000
> +    ramdisk_addr_r=0x88080000
> +    bootm_size=0x10000000
> +
> +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB

Should it say 'We use a 32MiB' ?


> +buffer from the start of memory as our default load address, and so where the
> +kernel would also be loaded to.  This will hopefully allow for us to have the
> +whole of the compressed kernel image exist in memory above where the whole of
> +the decompressed kernel image will be, and allow for a quicker boot.  Next, we
> +say that the device tree will be placed at 128MiB offset from the start of
> +memory.  This is suggested by the kernel documment as it is exceedingly
> +unlikely to be overwritten by the kernel itself given other architectural
> +constraints.  We then allow for the device tree to be up to 512KiB in size
> +before placing the ramdisk in memory.  We then say that everything should be
> +within the first 256MiB of memory so that U-Boot can relocate things as needed
> +to ensure proper alignment.  We pick 256MiB as our value here because we know
> +there are very few platforms on in this family with less memory.  It could be
> +as high as 768MiB and still ensure that everything would be visible to the
> +kernel, but again we go with what we assume is the safest assumption.
>
>  Automatically updated variables
>  -------------------------------
> @@ -472,3 +508,6 @@ Implementation
>  --------------
>
>  See :doc:`../develop/environment` for internal development details.
> +
> +.. _`Booting ARM Linux`: https://www.kernel.org/doc/html/latest/arm/booting.html
> +.. _`Booting AArch64 Linux`: https://www.kernel.org/doc/html/latest/arm64/booting.html
> --
> 2.25.1
>

Re: [PATCH 1/3] doc: environment: Drop u-boot_addr_r

[Heinrich Schuchardt]

On 6/20/22 16:31, Tom Rini wrote:
> This variable is never set nor explained why it would be set, drop it.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   doc/usage/environment.rst | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> index 28a8952b7528..a3f23d4e4e22 100644
> --- a/doc/usage/environment.rst
> +++ b/doc/usage/environment.rst
> @@ -390,7 +390,6 @@ in U-Boot code.
>   ================= ============== ================ ==============
>   Image             File Name      RAM Address      Flash Location
>   ================= ============== ================ ==============
> -u-boot            u-boot         u-boot_addr_r    u-boot_addr
>   Linux kernel      bootfile       kernel_addr_r    kernel_addr
>   device tree blob  fdtfile        fdt_addr_r       fdt_addr
>   ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr

Re: [PATCH 2/3] doc: environment: Expand on fdt_addr, initrd_addr and loadaddr

[Heinrich Schuchardt]

On 6/20/22 16:31, Tom Rini wrote:
> - Explain why fdt_addr and initrd_addr should not be set to disable
>    relocation normally.
> - Provide some advice on the typical loadaddr default value.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>   doc/usage/environment.rst | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> index a3f23d4e4e22..a9a4702632d2 100644
> --- a/doc/usage/environment.rst
> +++ b/doc/usage/environment.rst
> @@ -204,7 +204,9 @@ fdt_high
>       to work it must reside in writable memory, have
>       sufficient padding on the end of it for u-boot to
>       add the information it needs into it, and the memory
> -    must be accessible by the kernel.
> +    must be accessible by the kernel.  This usage is strongly discouraged
> +    however as it also stops U-Boot from ensuring the device tree starting
> +    address is properly aligned and a misaligned tree will cause OS failures.

fdt_addr_r is typically 4 byte aligned. Is there a bug in some part of
the code?

>
>   fdtcontroladdr
>       if set this is the address of the control flattened
> @@ -240,14 +242,21 @@ initrd_high
>       memory. In this case U-Boot will NOT COPY the
>       ramdisk at all. This may be useful to reduce the
>       boot time on your system, but requires that this
> -    feature is supported by your Linux kernel.
> +    feature is supported by your Linux kernel.  This usage however requires
> +    that the user ensure that there will be no overlap with other parts of the

It is not the user but the developer who define $kernel_addr_r,
$initr_addr_r, and $fdt_addr_r.

Relocating initrd does not stop the user from loading the kernel to
initrd_high. The user is always responsible for avoiding overlap. In
this regard there is nothing special about initrd_high=~0UL.

> +    iamge such as the Linux kernel BSS.  It should not be enabled by default

%s/iamge/image

> +    and only done as part of optimizing a deployment.

Relocating initrd and fdt is using boot time. On many boards it is
simply superfluous. On these boards I cannot see any reason not to
disable it by default.

Best regards

Heinrich

>
>   ipaddr
>       IP address; needed for tftpboot command
>
>   loadaddr
>       Default load address for commands like "bootp",
> -    "rarpboot", "tftpboot", "loadb" or "diskboot"
> +    "rarpboot", "tftpboot", "loadb" or "diskboot".  Note that the optimal
> +    default values here will vary between architectures.  On 32bit ARM for
> +    example, some offset from start of memory is used as the Linux kernel
> +    zImage has a self decompressor and it's best if we stay out of where that
> +    will be working.
>
>   loads_echo
>       see CONFIG_LOADS_ECHO

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Heinrich Schuchardt]

On 6/30/22 12:06, Simon Glass wrote:
> On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
>>
>> Start by elaborating on what some of our constraints tend to be with
>> image location values, and document where these external constraints
>> come from.  Provide a new subsection, an example based on the TI ARMv7
>> OMAP2PLUS families of chips, that gives sample values and explains why
>> we use these particular values.  This is based on what is in
>> include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
>> DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
>> values referenced in this document now and not some of the further
>> additions that are less generic.
>>
>> Signed-off-by: Tom Rini <trini@konsulko.com>
>> ---
>>   doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Below you want a change?

>
>>
>> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
>> index a9a4702632d2..f70ccd6a58ee 100644
>> --- a/doc/usage/environment.rst
>> +++ b/doc/usage/environment.rst
>> @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
>>   ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
>>   ================= ============== ================ ==============
>>
>> +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
>> +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
>> +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
>> +the requirements for booting all ARM platforms, including both alignment and
>> +where within memory various things must be.  These guidelines tend to also be
>> +correct for other OSes and unless specifically contradicted by documentation

What makes you think that BSD or Haiku have the same constraints as Linux?

>> +specific to another architecture, are good rules to follow for other
>> +architectures as well.

No. RISC-V does not have the same requirements as ARM. E.g. the initrd
can be located anywhere in memory.

>> +
>> +Example Image locations
>> +^^^^^^^^^^^^^^^^^^^^^^^

You seem not to refer to a file 'Image'.

%s/Image/image/

>> +
>> +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
>> +example for the above listed variables, we would do::

%s/we would do/we chose/ ?

>> +
>> +    loadaddr=0x82000000
>> +    kernel_addr_r=${loadaddr}
>> +    fdt_addr_r=0x88000000
>> +    ramdisk_addr_r=0x88080000
>> +    bootm_size=0x10000000
>> +
>> +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
>
> Should it say 'We use a 32MiB' ?

Please, mention that MAX_UNCOMP_KERNEL_SIZE = 32 MiB is ARMv7 specific.

>
>
>> +buffer from the start of memory as our default load address, and so where the
>> +kernel would also be loaded to.  This will hopefully allow for us to have the

%s/allow for us/allow/

>> +whole of the compressed kernel image exist in memory above where the whole of
>> +the decompressed kernel image will be, and allow for a quicker boot.  Next, we

Please, mention that decompressor code otherwise will have to relocate
the compressed kernel.

>> +say that the device tree will be placed at 128MiB offset from the start of

Please, mention that initrd must be

* within 512 MiB (0x20000000) of the memory start on arm
   (which restricts initrd_high)
* in a a 1 GB aligned region of size '1UL << (VA_BITS_MIN - 1)' that
   includes the kernel on arm64

On RISC-V such a limitation does not exist.

>> +memory.  This is suggested by the kernel documment as it is exceedingly

%s/documment/document

>> +unlikely to be overwritten by the kernel itself given other architectural
>> +constraints.  We then allow for the device tree to be up to 512KiB in size
>> +before placing the ramdisk in memory.  We then say that everything should be
>> +within the first 256MiB of memory so that U-Boot can relocate things as needed
>> +to ensure proper alignment.  We pick 256MiB as our value here because we know

If the load address ranges occupy the first 256 MiB, where will be the
space for relocation on a 256 MiB board? Without mentioning initrd_high
this paragraph is incomplete.

>> +there are very few platforms on in this family with less memory.  It could be
>> +as high as 768MiB and still ensure that everything would be visible to the
>> +kernel, but again we go with what we assume is the safest assumption.

A section per architecture clearly naming the architecture specific
restrictions of Linux would be much more helpful.

Best regards

Heinrich

>>
>>   Automatically updated variables
>>   -------------------------------
>> @@ -472,3 +508,6 @@ Implementation
>>   --------------
>>
>>   See :doc:`../develop/environment` for internal development details.
>> +
>> +.. _`Booting ARM Linux`: https://www.kernel.org/doc/html/latest/arm/booting.html
>> +.. _`Booting AArch64 Linux`: https://www.kernel.org/doc/html/latest/arm64/booting.html
>> --
>> 2.25.1
>>

Re: [PATCH 2/3] doc: environment: Expand on fdt_addr, initrd_addr and loadaddr

[Tom Rini]

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

On Sun, Jul 10, 2022 at 01:02:28PM +0200, Heinrich Schuchardt wrote:
> On 6/20/22 16:31, Tom Rini wrote:
> > - Explain why fdt_addr and initrd_addr should not be set to disable
> >    relocation normally.
> > - Provide some advice on the typical loadaddr default value.
> > 
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> >   doc/usage/environment.rst | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> > index a3f23d4e4e22..a9a4702632d2 100644
> > --- a/doc/usage/environment.rst
> > +++ b/doc/usage/environment.rst
> > @@ -204,7 +204,9 @@ fdt_high
> >       to work it must reside in writable memory, have
> >       sufficient padding on the end of it for u-boot to
> >       add the information it needs into it, and the memory
> > -    must be accessible by the kernel.
> > +    must be accessible by the kernel.  This usage is strongly discouraged
> > +    however as it also stops U-Boot from ensuring the device tree starting
> > +    address is properly aligned and a misaligned tree will cause OS failures.
> 
> fdt_addr_r is typically 4 byte aligned. Is there a bug in some part of
> the code?

It must be 8 byte aligned.  And we don't know the alignment inside of
FIT images.  Do not disable device tree relocation except under the most
calculated of circumstances.

> >   fdtcontroladdr
> >       if set this is the address of the control flattened
> > @@ -240,14 +242,21 @@ initrd_high
> >       memory. In this case U-Boot will NOT COPY the
> >       ramdisk at all. This may be useful to reduce the
> >       boot time on your system, but requires that this
> > -    feature is supported by your Linux kernel.
> > +    feature is supported by your Linux kernel.  This usage however requires
> > +    that the user ensure that there will be no overlap with other parts of the
> 
> It is not the user but the developer who define $kernel_addr_r,
> $initr_addr_r, and $fdt_addr_r.

No, it can be either.  See stackoverflow and assorted blog posts.
Person who picked up an SBC and just wants the thing to work will find
various posts saying to do ... something ... here.

> Relocating initrd does not stop the user from loading the kernel to
> initrd_high. The user is always responsible for avoiding overlap. In
> this regard there is nothing special about initrd_high=~0UL.

And we're responsible for providing reasonable defaults.  Do not disable
initrd relocation by default as it gains little and can break the OS.

> > +    iamge such as the Linux kernel BSS.  It should not be enabled by default
> 
> %s/iamge/image

Please fix when applying.

> > +    and only done as part of optimizing a deployment.
> 
> Relocating initrd and fdt is using boot time. On many boards it is
> simply superfluous. On these boards I cannot see any reason not to
> disable it by default.

The way to avoid "superfluous" time here is to set reasonable default
locations, not disabling relocation.

Please note this is what the policy is, not something new.

-- 
Tom

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

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Tom Rini]

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

On Sun, Jul 10, 2022 at 02:26:04PM +0200, Heinrich Schuchardt wrote:
> On 6/30/22 12:06, Simon Glass wrote:
> > On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > 
> > > Start by elaborating on what some of our constraints tend to be with
> > > image location values, and document where these external constraints
> > > come from.  Provide a new subsection, an example based on the TI ARMv7
> > > OMAP2PLUS families of chips, that gives sample values and explains why
> > > we use these particular values.  This is based on what is in
> > > include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
> > > DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
> > > values referenced in this document now and not some of the further
> > > additions that are less generic.
> > > 
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > >   doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 39 insertions(+)
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Below you want a change?

Yes, often Simon does that (and it's fine) to both offer a tag but if
another iteration is needed to make a minor adjustment to some wording
or another, or to make when applying.  Which is fine with me.

> > > diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> > > index a9a4702632d2..f70ccd6a58ee 100644
> > > --- a/doc/usage/environment.rst
> > > +++ b/doc/usage/environment.rst
> > > @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
> > >   ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
> > >   ================= ============== ================ ==============
> > > 
> > > +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
> > > +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
> > > +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
> > > +the requirements for booting all ARM platforms, including both alignment and
> > > +where within memory various things must be.  These guidelines tend to also be
> > > +correct for other OSes and unless specifically contradicted by documentation
> 
> What makes you think that BSD or Haiku have the same constraints as Linux?

Because of what I said, and experience?  Now, one may be a subset of
another, but that still means it will work for both.  This is intended
to be general best practices.  If you follow this then it's likely
anything else will work too.  The danger comes from trying to optimize
the sizes to be as small as possible, rather than as large/flexible as
will likely work anywhere.  I will try and expand on that idea in the
next iteration.

> > > +specific to another architecture, are good rules to follow for other
> > > +architectures as well.
> 
> No. RISC-V does not have the same requirements as ARM. E.g. the initrd
> can be located anywhere in memory.

Please point to documentation that confirms that, and some otherwise bad
examples that actually work.

> > > +
> > > +Example Image locations
> > > +^^^^^^^^^^^^^^^^^^^^^^^
> 
> You seem not to refer to a file 'Image'.
> 
> %s/Image/image/

OK.

> > > +
> > > +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
> > > +example for the above listed variables, we would do::
> 
> %s/we would do/we chose/ ?

Either?  I don't see it mattering either way.

> > > +
> > > +    loadaddr=0x82000000
> > > +    kernel_addr_r=${loadaddr}
> > > +    fdt_addr_r=0x88000000
> > > +    ramdisk_addr_r=0x88080000
> > > +    bootm_size=0x10000000
> > > +
> > > +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
> > 
> > Should it say 'We use a 32MiB' ?
> 
> Please, mention that MAX_UNCOMP_KERNEL_SIZE = 32 MiB is ARMv7 specific.

Sorry?  As I understood it last, the maximum size was something like
96MiB before you have to employ some funky tricks.

> > > +buffer from the start of memory as our default load address, and so where the
> > > +kernel would also be loaded to.  This will hopefully allow for us to have the
> 
> %s/allow for us/allow/
> 
> > > +whole of the compressed kernel image exist in memory above where the whole of
> > > +the decompressed kernel image will be, and allow for a quicker boot.  Next, we

We use 32MiB for the reason I said here.  Which is only a slight
rewording of the arm32 Linux booting document, and the section starts
out by saying this is an example for ARMv7 platforms.

> Please, mention that decompressor code otherwise will have to relocate
> the compressed kernel.

I'm not sure.  Perhaps it would be good to also link to some of the
articles expanding upon how Linux on ARM32 boots, as part of more
general documentation, rather than a specific example here.

> > > +say that the device tree will be placed at 128MiB offset from the start of
> 
> Please, mention that initrd must be
> 
> * within 512 MiB (0x20000000) of the memory start on arm
>   (which restricts initrd_high)
> * in a a 1 GB aligned region of size '1UL << (VA_BITS_MIN - 1)' that
>   includes the kernel on arm64

No, because this is not intended to list every constraint on ARM32 (nor
arm64, which would benefit from an example that's not TI, as TI arm64
platforms share the same base address for memory).

> On RISC-V such a limitation does not exist.

I do wish that RISC-V had a similar document to ARM/ARM64, in Linux.

> > > +memory.  This is suggested by the kernel documment as it is exceedingly
> 
> %s/documment/document
> 
> > > +unlikely to be overwritten by the kernel itself given other architectural
> > > +constraints.  We then allow for the device tree to be up to 512KiB in size
> > > +before placing the ramdisk in memory.  We then say that everything should be
> > > +within the first 256MiB of memory so that U-Boot can relocate things as needed
> > > +to ensure proper alignment.  We pick 256MiB as our value here because we know
> 
> If the load address ranges occupy the first 256 MiB, where will be the
> space for relocation on a 256 MiB board? Without mentioning initrd_high
> this paragraph is incomplete.

You don't need initrd_high / fdt_high when bootm_size is used.  Maybe
that needs to be clearer in this paragraph and also the rest of the
documentation.

> > > +there are very few platforms on in this family with less memory.  It could be
> > > +as high as 768MiB and still ensure that everything would be visible to the
> > > +kernel, but again we go with what we assume is the safest assumption.
> 
> A section per architecture clearly naming the architecture specific
> restrictions of Linux would be much more helpful.

I did ask for further explained examples as it would indeed be good to
have at least one per architecture.

-- 
Tom

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

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Heinrich Schuchardt]

On 7/10/22 18:17, Tom Rini wrote:
> On Sun, Jul 10, 2022 at 02:26:04PM +0200, Heinrich Schuchardt wrote:
>> On 6/30/22 12:06, Simon Glass wrote:
>>> On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> Start by elaborating on what some of our constraints tend to be with
>>>> image location values, and document where these external constraints
>>>> come from.  Provide a new subsection, an example based on the TI ARMv7
>>>> OMAP2PLUS families of chips, that gives sample values and explains why
>>>> we use these particular values.  This is based on what is in
>>>> include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
>>>> DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
>>>> values referenced in this document now and not some of the further
>>>> additions that are less generic.
>>>>
>>>> Signed-off-by: Tom Rini <trini@konsulko.com>
>>>> ---
>>>>    doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 39 insertions(+)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Below you want a change?
>
> Yes, often Simon does that (and it's fine) to both offer a tag but if
> another iteration is needed to make a minor adjustment to some wording
> or another, or to make when applying.  Which is fine with me.
>
>>>> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
>>>> index a9a4702632d2..f70ccd6a58ee 100644
>>>> --- a/doc/usage/environment.rst
>>>> +++ b/doc/usage/environment.rst
>>>> @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
>>>>    ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
>>>>    ================= ============== ================ ==============
>>>>
>>>> +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
>>>> +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
>>>> +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
>>>> +the requirements for booting all ARM platforms, including both alignment and
>>>> +where within memory various things must be.  These guidelines tend to also be
>>>> +correct for other OSes and unless specifically contradicted by documentation
>>
>> What makes you think that BSD or Haiku have the same constraints as Linux?
>
> Because of what I said, and experience?  Now, one may be a subset of
> another, but that still means it will work for both.  This is intended
> to be general best practices.  If you follow this then it's likely
> anything else will work too.  The danger comes from trying to optimize
> the sizes to be as small as possible, rather than as large/flexible as
> will likely work anywhere.  I will try and expand on that idea in the
> next iteration.
>
>>>> +specific to another architecture, are good rules to follow for other
>>>> +architectures as well.
>>
>> No. RISC-V does not have the same requirements as ARM. E.g. the initrd
>> can be located anywhere in memory.
>
> Please point to documentation that confirms that, and some otherwise bad
> examples that actually work.

[PATCH 1/1] RISC-V: load initrd wherever it fits into memory
https://lore.kernel.org/all/20210629134018.62859-1-xypron.glpk@gmx.de/

Please, have a look at efi_get_max_initrd_addr() in these files:

arch/arm/include/asm/efi.h:73
arch/arm64/include/asm/efi.h:77
arch/loongarch/include/asm/efi.h:36
arch/riscv/include/asm/efi.h:31

MAX_UNCOMP_KERNEL_SIZE = 32 MiB is only enforced in
drivers/firmware/efi/libstub/arm32-stub.c.

>
>>>> +
>>>> +Example Image locations
>>>> +^^^^^^^^^^^^^^^^^^^^^^^
>>
>> You seem not to refer to a file 'Image'.
>>
>> %s/Image/image/
>
> OK.
>
>>>> +
>>>> +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
>>>> +example for the above listed variables, we would do::
>>
>> %s/we would do/we chose/ ?
>
> Either?  I don't see it mattering either way.
>
>>>> +
>>>> +    loadaddr=0x82000000
>>>> +    kernel_addr_r=${loadaddr}
>>>> +    fdt_addr_r=0x88000000
>>>> +    ramdisk_addr_r=0x88080000
>>>> +    bootm_size=0x10000000
>>>> +
>>>> +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
>>>
>>> Should it say 'We use a 32MiB' ?
>>
>> Please, mention that MAX_UNCOMP_KERNEL_SIZE = 32 MiB is ARMv7 specific.
>
> Sorry?  As I understood it last, the maximum size was something like
> 96MiB before you have to employ some funky tricks.

Look at the use of MAX_UNCOMP_KERNEL_SIZE in handle_kernel_image() of
the EFI stub (drivers/firmware/efi/libstub/arm32-stub.c).

>
>>>> +buffer from the start of memory as our default load address, and so where the
>>>> +kernel would also be loaded to.  This will hopefully allow for us to have the
>>
>> %s/allow for us/allow/
>>
>>>> +whole of the compressed kernel image exist in memory above where the whole of
>>>> +the decompressed kernel image will be, and allow for a quicker boot.  Next, we
>
> We use 32MiB for the reason I said here.  Which is only a slight
> rewording of the arm32 Linux booting document, and the section starts
> out by saying this is an example for ARMv7 platforms.

You ask all other architectures to follow this example?

>
>> Please, mention that decompressor code otherwise will have to relocate
>> the compressed kernel.
>
> I'm not sure.  Perhaps it would be good to also link to some of the
> articles expanding upon how Linux on ARM32 boots, as part of more
> general documentation, rather than a specific example here.

Just look at the comment above the definition of MAX_UNCOMP_KERNEL_SIZE
in arch/arm/include/asm/efi.h.

>
>>>> +say that the device tree will be placed at 128MiB offset from the start of
>>
>> Please, mention that initrd must be
>>
>> * within 512 MiB (0x20000000) of the memory start on arm
>>    (which restricts initrd_high)
>> * in a a 1 GB aligned region of size '1UL << (VA_BITS_MIN - 1)' that
>>    includes the kernel on arm64
>
> No, because this is not intended to list every constraint on ARM32 (nor
> arm64, which would benefit from an example that's not TI, as TI arm64
> platforms share the same base address for memory).

You ask above to follow the example of ARMv7 on all architectures. Hence
it is necessary to point out the differences.

Best regards

Heinrich

>
>> On RISC-V such a limitation does not exist.
>
> I do wish that RISC-V had a similar document to ARM/ARM64, in Linux.
>
>>>> +memory.  This is suggested by the kernel documment as it is exceedingly
>>
>> %s/documment/document
>>
>>>> +unlikely to be overwritten by the kernel itself given other architectural
>>>> +constraints.  We then allow for the device tree to be up to 512KiB in size
>>>> +before placing the ramdisk in memory.  We then say that everything should be
>>>> +within the first 256MiB of memory so that U-Boot can relocate things as needed
>>>> +to ensure proper alignment.  We pick 256MiB as our value here because we know
>>
>> If the load address ranges occupy the first 256 MiB, where will be the
>> space for relocation on a 256 MiB board? Without mentioning initrd_high
>> this paragraph is incomplete.
>
> You don't need initrd_high / fdt_high when bootm_size is used.  Maybe
> that needs to be clearer in this paragraph and also the rest of the
> documentation.
>
>>>> +there are very few platforms on in this family with less memory.  It could be
>>>> +as high as 768MiB and still ensure that everything would be visible to the
>>>> +kernel, but again we go with what we assume is the safest assumption.
>>
>> A section per architecture clearly naming the architecture specific
>> restrictions of Linux would be much more helpful.
>
> I did ask for further explained examples as it would indeed be good to
> have at least one per architecture.
>

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Tom Rini]

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

On Mon, Jul 11, 2022 at 08:42:08AM +0200, Heinrich Schuchardt wrote:
> On 7/10/22 18:17, Tom Rini wrote:
> > On Sun, Jul 10, 2022 at 02:26:04PM +0200, Heinrich Schuchardt wrote:
> > > On 6/30/22 12:06, Simon Glass wrote:
> > > > On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > > > 
> > > > > Start by elaborating on what some of our constraints tend to be with
> > > > > image location values, and document where these external constraints
> > > > > come from.  Provide a new subsection, an example based on the TI ARMv7
> > > > > OMAP2PLUS families of chips, that gives sample values and explains why
> > > > > we use these particular values.  This is based on what is in
> > > > > include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
> > > > > DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
> > > > > values referenced in this document now and not some of the further
> > > > > additions that are less generic.
> > > > > 
> > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > >    doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 39 insertions(+)
> > > > 
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > 
> > > Below you want a change?
> > 
> > Yes, often Simon does that (and it's fine) to both offer a tag but if
> > another iteration is needed to make a minor adjustment to some wording
> > or another, or to make when applying.  Which is fine with me.
> > 
> > > > > diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> > > > > index a9a4702632d2..f70ccd6a58ee 100644
> > > > > --- a/doc/usage/environment.rst
> > > > > +++ b/doc/usage/environment.rst
> > > > > @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
> > > > >    ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
> > > > >    ================= ============== ================ ==============
> > > > > 
> > > > > +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
> > > > > +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
> > > > > +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
> > > > > +the requirements for booting all ARM platforms, including both alignment and
> > > > > +where within memory various things must be.  These guidelines tend to also be
> > > > > +correct for other OSes and unless specifically contradicted by documentation
> > > 
> > > What makes you think that BSD or Haiku have the same constraints as Linux?
> > 
> > Because of what I said, and experience?  Now, one may be a subset of
> > another, but that still means it will work for both.  This is intended
> > to be general best practices.  If you follow this then it's likely
> > anything else will work too.  The danger comes from trying to optimize
> > the sizes to be as small as possible, rather than as large/flexible as
> > will likely work anywhere.  I will try and expand on that idea in the
> > next iteration.
> > 
> > > > > +specific to another architecture, are good rules to follow for other
> > > > > +architectures as well.
> > > 
> > > No. RISC-V does not have the same requirements as ARM. E.g. the initrd
> > > can be located anywhere in memory.
> > 
> > Please point to documentation that confirms that, and some otherwise bad
> > examples that actually work.
> 
> [PATCH 1/1] RISC-V: load initrd wherever it fits into memory
> https://lore.kernel.org/all/20210629134018.62859-1-xypron.glpk@gmx.de/

Looks like someone ran in to the first common case of "oops, overwrote
the ramdisk with the kernel bss" or something along those lines.

Which is why I'm asking for more architectures to add good examples of
where to load each payload in to memory, with explanations of why and
how big of a gap to have.  I _think_ in Linux RISC-V (and hopefully for
32bit and 64bit) used the arm64 Image format and so BSS size is
available in the header and so we can safely check for that overlap and
relocate rather than fail to boot.  Checking for, and avoiding to start
with, these types of problems is why I want to add the examples.

> Please, have a look at efi_get_max_initrd_addr() in these files:
> 
> arch/arm/include/asm/efi.h:73
> arch/arm64/include/asm/efi.h:77
> arch/loongarch/include/asm/efi.h:36
> arch/riscv/include/asm/efi.h:31
> 
> MAX_UNCOMP_KERNEL_SIZE = 32 MiB is only enforced in
> drivers/firmware/efi/libstub/arm32-stub.c.

This isn't an EFI thing however.  The max uncompressed Linux kernel
image for arm32 is something along the lines of 96MiB I recall rmk
telling me when I asked about it at the time.  The base+32MiB in the
example here is for optimal (but not REQUIRED) decompressor location.

> > > > > +Example Image locations
> > > > > +^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > You seem not to refer to a file 'Image'.
> > > 
> > > %s/Image/image/
> > 
> > OK.
> > 
> > > > > +
> > > > > +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
> > > > > +example for the above listed variables, we would do::
> > > 
> > > %s/we would do/we chose/ ?
> > 
> > Either?  I don't see it mattering either way.
> > 
> > > > > +
> > > > > +    loadaddr=0x82000000
> > > > > +    kernel_addr_r=${loadaddr}
> > > > > +    fdt_addr_r=0x88000000
> > > > > +    ramdisk_addr_r=0x88080000
> > > > > +    bootm_size=0x10000000
> > > > > +
> > > > > +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
> > > > 
> > > > Should it say 'We use a 32MiB' ?
> > > 
> > > Please, mention that MAX_UNCOMP_KERNEL_SIZE = 32 MiB is ARMv7 specific.
> > 
> > Sorry?  As I understood it last, the maximum size was something like
> > 96MiB before you have to employ some funky tricks.
> 
> Look at the use of MAX_UNCOMP_KERNEL_SIZE in handle_kernel_image() of
> the EFI stub (drivers/firmware/efi/libstub/arm32-stub.c).
> 
> > 
> > > > > +buffer from the start of memory as our default load address, and so where the
> > > > > +kernel would also be loaded to.  This will hopefully allow for us to have the
> > > 
> > > %s/allow for us/allow/
> > > 
> > > > > +whole of the compressed kernel image exist in memory above where the whole of
> > > > > +the decompressed kernel image will be, and allow for a quicker boot.  Next, we
> > 
> > We use 32MiB for the reason I said here.  Which is only a slight
> > rewording of the arm32 Linux booting document, and the section starts
> > out by saying this is an example for ARMv7 platforms.
> 
> You ask all other architectures to follow this example?

I could have sworn that somewhere within the comments of this series I
asked for more examples to be added, yes.  And I know I intended to
(since we _need_ them, and I think I've expressed me desire to have them
before) and I am asking now.

> > > Please, mention that decompressor code otherwise will have to relocate
> > > the compressed kernel.
> > 
> > I'm not sure.  Perhaps it would be good to also link to some of the
> > articles expanding upon how Linux on ARM32 boots, as part of more
> > general documentation, rather than a specific example here.
> 
> Just look at the comment above the definition of MAX_UNCOMP_KERNEL_SIZE
> in arch/arm/include/asm/efi.h.

Keep in mind this is only vaguely EFI-related.  Given how long ago edkII
for beagleboard was done, it doesn't quite predate EFI on ARM, but this
example has been in long use for the common non-EFI case on 32bit ARM.

> > > > > +say that the device tree will be placed at 128MiB offset from the start of
> > > 
> > > Please, mention that initrd must be
> > > 
> > > * within 512 MiB (0x20000000) of the memory start on arm
> > >    (which restricts initrd_high)
> > > * in a a 1 GB aligned region of size '1UL << (VA_BITS_MIN - 1)' that
> > >    includes the kernel on arm64
> > 
> > No, because this is not intended to list every constraint on ARM32 (nor
> > arm64, which would benefit from an example that's not TI, as TI arm64
> > platforms share the same base address for memory).
> 
> You ask above to follow the example of ARMv7 on all architectures. Hence
> it is necessary to point out the differences.

No, I'm asking for more examples to be added for each architecture.

-- 
Tom

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

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Heinrich Schuchardt]

On 7/11/22 14:41, Tom Rini wrote:
> On Mon, Jul 11, 2022 at 08:42:08AM +0200, Heinrich Schuchardt wrote:
>> On 7/10/22 18:17, Tom Rini wrote:
>>> On Sun, Jul 10, 2022 at 02:26:04PM +0200, Heinrich Schuchardt wrote:
>>>> On 6/30/22 12:06, Simon Glass wrote:
>>>>> On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
>>>>>>
>>>>>> Start by elaborating on what some of our constraints tend to be with
>>>>>> image location values, and document where these external constraints
>>>>>> come from.  Provide a new subsection, an example based on the TI ARMv7
>>>>>> OMAP2PLUS families of chips, that gives sample values and explains why
>>>>>> we use these particular values.  This is based on what is in
>>>>>> include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
>>>>>> DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
>>>>>> values referenced in this document now and not some of the further
>>>>>> additions that are less generic.
>>>>>>
>>>>>> Signed-off-by: Tom Rini <trini@konsulko.com>
>>>>>> ---
>>>>>>     doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 39 insertions(+)
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> Below you want a change?
>>>
>>> Yes, often Simon does that (and it's fine) to both offer a tag but if
>>> another iteration is needed to make a minor adjustment to some wording
>>> or another, or to make when applying.  Which is fine with me.
>>>
>>>>>> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
>>>>>> index a9a4702632d2..f70ccd6a58ee 100644
>>>>>> --- a/doc/usage/environment.rst
>>>>>> +++ b/doc/usage/environment.rst
>>>>>> @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
>>>>>>     ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
>>>>>>     ================= ============== ================ ==============
>>>>>>
>>>>>> +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
>>>>>> +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
>>>>>> +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
>>>>>> +the requirements for booting all ARM platforms, including both alignment and
>>>>>> +where within memory various things must be.  These guidelines tend to also be
>>>>>> +correct for other OSes and unless specifically contradicted by documentation
>>>>
>>>> What makes you think that BSD or Haiku have the same constraints as Linux?
>>>
>>> Because of what I said, and experience?  Now, one may be a subset of
>>> another, but that still means it will work for both.  This is intended
>>> to be general best practices.  If you follow this then it's likely
>>> anything else will work too.  The danger comes from trying to optimize
>>> the sizes to be as small as possible, rather than as large/flexible as
>>> will likely work anywhere.  I will try and expand on that idea in the
>>> next iteration.
>>>
>>>>>> +specific to another architecture, are good rules to follow for other
>>>>>> +architectures as well.
>>>>
>>>> No. RISC-V does not have the same requirements as ARM. E.g. the initrd
>>>> can be located anywhere in memory.
>>>
>>> Please point to documentation that confirms that, and some otherwise bad
>>> examples that actually work.
>>
>> [PATCH 1/1] RISC-V: load initrd wherever it fits into memory
>> https://lore.kernel.org/all/20210629134018.62859-1-xypron.glpk@gmx.de/
>
> Looks like someone ran in to the first common case of "oops, overwrote
> the ramdisk with the kernel bss" or something along those lines.

Not at all. The ramdisk was relocated by U-Boot unnecessarily to above
256 MiB after start of RAM and the EFI stub before the patch did not
accept this address for no reason.

>
> Which is why I'm asking for more architectures to add good examples of
> where to load each payload in to memory, with explanations of why and
> how big of a gap to have.  I _think_ in Linux RISC-V (and hopefully for
> 32bit and 64bit) used the arm64 Image format and so BSS size is
> available in the header and so we can safely check for that overlap and
> relocate rather than fail to boot.  Checking for, and avoiding to start
> with, these types of problems is why I want to add the examples.

I am not aware of any restrictions for the placement of kernel, initrd,
fdt for RISC-V. Therefore there is no need to relocate anything after
loading (without overlap).

The bootefi command will never relocate a kernel or an fdt on any
architecture. You just pass the original load addresses.

>
>> Please, have a look at efi_get_max_initrd_addr() in these files:
>>
>> arch/arm/include/asm/efi.h:73
>> arch/arm64/include/asm/efi.h:77
>> arch/loongarch/include/asm/efi.h:36
>> arch/riscv/include/asm/efi.h:31
>>
>> MAX_UNCOMP_KERNEL_SIZE = 32 MiB is only enforced in
>> drivers/firmware/efi/libstub/arm32-stub.c.
>
> This isn't an EFI thing however.  The max uncompressed Linux kernel
> image for arm32 is something along the lines of 96MiB I recall rmk
> telling me when I asked about it at the time.  The base+32MiB in the
> example here is for optimal (but not REQUIRED) decompressor location.

The decompressor is what follows the EFI stub in the image?

>
>>>>>> +Example Image locations
>>>>>> +^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> You seem not to refer to a file 'Image'.
>>>>
>>>> %s/Image/image/
>>>
>>> OK.
>>>
>>>>>> +
>>>>>> +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
>>>>>> +example for the above listed variables, we would do::
>>>>
>>>> %s/we would do/we chose/ ?
>>>
>>> Either?  I don't see it mattering either way.
>>>
>>>>>> +
>>>>>> +    loadaddr=0x82000000
>>>>>> +    kernel_addr_r=${loadaddr}
>>>>>> +    fdt_addr_r=0x88000000
>>>>>> +    ramdisk_addr_r=0x88080000
>>>>>> +    bootm_size=0x10000000
>>>>>> +
>>>>>> +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
>>>>>
>>>>> Should it say 'We use a 32MiB' ?
>>>>
>>>> Please, mention that MAX_UNCOMP_KERNEL_SIZE = 32 MiB is ARMv7 specific.
>>>
>>> Sorry?  As I understood it last, the maximum size was something like
>>> 96MiB before you have to employ some funky tricks.
>>
>> Look at the use of MAX_UNCOMP_KERNEL_SIZE in handle_kernel_image() of
>> the EFI stub (drivers/firmware/efi/libstub/arm32-stub.c).
>>
>>>
>>>>>> +buffer from the start of memory as our default load address, and so where the
>>>>>> +kernel would also be loaded to.  This will hopefully allow for us to have the
>>>>
>>>> %s/allow for us/allow/
>>>>
>>>>>> +whole of the compressed kernel image exist in memory above where the whole of
>>>>>> +the decompressed kernel image will be, and allow for a quicker boot.  Next, we
>>>
>>> We use 32MiB for the reason I said here.  Which is only a slight
>>> rewording of the arm32 Linux booting document, and the section starts
>>> out by saying this is an example for ARMv7 platforms.
>>
>> You ask all other architectures to follow this example?
>
> I could have sworn that somewhere within the comments of this series I
> asked for more examples to be added, yes.  And I know I intended to
> (since we _need_ them, and I think I've expressed me desire to have them
> before) and I am asking now.
>
>>>> Please, mention that decompressor code otherwise will have to relocate
>>>> the compressed kernel.
>>>
>>> I'm not sure.  Perhaps it would be good to also link to some of the
>>> articles expanding upon how Linux on ARM32 boots, as part of more
>>> general documentation, rather than a specific example here.
>>
>> Just look at the comment above the definition of MAX_UNCOMP_KERNEL_SIZE
>> in arch/arm/include/asm/efi.h.
>
> Keep in mind this is only vaguely EFI-related.  Given how long ago edkII
> for beagleboard was done, it doesn't quite predate EFI on ARM, but this
> example has been in long use for the common non-EFI case on 32bit ARM.

The EFI stub is using the value due to (assumed) restrictions in the
decompressor and main kernel.

Best regards

Heinrich

>
>>>>>> +say that the device tree will be placed at 128MiB offset from the start of
>>>>
>>>> Please, mention that initrd must be
>>>>
>>>> * within 512 MiB (0x20000000) of the memory start on arm
>>>>     (which restricts initrd_high)
>>>> * in a a 1 GB aligned region of size '1UL << (VA_BITS_MIN - 1)' that
>>>>     includes the kernel on arm64
>>>
>>> No, because this is not intended to list every constraint on ARM32 (nor
>>> arm64, which would benefit from an example that's not TI, as TI arm64
>>> platforms share the same base address for memory).
>>
>> You ask above to follow the example of ARMv7 on all architectures. Hence
>> it is necessary to point out the differences.
>
> No, I'm asking for more examples to be added for each architecture.
>

Re: [PATCH 3/3] doc: environment: Further expand on Image locations and provide example

[Tom Rini]

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

On Mon, Jul 11, 2022 at 03:10:36PM +0200, Heinrich Schuchardt wrote:
> On 7/11/22 14:41, Tom Rini wrote:
> > On Mon, Jul 11, 2022 at 08:42:08AM +0200, Heinrich Schuchardt wrote:
> > > On 7/10/22 18:17, Tom Rini wrote:
> > > > On Sun, Jul 10, 2022 at 02:26:04PM +0200, Heinrich Schuchardt wrote:
> > > > > On 6/30/22 12:06, Simon Glass wrote:
> > > > > > On Mon, 20 Jun 2022 at 08:32, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > 
> > > > > > > Start by elaborating on what some of our constraints tend to be with
> > > > > > > image location values, and document where these external constraints
> > > > > > > come from.  Provide a new subsection, an example based on the TI ARMv7
> > > > > > > OMAP2PLUS families of chips, that gives sample values and explains why
> > > > > > > we use these particular values.  This is based on what is in
> > > > > > > include/configs/ti_armv7_common.h as of fb3ad9bd923d ("TI: Add, use a
> > > > > > > DEFAULT_LINUX_BOOT_ENV environment string") as this contains just the
> > > > > > > values referenced in this document now and not some of the further
> > > > > > > additions that are less generic.
> > > > > > > 
> > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > ---
> > > > > > >     doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
> > > > > > >     1 file changed, 39 insertions(+)
> > > > > > 
> > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > 
> > > > > Below you want a change?
> > > > 
> > > > Yes, often Simon does that (and it's fine) to both offer a tag but if
> > > > another iteration is needed to make a minor adjustment to some wording
> > > > or another, or to make when applying.  Which is fine with me.
> > > > 
> > > > > > > diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> > > > > > > index a9a4702632d2..f70ccd6a58ee 100644
> > > > > > > --- a/doc/usage/environment.rst
> > > > > > > +++ b/doc/usage/environment.rst
> > > > > > > @@ -404,6 +404,42 @@ device tree blob  fdtfile        fdt_addr_r       fdt_addr
> > > > > > >     ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
> > > > > > >     ================= ============== ================ ==============
> > > > > > > 
> > > > > > > +When setting the RAM addresses for `kernel_addr_r`, `fdt_addr_r` and
> > > > > > > +`ramdisk_addr_r` there are several constraints to keep in mind. When booting
> > > > > > > +Linux, the `Booting ARM Linux`_ and `Booting AArch64 Linux`_ documents lay out
> > > > > > > +the requirements for booting all ARM platforms, including both alignment and
> > > > > > > +where within memory various things must be.  These guidelines tend to also be
> > > > > > > +correct for other OSes and unless specifically contradicted by documentation
> > > > > 
> > > > > What makes you think that BSD or Haiku have the same constraints as Linux?
> > > > 
> > > > Because of what I said, and experience?  Now, one may be a subset of
> > > > another, but that still means it will work for both.  This is intended
> > > > to be general best practices.  If you follow this then it's likely
> > > > anything else will work too.  The danger comes from trying to optimize
> > > > the sizes to be as small as possible, rather than as large/flexible as
> > > > will likely work anywhere.  I will try and expand on that idea in the
> > > > next iteration.
> > > > 
> > > > > > > +specific to another architecture, are good rules to follow for other
> > > > > > > +architectures as well.
> > > > > 
> > > > > No. RISC-V does not have the same requirements as ARM. E.g. the initrd
> > > > > can be located anywhere in memory.
> > > > 
> > > > Please point to documentation that confirms that, and some otherwise bad
> > > > examples that actually work.
> > > 
> > > [PATCH 1/1] RISC-V: load initrd wherever it fits into memory
> > > https://lore.kernel.org/all/20210629134018.62859-1-xypron.glpk@gmx.de/
> > 
> > Looks like someone ran in to the first common case of "oops, overwrote
> > the ramdisk with the kernel bss" or something along those lines.
> 
> Not at all. The ramdisk was relocated by U-Boot unnecessarily to above
> 256 MiB after start of RAM and the EFI stub before the patch did not
> accept this address for no reason.

Then bootm_size / bootm_low should have been set appropriately in
U-Boot.

> > Which is why I'm asking for more architectures to add good examples of
> > where to load each payload in to memory, with explanations of why and
> > how big of a gap to have.  I _think_ in Linux RISC-V (and hopefully for
> > 32bit and 64bit) used the arm64 Image format and so BSS size is
> > available in the header and so we can safely check for that overlap and
> > relocate rather than fail to boot.  Checking for, and avoiding to start
> > with, these types of problems is why I want to add the examples.
> 
> I am not aware of any restrictions for the placement of kernel, initrd,
> fdt for RISC-V. Therefore there is no need to relocate anything after
> loading (without overlap).
> 
> The bootefi command will never relocate a kernel or an fdt on any
> architecture. You just pass the original load addresses.

Which is all the more reason we need to ensure that the fdt and initrd
are placed in appropriately aligned and non-overlapping locations.
Because if you put the initrd too close to where the kernel ends up, the
BSS will eat your ramdisk before it can go "oh, there's a ramdisk
there".  That's a practical, not architectural, problem.  Same with the
device tree.

> > > Please, have a look at efi_get_max_initrd_addr() in these files:
> > > 
> > > arch/arm/include/asm/efi.h:73
> > > arch/arm64/include/asm/efi.h:77
> > > arch/loongarch/include/asm/efi.h:36
> > > arch/riscv/include/asm/efi.h:31
> > > 
> > > MAX_UNCOMP_KERNEL_SIZE = 32 MiB is only enforced in
> > > drivers/firmware/efi/libstub/arm32-stub.c.
> > 
> > This isn't an EFI thing however.  The max uncompressed Linux kernel
> > image for arm32 is something along the lines of 96MiB I recall rmk
> > telling me when I asked about it at the time.  The base+32MiB in the
> > example here is for optimal (but not REQUIRED) decompressor location.
> 
> The decompressor is what follows the EFI stub in the image?

I'm not talking about EFI at all in this example, as it's 32bit ARM, not
64bit ARM or RISC-V.

> > > > > > > +Example Image locations
> > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^
> > > > > 
> > > > > You seem not to refer to a file 'Image'.
> > > > > 
> > > > > %s/Image/image/
> > > > 
> > > > OK.
> > > > 
> > > > > > > +
> > > > > > > +If we take the Texas Instruments OMAP2PLUS family of ARMv7 processors as an
> > > > > > > +example for the above listed variables, we would do::
> > > > > 
> > > > > %s/we would do/we chose/ ?
> > > > 
> > > > Either?  I don't see it mattering either way.
> > > > 
> > > > > > > +
> > > > > > > +    loadaddr=0x82000000
> > > > > > > +    kernel_addr_r=${loadaddr}
> > > > > > > +    fdt_addr_r=0x88000000
> > > > > > > +    ramdisk_addr_r=0x88080000
> > > > > > > +    bootm_size=0x10000000
> > > > > > > +
> > > > > > > +To explain this, we start by noting that DRAM starts at 0x80000000.  A 32MiB
> > > > > > 
> > > > > > Should it say 'We use a 32MiB' ?
> > > > > 
> > > > > Please, mention that MAX_UNCOMP_KERNEL_SIZE = 32 MiB is ARMv7 specific.
> > > > 
> > > > Sorry?  As I understood it last, the maximum size was something like
> > > > 96MiB before you have to employ some funky tricks.
> > > 
> > > Look at the use of MAX_UNCOMP_KERNEL_SIZE in handle_kernel_image() of
> > > the EFI stub (drivers/firmware/efi/libstub/arm32-stub.c).
> > > 
> > > > 
> > > > > > > +buffer from the start of memory as our default load address, and so where the
> > > > > > > +kernel would also be loaded to.  This will hopefully allow for us to have the
> > > > > 
> > > > > %s/allow for us/allow/
> > > > > 
> > > > > > > +whole of the compressed kernel image exist in memory above where the whole of
> > > > > > > +the decompressed kernel image will be, and allow for a quicker boot.  Next, we
> > > > 
> > > > We use 32MiB for the reason I said here.  Which is only a slight
> > > > rewording of the arm32 Linux booting document, and the section starts
> > > > out by saying this is an example for ARMv7 platforms.
> > > 
> > > You ask all other architectures to follow this example?
> > 
> > I could have sworn that somewhere within the comments of this series I
> > asked for more examples to be added, yes.  And I know I intended to
> > (since we _need_ them, and I think I've expressed me desire to have them
> > before) and I am asking now.
> > 
> > > > > Please, mention that decompressor code otherwise will have to relocate
> > > > > the compressed kernel.
> > > > 
> > > > I'm not sure.  Perhaps it would be good to also link to some of the
> > > > articles expanding upon how Linux on ARM32 boots, as part of more
> > > > general documentation, rather than a specific example here.
> > > 
> > > Just look at the comment above the definition of MAX_UNCOMP_KERNEL_SIZE
> > > in arch/arm/include/asm/efi.h.
> > 
> > Keep in mind this is only vaguely EFI-related.  Given how long ago edkII
> > for beagleboard was done, it doesn't quite predate EFI on ARM, but this
> > example has been in long use for the common non-EFI case on 32bit ARM.
> 
> The EFI stub is using the value due to (assumed) restrictions in the
> decompressor and main kernel.

OK, but EFI is largely irrelevant to the 32bit ARM case, which is what
this example is for.

-- 
Tom

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

[PATCH 1/3] doc: environment: Drop u-boot_addr_r

[Tom Rini]

This variable is never set nor explained why it would be set, drop it.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v2:
- None
---
 doc/usage/environment.rst | 1 -
 1 file changed, 1 deletion(-)

diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index 28a8952b7528..a3f23d4e4e22 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -390,7 +390,6 @@ in U-Boot code.
 ================= ============== ================ ==============
 Image             File Name      RAM Address      Flash Location
 ================= ============== ================ ==============
-u-boot            u-boot         u-boot_addr_r    u-boot_addr
 Linux kernel      bootfile       kernel_addr_r    kernel_addr
 device tree blob  fdtfile        fdt_addr_r       fdt_addr
 ramdisk           ramdiskfile    ramdisk_addr_r   ramdisk_addr
-- 
2.25.1