[PATCH 10/12] binman: btool: Add Xilinx Bootgen btool

[Simon Glass]

Hi Lukas,

On Fri, 30 Jun 2023 at 13:28, Lukas Funke
<lukas.funke-oss at weidmueller.com> wrote:
>
> On 30.06.2023 06:18, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 29 Jun 2023 at 15:59, <lukas.funke-oss at weidmueller.com> wrote:
> >>
> >> From: Lukas Funke <lukas.funke at weidmueller.com>
> >>
> >> Add the Xilinx Bootgen as bintool. Xilinx Bootgen is used to create
> >> bootable SPL (FSBL in Xilinx terms) images for Zynq/ZynqMP devices. The
> >> btool creates a signed version of the SPL.
> >>
> >> Signed-off-by: Lukas Funke <lukas.funke at weidmueller.com>
> >> ---
> >>
> >>   tools/binman/btool/bootgen.py | 82 +++++++++++++++++++++++++++++++++++
> >>   1 file changed, 82 insertions(+)
> >>   create mode 100644 tools/binman/btool/bootgen.py
> >>
> >> diff --git a/tools/binman/btool/bootgen.py b/tools/binman/btool/bootgen.py
> >> new file mode 100644
> >> index 0000000000..8bc727a54f
> >> --- /dev/null
> >> +++ b/tools/binman/btool/bootgen.py
> >> @@ -0,0 +1,82 @@
> >> +# SPDX-License-Identifier: GPL-2.0+
> >> +# Copyright (C) 2023 Weidmüller Interface GmbH & Co. KG
> >> +# Lukas Funke <lukas.funke at weidmueller.com>
> >> +#
> >> +"""Bintool implementation for bootgen
> >> +
> >> +bootgen allows creating bootable SPL for Zynq(MP)
> >> +
> >> +Documentation is available via::
> >> +https://www.xilinx.com/support/documents/sw_manuals/xilinx2022_1/ug1283-bootgen-user-guide.pdf
> >
> > But you need to create some info here. It is good to have that link,
> > but we also need some about of info about it here.
> >
> > Overall this whole patch needs more docs and detail.
> >
> >> +
> >> +Source code is available at:
> >> +
> >> +https://github.com/Xilinx/bootgen
> >> +
> >> +"""
> >> +import tempfile
> >> +
> >> +from binman import bintool
> >> +from u_boot_pylib import tools
> >> +
> >> +# pylint: disable=C0103
> >> +class Bintoolbootgen(bintool.Bintool):
> >> +    """Generate bootable fsbl image for zynq/zynqmp
> >> +
> >> +    This bintools supports running Xilinx "bootgen" in order
> >> +    to generate a bootable, authenticated image form an SPL.
> >> +
> >> +    """
> >> +    def __init__(self, name):
> >> +        super().__init__(name, 'Xilinx Bootgen',
> >> +                         version_regex=r'^\*\*\*\*\*\* Xilinx Bootgen',
> >> +                         version_args='')
> >> +
> >> +    # pylint: disable=R0913
> >> +    def sign(self, arch, spl_elf_fname, pmufw_elf_fname,
> >> +             psk_fname, ssk_fname, fsbl_config, auth_params, output_fname):
> >> +        """ Sign FSBL(SPL) elf file and bundle it with pmu firmware
> >> +            to a bootable image
> >> +
> >> +        Args:
> >> +            arch (str): Xilinx SoC architecture
> >
> > what options are valid?
> >
> >> +            spl_elf_fname (str): Filename of FSBL ELF file
> >
> > what is FSBL?
> >
> >> +            pmufw_elf_fname (str): Filename pmu firmware
> >
> > what is pmu?
> >
> >> +            psk_fname (str): Filename of the primary secret key
> >> +            ssk_fname (str): Filename of the secondary secret key
> >
> > why are there two keys? What format is used for these keys?
> >
> >> +            fsbl_config (str): FSBL config options
> >
> > what options are available? What are valid vaulues for this arg?
> >
> >> +            auth_params (str): Authentication parameter
> >
> > what are valid values?
> >
> >> +            output_fname (str): Filename where bootgen should write the result
> >
> > This should really be handled automatically, I think. See how the
> > mkimage etype creates an output filename.
>
> Simon, thanks for the review!
>
> What to you mean by automatically? The mkimage btool also has an
> 'output_fname' argument as well. And the output filename is passed down
> from the mkimage etype to the mkimage btool. Should the bootgen btool
> generate the output_fname by itself and pass it back to the caller?

Oh I think I was getting confused with the 'filename' property used by
the mkimage entry type. What you are doing here looks fine to me.

>
> >
> >> +        """
> >> +
> >> +        _fsbl_config = f"[fsbl_config] {fsbl_config}" if fsbl_config else ""
> >> +        _auth_params = f"[auth_params] {auth_params}" if auth_params else ""
> >> +
> >> +        bif_template = f"""u_boot_spl_aes_rsa: {{
> >> +            [pskfile] {psk_fname}
> >> +            [sskfile] {ssk_fname}
> >> +            {_fsbl_config}
> >> +            {_auth_params}
> >> +            [ bootloader,
> >> +              authentication = rsa,
> >> +              destination_cpu=a53-0] {spl_elf_fname}
> >> +            [pmufw_image] {pmufw_elf_fname}
> >> +        }}"""
> >> +        args = ["-arch", arch]
> >> +
> >> +        with tempfile.NamedTemporaryFile(suffix=".bif",
> >> +                                         dir=tools.get_output_dir()) as bif:
> >
> > Please use a deterministic name - see mkimage etype for an example.
>
> The .bif file is a temporary input file passed to 'bootgen'.
>
> My intention was to make sure that the file is deleted afterwards. Would
> you prefer that the intermediate files are kept and the output file is
> deterministically created with "uniq = self.GetUniqueName()" and
> "output_fname = tools.get_output_filename('foo.%s' % uniq)"?
>
> If so, I would also change the way the ELF file is created in order to
> keep the intermediate results.
>
> >
> >> +            tools.write_file(bif.name, bif_template, binary=False)
> >> +            args += ["-image", bif.name, '-w', '-o', output_fname]
> >> +            self.run_cmd(*args)
> >> +
> >> +    def fetch(self, method):
> >> +        """Fetch bootgen from git"""
> >> +        if method != bintool.FETCH_BUILD:
> >> +            return None
> >> +
> >> +        result = self.build_from_git(
> >> +            'https://github.com/Xilinx/bootgen',
> >> +            'all',
> >> +            'build/bootgen/bootgen')
> >> +        return result
> >> --
> >> 2.30.2
> >>
Regards,
Simon