[U-Boot] [PATCH] ARM: crt0: Pass malloc base address

[Simon Glass]

Hi Albert,

On 11 November 2015 at 23:57, Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:
> Hello Simon,
>
> On Wed, 11 Nov 2015 14:49:05 -0700, Simon Glass <sjg at chromium.org>
> wrote:
>> Hi Fabio,
>>
>> On 11 November 2015 at 14:24, Fabio Estevam <festevam at gmail.com>
>> wrote:
>> > Hi Simon,
>> >
>> > On Wed, Nov 11, 2015 at 7:08 PM, Simon Glass <sjg at chromium.org>
>> > wrote:
>> >
>> >> That test is intended to avoid setting up simple malloc() if we
>> >> plan to use full malloc() in SPL. Of course, full malloc() is set
>> >> up a little later (in spl_init()). But we should not need both -
>> >> either we use simple malloc() or full malloc().
>> >>
>> >> But for your board I see:
>> >>
>> >> $ grep CONFIG_SYS_SPL_MALLOC_SIZE b/mx6sabresd_spl/u-boot.cfg
>> >> #define CONFIG_SYS_SPL_MALLOC_SIZE 0x3200000
>> >>
>> >> So you will not be able to use simple malloc(). I'd suggest calling
>> >> spl_init() from board_init_f() if you need malloc() there. But it
>> >
>> > Yes, I do call spl_init() from board_init_f() prior to calling
>> > malloc() and this has been working fine prior to 5ba534d247d418.
>> >
>> >> presumably needs to be done after you have SDRAM up. So perhaps
>> >
>> > The trick here is that I need malloc to work prior to have SDRAM
>> > configured.
>> >
>> > On cgtqmx6eval we need to read SPI NOR to determine what kind of DDR
>> > we will need to configure.
>> >
>> > Otavio has sent the SPL support for cgtqmx6eval, but it has not
>> > reached U-boot mainline yet.
>> >
>> > I am reproducing the problem on a mx6sabresd_spl target with the
>> > simple example code I sent previously.
>>
>> I see. That's not a use case I have seen before.
>>
>> I'd suggest changing the #ifdef to always set up early malloc() if
>> CONFIG_SYS_MALLOC_F is set. There will of course be a new malloc()
>> once spi_init() is called, but that does not matter.
>>
>> In your patch, please be careful to add some docs to the README on
>> this point (the early malloc() is only there to permit DRAM init,
>> etc.). It could get quite confusing...
>
> I'm not sure all details are clear to me so let me chime in on this.
>
> IIUC, what Fabio needs is a working C runtime including malloc, based on
> some IRAM rather than SDRAM.
>
> This means there will be two phases where malloc can be used, the usual
> one after SDRAM init, and a new one before SDRAM init.
>
> Of course, the amount of memory reserved for the malloc arena cannot
> be the same in IRAM as it will be in SDRAM, meaning that the routine
> which reserves the arena must handle both cases, by detecting whether
> it is running before or after SDRAM init and choosing the right malloc
> size macro, or by just bein passed that info from the code responsible
> for maintaining the C runtime environment (arch/arm/lib/crt0.C in the
> ARM case) -- via the 'chunk_id' method I described earlier in
> https://www.mail-archive.com/u-boot@lists.denx.de/msg191898.html
>
> There is a *VERY* *BIG* *PROBLEM* to the whole idea of
> malloc-before-SDRAM:
>
> We could manage to transfer the pre-SDRAM-init maloc arena content to
> the post-SDRAM-init malloc arena, thus preserving all pre- SDRAM-init
> mallocs() across the stack switch.
>
> *BUT*: all (variable-stored) pointers to pre-SDRAM-malloc'ed space will
> become dangling pointers, because we have no easy way of telling where
> these pointers are and relocating them.
>
> This means that the mallocs() done before SDRAM init are lost and that
> al pointers to them /will/ become dangling, including any copies and
> derivatives of these; all code which depend these pointers and their
> will have to handle the situation, which will prove *quite* complex or,
> to be blunter, will prove not actually done, and will prove so through
> weird bugs creeping up that we'll take some time to relate to malloc
> issues, and to fix.
>
> Of course, we could just decide that any malloc done before SDRAM init
> is lost and that user code should deal with it. But I fear it will do so
> just as badly as it would handle the "maloc transfer" alternative I just
> described. Plus, if the malloc'ed data contains hardware module state
> or any non-recoverable-again information, then we *cannot* just drop
> it.
>
> Now, there is a systemic solution: that all code which stores a
> malloc-arena-based pointer value in memory call a system-provided
> function which, if running pre-SDRAM-init, would record the location of
> the pointer in a list; upon C environment switch from pre- to
> post-SDRAM contexts, the list would be run through and each pointer
> would be 'relocated' from the old to the new maloc arena. Post-init,
> the functon would not do any recording. That causes a slight
> performance hit on mallocs, but I don't think it'll affect boot time
> that much.
>
> Only code which has to run before SDRAM init would have to use the
> function.
>
> The benefit of this approach is that maloc'ed space would remain
> malloc'ed after SDRAM init and declared pointers to it would remain
> valid. Code which has to malloc before SDRAM init would not have to
> re-malloc or fix anything, or even realize the stack and malloc arena
> have moved, as long as it has declared its malloc-related pointer(s).
>
> Of course the list would be limited. But seeing as we'd be running in a
> tight context and only to get SDRAM running, the list would not extend
> much.
>
> I imagine a scheme where the list is kept in the malloc arena. The
> malloc space itself would still grow 'down' from the stack top while
> the pointer list would grow 'up' from the stack 'bottom' limit.
>
> I could whip up an RFC patch (with ARM support, to be extended to other
> platforms as my recent experience showed I'm not that good at NIOS2 for
> instance) if people are interested.

I'm really not sure that this problem is worth solving.

We have quite a hard boundary between board_init_f() and
board_init_r() in U-Boot proper. For some platforms the memory
available in the former is not available in the latter. People
understand (I think) that pre-relocation stuff goes away and exists
only to assist with bringing up the new environment.

Driver model is started up in board_init_f() but then the whole thing
is thrown away and started again in board_init_r(). So if someone kept
a device pointer around for later (e.g. in global_data) it would not
work (Don't Do That!). This approach has not caused any reported
problems yet.

While you can copy memory and fix up pointers returned by malloc(),
pointers to rodata and other things may still exist. It would be
tedious or error-prone to relocate these.

My preference would be to keep it simple, and just make it clear that
board_init_f() may have an early malloc(), but even if it does it goes
away in board_init_r(). That rule can apply to SPL just as easily as
U-Boot proper.

Regards,
Simon