[PATCH 1/1] image: usage of value ~0UL for intrd_high

[Tom Rini <trini@konsulko.com>]

On Sun, Jan 10, 2021 at 05:20:50PM +0100, Heinrich Schuchardt wrote:
> On 1/10/21 2:43 PM, Tom Rini wrote:
> > On Sun, Jan 10, 2021 at 01:05:14PM +0100, Heinrich Schuchardt wrote:
> > > On 1/9/21 10:23 PM, Tom Rini wrote:
> > > > On Sat, Jan 09, 2021 at 08:59:01PM +0100, Heinrich Schuchardt wrote:
> > > > > Am 9. Januar 2021 20:40:04 MEZ schrieb Tom Rini <trini@konsulko.com>:
> > > > > > On Sat, Jan 09, 2021 at 08:33:40PM +0100, Heinrich Schuchardt wrote:
> > > > > > > On 1/9/21 7:58 PM, Tom Rini wrote:
> > > > > > > > On Sat, Jan 09, 2021 at 08:47:07PM +0200, Andy Shevchenko wrote:
> > > > > > > > > On Sat, Jan 9, 2021 at 8:06 PM Heinrich Schuchardt
> > > > > > <xypron.glpk@gmx.de> wrote:
> > > > > > > > > > 
> > > > > > > > > > The comment for initrd_high in the coding and in README were
> > > > > > contradicting
> > > > > > > > > > and neither fully described what the coding does.
> > > > > > > > > > 
> > > > > > > > > > Clarify the usage of the special value ~0UL for the environment
> > > > > > variable
> > > > > > > > > > initrd_high.
> > > > > > > > > 
> > > > > > > > > All those F:s are hard to read in the comments and documentation
> > > > > > and
> > > > > > > > > typo prone. I would prefer to rephrase like "all 1:s value in 32-
> > > > > > or
> > > > > > > > > 64-bit format" or alike.
> > > > > > > > 
> > > > > > > > If we're going to improve this we should also note it's discouraged
> > > > > > > > unless you know for certain there will be no overlap and it's
> > > > > > strongly
> > > > > > > > discouraged in default environments.
> > > > > > > 
> > > > > > > What exactly is discouraged?
> > > > > > > 
> > > > > > > * setting initrd_high to a value != ~0? Here I would agree.
> > > > > > > * setting intird_high to ~0? Why should we copy initrd to a
> > > > > > >     different place? Is it for some outdated Linux release?
> > > > > > 
> > > > > > We should always default to allowing the initrd to be relocated because
> > > > > > we can see (in many cases) overlap that will lead to failure to boot
> > > > > > but
> > > > > > this forces us to ignore that.  Having good default load values means
> > > > > > we
> > > > > > don't have a problem here.
> > > > > 
> > > > > We have an initrd that is already in memory. What could it overlap
> > > > > with that is not already overwritten?
> > > > 
> > > > Having the kernel and initrd too close in memory has the kernel BSS
> > > > overwrite the initrd.  This has happened time and time again before
> > > > I went around making some platforms have reasonable (ie kernel early,
> > > > ramdisk in lowmem but beyond where a kernel+bss can be, etc) defaults
> > > > and pushing others to do the same.
> > > > 
> > > > > Can you provide the text you want to see here?
> > > > 
> > > > Off-hand, it should look more like the big comment block in
> > > > include/configs/ti_armv7_common.h and reference the Linux booting on
> > > > arm/arm64 documents while noting that other architectures have the same
> > > > fundamental issues and their exact limits may or may not be as well
> > > > documented.
> > > > 
> > > 
> > > There is nothing in the include/configs/ti_armv7_common.h comments
> > > requiring to relocate initrd.
> > 
> > /*
> >   * We setup defaults based on constraints from the Linux kernel, which should
> >   * also be safe elsewhere.  We have the default load at 32MB into DDR (for
> >   * the kernel), FDT above 128MB (the maximum location for the end of the
> >   * kernel), and the ramdisk 512KB above that (allowing for hopefully never
> >   * seen large trees).  We say all of this must be within the first 256MB
> >   * as that will normally be within the kernel lowmem and thus visible via
> >   * bootm_size and we only run on platforms with 256MB or more of memory.
> >   *
> >   * As a temporary storage for DTBO blobs (which should be applied into DTB
> >   * blob), we use the location 15.5 MB above the ramdisk. If someone wants to
> >   * use ramdisk bigger than 15.5 MB, then DTBO can be loaded and applied to DTB
> >   * blob before loading the ramdisk, as DTBO location is only used as a temporary
> >   * storage, and can be re-used after 'fdt apply' command is done.
> >   */
> 
> I cannot see how this relates to initrd relocation.

You cannot safely disable initrd relocation without ensuring it won't be
locked in an unsafe location first.  It's also _generally_ going to be
noise in the boot sequence and not an advisable default.  It should be
documented as part of how to tune a system for optimal boot times.  But
given how often defaults are copy/pasted to the next platform without
fully understanding why those values were picked is why what we use as
default in one platform needs to be very carefully done.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210115/4e632d1b/attachment.sig>