| Message ID | 20210503231136.744283-1-sjg@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | image: Reduce #ifdefs and ad-hoc defines in image code | expand |
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?
Hi Tom, On Tue, 4 May 2021 at 15:40, Tom Rini <trini@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? OK thanks for that. I will take a look at the cases you mention. I found a fair few problems but clearly not all. Regards, Simon
Hi Tom, On 5/4/21 5:40 PM, Tom Rini 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): What tool do you use to generate this output? Thanks. --Sean > 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? >
On Tue, May 04, 2021 at 07:24:25PM -0400, Sean Anderson wrote: > Hi Tom, > > On 5/4/21 5:40 PM, Tom Rini 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): > What tool do you use to generate this output? Thanks. buildman will give that for you. This was from my world build before/after wrapper, but for a single machine I have: #!/bin/bash # Initial and constant buildman args ARGS="-devl" ALL=0 KEEP=0 # Find our arguments while test $# -ne 0; do if [ "$1" == "--all" ]; then ALL=1 shift 1 elif [ "$1" == "--branch" ]; then BRANCH=$2 shift 2 elif [ "$1" == "--keep" ]; then KEEP=1 ARGS="$ARGS -k" shift 1 else MACHINE=$1 shift fi done if [ -z $MACHINE ]; then echo Usage: $0 MACHINE [--all] [--keep] [--branch BRANCH] exit 1 fi # If not all, then only first/last if [ $ALL -ne 1 ]; then ARGS="$ARGS --step 0" fi if [ ! -z $BRANCH ]; then ARGS="$ARGS -b $BRANCH" else ARGS="$ARGS -b `git rev-parse --abbrev-ref HEAD`" fi mkdir -p /tmp/$MACHINE export SOURCE_DATE_EPOCH=`date +%s` ./tools/buildman/buildman -o /tmp/$MACHINE $ARGS -SBC $MACHINE ./tools/buildman/buildman -o /tmp/$MACHINE $ARGS -SsB $MACHINE [ $KEEP -eq 0 ] && rm -rf /tmp/$MACHINE In a script called "uboot-size-test.sh" to dig in to individual cases more.
Hi Tom, On Tue, 4 May 2021 at 14:40, Tom Rini <trini@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