[U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size

[Tom Rini]

On Tue, Nov 27, 2018 at 07:31:24PM +0100, Simon Goldschmidt wrote:
> On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut at gmail.com> wrote:
> >
> > On 11/27/2018 04:26 PM, Simon Goldschmidt wrote:
> > > On 27.11.2018 14:09, Marek Vasut wrote:
> > >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote:
> > >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut at gmail.com>
> > >>> wrote:
> > >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote:
> > >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut at gmail.com>
> > >>>>> wrote:
> > >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop
> > >>>>>> both
> > >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd
> > >>>>>> will always be copied into the first 256 MiB of RAM instead of being
> > >>>>>> used in place, which can cause various kinds of inobvious problems.
> > >>>>>>
> > >>>>>> The simpler problems include FDT or initrd being overwritten or being
> > >>>>>> used from unaligned addresses, especially on ARM64. The overhead of
> > >>>>>> copying the FDT to aligned location is negligible and these problems
> > >>>>>> go away, so the benefit is significant.
> > >>>>>>
> > >>>>>> Regarding alignment problems with fitImage. The alignment of DT
> > >>>>>> properties
> > >>>>>> is always 32 bits, which implies that the alignment of the "data"
> > >>>>>> property
> > >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role
> > >>>>>> here. The
> > >>>>>> kernel expects all elements, including DT and initrd, to be
> > >>>>>> aligned to
> > >>>>>> 64 bits on ARM64, thus using them in place may not be possible.
> > >>>>>> Using the
> > >>>>>> bootm_size assures correct alignment, again with negligible overhead.
> > >>>>> In my opinion, all of these raw addresses defined in scripts or config
> > >>>>> should be removed: They are probably vulnerable to overwriting
> > >>>>> themselves as they only provide an address, not a range.
> > >>>> This is not an address, it's size. And this one at least assures that
> > >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during
> > >>>> bootm time.
> > >>> Sorry I did not express myself clear enough. I meant that "fdt_high"
> > >>> and "initrd_high" are bad because they contain an address only, not a
> > >>> range. The 'bootm_size' thing is much better!
> > >> Well the fdt_high and intrd_high can also contain a special ~0 value,
> > >> which says "use the fdt/initrd in place", which is dangerous.
> > >>
> > >>>>> Just out of curiosity: is it required to put fdt and initrd into the
> > >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb
> > >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my
> > >>>>> series to fix the recent CVE issues improves lmb to not overwrite
> > >>>>> U-Boot and other reserved addresses and you might be able to remove
> > >>>>> 'bootm_size', too. The improved lmb code would just allocate an
> > >>>>> aligned address somewhere in the available RAM.
> > >>>> It's just the first 256 MiB from the beginning, so there's enough space
> > >>>> between that and U-Boot on all these boards.
> > >>> Of course. I wanted to know if it would be good enough if U-Boot would
> > >>> just put it somewhere without overwriting things or do you really need
> > >>> them in the first 256 MiB? Because the revised lmb code would make
> > >>> sure there's nothing overwritten, so there would be no need to trim at
> > >>> 256 MiB.
> > >> You can put them anywhere, you just need to meet the alignment
> > >> requirements. Can the new LMB code help somehow with that ? And if so,
> > >> how ?
> > >
> > > My additions to the LMB code should only ensure nothing gets overwritten
> > > so you don't have to limit boom_size to 256MiB (but use the complete RAM
> > > when bootm_size is not set).
> > > Alignment does not change but should already be OK with LMB as you use it?
> >
> > If I can use the entire RAM (except U-Boot and fitImage), that'd be
> > nice. What change do I need to do ?
> 
> I don't know yet, sorry. I basically asked this question to find out
> about the usage of 'bootm_size'. It's not really documented and I
> couldn't find out it's full meaning yet. Because e.g. it is set to 16
> MiB for socfpga gen5, which sounds a little low...
> 
> From reading the code, doesn't it already work when leaving out
> 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not
> defining CONFIG_SYS_BOOTMAPSZ?)

It's all a little funny, yes.  common/image.c is where all of this gets
laid out and we keep everything being passed to bootm (whatever image
type it may be) between bootm_low (either set explicitly or start of
first bank of DRAM) and bootm_size (default ends up basically being all
of DRAM) and then we do lmb stuff.

The main reason, as I replied just now earlier in the thread, is that
for Linux we need to make sure everything is in "lowmem" and rather than
disabling relocation (which leads to people seeing fail to boots when
something overlaps something else, over time, and they spend a bunch of
time debugging) via initrd_high/fdt_high=0xffffffff SoCs should instead
set a reasonable constraint on bootm_size so that we can make sure
nothing overlaps, when we can do that.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181127/59ef6df4/attachment.sig>