Hi Lukas,
On Fri, 30 Jun 2023 at 13:28, Lukas Funke
<lukas.funke-...@weidmueller.com> wrote:
>
> On 30.06.2023 06:18, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 29 Jun 2023 at 15:59, <lukas.funke-...@weidmueller.com> wrote:
> >>
> >> From: Lukas Funke <lukas.fu...@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.fu...@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.fu...@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
