[PATCH v4 0/1] Makefile: rework u-boot-initial-env target

[PATCH v4 0/1] Makefile: rework u-boot-initial-env target

[Max Krummenacher]

From: Max Krummenacher <max.krummenacher@toradex.com>


With CONFIG_LTO enabled the current way of extracting the
configured environment no longer works, i.e. the object file
content changes due to LTO.

Build a host tool which prints the configured environment instead
of using objcopy and friends to achive the same.

The code and Makefile changes were mostly stolen from tools/env/
i.e. the target userspace tools to access the environment.


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"

Max Krummenacher (1):
  u-boot-initial-env: rework make target

 Makefile                |  9 +++++----
 tools/.gitignore        |  1 +
 tools/Makefile          |  3 +++
 tools/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100644 tools/printinitialenv.c

-- 
2.35.3

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

[Max Krummenacher]

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>

---

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                |  9 +++++----
 tools/.gitignore        |  1 +
 tools/Makefile          |  3 +++
 tools/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100644 tools/printinitialenv.c

diff --git a/Makefile b/Makefile
index 0f1174718f7..c0669840dc7 100644
--- a/Makefile
+++ b/Makefile
@@ -2442,11 +2442,12 @@ 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) tools FORCE
 	$(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 34a1aa7a8b7..a3afdee7813 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -245,6 +245,9 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs
 hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
 HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
 
+# host tool to dump the currently configured default environment
+hostprogs-y				+= printinitialenv
+
 HOSTCFLAGS_mkeficapsule.o += \
 	$(shell pkg-config --cflags gnutls 2> /dev/null || echo "")
 HOSTCFLAGS_mkeficapsule.o += \
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

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

[Pali Rohár]

On Tuesday 08 November 2022 09:52:22 Max Krummenacher 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>

Looks good,

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                |  9 +++++----
>  tools/.gitignore        |  1 +
>  tools/Makefile          |  3 +++
>  tools/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 53 insertions(+), 4 deletions(-)
>  create mode 100644 tools/printinitialenv.c
> 
> diff --git a/Makefile b/Makefile
> index 0f1174718f7..c0669840dc7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2442,11 +2442,12 @@ 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) tools FORCE
>  	$(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 34a1aa7a8b7..a3afdee7813 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -245,6 +245,9 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs
>  hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
>  HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
>  
> +# host tool to dump the currently configured default environment
> +hostprogs-y				+= printinitialenv
> +
>  HOSTCFLAGS_mkeficapsule.o += \
>  	$(shell pkg-config --cflags gnutls 2> /dev/null || echo "")
>  HOSTCFLAGS_mkeficapsule.o += \
> 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
> 

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

[Francesco Dolcini]

On Tue, Nov 08, 2022 at 06:09:11PM +0100, Pali Rohár wrote:
> On Tuesday 08 November 2022 09:52:22 Max Krummenacher 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>
> 
> Looks good,
> 
> Acked-by: Pali Rohár <pali@kernel.org>

Hello Tom,
what's your plan for this patch? Any concern?

Francesco

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Tue, Nov 08, 2022 at 09:52:22AM +0100, Max Krummenacher 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>

Alright, so 'make tools-only_defconfig tools-only' now fails to build
because we're missing the dependencies to make sure that we have
generated/environment.h available.
https://source.denx.de/u-boot/u-boot/-/jobs/532186

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Pali Rohár]

On Wednesday 23 November 2022 13:18:40 Tom Rini wrote:
> On Tue, Nov 08, 2022 at 09:52:22AM +0100, Max Krummenacher 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>
> 
> Alright, so 'make tools-only_defconfig tools-only' now fails to build
> because we're missing the dependencies to make sure that we have
> generated/environment.h available.
> https://source.denx.de/u-boot/u-boot/-/jobs/532186

We cannot generate generated/environment.h in tools-only mode without
real board because environment is always board dependent.

Therefore that new env tool has to be compiled only when doing full real
board build, not just tools-only build.

So I would suggest to extend Makefile put that new tool under some "if"
to exclude compilation in tools-only mode.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Wed, Nov 23, 2022 at 07:22:52PM +0100, Pali Rohár wrote:
> On Wednesday 23 November 2022 13:18:40 Tom Rini wrote:
> > On Tue, Nov 08, 2022 at 09:52:22AM +0100, Max Krummenacher 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>
> > 
> > Alright, so 'make tools-only_defconfig tools-only' now fails to build
> > because we're missing the dependencies to make sure that we have
> > generated/environment.h available.
> > https://source.denx.de/u-boot/u-boot/-/jobs/532186
> 
> We cannot generate generated/environment.h in tools-only mode without
> real board because environment is always board dependent.
> 
> Therefore that new env tool has to be compiled only when doing full real
> board build, not just tools-only build.
> 
> So I would suggest to extend Makefile put that new tool under some "if"
> to exclude compilation in tools-only mode.

Note that "tools-only_defconfig" builds a functional sandbox U-Boot, so
it's probably easier to get the dependencies right in the Makefile
logic, rather than special case them.  Especially since we're now
building this for everyone it's now a race to make sure we did generate
that file? I don't have a strong preference on how this is solved,
however.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]