Re: [PATCH v5 1/1] u-boot-initial-env: rework make target

[Simon Glass <sjg@chromium.org>]

Hi Max,

On Mon, 28 Nov 2022 at 21:41, Max Krummenacher <max.oss.09@gmail.com> wrote:
>
> From: Max Krummenacher <max.krummenacher@toradex.com>
>
> With LTO enabled the U-Boot initial environment is no longer stored
> in an easy accessible section in env/common.o. I.e. the section name
> changes from build to build, its content maybe compressed and it is
> annotated with additional data.
>
> Drop trying to read the initial env with elf tools from the compiler
> specific object file in favour of adding and using a host tool with
> the only functionality of printing the initial env to stdout.
>
> See also:
> https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332c77@foss.st.com/
>
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> Acked-by: Pali Rohár <pali@kernel.org>
>
> ---
>
> Changes in v5:
> - don't build the printinitialenv tool unconditionally but build it
>   only as part of the u-boot-initial-env target.
>   This no longer fails the 'make tools-only_defconfig tools-only'
>   use-case which is reported by Tom Rini.
>   Adding the $(env_h) dependencies to the tools target might give
>   circular dependencies issues with some future tool.
> - add Acked-by: Pali Rohár <pali@kernel.org>
>
> Changes in v4:
> - add '(objtree)/' when calling the tool. Suggested by Pali Rohár.
> - renamed patch, as more than just the Makefile has changes
>
> Changes in v3:
> - moved the tool from scripts/ to tools/. Suggested by Tom Rini
> - changed the dependencies to '$(env_h)' and 'tools'.
>   Suggested by Tom Rini and Pali Rohár.
> - removed the sed rule which replaces \x00 with \x0A as this is already
>   done by the tool. Suggested by Pali Rohár.
>
> Changes in v2:
> - reworked to build a host tool which prints the configured
>   environment as proposed by Pali Rohár
>   https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss.09@gmail.com/
> - renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is enabled"
>
>  Makefile                | 10 ++++++----
>  tools/.gitignore        |  1 +
>  tools/Makefile          |  4 ++++
>  tools/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 55 insertions(+), 4 deletions(-)
>  create mode 100644 tools/printinitialenv.c

Reviewed-by: Simon Glass <sjg@chromium.org>

It would be nice to have a test for this. It could go in test_env.py
and you can use test_event.py as an example.


- Simon


>
> diff --git a/Makefile b/Makefile
> index 2d24ac3959f..32e4bef10f5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2439,11 +2439,13 @@ endif
>         $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
>
>  quiet_cmd_genenv = GENENV  $@
> -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> -       sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> -       sort --field-separator== -k1,1 --stable $@ -o $@
> +cmd_genenv = \
> +       $(objtree)/tools/printinitialenv | \
> +       sed -e '/^\s*$$/d' | \
> +       sort --field-separator== -k1,1 --stable -o $@
>
> -u-boot-initial-env: u-boot.bin
> +u-boot-initial-env: $(env_h) FORCE
> +       $(Q)$(MAKE) $(build)=tools $(objtree)/tools/printinitialenv
>         $(call if_changed,genenv)
>
>  # Consistency checks
> diff --git a/tools/.gitignore b/tools/.gitignore
> index d3a93ff294a..28e8ce2a07a 100644
> --- a/tools/.gitignore
> +++ b/tools/.gitignore
> @@ -28,6 +28,7 @@
>  /mxsboot
>  /ncb
>  /prelink-riscv
> +/printinitialenv
>  /proftool
>  /relocate-rela
>  /spl_size_limit
> diff --git a/tools/Makefile b/tools/Makefile
> index 26be0a7ba2e..80bc62befcb 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -275,6 +275,10 @@ clean-dirs := lib common
>
>  always := $(hostprogs-y)
>
> +# Host tool to dump the currently configured default environment,
> +# build it on demand, i.e. not add it to 'always'.
> +hostprogs-y += printinitialenv
> +
>  # Generated LCD/video logo
>  LOGO_H = $(objtree)/include/bmp_logo.h
>  LOGO_DATA_H = $(objtree)/include/bmp_logo_data.h
> diff --git a/tools/printinitialenv.c b/tools/printinitialenv.c
> new file mode 100644
> index 00000000000..c58b234d679
> --- /dev/null
> +++ b/tools/printinitialenv.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2022
> + * Max Krummenacher, Toradex
> + *
> + * Snippets taken from tools/env/fw_env.c
> + *
> + * This prints the list of default environment variables as currently
> + * configured.
> + *
> + */
> +
> +#include <stdio.h>
> +
> +/* Pull in the current config to define the default environment */
> +#include <linux/kconfig.h>
> +
> +#ifndef __ASSEMBLY__
> +#define __ASSEMBLY__ /* get only #defines from config.h */
> +#include <config.h>
> +#undef __ASSEMBLY__
> +#else
> +#include <config.h>
> +#endif
> +
> +#define DEFAULT_ENV_INSTANCE_STATIC
> +#include <generated/environment.h>
> +#include <env_default.h>
> +
> +int main(void)
> +{
> +       char *env, *nxt;
> +
> +       for (env = default_environment; *env; env = nxt + 1) {
> +               for (nxt = env; *nxt; ++nxt) {
> +                       if (nxt >= &default_environment[sizeof(default_environment)]) {
> +                               fprintf(stderr, "## Error: environment not terminated\n");
> +                               return -1;
> +                       }
> +               }
> +               printf("%s\n", env);
> +       }
> +       return 0;
> +}
> --
> 2.35.3
>