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

[Marek Vasut]

On 11/01/2018 10:13 PM, Simon Goldschmidt wrote:
> 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.

Ouch :(

> 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...

Makes sense.

> 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.

Wouldn't the stack usage change with usage too ? I think it might,
albeit slightly. I'm not sure this would be really useful then.

-- 
Best regards,
Marek Vasut