Re: [u-boot][PATCH] binman: Add support for TEE BL32

[Roger Quadros <rogerq@kernel.org>]

Hi Simon,

On 04/02/2022 19:17, Simon Glass wrote:
> Hi Roger,
> 
> On Fri, 4 Feb 2022 at 06:00, Roger Quadros <rogerq@kernel.org> wrote:
>>
>> Add an entry for OP-TEE Trusted OS 'BL32' payload.
>> This is required by platforms using Cortex-A cores with TrustZone
>> technology.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  Makefile                           |  1 +
>>  tools/binman/entries.rst           | 13 +++++++++++++
>>  tools/binman/etype/atf_bl32.py     | 22 ++++++++++++++++++++++
>>  tools/binman/ftest.py              |  7 +++++++
>>  tools/binman/test/170_atf_bl32.dts | 16 ++++++++++++++++
>>  5 files changed, 59 insertions(+)
>>  create mode 100644 tools/binman/etype/atf_bl32.py
>>  create mode 100644 tools/binman/test/170_atf_bl32.dts
> 
> Funnily enough I have been fiddling with some patches to replace the
> rockchip SPL_FIT_GENERATOR script and have added an op-tee entry type
> in that. I did not think of op-tee as part of ATF, so didn't call it
> bl32, etc. That is perhaps just because I didn't know that.
> 
> So is op-tee always added as part of ATF and with a BL32 name?

In fact at build time the filename is tee-pager_v2.bin.
I just picked the name BL32 but now when I look back it doesn't seem right.

I think tee.bin is a better name.

> 
>>
>> diff --git a/Makefile b/Makefile
>> index 184223ec63..5e2f89d742 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1326,6 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
>>                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
>>                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
>>                 -a atf-bl31-path=${BL31} \
>> +               -a atf-bl32-path=${BL32} \
> 
> Some boards use $(TEE), but I suppose it is fine to use this code word.

Let's use $(TEE) then. It seems more appropriate.

> 
>>                 -a opensbi-path=${OPENSBI} \
>>                 -a default-dt=$(default_dt) \
>>                 -a scp-path=$(SCP) \
>> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
>> index c47f7df098..5215df7734 100644
>> --- a/tools/binman/entries.rst
>> +++ b/tools/binman/entries.rst
>> @@ -25,6 +25,19 @@ about ATF.
>>
>>
>>
>> +Entry: atf-bl32: Entry containing an OP-TEE Trusted OS (TEE) BL32 blob
> 
> So is bl32 always OP-TEE or could it be something else?
> 
>> +-------------------------------------------------------------------------
>> +
>> +Properties / Entry arguments:
>> +    - atf-bl32-path: Filename of file to read into entry. This is typically
>> +        called bl32.bin or bl32.elf
>> +
>> +This entry holds the run-time firmware, typically started by U-Boot SPL.
>> +See the U-Boot README for your architecture or board for how to use it. See
>> +https://github.com/OP-TEE/optee_os for more information about OP-TEE.
>> +
>> +
>> +
>>  Entry: atf-fip: ARM Trusted Firmware's Firmware Image Package (FIP)
>>  -------------------------------------------------------------------
>>
>> diff --git a/tools/binman/etype/atf_bl32.py b/tools/binman/etype/atf_bl32.py
>> new file mode 100644
>> index 0000000000..e74b4e4428
>> --- /dev/null
>> +++ b/tools/binman/etype/atf_bl32.py
>> @@ -0,0 +1,22 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +# Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>> +#
>> +# Entry-type module for OP-TEE Trusted OS firmware blob
>> +#
>> +
>> +from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
>> +
>> +class Entry_atf_bl32(Entry_blob_named_by_arg):
>> +    """Entry containing an OP-TEE Trusted OS (TEE) BL32 blob
>> +
>> +    Properties / Entry arguments:
>> +        - atf-bl32-path: Filename of file to read into entry. This is typically
>> +            called bl32.bin or bl32.elf
>> +
>> +    This entry holds the run-time firmware, typically started by U-Boot SPL.
>> +    See the U-Boot README for your architecture or board for how to use it. See
>> +    https://github.com/OP-TEE/optee_os for more information about OP-TEE.
>> +    """
>> +    def __init__(self, section, etype, node):
>> +        super().__init__(section, etype, node, 'atf-bl32')
>> +        self.external = True
>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>> index 5400f76c67..9366581bee 100644
>> --- a/tools/binman/ftest.py
>> +++ b/tools/binman/ftest.py
>> @@ -81,6 +81,7 @@ FSP_M_DATA            = b'fsp_m'
>>  FSP_S_DATA            = b'fsp_s'
>>  FSP_T_DATA            = b'fsp_t'
>>  ATF_BL31_DATA         = b'bl31'
>> +ATF_BL32_DATA         = b'bl32'
>>  ATF_BL2U_DATA         = b'bl2u'
>>  OPENSBI_DATA          = b'opensbi'
>>  SCP_DATA              = b'scp'
>> @@ -185,6 +186,7 @@ class TestFunctional(unittest.TestCase):
>>          TestFunctional._MakeInputFile('compress', COMPRESS_DATA)
>>          TestFunctional._MakeInputFile('compress_big', COMPRESS_DATA_BIG)
>>          TestFunctional._MakeInputFile('bl31.bin', ATF_BL31_DATA)
>> +        TestFunctional._MakeInputFile('bl32.bin', ATF_BL32_DATA)
>>          TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
>>          TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
>>          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>> @@ -3877,6 +3879,11 @@ class TestFunctional(unittest.TestCase):
>>          data = self._DoReadFile('169_atf_bl31.dts')
>>          self.assertEqual(ATF_BL31_DATA, data[:len(ATF_BL31_DATA)])
>>
>> +    def testPackBl32(self):
>> +        """Test that an image with an ATF BL32 binary can be created"""
>> +        data = self._DoReadFile('170_atf_bl32.dts')
>> +        self.assertEqual(ATF_BL32_DATA, data[:len(ATF_BL32_DATA)])
>> +
>>      def testPackScp(self):
>>          """Test that an image with an SCP binary can be created"""
>>          data = self._DoReadFile('172_scp.dts')
>> diff --git a/tools/binman/test/170_atf_bl32.dts b/tools/binman/test/170_atf_bl32.dts
>> new file mode 100644
>> index 0000000000..8c15c79c86
>> --- /dev/null
>> +++ b/tools/binman/test/170_atf_bl32.dts
> 
> Please use the next available number. We are up to 220 I think now.

OK.

> 
>> @@ -0,0 +1,16 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +/dts-v1/;
>> +
>> +/ {
>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +
>> +       binman {
>> +               size = <16>;
> 
> Do you need that? Without it you could simplify your assert above

Not required. I will remove it.
> 
>> +
>> +               atf-bl32 {
>> +                       filename = "bl32.bin";
>> +               };
>> +       };
>> +};
>> --
>> 2.17.1
>>
> 
> Regards,
> Simon

--
cheers,
-roger