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

[Lukas Funke]

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?

> 
>> +        """
>> +
>> +        _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

Regards,
Lukas