[PATCH 5/7] Makefile: Add a target for building capsules

Tue Jun 27 19:42:00 CEST 2023

hi Simon,

On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > hi Simon,
> > > > > > > > >
> > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Sughosh,
> > > > > > > > > >
> > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Add a target for building EFI capsules. The capsule parameters are
> > > > > > > > > > > specified through a config file, and the path to the config file is
> > > > > > > > > > > specified through CONFIG_EFI_CAPSULE_CFG_FILE. When the config file is
> > > > > > > > > > > not specified, the command only builds tools.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  Makefile | 9 +++++++++
> > > > > > > > > > >  1 file changed, 9 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/Makefile b/Makefile
> > > > > > > > > > > index 10bfaa52ad..96db29aa77 100644
> > > > > > > > > > > --- a/Makefile
> > > > > > > > > > > +++ b/Makefile
> > > > > > > > > > > @@ -1151,6 +1151,15 @@ dtbs: dts/dt.dtb
> > > > > > > > > > >  dts/dt.dtb: u-boot
> > > > > > > > > > >         $(Q)$(MAKE) $(build)=dts dtbs
> > > > > > > > > > >
> > > > > > > > > > > +quiet_cmd_mkeficapsule = MKEFICAPSULE $@
> > > > > > > > > > > +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule $@
> > > > > > > > > > > +
> > > > > > > > > > > +PHONY += capsule
> > > > > > > > > > > +capsule: tools
> > > > > > > > > > > +ifneq ($(CONFIG_EFI_CAPSULE_CFG_FILE),"")
> > > > > > > > > > > +       $(call cmd,mkeficapsule)
> > > > > > > > > > > +endif
> > > > > > > > > > > +
> > > > > > > > > > >  quiet_cmd_copy = COPY    $@
> > > > > > > > > > >        cmd_copy = cp $< $@
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > 2.34.1
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > We should be using binman to build images...you seem to be building
> > > > > > > > > > something in parallel with that. Can you please take a look at binman?
> > > > > > > > >
> > > > > > > > > Again, I had explored using binman for this task. The one issue where
> > > > > > > > > I find the above flow better is that I can simply build my payload
> > > > > > > > > image(s) followed by 'make capsule' to generate the capsules for
> > > > > > > > > earlier generated images. In it's current form, I don't see an easy
> > > > > > > > > way to enforce this dependency in binman when I want to build the
> > > > > > > > > payload followed by generation of capsules. I did see the mention of
> > > > > > > > > encapsulating an entry within another dependent entry, but I think
> > > > > > > > > that makes the implementation more complex than it ought to be.
> > > > > > > > >
> > > > > > > > > I think it is much easier to use the make flow to generate the images
> > > > > > > > > followed by capsules, instead of tweaking the binman node to first
> > > > > > > > > generate the payload images, followed by enabling the capsule node to
> > > > > > > > > build the capsules. If there is an easy way of enforcing this
> > > > > > > > > dependency, please let me know. Thanks
> > > > > > > >
> > > > > > > > Can you share your explorations? I think the capsule should be created
> > > > > > > > as part of the build, if enabled. Rather than changing the input
> > > > > > > > files, binman should produce new output files.
> > > > > > >
> > > > > > > This is an issue of handling dependencies in binman, and not changing
> > > > > > > input files. We do not have support for telling binman "build/generate
> > > > > > > this particular image first before you proceed to build the capsules
> > > > > > > using the earlier built images". I am not sure if this can be done in
> > > > > > > a generic manner in binman, so that irrespective of the image being
> > > > > > > generated, it can be specified to build capsules once the capsule
> > > > > > > input images have been generated.
> > > > > >
> > > > > > I'm just not sure what you are getting out here.
> > > > > >
> > > > > > See INPUTS-y for the input files to binman. Then binman uses these to
> > > > > > generate output files. It does not mess with the input files, nor
> > > > > > should it. Please read the top part of the Binman motivation to
> > > > > > understand how all this works.
> > > > > >
> > > > > > At the risk of repeating myself, we want the Makefile to focus on
> > > > > > building U-Boot, with Binman handling the laterprocessing of those
> > > > > > files. Binman may run as part of the U-Boot build, and/or can be run
> > > > > > later, with the input files.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > We are trying to remove most of the output logic in Makefile. It
> > > > > > > > should just be producing input files for binman.
> > > > > > >
> > > > > > > I understand. However, like I mentioned above, as of now, we don't
> > > > > > > have a way of handling dependencies in binman, at least in a generic
> > > > > > > manner. Once this support gets added, I know that it would be trivial
> > > > > > > to add support for building capsules in binman.
> > > > > >
> > > > > > What dependencies do you need? Please describe it in a simple manner
> > > > > > so I can understand. It cannot involve change the binman input files.
> > > > >
> > > > > Consider the following scenarios.
> > > > >
> > > > > One board, say Board A uses fip.bin as the input file(payload) for
> > > > > generating the capsule file. The fip.bin is being generated through
> > > > > binman. A binman entry is also added for generating the capsule(say
> > > > > fip.capule). Now, binman has to generate fip.bin *and subsequently*
> > > > > fip.capsule, as the capsule file will contain fip.bin as it's
> > > > > payload(input).
> > > > >
> > > > > Second Board B, uses u-boot.itb, which is a FIT image, as the input
> > > > > file for generating the capsule. The u-boot.itb is being generated
> > > > > through binman, and so is the capsule. But binman needs to build the
> > > > > u-boot.itb *before* it generates the corresponding capsule file, which
> > > > > uses u-boot.itb as the capsule payload.
> > > > >
> > > > > There can be multiple such examples of input files being generated by
> > > > > binman, followed by the capsule getting generated by binman. How do I
> > > > > specify this dependency in binman -- build/generate the input file
> > > > > first, and then use that files in generating the capsule.
> > >
> > > At present you can do this by ordering the images correctly, i.e. put
> > > the first image first and the dependent image after it. For the
> > > dependent image you can have a blob which is the entire first image.
> >
> > If putting the image entries in a certain order under the binman node
> > *guarantees* the generation of the first image prior to the generation
> > of the second image, I think that should work for my use case.
> > However, when I look at the binman.rst document, I see this mentioned
> > under the 'Image dependencies' section.
> >
> > <quote>
> > Binman does not currently support images that depend on each other. For example,
> > if one image creates `fred.bin` and then the next uses this `fred.bin` to
> > produce a final `image.bin`, then the behaviour is undefined. It may work, or it
> > may produce an error about `fred.bin` being missing, or it may use a version of
> > `fred.bin` from a previous run.
> > </quote>
> >
> > I believe the above is precisely what my use case is. One image
> > generating fip.bin(for e.g.), and the next image using this fip.bin to
> > produce the final fip.capsule. Or is this a case of the document not
> > reflecting the actual code?
>
> So long as the images are in the correct order in the resulting .dtb,
> then it will work. Quite a few boards rely on this.
>
> This comment is there because I planned to implement concurrent image
> generation (as is done for sections). But for now this is not
> implemented. Also I plan to implement templates before parallel
> images, so we can handle dependencies in a more general way.
>
> So if that is the only blocker, I am sorry for the docs being too
> conservative. I will send a patch.
>
> >
> > >
> > > If you are trying to do a second binman operation later, then perhaps
> > > something like 'binman sign' would be useful. Then people can provide
> > > their own key and sign the images in a separate binman operation. This
> > > is likely needed anyway for things to work on a signing server.
> > >
> > > >
> > > > Can you confirm if the above dependencies can be handled in binman
> > > > currently. If not, I'd suggest you remove your Nak for patch 6 of this
> > > > series [1]. Like I mentioned earlier, if there is a means of
> > > > specifying dependencies for generating images in binman, moving the
> > > > capsule generation to binman will not be a difficult task.
> > > >
> > > > Also, can you go through the other set of patches in V2, specifically
> > > > the ones where putting the public key ESL into the dtb is being done
> > > > through binman.
> > >
> > > The order of operation is supposed to be:
> > >
> > > 1. Various projects used to build their ouputs (e.g. TF-A)
> > > 2. Makefile used to build U-Boot:
> > > 2a. The build produces a set of files which serve as inputs to binman (INPUTS-y)
> > > 2b. Binman runs on INPUTS-y, picking up all the bits and creating the
> > > final firmware image(s)
> > > 3. If necessary, separate from the U-Boot build, binman can be used
> > > separately to do signing or whatever is needed on the final image(s)
> > >
> > > I understand that the public key is available in a CONFIG, so it
> > > should be possible to embed it in the build as input, either as a
> > > .dtsi build using 2a, or as a binary file pulled in by binman in 2b.
> >
> > Using a dtsi would mean that every platform which wants to enable
> > capsule authentication would need to add a signature node to it's
> > dtsi. Instead, is it not simpler to just generate a dtbo and merge it
> > with the dtb being generated. That is what was being done in V1 [1].
>
> Why is it simpler? The .dtsi is where we are supposed to put
> devicetree properties. It seems a lot harder (to me) to add it later.
> It could be a #include, or even just put it in the .dtsi if it is the
> same for all boards?

I was referring to the solution in V1 of the patch series [1]. All
that was needed there was the path to the public key ESL file, which
was being provided through the CONFIG_EFI_CAPSULE_ESL_FILE. The
embed_capsule_key.sh would take care of doing what you are suggesting
to be done via dtsi. When adding a node to the dtsi, the user will
still have to add a node to the dtsi and include the contents of the
ESL file. All that was being automated through the script in V1.

>
> > For your suggestion to pull it in as a binary file in binman, that
> > still does not fix the issue of not changing INPUTS-y.
> >
> > If you ask me, the embedding of the public-key into the dtb is not a
> > task suitable for binman. Why? Because this task is actually changing
> > one of the INPUTS-y file that feeds into binman. And yes, we can
> > generate a different set of files, like u-boot-capsule.dtb and
> > u-boot-capsule.bin -- implementing that is not at all difficult. But
> > like I had highlighted earlier, and also explained by Ilias, we
> > already have platforms that use capsule updates and which use
> > u-boot.dtb and u-boot.bin. Also, platforms would not want a separate
>
> Those platforms should change, IMO. But how can this be, when this
> functionality has not yet been added to U-Boot?
>
> > set of files, one for normal boot, one when using capsules. So I think
> > it is imperative that we generate the same set of files irrespective
> > of whether a platform enables capsule updates. So a proper design
> > would be to add/embed the public key into the dtb as the dtb is
> > getting built. Again, this is what was being done in V1.
>
> I completely disagree with this. A capsule update is not the same as
> the vanilla board build / binary. Please can you just give up on this
> idea? Many platforms generate their output in separate files, e.g. see
> u-boot-rockchip.bin - it just does not make sense to change the built
> binary after it is built.

I completely agree with your last statement. Which is why I said that
if we want to use the same files which are otherwise
generated(u-boot.dtb, u-boot.bin), binman is not the right way to add
the public key to the platform's dtb -- that should be done when the
dtb is being built.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2023-June/520121.html

>
> >
> >
> > >
> > > The patches I reviewed are modifying the input files (INPUTS-y) which
> > > is not allowed. Nor does it seem to be necessary.
> >
> > The necessity was explained by me and Ilias earlier, and above.
>
> OK, but it still is not correct.
>
> Regards,
> Simon
>
> >
> > -sughosh
> >
> > [1] - https://lists.denx.de/pipermail/u-boot/2023-June/520121.html
> >
> >
> >
> >
> > >
> > > As to having 'real' dependencies between images in Binman, I believe
> > > that should be possible. At the moment images are entirely
> > > independent, but I am looking at implementing a simple 'template'
> > > scheme, where you can include some or all of an image description in
> > > another image. See [1] common-part in case you have thoughts on that.
> > >
> > > Regards,
> > > Simon
> > >
> > > [1] https://patchwork.ozlabs.org/project/uboot/patch/9e905abd369f1d8b182e6fc6e3c9ef4b1526bf76.1685975993.git.jan.kiszka@siemens.com/