[PATCH 1/4] tools: mkeficapsule: Add support for parsing capsule params from config file

[Simon Glass]

Hi Sughosh,

On Fri, Dec 29, 2023 at 6:53 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 27 Dec 2023 at 23:19, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, Dec 4, 2023 at 7:15 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 2 Dec 2023 at 00:02, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Thu, 30 Nov 2023 at 23:39, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Thu, 30 Nov 2023 at 08:16, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 22 Nov 2023 at 00:40, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > hi Ilias,
> > > > > > >
> > > > > > > On Wed, 22 Nov 2023 at 13:06, Ilias Apalodimas
> > > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > On Wed, 22 Nov 2023 at 07:23, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > hi Simon,
> > > > > > > > >
> > > > > > > > > On Wed, 22 Nov 2023 at 03:42, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Sughosh,
> > > > > > > > > >
> > > > > > > > > > On Tue, 21 Nov 2023 at 00:02, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Add support for specifying the parameters needed for capsule
> > > > > > > > > > > generation through a config file, instead of passing them through
> > > > > > > > > > > command-line. Parameters for more than a single capsule file can be
> > > > > > > > > > > specified, resulting in generation of multiple capsules through a
> > > > > > > > > > > single invocation of the command.
> > > > > > > > > > >
> > > > > > > > > > > The config file can then be passed to the mkeficapsule tool in such
> > > > > > > > > > > manner
> > > > > > > > > > >
> > > > > > > > > > >  $ ./tools/mkeficapsule -f <path/to/the/config/file>
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  tools/Kconfig              |  15 ++
> > > > > > > > > > >  tools/Makefile             |   1 +
> > > > > > > > > > >  tools/eficapsule.h         | 114 ++++++++++++
> > > > > > > > > > >  tools/mkeficapsule.c       |  87 +++++----
> > > > > > > > > > >  tools/mkeficapsule_parse.c | 352 +++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  5 files changed, 538 insertions(+), 31 deletions(-)
> > > > > > > > > > >  create mode 100644 tools/mkeficapsule_parse.c
> > > > > > > > > >
> > > > > > > > > > This patch keeps coming back :-)
> > > > > > > > > >
> > > > > > > > > > Can we not add multiple capsules in the binman description? Why do we
> > > > > > > > > > need a new file format? How can binman decode images produced in this
> > > > > > > > > > way?
> > > > > > > > >
> > > > > > > > > So as Tom mentions, this brings parity with respect to the other
> > > > > > > > > capsule generation tool in EDKII that generates capsules. IIRC, this
> > > > > > > > > is something which even Xilix was interested in, and Michal had kind
> > > > > > > > > of gone through these patches earlier. Lastly, it would be good to
> > > > > > > > > have support in U-Boot's mkeficapsule tool for generating a single
> > > > > > > > > capsule file with multiple payloads, and having support for this
> > > > > > > > > functionality helps in that goal.
> > > > > > > > >
> > > > > > > > > Also, you might have noticed that, since your objection to the last
> > > > > > > > > series, I have removed putting this in binman. So now, this aspect of
> > > > > > > > > the capsule generation would only be supported through the
> > > > > > > > > command-line invocation of the tool.
> > > > > > > >
> > > > > > > > I think that overall the approach is sane. mkeficapsule is currently
> > > > > > > > supported and compiled for distros, so the multiple payload support is
> > > > > > > > useful. If we want to add support to binman, instead of rewriting this
> > > > > > > > in python, we could just call that tool for parsing and creating
> > > > > > > > capsules
> > > > > > >
> > > > > > > Given the amount of time these patches have been under review(also
> > > > > > > number of iterations), I would request that this series be reviewed
> > > > > > > and merged first. I think there is general consensus that there is
> > > > > > > value to have this functionality in the mkeficapsule tool. If it is
> > > > > > > deemed fit to support this through binman as well, that task can be
> > > > > > > taken up separately. Thanks.
> > > > > >
> > > > > > The point you are missing is that it is the entire goal of 'skirting
> > > > > > around' binman which is suspect.
> > > > > >
> > > > > > If there is a need to generate an output file from the build, we
> > > > > > should support it in binman. If people start creating configuration
> > > > > > files all over the place, then they are not using binman, right?
> > > > > >
> > > > > > I understand that there are pre-existing vendor-specific config files,
> > > > > > etc. that the EFI thing is a grey area, but I cannot imagine that this
> > > > > > patch would lead to a good outcome.
> > > > > >
> > > > > > The goal of binman is to bring order to the chaos of firmware
> > > > > > packaging...we cannot do that if it is not actually used.
> > > > > >
> > > > > > So let's figure out what is missing from binman's capsule generation
> > > > > > (multiple capsules? accept/reject capsules?) and how best to add it.
> > > > >
> > > > > I think I need to jog your memory back a bit. For context, I have
> > > > > jotted down the points.
> > > > >
> > > > > * The mkeficapsule tool generates capsules in U-Boot.
> > > > > * Currently, when the tool is invoked from the command-line, the
> > > > > capsules are generated by passing the capsule parameters as cmd-line
> > > > > options.
> > > > > * I had earlier added support for generating the capsules as part of
> > > > > U-Boot build, through binman. This support has been merged.
> > > > > * I had followed these patches up with another series [1] which
> > > > > generates capsules by parsing the capsule parameters through a config
> > > > > file instead of cmd-line options.
> > > > > * This series also had patches which were attempting to integrate this
> > > > > functionality into binman [2].
> > > > > * As part of reviewing the patch series, you had objected to adding
> > > > > this support in binman, primarily because this way of specifying the
> > > > > capsule parameters goes against the normal way of image description in
> > > > > binman [3].
> > > > > * I have described in this mail thread about why we need to have
> > > > > support for generating capsules through config files [4].
> > > >
> > > > Thanks for that. Yes this fits with my memory, such as it is.
> > > >
> > > > I regard the tool as a binman tool, though, in the sense that binman
> > > > is responsible for generating the images. For example, binman calls
> > > > mkimage to handle FIT images. People are free to write a script to
> > > > call mkimage manually, but the in-tree use of mkimage is supposed to
> > > > be with binman.
> > >
> > > No issues with supporting the functionality through binman. However, I
> > > think that if binman is to be used as the encompassing packaging tool
> > > which in turn calls other tools like mkimage or mkeficapsule, it
> > > should make allowances for accepting formats, or ways of describing
> > > payloads that might not sit with it's core design of "each image is a
> > > DT node".
> > >
> > > >
> > > > >
> > > > > So, in essence, this functionality is needed to be added to the tool.
> > > > > I have earlier tried integrating this in binman, but that was
> > > > > rejected. So, the way I see this moving ahead is to first add support
> > > > > for this feature in the tool, and then see if this can also be added
> > > > > in binman. As I mentioned earlier, I am fine with this functionality
> > > > > not getting integrated with binman, if this contravenes the idea of
> > > > > describing images in binman, and if no exceptions can be made on that
> > > > > regard. But please do understand that no attempt is being made at
> > > > > 'skirting around binman'. In fact, I had worked on your earlier
> > > > > objection of using absolute paths in testing this functionality in
> > > > > binman. But then you had put the other objection of how this does not
> > > > > follow the idea or concept of image description in binman. Hence this
> > > > > approach.
> > > >
> > > > It would be easier to talk this through f2f, but I will make an attempt here.
> > > >
> > > > The goal here is for people to be able to create an EFI capsule (or
> > > > more than one?) automatically as part of the build.
> > >
> > > That is one of the goals, yes. But the other goal is also to add
> > > functionality to the base capsule generation tool which extends the
> > > tool to support generating capsules through the config file. I would
> > > be more than happy if this can be plumbed in as part of the build.
> > >
> > >
> > > >  Since the config
> > > > is in the binman description, putting it somewhere else doesn't help
> > > > anyone. It will lead to .cfg files in U-Boot and Binman will just be a
> > > > side show. That is my concern.
> > >
> > > Okay. But I don't think that is the main issue here. For example, we
> > > can require the .cfg files to be placed in the board dir, similar to
> > > how the capsule keys are placed. The issue here is that can binman
> > > make allowances(IMO it should) to have the capsule payloads be
> > > specified through a .cfg file parameter, instead of the usual way of
> > > specifying it as a DT node. I think if this allowance can be made, it
> > > solves all the issues here.
> > >
> > > >
> > > > I would really like to see a capsule generated for sandbox so I can
> > > > understand this better.
> > >
> > > I will work on the capsule generation for sandbox. I will generate
> > > signed capsules, as you suggested in this mail thread as part of the
> > > sandbox U-Boot build. However, this will be using the currently merged
> > > capsule generation logic, where all the capsule parameters, it's
> > > payload included, are specified in binman nodes. I suspect this won't
> > > give you a picture of capsule generation through config files.
> >
> > Is there is progress on this? I would still really like to see a
> > single capsule generated by the sandbox build.
>
> I will come up with a patch to move the generation of signed capsules
> for sandbox as part of the build. We can take the discussion forward
> once this patch gets in. But I would really love to see progress on
> this series, without or without binman support. Thanks.

Just to be clear, this is independent of the tests you have written,
which are fine as is. I am just looking for a single capsule to be
built.

Re this series, I am still unsure whether this will result in .cfg
files appearing in U-Boot, or what this will actually be used for. I
am hoping that with a sandbox build to play around with, it will
become clearer. That said, if people want to add new features to
U-Boot, we normally take them.

Regards,
Simon


>
> -sughosh
>
> >
> > >
> > > >
> > > > Anyway, I hope that makes sense? If not, I think we should discuss it
> > > > f2f or VC or somehow.
> > >
> > > Sure. I will be happy to have a VC anytime if you think that would
> > > help. Let me know and we can sync up over IRC. Thanks.
> >
> > Perhaps we can do this in the new year. I think it would help to do a
> > patch for the above first, though, since it might answer many of my
> > questions.
> >
> > Regards,
> > Simon