[PATCH v4 04/11] tools: add fdtsig.sh

[Simon Glass]

Hi Takahiro,

On Tue, 26 Oct 2021 at 00:00, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> On Mon, Oct 25, 2021 at 12:06:39PM +0900, AKASHI Takahiro wrote:
> > Simon,
> >
> > On Thu, Oct 14, 2021 at 06:40:24PM -0600, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Mon, 11 Oct 2021 at 19:42, AKASHI Takahiro
> > > <takahiro.akashi at linaro.org> wrote:
> > > >
> > > > Simon,
> > > >
> > > > On Mon, Oct 11, 2021 at 08:54:09AM -0600, Simon Glass wrote:
> > > > > Hi Takahiro,
> > > > >
> > > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi at linaro.org> wrote:
> > > > > >
> > > > > > With this script, a public key is added to a device tree blob
> > > > > > as the default efi_get_public_key_data() expects.
> > > > > >
> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > > > > ---
> > > > > >  MAINTAINERS     |  1 +
> > > > > >  tools/fdtsig.sh | 40 ++++++++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 41 insertions(+)
> > > > > >  create mode 100755 tools/fdtsig.sh
> > > > >
> > > > > Instead of an ad-hoc script with no tests,
> > > >
> > > > Basically I intended to provide fdtsig.sh as a *sample* script so that
> > > > people may want to integrate the logic into their own build rule/systems.
> > > > But I could use this script in my 'capsule authentication' test
> > > > that is also added in patch#22.
> > > >
> > > > > could we use binman for
> > > > > putting the image together and inserting it?
> > > >
> > > > First, as you can see, the script is quite simple and secondly,
> > > > the purpose of binman, IIUC, is to help handle/manipulate U-Boot
> > > > image binaries.
> > > > So I'm not sure whether it is really useful to add such a feature to binman.
> > >
> > > I'm not sure. The script seems very ad-hoc to me, for a feature that
> > > Linaro is pushing so hard.
> >
> > To be honest, I've never used binman :) So I'm not sure whether binman
> > is the best place to add this feature. For example, README under tools/binman
> > says, "It seems better to use the mkimage tool to generate binaries and avoid
> > blurring the boundaries between building input files (mkimage) and packaging
> > then into a final image (binman)."
> > Obviously, dtb is not the final image.
> >
> > > I don't see where the script is used in the tests or even mentioned in
> > > the documentation. Am I missing something?
> >
> > Due to the history of submissions of this series, the current pytest
> > scenario doesn't use the script, but you can see the exact same
> > sequence of commands at test/py/tests/test_efi_capsule/conftest.py:
> > ---8<---
> >         # Update dtb adding capsule certificate
> >         check_call('cd %s; cp %s/test/py/tests/test_efi_capsule/signature.dts .'
> >                    % (data_dir, u_boot_config.source_dir), shell=True)
> >         check_call('cd %s; dtc -@ -I dts -O dtb -o signature.dtbo signature.dts; fdtoverlay -i %s/arch/sandbox/dts/test.dtb -o test_sig.dtb signature.dtbo'
> >                    % (data_dir, u_boot_config.build_dir), shell=True)
> > --->8---
> > (Please see my patch#11.)
> >
> > What I meant is that we can directly use fdtsig.sh here if your concern
> > is that the script is *not exercised* anywhere.
>
> Besides binman or fdtsig.sh, I found that the crucial information was
> missing here; the format or how a public key is encoded in a device tree.
> With an example of command sequence, we may drop this patch.
>
> So will adding some description to uefi.rst satisfy your needs,

Seems OK to me, and some sort of CI test.

> or do you expect an extra rule for embedding a key to be added
> in some Makefile?
> (I don't think this step is part of build process, though.)

I agree, this is best done in the packaging step, after all the bits
have been built.

Binman is designed to run inside the U-Boot build and also separately,
to collect everything and redo whatever happened in the U-Boot build.

REgards,
Simon