[PATCH] arm64: dts: imx8m: Reinstate TEE ifdeffery

[Marek Vasut]

On 7/9/26 9:16 AM, Yannic Moog wrote:

Hello Yannic,

>>>>>> When you include optee-os in the U-Boot fitImage, what do you do with
>>>>>> that optee-os which U-Boot does not support ? Details please ?
>>>>>
>>>>> Sure, sorry. OS can/will use it. There is no requirement, that I know, of any bootloader
>>>>> interfacing with OP-TEE for proper functionality and its purpose is to work with Linux
>>>>> kernel.
>>>>
>>>> At minimum, if CONFIG_OPTEE is not enabled, U-Boot won't execute
>>>> optee_copy_fdt_nodes() and won't transfer any optee-os firmware and
>>>> reserved-memory DT nodes to the Linux DT when starting the kernel, so
>>>> Linux DT will be buggy and Linux may possibly misbehave.
>>>
>>> Got it. I tested your theory and with all TEE configs off and OP-TEE packaged, kernel device
>>> tree
>>> still has the necessary device tree nodes. Is there something I missed (am using phycore-imx8mp
>>> board) ? How does your Linux dtb look like in that scenario?
>>
>> I cannot answer this question, the test you performed is not well
>> documented -- kernel version is unknown, whether the kernel was in any
>> way modified is also unknown, etc. Maybe your kernel tree already
>> contains some optee-os node ?
>>
>>>>> I guess the only job bootloader needs to do is load it into memory. I don't know details
>>>>> about
>>>>> specific uses within the security context and if there are any that require bootloader
>>>>> support.
>>>>
>>>> If in your use case, U-Boot is supposed to only load optee-os into
>>>> memory , why not load it from U-Boot command line or as part of fitImage
>>>> when booting Linux kernel ?
>>>
>>> I figured via the u-boot.dtsi is the easiest solution.
>>>
>>>>
>>>> It seems in your use case, optee-os does not have to be part of the
>>>> bootloader image at all ?
>>>
>>> I believe it does. The init phase looks the following (on imx8m)
>>>
>>> ROM -> U-Boot SPL -> tf-a -> OP-TEE -> tf-a -> U-boot proper
>>>
>>> and something has to load OP-TEE into main memory when tf-a is done with init and jumps to the
>>> OPTEE load addr. That's what U-Boot does.
>>
>> In that case, U-Boot should be aware of that optee-os , i.e. enable
>> CONFIG_OPTEE .
>>
>> [...]
>>
>>>>>>> Also, keeping optional when using if defs is not a good idea. Binman will still build
>>>>>>> you
>>>>>>> an
>>>>>>> image
>>>>>>> when no OP-TEE blob is present, but should not since you enabled OPTEE configs. If you
>>>>>>> enable
>>>>>>> OPTEE, you should have an OP-TEE blob packaged.
>>>>>>> We could add ifndef OPTEE around the optional property.
>>>>>>
>>>>>> The optional keyword is actually kept in this case, it is not a full revert.
>>>>>>
>>>>>> You did however miss one vital point -- the tee node is part of the
>>>>>> U-boot fitImage, even if the tee.bin binary is not present and OPTEE
>>>>>> support in U-Boot is disabled. Binman prints a confusing message, and
>>>>>> generates a bogus empty tee {} node in the fitImage. That is incorrect.
>>>>>
>>>>> Yes, forgot to address that. I don't think it is confusing, at least content wise.
>>>>> Visually, I
>>>>> can
>>>>> see how that is confusing. I want to neither agree nor disagree on the correctness part
>>>>> because
>>>>> I
>>>>> don't really know the fitImage spec. Having the tee node be part of the fitImage is a
>>>>> design
>>>>> choice, all optional blobs behave that way from my understanding.
>>>>
>>>> It is a broken node introduced by commit b57ed147939c ("dts:
>>>> imx8m{m,n,p,q}: Make optee packaging optional") , it is not supposed to
>>>> be part of the fitImage unless optee-os is bundled into the fitImage.
>>>
>>> This I still do not understand how you come to the conclusion that it (tee node) is not
>>> supposed to
>>> be part of the fitImage. Can you point me to the doc that disallows this?
>>
>> The empty tee node is dead code, that is why it should not be in the
>> fitImage. Before b57ed147939c ("dts: imx8m{m,n,p,q}: Make optee
>> packaging optional") that tee node was correctly not part of the
>> fitImage unless explicitly configured in by the user (CONFIG_OPTEE=y).
>>
>>>>> I guess it means that there is a bug in binman if you say having empty tee node is
>>>>> incorrect.
>>>>> And so it should be fixed in binman itself imo, not here for imx8.
>>>> No, this is not a bug in binman, please see above.
>>>
>>> Also, assuming that it is indeed illegal: Binman looks at an external blob and does not remove
>>> the
>>> parent node when it is the only child and found to be missing.
>>> Can you please clarify what should happen when no tee blob could be found and packaged?
>>
>> In case of CONFIG_OPTEE=y and "optional" keyword and missing tee.bin
>> blob , I think the node should be retained, because the user might
>> inject tee.bin into the u-boot.itb after the build with some "mkimage -F
>> ..." invocation.
>>
>> In case of CONFIG_OPTEE=n , there should be no tee node.
> 
> I think I need more time to digest your points. You did not convince me that when CONFIG_OPTEE=n
> there should not be any tee node. The rest of the discussion is based on this point so I would like
> to avoid discussing things further for now.
> If we can get another opinion on this discussion that'd be great I think. Either way, I am okay
> with taking this patch. Again, it is my opinion that this is not a fix, rather we are losing a
> feature.

Please do keep digesting. Please do keep two things in mind -- if the 
platform does run optee-os, the software stack should be aware of it. If 
there is a special case where the software stack should not be aware of 
it, please put that into board DTs or board U-Boot extras DTs, but that 
use case is special and non-default. Finally, including dead code / dead 
DT nodes in build artifacts that only grows the size of the build 
artifact is not good.