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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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@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@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@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,
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.)

-Takahiro Akashi

> -Takahiro Akashi
> 
> > Regards,
> > Simon