Message-ID: <aJn6EPjXzq07aDTM@sumit-X1>
Date: Mon, 11 Aug 2025 19:41:28 +0530
From: Sumit Garg <sumit.garg@...nel.org>
To: Harshal Dev <quic_hdev@...cinc.com>
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 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.

-Sumit

Powered by blists - more mailing lists