Hi Tom,
On Fri, 14 Jan 2022 at 08:41, Tom Rini <tr...@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 <s...@chromium.org>
> > > Signed-off-by: Rasmus Villemoes <rasmus.villem...@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
