[U-Boot] [PATCH] arm: socfpga: check total size of SPL

[Simon Goldschmidt]

On 01.11.2018 21:59, Marek Vasut wrote:
> On 11/01/2018 09:31 PM, Simon Goldschmidt wrote:
>> On Thu, Nov 1, 2018 at 8:26 PM Simon Goldschmidt
>> <simon.k.r.goldschmidt at gmail.com> wrote:
>>> On 31.10.2018 11:00, Marek Vasut wrote:
>>>> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote:
>>>>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote:
>>>>>>> On 30.10.2018 22:26, Marek Vasut wrote:
>>>>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote:
>>>>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm
>>>>>>>>> linker script for SPL check the total SRAM size available for SPL
>>>>>>>>> (code, data, bss, heap, global data).
>>>>>>>>>
>>>>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only
>>>>>>>>> check the binary size (which is without bss, heap and gd).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>     include/configs/socfpga_common.h | 7 +++++++
>>>>>>>>>     1 file changed, 7 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/include/configs/socfpga_common.h
>>>>>>>>> b/include/configs/socfpga_common.h
>>>>>>>>> index 2330143cf1..9103d0a966 100644
>>>>>>>>> --- a/include/configs/socfpga_common.h
>>>>>>>>> +++ b/include/configs/socfpga_common.h
>>>>>>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>>>>>>>>>     #define CONFIG_SPL_TEXT_BASE        CONFIG_SYS_INIT_RAM_ADDR
>>>>>>>>>     #define CONFIG_SPL_MAX_SIZE        CONFIG_SYS_INIT_RAM_SIZE
>>>>>>>>>     +/* Check total size of SPL including BSS, malloc area and gd */
>>>>>>>>> +#include <generated/generic-asm-offsets.h>
>>>>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
>>>>>>>>> +                     CONFIG_SYS_INIT_RAM_ADDR - \
>>>>>>>>> +                     CONFIG_SYS_MALLOC_F_LEN - \
>>>>>>>>> +                     GENERATED_GBL_DATA_SIZE)
>>>>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the
>>>>>>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or
>>>>>>>> something ?
>>>>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR +
>>>>>>> INIT_RAM_SIZE.
>>>>>>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
>>>>>>> - MALLOC_F_LEN - GBL_DATA_SIZE".
>>>>>>>
>>>>>>> But I did it this way to keep it working after Stefan's fix for
>>>>>>> reserving the boot counter location is applied.
>>>>>> Now that's confusing :-) Add a comment explaining this into a V2 please,
>>>>>> otherwise the confusion will continue ...
>>>>> You're probably right. I'll send a V2 making this more clear.
>>>> Thanks
>>> Re-checking this, I found the check is still not correct as it would
>>> leave 4 bytes for the stack only. Is there an option that controls how
>>> much stack should be preserved for SPL somewhere?
>> And to make matters worse, the devicetree is also not included in the
>> size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select
>> CONFIG_OF_EMBED.
>>
>> While this would be OK for SPL, I think it wouldn't be for U-Boot. How
>> do other platforms handle this size check?
> Can git grep help here ?

Not really. If so, I wouldn't have asked ;-)

I can only git grep for known strings and I only see constants for 
CONFIG_SPL_MAX_SIZE.
CONFIG_SPL_MAX_FOOTPRINT seems to be rarely used and no platform seems 
to be taking the devicetree into account.

Stack usage seems to be covered in one or two boards, but I expect this 
to be quite platform specific (given he different drivers at least), so 
we might need an additional runtime check, which I haven't seen so far? 
In my (smaller) embedded projects, I often enable runtime stack checks 
to be on the safe side...

Maybe printing free memory at build-time and printing stack usage at 
runtime would be a solution? But I only know U-Boot from the socfpga 
gen5 perspective, that's why I ask.

Simon