On 29.06.2023 17:08, Quentin Schulz wrote:

Hi Lukas,

On 6/29/23 16:59, lukas.funke-oss@weidmueller.com wrote:

...

[You don't often get email from lukas.funke-oss@weidmueller.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

From: Lukas Funke lukas.funke@weidmueller.com

Remove extra colon typo

Signed-off-by: Lukas Funke lukas.funke@weidmueller.com

tools/binman/etype/mkimage.py | 2 +-   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py index e028c44070..dd734fc779 100644 --- a/tools/binman/etype/mkimage.py +++ b/tools/binman/etype/mkimage.py @@ -24,7 +24,7 @@ class Entry_mkimage(Entry):           - filename: filename of output binary generated by mkimage

The data passed to mkimage via the -d flag is collected from subnodes of the -    mkimage node, e.g.:: +    mkimage node, e.g.:

This is by far not the only occurrence in the docstrings, see:

$ ag -c :: tools/binman/**/*.py tools/binman/btool/btool_gzip.py:2 tools/binman/btool/bzip2.py:2 tools/binman/btool/lz4.py:2 tools/binman/btool/lzma_alone.py:2 tools/binman/btool/lzop.py:2 tools/binman/btool/xz.py:2 tools/binman/btool/zstd.py:2 tools/binman/cbfs_util.py:1 tools/binman/etype/atf_fip.py:3 tools/binman/etype/cbfs.py:5 tools/binman/etype/fdtmap.py:1 tools/binman/etype/fit.py:7 tools/binman/etype/mkimage.py:7 tools/binman/etype/pre_load.py:1 tools/binman/etype/section.py:1 tools/binman/etype/tee_os.py:2 tools/binman/etype/text.py:3 tools/binman/ftest.py:15 tools/binman/setup.py:3 tools/binman/state.py:1

If I'm not mistaken, we (manually) populate the docstring from the docs written in rST where `::` does actually mean something (start of a literal block, c.f. https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lite...). I think it's fine to keep it as is (and maybe we could use the docstring directly from within sphinx instead of duplicating it, but I don't have experience with that so I don't know if it's possible, desirable and reasonably easy to implement and maintain).

Cheers, Quentin

Hi Quentin,

I wasn't aware of that, thanks for pointing it out. I'll drop the patch in the next iteration.

Best regards

Lukas

Hi,

On Thu, 29 Jun 2023 at 15:59, lukas.funke-oss@weidmueller.com wrote:

From: Lukas Funke lukas.funke@weidmueller.com

While signing a fit compressed data (i.e. 'blob-ext') is decompressed, but never compressed again. When compressed data was wrapped in a section, decompression leads to an error because the outer section had the original compressed size but the inner entry has the uncompressed size now.

While singing there is no reason to decompress data. Thus, decompression

signing ?

should be disabled.

Furthermore, bintools should be collected before loading the data. This way bintools are available if processing is required on a node.

Signed-off-by: Lukas Funke lukas.funke@weidmueller.com

tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/tools/binman/control.py b/tools/binman/control.py index 68597c4e77..affc33ff3d 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -306,8 +306,8 @@ def BeforeReplace(image, allow_resize): image: Image to prepare """ state.PrepareFromLoadedData(image)

• image.LoadData() image.CollectBintools()

• image.LoadData(decomp=False)

  # If repacking, drop the old offset/size values except for the original # ones, so we are only left with the constraints.

-- 2.30.2

On Thu, 29 Jun 2023 at 15:59, lukas.funke-oss@weidmueller.com wrote:

From: Lukas Funke lukas.funke@weidmueller.com

Add test for u_boot_spl_pubkey_dtb. The test adds a public key to the dtb and checks if the required nodes will be added to the images dtb.

Signed-off-by: Lukas Funke lukas.funke@weidmueller.com

tools/binman/ftest.py | 32 ++++++++++++++++++++++++ tools/binman/test/281_spl_pubkey_dtb.dts | 16 ++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 tools/binman/test/281_spl_pubkey_dtb.dts

Reviewed-by: Simon Glass sjg@chromium.org

nit below

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 43b4f850a6..3bd09d3fea 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -638,6 +638,16 @@ class TestFunctional(unittest.TestCase): TestFunctional._MakeInputFile('vpl/u-boot-vpl', tools.read_file(cls.ElfTestFile(src_fname)))

• @classmethod
• def _SetupPmuFwlElf(cls, src_fname='bss_data'):

•    """Set up an ELF file with a '_dt_ucode_base_size' symbol
  

•    Args:
  

•        Filename of ELF file to use as VPL
  

•    """
  

•    TestFunctional._MakeInputFile('pmu-firmware.elf',
  

•        tools.read_file(cls.ElfTestFile(src_fname)))
  

• @classmethod def _SetupDescriptor(cls): with open(cls.TestFile('descriptor.bin'), 'rb') as fd:

@@ -6677,5 +6687,27 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception))

• def testSplPubkeyDtb(self):

•     """Test u_boot_spl_pubkey_dtb etype"""
  

•     data = tools.read_file(self.TestFile("key.pem"))
  

•     self._MakeInputFile("key.crt", data)
  

•     self._DoReadFileRealDtb('281_spl_pubkey_dtb.dts')
  

•     image = control.images['image']
  

•     entries = image.GetEntries()
  

•     dtb_entry = entries['u_boot_spl_pubkey_dtb']
  

•     dtb_data = dtb_entry.GetData()
  

•     dtb = fdt.Fdt.FromData(dtb_data)
  

•     dtb.Scan()
  

•     signature_node = dtb.GetNode('/signature')
  

•     self.assertIsNotNone(signature_node)
  

•     key_node = signature_node.FindNode("key-key")
  

•     self.assertIsNotNone(key_node)
  

•     self.assertEqual(fdt_util.GetString(key_node, "required"),
  

•                      "conf")
  

•     self.assertEqual(fdt_util.GetString(key_node, "algo"),
  

•                      "sha384,rsa4096")
  

•     self.assertEqual(fdt_util.GetString(key_node, "key-name-hint"),
  

•                      "key")
  

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/281_spl_pubkey_dtb.dts b/tools/binman/test/281_spl_pubkey_dtb.dts new file mode 100644 index 0000000000..5a2952ed7d --- /dev/null +++ b/tools/binman/test/281_spl_pubkey_dtb.dts @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+/ {

•   #address-cells = <1>;
  

•   #size-cells = <1>;
  

•   binman {
  

•           u_boot_spl_pubkey_dtb {
  

please use - instead of _

•                   algo = "sha384,rsa4096";
  

•                   required = "conf";
  

•                   key-name = "key";
  

•           };
  

•   };
  

+};

2.30.2

On Thu, 29 Jun 2023 at 15:59, lukas.funke-oss@weidmueller.com wrote:

From: Lukas Funke lukas.funke@weidmueller.com

This adds a new etype 'u_boot_spl_pubkey_dtb'. The etype adds the public key from a certificate to the dtb. This creates a '/signature' node which is turn contains the fields which make up the public key. Usually this is done by 'mkimage -K'. However, 'binman sign' does not add the public key to the SPL. This is why the pubkey is added using this etype.

The etype calls the underlying 'fdt_add_pubkey' tool.

Signed-off-by: Lukas Funke lukas.funke@weidmueller.com

tools/binman/etype/u_boot_spl_pubkey_dtb.py | 105 ++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 tools/binman/etype/u_boot_spl_pubkey_dtb.py

Please can you use 'binman entry-docs >tools/binman/entries.rst' and add to patch?

diff --git a/tools/binman/etype/u_boot_spl_pubkey_dtb.py b/tools/binman/etype/u_boot_spl_pubkey_dtb.py new file mode 100644 index 0000000000..25aa817975 --- /dev/null +++ b/tools/binman/etype/u_boot_spl_pubkey_dtb.py @@ -0,0 +1,105 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2023 Weidmueller GmbH +# Written by Lukas Funke lukas.funke@weidmueller.com +# +# Entry-type module for 'u-boot-spl-pubkey.dtb' +#

+import tempfile +import os

+from binman.etype.blob_dtb import Entry_blob_dtb

+from dtoc import fdt_util

+from u_boot_pylib import tools

+# pylint: disable=C0103 +class Entry_u_boot_spl_pubkey_dtb(Entry_blob_dtb):

• """U-Boot SPL device tree including public key
• Properties / Entry arguments:

•    - key-name: Public key name without extension (e.g. .crt). Default is
  

•                determined by underlying bintool (fdt_add_pubkey),
  

•                usually 'key'
  

•    - algo: (Optional) Algorithm used for signing. Default is determined by
  

•            underlying bintool (fdt_add_pubkey), usually 'sha1,rsa2048'
  

•    - required: (Optional) If present this indicates that the key must be
  

•                verified for the image / configuration to be
  

•                considered valid
  

• The following example shows an image containing an SPL which
• is packed together with the dtb. Binman will add a signature
• node to the dtb:

•    image {
  

•    ...
  

•        spl {
  

•            filename = "spl.bin"
  

•            u_boot_spl_nodtb {
  

•            };
  

•            u_boot_spl_pubkey_dtb {
  

•                algo = "sha384,rsa4096";
  

•                required = "conf";
  

•                key-name = "dev";
  

•            };
  

•        };
  

•    ...
  

•    }
  

• """
• def __init__(self, section, etype, node):

•    # Put this here to allow entry-docs and help to work without libfdt
  

•    global state
  

•    from binman import state
  

•    super().__init__(section, etype, node)
  

•    self.required_props = ['key-name']
  

•    self.fdt_add_pubkey = None
  

•    self._algo = fdt_util.GetString(self._node, 'algo')
  

•    self._required = fdt_util.GetString(self._node, 'required')
  

•    self._keyname = fdt_util.GetString(self._node, 'key-name')
  

• def ObtainContents(self, fake_size=0):

•    """ Add public key which is pointed out by
  

Please check comment style. The first line should a summary, then a blank line, then more info

•        'key-name' to node 'signature' in the spl-dtb
  

•        This is equivalent to the '-K' option of 'mkimage'
  

•    Args:
  

•        fake_size (int): unused
  

•    """
  

•    # We don't pass fake_size and skip_entry upwards
  

•    # because this is currently not support by the blob type
  

supported

•    super().ObtainContents()
  

•    with tempfile.NamedTemporaryFile(prefix=os.path.basename(
  

•                                     self.GetFdtEtype()),
  

•                                     dir=tools.get_output_dir())\
  

•                                          as pubkey_tdb:
  

•        tools.write_file(pubkey_tdb.name, self.GetData())
  

•        keyname = tools.get_input_filename(self._keyname + ".crt")
  

•        self.fdt_add_pubkey.run(pubkey_tdb.name,
  

•                                os.path.dirname(keyname),
  

•                                self._keyname,
  

•                                self._required, self._algo)
  

•        dtb = tools.read_file(pubkey_tdb.name)
  

•        self.SetContents(dtb)
  

•        state.UpdateFdtContents(self.GetFdtEtype(), dtb)
  

•    return True
  

• # pylint: disable=R0201,C0116
• def GetDefaultFilename(self):

•    return 'spl/u-boot-spl-pubkey.dtb'
  

• # pylint: disable=R0201,C0116
• def GetFdtEtype(self):

•    return 'u-boot-spl-dtb'
  

• # pylint: disable=R0201,C0116
• def AddBintools(self, btools):

•    super().AddBintools(btools)
  

•    self.fdt_add_pubkey = self.AddBintool(btools, 'fdt_add_pubkey')
  

-- 2.30.2

Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon

Hi,

On Thu, 29 Jun 2023 at 15:59, lukas.funke-oss@weidmueller.com wrote:

From: Lukas Funke lukas.funke@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@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@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-boot...

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.

•    """
  

•    _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.

•        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

Hi Lukas,

On Fri, 30 Jun 2023 at 13:28, Lukas Funke lukas.funke-oss@weidmueller.com wrote:

On 30.06.2023 06:18, Simon Glass wrote:

...

Hi,

On Thu, 29 Jun 2023 at 15:59, lukas.funke-oss@weidmueller.com wrote:

...

From: Lukas Funke lukas.funke@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@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@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-boot...

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