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

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@linaro.org>
---
 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

Hi Sughosh,

On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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?

Regards,
Simon

hi Simon,

On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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

-sughosh

Hi Sughosh,

On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

We are trying to remove most of the output logic in Makefile. It
should just be producing input files for binman.

Regards,
Simon

hi Simon,

On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

>
> 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.

-sughosh

Hi Sughosh,

On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

Regards,
Simon

hi Simon,

On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

-sughosh

hi Simon,

On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

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.

-sughosh

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

Hi Sughosh,

On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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 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.

The patches I reviewed are modifying the input files (INPUTS-y) which
is not allowed. Nor does it seem to be necessary.

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/

hi Simon,

On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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?

>
> 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].
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
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.


>
> 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.

-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/

Hi Sughosh,

On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > hi Simon,
> > > > > > > >
> > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Sughosh,
> > > > > > > > >
> > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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?

> 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.

>
>
> >
> > 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/

hi Simon,

On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > hi Simon,
> > > > > > > > >
> > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Sughosh,
> > > > > > > > > >
> > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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/

Hi Sughosh,

On Tue, 27 Jun 2023 at 18:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > hi Simon,
> > > > > > > >
> > > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Sughosh,
> > > > > > > > >
> > > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > hi Simon,
> > > > > > > > > >
> > > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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

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?

>
> >
> > > 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.

Regards,
Simon

>
> -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/

[...]

> > > > > 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
>
> 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?
>
> >
> > >
> > > > 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.

+CC Jonas who proposed the idea.

So one reasonable way to plug this was what Jonas and I discussed over IRC.
Instead of having a custom script injecting the 'signature' node in
the .dts we could add
- u-boot-cap-key.dtsi (or similar)
- if authenticated capsule updates are selected then
CONFIG_DEVICE_TREE_INCLUDES can be automatically selected to include
the .dts that contains the public portion of the key

That would take care of the public key inclusion in the final binary,
without going through binman since this is already working in
makefiles.   We need to add a few sanity checks.  E.g if the .esl file
is empty we need to pop a build error, but that's all sanely doable.

For the last part of the capsule generation, you can use binman.  You
can include the private key you need to sign the capsule in a config
file that's fed into it.  That tool would then call mkeficapsule and
generate the signed capsule update files.  You'll be missing a
makefile target 'make capsules', but given the fact you need to build
u-boot to include it in the capsule eventually, I don't think this is
a big deal.

Regards
/Ilias



>
> Regards,
> Simon
>
> >
> > -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/

hi Simon,

On Wed, 28 Jun 2023 at 13:12, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 27 Jun 2023 at 18:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > hi Simon,
> > > > > > > > >
> > > > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Sughosh,
> > > > > > > > > >
> > > > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > hi Simon,
> > > > > > > > > > >
> > > > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

>
> 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.

>
> >
> > >
> > > > 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.

-sughosh

>
> Regards,
> Simon
>
> >
> > -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/

Hi Sughosh,

On Wed, 28 Jun 2023 at 11:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 28 Jun 2023 at 13:12, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 27 Jun 2023 at 18:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > hi Simon,
> > > > > > > >
> > > > > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Sughosh,
> > > > > > > > >
> > > > > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > hi Simon,
> > > > > > > > > >
> > > > > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > >
> > > > > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > hi Simon,
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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?

Regards,
Simon
[..]

hi Simon,

On Wed, 28 Jun 2023 at 15:49, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 28 Jun 2023 at 11:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Wed, 28 Jun 2023 at 13:12, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 27 Jun 2023 at 18:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > hi Simon,
> > > > > > > >
> > > > > > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > hi Simon,
> > > > > > > > >
> > > > > > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Sughosh,
> > > > > > > > > >
> > > > > > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > hi Simon,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > hi Simon,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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
> [..]

Hi Sughosh,

On Wed, 28 Jun 2023 at 13:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 28 Jun 2023 at 15:49, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Wed, 28 Jun 2023 at 11:00, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Wed, 28 Jun 2023 at 13:12, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 27 Jun 2023 at 18:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Tue, 27 Jun 2023 at 17:51, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Tue, 27 Jun 2023 at 13:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Tue, 27 Jun 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Tue, 27 Jun 2023 at 05:57, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > hi Simon,
> > > > > > > > >
> > > > > > > > > On Mon, 26 Jun 2023 at 17:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > hi Simon,
> > > > > > > > > >
> > > > > > > > > > On Mon, 26 Jun 2023 at 14:38, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > >
> > > > > > > > > > > On Wed, 21 Jun 2023 at 05:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > hi Simon,
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, 19 Jun 2023 at 18:07, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, 15 Jun 2023 at 17:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > hi Simon,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, 15 Jun 2023 at 14:44, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Tue, 13 Jun 2023 at 11:39, Sughosh Ganu <sughosh.ganu@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@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.

Yes that sounds right, thanks.

Regards,
Simon