All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alper Nebi Yasak <alpernebiyasak@gmail.com>
To: Neha Malcom Francis <n-francis@ti.com>
Cc: sjg@chromium.org, marek.behun@nic.cz, xypron.glpk@gmx.de,
	vigneshr@ti.com, a-govindraju@ti.com, kristo@kernel.org,
	s-anna@ti.com, kishon@ti.com, joel.peshkin@broadcom.com,
	patrick.delaunay@foss.st.com, mr.nuke.me@gmail.com, nm@ti.com,
	u-boot@lists.denx.de
Subject: Re: [RESEND, RFC 2/8] binman: etype: sysfw: Add entry type for sysfw
Date: Mon, 18 Apr 2022 22:56:15 +0300	[thread overview]
Message-ID: <bc54deaf-0157-0e37-c0ed-a6d4d936f1c0@gmail.com> (raw)
In-Reply-To: <20220406122919.6104-3-n-francis@ti.com>

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.

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

  reply	other threads:[~2022-04-18 19:58 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-06 12:29 [RESEND, RFC 0/8] Integration of sysfw and tispl with U-Boot Neha Malcom Francis
2022-04-06 12:29 ` [RESEND, RFC 1/8] tools: config: yaml: Add board config class to generate config binaries Neha Malcom Francis
2022-04-18 19:55   ` Alper Nebi Yasak
2022-04-19  2:49     ` Neha Malcom Francis
2022-04-06 12:29 ` [RESEND, RFC 2/8] binman: etype: sysfw: Add entry type for sysfw Neha Malcom Francis
2022-04-18 19:56   ` Alper Nebi Yasak [this message]
2022-04-19  2:49     ` Neha Malcom Francis
2022-04-06 12:29 ` [RESEND, RFC 3/8] schema: yaml: Add board config schema Neha Malcom Francis
2022-04-06 12:29 ` [RESEND, RFC 4/8] config: yaml: j721e_evm: Add board config for J721E EVM Neha Malcom Francis
2022-04-06 12:29 ` [RESEND, RFC 5/8] binman: sysfw: Add support for packaging tiboot3.bin and sysfw.itb Neha Malcom Francis
2022-04-18 19:56   ` Alper Nebi Yasak
2022-04-06 12:29 ` [RESEND, RFC 6/8] binman: dtsi: sysfw: j721e: Use binman to package sysfw.itb Neha Malcom Francis
2022-04-18 19:56   ` Alper Nebi Yasak
2022-04-06 12:29 ` [RESEND, RFC 7/8] binman: etype: dm: Add entry type for TI DM Neha Malcom Francis
2022-04-18 19:56   ` Alper Nebi Yasak
2022-04-06 12:29 ` [RESEND, RFC 8/8] binman: dtsi: tispl: j721e: Use binman to package tispl.bin Neha Malcom Francis
2022-04-18 19:57   ` Alper Nebi Yasak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bc54deaf-0157-0e37-c0ed-a6d4d936f1c0@gmail.com \
    --to=alpernebiyasak@gmail.com \
    --cc=a-govindraju@ti.com \
    --cc=joel.peshkin@broadcom.com \
    --cc=kishon@ti.com \
    --cc=kristo@kernel.org \
    --cc=marek.behun@nic.cz \
    --cc=mr.nuke.me@gmail.com \
    --cc=n-francis@ti.com \
    --cc=nm@ti.com \
    --cc=patrick.delaunay@foss.st.com \
    --cc=s-anna@ti.com \
    --cc=sjg@chromium.org \
    --cc=u-boot@lists.denx.de \
    --cc=vigneshr@ti.com \
    --cc=xypron.glpk@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.