[PATCH 00/49] image: Reduce #ifdefs and ad-hoc defines in image code

[Simon Glass]

Hi Tom,

On Tue, 4 May 2021 at 14:40, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, May 03, 2021 at 05:10:47PM -0600, Simon Glass wrote:
>
> > Much of the image-handling code predates the introduction of Kconfig and
> > has quite a few #ifdefs in it. It also uses its own IMAGE_... defines to
> > help reduce the #ifdefs, which is unnecessary now that we can use
> > IS_ENABLED() et al.
> >
> > The image code is also where quite a bit of code is shared with the host
> > tools. At present this uses a lot of checks of USE_HOSTCC.
> >
> > This series introduces 'host' Kconfig options and a way to use
> > CONFIG_IS_ENABLED() to check them. This works in a similar way to SPL, so
> >
> >    CONFIG_IS_ENABLED(FIT)
> >
> > will evaluate to true on the host build (USE_HOSTCC) if CONFIG_HOST_FIT is
> > enabled. This allows quite a bit of clean-up of the image.h header file
> > and many of the image C files.
> >
> > The 'host' Kconfig options should help to solve a more general problem in
> > that we mostly want the host tools to build with all features enabled, no
> > matter which features the 'target' build actually uses. This is a pain to
> > arrange at present, but with 'host' Kconfigs, we can just define them all
> > to y.
> >
> > There are cases where the host tools do not have features which are
> > present on the target, for example environment and physical addressing.
> > To help with this, some of the core image code is split out into
> > image-board.c and image-host.c files.
> >
> > Even with these changes, some #ifdefs remain (101 down to 42 in
> > common/image*). But the code is somewhat easier to follow and there are
> > fewer build paths.
> >
> > In service of the above, this series includes a patch to add an API function
> > for zstd, so the code can be dropped from bootm.c
> >
> > It also introduces a function to handle manual relocation.
>
> I like this idea overall.  The good news is this reduces the size in a
> few places.  The bad news, but I can live with if we can't restructure
> the changes more, is a few functions grow a bit.  This shows the good
> and the bad (something like sama5d2_ptc_ek_mmc shows only growth, to be
> clear):
>             px30-core-edimm2.2-px30: all -36 rodata -24 text -12
>                u-boot: add: 0/0, grow: 3/-4 bytes: 36/-48 (-12)
>                  function                                   old     new   delta
>                  boot_get_fdt                               896     924     +28
>                  image_decomp                               372     376      +4
>                  boot_get_ramdisk                           868     872      +4
>                  do_bootm_vxworks                           552     540     -12
>                  do_bootm_rtems                             124     112     -12
>                  do_bootm_plan9                             228     216     -12
>                  do_bootm_netbsd                            324     312     -12
>             odroid-c2      : all -105 bss +128 rodata -65 text -168
>                u-boot: add: 0/0, grow: 2/-3 bytes: 108/-172 (-64)
>                  function                                   old     new   delta
>                  images                                     504     608    +104
>                  image_decomp                               372     376      +4
>                  image_setup_linux                          108      96     -12
>                  boot_get_ramdisk                           620     580     -40
>                  boot_get_fdt                               660     540    -120
>             origen         : all +47 bss +96 rodata -57 text +8
>                u-boot: add: 0/0, grow: 15/-2 bytes: 180/-104 (76)
>                  function                                   old     new   delta
>                  images                                     288     340     +52
>                  do_bootm_states                           1304    1348     +44
>                  do_bootz                                   164     176     +12
>                  do_bootm_vxworks                           332     344     +12
>                  image_setup_libfdt                         168     176      +8
>                  image_decomp                               156     164      +8
>                  bootm_find_images                          212     220      +8
>                  boot_prep_linux                            276     284      +8
>                  image_setup_linux                           54      58      +4
>                  do_bootm_standalone                         60      64      +4
>                  do_bootm_plan9                             104     108      +4
>                  do_bootm_netbsd                            168     172      +4
>                  boot_prep_vxworks                           48      52      +4
>                  boot_jump_vxworks                            6      10      +4
>                  boot_jump_linux                            148     152      +4
>                  boot_get_ramdisk                           420     392     -28
>                  boot_get_fdt                               420     344     -76
>
> And looking at ls1088ardb_sdcard_qspi_SECURE_BOOT I think there might be
> something wrong as that looks to drop all crypto algos from SPL.  Other
> layerscape SECURE_BOOT configs show this as well.  It does however seem
> to clear up some other issues around unused code, so a deeper dive on
> which patch is dropping stuff is needed.  I see a huge drop on
> am65x_evm_a53 / j721e_evm_a72 SPL as well but I can test those and at
> least the basic case is fine.  socfpga_agilex_atf is one I don't know
> about being right or wrong.  socfpga_agilex_vab dropping hashing code
> does look worrying however, but maybe it's a configuration issue in the
> end?

Yes it seems to be config. Thank you for all the pointers.

The small increase is unavoidable with this approach - basically we
add 16 bytes of rodata as part of making the switch cases into if()
instead of #ifdef. I found that SPL hashing was dropped, which
explained another problem that I had seen but not yet diagnosed.

I will send v2.

Regards,
Simon