[PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

The build system already automatically looks for and includes an
in-tree *-u-boot.dtsi when building the control .dtb. However, there
are some things that are awkward to maintain in such an in-tree file,
most notably the metadata associated to public keys used for verified
boot.

The only "official" API to get that metadata into the .dtb is via
mkimage, as a side effect of building an actual signed image. But
there are multiple problems with that. First of all, the final U-Boot
(be it U-Boot proper or an SPL) image is built based on a binary
image, the .dtb, and possibly some other binary artifacts. So
modifying the .dtb after the build requires the meta-buildsystem
(Yocto, buildroot, whatnot) to know about and repeat some of the steps
that are already known to and handled by U-Boot's build system,
resulting in needless duplication of code. It's also somewhat annoying
and inconsistent to have a .dtb file in the build folder which is not
generated by the command listed in the corresponding .cmd file (that
of course applies to any generated file).

So the contents of the /signature node really needs to be baked into
the .dtb file when it is first created, which means providing the
relevant data in the form of a .dtsi file. One could in theory put
that data into the *-u-boot.dtsi file, but it's more convenient to be
able to provide it externally: For example, when developing for a
customer, it's common to use a set of dummy keys for development,
while the consultants do not (and should not) have access to the
actual keys used in production. For such a setup, it's easier if the
keys used are chosen via the meta-buildsystem and the path(s) patched
in during the configure step. And of course, nothing prevents anybody
from having DEVICE_TREE_INCLUDES point at files maintained in git, or
for that matter from including the public key metadata in the
*-u-boot.dtsi directly and ignore this feature.

There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
it can be used for providing the contents of the /config/environment
node, so I don't want to tie this exclusively to use for verified
boot.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

Getting the public key metadata into .dtsi form can be done with a
little scripting (roughly 'mkimage -K' of a dummy image followed by
'dtc -I dtb -O dts'). I have a script that does that, along with
options to set 'required' and 'required-mode' properties, include
u-boot,dm-spl properties in case one wants to check U-Boot proper from
SPL with the verified boot mechanism, etc. I'm happy to share that
script if this gets accepted, but it's moot if this is rejected.

I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
to disentangle the kernel and u-boot builds (or u-boot and SPL builds
for that matter!), but as I've since realized, that isn't quite enough
- the above points re modifying the .dtb after it is created but
before that is used to create further build artifacts still
stand. However, such a tool could still be useful for creating the
.dtsi info without the private keys being present, and my key2dtsi.sh
script could easily be modified to use a tool like that should it ever
appear.

[1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
[2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/

 dts/Kconfig          | 9 +++++++++
 scripts/Makefile.lib | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/dts/Kconfig b/dts/Kconfig
index dabe0080c1..593dddbaf0 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
 	  It can be overridden from the command line:
 	  $ make DEVICE_TREE=<device-tree-name>
 
+config DEVICE_TREE_INCLUDES
+       string "Extra .dtsi files to include when building DT control"
+	depends on OF_CONTROL
+	help
+	  U-Boot's control .dtb is usually built from an in-tree .dts
+	  file, plus (if available) an in-tree U-Boot-specific .dtsi
+	  file. This option specifies a space-separated list of extra
+	  .dtsi files that will also be used.
+
 config OF_LIST
 	string "List of device tree files to include for DT control"
 	depends on SPL_LOAD_FIT || MULTI_DTB_FIT
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 78bbebe7e9..a2accba940 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
 # Bring in any U-Boot-specific include at the end of the file
 cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
+	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
+	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
 	$(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
 	$(DTC) -O dtb -o $@ -b 0 \
 		-i $(dir $<) $(DTC_FLAGS) \
-- 
2.31.1

RE: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Roman Kopytin]

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

Hi, all
I prepared 3 patches for fdt_add_pubkey adding.
But in our company infrastructure I can't use git send-email. Our IT can't help me to resolve issue.

-----Original Message-----
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> 
Sent: Tuesday, September 28, 2021 11:57 AM
To: u-boot@lists.denx.de
Cc: Alex Kiernan <alex.kiernan@gmail.com>; Simon Glass <sjg@chromium.org>; Roman Kopytin <Roman.Kopytin@kaspersky.com>; Masahiro Yamada <masahiroy@kernel.org>; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

The build system already automatically looks for and includes an in-tree *-u-boot.dtsi when building the control .dtb. However, there are some things that are awkward to maintain in such an in-tree file, most notably the metadata associated to public keys used for verified boot.

The only "official" API to get that metadata into the .dtb is via mkimage, as a side effect of building an actual signed image. But there are multiple problems with that. First of all, the final U-Boot (be it U-Boot proper or an SPL) image is built based on a binary image, the .dtb, and possibly some other binary artifacts. So modifying the .dtb after the build requires the meta-buildsystem (Yocto, buildroot, whatnot) to know about and repeat some of the steps that are already known to and handled by U-Boot's build system, resulting in needless duplication of code. It's also somewhat annoying and inconsistent to have a .dtb file in the build folder which is not generated by the command listed in the corresponding .cmd file (that of course applies to any generated file).

So the contents of the /signature node really needs to be baked into the .dtb file when it is first created, which means providing the relevant data in the form of a .dtsi file. One could in theory put that data into the *-u-boot.dtsi file, but it's more convenient to be able to provide it externally: For example, when developing for a customer, it's common to use a set of dummy keys for development, while the consultants do not (and should not) have access to the actual keys used in production. For such a setup, it's easier if the keys used are chosen via the meta-buildsystem and the path(s) patched in during the configure step. And of course, nothing prevents anybody from having DEVICE_TREE_INCLUDES point at files maintained in git, or for that matter from including the public key metadata in the *-u-boot.dtsi directly and ignore this feature.

There are other uses for this, e.g. in combination with ENV_IMPORT_FDT it can be used for providing the contents of the /config/environment node, so I don't want to tie this exclusively to use for verified boot.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

Getting the public key metadata into .dtsi form can be done with a little scripting (roughly 'mkimage -K' of a dummy image followed by 'dtc -I dtb -O dts'). I have a script that does that, along with options to set 'required' and 'required-mode' properties, include u-boot,dm-spl properties in case one wants to check U-Boot proper from SPL with the verified boot mechanism, etc. I'm happy to share that script if this gets accepted, but it's moot if this is rejected.

I have previously tried to get an fdt_add_pubkey tool accepted [1,2] to disentangle the kernel and u-boot builds (or u-boot and SPL builds for that matter!), but as I've since realized, that isn't quite enough
- the above points re modifying the .dtb after it is created but before that is used to create further build artifacts still stand. However, such a tool could still be useful for creating the .dtsi info without the private keys being present, and my key2dtsi.sh script could easily be modified to use a tool like that should it ever appear.

[1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
[2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/

 dts/Kconfig          | 9 +++++++++
 scripts/Makefile.lib | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/dts/Kconfig b/dts/Kconfig
index dabe0080c1..593dddbaf0 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
 	  It can be overridden from the command line:
 	  $ make DEVICE_TREE=<device-tree-name>
 
+config DEVICE_TREE_INCLUDES
+       string "Extra .dtsi files to include when building DT control"
+	depends on OF_CONTROL
+	help
+	  U-Boot's control .dtb is usually built from an in-tree .dts
+	  file, plus (if available) an in-tree U-Boot-specific .dtsi
+	  file. This option specifies a space-separated list of extra
+	  .dtsi files that will also be used.
+
 config OF_LIST
 	string "List of device tree files to include for DT control"
 	depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 78bbebe7e9..a2accba940 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
 # Bring in any U-Boot-specific include at the end of the file  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
+	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
+	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
 	$(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
 	$(DTC) -O dtb -o $@ -b 0 \
 		-i $(dir $<) $(DTC_FLAGS) \
--
2.31.1


[-- Attachment #2: 0000-cover-letter.patch --]
[-- Type: application/octet-stream, Size: 597 bytes --]

From c9cec63586fbce1a5f665ff22b9c3b01a43cfbc4 Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Fri, 6 Aug 2021 18:15:35 +0300
Subject: [PATCH 0/2] *** SUBJECT HERE ***

*** BLURB HERE ***

Roman Kopytin (2):
  tools: add fdt_add_pubkey
  test_vboot.py: include test of fdt_add_pubkey tool

 test/py/tests/test_vboot.py |  8 +++
 tools/.gitignore            |  1 +
 tools/Makefile              |  3 ++
 tools/fdt_add_pubkey.c      | 97 +++++++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+)
 create mode 100755 tools/fdt_add_pubkey.c

-- 
2.25.1


[-- Attachment #3: 0001-tools-add-fdt_add_pubkey.patch --]
[-- Type: application/octet-stream, Size: 5376 bytes --]

From 8c7eab9d5e5a1428c84114e04c21a5d0940f0966 Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Fri, 6 Aug 2021 15:21:50 +0300
Subject: [PATCH 1/2] tools: add fdt_add_pubkey

Having to use the -K option to mkimage to populate U-Boot's .dtb with the
public key while signing the kernel FIT image is often a little
awkward. In particular, when using a meta-build system such as
bitbake/Yocto, having the tasks of the kernel and U-Boot recipes
intertwined, modifying deployed artifacts and rebuilding U-Boot with
an updated .dtb is quite cumbersome. Also, in some scenarios one may
wish to build U-Boot complete with the public key(s) embedded in the
.dtb without the corresponding private keys being present on the same
build host.

So this adds a simple tool that allows one to disentangle the kernel
and U-Boot builds, by simply copy-pasting just enough of the mkimage
code to allow one to add a public key to a .dtb. When using mkimage,
some of the information is taken from the .its used to build the
kernel (algorithm and key name), so that of course needs to be
supplied on the command line.

Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/.gitignore       |  1 +
 tools/Makefile         |  3 ++
 tools/fdt_add_pubkey.c | 97 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100755 tools/fdt_add_pubkey.c

diff --git a/tools/.gitignore b/tools/.gitignore
index a88453f64d..f312b760e4 100644
--- a/tools/.gitignore
+++ b/tools/.gitignore
@@ -6,6 +6,7 @@
 /dumpimage
 /easylogo/easylogo
 /envcrc
+/fdt_add_pubkey
 /fdtgrep
 /file2include
 /fit_check_sign
diff --git a/tools/Makefile b/tools/Makefile
index 4a86321f64..44f25dda18 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -73,6 +73,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
 
 hostprogs-y += dumpimage mkimage
 hostprogs-$(CONFIG_TOOLS_LIBCRYPTO) += fit_info fit_check_sign
+hostprogs-$(CONFIG_TOOLS_LIBCRYPTO) += fdt_add_pubkey
 
 hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
 
@@ -153,6 +154,7 @@ dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o
 mkimage-objs   := $(dumpimage-mkimage-objs) mkimage.o
 fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
 fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
+fdt_add_pubkey-objs   := $(dumpimage-mkimage-objs) fdt_add_pubkey.o
 file2include-objs := file2include.o
 
 ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
@@ -190,6 +192,7 @@ HOSTCFLAGS_fit_image.o += -DMKIMAGE_DTC=\"$(CONFIG_MKIMAGE_DTC_PATH)\"
 HOSTLDLIBS_dumpimage := $(HOSTLDLIBS_mkimage)
 HOSTLDLIBS_fit_info := $(HOSTLDLIBS_mkimage)
 HOSTLDLIBS_fit_check_sign := $(HOSTLDLIBS_mkimage)
+HOSTLDLIBS_fdt_add_pubkey := $(HOSTLDLIBS_mkimage)
 
 hostprogs-$(CONFIG_EXYNOS5250) += mkexynosspl
 hostprogs-$(CONFIG_EXYNOS5420) += mkexynosspl
diff --git a/tools/fdt_add_pubkey.c b/tools/fdt_add_pubkey.c
new file mode 100755
index 0000000000..9306ecedd1
--- /dev/null
+++ b/tools/fdt_add_pubkey.c
@@ -0,0 +1,97 @@
+#include <image.h>
+#include "fit_common.h"
+
+static const char *cmdname;
+
+static const char *algo_name = "sha1,rsa2048"; /* -a <algo> */
+static const char *keydir = "."; /* -k <keydir> */
+static const char *keyname = "key"; /* -n <keyname> */
+static const char *require_keys; /* -r <conf|image> */
+static const char *keydest; /* argv[n] */
+
+static void usage(const char *msg)
+{
+	fprintf(stderr, "Error: %s\n", msg);
+	fprintf(stderr, "Usage: %s [-a <algo>] [-k <keydir>] [-n <keyname>] [-r <conf|image>] <fdt blob>\n",
+		cmdname);
+	exit(EXIT_FAILURE);
+}
+
+static void process_args(int argc, char *argv[])
+{
+	int opt;
+
+	while((opt = getopt(argc, argv, "a:k:n:r:")) != -1) {
+		switch (opt) {
+		case 'k':
+			keydir = optarg;
+			break;
+		case 'a':
+			algo_name = optarg;
+			break;
+		case 'n':
+			keyname = optarg;
+			break;
+		case 'r':
+			require_keys = optarg;
+			break;
+		default:
+			usage("Invalid option");
+		}
+	}
+	/* The last parameter is expected to be the .dtb to add the public key to */
+	if (optind < argc)
+		keydest = argv[optind];
+
+	if (!keydest)
+		usage("Missing dtb file to update");
+}
+
+int main(int argc, char *argv[])
+{
+	struct image_sign_info info;
+	int destfd, ret;
+	void *dest_blob = NULL;
+	struct stat dest_sbuf;
+	size_t size_inc = 0;
+
+	cmdname = argv[0];
+
+	process_args(argc, argv);
+
+	memset(&info, 0, sizeof(info));
+
+	info.keydir = keydir;
+	info.keyname = keyname;
+	info.name = algo_name;
+	info.require_keys = require_keys;
+	info.crypto = image_get_crypto_algo(algo_name);
+	if (!info.crypto) {
+                fprintf(stderr, "Unsupported signature algorithm '%s'\n", algo_name);
+		exit(EXIT_FAILURE);
+	}
+
+	while (1) {
+		destfd = mmap_fdt(cmdname, keydest, size_inc, &dest_blob, &dest_sbuf, false, false);
+		if (destfd < 0)
+			exit(EXIT_FAILURE);
+
+		ret = info.crypto->add_verify_data(&info, dest_blob);
+
+		munmap(dest_blob, dest_sbuf.st_size);
+		close(destfd);
+		if (!ret || ret != -ENOSPC)
+			break;
+		fprintf(stderr, ".dtb too small, increasing size by 1024 bytes\n");
+		size_inc = 1024;
+	}
+
+	if (ret) {
+		fprintf(stderr, "%s: Cannot add public key to FIT blob: %s\n",
+			cmdname, strerror(-ret));
+		exit(EXIT_FAILURE);
+	}
+
+	exit(EXIT_SUCCESS);
+}
+
-- 
2.25.1


[-- Attachment #4: 0002-test_vboot.py-include-test-of-fdt_add_pubkey-tool.patch --]
[-- Type: application/octet-stream, Size: 1810 bytes --]

From c9cec63586fbce1a5f665ff22b9c3b01a43cfbc4 Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Fri, 6 Aug 2021 15:23:16 +0300
Subject: [PATCH 2/2] test_vboot.py: include test of fdt_add_pubkey tool

Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/py/tests/test_vboot.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index 6dff6779d1..cf7416b39a 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -230,6 +230,13 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
         cons.log.action('%s: Check signed config on the host' % sha_algo)
 
         util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
+        
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+        # Then add the dev key via the fdt_add_pubkey tool
+        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,rsa2048' % sha_algo,
+                                '-k', tmpdir, '-n', 'dev', '-r', 'conf', dtb])
+        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
 
         if full_test:
             # Make sure that U-Boot checks that the config is in the list of
@@ -370,6 +377,7 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
     fit = '%stest.fit' % tmpdir
     mkimage = cons.config.build_dir + '/tools/mkimage'
     fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
+    fdt_add_pubkey = cons.config.build_dir + '/tools/fdt_add_pubkey'
     dtc_args = '-I dts -O dtb -i %s' % tmpdir
     dtb = '%ssandbox-u-boot.dtb' % tmpdir
     sig_node = '/configurations/conf-1/signature'
-- 
2.25.1

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Roman,

On Tue, 28 Sept 2021 at 03:55, Roman Kopytin
<Roman.Kopytin@kaspersky.com> wrote:
>
> Hi, all
> I prepared 3 patches for fdt_add_pubkey adding.
> But in our company infrastructure I can't use git send-email. Our IT can't help me to resolve issue.

It might be easier to set up a new gmail account and send it with
patman than to see a new job :-)

Regards,
Simon

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

Please note that this is not a resubmission of fdt_add_pubkey, I merely
mentioned that in passing (and cc'ed Roman because of his interest in
that) as a previous attempt at solving the problem of getting the public
key info baked into U-Boot's .dtb, an approach that I've since learnt is
not without problems of its own. So please review this patch on its own
merits.

Rasmus

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Roman,

On Tue, 28 Sept 2021 at 03:55, Roman Kopytin
<Roman.Kopytin@kaspersky.com> wrote:
>
> Hi, all
> I prepared 3 patches for fdt_add_pubkey adding.
> But in our company infrastructure I can't use git send-email. Our IT can't help me to resolve issue.
>

I suppose I should point out that the Internet does normally work, so
it is most likely your IT that is stopping this from working. So you
only need them to 'stop' helping. It is a pretty poor situation when
you cannot use git send-email, a very common use case in software
companies.


- Simon

> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Sent: Tuesday, September 28, 2021 11:57 AM
> To: u-boot@lists.denx.de
> Cc: Alex Kiernan <alex.kiernan@gmail.com>; Simon Glass <sjg@chromium.org>; Roman Kopytin <Roman.Kopytin@kaspersky.com>; Masahiro Yamada <masahiroy@kernel.org>; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Subject: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES
>
> The build system already automatically looks for and includes an in-tree *-u-boot.dtsi when building the control .dtb. However, there are some things that are awkward to maintain in such an in-tree file, most notably the metadata associated to public keys used for verified boot.
>
> The only "official" API to get that metadata into the .dtb is via mkimage, as a side effect of building an actual signed image. But there are multiple problems with that. First of all, the final U-Boot (be it U-Boot proper or an SPL) image is built based on a binary image, the .dtb, and possibly some other binary artifacts. So modifying the .dtb after the build requires the meta-buildsystem (Yocto, buildroot, whatnot) to know about and repeat some of the steps that are already known to and handled by U-Boot's build system, resulting in needless duplication of code. It's also somewhat annoying and inconsistent to have a .dtb file in the build folder which is not generated by the command listed in the corresponding .cmd file (that of course applies to any generated file).
>
> So the contents of the /signature node really needs to be baked into the .dtb file when it is first created, which means providing the relevant data in the form of a .dtsi file. One could in theory put that data into the *-u-boot.dtsi file, but it's more convenient to be able to provide it externally: For example, when developing for a customer, it's common to use a set of dummy keys for development, while the consultants do not (and should not) have access to the actual keys used in production. For such a setup, it's easier if the keys used are chosen via the meta-buildsystem and the path(s) patched in during the configure step. And of course, nothing prevents anybody from having DEVICE_TREE_INCLUDES point at files maintained in git, or for that matter from including the public key metadata in the *-u-boot.dtsi directly and ignore this feature.
>
> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT it can be used for providing the contents of the /config/environment node, so I don't want to tie this exclusively to use for verified boot.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>
> Getting the public key metadata into .dtsi form can be done with a little scripting (roughly 'mkimage -K' of a dummy image followed by 'dtc -I dtb -O dts'). I have a script that does that, along with options to set 'required' and 'required-mode' properties, include u-boot,dm-spl properties in case one wants to check U-Boot proper from SPL with the verified boot mechanism, etc. I'm happy to share that script if this gets accepted, but it's moot if this is rejected.
>
> I have previously tried to get an fdt_add_pubkey tool accepted [1,2] to disentangle the kernel and u-boot builds (or u-boot and SPL builds for that matter!), but as I've since realized, that isn't quite enough
> - the above points re modifying the .dtb after it is created but before that is used to create further build artifacts still stand. However, such a tool could still be useful for creating the .dtsi info without the private keys being present, and my key2dtsi.sh script could easily be modified to use a tool like that should it ever appear.
>
> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
>
>  dts/Kconfig          | 9 +++++++++
>  scripts/Makefile.lib | 2 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/dts/Kconfig b/dts/Kconfig
> index dabe0080c1..593dddbaf0 100644
> --- a/dts/Kconfig
> +++ b/dts/Kconfig
> @@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
>           It can be overridden from the command line:
>           $ make DEVICE_TREE=<device-tree-name>
>
> +config DEVICE_TREE_INCLUDES
> +       string "Extra .dtsi files to include when building DT control"
> +       depends on OF_CONTROL
> +       help
> +         U-Boot's control .dtb is usually built from an in-tree .dts
> +         file, plus (if available) an in-tree U-Boot-specific .dtsi
> +         file. This option specifies a space-separated list of extra
> +         .dtsi files that will also be used.
> +
>  config OF_LIST
>         string "List of device tree files to include for DT control"
>         depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 78bbebe7e9..a2accba940 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
>  # Bring in any U-Boot-specific include at the end of the file  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>         (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
> +       $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> +         echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
>         $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
>         $(DTC) -O dtb -o $@ -b 0 \
>                 -i $(dir $<) $(DTC_FLAGS) \
> --
> 2.31.1
>

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

On 28/09/2021 10.56, Rasmus Villemoes wrote:
> The build system already automatically looks for and includes an
> in-tree *-u-boot.dtsi when building the control .dtb. However, there
> are some things that are awkward to maintain in such an in-tree file,
> most notably the metadata associated to public keys used for verified
> boot.
> 
> The only "official" API to get that metadata into the .dtb is via
> mkimage, as a side effect of building an actual signed image. But
> there are multiple problems with that. First of all, the final U-Boot
> (be it U-Boot proper or an SPL) image is built based on a binary
> image, the .dtb, and possibly some other binary artifacts. So
> modifying the .dtb after the build requires the meta-buildsystem
> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> that are already known to and handled by U-Boot's build system,
> resulting in needless duplication of code.

I should add that it's one thing when dealing with U-Boot proper and
that just needs to be generated by cat'ing u-boot-nodtb.bin and a
modified .dtb. But when the final generation is more complicated than
that, e.g. involving black magic binman (which doesn't care to write out
.cmd files so one can figure out what exactly happened under the hood),
it's really really cumbersome.

It's also somewhat annoying
> and inconsistent to have a .dtb file in the build folder which is not
> generated by the command listed in the corresponding .cmd file (that
> of course applies to any generated file).
> 
> So the contents of the /signature node really needs to be baked into
> the .dtb file when it is first created, which means providing the
> relevant data in the form of a .dtsi file. One could in theory put
> that data into the *-u-boot.dtsi file, but it's more convenient to be
> able to provide it externally: For example, when developing for a
> customer, it's common to use a set of dummy keys for development,
> while the consultants do not (and should not) have access to the
> actual keys used in production. For such a setup, it's easier if the
> keys used are chosen via the meta-buildsystem and the path(s) patched
> in during the configure step. And of course, nothing prevents anybody
> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> for that matter from including the public key metadata in the
> *-u-boot.dtsi directly and ignore this feature.
> 
> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> it can be used for providing the contents of the /config/environment
> node, so I don't want to tie this exclusively to use for verified
> boot.

ping

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Sean Anderson]

Hi Roman,

On 9/28/21 5:55 AM, Roman Kopytin wrote:
> Hi, all
> I prepared 3 patches for fdt_add_pubkey adding.
> But in our company infrastructure I can't use git send-email. Our IT can't help me to resolve issue.

It appears that Linux offers email hosting [1] for this purpose. However,
you need to already be a kernel developer. So you best choice may be
GMail.

--Sean

[1] https://korg.docs.kernel.org/linuxdev.html

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Rasmus,

On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The build system already automatically looks for and includes an
> in-tree *-u-boot.dtsi when building the control .dtb. However, there
> are some things that are awkward to maintain in such an in-tree file,
> most notably the metadata associated to public keys used for verified
> boot.
>
> The only "official" API to get that metadata into the .dtb is via
> mkimage, as a side effect of building an actual signed image. But
> there are multiple problems with that. First of all, the final U-Boot
> (be it U-Boot proper or an SPL) image is built based on a binary
> image, the .dtb, and possibly some other binary artifacts. So
> modifying the .dtb after the build requires the meta-buildsystem
> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> that are already known to and handled by U-Boot's build system,
> resulting in needless duplication of code. It's also somewhat annoying
> and inconsistent to have a .dtb file in the build folder which is not
> generated by the command listed in the corresponding .cmd file (that
> of course applies to any generated file).
>
> So the contents of the /signature node really needs to be baked into
> the .dtb file when it is first created, which means providing the
> relevant data in the form of a .dtsi file. One could in theory put
> that data into the *-u-boot.dtsi file, but it's more convenient to be
> able to provide it externally: For example, when developing for a
> customer, it's common to use a set of dummy keys for development,
> while the consultants do not (and should not) have access to the
> actual keys used in production. For such a setup, it's easier if the
> keys used are chosen via the meta-buildsystem and the path(s) patched
> in during the configure step. And of course, nothing prevents anybody
> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> for that matter from including the public key metadata in the
> *-u-boot.dtsi directly and ignore this feature.
>
> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> it can be used for providing the contents of the /config/environment
> node, so I don't want to tie this exclusively to use for verified
> boot.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>
> Getting the public key metadata into .dtsi form can be done with a
> little scripting (roughly 'mkimage -K' of a dummy image followed by
> 'dtc -I dtb -O dts'). I have a script that does that, along with
> options to set 'required' and 'required-mode' properties, include
> u-boot,dm-spl properties in case one wants to check U-Boot proper from
> SPL with the verified boot mechanism, etc. I'm happy to share that
> script if this gets accepted, but it's moot if this is rejected.
>
> I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
> to disentangle the kernel and u-boot builds (or u-boot and SPL builds
> for that matter!), but as I've since realized, that isn't quite enough
> - the above points re modifying the .dtb after it is created but
> before that is used to create further build artifacts still
> stand. However, such a tool could still be useful for creating the
> .dtsi info without the private keys being present, and my key2dtsi.sh
> script could easily be modified to use a tool like that should it ever
> appear.
>
> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
>
>  dts/Kconfig          | 9 +++++++++
>  scripts/Makefile.lib | 2 ++
>  2 files changed, 11 insertions(+)

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

I suggest adding this to the documentation somewhere in doc/develop/devicetree/

Getting the key into the U-Boot .dtb is normally done with mkimage, as
you say. I don't really understand why this approach is easier.

Also, is there any interest in using binman? It is designed to do the
'packaging' step right at the end, when all the bits are available and
just need to be put together.

I am trying to encourage people to move away from building from source
always, to a two-step process:

- build all the bits
- package them, update devicetree, etc.

https://u-boot.readthedocs.io/en/latest/develop/package/index.html

BTW if Roman can figure out how to send the patches I think that tool
would be useful too.

Regards,
Simon



>
> diff --git a/dts/Kconfig b/dts/Kconfig
> index dabe0080c1..593dddbaf0 100644
> --- a/dts/Kconfig
> +++ b/dts/Kconfig
> @@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
>           It can be overridden from the command line:
>           $ make DEVICE_TREE=<device-tree-name>
>
> +config DEVICE_TREE_INCLUDES
> +       string "Extra .dtsi files to include when building DT control"
> +       depends on OF_CONTROL
> +       help
> +         U-Boot's control .dtb is usually built from an in-tree .dts
> +         file, plus (if available) an in-tree U-Boot-specific .dtsi
> +         file. This option specifies a space-separated list of extra
> +         .dtsi files that will also be used.
> +
>  config OF_LIST
>         string "List of device tree files to include for DT control"
>         depends on SPL_LOAD_FIT || MULTI_DTB_FIT
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 78bbebe7e9..a2accba940 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
>  # Bring in any U-Boot-specific include at the end of the file
>  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>         (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
> +       $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> +         echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
>         $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
>         $(DTC) -O dtb -o $@ -b 0 \
>                 -i $(dir $<) $(DTC_FLAGS) \
> --
> 2.31.1
>

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

On 26/10/2021 03.28, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> The build system already automatically looks for and includes an
>> in-tree *-u-boot.dtsi when building the control .dtb. However, there
>> are some things that are awkward to maintain in such an in-tree file,
>> most notably the metadata associated to public keys used for verified
>> boot.
>>
>> The only "official" API to get that metadata into the .dtb is via
>> mkimage, as a side effect of building an actual signed image. But
>> there are multiple problems with that. First of all, the final U-Boot
>> (be it U-Boot proper or an SPL) image is built based on a binary
>> image, the .dtb, and possibly some other binary artifacts. So
>> modifying the .dtb after the build requires the meta-buildsystem
>> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
>> that are already known to and handled by U-Boot's build system,
>> resulting in needless duplication of code. It's also somewhat annoying
>> and inconsistent to have a .dtb file in the build folder which is not
>> generated by the command listed in the corresponding .cmd file (that
>> of course applies to any generated file).
>>
>> So the contents of the /signature node really needs to be baked into
>> the .dtb file when it is first created, which means providing the
>> relevant data in the form of a .dtsi file. One could in theory put
>> that data into the *-u-boot.dtsi file, but it's more convenient to be
>> able to provide it externally: For example, when developing for a
>> customer, it's common to use a set of dummy keys for development,
>> while the consultants do not (and should not) have access to the
>> actual keys used in production. For such a setup, it's easier if the
>> keys used are chosen via the meta-buildsystem and the path(s) patched
>> in during the configure step. And of course, nothing prevents anybody
>> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
>> for that matter from including the public key metadata in the
>> *-u-boot.dtsi directly and ignore this feature.
>>
>> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
>> it can be used for providing the contents of the /config/environment
>> node, so I don't want to tie this exclusively to use for verified
>> boot.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>
>> Getting the public key metadata into .dtsi form can be done with a
>> little scripting (roughly 'mkimage -K' of a dummy image followed by
>> 'dtc -I dtb -O dts'). I have a script that does that, along with
>> options to set 'required' and 'required-mode' properties, include
>> u-boot,dm-spl properties in case one wants to check U-Boot proper from
>> SPL with the verified boot mechanism, etc. I'm happy to share that
>> script if this gets accepted, but it's moot if this is rejected.
>>
>> I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
>> to disentangle the kernel and u-boot builds (or u-boot and SPL builds
>> for that matter!), but as I've since realized, that isn't quite enough
>> - the above points re modifying the .dtb after it is created but
>> before that is used to create further build artifacts still
>> stand. However, such a tool could still be useful for creating the
>> .dtsi info without the private keys being present, and my key2dtsi.sh
>> script could easily be modified to use a tool like that should it ever
>> appear.
>>
>> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
>> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
>>
>>  dts/Kconfig          | 9 +++++++++
>>  scripts/Makefile.lib | 2 ++
>>  2 files changed, 11 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I suggest adding this to the documentation somewhere in doc/develop/devicetree/

Will do.

> Getting the key into the U-Boot .dtb is normally done with mkimage, as
> you say. I don't really understand why this approach is easier.

I think I explained that in the commit message, but let me try with a
more concrete example. I'm building, using Yocto as the umbrella build
system, for a SOC that needs an SPL + U-Boot proper.

So I have a U-Boot recipe that handles the configuration and
compilation. That's mostly a matter of "make foo_defconfig" followed by
"make ${UBOOT_TARGETS}" where UBOOT_TARGETS is something set in the
machine config. That results in two artifacts, say SPL and u-boot.itb
(the names don't really matter) that are almost ready to be written to
whatever storage is used for them. Most likely, the SPL binary is built
from a u-boot-spl-nodtb.bin and a spl.dtb and perhaps there's some
SOC-specific header in front of it all that the hardware/firmware needs,
those details are hidden by U-Boot's build system invoking the right
flavour of mkimage with the right parameters. If I really want to know,
I can look in spl/.SPL.cmd to see just how it was made [unless it's
binman creating a flash.bin, because then it's just black magic]. But I
usually don't need to.

Enter verified boot. SPL needs to verify U-Boot, and U-Boot must in turn
verify the kernel. I can easily, as a post-compile step, create a signed
version of u-boot.itb. But the -K option is rather useless here, because
SPL has already been built. So if I were to only add the corresponding
public key to SPL's dtb after/while signing U-Boot proper, I'd have to
manually repeat the step(s) needed to create SPL in the first place. Not
to mention that the .dtb living inside u-boot.itb doesn't have the
public key needed for verifying the kernel, so I'd _actually_ first have
to repeat whatever steps were done to create u-boot.itb, after using
mkimage to sign some dummy image just to use the -K option to modify
u-boot.dtb. It's really cumbersome.

So part of the problem is this "you can only get the public keys in the
form required inside the .dtb as a side-effect of signing an image". I
believe that's a fundamental design mistake, hence my attempt at
creating the fdt_add_pubkey tool. But even with that available, adding
the pubkey info into an already-compiled .dtb isn't really enough,
because the .dtb gets built as part of a normal "make". Hence the need
for a way to ensure the pubkey info gets baked into that .dtb during the
build.

Yes, I then need to prepare proper .dtsi files. But since key generation
is a mostly one-time/manual thing, that easily goes along with the
instructions (or script) that generates those, and for every foo.crt,
I'll just have that directory also contain a foo.dtsi, which I can then
point at during do_configure.

> Also, is there any interest in using binman? 

Not really. I mean, it's fine if U-Boot internally uses that, and I can
just take the final output and use that. But as long as binman doesn't
play nice with Kbuild and e.g. tells the commands that were used to
create a given binary, and pollutes the build dir with stuff that's not
removed by "make clean", I'm not very enthusiastic about adding more
uses myself.

Also, this:

  BINMAN  all
Image 'main-section' is missing external blobs and is non-functional:
blob-ext@3

Some images are invalid
$ echo $?
0

Really? When building manually, perhaps the developer sees that warning
(unless there were other non-BINMAN targets that make decided to build
later, making the above scroll away...), but it's not very useful in
some CI scenario where artifacts get deployed automatically to a test
system after a successful build. Recovering boards with a broken
bootloader easily costs many man-hours, plus the time to figure out
what's wrong.

Rasmus

RE: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Roman Kopytin]

Hi, all
Currently I am waiting some help from our IT infrastructure department.

-----Original Message-----
From: Simon Glass <sjg@chromium.org> 
Sent: Tuesday, October 26, 2021 4:28 AM
To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Alex Kiernan <alex.kiernan@gmail.com>; Roman Kopytin <Roman.Kopytin@kaspersky.com>; Masahiro Yamada <masahiroy@kernel.org>
Subject: Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

Hi Rasmus,

On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
>
> The build system already automatically looks for and includes an 
> in-tree *-u-boot.dtsi when building the control .dtb. However, there 
> are some things that are awkward to maintain in such an in-tree file, 
> most notably the metadata associated to public keys used for verified 
> boot.
>
> The only "official" API to get that metadata into the .dtb is via 
> mkimage, as a side effect of building an actual signed image. But 
> there are multiple problems with that. First of all, the final U-Boot 
> (be it U-Boot proper or an SPL) image is built based on a binary 
> image, the .dtb, and possibly some other binary artifacts. So 
> modifying the .dtb after the build requires the meta-buildsystem 
> (Yocto, buildroot, whatnot) to know about and repeat some of the steps 
> that are already known to and handled by U-Boot's build system, 
> resulting in needless duplication of code. It's also somewhat annoying 
> and inconsistent to have a .dtb file in the build folder which is not 
> generated by the command listed in the corresponding .cmd file (that 
> of course applies to any generated file).
>
> So the contents of the /signature node really needs to be baked into 
> the .dtb file when it is first created, which means providing the 
> relevant data in the form of a .dtsi file. One could in theory put 
> that data into the *-u-boot.dtsi file, but it's more convenient to be 
> able to provide it externally: For example, when developing for a 
> customer, it's common to use a set of dummy keys for development, 
> while the consultants do not (and should not) have access to the 
> actual keys used in production. For such a setup, it's easier if the 
> keys used are chosen via the meta-buildsystem and the path(s) patched 
> in during the configure step. And of course, nothing prevents anybody 
> from having DEVICE_TREE_INCLUDES point at files maintained in git, or 
> for that matter from including the public key metadata in the 
> *-u-boot.dtsi directly and ignore this feature.
>
> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT 
> it can be used for providing the contents of the /config/environment 
> node, so I don't want to tie this exclusively to use for verified 
> boot.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>
> Getting the public key metadata into .dtsi form can be done with a 
> little scripting (roughly 'mkimage -K' of a dummy image followed by 
> 'dtc -I dtb -O dts'). I have a script that does that, along with 
> options to set 'required' and 'required-mode' properties, include 
> u-boot,dm-spl properties in case one wants to check U-Boot proper from 
> SPL with the verified boot mechanism, etc. I'm happy to share that 
> script if this gets accepted, but it's moot if this is rejected.
>
> I have previously tried to get an fdt_add_pubkey tool accepted [1,2] 
> to disentangle the kernel and u-boot builds (or u-boot and SPL builds 
> for that matter!), but as I've since realized, that isn't quite enough
> - the above points re modifying the .dtb after it is created but 
> before that is used to create further build artifacts still stand. 
> However, such a tool could still be useful for creating the .dtsi info 
> without the private keys being present, and my key2dtsi.sh script 
> could easily be modified to use a tool like that should it ever 
> appear.
>
> [1] 
> https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes
> @prevas.dk/ [2] 
> https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kasper
> sky.com/
>
>  dts/Kconfig          | 9 +++++++++
>  scripts/Makefile.lib | 2 ++
>  2 files changed, 11 insertions(+)

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

I suggest adding this to the documentation somewhere in doc/develop/devicetree/

Getting the key into the U-Boot .dtb is normally done with mkimage, as you say. I don't really understand why this approach is easier.

Also, is there any interest in using binman? It is designed to do the 'packaging' step right at the end, when all the bits are available and just need to be put together.

I am trying to encourage people to move away from building from source always, to a two-step process:

- build all the bits
- package them, update devicetree, etc.

https://u-boot.readthedocs.io/en/latest/develop/package/index.html

BTW if Roman can figure out how to send the patches I think that tool would be useful too.

Regards,
Simon



>
> diff --git a/dts/Kconfig b/dts/Kconfig index dabe0080c1..593dddbaf0 
> 100644
> --- a/dts/Kconfig
> +++ b/dts/Kconfig
> @@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
>           It can be overridden from the command line:
>           $ make DEVICE_TREE=<device-tree-name>
>
> +config DEVICE_TREE_INCLUDES
> +       string "Extra .dtsi files to include when building DT control"
> +       depends on OF_CONTROL
> +       help
> +         U-Boot's control .dtb is usually built from an in-tree .dts
> +         file, plus (if available) an in-tree U-Boot-specific .dtsi
> +         file. This option specifies a space-separated list of extra
> +         .dtsi files that will also be used.
> +
>  config OF_LIST
>         string "List of device tree files to include for DT control"
>         depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git 
> a/scripts/Makefile.lib b/scripts/Makefile.lib index 
> 78bbebe7e9..a2accba940 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
>  # Bring in any U-Boot-specific include at the end of the file  
> cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>         (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include 
> "$(u_boot_dtsi)"')) > $(pre-tmp); \
> +       $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> +         echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
>         $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
>         $(DTC) -O dtb -o $@ -b 0 \
>                 -i $(dir $<) $(DTC_FLAGS) \
> --
> 2.31.1
>

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Roman,

Good luck! I must get a copy of that BOFH book.

Regards,
Simon



On Sun, 31 Oct 2021 at 22:30, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
>
> Hi, all
> Currently I am waiting some help from our IT infrastructure department.
>
> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Tuesday, October 26, 2021 4:28 AM
> To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Alex Kiernan <alex.kiernan@gmail.com>; Roman Kopytin <Roman.Kopytin@kaspersky.com>; Masahiro Yamada <masahiroy@kernel.org>
> Subject: Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES
>
> Hi Rasmus,
>
> On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> >
> > The build system already automatically looks for and includes an
> > in-tree *-u-boot.dtsi when building the control .dtb. However, there
> > are some things that are awkward to maintain in such an in-tree file,
> > most notably the metadata associated to public keys used for verified
> > boot.
> >
> > The only "official" API to get that metadata into the .dtb is via
> > mkimage, as a side effect of building an actual signed image. But
> > there are multiple problems with that. First of all, the final U-Boot
> > (be it U-Boot proper or an SPL) image is built based on a binary
> > image, the .dtb, and possibly some other binary artifacts. So
> > modifying the .dtb after the build requires the meta-buildsystem
> > (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> > that are already known to and handled by U-Boot's build system,
> > resulting in needless duplication of code. It's also somewhat annoying
> > and inconsistent to have a .dtb file in the build folder which is not
> > generated by the command listed in the corresponding .cmd file (that
> > of course applies to any generated file).
> >
> > So the contents of the /signature node really needs to be baked into
> > the .dtb file when it is first created, which means providing the
> > relevant data in the form of a .dtsi file. One could in theory put
> > that data into the *-u-boot.dtsi file, but it's more convenient to be
> > able to provide it externally: For example, when developing for a
> > customer, it's common to use a set of dummy keys for development,
> > while the consultants do not (and should not) have access to the
> > actual keys used in production. For such a setup, it's easier if the
> > keys used are chosen via the meta-buildsystem and the path(s) patched
> > in during the configure step. And of course, nothing prevents anybody
> > from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> > for that matter from including the public key metadata in the
> > *-u-boot.dtsi directly and ignore this feature.
> >
> > There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> > it can be used for providing the contents of the /config/environment
> > node, so I don't want to tie this exclusively to use for verified
> > boot.
> >
> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >
> > Getting the public key metadata into .dtsi form can be done with a
> > little scripting (roughly 'mkimage -K' of a dummy image followed by
> > 'dtc -I dtb -O dts'). I have a script that does that, along with
> > options to set 'required' and 'required-mode' properties, include
> > u-boot,dm-spl properties in case one wants to check U-Boot proper from
> > SPL with the verified boot mechanism, etc. I'm happy to share that
> > script if this gets accepted, but it's moot if this is rejected.
> >
> > I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
> > to disentangle the kernel and u-boot builds (or u-boot and SPL builds
> > for that matter!), but as I've since realized, that isn't quite enough
> > - the above points re modifying the .dtb after it is created but
> > before that is used to create further build artifacts still stand.
> > However, such a tool could still be useful for creating the .dtsi info
> > without the private keys being present, and my key2dtsi.sh script
> > could easily be modified to use a tool like that should it ever
> > appear.
> >
> > [1]
> > https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes
> > @prevas.dk/ [2]
> > https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kasper
> > sky.com/
> >
> >  dts/Kconfig          | 9 +++++++++
> >  scripts/Makefile.lib | 2 ++
> >  2 files changed, 11 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I suggest adding this to the documentation somewhere in doc/develop/devicetree/
>
> Getting the key into the U-Boot .dtb is normally done with mkimage, as you say. I don't really understand why this approach is easier.
>
> Also, is there any interest in using binman? It is designed to do the 'packaging' step right at the end, when all the bits are available and just need to be put together.
>
> I am trying to encourage people to move away from building from source always, to a two-step process:
>
> - build all the bits
> - package them, update devicetree, etc.
>
> https://u-boot.readthedocs.io/en/latest/develop/package/index.html
>
> BTW if Roman can figure out how to send the patches I think that tool would be useful too.
>
> Regards,
> Simon
>
>
>
> >
> > diff --git a/dts/Kconfig b/dts/Kconfig index dabe0080c1..593dddbaf0
> > 100644
> > --- a/dts/Kconfig
> > +++ b/dts/Kconfig
> > @@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
> >           It can be overridden from the command line:
> >           $ make DEVICE_TREE=<device-tree-name>
> >
> > +config DEVICE_TREE_INCLUDES
> > +       string "Extra .dtsi files to include when building DT control"
> > +       depends on OF_CONTROL
> > +       help
> > +         U-Boot's control .dtb is usually built from an in-tree .dts
> > +         file, plus (if available) an in-tree U-Boot-specific .dtsi
> > +         file. This option specifies a space-separated list of extra
> > +         .dtsi files that will also be used.
> > +
> >  config OF_LIST
> >         string "List of device tree files to include for DT control"
> >         depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git
> > a/scripts/Makefile.lib b/scripts/Makefile.lib index
> > 78bbebe7e9..a2accba940 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
> >  # Bring in any U-Boot-specific include at the end of the file
> > cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> >         (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include
> > "$(u_boot_dtsi)"')) > $(pre-tmp); \
> > +       $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> > +         echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
> >         $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
> >         $(DTC) -O dtb -o $@ -b 0 \
> >                 -i $(dir $<) $(DTC_FLAGS) \
> > --
> > 2.31.1
> >

RE: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Roman Kopytin]

Hi, I sent patches, please check.
But before correct emails I sent several test emails.

-----Original Message-----
From: Simon Glass <sjg@chromium.org> 
Sent: Friday, November 5, 2021 5:02 AM
To: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>; U-Boot Mailing List <u-boot@lists.denx.de>; Alex Kiernan <alex.kiernan@gmail.com>; Masahiro Yamada <masahiroy@kernel.org>
Subject: Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

Hi Roman,

Good luck! I must get a copy of that BOFH book.

Regards,
Simon



On Sun, 31 Oct 2021 at 22:30, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
>
> Hi, all
> Currently I am waiting some help from our IT infrastructure department.
>
> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Tuesday, October 26, 2021 4:28 AM
> To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Alex Kiernan 
> <alex.kiernan@gmail.com>; Roman Kopytin <Roman.Kopytin@kaspersky.com>; 
> Masahiro Yamada <masahiroy@kernel.org>
> Subject: Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES
>
> Hi Rasmus,
>
> On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> >
> > The build system already automatically looks for and includes an 
> > in-tree *-u-boot.dtsi when building the control .dtb. However, there 
> > are some things that are awkward to maintain in such an in-tree 
> > file, most notably the metadata associated to public keys used for 
> > verified boot.
> >
> > The only "official" API to get that metadata into the .dtb is via 
> > mkimage, as a side effect of building an actual signed image. But 
> > there are multiple problems with that. First of all, the final 
> > U-Boot (be it U-Boot proper or an SPL) image is built based on a 
> > binary image, the .dtb, and possibly some other binary artifacts. So 
> > modifying the .dtb after the build requires the meta-buildsystem 
> > (Yocto, buildroot, whatnot) to know about and repeat some of the 
> > steps that are already known to and handled by U-Boot's build 
> > system, resulting in needless duplication of code. It's also 
> > somewhat annoying and inconsistent to have a .dtb file in the build 
> > folder which is not generated by the command listed in the 
> > corresponding .cmd file (that of course applies to any generated file).
> >
> > So the contents of the /signature node really needs to be baked into 
> > the .dtb file when it is first created, which means providing the 
> > relevant data in the form of a .dtsi file. One could in theory put 
> > that data into the *-u-boot.dtsi file, but it's more convenient to 
> > be able to provide it externally: For example, when developing for a 
> > customer, it's common to use a set of dummy keys for development, 
> > while the consultants do not (and should not) have access to the 
> > actual keys used in production. For such a setup, it's easier if the 
> > keys used are chosen via the meta-buildsystem and the path(s) 
> > patched in during the configure step. And of course, nothing 
> > prevents anybody from having DEVICE_TREE_INCLUDES point at files 
> > maintained in git, or for that matter from including the public key 
> > metadata in the *-u-boot.dtsi directly and ignore this feature.
> >
> > There are other uses for this, e.g. in combination with 
> > ENV_IMPORT_FDT it can be used for providing the contents of the 
> > /config/environment node, so I don't want to tie this exclusively to 
> > use for verified boot.
> >
> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >
> > Getting the public key metadata into .dtsi form can be done with a 
> > little scripting (roughly 'mkimage -K' of a dummy image followed by 
> > 'dtc -I dtb -O dts'). I have a script that does that, along with 
> > options to set 'required' and 'required-mode' properties, include 
> > u-boot,dm-spl properties in case one wants to check U-Boot proper 
> > from SPL with the verified boot mechanism, etc. I'm happy to share 
> > that script if this gets accepted, but it's moot if this is rejected.
> >
> > I have previously tried to get an fdt_add_pubkey tool accepted [1,2] 
> > to disentangle the kernel and u-boot builds (or u-boot and SPL 
> > builds for that matter!), but as I've since realized, that isn't 
> > quite enough
> > - the above points re modifying the .dtb after it is created but 
> > before that is used to create further build artifacts still stand.
> > However, such a tool could still be useful for creating the .dtsi 
> > info without the private keys being present, and my key2dtsi.sh 
> > script could easily be modified to use a tool like that should it 
> > ever appear.
> >
> > [1]
> > https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemo
> > es
> > @prevas.dk/ [2]
> > https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kasp
> > er
> > sky.com/
> >
> >  dts/Kconfig          | 9 +++++++++
> >  scripts/Makefile.lib | 2 ++
> >  2 files changed, 11 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I suggest adding this to the documentation somewhere in 
> doc/develop/devicetree/
>
> Getting the key into the U-Boot .dtb is normally done with mkimage, as you say. I don't really understand why this approach is easier.
>
> Also, is there any interest in using binman? It is designed to do the 'packaging' step right at the end, when all the bits are available and just need to be put together.
>
> I am trying to encourage people to move away from building from source always, to a two-step process:
>
> - build all the bits
> - package them, update devicetree, etc.
>
> https://u-boot.readthedocs.io/en/latest/develop/package/index.html
>
> BTW if Roman can figure out how to send the patches I think that tool would be useful too.
>
> Regards,
> Simon
>
>
>
> >
> > diff --git a/dts/Kconfig b/dts/Kconfig index dabe0080c1..593dddbaf0
> > 100644
> > --- a/dts/Kconfig
> > +++ b/dts/Kconfig
> > @@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
> >           It can be overridden from the command line:
> >           $ make DEVICE_TREE=<device-tree-name>
> >
> > +config DEVICE_TREE_INCLUDES
> > +       string "Extra .dtsi files to include when building DT control"
> > +       depends on OF_CONTROL
> > +       help
> > +         U-Boot's control .dtb is usually built from an in-tree .dts
> > +         file, plus (if available) an in-tree U-Boot-specific .dtsi
> > +         file. This option specifies a space-separated list of extra
> > +         .dtsi files that will also be used.
> > +
> >  config OF_LIST
> >         string "List of device tree files to include for DT control"
> >         depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git 
> > a/scripts/Makefile.lib b/scripts/Makefile.lib index
> > 78bbebe7e9..a2accba940 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
> >  # Bring in any U-Boot-specific include at the end of the file 
> > cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> >         (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include
> > "$(u_boot_dtsi)"')) > $(pre-tmp); \
> > +       $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> > +         echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
> >         $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
> >         $(DTC) -O dtb -o $@ -b 0 \
> >                 -i $(dir $<) $(DTC_FLAGS) \
> > --
> > 2.31.1
> >

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Roman,

On Mon, 8 Nov 2021 at 08:43, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
>
> Hi, I sent patches, please check.
> But before correct emails I sent several test emails.

OK I see them, not copied to me, but I see them in the mailing list, thank you.

Regards,
Simon


>
> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Friday, November 5, 2021 5:02 AM
> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>; U-Boot Mailing List <u-boot@lists.denx.de>; Alex Kiernan <alex.kiernan@gmail.com>; Masahiro Yamada <masahiroy@kernel.org>
> Subject: Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES
>
> Hi Roman,
>
> Good luck! I must get a copy of that BOFH book.
>
> Regards,
> Simon
>
>
>
> On Sun, 31 Oct 2021 at 22:30, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
> >
> > Hi, all
> > Currently I am waiting some help from our IT infrastructure department.
> >
> > -----Original Message-----
> > From: Simon Glass <sjg@chromium.org>
> > Sent: Tuesday, October 26, 2021 4:28 AM
> > To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Alex Kiernan
> > <alex.kiernan@gmail.com>; Roman Kopytin <Roman.Kopytin@kaspersky.com>;
> > Masahiro Yamada <masahiroy@kernel.org>
> > Subject: Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES
> >
> > Hi Rasmus,
> >
> > On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> > >
> > > The build system already automatically looks for and includes an
> > > in-tree *-u-boot.dtsi when building the control .dtb. However, there
> > > are some things that are awkward to maintain in such an in-tree
> > > file, most notably the metadata associated to public keys used for
> > > verified boot.
> > >
> > > The only "official" API to get that metadata into the .dtb is via
> > > mkimage, as a side effect of building an actual signed image. But
> > > there are multiple problems with that. First of all, the final
> > > U-Boot (be it U-Boot proper or an SPL) image is built based on a
> > > binary image, the .dtb, and possibly some other binary artifacts. So
> > > modifying the .dtb after the build requires the meta-buildsystem
> > > (Yocto, buildroot, whatnot) to know about and repeat some of the
> > > steps that are already known to and handled by U-Boot's build
> > > system, resulting in needless duplication of code. It's also
> > > somewhat annoying and inconsistent to have a .dtb file in the build
> > > folder which is not generated by the command listed in the
> > > corresponding .cmd file (that of course applies to any generated file).
> > >
> > > So the contents of the /signature node really needs to be baked into
> > > the .dtb file when it is first created, which means providing the
> > > relevant data in the form of a .dtsi file. One could in theory put
> > > that data into the *-u-boot.dtsi file, but it's more convenient to
> > > be able to provide it externally: For example, when developing for a
> > > customer, it's common to use a set of dummy keys for development,
> > > while the consultants do not (and should not) have access to the
> > > actual keys used in production. For such a setup, it's easier if the
> > > keys used are chosen via the meta-buildsystem and the path(s)
> > > patched in during the configure step. And of course, nothing
> > > prevents anybody from having DEVICE_TREE_INCLUDES point at files
> > > maintained in git, or for that matter from including the public key
> > > metadata in the *-u-boot.dtsi directly and ignore this feature.
> > >
> > > There are other uses for this, e.g. in combination with
> > > ENV_IMPORT_FDT it can be used for providing the contents of the
> > > /config/environment node, so I don't want to tie this exclusively to
> > > use for verified boot.
> > >
> > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > >
> > > Getting the public key metadata into .dtsi form can be done with a
> > > little scripting (roughly 'mkimage -K' of a dummy image followed by
> > > 'dtc -I dtb -O dts'). I have a script that does that, along with
> > > options to set 'required' and 'required-mode' properties, include
> > > u-boot,dm-spl properties in case one wants to check U-Boot proper
> > > from SPL with the verified boot mechanism, etc. I'm happy to share
> > > that script if this gets accepted, but it's moot if this is rejected.
> > >
> > > I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
> > > to disentangle the kernel and u-boot builds (or u-boot and SPL
> > > builds for that matter!), but as I've since realized, that isn't
> > > quite enough
> > > - the above points re modifying the .dtb after it is created but
> > > before that is used to create further build artifacts still stand.
> > > However, such a tool could still be useful for creating the .dtsi
> > > info without the private keys being present, and my key2dtsi.sh
> > > script could easily be modified to use a tool like that should it
> > > ever appear.
> > >
> > > [1]
> > > https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemo
> > > es
> > > @prevas.dk/ [2]
> > > https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kasp
> > > er
> > > sky.com/
> > >
> > >  dts/Kconfig          | 9 +++++++++
> > >  scripts/Makefile.lib | 2 ++
> > >  2 files changed, 11 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I suggest adding this to the documentation somewhere in
> > doc/develop/devicetree/
> >
> > Getting the key into the U-Boot .dtb is normally done with mkimage, as you say. I don't really understand why this approach is easier.
> >
> > Also, is there any interest in using binman? It is designed to do the 'packaging' step right at the end, when all the bits are available and just need to be put together.
> >
> > I am trying to encourage people to move away from building from source always, to a two-step process:
> >
> > - build all the bits
> > - package them, update devicetree, etc.
> >
> > https://u-boot.readthedocs.io/en/latest/develop/package/index.html
> >
> > BTW if Roman can figure out how to send the patches I think that tool would be useful too.
> >
> > Regards,
> > Simon
> >
> >
> >
> > >
> > > diff --git a/dts/Kconfig b/dts/Kconfig index dabe0080c1..593dddbaf0
> > > 100644
> > > --- a/dts/Kconfig
> > > +++ b/dts/Kconfig
> > > @@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
> > >           It can be overridden from the command line:
> > >           $ make DEVICE_TREE=<device-tree-name>
> > >
> > > +config DEVICE_TREE_INCLUDES
> > > +       string "Extra .dtsi files to include when building DT control"
> > > +       depends on OF_CONTROL
> > > +       help
> > > +         U-Boot's control .dtb is usually built from an in-tree .dts
> > > +         file, plus (if available) an in-tree U-Boot-specific .dtsi
> > > +         file. This option specifies a space-separated list of extra
> > > +         .dtsi files that will also be used.
> > > +
> > >  config OF_LIST
> > >         string "List of device tree files to include for DT control"
> > >         depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git
> > > a/scripts/Makefile.lib b/scripts/Makefile.lib index
> > > 78bbebe7e9..a2accba940 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
> > >  # Bring in any U-Boot-specific include at the end of the file
> > > cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> > >         (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include
> > > "$(u_boot_dtsi)"')) > $(pre-tmp); \
> > > +       $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> > > +         echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
> > >         $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
> > >         $(DTC) -O dtb -o $@ -b 0 \
> > >                 -i $(dir $<) $(DTC_FLAGS) \
> > > --
> > > 2.31.1
> > >

[PATCH v2] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

The build system already automatically looks for and includes an
in-tree *-u-boot.dtsi when building the control .dtb. However, there
are some things that are awkward to maintain in such an in-tree file,
most notably the metadata associated to public keys used for verified
boot.

The only "official" API to get that metadata into the .dtb is via
mkimage, as a side effect of building an actual signed image. But
there are multiple problems with that. First of all, the final U-Boot
(be it U-Boot proper or an SPL) image is built based on a binary
image, the .dtb, and possibly some other binary artifacts. So
modifying the .dtb after the build requires the meta-buildsystem
(Yocto, buildroot, whatnot) to know about and repeat some of the steps
that are already known to and handled by U-Boot's build system,
resulting in needless duplication of code. It's also somewhat annoying
and inconsistent to have a .dtb file in the build folder which is not
generated by the command listed in the corresponding .cmd file (that
of course applies to any generated file).

So the contents of the /signature node really needs to be baked into
the .dtb file when it is first created, which means providing the
relevant data in the form of a .dtsi file. One could in theory put
that data into the *-u-boot.dtsi file, but it's more convenient to be
able to provide it externally: For example, when developing for a
customer, it's common to use a set of dummy keys for development,
while the consultants do not (and should not) have access to the
actual keys used in production. For such a setup, it's easier if the
keys used are chosen via the meta-buildsystem and the path(s) patched
in during the configure step. And of course, nothing prevents anybody
from having DEVICE_TREE_INCLUDES point at files maintained in git, or
for that matter from including the public key metadata in the
*-u-boot.dtsi directly and ignore this feature.

There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
it can be used for providing the contents of the /config/environment
node, so I don't want to tie this exclusively to use for verified
boot.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
v2: rebase to current master, add paragraph to
doc/develop/devicetree/control.rst as suggested by Simon. I've taken
the liberty of keeping his R-b tag as this mostly just repeats what is
in the Kconfig help text and commit message.

 doc/develop/devicetree/control.rst | 18 ++++++++++++++++++
 dts/Kconfig                        |  9 +++++++++
 scripts/Makefile.lib               |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst
index 0e6f85d5af..ff008ba943 100644
--- a/doc/develop/devicetree/control.rst
+++ b/doc/develop/devicetree/control.rst
@@ -182,6 +182,24 @@ main file, in this order::
 Only one of these is selected but of course you can #include another one within
 that file, to create a hierarchy of shared files.
 
+
+External .dtsi fragments
+------------------------
+
+Apart from describing the hardware present, U-Boot also uses its
+control dtb for various configuration purposes. For example, the
+public key(s) used for Verified Boot are embedded in a specific format
+in a /signature node.
+
+As mentioned above, the U-Boot build system automatically includes a
+*-u-boot.dtsi file, if found, containing U-Boot specific
+quirks. However, some data, such as the mentioned public keys, are not
+appropriate for upstream U-Boot but are better kept and maintained
+outside the U-Boot repository. You can use CONFIG_DEVICE_TREE_INCLUDES
+to specify a list of .dtsi files that will also be included when
+building .dtb files.
+
+
 Relocation, SPL and TPL
 -----------------------
 
diff --git a/dts/Kconfig b/dts/Kconfig
index b7c4a2fec0..1f8debf1a8 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -131,6 +131,15 @@ config DEFAULT_DEVICE_TREE
 	  It can be overridden from the command line:
 	  $ make DEVICE_TREE=<device-tree-name>
 
+config DEVICE_TREE_INCLUDES
+       string "Extra .dtsi files to include when building DT control"
+	depends on OF_CONTROL
+	help
+	  U-Boot's control .dtb is usually built from an in-tree .dts
+	  file, plus (if available) an in-tree U-Boot-specific .dtsi
+	  file. This option specifies a space-separated list of extra
+	  .dtsi files that will also be used.
+
 config OF_LIST
 	string "List of device tree files to include for DT control"
 	depends on SPL_LOAD_FIT || MULTI_DTB_FIT
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 39f03398ed..4ab422c231 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -318,8 +318,11 @@ endif
 quiet_cmd_dtc = DTC     $@
 # Modified for U-Boot
 # Bring in any U-Boot-specific include at the end of the file
+# And finally any custom .dtsi fragments specified with CONFIG_DEVICE_TREE_INCLUDES
 cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
+	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
+	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
 	$(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
 	$(DTC) -O dtb -o $@ -b 0 \
 		-i $(dir $<) $(DTC_FLAGS) \
-- 
2.31.1

Re: [PATCH v2] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

Ping

On 21/11/2021 14.52, Rasmus Villemoes wrote:
> The build system already automatically looks for and includes an
> in-tree *-u-boot.dtsi when building the control .dtb. However, there
> are some things that are awkward to maintain in such an in-tree file,
> most notably the metadata associated to public keys used for verified
> boot.
> 
> The only "official" API to get that metadata into the .dtb is via
> mkimage, as a side effect of building an actual signed image. But
> there are multiple problems with that. First of all, the final U-Boot
> (be it U-Boot proper or an SPL) image is built based on a binary
> image, the .dtb, and possibly some other binary artifacts. So
> modifying the .dtb after the build requires the meta-buildsystem
> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> that are already known to and handled by U-Boot's build system,
> resulting in needless duplication of code. It's also somewhat annoying
> and inconsistent to have a .dtb file in the build folder which is not
> generated by the command listed in the corresponding .cmd file (that
> of course applies to any generated file).
> 
> So the contents of the /signature node really needs to be baked into
> the .dtb file when it is first created, which means providing the
> relevant data in the form of a .dtsi file. One could in theory put
> that data into the *-u-boot.dtsi file, but it's more convenient to be
> able to provide it externally: For example, when developing for a
> customer, it's common to use a set of dummy keys for development,
> while the consultants do not (and should not) have access to the
> actual keys used in production. For such a setup, it's easier if the
> keys used are chosen via the meta-buildsystem and the path(s) patched
> in during the configure step. And of course, nothing prevents anybody
> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> for that matter from including the public key metadata in the
> *-u-boot.dtsi directly and ignore this feature.
> 
> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> it can be used for providing the contents of the /config/environment
> node, so I don't want to tie this exclusively to use for verified
> boot.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> v2: rebase to current master, add paragraph to
> doc/develop/devicetree/control.rst as suggested by Simon. I've taken
> the liberty of keeping his R-b tag as this mostly just repeats what is
> in the Kconfig help text and commit message.
> 
>  doc/develop/devicetree/control.rst | 18 ++++++++++++++++++
>  dts/Kconfig                        |  9 +++++++++
>  scripts/Makefile.lib               |  3 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst
> index 0e6f85d5af..ff008ba943 100644
> --- a/doc/develop/devicetree/control.rst
> +++ b/doc/develop/devicetree/control.rst
> @@ -182,6 +182,24 @@ main file, in this order::
>  Only one of these is selected but of course you can #include another one within
>  that file, to create a hierarchy of shared files.
>  
> +
> +External .dtsi fragments
> +------------------------
> +
> +Apart from describing the hardware present, U-Boot also uses its
> +control dtb for various configuration purposes. For example, the
> +public key(s) used for Verified Boot are embedded in a specific format
> +in a /signature node.
> +
> +As mentioned above, the U-Boot build system automatically includes a
> +*-u-boot.dtsi file, if found, containing U-Boot specific
> +quirks. However, some data, such as the mentioned public keys, are not
> +appropriate for upstream U-Boot but are better kept and maintained
> +outside the U-Boot repository. You can use CONFIG_DEVICE_TREE_INCLUDES
> +to specify a list of .dtsi files that will also be included when
> +building .dtb files.
> +
> +
>  Relocation, SPL and TPL
>  -----------------------
>  
> diff --git a/dts/Kconfig b/dts/Kconfig
> index b7c4a2fec0..1f8debf1a8 100644
> --- a/dts/Kconfig
> +++ b/dts/Kconfig
> @@ -131,6 +131,15 @@ config DEFAULT_DEVICE_TREE
>  	  It can be overridden from the command line:
>  	  $ make DEVICE_TREE=<device-tree-name>
>  
> +config DEVICE_TREE_INCLUDES
> +       string "Extra .dtsi files to include when building DT control"
> +	depends on OF_CONTROL
> +	help
> +	  U-Boot's control .dtb is usually built from an in-tree .dts
> +	  file, plus (if available) an in-tree U-Boot-specific .dtsi
> +	  file. This option specifies a space-separated list of extra
> +	  .dtsi files that will also be used.
> +
>  config OF_LIST
>  	string "List of device tree files to include for DT control"
>  	depends on SPL_LOAD_FIT || MULTI_DTB_FIT
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 39f03398ed..4ab422c231 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -318,8 +318,11 @@ endif
>  quiet_cmd_dtc = DTC     $@
>  # Modified for U-Boot
>  # Bring in any U-Boot-specific include at the end of the file
> +# And finally any custom .dtsi fragments specified with CONFIG_DEVICE_TREE_INCLUDES
>  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>  	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
> +	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> +	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
>  	$(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
>  	$(DTC) -O dtb -o $@ -b 0 \
>  		-i $(dir $<) $(DTC_FLAGS) \
> 


-- 
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villemoes@prevas.dk
www.prevas.dk

Re: [PATCH v2] introduce CONFIG_DEVICE_TREE_INCLUDES

[Tom Rini]

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

On Fri, Jan 14, 2022 at 09:30:29AM +0100, Rasmus Villemoes wrote:
> Ping
> 
> On 21/11/2021 14.52, Rasmus Villemoes wrote:
> > The build system already automatically looks for and includes an
> > in-tree *-u-boot.dtsi when building the control .dtb. However, there
> > are some things that are awkward to maintain in such an in-tree file,
> > most notably the metadata associated to public keys used for verified
> > boot.
> > 
> > The only "official" API to get that metadata into the .dtb is via
> > mkimage, as a side effect of building an actual signed image. But
> > there are multiple problems with that. First of all, the final U-Boot
> > (be it U-Boot proper or an SPL) image is built based on a binary
> > image, the .dtb, and possibly some other binary artifacts. So
> > modifying the .dtb after the build requires the meta-buildsystem
> > (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> > that are already known to and handled by U-Boot's build system,
> > resulting in needless duplication of code. It's also somewhat annoying
> > and inconsistent to have a .dtb file in the build folder which is not
> > generated by the command listed in the corresponding .cmd file (that
> > of course applies to any generated file).
> > 
> > So the contents of the /signature node really needs to be baked into
> > the .dtb file when it is first created, which means providing the
> > relevant data in the form of a .dtsi file. One could in theory put
> > that data into the *-u-boot.dtsi file, but it's more convenient to be
> > able to provide it externally: For example, when developing for a
> > customer, it's common to use a set of dummy keys for development,
> > while the consultants do not (and should not) have access to the
> > actual keys used in production. For such a setup, it's easier if the
> > keys used are chosen via the meta-buildsystem and the path(s) patched
> > in during the configure step. And of course, nothing prevents anybody
> > from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> > for that matter from including the public key metadata in the
> > *-u-boot.dtsi directly and ignore this feature.
> > 
> > There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> > it can be used for providing the contents of the /config/environment
> > node, so I don't want to tie this exclusively to use for verified
> > boot.
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> > v2: rebase to current master, add paragraph to
> > doc/develop/devicetree/control.rst as suggested by Simon. I've taken
> > the liberty of keeping his R-b tag as this mostly just repeats what is
> > in the Kconfig help text and commit message.
> > 
> >  doc/develop/devicetree/control.rst | 18 ++++++++++++++++++
> >  dts/Kconfig                        |  9 +++++++++
> >  scripts/Makefile.lib               |  3 +++
> >  3 files changed, 30 insertions(+)
> > 
> > diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst
> > index 0e6f85d5af..ff008ba943 100644
> > --- a/doc/develop/devicetree/control.rst
> > +++ b/doc/develop/devicetree/control.rst
> > @@ -182,6 +182,24 @@ main file, in this order::
> >  Only one of these is selected but of course you can #include another one within
> >  that file, to create a hierarchy of shared files.
> >  
> > +
> > +External .dtsi fragments
> > +------------------------
> > +
> > +Apart from describing the hardware present, U-Boot also uses its
> > +control dtb for various configuration purposes. For example, the
> > +public key(s) used for Verified Boot are embedded in a specific format
> > +in a /signature node.
> > +
> > +As mentioned above, the U-Boot build system automatically includes a
> > +*-u-boot.dtsi file, if found, containing U-Boot specific
> > +quirks. However, some data, such as the mentioned public keys, are not
> > +appropriate for upstream U-Boot but are better kept and maintained
> > +outside the U-Boot repository. You can use CONFIG_DEVICE_TREE_INCLUDES
> > +to specify a list of .dtsi files that will also be included when
> > +building .dtb files.
> > +
> > +
> >  Relocation, SPL and TPL
> >  -----------------------
> >  
> > diff --git a/dts/Kconfig b/dts/Kconfig
> > index b7c4a2fec0..1f8debf1a8 100644
> > --- a/dts/Kconfig
> > +++ b/dts/Kconfig
> > @@ -131,6 +131,15 @@ config DEFAULT_DEVICE_TREE
> >  	  It can be overridden from the command line:
> >  	  $ make DEVICE_TREE=<device-tree-name>
> >  
> > +config DEVICE_TREE_INCLUDES
> > +       string "Extra .dtsi files to include when building DT control"
> > +	depends on OF_CONTROL
> > +	help
> > +	  U-Boot's control .dtb is usually built from an in-tree .dts
> > +	  file, plus (if available) an in-tree U-Boot-specific .dtsi
> > +	  file. This option specifies a space-separated list of extra
> > +	  .dtsi files that will also be used.
> > +
> >  config OF_LIST
> >  	string "List of device tree files to include for DT control"
> >  	depends on SPL_LOAD_FIT || MULTI_DTB_FIT
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 39f03398ed..4ab422c231 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -318,8 +318,11 @@ endif
> >  quiet_cmd_dtc = DTC     $@
> >  # Modified for U-Boot
> >  # Bring in any U-Boot-specific include at the end of the file
> > +# And finally any custom .dtsi fragments specified with CONFIG_DEVICE_TREE_INCLUDES
> >  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> >  	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
> > +	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> > +	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
> >  	$(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
> >  	$(DTC) -O dtb -o $@ -b 0 \
> >  		-i $(dir $<) $(DTC_FLAGS) \

Simon?

-- 
Tom

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

Re: [PATCH v2] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Tom,

On Fri, 14 Jan 2022 at 08:41, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jan 14, 2022 at 09:30:29AM +0100, Rasmus Villemoes wrote:
> > Ping
> >
> > On 21/11/2021 14.52, Rasmus Villemoes wrote:
> > > The build system already automatically looks for and includes an
> > > in-tree *-u-boot.dtsi when building the control .dtb. However, there
> > > are some things that are awkward to maintain in such an in-tree file,
> > > most notably the metadata associated to public keys used for verified
> > > boot.
> > >
> > > The only "official" API to get that metadata into the .dtb is via
> > > mkimage, as a side effect of building an actual signed image. But
> > > there are multiple problems with that. First of all, the final U-Boot
> > > (be it U-Boot proper or an SPL) image is built based on a binary
> > > image, the .dtb, and possibly some other binary artifacts. So
> > > modifying the .dtb after the build requires the meta-buildsystem
> > > (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> > > that are already known to and handled by U-Boot's build system,
> > > resulting in needless duplication of code. It's also somewhat annoying
> > > and inconsistent to have a .dtb file in the build folder which is not
> > > generated by the command listed in the corresponding .cmd file (that
> > > of course applies to any generated file).
> > >
> > > So the contents of the /signature node really needs to be baked into
> > > the .dtb file when it is first created, which means providing the
> > > relevant data in the form of a .dtsi file. One could in theory put
> > > that data into the *-u-boot.dtsi file, but it's more convenient to be
> > > able to provide it externally: For example, when developing for a
> > > customer, it's common to use a set of dummy keys for development,
> > > while the consultants do not (and should not) have access to the
> > > actual keys used in production. For such a setup, it's easier if the
> > > keys used are chosen via the meta-buildsystem and the path(s) patched
> > > in during the configure step. And of course, nothing prevents anybody
> > > from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> > > for that matter from including the public key metadata in the
> > > *-u-boot.dtsi directly and ignore this feature.
> > >
> > > There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> > > it can be used for providing the contents of the /config/environment
> > > node, so I don't want to tie this exclusively to use for verified
> > > boot.
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > > v2: rebase to current master, add paragraph to
> > > doc/develop/devicetree/control.rst as suggested by Simon. I've taken
> > > the liberty of keeping his R-b tag as this mostly just repeats what is
> > > in the Kconfig help text and commit message.
> > >
> > >  doc/develop/devicetree/control.rst | 18 ++++++++++++++++++
> > >  dts/Kconfig                        |  9 +++++++++
> > >  scripts/Makefile.lib               |  3 +++
> > >  3 files changed, 30 insertions(+)
> > >
> > > diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst
> > > index 0e6f85d5af..ff008ba943 100644
> > > --- a/doc/develop/devicetree/control.rst
> > > +++ b/doc/develop/devicetree/control.rst
> > > @@ -182,6 +182,24 @@ main file, in this order::
> > >  Only one of these is selected but of course you can #include another one within
> > >  that file, to create a hierarchy of shared files.
> > >
> > > +
> > > +External .dtsi fragments
> > > +------------------------
> > > +
> > > +Apart from describing the hardware present, U-Boot also uses its
> > > +control dtb for various configuration purposes. For example, the
> > > +public key(s) used for Verified Boot are embedded in a specific format
> > > +in a /signature node.
> > > +
> > > +As mentioned above, the U-Boot build system automatically includes a
> > > +*-u-boot.dtsi file, if found, containing U-Boot specific
> > > +quirks. However, some data, such as the mentioned public keys, are not
> > > +appropriate for upstream U-Boot but are better kept and maintained
> > > +outside the U-Boot repository. You can use CONFIG_DEVICE_TREE_INCLUDES
> > > +to specify a list of .dtsi files that will also be included when
> > > +building .dtb files.
> > > +
> > > +
> > >  Relocation, SPL and TPL
> > >  -----------------------
> > >
> > > diff --git a/dts/Kconfig b/dts/Kconfig
> > > index b7c4a2fec0..1f8debf1a8 100644
> > > --- a/dts/Kconfig
> > > +++ b/dts/Kconfig
> > > @@ -131,6 +131,15 @@ config DEFAULT_DEVICE_TREE
> > >       It can be overridden from the command line:
> > >       $ make DEVICE_TREE=<device-tree-name>
> > >
> > > +config DEVICE_TREE_INCLUDES
> > > +       string "Extra .dtsi files to include when building DT control"
> > > +   depends on OF_CONTROL
> > > +   help
> > > +     U-Boot's control .dtb is usually built from an in-tree .dts
> > > +     file, plus (if available) an in-tree U-Boot-specific .dtsi
> > > +     file. This option specifies a space-separated list of extra
> > > +     .dtsi files that will also be used.
> > > +
> > >  config OF_LIST
> > >     string "List of device tree files to include for DT control"
> > >     depends on SPL_LOAD_FIT || MULTI_DTB_FIT
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index 39f03398ed..4ab422c231 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -318,8 +318,11 @@ endif
> > >  quiet_cmd_dtc = DTC     $@
> > >  # Modified for U-Boot
> > >  # Bring in any U-Boot-specific include at the end of the file
> > > +# And finally any custom .dtsi fragments specified with CONFIG_DEVICE_TREE_INCLUDES
> > >  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> > >     (cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
> > > +   $(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
> > > +     echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
> > >     $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
> > >     $(DTC) -O dtb -o $@ -b 0 \
> > >             -i $(dir $<) $(DTC_FLAGS) \
>
> Simon?

I'm happy to apply it if you like, but it is not in my patchwork queue
at present. I agree that keeping the review tag from the last version
makes sense. I'll reply the other points separately, as I haven't got
around to it.

Regards,
Simon

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Rasmus,

On Tue, 26 Oct 2021 at 02:08, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 26/10/2021 03.28, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> The build system already automatically looks for and includes an
> >> in-tree *-u-boot.dtsi when building the control .dtb. However, there
> >> are some things that are awkward to maintain in such an in-tree file,
> >> most notably the metadata associated to public keys used for verified
> >> boot.
> >>
> >> The only "official" API to get that metadata into the .dtb is via
> >> mkimage, as a side effect of building an actual signed image. But
> >> there are multiple problems with that. First of all, the final U-Boot
> >> (be it U-Boot proper or an SPL) image is built based on a binary
> >> image, the .dtb, and possibly some other binary artifacts. So
> >> modifying the .dtb after the build requires the meta-buildsystem
> >> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> >> that are already known to and handled by U-Boot's build system,
> >> resulting in needless duplication of code. It's also somewhat annoying
> >> and inconsistent to have a .dtb file in the build folder which is not
> >> generated by the command listed in the corresponding .cmd file (that
> >> of course applies to any generated file).
> >>
> >> So the contents of the /signature node really needs to be baked into
> >> the .dtb file when it is first created, which means providing the
> >> relevant data in the form of a .dtsi file. One could in theory put
> >> that data into the *-u-boot.dtsi file, but it's more convenient to be
> >> able to provide it externally: For example, when developing for a
> >> customer, it's common to use a set of dummy keys for development,
> >> while the consultants do not (and should not) have access to the
> >> actual keys used in production. For such a setup, it's easier if the
> >> keys used are chosen via the meta-buildsystem and the path(s) patched
> >> in during the configure step. And of course, nothing prevents anybody
> >> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> >> for that matter from including the public key metadata in the
> >> *-u-boot.dtsi directly and ignore this feature.
> >>
> >> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> >> it can be used for providing the contents of the /config/environment
> >> node, so I don't want to tie this exclusively to use for verified
> >> boot.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> ---
> >>
> >> Getting the public key metadata into .dtsi form can be done with a
> >> little scripting (roughly 'mkimage -K' of a dummy image followed by
> >> 'dtc -I dtb -O dts'). I have a script that does that, along with
> >> options to set 'required' and 'required-mode' properties, include
> >> u-boot,dm-spl properties in case one wants to check U-Boot proper from
> >> SPL with the verified boot mechanism, etc. I'm happy to share that
> >> script if this gets accepted, but it's moot if this is rejected.
> >>
> >> I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
> >> to disentangle the kernel and u-boot builds (or u-boot and SPL builds
> >> for that matter!), but as I've since realized, that isn't quite enough
> >> - the above points re modifying the .dtb after it is created but
> >> before that is used to create further build artifacts still
> >> stand. However, such a tool could still be useful for creating the
> >> .dtsi info without the private keys being present, and my key2dtsi.sh
> >> script could easily be modified to use a tool like that should it ever
> >> appear.
> >>
> >> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
> >> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
> >>
> >>  dts/Kconfig          | 9 +++++++++
> >>  scripts/Makefile.lib | 2 ++
> >>  2 files changed, 11 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I suggest adding this to the documentation somewhere in doc/develop/devicetree/
>
> Will do.
>
> > Getting the key into the U-Boot .dtb is normally done with mkimage, as
> > you say. I don't really understand why this approach is easier.
>
> I think I explained that in the commit message, but let me try with a
> more concrete example. I'm building, using Yocto as the umbrella build
> system, for a SOC that needs an SPL + U-Boot proper.
>
> So I have a U-Boot recipe that handles the configuration and
> compilation. That's mostly a matter of "make foo_defconfig" followed by
> "make ${UBOOT_TARGETS}" where UBOOT_TARGETS is something set in the
> machine config. That results in two artifacts, say SPL and u-boot.itb
> (the names don't really matter) that are almost ready to be written to
> whatever storage is used for them. Most likely, the SPL binary is built
> from a u-boot-spl-nodtb.bin and a spl.dtb and perhaps there's some
> SOC-specific header in front of it all that the hardware/firmware needs,
> those details are hidden by U-Boot's build system invoking the right
> flavour of mkimage with the right parameters. If I really want to know,
> I can look in spl/.SPL.cmd to see just how it was made [unless it's
> binman creating a flash.bin, because then it's just black magic]. But I
> usually don't need to.
>
> Enter verified boot. SPL needs to verify U-Boot, and U-Boot must in turn
> verify the kernel. I can easily, as a post-compile step, create a signed
> version of u-boot.itb. But the -K option is rather useless here, because
> SPL has already been built. So if I were to only add the corresponding
> public key to SPL's dtb after/while signing U-Boot proper, I'd have to
> manually repeat the step(s) needed to create SPL in the first place. Not
> to mention that the .dtb living inside u-boot.itb doesn't have the
> public key needed for verifying the kernel, so I'd _actually_ first have
> to repeat whatever steps were done to create u-boot.itb, after using
> mkimage to sign some dummy image just to use the -K option to modify
> u-boot.dtb. It's really cumbersome.
>
> So part of the problem is this "you can only get the public keys in the
> form required inside the .dtb as a side-effect of signing an image". I
> believe that's a fundamental design mistake, hence my attempt at
> creating the fdt_add_pubkey tool. But even with that available, adding
> the pubkey info into an already-compiled .dtb isn't really enough,
> because the .dtb gets built as part of a normal "make". Hence the need
> for a way to ensure the pubkey info gets baked into that .dtb during the
> build.
>
> Yes, I then need to prepare proper .dtsi files. But since key generation
> is a mostly one-time/manual thing, that easily goes along with the
> instructions (or script) that generates those, and for every foo.crt,
> I'll just have that directory also contain a foo.dtsi, which I can then
> point at during do_configure.
>
> > Also, is there any interest in using binman?
>
> Not really. I mean, it's fine if U-Boot internally uses that, and I can
> just take the final output and use that. But as long as binman doesn't
> play nice with Kbuild and e.g. tells the commands that were used to
> create a given binary, and pollutes the build dir with stuff that's not
> removed by "make clean", I'm not very enthusiastic about adding more
> uses myself.
>
> Also, this:
>
>   BINMAN  all
> Image 'main-section' is missing external blobs and is non-functional:
> blob-ext@3
>
> Some images are invalid
> $ echo $?
> 0
>
> Really? When building manually, perhaps the developer sees that warning
> (unless there were other non-BINMAN targets that make decided to build
> later, making the above scroll away...), but it's not very useful in
> some CI scenario where artifacts get deployed automatically to a test
> system after a successful build. Recovering boards with a broken
> bootloader easily costs many man-hours, plus the time to figure out
> what's wrong.

I still think binman is the best solution here. The points you are
raising are annoyances that can be resolved.

We could return a non-zero value from binman when external blobs are
missing; we'd just need to use a special value (we use 101 for
buildman) and update the CI to detect and ignore that.

Normally people use a separate output directory from the source
directory, which is why the 'pollution' is not normally a big problem.
But it is possible to resolve that problem and it has been discussed
on the mailing list. It's just that no one has done it yet.

In what way does binman play badly with Kbuild?

I'd really suggest changing the mindset to separating build from
packaging, perhaps by having a separate yocto package/recipe that puts
the firmware together, adds the signature, etc. Images are only
getting more complicated. The idea that you'll be able to build
everyone in the U-Boot tree and it won't need any post-processing is
not going to survive very long IMO, so you may as well invest in it
now :-)

Regards,
Simon

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

On 14/01/2022 17.51, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 26 Oct 2021 at 02:08, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 26/10/2021 03.28, Simon Glass wrote:
>>> Hi Rasmus,
>>>
>>> On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
>>>> The build system already automatically looks for and includes an
>>>> in-tree *-u-boot.dtsi when building the control .dtb. However, there
>>>> are some things that are awkward to maintain in such an in-tree file,
>>>> most notably the metadata associated to public keys used for verified
>>>> boot.
>>>>
>>>> The only "official" API to get that metadata into the .dtb is via
>>>> mkimage, as a side effect of building an actual signed image. But
>>>> there are multiple problems with that. First of all, the final U-Boot
>>>> (be it U-Boot proper or an SPL) image is built based on a binary
>>>> image, the .dtb, and possibly some other binary artifacts. So
>>>> modifying the .dtb after the build requires the meta-buildsystem
>>>> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
>>>> that are already known to and handled by U-Boot's build system,
>>>> resulting in needless duplication of code. It's also somewhat annoying
>>>> and inconsistent to have a .dtb file in the build folder which is not
>>>> generated by the command listed in the corresponding .cmd file (that
>>>> of course applies to any generated file).
>>>>
>>>> So the contents of the /signature node really needs to be baked into
>>>> the .dtb file when it is first created, which means providing the
>>>> relevant data in the form of a .dtsi file. One could in theory put
>>>> that data into the *-u-boot.dtsi file, but it's more convenient to be
>>>> able to provide it externally: For example, when developing for a
>>>> customer, it's common to use a set of dummy keys for development,
>>>> while the consultants do not (and should not) have access to the
>>>> actual keys used in production. For such a setup, it's easier if the
>>>> keys used are chosen via the meta-buildsystem and the path(s) patched
>>>> in during the configure step. And of course, nothing prevents anybody
>>>> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
>>>> for that matter from including the public key metadata in the
>>>> *-u-boot.dtsi directly and ignore this feature.
>>>>
>>>> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
>>>> it can be used for providing the contents of the /config/environment
>>>> node, so I don't want to tie this exclusively to use for verified
>>>> boot.
>>>>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>> ---
>>>>
>>>> Getting the public key metadata into .dtsi form can be done with a
>>>> little scripting (roughly 'mkimage -K' of a dummy image followed by
>>>> 'dtc -I dtb -O dts'). I have a script that does that, along with
>>>> options to set 'required' and 'required-mode' properties, include
>>>> u-boot,dm-spl properties in case one wants to check U-Boot proper from
>>>> SPL with the verified boot mechanism, etc. I'm happy to share that
>>>> script if this gets accepted, but it's moot if this is rejected.
>>>>
>>>> I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
>>>> to disentangle the kernel and u-boot builds (or u-boot and SPL builds
>>>> for that matter!), but as I've since realized, that isn't quite enough
>>>> - the above points re modifying the .dtb after it is created but
>>>> before that is used to create further build artifacts still
>>>> stand. However, such a tool could still be useful for creating the
>>>> .dtsi info without the private keys being present, and my key2dtsi.sh
>>>> script could easily be modified to use a tool like that should it ever
>>>> appear.
>>>>
>>>> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
>>>> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
>>>>
>>>>  dts/Kconfig          | 9 +++++++++
>>>>  scripts/Makefile.lib | 2 ++
>>>>  2 files changed, 11 insertions(+)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> I suggest adding this to the documentation somewhere in doc/develop/devicetree/
>>
>> Will do.
>>
>>> Getting the key into the U-Boot .dtb is normally done with mkimage, as
>>> you say. I don't really understand why this approach is easier.
>>
>> I think I explained that in the commit message, but let me try with a
>> more concrete example. I'm building, using Yocto as the umbrella build
>> system, for a SOC that needs an SPL + U-Boot proper.
>>
>> So I have a U-Boot recipe that handles the configuration and
>> compilation. That's mostly a matter of "make foo_defconfig" followed by
>> "make ${UBOOT_TARGETS}" where UBOOT_TARGETS is something set in the
>> machine config. That results in two artifacts, say SPL and u-boot.itb
>> (the names don't really matter) that are almost ready to be written to
>> whatever storage is used for them. Most likely, the SPL binary is built
>> from a u-boot-spl-nodtb.bin and a spl.dtb and perhaps there's some
>> SOC-specific header in front of it all that the hardware/firmware needs,
>> those details are hidden by U-Boot's build system invoking the right
>> flavour of mkimage with the right parameters. If I really want to know,
>> I can look in spl/.SPL.cmd to see just how it was made [unless it's
>> binman creating a flash.bin, because then it's just black magic]. But I
>> usually don't need to.
>>
>> Enter verified boot. SPL needs to verify U-Boot, and U-Boot must in turn
>> verify the kernel. I can easily, as a post-compile step, create a signed
>> version of u-boot.itb. But the -K option is rather useless here, because
>> SPL has already been built. So if I were to only add the corresponding
>> public key to SPL's dtb after/while signing U-Boot proper, I'd have to
>> manually repeat the step(s) needed to create SPL in the first place. Not
>> to mention that the .dtb living inside u-boot.itb doesn't have the
>> public key needed for verifying the kernel, so I'd _actually_ first have
>> to repeat whatever steps were done to create u-boot.itb, after using
>> mkimage to sign some dummy image just to use the -K option to modify
>> u-boot.dtb. It's really cumbersome.
>>
>> So part of the problem is this "you can only get the public keys in the
>> form required inside the .dtb as a side-effect of signing an image". I
>> believe that's a fundamental design mistake, hence my attempt at
>> creating the fdt_add_pubkey tool. But even with that available, adding
>> the pubkey info into an already-compiled .dtb isn't really enough,
>> because the .dtb gets built as part of a normal "make". Hence the need
>> for a way to ensure the pubkey info gets baked into that .dtb during the
>> build.
>>
>> Yes, I then need to prepare proper .dtsi files. But since key generation
>> is a mostly one-time/manual thing, that easily goes along with the
>> instructions (or script) that generates those, and for every foo.crt,
>> I'll just have that directory also contain a foo.dtsi, which I can then
>> point at during do_configure.
>>
>>> Also, is there any interest in using binman?
>>
>> Not really. I mean, it's fine if U-Boot internally uses that, and I can
>> just take the final output and use that. But as long as binman doesn't
>> play nice with Kbuild and e.g. tells the commands that were used to
>> create a given binary, and pollutes the build dir with stuff that's not
>> removed by "make clean", I'm not very enthusiastic about adding more
>> uses myself.
>>
>> Also, this:
>>
>>   BINMAN  all
>> Image 'main-section' is missing external blobs and is non-functional:
>> blob-ext@3
>>
>> Some images are invalid
>> $ echo $?
>> 0
>>
>> Really? When building manually, perhaps the developer sees that warning
>> (unless there were other non-BINMAN targets that make decided to build
>> later, making the above scroll away...), but it's not very useful in
>> some CI scenario where artifacts get deployed automatically to a test
>> system after a successful build. Recovering boards with a broken
>> bootloader easily costs many man-hours, plus the time to figure out
>> what's wrong.
> 
> I still think binman is the best solution here. The points you are
> raising are annoyances that can be resolved.
> 
> We could return a non-zero value from binman when external blobs are
> missing; we'd just need to use a special value (we use 101 for
> buildman) and update the CI to detect and ignore that.

Eh, does make(1) exit with a code that depends on the exit code from a
failing target? What if multiple targets failed to build? Unless one can
quote chapter-and-verse from make documentation, I don't think that's a
particularly robust solution.

> Normally people use a separate output directory from the source
> directory, which is why the 'pollution' is not normally a big problem.

Well, build systems like Yocto do set up a separate build dir, but
that's not really important, as I rarely look inside ${S} nor ${B} - but
when doing my own tinkering, I almost always build in the source tree,
simply because that's a lot faster for quick experiments. So I'd wish
all binman's temporary files were stowed away in some .binman_tmp dir in
the top build dir (whether that's the source dir or an out-of-tree dir).
Commits like 824204e42 are a strong indication that the current state of
things is broken and doesn't scale.

> But it is possible to resolve that problem and it has been discussed
> on the mailing list. It's just that no one has done it yet.
> 
> In what way does binman play badly with Kbuild?

My main grievance is that when binman is merely a thin wrapper around
some external tool (often mkimage or one of its derivates), there's no
.<target>.cmd file generated allowing me to see just what command was
run to generate <target>. Another thing is that I can't do "make <target>".

> I'd really suggest changing the mindset to separating build from
> packaging, perhaps by having a separate yocto package/recipe that puts
> the firmware together, adds the signature, etc. 

Oh, I couldn't agree more, and in fact I've already done that for all
the kernels I build; Yocto's kernel-fitimage.bbclass is simply too
inflexible to use directly from the kernel recipe, so I just build an
Image (or Image.gz) in the kernel recipe, then have a separate
kernel-fit.bb recipe for actually assembling the different FIT images I
need (often one for normal use and another for bootstrapping, but
possibly more).

But I really can't see how binman helps in any way or form - in fact, it
obscures the process of creating such a separate recipe, exactly because
I can't extract the necessary magic invocation of mkimage that is needed
to wrap SPL in the header format expected by the hardware.

And the thing about "adding the signature" - yes, indeed, _signing_ can
and should be done after building. But that is not at all what this
started with, this is about embedding the metadata that U-Boot (or SPL)
will need for _verifying_ during the build itself - when the private key
may not even be available. Again, I think that it's a fundamental design
bug that generating and adding that metadata in the form needed by
U-Boot can only be done as a side effect of signing some unrelated image.

Rasmus

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Rasmus,

On Mon, 24 Jan 2022 at 03:42, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 14/01/2022 17.51, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 26 Oct 2021 at 02:08, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> On 26/10/2021 03.28, Simon Glass wrote:
> >>> Hi Rasmus,
> >>>
> >>> On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes
> >>> <rasmus.villemoes@prevas.dk> wrote:
> >>>>
> >>>> The build system already automatically looks for and includes an
> >>>> in-tree *-u-boot.dtsi when building the control .dtb. However, there
> >>>> are some things that are awkward to maintain in such an in-tree file,
> >>>> most notably the metadata associated to public keys used for verified
> >>>> boot.
> >>>>
> >>>> The only "official" API to get that metadata into the .dtb is via
> >>>> mkimage, as a side effect of building an actual signed image. But
> >>>> there are multiple problems with that. First of all, the final U-Boot
> >>>> (be it U-Boot proper or an SPL) image is built based on a binary
> >>>> image, the .dtb, and possibly some other binary artifacts. So
> >>>> modifying the .dtb after the build requires the meta-buildsystem
> >>>> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> >>>> that are already known to and handled by U-Boot's build system,
> >>>> resulting in needless duplication of code. It's also somewhat annoying
> >>>> and inconsistent to have a .dtb file in the build folder which is not
> >>>> generated by the command listed in the corresponding .cmd file (that
> >>>> of course applies to any generated file).
> >>>>
> >>>> So the contents of the /signature node really needs to be baked into
> >>>> the .dtb file when it is first created, which means providing the
> >>>> relevant data in the form of a .dtsi file. One could in theory put
> >>>> that data into the *-u-boot.dtsi file, but it's more convenient to be
> >>>> able to provide it externally: For example, when developing for a
> >>>> customer, it's common to use a set of dummy keys for development,
> >>>> while the consultants do not (and should not) have access to the
> >>>> actual keys used in production. For such a setup, it's easier if the
> >>>> keys used are chosen via the meta-buildsystem and the path(s) patched
> >>>> in during the configure step. And of course, nothing prevents anybody
> >>>> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> >>>> for that matter from including the public key metadata in the
> >>>> *-u-boot.dtsi directly and ignore this feature.
> >>>>
> >>>> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> >>>> it can be used for providing the contents of the /config/environment
> >>>> node, so I don't want to tie this exclusively to use for verified
> >>>> boot.
> >>>>
> >>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>>> ---
> >>>>
> >>>> Getting the public key metadata into .dtsi form can be done with a
> >>>> little scripting (roughly 'mkimage -K' of a dummy image followed by
> >>>> 'dtc -I dtb -O dts'). I have a script that does that, along with
> >>>> options to set 'required' and 'required-mode' properties, include
> >>>> u-boot,dm-spl properties in case one wants to check U-Boot proper from
> >>>> SPL with the verified boot mechanism, etc. I'm happy to share that
> >>>> script if this gets accepted, but it's moot if this is rejected.
> >>>>
> >>>> I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
> >>>> to disentangle the kernel and u-boot builds (or u-boot and SPL builds
> >>>> for that matter!), but as I've since realized, that isn't quite enough
> >>>> - the above points re modifying the .dtb after it is created but
> >>>> before that is used to create further build artifacts still
> >>>> stand. However, such a tool could still be useful for creating the
> >>>> .dtsi info without the private keys being present, and my key2dtsi.sh
> >>>> script could easily be modified to use a tool like that should it ever
> >>>> appear.
> >>>>
> >>>> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
> >>>> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
> >>>>
> >>>>  dts/Kconfig          | 9 +++++++++
> >>>>  scripts/Makefile.lib | 2 ++
> >>>>  2 files changed, 11 insertions(+)
> >>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>
> >>> I suggest adding this to the documentation somewhere in doc/develop/devicetree/
> >>
> >> Will do.
> >>
> >>> Getting the key into the U-Boot .dtb is normally done with mkimage, as
> >>> you say. I don't really understand why this approach is easier.
> >>
> >> I think I explained that in the commit message, but let me try with a
> >> more concrete example. I'm building, using Yocto as the umbrella build
> >> system, for a SOC that needs an SPL + U-Boot proper.
> >>
> >> So I have a U-Boot recipe that handles the configuration and
> >> compilation. That's mostly a matter of "make foo_defconfig" followed by
> >> "make ${UBOOT_TARGETS}" where UBOOT_TARGETS is something set in the
> >> machine config. That results in two artifacts, say SPL and u-boot.itb
> >> (the names don't really matter) that are almost ready to be written to
> >> whatever storage is used for them. Most likely, the SPL binary is built
> >> from a u-boot-spl-nodtb.bin and a spl.dtb and perhaps there's some
> >> SOC-specific header in front of it all that the hardware/firmware needs,
> >> those details are hidden by U-Boot's build system invoking the right
> >> flavour of mkimage with the right parameters. If I really want to know,
> >> I can look in spl/.SPL.cmd to see just how it was made [unless it's
> >> binman creating a flash.bin, because then it's just black magic]. But I
> >> usually don't need to.
> >>
> >> Enter verified boot. SPL needs to verify U-Boot, and U-Boot must in turn
> >> verify the kernel. I can easily, as a post-compile step, create a signed
> >> version of u-boot.itb. But the -K option is rather useless here, because
> >> SPL has already been built. So if I were to only add the corresponding
> >> public key to SPL's dtb after/while signing U-Boot proper, I'd have to
> >> manually repeat the step(s) needed to create SPL in the first place. Not
> >> to mention that the .dtb living inside u-boot.itb doesn't have the
> >> public key needed for verifying the kernel, so I'd _actually_ first have
> >> to repeat whatever steps were done to create u-boot.itb, after using
> >> mkimage to sign some dummy image just to use the -K option to modify
> >> u-boot.dtb. It's really cumbersome.
> >>
> >> So part of the problem is this "you can only get the public keys in the
> >> form required inside the .dtb as a side-effect of signing an image". I
> >> believe that's a fundamental design mistake, hence my attempt at
> >> creating the fdt_add_pubkey tool. But even with that available, adding
> >> the pubkey info into an already-compiled .dtb isn't really enough,
> >> because the .dtb gets built as part of a normal "make". Hence the need
> >> for a way to ensure the pubkey info gets baked into that .dtb during the
> >> build.
> >>
> >> Yes, I then need to prepare proper .dtsi files. But since key generation
> >> is a mostly one-time/manual thing, that easily goes along with the
> >> instructions (or script) that generates those, and for every foo.crt,
> >> I'll just have that directory also contain a foo.dtsi, which I can then
> >> point at during do_configure.
> >>
> >>> Also, is there any interest in using binman?
> >>
> >> Not really. I mean, it's fine if U-Boot internally uses that, and I can
> >> just take the final output and use that. But as long as binman doesn't
> >> play nice with Kbuild and e.g. tells the commands that were used to
> >> create a given binary, and pollutes the build dir with stuff that's not
> >> removed by "make clean", I'm not very enthusiastic about adding more
> >> uses myself.
> >>
> >> Also, this:
> >>
> >>   BINMAN  all
> >> Image 'main-section' is missing external blobs and is non-functional:
> >> blob-ext@3
> >>
> >> Some images are invalid
> >> $ echo $?
> >> 0
> >>
> >> Really? When building manually, perhaps the developer sees that warning
> >> (unless there were other non-BINMAN targets that make decided to build
> >> later, making the above scroll away...), but it's not very useful in
> >> some CI scenario where artifacts get deployed automatically to a test
> >> system after a successful build. Recovering boards with a broken
> >> bootloader easily costs many man-hours, plus the time to figure out
> >> what's wrong.
> >
> > I still think binman is the best solution here. The points you are
> > raising are annoyances that can be resolved.
> >
> > We could return a non-zero value from binman when external blobs are
> > missing; we'd just need to use a special value (we use 101 for
> > buildman) and update the CI to detect and ignore that.
>
> Eh, does make(1) exit with a code that depends on the exit code from a
> failing target? What if multiple targets failed to build? Unless one can
> quote chapter-and-verse from make documentation, I don't think that's a
> particularly robust solution.

Make is not the issue here. The basic problem is that we want to have
three results:

- build failed, e.g. a compiler error
- build succeeded but some binary blobs or tools are missing so the
image won't work
- build succeeded, image is valid

>
> > Normally people use a separate output directory from the source
> > directory, which is why the 'pollution' is not normally a big problem.
>
> Well, build systems like Yocto do set up a separate build dir, but
> that's not really important, as I rarely look inside ${S} nor ${B} - but
> when doing my own tinkering, I almost always build in the source tree,
> simply because that's a lot faster for quick experiments. So I'd wish
> all binman's temporary files were stowed away in some .binman_tmp dir in
> the top build dir (whether that's the source dir or an out-of-tree dir).
> Commits like 824204e42 are a strong indication that the current state of
> things is broken and doesn't scale.

Well that commit was really just rearranging the deck chairs. I didn't
even see it.

The topic has come up before and I have suggested how it could be
done. There is even a long-standing TODO in the binman readme about
it.

>
> > But it is possible to resolve that problem and it has been discussed
> > on the mailing list. It's just that no one has done it yet.
> >
> > In what way does binman play badly with Kbuild?
>
> My main grievance is that when binman is merely a thin wrapper around
> some external tool (often mkimage or one of its derivates), there's no
> .<target>.cmd file generated allowing me to see just what command was
> run to generate <target>. Another thing is that I can't do "make <target>".

You can enable logging with BINMAN_VERBOSE. See also the bintool
series (which I should apply soon) which provides a single place for
running these tools. So we could perhaps improve things in this area.

>
> > I'd really suggest changing the mindset to separating build from
> > packaging, perhaps by having a separate yocto package/recipe that puts
> > the firmware together, adds the signature, etc.
>
> Oh, I couldn't agree more, and in fact I've already done that for all
> the kernels I build; Yocto's kernel-fitimage.bbclass is simply too
> inflexible to use directly from the kernel recipe, so I just build an
> Image (or Image.gz) in the kernel recipe, then have a separate
> kernel-fit.bb recipe for actually assembling the different FIT images I
> need (often one for normal use and another for bootstrapping, but
> possibly more).
>
> But I really can't see how binman helps in any way or form - in fact, it
> obscures the process of creating such a separate recipe, exactly because
> I can't extract the necessary magic invocation of mkimage that is needed
> to wrap SPL in the header format expected by the hardware.

Why are you wanting to do that? You can just run binman again, can't you?

>
> And the thing about "adding the signature" - yes, indeed, _signing_ can
> and should be done after building. But that is not at all what this
> started with, this is about embedding the metadata that U-Boot (or SPL)
> will need for _verifying_ during the build itself - when the private key
> may not even be available. Again, I think that it's a fundamental design
> bug that generating and adding that metadata in the form needed by
> U-Boot can only be done as a side effect of signing some unrelated image.

It is a side effect of signing *the same* image, i.e. the image that
holds the signature and the public key. There is only one image, the
firmware image produced by binman.

Regards,
Simon

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

On 24/01/2022 18.57, Simon Glass wrote:

>> And the thing about "adding the signature" - yes, indeed, _signing_ can
>> and should be done after building. But that is not at all what this
>> started with, this is about embedding the metadata that U-Boot (or SPL)
>> will need for _verifying_ during the build itself - when the private key
>> may not even be available. Again, I think that it's a fundamental design
>> bug that generating and adding that metadata in the form needed by
>> U-Boot can only be done as a side effect of signing some unrelated image.
> 
> It is a side effect of signing *the same* image, i.e. the image that
> holds the signature and the public key. There is only one image, the
> firmware image produced by binman.

Huh? Are we talking about the same thing? What you write makes no sense
at all.

Rasmus

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Rasmus,

On Mon, 24 Jan 2022 at 15:15, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 24/01/2022 18.57, Simon Glass wrote:
>
> >> And the thing about "adding the signature" - yes, indeed, _signing_ can
> >> and should be done after building. But that is not at all what this
> >> started with, this is about embedding the metadata that U-Boot (or SPL)
> >> will need for _verifying_ during the build itself - when the private key
> >> may not even be available. Again, I think that it's a fundamental design
> >> bug that generating and adding that metadata in the form needed by
> >> U-Boot can only be done as a side effect of signing some unrelated image.
> >
> > It is a side effect of signing *the same* image, i.e. the image that
> > holds the signature and the public key. There is only one image, the
> > firmware image produced by binman.
>
> Huh? Are we talking about the same thing? What you write makes no sense
> at all.

Perhaps it is a terminology thing. For me:

image: the final firmware image with everything in it
binary: a component of the image

So there is only one image.

Regards,
Simon

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Rasmus Villemoes]

On 25/01/2022 00.50, Simon Glass wrote:
> Hi Rasmus,
> 
> On Mon, 24 Jan 2022 at 15:15, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 24/01/2022 18.57, Simon Glass wrote:
>>
>>>> And the thing about "adding the signature" - yes, indeed, _signing_ can
>>>> and should be done after building. But that is not at all what this
>>>> started with, this is about embedding the metadata that U-Boot (or SPL)
>>>> will need for _verifying_ during the build itself - when the private key
>>>> may not even be available. Again, I think that it's a fundamental design
>>>> bug that generating and adding that metadata in the form needed by
>>>> U-Boot can only be done as a side effect of signing some unrelated image.
>>>
>>> It is a side effect of signing *the same* image, i.e. the image that
>>> holds the signature and the public key. There is only one image, the
>>> firmware image produced by binman.
>>
>> Huh? Are we talking about the same thing? What you write makes no sense
>> at all.
> 
> Perhaps it is a terminology thing. For me:
> 
> image: the final firmware image with everything in it
> binary: a component of the image
> 
> So there is only one image.

It still makes no sense. I'm talking about the -K option to mkimage,
which modifies the .dtb-which-is-to-be-used-by-U-Boot while building a
FIT image containing kernel, initramfs and whatnot. That's certainly two
very different images (or, in your terminology, one image and an
entirely distinct binary destined to be included in another image).

Or for that matter, if one wants to do the same dance during SPL ->
U-Boot, this could be the FIT image containing U-Boot proper and its
control dtb (which must, of course, already have had the
public-key-for-verifying-the-kernel embedded) vs the SPL's control .dtb,
again two entirely different beasts.

The point is, I should not _need_ to sign anything in order to get the
public key info into the control .dtbs. That's why I call it a design bug.

I have explained all of this several times, including in the commit
message.

And the more you push for everyone to be using binman to generate the
final images, the more important it is that the U-Boot .dtb contains the
public-key-info before it gets used to build the final U-Boot image
(whether that's itself a FIT or just a concatenation of u-boot.bin and
u-boot.dtb). If I was assembling the final U-Boot image myself, I would
just need the u-boot recipe to produce u-boot.bin and u-boot.dtb, and
then I could have a separate step that updated u-boot.dtb. But when
binman is in charge of doing that assembly, there's no such place to
hook in, so I just want a way to ensure that the u-boot.dtb that gets
generated is already the final one - which is what this patch is
supposed to help with.

Rasmus

Re: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

On 25/01/2022 00.50, Simon Glass wrote:
> Hi Rasmus,
>
> On Mon, 24 Jan 2022 at 15:15, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 24/01/2022 18.57, Simon Glass wrote:
>>
>>>> And the thing about "adding the signature" - yes, indeed, _signing_ can
>>>> and should be done after building. But that is not at all what this
>>>> started with, this is about embedding the metadata that U-Boot (or SPL)
>>>> will need for _verifying_ during the build itself - when the private key
>>>> may not even be available. Again, I think that it's a fundamental design
>>>> bug that generating and adding that metadata in the form needed by
>>>> U-Boot can only be done as a side effect of signing some unrelated image.
>>>
>>> It is a side effect of signing *the same* image, i.e. the image that
>>> holds the signature and the public key. There is only one image, the
>>> firmware image produced by binman.
>>
>> Huh? Are we talking about the same thing? What you write makes no sense
>> at all.
>
> Perhaps it is a terminology thing. For me:
>
> image: the final firmware image with everything in it
> binary: a component of the image
>
> So there is only one image.

It still makes no sense. I'm talking about the -K option to mkimage,
which modifies the .dtb-which-is-to-be-used-by-U-Boot while building a
FIT image containing kernel, initramfs and whatnot. That's certainly two
very different images (or, in your terminology, one image and an
entirely distinct binary destined to be included in another image).

Or for that matter, if one wants to do the same dance during SPL ->
U-Boot, this could be the FIT image containing U-Boot proper and its
control dtb (which must, of course, already have had the
public-key-for-verifying-the-kernel embedded) vs the SPL's control .dtb,
again two entirely different beasts.

The point is, I should not _need_ to sign anything in order to get the
public key info into the control .dtbs. That's why I call it a design bug.

I have explained all of this several times, including in the commit
message.

And the more you push for everyone to be using binman to generate the
final images, the more important it is that the U-Boot .dtb contains the
public-key-info before it gets used to build the final U-Boot image
(whether that's itself a FIT or just a concatenation of u-boot.bin and
u-boot.dtb). If I was assembling the final U-Boot image myself, I would
just need the u-boot recipe to produce u-boot.bin and u-boot.dtb, and
then I could have a separate step that updated u-boot.dtb. But when
binman is in charge of doing that assembly, there's no such place to
hook in, so I just want a way to ensure that the u-boot.dtb that gets
generated is already the final one - which is what this patch is
supposed to help with.

Rasmus

Applied to u-boot-dm, thanks!