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

[Neha Malcom Francis <n-francis@ti.com>]

On 19/04/22 01:26, Alper Nebi Yasak wrote:
> 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@ti.com>
>> [n-francis@ti.com: added tests for addition of etype]
>> Signed-off-by: Neha Malcom Francis <n-francis@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.

Having an generalized etype for x509 is a good addition that would help 
when we scale this to the other K3 devices as well, and possibly other 
devices that use it. My only concern was whether it would drive away 
from the purpose of this patch, but as you put it, it may be better to 
do so. I will try seeing if converting the script into an etype is possible.

>
> ... 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";
>> +		};
>> +	};
>> +};

-- 
Thanking You
Neha Malcom Francis