[PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Tom,

On Thu, 24 Aug 2023 at 08:45, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Aug 24, 2023 at 08:41:21AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 24 Aug 2023 at 07:38, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Wed, Aug 23, 2023 at 09:02:53PM -0600, Simon Glass wrote:
> > >
> > > > In this early stage of using binman to produce output files, we are mostly
> > > > seeing people using common extensions such as '.bin' and '.rom'
> > > >
> > > > But unusual extensions appear in some places.
> > > >
> > > > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > > there is no consistency as to the extension used.
> > > >
> > > > This series adjusts binman to enforce just 4 extensions for output images:
> > > >
> > > >    .bin
> > > >    .rom
> > > >    .itb
> > > >    .img
> > > >
> > > > Other extensions will produce an error. With this rule observed, buildman
> > > > can keep the required files.
> > > >
> > > > Some patches are included to fix up some easy problems. But the following
> > > > boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
> > > > asking for help from the maintainers:
> > > >
> > > >    am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
> > > >    am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
> > > >    j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
> > > >    am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
> > > >    am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
> > > >    verdin-am62_a53
> > > >
> > > > It looks like the .pem files are listed as top-level images, e.g.:
> > > >
> > > >    &custmpk_pem {
> > > >       filename = "../../ti/keys/custMpk.pem";
> > > >    };
> > > >
> > > > but if the only objective is to pick up an existing file, it is better to
> > > > set BINMAN_INDIRS to include that directory. Also we should only have
> > > > simple leafnames in the 'filename' property, so the '../../ti/keys' is not
> > > > correct. It makes it harder for people to get the files from other places.
> > > > Making this easier is one of binman's goals.
> > > >
> > > > Most likely the custmpk_pem node can be removed and the .pem file can be
> > > > included directly in the place that needs it, e.g. by adjusting the
> > > > ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
> > > > reading the key file.
> > > >
> > > > For example, this:
> > > >
> > > >    custMpk {
> > > >       filename = "custMpk.pem";
> > > >       custmpk_pem: blob-ext {
> > > >          filename = "../keys/custMpk.pem";
> > > >       };
> > > >    };
> > > >
> > > > is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
> > > > so is equivalent to:
> > > >
> > > >    custMpk {
> > > >       type = "blob";
> > > >       filename = "custMpk.pem";
> > > >    }
> > > >
> > > > (note that blob-ext implies that the blob may be missing, so blob is a
> > > > better choice, since the key cannot be missing)
> > > >
> > > > The fact that the .pem files are at the top level means that they are
> > > > output images, which surely is not intended. They should be buried in the
> > > > image description, at a lower level, as part of something else.
> > > >
> > > > So please take a loke at the above and see if the binman descriptions can
> > > > be reworked slightly to follow these new rules.
> > > >
> > > >
> > > > Simon Glass (6):
> > > >   binman: Require image filenames to have certain extensions
> > > >   buildman: Keep all permitted output files
> > > >   buildman: Show progress when regenerating the board.cfg file
> > > >   buildman: Start the clock when the build starts
> > > >   kontron_sl28: Use u-boot-update.bin instead of u-boot.update
> > > >   stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32
> > > >
> > > >  .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
> > > >  arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
> > > >  doc/board/kontron/sl28.rst                     |  4 ++--
> > > >  doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
> > > >  include/configs/stm32mp15_dh_dhsom.h           |  2 +-
> > > >  tools/binman/binman.rst                        |  5 +++++
> > > >  tools/binman/etype/section.py                  |  3 +--
> > > >  tools/binman/ftest.py                          | 12 ++++++++++--
> > > >  tools/binman/image.py                          |  9 +++++++++
> > > >  tools/binman/test/022_image_name.dts           |  4 ++--
> > > >  tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
> > > >  tools/buildman/boards.py                       | 15 ++++++++++++---
> > > >  tools/buildman/builder.py                      |  3 ++-
> > > >  tools/buildman/builderthread.py                | 11 +++++++----
> > > >  tools/buildman/control.py                      |  3 ++-
> > > >  15 files changed, 81 insertions(+), 29 deletions(-)
> > > >  create mode 100644 tools/binman/test/311_bad_image_name.dts
> > >
> > > This doesn't seem to address the ones I pointed out on IRC as being the
> > > first order (to me anyhow) problem of needing the _unsigned files on the
> > > platforms you list above for PEM files.
> >
> > I think it should be xxx-unsigned.bin instead of xxx.bin_unsigned
>
> As I was saying on IRC, you need to make that change then, if we're
> going to go down this path, before v2023.10 is out.

Yes I can do that one...but I am stuck on the PEM files so need the
maintainer there to figure out what is up, as described in detail in
the cover letter.

>
> And to repeat what I said inside another part of the thread, the u-boot*
> glob is hiding other "arbitrary" extensions that we've had in-use for
> forever.

Yes, understood. Do you think that is a problem? I should improve the
docs for buildman -k too.

Regrads,
Simon