Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Heiko, Chris,

On 2/10/25 6:06 PM, Chris Morgan wrote:
> On Mon, Feb 10, 2025 at 11:20:27AM +0100, Heiko Stübner wrote:
>> Hi,
>>
>> Am Dienstag, 4. Februar 2025, 17:16:22 MEZ schrieb Chris Morgan:
>>> On Tue, Feb 04, 2025 at 10:12:26AM +0100, Quentin Schulz wrote:
>>>> Hi Chris,
>>>>
>>>> On 2/3/25 10:27 PM, Chris Morgan wrote:
>>>>> On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> On 1/31/25 5:59 PM, Chris Morgan wrote:
>>>>>>> On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> On 1/30/25 7:10 PM, Chris Morgan wrote:
>>>>>> [...]
>>>>>>>>> When Optee is not present or used, the node will trigger a probe
>>>>>>>>> that generates a (harmless) message on the kernel log.
>>>>>>>>>
>>>>>>>>
>>>>>>>> And what if we have OP-TEE without this node present, which would be
>>>>>>>> possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
>>>>>>>>
>>>>>>>> I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
>>>>>>>> and fixup the DTB otherwise to mark this node as status = "disabled"?
>>>>>>>>
>>>>>>>
>>>>>>> Based on my understanding of how each of these projects communicate
>>>>>>> with each other, Optee can only work if you either include both the
>>>>>>> Optee node in the firmware AND the reserved memory nodes yourself, or
>>>>>>> if you have neither and let U-Boot do it (by including each of these
>>>>>>> patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
>>>>>>> basically just this node alone is insufficient for it to work today.
>>>>>>>
>>>>>>
>>>>>> Therefore I think we should either have this node defined + the
>>>>>> reserved-memory node (with hardcoded address and size if guaranteed to be
>>>>>> stable forever) added or nothing.
>>>>>>
>>>>>> Can we mark a reserved-memory node as "disabled"? If not, then we should
>>>>>> rather have nothing at all I believe.
>>>>>
>>>>> I honestly would just rather remove this node. The reason I say that is
>>>>> while we support Optee with the RK3399, RK3228, the PX30, and the
>>>>> RK3588; howver only the RK3588 has the node already populated in Linux.
>>>>>
>>>>
>>>> And we have a product based on PX30 that has OP-TEE OS running without a
>>>> hardcoded node in the DTS, so that's a valid point of comparison to me. That
>>>> seems to justify the deletion to me!
>>>>
>>>>>>
>>>>>>> The core issue is that Optee requires you to map the memory as
>>>>>>> reserved so that Linux doesn't try to use it. You can either do that
>>>>>>> yourself or you can have U-Boot do it automatically. Having the Optee
>>>>>>> node in the devicetree makes no sense today unless we also carve out
>>>>>>> the memory.
>>>>>>>
>>>>>>
>>>>>> We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
>>>>>> passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
>>>>>> nodes in the kernel DT. What do you think?
>>>>>>
>>>>>
>>>>> We would have to assume some hard coded values in that event as I'm not
>>>>> aware of a mechanism to grab them at runtime from Optee except when you
>>>>> pass it a device tree. That said I think the concern above where you
>>>>
>>>> Yes, but that would be the same as BL31 for example and an expected side
>>>> effect of using CONFIG_SPL_ATF_NO_PLATFORM_PARAM except if there's a way to
>>>> provide information back from TEE to U-Boot without using the params that
>>>> would be passed by U-Boot to TF-A had we CONFIG_SPL_ATF_NO_PLATFORM_PARAM
>>>> disabled.
>>>>
>>>>> mention "guaranteed to be stable forever" is the problem, as even now
>>>>> the current address in the Optee upstream project conflicts with the
>>>>> kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
>>>>> changed (I'm attempting to change the Optee one, for what it's worth).
>>>>>
>>>>
>>>> I think it makes more sense to change the load addresses in U-Boot,
>>>> especially since those are just the default values for variables which are
>>>> configurable per board type, per board or even per boot, so it's something
>>>> one would be able to modify without necessarily rebuilding anything.
>>>> Essentially, it's easier to move that around than checking which OP-TEE OS
>>>> version one is booting before providing advice on where to load the image?
>>>> Up to you though, no strong opinion there.
>>>
>>> I think I mentioned this but in IRC but the consensus was to change Optee
>>> to match the same addresses as the PX30 and RK3399. No strong opinion from
>>> me either, just trying to get it working without stepping on toes anywhere.
>>
>> glancing through the thread, did you come to a conclusion whether to drop
>> or keep the optee node?
>>
>> I.e. from what I see, having this default optee node _without_ the needed
>> memory reservation would mean that _if_ an optee runs, the kernel would
>> either possibly write over its memory, or we'd end up in an exception thing
>> because the kernel would write over firewall'd memory.
>>
>> So at this point, having the optee node here, makes no real sense to me,
>> as firmware that would need to add reserved memory to the dt, could also
>> just add the optee node in the same run - as we do on other socs already?
>>
>>
>> Heiko
>>
>>
> 
> I think if I am hearing correctly we decided to drop this node.

Agreed.

Thanks!
Quentin

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip