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

[Tom Rini <trini@konsulko.com>]

[-- 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 --]