Message-ID: <ce93ba16-e2a8-4015-bc01-139917d37782@quicinc.com>
Date: Thu, 14 Aug 2025 16:17:55 +0530
From: Harshal Dev <quic_hdev@...cinc.com>
To: Sumit Garg <sumit.garg@...nel.org>
CC: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Jens
 Wiklander <jens.wiklander@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <op-tee@...ts.trustedfirmware.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v19 2/6] remoteproc: Add TEE support

Hi Sumit,

On 8/11/2025 7:41 PM, Sumit Garg wrote:
> Hi Harshal,
> 
> On Mon, Aug 04, 2025 at 02:56:18PM +0530, Harshal Dev wrote:
>> Hi Arnaud,
>>
>> On 8/1/2025 12:53 PM, Arnaud POULIQUEN wrote:
>>> Hello Harshal,
>>>
>>>
>>> On 7/31/25 12:25, Harshal Dev wrote:
>>>> Hello Arnaud,
>>>>
>>>> On 6/25/2025 3:10 PM, Arnaud Pouliquen wrote:
>>>>> Add a remoteproc TEE (Trusted Execution Environment) driver that will be
>>>>> probed by the TEE bus. If the associated Trusted application is supported
>>>>> on the secure part, this driver offers a client interface to load firmware
>>>>> by the secure part.
>>>>> This firmware could be authenticated by the secure trusted application.
>>>>>
>>>>> A specificity of the implementation is that the firmware has to be
>>>>> authenticated and optionally decrypted to access the resource table.
>>>>>
>>>>> Consequently, the boot sequence is:
>>>>>
>>>>> 1) rproc_parse_fw --> rproc_tee_parse_fw
>>>>>    remoteproc TEE:
>>>>>    - Requests the TEE application to authenticate and load the firmware
>>>>>      in the remote processor memories.
>>>>>    - Requests the TEE application for the address of the resource table.
>>>>>    - Creates a copy of the resource table stored in rproc->cached_table.
>>>>>
>>>>> 2) rproc_load_segments --> rproc_tee_load_fw
>>>>>    remoteproc TEE:
>>>>>    - Requests the TEE application to load the firmware. Nothing is done
>>>>>      at the TEE application as the firmware is already loaded.
>>>>>    - In case of recovery, the TEE application has to reload the firmware.
>>>>>
>>>>> 3) rproc_tee_get_loaded_rsc_table
>>>>>    remoteproc TEE requests the TEE application for the address of the
>>>>>    resource table.
>>>>>
>>>>> 4) rproc_start --> rproc_tee_start
>>>>>    - Requests the TEE application to start the remote processor.
>>>>>
>>>>> The shutdown sequence is:
>>>>>
>>>>> 5) rproc_stop --> rproc_tee_stop
>>>>>    - Requests the TEE application to stop the remote processor.
>>>>>
>>>>> 6) rproc_tee_release_fw
>>>>>    This function is used to request the TEE application to perform actions
>>>>>    to return to the initial state on stop or on error during the boot
>>>>>    sequence.
>>>>>
>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
>>>>> ---
>>>>> Updates vs version [18]:
>>>>> - rework/fix function headers
>>>>> - use memremap instead of ioremap for the resource table.
>>>>> - realign comments to 80 chars limit, with few exceptions for readability
>>>>> - replace spinlock by mutex and and protect APIs from concurrent access
>>>>> - add support of 64-bit address in rproc_tee_get_loaded_rsc_table()
>>>>> - Generalize teston rproc_tee_ctx.dev to prevent an unbind
>>>>> - update copyright year
>>>>>
>>>>> Updates vs version [17]:
>>>>> Fix warning:
>>>>> warning: EXPORT_SYMBOL() is used, but #include <linux/export.h> is missing
>>>>> ---
>>>>>  drivers/remoteproc/Kconfig          |  10 +
>>>>>  drivers/remoteproc/Makefile         |   1 +
>>>>>  drivers/remoteproc/remoteproc_tee.c | 708 ++++++++++++++++++++++++++++
>>>>>  include/linux/remoteproc_tee.h      |  87 ++++
>>>>>  4 files changed, 806 insertions(+)
>>>>>  create mode 100644 drivers/remoteproc/remoteproc_tee.c
>>>>>  create mode 100644 include/linux/remoteproc_tee.h
>>>>>
> 
> <snip>
> 
>>>>> +
>>>>> +static int rproc_tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
>>>>> +{
>>>>> +	/* Today we support only the OP-TEE, could be extend to other tees */
>>>>> +	return (ver->impl_id == TEE_IMPL_ID_OPTEE);
>>>>> +}
>>>>> +
>>>>> +static int rproc_tee_probe(struct device *dev)
>>>>> +{
>>>>> +	struct tee_context *tee_ctx;
>>>>> +	int ret;
>>>>> +
>>>>> +	/* Open context with TEE driver */
>>>>> +	tee_ctx = tee_client_open_context(NULL, rproc_tee_ctx_match, NULL, NULL);
>>>>> +	if (IS_ERR(tee_ctx))
>>>>> +		return PTR_ERR(tee_ctx);
>>>>> +
>>>>> +	ret = mutex_lock_interruptible(&ctx_lock);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	rproc_tee_ctx.dev = dev;
>>>>> +	rproc_tee_ctx.tee_ctx = tee_ctx;
>>>>> +	INIT_LIST_HEAD(&rproc_tee_ctx.sessions);
>>>>> +	mutex_unlock(&ctx_lock);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>
>>>> As you mentioned above, this could be extended to other TEEs. If so, is it possible for probe
>>>> to be called multiple times if we we have other TEE devices exposing the firmware authentication
>>>> service? In that case, I think rproc_tee_ctx should be dynamically initializated instead of being
>>>> static. And since we are creating a link between the Rproc device and TEE device, a call to a
>>>> function like rproc_tee_start() could retreive the associated TEE device, and then the associated
>>>> rproc_tee? :)
>>>
>>> I have never seen a use case that requires multiple instances, but perhaps you
>>> have some?
>>>
>>> We can expect only one TEE, which could be OP-TEE, Trusty, or another.
>>> The device is associated with a unique UUID, so only one instance is expected.
>>>
>>> That said, making this driver support multiple instances seems like a valid
>>> future enhancement. However, I would suggest implementing it as a second step
>>> when there is a concrete need.
>>>
>>
>> My thought process on this stems from 1) the recent ARM FF-A developments and 2) from the current
>> implementation of the TEE subsystem which allows multiple back-end drivers to register themselves
>> via the tee_device_register() API. This means, that it's possible to have a configuration
>> where a platform supports multiple TEEs running as Secure Partitions via FF-A, and each of those
>> TEEs register their services as PTA devices on the TEE bus.
>>
>> However, I do not really know if it's possible to have a UUID collision in such a case, which
>> would lead to rproc_tee_probe() being called twice above, which is why I raised this question. :)
>>
>> All of this aside, I realize now that other TEE client drivers are also implemented with a static
>> private data similar to how you are doing it. So perhaps we can think of this as a later
>> enhancement if we believe that the scenario I am describing is not possible in the near future..
>>
> 
> Theoretically it is possible for multiple TEE services to be there but
> why should a platform/silicon vendor require 2 redundant remoteproc firmware
> loading services to be supported? It should either be a service hosted
> by the trusted OS or can rather be an independent platform service
> running as a FF-A secure partition.
> 
I agree that it doesn't make sense for a system integrator to have two remoteproc firmware
loading services supported from two different TEEs running as Secure Partitions.
After all, one service exposed by one TEE is good enough for fulfilling any use-case.

My concern is that ARM FF-A makes its possible to have a platform running two TEEs, which
each have their own remoteproc firmware authentication service implemented (as usually TEEs do).
In such a scenario, when both TEEs enumerate their services on the TEE bus, and find a match
because the rproc_tee_id_table has a UUID for say, both the TS-TEE remoteproc service and
OP-TEE remoteproc service, rproc_tee_probe() will be called twice, and the current implementation
will break because it uses a single static rproc_tee_ctx, whose contents would be overwritten
leading to unexpected scenarios.

And so, should TEE subsystem clients (like this one) be prepared to handle such as scenario?

Thanks,
Harshal
> -Sumit

Powered by blists - more mailing lists