[PATCH v7 08/11] binman: capsule: Add support for generating EFI capsules

[Sughosh Ganu]

hi Simon,

On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sat, 5 Aug 2023 at 22:50, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sat, 5 Aug 2023 at 09:03, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > test cases in binman for testing capsule generation.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > ---
> > > > > Changes since V6:
> > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > >   signing.
> > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > * Use local vars for input payload and capsule filenames in
> > > > >   BuildSectionData().
> > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > >   functions suffice.
> > > > > * Use GUID macro names in the capsule test dts files.
> > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > >
> > > > >
> > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > >
> > > > Please move this file to a later patch - see below.
> > > >
> > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > > >
> > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > >  13 files changed, 546 insertions(+)
> > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > >
> > >
> > > [..]
> > >
> > > > > diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> > > > > new file mode 100644
> > > > > index 0000000000..86552ff83f
> > > > > --- /dev/null
> > > > > +++ b/tools/binman/test/307_capsule.dts
> > > > > @@ -0,0 +1,23 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +
> > > > > +/dts-v1/;
> > > > > +
> > > > > +#include <sandbox_efi_capsule.h>
> > > >
> > > > We can't include sandbox files in binman! I Does it actually matter
> > > > what the GUID value is? Can they all be the same? For now I think it
> > > > is best to have
> > > >
> > > > #define BINMAN_TEST_GUID "xxx"
> > > >
> > > > repeated at the top of each .dts file. Hopefully at some point we can
> > > > figure out a sensible way to provide a decoder ring for this mess, so
> > > > we can use actually names in the binman definition instead of GUIDs.
> > >
> > > Oh, having thought about this a bit more, there is a much better way.
> > >
> > > In the entry type, allow the image_guid to be a string, like
> > > 'binman-test' or 'sandbox-test'. Then binman can have a conversion
> > > function to figure out the GUID to pass to the tool, e.g. in
> > > efi_capsule.py:
> > >
> > > TYPE_TO_GUID = {
> > >    'binman-test': '09123987987f98712',
> > >    'sandbox-test':, '98123987123123987123987',
> > > }
> > >
> > > def get_guid(type_str):
> > >     'Get the GUID to use for a particular purpose"""
> > >     return TYPE_TO_GUID.get(type_str, type_str)
> > >
> > > Then you can use these same strings in the sandbox tests, without
> > > needing to include anything.
> >
> > While we can use this method for both sandbox and binman tests, I
> > would prefer using the actual GUID strings for the sandbox tests. This
> > will serve as an illustrative example to the user for how to pass the
> > image GUID value for generating the capsule. For the binman tests, I
> > can use this logic to avoid including the header file.
>
> So long as it is easy, that's OK. But how does the user know which GUID to use?

I think you got me wrong here. What I meant was that passing the GUID
value in the capsule entry like how it is currently done in the
u-boot.dtsi file would give the user an idea of how to specify the
GUID value for an image. If we use the string that you suggest above
for both binman tests as well as sandbox, the user might get confused
as to how the GUID value is to be passed in the normal scenario. I
want the sandbox capsule entries to also be kind of illustrative of
how a capsule entry would look like.

-sughosh





>
> Regards,
> Simon