[PATCH v4 06/12] binman: capsule: Add support for generating capsules

Tue Aug 1 15:06:02 CEST 2023

Hi Sughosh,

On Tue, 1 Aug 2023 at 06:29, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 26 Jul 2023 at 04:06, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 20 Jul 2023 at 03:20, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Thu, 20 Jul 2023 at 00:41, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Wed, 19 Jul 2023 at 02:42, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Wed, 19 Jul 2023 at 06:41, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Mon, 17 Jul 2023 at 04:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Sun, 16 Jul 2023 at 05:12, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Add support in binman for generating capsules. The capsule parameters
> > > > > > > > > can be specified either through a config file or through the capsule
> > > > > > > > > binman entry. Also add test cases in binman for capsule generation,
> > > > > > > > > and enable this testing on the sandbox_spl variant.
> > > > > > > >
> > > > > > > > Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for
> > > > > > > > SPL testing.
> > > > > > >
> > > > > > > Er, I am actually using the sandbox_spl variant.
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > ---
> > > > > > > > > Changes since V3:
> > > > > > > > > * Add test cases for covering the various capsule generation
> > > > > > > > >   scenarios.
> > > > > > > > > * Add function comments in the mkeficapsule bintool.
> > > > > > > > > * Fix the fetch method of the mkeficapsule bintool to enable building
> > > > > > > > >   the tool.
> > > > > > > > > * Add more details about the capsule parameters in the documentation
> > > > > > > > >   as well as the code.
> > > > > > > > > * Fix order of module imports, and addition of blank lines in the
> > > > > > > > >   capsule.py file.
> > > > > > > > > * Use SetContents in the ObtainContents method.
> > > > > > > > >
> > > > > > > > >  configs/sandbox_spl_defconfig                 |   1 +
> > > > > > > > >  tools/binman/btool/mkeficapsule.py            | 158 ++++++++++++++++++
> > > > > > > > >  tools/binman/entries.rst                      |  37 ++++
> > > > > > > > >  tools/binman/etype/capsule.py                 | 132 +++++++++++++++
> > > > > > > > >  tools/binman/ftest.py                         | 127 ++++++++++++++
> > > > > > > > >  tools/binman/test/282_capsule.dts             |  18 ++
> > > > > > > > >  tools/binman/test/283_capsule_signed.dts      |  20 +++
> > > > > > > > >  tools/binman/test/284_capsule_conf.dts        |  14 ++
> > > > > > > > >  tools/binman/test/285_capsule_missing_key.dts |  19 +++
> > > > > > > > >  .../binman/test/286_capsule_missing_index.dts |  17 ++
> > > > > > > > >  .../binman/test/287_capsule_missing_guid.dts  |  17 ++
> > > > > > > > >  .../test/288_capsule_missing_payload.dts      |  17 ++
> > > > > > > > >  tools/binman/test/289_capsule_missing.dts     |  17 ++
> > > > > > > > >  tools/binman/test/290_capsule_version.dts     |  19 +++
> > > > > > > > >  tools/binman/test/capsule_cfg.txt             |   6 +
> > > > > > > > >  15 files changed, 619 insertions(+)
> > > > > > > > >  create mode 100644 tools/binman/btool/mkeficapsule.py
> > > > > > > > >  create mode 100644 tools/binman/etype/capsule.py
> > > > > > > > >  create mode 100644 tools/binman/test/282_capsule.dts
> > > > > > > > >  create mode 100644 tools/binman/test/283_capsule_signed.dts
> > > > > > > > >  create mode 100644 tools/binman/test/284_capsule_conf.dts
> > > > > > > > >  create mode 100644 tools/binman/test/285_capsule_missing_key.dts
> > > > > > > > >  create mode 100644 tools/binman/test/286_capsule_missing_index.dts
> > > > > > > > >  create mode 100644 tools/binman/test/287_capsule_missing_guid.dts
> > > > > > > > >  create mode 100644 tools/binman/test/288_capsule_missing_payload.dts
> > > > > > > > >  create mode 100644 tools/binman/test/289_capsule_missing.dts
> > > > > > > > >  create mode 100644 tools/binman/test/290_capsule_version.dts
> > > > > > > > >  create mode 100644 tools/binman/test/capsule_cfg.txt
> > > > > > > >
> > > > > > > > This looks pretty good to me. Some nits below
> > > > > > > >
> > > > > > > > >
> > > > > > > > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
> > > > > > > > > index dd848c57c6..2fcc789347 100644
> > > > > > > > > --- a/configs/sandbox_spl_defconfig
> > > > > > > > > +++ b/configs/sandbox_spl_defconfig
> > > > > > > > > @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y
> > > > > > > > >  CONFIG_SPL_UNIT_TEST=y
> > > > > > > > >  CONFIG_UT_TIME=y
> > > > > > > > >  CONFIG_UT_DM=y
> > > > > > > > > +CONFIG_TOOLS_MKEFICAPSULE=y
> > > > > > > >
> > > > > > > > Why enabling this here? I don't think it is needed in sandbox_spl, but
> > > > > > > > in any case it should be in a different patch if needed.
> > > > > > >
> > > > > > > The binman tests run on the sandbox_spl variant. When running the
> > > > > > > capsule generation tests, the mkeficapsule tool should be present on
> > > > > > > the board variant no?
> > > > > >
> > > > > > Can we run this on the 'sandbox' variant instead?
> > > > >
> > > > > The capsule tests can be run on sandbox. But the change in the
> > > > > sandbox_spl_defconfig is for adding support for the capsule tests
> > > > > which are run as part of the binman test suite in CI. So it would be a
> > > >
> > > > The binman tests are run separately, in 'Run binman, buildman, dtoc,
> > > > Kconfig and patman testsuites' in gitlab.
> > >
> > > Yes, and that uses the sandbox_spl variant build for running the
> > > tests. Which is why I added building the mkeficapsule tool for the
> > > variant.
> >
> > Not really. It uses sandbox_spl to obtain the pylibfdt library, that's all.
> >
> > The tests should use sandbox. By enabling CONFIG_BINMAN you will get
> > pylibfdt anyway.
>
> I am not sure if I am missing something. But if I do not build the
> mkeficapsule tool as part of the sandbox_spl variant, I get errors in
> CI on the binman tests. And I believe that is because binman is being
> run with the '--toolpath /tmp/sandbox_spl/tools', which is built for
> the sandbox_spl variant. So if the mkeficapsule tool is not built for
> the sandbox_spl variant, this results in the binman capsule tests to
> fail in CI. I see this at least with the azure pipeline.

Yes, alll sandbox variants should build all tools, I believe. The
mkeficapsule tool should really be built by all boards, right? It also
needs to appear in the u-boot-tools debian package too, for example.

[..]

Regards,
Simon