[PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Patrick DELAUNAY]

Hi Alex

On 9/2/21 7:34 PM, Alex G. wrote:
> Hi Patrick,
>
> On 9/1/21 10:10 AM, Alex G. wrote:
>> Hi Patrick,
>>
>> On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
>>> Hi,
>>>
>>> On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
>>>> OP-TEE does not take a devicetree for its own use. However, it does
>>>> pass the devicetree to the normal world OS. In most cases that will
>>>> be some other devicetree-bearing platform, such as linux.
>>>>
>>>> As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
>>>> it is required to copy the optee nodes to he target's FDT. Do this as
>>>> part of spl_board_prepare_for_optee().
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>
>>>> ---
>>>>   arch/arm/mach-stm32mp/spl.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
>>>> index d9fdc5926c..94fbb45cf9 100644
>>>> --- a/arch/arm/mach-stm32mp/spl.c
>>>> +++ b/arch/arm/mach-stm32mp/spl.c
>>>> @@ -19,6 +19,7 @@
>>>>   #include <asm/arch/sys_proto.h>
>>>>   #include <mach/tzc.h>
>>>>   #include <linux/libfdt.h>
>>>> +#include <tee/optee.h>
>>>>   u32 spl_boot_device(void)
>>>>   {
>>>> @@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
>>>>   void spl_board_prepare_for_optee(void *fdt)
>>>>   {
>>>>       stm32_fdt_setup_mac_addr(fdt);
>>>> +    optee_copy_fdt_nodes(fdt);
>>>>       stm32_init_tzc_for_optee();
>>>>   }
>>>
>>>
>>> NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the 
>>> unsecure device tree (when CFG_DT is activated)
>>>
>>> before to jump to the kernel (that remove the need to have DT 
>>> allignement with U-Boot SPL device tree)
>>>
>>> => SPL should not copy the OP-TEE nodes in next stage DT
>>>
>>> reference in optee_os = core/arch/arm/kernel/boot.c: 
>>> add_optee_dt_node()
>>>
>>> add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
>>>
>>> It is the expected configuration for OP-TEE on STM32MP15 platform.
>>
>> I agree that if a component modifies the platform, it should be 
>> responsible for updating the devicetree. Two issues though
>>
>>
>> 1. Consistency
>>
>> The STM32IMAGE boot path expects u-boot to add the optee nodes, while 
>> the FIP path expects OP-TEE to add the nodes. Which one do we stick to?
>>
>>

STM32MP platform expects U-Boot to copy the OP-TEE nodes because it is a 
generic U-Boot behavior when OP-TEE is supported

FIP path expects OP-TEE to add their nodes because it is a generic 
OP-TEE behavior

then the added nodes are copied by U-Boot in kernel DT


It is generic, flexible (OP-TEE configuration update wihout change in 
kernel DT or in U-Boot)

but not time optimized...


for STM32IMAGE image support, I have a issue (restriction)  = the U-Boot 
DT can't updated by

OP-TEE (U-Boot DT is embedded in u-boot.stm32 not available by OP-TEE)

=> I force presence of OP-TEE node in U-Boot DT for STM32IMAGE support

       to allow copy to kernel DT.


But it is less generic (dependency between U-Boot device tree and OP-TEE):

it is one reason we switch to FIP support


in you use case: SPL -> OP-TEE -> kernel


I think the kernel device tree can be updated by OP-TEE (to be generic) or

the kernel DT have already the correct OP-TEE nodes to reduce the DT 
update time penalty,

as you are sure of the OP-TEE support / configuration for you board.


drawback: the kernel device tree need to be aligned with the OP-TEE 
configuration.


>> 2. Memory access
>>
>> I don't understand is how OP-TEE would get past the TZC.
>> If we look at optee_os => plat-0stm32mp1/plat_tzc400:
>>      "Early boot stage is in charge of configuring memory regions"
>> The following memory has been set up by SPL (or TF-A for that matter):
>>
>>      Nonsec: c0000000->ddffffff
>>      Sec:    de000000->dfdfffff
>>      SHMEM:  dfe00000->dfffffff
>>
>> The external DTB will be in the nonsec region, which OP-TEE allegedly 
>> can't access. So how would this get patched?
>
> Let's set aside the above for a bit. I tried the following OP-TEE 
> configs:
>
>
> "CFG_DT=y CFG_DTB_MAX_SIZE=0x80000"
>
> Crashes with 'unhandled pageable abort' somewhere in 
> core/arch/arm/plat-stm32mp1/ . I don't have much more time to spend 
> debugging this.
>
>
> "CFG_DT=y CFG_DTB_MAX_SIZE=0x80000 PLATFORM_FLAVOR=157C_DK2"
>
> This adds about 800 ms to the boot time. (power on to kernel printing 
> the first line). I have an allowance of 1 second to get to the kernel. 
> I cannot just add 800ms to the boot time and hope nobody notices. 
> However, calling optee_copy_fdt_nodes() adds no measureable amount of 
> time.
>
> Unless OP-TEE can modify the devicetree without increasing boot time, 
> I will not go down this path. This is an engineering decision driven 
> by the product requirements.
>
> That being said, I request that you reconsider your NAK based on this 
> new data.


in my previous answer I think on generic support and you have specific 
boot time requirement.


but after cross-check with TF-A boot in FIP mode,

in TF-A set the correct firewall configuration at the end of BL2 (it is 
not done in OP-TEE as I expect)

based on configuration description found in FIP.


To be coherent with you configuration (SPL->OP-TEE->kernel),

this firewall configuration should be done by SPL before to start OP-TEE.


And to avoid to lost time in device tree parsing the DT modification or 
copy of node should be avoid:

the OP-TEE reserved should be already present in your kernel device tree 
=> no more need to copied them


I dig again with OPTEE features support in SPL and to avoid dependancy 
issue I pushed a serie:

"lib: optee: remove the duplicate CONFIG_OPTEE"

http://patchwork.ozlabs.org/project/uboot/list/?series=260701&state=*


=> in your use case and for reduce the boot time

- activate CONFIG_SPL_OPTEE_IMAGE : load OP-TEE image by SPL

- manage OPTEE / firewall configuration with your defconfig

   (=> no more need of OP-TEE nodes in SPL DT / reduced SPL size and 
parsing time)

   by using the existing configurations in lib/optee/Kconfig instead DT 
information:

    => CONFIG_OPTEE_LOAD_ADDR

    => CONFIG_OPTEE_TZDRAM_SIZE

    => CONFIG_OPTEE_TZDRAM_BASE

    PS: need to add a configuration for OPTEE SHMEM_SIZE ?


- manually add OP-TEE nodes / reserved memory in the kernel (and U-Boot)

   device tree and OP-TEE config CFG_DT is not activated 
(update_external_dt not used)

=> OP-TEE don't modify the devicetre (as nodes are already present in NS DT)

=> in spl_board_prepare_for_optee, avoid to update device tree to lost time


in all the case : optee_copy_fdt_nodes in not done by SPL but by U-Boot 
started

by OP-TEE (for dynamic support)  or not done (no static configuration)


The only reason to copy the OP-TEE node for FIP support or for your use 
case is

to copy the nodes dynamically added by OP-TEE.


SPL / TFA -> OP-TEE -> U-Boot -> Kernel

                         (1)           (2)              (3)


OP-TEE only the U-Boot DT (non secure DT) in boot parameter

(1) => OP-TEE add the node in U-Boot DT

(2) => U-Boot load the kernel DT / copied the OP-TEE in kernel DT

(3) => kernel use the DT with correct OP-TEE node


The OP-TEE node copy is not necessary if the kernel start with the U-Boot DT

this sequence allow to have dynamic support of OP-TEE configuration.


But for product with fixed configuration it is not required.


I hope it is more clear.


one question:

why spl_fixup_fdt isn't called (for generic fixup = arch_fixup_fdt) for 
OP-TEE boot

before to call spl_board_prepare_for_optee ?


>
> Alex
>
>

Patrick