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

Mon Jul 11 15:10:36 CEST 2022

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 at 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 at konsulko.com>
>>>>>> ---
>>>>>>     doc/usage/environment.rst | 39 +++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 39 insertions(+)
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg at 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.
>