[RFC PATCH 2/4] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Sughosh Ganu]

hi Tom,

On Tue, 15 Aug 2023 at 00:48, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Aug 15, 2023 at 12:19:36AM +0530, Sughosh Ganu wrote:
> > hi Tom,
> >
> > On Mon, 14 Aug 2023 at 20:34, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Mon, Aug 14, 2023 at 02:33:07PM +0530, Sughosh Ganu wrote:
> > > > The EFI capsule authentication logic in u-boot expects the public key
> > > > in the form of an EFI Signature List(ESL) to be provided as part of
> > > > the platform's dtb. Currently, the embedding of the ESL file into the
> > > > dtb needs to be done manually.
> > > >
> > > > Add a target for generating a dtsi file which contains the signature
> > > > node with the ESL file included as a property under the signature
> > > > node. Include the dtsi file in the dtb. This brings the embedding of
> > > > the ESL in the dtb into the U-Boot build flow.
> > > >
> > > > The path to the ESL file is specified through the
> > > > CONFIG_EFI_CAPSULE_ESL_FILE symbol.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > ---
> > > >  lib/efi_loader/Kconfig             |  9 +++++++++
> > > >  lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
> > > >  scripts/Makefile.lib               | 17 +++++++++++++++++
> > > >  3 files changed, 37 insertions(+)
> > > >  create mode 100644 lib/efi_loader/capsule_esl.dtsi.in
> > > >
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index 9989e3f384..f40a62a0ba 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -272,6 +272,15 @@ config EFI_CAPSULE_MAX
> > > >         Select the max capsule index value used for capsule report
> > > >         variables. This value is used to create CapsuleMax variable.
> > > >
> > > > +config EFI_CAPSULE_ESL_FILE
> > > > +     string "Path to the EFI Signature List File"
> > > > +     default ""
> > >
> > > No default.
> >
> > I think Simon wanted to keep this so that it would not break the build
> > if no option was provided.
>
> No, that means that there's a missing dependency.  Blank defaults are
> almost never right.  And "so that we don't break configs" means that
> something else is wrong, generally a missing dependency.  And maybe I
> cut out too much context here as yes, this option needs to depend on
> some part of the capsule framework.

Okay. Yes, the above config does actually depend on EFI_CAPSULE_AUTHENTICATE.

>
> > > [snip]
> > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > index f41b16781d..cf4eef0b05 100644
> > > > --- a/scripts/Makefile.lib
> > > > +++ b/scripts/Makefile.lib
> > > > @@ -334,8 +334,25 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> > > >               ; \
> > > >       sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
> > > >
> > > > +ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > > > +quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
> > > > +cmd_capsule_esl_gen = \
> > > > +     $(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
> > > > +
> > > > +$(obj)/.capsule_esl.dtsi:
> > > > +     $(call cmd_capsule_esl_gen)
> > > > +
> > > > +capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
> > > > +capsule_esl_dtsi = .capsule_esl.dtsi
> > > > +capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))
> > >
> > > This seems right.
> > >
> > > > +include_files += $(capsule_esl_dtsi)
> > > > +
> > > > +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(obj)/.capsule_esl.dtsi FORCE
> > > > +     $(call if_changed_dep,dtc)
> > > > +else
> > > >  $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> > > >       $(call if_changed_dep,dtc)
> > > > +endif
> > >
> > > I think this implies to me that we should have been depending on
> > > $(include_files) (once renamed to be less vague) here already ?
> >
> > Okay. That can be done. I had kept the .capsule_esl.dtsi as a
> > dependency primarily because that file needs to be generated before it
> > can be included. I suppose the same can apply to some other dtsi as
> > well.
>
> You shouldn't need to have it be explicit, if it's in dtsi_include_list
> as there's a rule so make can resolve how to satisfy the dependency.
> But since we dynamically #include the other files, I don't know that
> they will be caught with the normal dependency logic.  But, it will
> still be cleaner to have the dtb rules depend on the automagic #include
> files too, rather than special casing the rule here I believe.

Okay, in that case, there can be a single rule for making the dtb.
Only the logic for generating the .capsule_esl.dtsi can then be put
under the ifdef.

-sughosh