[U-Boot] [PATCH v3] spl: add overall SPL size check

[Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>]

Am 28.05.2019 um 17:18 schrieb Jean-Jacques Hiblot:
> 
> On 27/05/2019 17:15, Simon Goldschmidt wrote:
>>
>>
>> Tom Rini <trini at konsulko.com <mailto:trini@konsulko.com>> schrieb am 
>> Mo., 27. Mai 2019, 16:54:
>>
>>     On Mon, May 27, 2019 at 03:47:13PM +0200, Jean-Jacques Hiblot wrote:
>>     > Simon,
>>     >
>>     >
>>     > On 24/05/2019 22:10, Simon Goldschmidt wrote:
>>     > >Am 24.05.2019 um 22:07 schrieb Simon Goldschmidt:
>>     > >>This adds a size check for SPL that can dynamically check
>>     generated
>>     > >>SPL binaries (including devicetree) for a size limit that ensures
>>     > >>this image plus global data, heap and stack fit in initial SRAM.
>>     > >>
>>     > >>Since some of these sizes are not available to make, a new
>>     host tool
>>     > >>'spl_size_limit' is added that dumps the resulting maximum
>>     size for
>>     > >>an SPL binary to stdout. This tool is used in toplevel Makefile to
>>     > >>implement the size check on SPL binaries.
>>     > >>
>>     > >>Signed-off-by: Simon Goldschmidt
>>     <simon.k.r.goldschmidt@gmail.com
>>     <mailto:simon.k.r.goldschmidt@gmail.com>>
>>     > >>---
>>     > >>
>>     > >>Changes in v3:
>>     > >>- don't build this new tools for 'make tools-only'
>>     > >
>>     > >So this is how far I got.
>>     > >
>>     > >Tom, your idea with making this multi-config aware (U-Boot, SPL
>>     and TPL)
>>     > >does not seem to work as 'tools' are only built once, not once per
>>     > >U-Boot/SPL/TPL. So if we wanted to use this for TPL, too, that
>>     would
>>     > >either mean create yet another tool or pass an option to this
>>     new tool to
>>     > >differ between SPL and TPL.
>>     >
>>     > If the trouble comes from GENERATED_GBL_DATA_SIZE, you could get
>>     its value
>>     > by parsing lib/asm-offsets.s. all the other values could be
>>     extracted from
>>     > {.,spl,tpl}/u-boot.cfg
>>
>>     Getting that file to exist has the same problem over for "tools-only".
>>
>>     > If this flies, it could be done by a python script without the
>>     need to
>>     > compile a program
>>
>>     I'm not sure that provides better clarity over what we have here tho.
>>
>>
>> I also think a python script would be less clear than a C tool. But it 
>> could have the problem hidden by not being used for "tools-only" - it 
>> would only be executed after linking SPL...
> 
> Yes that is  what I was trying to express.
> 
> Below is an draft of what I had in mind. Admittedly it less clear than 
> the C file.

If it works, then it's ok for me (maybe with a little added 
documentation to make it cleare). I'm not at all obsessed by my version :-)

Regards,
Simon

> 
> JJ
> 
>>
>> Regrds,
>> Simon
> 
> 
>   Makefile                |  2 +-
> 
>   tools/spl_size_limit.py | 51 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 52 insertions(+), 1 deletion(-)
>   create mode 100755 tools/spl_size_limit.py
> 
> diff --git a/Makefile b/Makefile
> index 8de3d4120a..440ea1da2d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -797,7 +797,7 @@ BOARD_SIZE_CHECK =
>   endif
> 
>   ifneq ($(CONFIG_SPL_SIZE_LIMIT),0)
> -SPL_SIZE_CHECK = @$(call size_check,$@,$$(tools/spl_size_limit))
> +SPL_SIZE_CHECK = $(call size_check,$@,$$($(src)/tools/spl_size_limit.py 
> SPL))
>   else
>   SPL_SIZE_CHECK =
>   endif
> diff --git a/tools/spl_size_limit.py b/tools/spl_size_limit.py
> new file mode 100755
> index 0000000000..2c40f5701e
> --- /dev/null
> +++ b/tools/spl_size_limit.py
> @@ -0,0 +1,51 @@
> +#! /usr/bin/env python
> +import os
> +import sys
> +
> +binary_types = {"U-BOOT":("","./"), "SPL":("SPL_","spl/"), 
> "TPL":("TPL_","tpl/")}
> +
> +def get_from_file(f, pattern):
> +    for l in f.readlines():
> +        if l.startswith(pattern):
> +            return l[len(pattern):].strip()
> +    return None
> +
> +def get_from_cfg_file(bintype, name):
> +    pattern = '#define CONFIG_{}{} '.format(binary_types[bintype][0], name)
> +    with open("{}u-boot.cfg".format(binary_types[bintype][1])) as f:
> +        return get_from_file(f, pattern)
> +
> +def get_from_header(fname, name):
> +    pattern = '#define {} '.format(name)
> +    with open("{}".format(fname)) as f:
> +        return get_from_file(f, pattern).split()[0]
> +
> +def usage():
> +    print("usage: {} [SPL|TPL]")
> +    sys.exit(1)
> +
> +if len(sys.argv) != 2 and len(sys.argv) != 1:
> +    usage()
> +if len(sys.argv) == 2 and sys.argv[1] not in binary_types.keys():
> +    usage()
> +
> +bintype = sys.argv[1]
> +
> +gbl_data_size = 
> int(get_from_header("include/generated/generic-asm-offsets.h",
> +                    "GENERATED_GBL_DATA_SIZE"), 0)
> +size_limit_subtract_gd = get_from_cfg_file(bintype, 
> "SIZE_LIMIT_SUBTRACT_GD")
> +size_limit_subtract_malloc = get_from_cfg_file(bintype,
> +                           "SIZE_LIMIT_SUBTRACT_MALLOC")
> +sys_malloc_f_len = int(get_from_cfg_file(bintype, "SYS_MALLOC_F_LEN"), 0)
> +size_limit_provide_stack = get_from_cfg_file(bintype,
> +                         "SIZE_LIMIT_PROVIDE_STACK")
> +
> +size_limit = int(get_from_cfg_file(bintype, "SIZE_LIMIT"), 0)
> +if size_limit_subtract_gd:
> +    size_limit = size_limit - gbl_data_size
> +if size_limit_subtract_malloc:
> +    size_limit = size_limit - sys_malloc_f_len
> +if size_limit_provide_stack:
> +    size_limit = size_limit - int(size_limit_provide_stack, 0)
> +
> +print(size_limit)
> --