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

[Alex G.]

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?
> 
> 
> 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.

Alex