hi Simon, On Wed, 28 Jun 2023 at 15:49, Simon Glass <s...@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 28 Jun 2023 at 11:00, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > hi Simon, > > > > On Wed, 28 Jun 2023 at 13:12, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Tue, 27 Jun 2023 at 18:42, Sughosh Ganu <sughosh.g...@linaro.org> > > > wrote: > > > > > > > > hi Simon, > > > > > > > > On Tue, 27 Jun 2023 at 17:51, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.g...@linaro.org> > > > > > wrote: > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > On Tue, 27 Jun 2023 at 16:50, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu > > > > > > > <sughosh.g...@linaro.org> wrote: > > > > > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > > > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu > > > > > > > > <sughosh.g...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > > > > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <s...@chromium.org> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu > > > > > > > > > > <sughosh.g...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > > > > > > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass > > > > > > > > > > > <s...@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > > > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu > > > > > > > > > > > > <sughosh.g...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > hi Simon, > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass > > > > > > > > > > > > > <s...@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu > > > > > > > > > > > > > > <sughosh.g...@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.g...@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. > > > > > > Instead of all that complexity, can you check the irc where Kwiboo > > > describes how to add a .dtsi fragment containing that CONFIG > > > > Not sure what is so complex about the script. But yes, I can add a > > dtsi file fragment. Only thing is, it will have to be added under all > > the arch/$ARCH/dts/ directories which would want to support capsule > > updates with authentication. The current logic in Makefile.lib looks > > for the *u-boot.dtsi files under the same dir location as the dts > > being built. But since you prefer having the dtsi file, I will add > > these for the sandbox and arm arch's for now. > > OK good. > > (the script is presumably not complex, but not having it at all is better) > (yes the dtsi includes are on a per-arch basis) > > > > > > > > > BTW the format you are using in the dtb looks to be binary. Have you > > > thought about using real properties and values like the existing > > > public key mechanism? Or is that binary format required by EFI? > > > > Yes, the UEFI spec defines the EFI Signature List(ESL) structure, and > > expects the public key certificate to be in the ESL format. The same > > structure is used for variable authentication as well -- the capsule > > authentication logic basically uses variable authentication > > functionality for signature verification. > > It does frustrate me that EFI insists on using ad-hoc binary formats > for things. It seems quite old-fashioned. It would be better to put > things in a node with real properties, etc. That way users can look at > it easily and we don't need so much special parsing code. > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > OK, so perhaps that is some progress. > > > > > > My second point (perhaps lost above) is that the capsule file should > > > be called u-boot-capsule.bin or something like that, not u-boot.bin, > > > since that is the output from the build system. > > > > I think this point is moot with the public key getting embedded > > through the dtsi file. > > Yes for u-boot-dtb since the public key will already be there. > > But for the capsule update, it should not modify u-boot.bin but > instead create a new file like u-boot-capsule.bin - right?
For the record, as discussed over irc, the capsule file is not part of u-boot.bin. It is placed separately on the EFI System Partition(ESP). I will move the generation of the capsules to binman and the embedding of the public key ESL to u-boot.dtsi in the next version. Thanks. -sughosh > > Regards, > Simon > [..]
