2024-05-21 08:12:43

by Arnaud Pouliquen

[permalink] [raw]
Subject: [PATCH v5 0/7] Introduction of a remoteproc tee to load signed firmware

Main updates from the previous version [1]:
------------------------------------------

1) use proc->table_ptr as unique reference to point to the resource table
--> update remoteproc_core.c to implement management of the resource table
base on rproc->rproc->tee_interface new field:
- on start get the resource table address from TEE remoteproc instead
of finding it in firmware (ops choice to confirm)
- on stop unmap the resource table before updating the
proc->table_ptr pointer.

2) retrieve the TEE rproc Identifier from the device tree instead of
hardcoding it
--> Add a new "st,proc-id" property in device tree.

More details on updates are listed in commits messages

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#m9ebb2e8f6d5e90f055827e4f227ce0877bc6d761

base-commit: c8d8f841e95bcc07ac8c5621fc171a24f1fd5cdb

Description of the feature:
--------------------------
This series proposes the implementation of a remoteproc tee driver to
communicate with a TEE trusted application responsible for authenticating
and loading the remoteproc firmware image in an Arm secure context.

1) Principle:

The remoteproc tee driver provides services to communicate with the OP-TEE
trusted application running on the Trusted Execution Context (TEE).
The trusted application in TEE manages the remote processor lifecycle:

- authenticating and loading firmware images,
- isolating and securing the remote processor memories,
- supporting multi-firmware (e.g., TF-M + Zephyr on a Cortex-M33),
- managing the start and stop of the firmware by the TEE.

2) Format of the signed image:

Refer to:
https://github.com/OP-TEE/optee_os/blob/master/ta/remoteproc/src/remoteproc_core.c#L18-L57

3) OP-TEE trusted application API:

Refer to:
https://github.com/OP-TEE/optee_os/blob/master/ta/remoteproc/include/ta_remoteproc.h

4) OP-TEE signature script

Refer to:
https://github.com/OP-TEE/optee_os/blob/master/scripts/sign_rproc_fw.py

Example of usage:
sign_rproc_fw.py --in <fw1.elf> --in <fw2.elf> --out <signed_fw.sign> --key ${OP-TEE_PATH}/keys/default.pem


5) Impact on User space Application

No sysfs impact.the user only needs to provide the signed firmware image
instead of the ELF image.


For more information about the implementation, a presentation is available here
(note that the format of the signed image has evolved between the presentation
and the integration in OP-TEE).

https://resources.linaro.org/en/resource/6c5bGvZwUAjX56fvxthxds

Arnaud Pouliquen (7):
remoteproc: Add TEE support
dt-bindings: remoteproc: Add compatibility for TEE support
dt-bindings: remoteproc: Add processor identifier property
remoteproc: core introduce rproc_set_rsc_table_on_start function
remoteproc: core: support of the tee interface
remoteproc: stm32: Create sub-functions to request shutdown and
release
remoteproc: stm32: Add support of an OP-TEE TA to load the firmware

.../bindings/remoteproc/st,stm32-rproc.yaml | 58 ++-
drivers/remoteproc/Kconfig | 10 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_core.c | 135 +++---
drivers/remoteproc/stm32_rproc.c | 149 ++++--
drivers/remoteproc/tee_remoteproc.c | 429 ++++++++++++++++++
include/linux/remoteproc.h | 4 +
include/linux/tee_remoteproc.h | 99 ++++
8 files changed, 784 insertions(+), 101 deletions(-)
create mode 100644 drivers/remoteproc/tee_remoteproc.c
create mode 100644 include/linux/tee_remoteproc.h

--
2.25.1



2024-05-21 08:13:34

by Arnaud Pouliquen

[permalink] [raw]
Subject: [PATCH v5 6/7] remoteproc: stm32: Create sub-functions to request shutdown and release

To prepare for the support of TEE remoteproc, create sub-functions
that can be used in both cases, with and without remoteproc TEE support.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/stm32_rproc.c | 84 +++++++++++++++++++-------------
1 file changed, 51 insertions(+), 33 deletions(-)

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 88623df7d0c3..8cd838df4e92 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -209,6 +209,54 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
return -EINVAL;
}

+static void stm32_rproc_request_shutdown(struct rproc *rproc)
+{
+ struct stm32_rproc *ddata = rproc->priv;
+ int err, dummy_data, idx;
+
+ /* Request shutdown of the remote processor */
+ if (rproc->state != RPROC_OFFLINE && rproc->state != RPROC_CRASHED) {
+ idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_SHUTDOWN);
+ if (idx >= 0 && ddata->mb[idx].chan) {
+ /* A dummy data is sent to allow to block on transmit. */
+ err = mbox_send_message(ddata->mb[idx].chan,
+ &dummy_data);
+ if (err < 0)
+ dev_warn(&rproc->dev, "warning: remote FW shutdown without ack\n");
+ }
+ }
+}
+
+static int stm32_rproc_release(struct rproc *rproc)
+{
+ struct stm32_rproc *ddata = rproc->priv;
+ unsigned int err = 0;
+
+ /* To allow platform Standby power mode, set remote proc Deep Sleep. */
+ if (ddata->pdds.map) {
+ err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
+ ddata->pdds.mask, 1);
+ if (err) {
+ dev_err(&rproc->dev, "failed to set pdds\n");
+ return err;
+ }
+ }
+
+ /* Update coprocessor state to OFF if available. */
+ if (ddata->m4_state.map) {
+ err = regmap_update_bits(ddata->m4_state.map,
+ ddata->m4_state.reg,
+ ddata->m4_state.mask,
+ M4_STATE_OFF);
+ if (err) {
+ dev_err(&rproc->dev, "failed to set copro state\n");
+ return err;
+ }
+ }
+
+ return 0;
+}
+
static int stm32_rproc_prepare(struct rproc *rproc)
{
struct device *dev = rproc->dev.parent;
@@ -519,17 +567,9 @@ static int stm32_rproc_detach(struct rproc *rproc)
static int stm32_rproc_stop(struct rproc *rproc)
{
struct stm32_rproc *ddata = rproc->priv;
- int err, idx;
+ int err;

- /* request shutdown of the remote processor */
- if (rproc->state != RPROC_OFFLINE && rproc->state != RPROC_CRASHED) {
- idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_SHUTDOWN);
- if (idx >= 0 && ddata->mb[idx].chan) {
- err = mbox_send_message(ddata->mb[idx].chan, "detach");
- if (err < 0)
- dev_warn(&rproc->dev, "warning: remote FW shutdown without ack\n");
- }
- }
+ stm32_rproc_request_shutdown(rproc);

err = stm32_rproc_set_hold_boot(rproc, true);
if (err)
@@ -541,29 +581,7 @@ static int stm32_rproc_stop(struct rproc *rproc)
return err;
}

- /* to allow platform Standby power mode, set remote proc Deep Sleep */
- if (ddata->pdds.map) {
- err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
- ddata->pdds.mask, 1);
- if (err) {
- dev_err(&rproc->dev, "failed to set pdds\n");
- return err;
- }
- }
-
- /* update coprocessor state to OFF if available */
- if (ddata->m4_state.map) {
- err = regmap_update_bits(ddata->m4_state.map,
- ddata->m4_state.reg,
- ddata->m4_state.mask,
- M4_STATE_OFF);
- if (err) {
- dev_err(&rproc->dev, "failed to set copro state\n");
- return err;
- }
- }
-
- return 0;
+ return stm32_rproc_release(rproc);
}

static void stm32_rproc_kick(struct rproc *rproc, int vqid)
--
2.25.1


2024-05-21 08:13:40

by Arnaud Pouliquen

[permalink] [raw]
Subject: [PATCH v5 4/7] remoteproc: core introduce rproc_set_rsc_table_on_start function

Split rproc_start()to prepare the update of the management of
the cache table on start, for the support of the firmware loading
by the TEE interface.
- create rproc_set_rsc_table_on_start() to address the management of
the cache table in a specific function, as done in
rproc_reset_rsc_table_on_stop().
- rename rproc_set_rsc_table in rproc_set_rsc_table_on_attach()
- move rproc_reset_rsc_table_on_stop() to be close to the
rproc_set_rsc_table_on_start() function

Suggested-by: Mathieu Poirier <[email protected]>
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 116 ++++++++++++++-------------
1 file changed, 62 insertions(+), 54 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f276956f2c5c..42bca01f3bde 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1264,18 +1264,9 @@ void rproc_resource_cleanup(struct rproc *rproc)
}
EXPORT_SYMBOL(rproc_resource_cleanup);

-static int rproc_start(struct rproc *rproc, const struct firmware *fw)
+static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct firmware *fw)
{
struct resource_table *loaded_table;
- struct device *dev = &rproc->dev;
- int ret;
-
- /* load the ELF segments to memory */
- ret = rproc_load_segments(rproc, fw);
- if (ret) {
- dev_err(dev, "Failed to load program segments: %d\n", ret);
- return ret;
- }

/*
* The starting device has been given the rproc->cached_table as the
@@ -1291,6 +1282,64 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
rproc->table_ptr = loaded_table;
}

+ return 0;
+}
+
+static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
+{
+ /* A resource table was never retrieved, nothing to do here */
+ if (!rproc->table_ptr)
+ return 0;
+
+ /*
+ * If a cache table exists the remote processor was started by
+ * the remoteproc core. That cache table should be used for
+ * the rest of the shutdown process.
+ */
+ if (rproc->cached_table)
+ goto out;
+
+ /*
+ * If we made it here the remote processor was started by another
+ * entity and a cache table doesn't exist. As such make a copy of
+ * the resource table currently used by the remote processor and
+ * use that for the rest of the shutdown process. The memory
+ * allocated here is free'd in rproc_shutdown().
+ */
+ rproc->cached_table = kmemdup(rproc->table_ptr,
+ rproc->table_sz, GFP_KERNEL);
+ if (!rproc->cached_table)
+ return -ENOMEM;
+
+ /*
+ * Since the remote processor is being switched off the clean table
+ * won't be needed. Allocated in rproc_set_rsc_table_on_start().
+ */
+ kfree(rproc->clean_table);
+
+out:
+ /*
+ * Use a copy of the resource table for the remainder of the
+ * shutdown process.
+ */
+ rproc->table_ptr = rproc->cached_table;
+ return 0;
+}
+
+static int rproc_start(struct rproc *rproc, const struct firmware *fw)
+{
+ struct device *dev = &rproc->dev;
+ int ret;
+
+ /* load the ELF segments to memory */
+ ret = rproc_load_segments(rproc, fw);
+ if (ret) {
+ dev_err(dev, "Failed to load program segments: %d\n", ret);
+ return ret;
+ }
+
+ rproc_set_rsc_table_on_start(rproc, fw);
+
ret = rproc_prepare_subdevices(rproc);
if (ret) {
dev_err(dev, "failed to prepare subdevices for %s: %d\n",
@@ -1450,7 +1499,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
return ret;
}

-static int rproc_set_rsc_table(struct rproc *rproc)
+static int rproc_set_rsc_table_on_attach(struct rproc *rproc)
{
struct resource_table *table_ptr;
struct device *dev = &rproc->dev;
@@ -1540,54 +1589,13 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)

/*
* The clean resource table is no longer needed. Allocated in
- * rproc_set_rsc_table().
+ * rproc_set_rsc_table_on_attach().
*/
kfree(rproc->clean_table);

return 0;
}

-static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
-{
- /* A resource table was never retrieved, nothing to do here */
- if (!rproc->table_ptr)
- return 0;
-
- /*
- * If a cache table exists the remote processor was started by
- * the remoteproc core. That cache table should be used for
- * the rest of the shutdown process.
- */
- if (rproc->cached_table)
- goto out;
-
- /*
- * If we made it here the remote processor was started by another
- * entity and a cache table doesn't exist. As such make a copy of
- * the resource table currently used by the remote processor and
- * use that for the rest of the shutdown process. The memory
- * allocated here is free'd in rproc_shutdown().
- */
- rproc->cached_table = kmemdup(rproc->table_ptr,
- rproc->table_sz, GFP_KERNEL);
- if (!rproc->cached_table)
- return -ENOMEM;
-
- /*
- * Since the remote processor is being switched off the clean table
- * won't be needed. Allocated in rproc_set_rsc_table().
- */
- kfree(rproc->clean_table);
-
-out:
- /*
- * Use a copy of the resource table for the remainder of the
- * shutdown process.
- */
- rproc->table_ptr = rproc->cached_table;
- return 0;
-}
-
/*
* Attach to remote processor - similar to rproc_fw_boot() but without
* the steps that deal with the firmware image.
@@ -1614,7 +1622,7 @@ static int rproc_attach(struct rproc *rproc)
goto disable_iommu;
}

- ret = rproc_set_rsc_table(rproc);
+ ret = rproc_set_rsc_table_on_attach(rproc);
if (ret) {
dev_err(dev, "can't load resource table: %d\n", ret);
goto unprepare_device;
--
2.25.1


2024-05-21 08:13:42

by Arnaud Pouliquen

[permalink] [raw]
Subject: [PATCH v5 1/7] remoteproc: Add TEE support

Add a remoteproc TEE (Trusted Execution Environment) driver
that will be probed by the TEE bus. If the associated Trusted
application is supported on secure part this driver offers a client
interface to load a firmware in the secure part.
This firmware could be authenticated by the secure trusted application.

Signed-off-by: Arnaud Pouliquen <[email protected]>
---
update from V4
- fix commit message,
- fix Kconfig typo,
- introduce tee_rproc_release_loaded_rsc_table function to release the
resource table,
- reorder function variables in declaration in reverse ascending order,
- introduce try_module_get and module_put to prevent module removed while
used,
- remove rsc_table field in tee_rproc structure,
- remove tee_rproc_find_loaded_rsc_table as seems not correspond to the
propoer usage regarding ops definition [1]. The resource table is
loaded before used,
- add __force attribute when cast the type aof the resource table to fix
build warning.

[1]https://elixir.bootlin.com/linux/latest/source/include/linux/remoteproc.h#L374
---
drivers/remoteproc/Kconfig | 10 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/tee_remoteproc.c | 429 ++++++++++++++++++++++++++++
include/linux/remoteproc.h | 4 +
include/linux/tee_remoteproc.h | 99 +++++++
5 files changed, 543 insertions(+)
create mode 100644 drivers/remoteproc/tee_remoteproc.c
create mode 100644 include/linux/tee_remoteproc.h

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48845dc8fa85..6c1c07202276 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -365,6 +365,16 @@ config XLNX_R5_REMOTEPROC

It's safe to say N if not interested in using RPU r5f cores.

+
+config TEE_REMOTEPROC
+ tristate "Remoteproc support by a TEE application"
+ depends on OPTEE
+ help
+ Support a remote processor with a TEE application. The Trusted
+ Execution Context is responsible for loading the trusted firmware
+ image and managing the remote processor's lifecycle.
+ This can be either built-in or a loadable module.
+
endif # REMOTEPROC

endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 91314a9b43ce..fa8daebce277 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_RCAR_REMOTEPROC) += rcar_rproc.o
obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
+obj-$(CONFIG_TEE_REMOTEPROC) += tee_remoteproc.o
obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
diff --git a/drivers/remoteproc/tee_remoteproc.c b/drivers/remoteproc/tee_remoteproc.c
new file mode 100644
index 000000000000..f13546628ec9
--- /dev/null
+++ b/drivers/remoteproc/tee_remoteproc.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) STMicroelectronics 2024 - All Rights Reserved
+ * Author: Arnaud Pouliquen <[email protected]>
+ */
+
+#include <linux/firmware.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/remoteproc.h>
+#include <linux/slab.h>
+#include <linux/tee_drv.h>
+#include <linux/tee_remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define MAX_TEE_PARAM_ARRY_MEMBER 4
+
+/*
+ * Authentication of the firmware and load in the remote processor memory
+ *
+ * [in] params[0].value.a: unique 32bit identifier of the remote processor
+ * [in] params[1].memref: buffer containing the image of the buffer
+ */
+#define TA_RPROC_FW_CMD_LOAD_FW 1
+
+/*
+ * Start the remote processor
+ *
+ * [in] params[0].value.a: unique 32bit identifier of the remote processor
+ */
+#define TA_RPROC_FW_CMD_START_FW 2
+
+/*
+ * Stop the remote processor
+ *
+ * [in] params[0].value.a: unique 32bit identifier of the remote processor
+ */
+#define TA_RPROC_FW_CMD_STOP_FW 3
+
+/*
+ * Return the address of the resource table, or 0 if not found
+ * No check is done to verify that the address returned is accessible by
+ * the non secure context. If the resource table is loaded in a protected
+ * memory the access by the non secure context will lead to a data abort.
+ *
+ * [in] params[0].value.a: unique 32bit identifier of the remote processor
+ * [out] params[1].value.a: 32bit LSB resource table memory address
+ * [out] params[1].value.b: 32bit MSB resource table memory address
+ * [out] params[2].value.a: 32bit LSB resource table memory size
+ * [out] params[2].value.b: 32bit MSB resource table memory size
+ */
+#define TA_RPROC_FW_CMD_GET_RSC_TABLE 4
+
+/*
+ * Return the address of the core dump
+ *
+ * [in] params[0].value.a: unique 32bit identifier of the remote processor
+ * [out] params[1].memref: address of the core dump image if exist,
+ * else return Null
+ */
+#define TA_RPROC_FW_CMD_GET_COREDUMP 5
+
+struct tee_rproc_context {
+ struct list_head sessions;
+ struct tee_context *tee_ctx;
+ struct device *dev;
+};
+
+static struct tee_rproc_context *tee_rproc_ctx;
+
+static void tee_rproc_prepare_args(struct tee_rproc *trproc, int cmd,
+ struct tee_ioctl_invoke_arg *arg,
+ struct tee_param *param,
+ unsigned int num_params)
+{
+ memset(arg, 0, sizeof(*arg));
+ memset(param, 0, MAX_TEE_PARAM_ARRY_MEMBER * sizeof(*param));
+
+ arg->func = cmd;
+ arg->session = trproc->session_id;
+ arg->num_params = num_params + 1;
+
+ param[0] = (struct tee_param) {
+ .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ .u.value.a = trproc->rproc_id,
+ };
+}
+
+static void tee_rproc_release_loaded_rsc_table(struct rproc *rproc)
+{
+ if (rproc->table_ptr) {
+ iounmap((__force __iomem void *)rproc->table_ptr);
+ /*
+ * Use a copy of the resource table for the remainder of the
+ * shutdown or recovery process.
+ */
+ rproc->table_ptr = rproc->cached_table;
+ }
+}
+
+int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = rproc->tee_interface;
+ struct tee_ioctl_invoke_arg arg;
+ struct tee_shm *fw_shm;
+ int ret;
+
+ if (!trproc)
+ return -EINVAL;
+
+ fw_shm = tee_shm_register_kernel_buf(tee_rproc_ctx->tee_ctx, (void *)fw->data, fw->size);
+ if (IS_ERR(fw_shm))
+ return PTR_ERR(fw_shm);
+
+ tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_LOAD_FW, &arg, param, 1);
+
+ /* Provide the address of the firmware image */
+ param[1] = (struct tee_param) {
+ .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
+ .u.memref = {
+ .shm = fw_shm,
+ .size = fw->size,
+ .shm_offs = 0,
+ },
+ };
+
+ ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
+ if (ret < 0 || arg.ret != 0) {
+ dev_err(tee_rproc_ctx->dev,
+ "TA_RPROC_FW_CMD_LOAD_FW invoke failed TEE err: %x, ret:%x\n",
+ arg.ret, ret);
+ if (!ret)
+ ret = -EIO;
+ }
+
+ tee_shm_free(fw_shm);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_load_fw);
+
+struct resource_table *tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
+{
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = rproc->tee_interface;
+ struct resource_table *rsc_table;
+ struct tee_ioctl_invoke_arg arg;
+ int ret;
+
+ if (!trproc)
+ return ERR_PTR(-EINVAL);
+
+ tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_GET_RSC_TABLE, &arg, param, 2);
+
+ param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+ param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+ ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
+ if (ret < 0 || arg.ret != 0) {
+ dev_err(tee_rproc_ctx->dev,
+ "TA_RPROC_FW_CMD_GET_RSC_TABLE invoke failed TEE err: %x, ret:%x\n",
+ arg.ret, ret);
+ return ERR_PTR(-EIO);
+ }
+
+ *table_sz = param[2].u.value.a;
+
+ /* If the size is null no resource table defined in the image */
+ if (!*table_sz)
+ return NULL;
+
+ /* Store the resource table address that would be updated by the remote core. */
+ rsc_table = (__force struct resource_table *)ioremap_wc(param[1].u.value.a, *table_sz);
+ if (IS_ERR_OR_NULL(rsc_table)) {
+ dev_err(tee_rproc_ctx->dev, "Unable to map memory region: %lld+%zx\n",
+ param[1].u.value.a, *table_sz);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ return rsc_table;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
+
+int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ struct resource_table *rsc_table;
+ size_t table_sz;
+ int ret;
+
+ ret = tee_rproc_load_fw(rproc, fw);
+ if (ret)
+ return ret;
+
+ rsc_table = rproc_get_loaded_rsc_table(rproc, &table_sz);
+ if (IS_ERR(rsc_table))
+ return PTR_ERR(rsc_table);
+
+ rproc->table_ptr = rsc_table;
+ rproc->table_sz = table_sz;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_parse_fw);
+
+int tee_rproc_start(struct rproc *rproc)
+{
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = rproc->tee_interface;
+ struct tee_ioctl_invoke_arg arg;
+ int ret = 0;
+
+ if (!trproc) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_START_FW, &arg, param, 0);
+
+ ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
+ if (ret < 0 || arg.ret != 0) {
+ dev_err(tee_rproc_ctx->dev,
+ "TA_RPROC_FW_CMD_START_FW invoke failed TEE err: %x, ret:%x\n",
+ arg.ret, ret);
+ if (!ret)
+ ret = -EIO;
+ goto err;
+ }
+
+ return 0;
+
+err:
+ tee_rproc_release_loaded_rsc_table(rproc);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_start);
+
+int tee_rproc_stop(struct rproc *rproc)
+{
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = rproc->tee_interface;
+ struct tee_ioctl_invoke_arg arg;
+ int ret;
+
+ if (!trproc)
+ return -EINVAL;
+
+ tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_STOP_FW, &arg, param, 0);
+
+ ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
+ if (ret < 0 || arg.ret != 0) {
+ dev_err(tee_rproc_ctx->dev,
+ "TA_RPROC_FW_CMD_STOP_FW invoke failed TEE err: %x, ret:%x\n",
+ arg.ret, ret);
+ if (!ret)
+ ret = -EIO;
+ }
+
+ tee_rproc_release_loaded_rsc_table(rproc);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_stop);
+
+static const struct tee_client_device_id stm32_tee_rproc_id_table[] = {
+ {UUID_INIT(0x80a4c275, 0x0a47, 0x4905,
+ 0x82, 0x85, 0x14, 0x86, 0xa9, 0x77, 0x1a, 0x08)},
+ {}
+};
+
+struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc, unsigned int rproc_id)
+{
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_ioctl_open_session_arg sess_arg;
+ struct tee_client_device *tee_device;
+ struct tee_rproc *trproc, *p_err;
+ int ret;
+
+ /*
+ * Test if the device has been probed by the TEE bus. In case of failure, we ignore the
+ * reason. The bus could be not yet probed or the service not available in the secure
+ * firmware.The assumption in such a case is that the TEE remoteproc is not probed.
+ */
+ if (!tee_rproc_ctx)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ /* Prevent tee rproc module from being removed */
+ if (!try_module_get(THIS_MODULE)) {
+ dev_err(tee_rproc_ctx->dev, "can't get owner\n");
+ p_err = ERR_PTR(-ENODEV);
+ goto module_put;
+ }
+
+ trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
+ if (!trproc) {
+ p_err = ERR_PTR(-ENOMEM);
+ goto module_put;
+ }
+ tee_device = to_tee_client_device(tee_rproc_ctx->dev);
+ memset(&sess_arg, 0, sizeof(sess_arg));
+
+ memcpy(sess_arg.uuid, tee_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
+
+ sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+ sess_arg.num_params = 1;
+
+ param[0] = (struct tee_param) {
+ .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ .u.value.a = rproc_id,
+ };
+
+ ret = tee_client_open_session(tee_rproc_ctx->tee_ctx, &sess_arg, param);
+ if (ret < 0 || sess_arg.ret != 0) {
+ dev_err(dev, "tee_client_open_session failed, err: %x\n", sess_arg.ret);
+ p_err = ERR_PTR(-EINVAL);
+ goto module_put;
+ }
+
+ trproc->parent = dev;
+ trproc->rproc_id = rproc_id;
+ trproc->session_id = sess_arg.session;
+
+ trproc->rproc = rproc;
+ rproc->tee_interface = trproc;
+
+ list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
+
+ return trproc;
+
+module_put:
+ module_put(THIS_MODULE);
+ return p_err;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_register);
+
+int tee_rproc_unregister(struct tee_rproc *trproc)
+{
+ struct rproc *rproc = trproc->rproc;
+ int ret;
+
+ ret = tee_client_close_session(tee_rproc_ctx->tee_ctx, trproc->session_id);
+ if (ret < 0)
+ dev_err(trproc->parent, "tee_client_close_session failed, err: %x\n", ret);
+
+ list_del(&trproc->node);
+ rproc->tee_interface = NULL;
+
+ module_put(THIS_MODULE);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_unregister);
+
+static int tee_rproc_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 tee_rproc_probe(struct device *dev)
+{
+ struct tee_context *tee_ctx;
+ int ret;
+
+ /* Open context with TEE driver */
+ tee_ctx = tee_client_open_context(NULL, tee_rproc_ctx_match, NULL, NULL);
+ if (IS_ERR(tee_ctx))
+ return PTR_ERR(tee_ctx);
+
+ tee_rproc_ctx = devm_kzalloc(dev, sizeof(*tee_ctx), GFP_KERNEL);
+ if (!tee_rproc_ctx) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ tee_rproc_ctx->dev = dev;
+ tee_rproc_ctx->tee_ctx = tee_ctx;
+ INIT_LIST_HEAD(&tee_rproc_ctx->sessions);
+
+ return 0;
+err:
+ tee_client_close_context(tee_ctx);
+
+ return ret;
+}
+
+static int tee_rproc_remove(struct device *dev)
+{
+ struct tee_rproc *entry, *tmp;
+
+ list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
+ tee_client_close_session(tee_rproc_ctx->tee_ctx, entry->session_id);
+ list_del(&entry->node);
+ kfree(entry);
+ }
+
+ tee_client_close_context(tee_rproc_ctx->tee_ctx);
+
+ return 0;
+}
+
+MODULE_DEVICE_TABLE(tee, stm32_tee_rproc_id_table);
+
+static struct tee_client_driver tee_rproc_fw_driver = {
+ .id_table = stm32_tee_rproc_id_table,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .bus = &tee_bus_type,
+ .probe = tee_rproc_probe,
+ .remove = tee_rproc_remove,
+ },
+};
+
+static int __init tee_rproc_fw_mod_init(void)
+{
+ return driver_register(&tee_rproc_fw_driver.driver);
+}
+
+static void __exit tee_rproc_fw_mod_exit(void)
+{
+ driver_unregister(&tee_rproc_fw_driver.driver);
+}
+
+module_init(tee_rproc_fw_mod_init);
+module_exit(tee_rproc_fw_mod_exit);
+
+MODULE_DESCRIPTION(" TEE remote processor control driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index b4795698d8c2..8b678009e481 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -503,6 +503,8 @@ enum rproc_features {
RPROC_MAX_FEATURES,
};

+struct tee_rproc;
+
/**
* struct rproc - represents a physical remote processor device
* @node: list node of this rproc object
@@ -545,6 +547,7 @@ enum rproc_features {
* @cdev: character device of the rproc
* @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
* @features: indicate remoteproc features
+ * @tee_interface: pointer to the remoteproc tee context
*/
struct rproc {
struct list_head node;
@@ -586,6 +589,7 @@ struct rproc {
struct cdev cdev;
bool cdev_put_on_release;
DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
+ struct tee_rproc *tee_interface;
};

/**
diff --git a/include/linux/tee_remoteproc.h b/include/linux/tee_remoteproc.h
new file mode 100644
index 000000000000..7fbb5c3423a8
--- /dev/null
+++ b/include/linux/tee_remoteproc.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright(c) 2024 STMicroelectronics - All Rights Reserved
+ */
+
+#ifndef TEE_REMOTEPROC_H
+#define TEE_REMOTEPROC_H
+
+#include <linux/tee_drv.h>
+#include <linux/firmware.h>
+#include <linux/remoteproc.h>
+
+struct rproc;
+
+/**
+ * struct tee_rproc - TEE remoteproc structure
+ * @node: Reference in list
+ * @rproc: Remoteproc reference
+ * @parent: Parent device
+ * @rproc_id: Identifier of the target firmware
+ * @session_id: TEE session identifier
+ */
+struct tee_rproc {
+ struct list_head node;
+ struct rproc *rproc;
+ struct device *parent;
+ u32 rproc_id;
+ u32 session_id;
+};
+
+#if IS_REACHABLE(CONFIG_TEE_REMOTEPROC)
+
+struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc,
+ unsigned int rproc_id);
+int tee_rproc_unregister(struct tee_rproc *trproc);
+int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw);
+int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw);
+struct resource_table *tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz);
+int tee_rproc_start(struct rproc *rproc);
+int tee_rproc_stop(struct rproc *rproc);
+
+#else
+
+static inline struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc,
+ unsigned int rproc_id)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+static inline int tee_rproc_unregister(struct tee_rproc *trproc)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+static inline int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+static inline int tee_rproc_start(struct rproc *rproc)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+static inline int tee_rproc_stop(struct rproc *rproc)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+static inline struct resource_table *
+tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return NULL;
+}
+#endif /* CONFIG_TEE_REMOTEPROC */
+#endif /* TEE_REMOTEPROC_H */
--
2.25.1


2024-05-28 21:04:35

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH v5 4/7] remoteproc: core introduce rproc_set_rsc_table_on_start function

On Tue, May 21, 2024 at 10:09:58AM +0200, Arnaud Pouliquen wrote:
> Split rproc_start()to prepare the update of the management of

I don't see any "splitting" for rproc_start() in this patch. Please consider
rewording or removing.

> the cache table on start, for the support of the firmware loading
> by the TEE interface.
> - create rproc_set_rsc_table_on_start() to address the management of
> the cache table in a specific function, as done in
> rproc_reset_rsc_table_on_stop().
> - rename rproc_set_rsc_table in rproc_set_rsc_table_on_attach()
> - move rproc_reset_rsc_table_on_stop() to be close to the
> rproc_set_rsc_table_on_start() function

This patch is really hard to read due to all 3 operations happening at the same
time. Please split in 3 smaller patches.

>
> Suggested-by: Mathieu Poirier <[email protected]>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 116 ++++++++++++++-------------
> 1 file changed, 62 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f276956f2c5c..42bca01f3bde 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1264,18 +1264,9 @@ void rproc_resource_cleanup(struct rproc *rproc)
> }
> EXPORT_SYMBOL(rproc_resource_cleanup);
>
> -static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> +static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct firmware *fw)
> {
> struct resource_table *loaded_table;
> - struct device *dev = &rproc->dev;
> - int ret;
> -
> - /* load the ELF segments to memory */
> - ret = rproc_load_segments(rproc, fw);
> - if (ret) {
> - dev_err(dev, "Failed to load program segments: %d\n", ret);
> - return ret;
> - }
>
> /*
> * The starting device has been given the rproc->cached_table as the
> @@ -1291,6 +1282,64 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> rproc->table_ptr = loaded_table;
> }
>
> + return 0;
> +}
> +
> +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> +{
> + /* A resource table was never retrieved, nothing to do here */
> + if (!rproc->table_ptr)
> + return 0;
> +
> + /*
> + * If a cache table exists the remote processor was started by
> + * the remoteproc core. That cache table should be used for
> + * the rest of the shutdown process.
> + */
> + if (rproc->cached_table)
> + goto out;
> +
> + /*
> + * If we made it here the remote processor was started by another
> + * entity and a cache table doesn't exist. As such make a copy of
> + * the resource table currently used by the remote processor and
> + * use that for the rest of the shutdown process. The memory
> + * allocated here is free'd in rproc_shutdown().
> + */
> + rproc->cached_table = kmemdup(rproc->table_ptr,
> + rproc->table_sz, GFP_KERNEL);
> + if (!rproc->cached_table)
> + return -ENOMEM;
> +
> + /*
> + * Since the remote processor is being switched off the clean table
> + * won't be needed. Allocated in rproc_set_rsc_table_on_start().
> + */
> + kfree(rproc->clean_table);
> +
> +out:
> + /*
> + * Use a copy of the resource table for the remainder of the
> + * shutdown process.
> + */
> + rproc->table_ptr = rproc->cached_table;
> + return 0;
> +}
> +
> +static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct device *dev = &rproc->dev;
> + int ret;
> +
> + /* load the ELF segments to memory */
> + ret = rproc_load_segments(rproc, fw);
> + if (ret) {
> + dev_err(dev, "Failed to load program segments: %d\n", ret);
> + return ret;
> + }
> +
> + rproc_set_rsc_table_on_start(rproc, fw);
> +
> ret = rproc_prepare_subdevices(rproc);
> if (ret) {
> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> @@ -1450,7 +1499,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> return ret;
> }
>
> -static int rproc_set_rsc_table(struct rproc *rproc)
> +static int rproc_set_rsc_table_on_attach(struct rproc *rproc)
> {
> struct resource_table *table_ptr;
> struct device *dev = &rproc->dev;
> @@ -1540,54 +1589,13 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
>
> /*
> * The clean resource table is no longer needed. Allocated in
> - * rproc_set_rsc_table().
> + * rproc_set_rsc_table_on_attach().
> */
> kfree(rproc->clean_table);
>
> return 0;
> }
>
> -static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> -{
> - /* A resource table was never retrieved, nothing to do here */
> - if (!rproc->table_ptr)
> - return 0;
> -
> - /*
> - * If a cache table exists the remote processor was started by
> - * the remoteproc core. That cache table should be used for
> - * the rest of the shutdown process.
> - */
> - if (rproc->cached_table)
> - goto out;
> -
> - /*
> - * If we made it here the remote processor was started by another
> - * entity and a cache table doesn't exist. As such make a copy of
> - * the resource table currently used by the remote processor and
> - * use that for the rest of the shutdown process. The memory
> - * allocated here is free'd in rproc_shutdown().
> - */
> - rproc->cached_table = kmemdup(rproc->table_ptr,
> - rproc->table_sz, GFP_KERNEL);
> - if (!rproc->cached_table)
> - return -ENOMEM;
> -
> - /*
> - * Since the remote processor is being switched off the clean table
> - * won't be needed. Allocated in rproc_set_rsc_table().
> - */
> - kfree(rproc->clean_table);
> -
> -out:
> - /*
> - * Use a copy of the resource table for the remainder of the
> - * shutdown process.
> - */
> - rproc->table_ptr = rproc->cached_table;
> - return 0;
> -}
> -
> /*
> * Attach to remote processor - similar to rproc_fw_boot() but without
> * the steps that deal with the firmware image.
> @@ -1614,7 +1622,7 @@ static int rproc_attach(struct rproc *rproc)
> goto disable_iommu;
> }
>
> - ret = rproc_set_rsc_table(rproc);
> + ret = rproc_set_rsc_table_on_attach(rproc);
> if (ret) {
> dev_err(dev, "can't load resource table: %d\n", ret);
> goto unprepare_device;
> --
> 2.25.1
>