[RESEND, RFC 2/8] binman: etype: sysfw: Add entry type for sysfw

[Alper Nebi Yasak]

On 06/04/2022 15:29, Neha Malcom Francis wrote:
> For K3 devices that require a sysfw image, add entry for SYSFW. It can
> contain system firmware image that can be packaged into sysfw.itb by
> binman. The method ReadBlobContents in sysfw.py runs the TI K3
> certificate generation script to create the signed sysfw image that can
> be used for packaging by binman into sysfw.bin.
> 
> Signed-off-by: Tarun Sahu <t-sahu at ti.com>
> [n-francis at ti.com: added tests for addition of etype]
> Signed-off-by: Neha Malcom Francis <n-francis at ti.com>
> ---
>  tools/binman/entries.rst        | 11 ++++++
>  tools/binman/etype/sysfw.py     | 60 +++++++++++++++++++++++++++++++++
>  tools/binman/ftest.py           |  7 ++++
>  tools/binman/test/226_sysfw.dts | 13 +++++++
>  4 files changed, 91 insertions(+)
>  create mode 100644 tools/binman/etype/sysfw.py
>  create mode 100644 tools/binman/test/226_sysfw.dts
> 
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index 484cde5c80..7c95bbfbec 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -1019,6 +1019,17 @@ This entry holds firmware for an external platform-specific coprocessor.
>  
>  
>  
> +Entry: sysfw: Texas Instruments System Firmware (SYSFW) blob
> +------------------------------------------------------------

This title should match the first line of the docstring, normally this
entire file is regenerated from those by running `binman entry-docs`.

I like this title better than the one in docstring though, because of
the explicit TI mention. 'System Firmware' and 'sysfw' sound awfully
generic to me, consider renaming the entry to ti-sysfw (like ti-dm) or
even ti-k3-sysfw (and ti-k3-dm) if these are K3-specific.

> +
> +Properties / Entry arguments:
> +    - sysfw-path: Filename of file to read into the entry, typically sysfw.bin
> +
> +This entry contains system firmware necessary for booting of K3 architecture
> +devices.
> +
> +
> +
>  Entry: section: Entry that contains other entries
>  -------------------------------------------------
>  
> diff --git a/tools/binman/etype/sysfw.py b/tools/binman/etype/sysfw.py
> new file mode 100644
> index 0000000000..c73300400b
> --- /dev/null
> +++ b/tools/binman/etype/sysfw.py
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
> +#
> +# Entry type module for TI SYSFW binary blob
> +#
> +
> +import struct
> +import zlib
> +import os
> +import sys

Alphabetical order here would be nicer.

> +
> +from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
> +from dtoc import fdt_util
> +from patman import tools
> +
> +
> +class Entry_sysfw(Entry_blob_named_by_arg):
> +    """Entry containing System Firmware (SYSFW) blob
> +
> +    Properties / Entry arguments:
> +        - sysfw-path: Filename of file to read into the entry, typically sysfw.bin
> +
> +This entry contains system firmware necessary for booting of K3 architecture devices.

Should be indented same as the triple-quote marks. Also, the node
properties this uses should be documented (load, core?).

> +    """
> +
> +    def __init__(self, section, etype, node):
> +        super().__init__(section, etype, node, 'scp')

'scp' -> 'sysfw'

And I think you need to add '-a sysfw-path=...' to Makefile, like you do
for ti-dm.

> +        self.core = "0"

Is this useful enough to be read from dtb like the load address?

> +        self.missing_msg = "sysfw"
> +
> +    def ReadNode(self):
> +        self._load_addr = fdt_util.GetInt(self._node, 'load', 0)
> +        self._args = []

This self._args looks unused.

> +
> +    def _SignSysfw(self, out):
> +        """Sign the sysfw image and write it to the output directory"""
> +        # Try running the K3 x509 certificate signing script

I think this entry should just be the sysfw.bin data passed into binman.
You can add a second entry that can sign arbitrary content (this entry),
preferably by mostly converting that script to python. I imagine things
might look a bit like this in the binman description:

    ti-k3-x509-sign {
        key = "key.pem";
        load = <0x41c00000>;
        core = <0>;

        ti-sysfw {
        };
    };

The script doesn't seem too specialized to me (except the x509
certificate template details) so maybe it could be done as some
generalized x509-cert entry? I'm guessing something vblock-ish:

    sysfw {
        type = "section";

        x509-cert {
            content = <&sysfw_bin>;
            key = "key.pem";
            config = "x509-config.txt";
            outform = "DER";
            digest = "sha512";
        };

        sysfw_bin: ti-sysfw {
        };
    };

But I don't know the x509 details, never looked into it.

... or you can require the sysfw.bin to be pre-signed with that script
before passing it to binman (wouldn't be my favourite solution.)

> +        try:
> +            args = [
> +                '-c', "0",
> +                '-b', self._filename,
> +                '-l', str(self._load_addr),
> +                '-o', out
> +            ]
> +            k3_cert_gen_path = os.environ['srctree'] + \
> +                "/tools/k3_gen_x509_cert.sh"
> +            tools.run(k3_cert_gen_path, *args)

You'd normally implement a 'bintool' for executables instead of
calling them like this. That also lets you mock it in the tests for
coverage.

> +            self.SetContents(tools.read_file(out))
> +            return True
> +        # If not available (example, in the case of binman tests, set entry contents as dummy binary)
> +        except KeyError:
> +            self.missing = True
> +            self.SetContents(b'sysfw')
> +            return True
> +
> +    def ObtainContents(self):

It might be better to override ReadBlobContents() instead of this. I
can't really tell. It could let you avoid handling 'missing' manually.

> +        self.missing = False
> +        out = tools.get_output_filename("sysfwint")

Usually a name prefixed with self.GetUniqueName() (and a dot) is used
for these intermediate files.

> +        self._SignSysfw(out)
> +        return True
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 8f00db6945..7c12058fe4 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -87,6 +87,7 @@ ATF_BL31_DATA         = b'bl31'
>  TEE_OS_DATA           = b'this is some tee OS data'
>  ATF_BL2U_DATA         = b'bl2u'
>  OPENSBI_DATA          = b'opensbi'
> +SYSFW_DATA            = b'sysfw'
>  SCP_DATA              = b'scp'
>  TEST_FDT1_DATA        = b'fdt1'
>  TEST_FDT2_DATA        = b'test-fdt2'
> @@ -192,6 +193,7 @@ class TestFunctional(unittest.TestCase):
>          TestFunctional._MakeInputFile('tee-pager.bin', TEE_OS_DATA)
>          TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
>          TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
> +        TestFunctional._MakeInputFile('sysfw.bin', SYSFW_DATA)
>          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>  
>          # Add a few .dtb files for testing
> @@ -5321,6 +5323,11 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          self.assertIn("Node '/binman/fit': Unknown operation 'unknown'",
>                        str(exc.exception))
>  
> +    def testPackSysfw(self):
> +        """Test that an image with a SYSFW binary can be created"""
> +        data = self._DoReadFile('226_sysfw.dts')
> +        self.assertEqual(SYSFW_DATA, data[:len(SYSFW_DATA)])
> +
>  
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/226_sysfw.dts b/tools/binman/test/226_sysfw.dts
> new file mode 100644
> index 0000000000..23d64d3688
> --- /dev/null
> +++ b/tools/binman/test/226_sysfw.dts
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	binman {
> +		sysfw {
> +			filename = "sysfw.bin";
> +		};
> +	};
> +};