From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0A8C2C433F5 for ; Tue, 19 Apr 2022 02:49:55 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 87480839B2; Tue, 19 Apr 2022 04:49:53 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=ti.com header.i=@ti.com header.b="Hzi8/mhp"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 37C4383A9F; Tue, 19 Apr 2022 04:49:52 +0200 (CEST) Received: from lelv0143.ext.ti.com (lelv0143.ext.ti.com [198.47.23.248]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 3EC8980084 for ; Tue, 19 Apr 2022 04:49:48 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=n-francis@ti.com Received: from fllv0035.itg.ti.com ([10.64.41.0]) by lelv0143.ext.ti.com (8.15.2/8.15.2) with ESMTP id 23J2naWg042652; Mon, 18 Apr 2022 21:49:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1650336576; bh=+HSDnRLGCtArLhUZ/w8vthmELp97u/htkH0trG8ykKc=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=Hzi8/mhpimIsRx8QqUPFANNap9WUqY9NPYz+Hq+FNYBCxRg/ojrFoWMAzHyvPxSvL Id+PfsmMCsPbIeDdF3Kt5CMoj80/E4xjr4USFAKOuQLVZfjAz98W82gjuz4GigD5rn zrcpDyN51groyqTNnX3D3EpmTkZ7n3x2MOEMJ+js= Received: from DLEE115.ent.ti.com (dlee115.ent.ti.com [157.170.170.26]) by fllv0035.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 23J2na5O012096 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 18 Apr 2022 21:49:36 -0500 Received: from DLEE105.ent.ti.com (157.170.170.35) by DLEE115.ent.ti.com (157.170.170.26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.14; Mon, 18 Apr 2022 21:49:34 -0500 Received: from lelv0326.itg.ti.com (10.180.67.84) by DLEE105.ent.ti.com (157.170.170.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.14 via Frontend Transport; Mon, 18 Apr 2022 21:49:34 -0500 Received: from [10.250.235.96] (ileax41-snat.itg.ti.com [10.172.224.153]) by lelv0326.itg.ti.com (8.15.2/8.15.2) with ESMTP id 23J2nSXH017177; Mon, 18 Apr 2022 21:49:29 -0500 Message-ID: <184533a7-fddf-45f6-f013-386c4f1d45aa@ti.com> Date: Tue, 19 Apr 2022 08:19:28 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [RESEND, RFC 2/8] binman: etype: sysfw: Add entry type for sysfw Content-Language: en-US To: Alper Nebi Yasak CC: , , , , , , , , , , , , References: <20220406122919.6104-1-n-francis@ti.com> <20220406122919.6104-3-n-francis@ti.com> From: Neha Malcom Francis In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean 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 >> [n-francis@ti.com: added tests for addition of etype] >> Signed-off-by: Neha Malcom Francis >> --- >> 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