Updates from the previous version [1]:
This version proposes another approach based on an alternate load and boot
of the coprocessor. Therefore, the constraint introduced by tee_remoteproc
is that the firmware has to be authenticated and loaded before the resource
table can be obtained.
The existing boot sequence is:
1) Get the resource table and store it in a cache,
calling rproc->ops->parse_fw().
2) Parse the resource table and handle resources,
calling rproc_handle_resources.
3) Load the firmware, calling rproc->ops->load().
4) Start the firmware, calling rproc->ops->start().
=> Steps 1 and 2 are executed in rproc_fw_boot(), while steps 3 and 4 are
executed in rproc_start().
=> the use of rproc->ops->load() ops is mandatory
The boot sequence needed for TEE boot is:
1) Load the firmware.
2) Get the loaded resource, no cache.
3) Parse the resource table and handle resources.
4) Start the firmware.
Then the crash recovery also has to be managed.For recovery, the cache is
used to temporarily save the resource table and then reapply it on
restart:
1) Stop the remote processor, calling rproc->ops->stop().
2) Load the firmware, calling rproc->ops->load().
3) Copy cached resource table.
4) Start the remote processor, calling rproc->ops->start().
=> This sequence is also needed when TEE manages the boot of the remote
processor.
=> The rproc->ops->load() is also used in recovery sequence.
Based on the sequences described above, the proposal is to:
- Rework tee_rproc API to better match the rproc_ops structure.
This allows to simply map the function to implement the load ops, which
is not optional. The tee_rproc_load_fw() is updated in consequence.
- Remove the call of rproc_load_segments from rproc_start() to dissociate
the load and the start. This is necessary to implement the boot sequence
requested for the TEE remote proc support.
- Introduce an rproc_alt_fw_boot() function that is an alternative boot
sequence, which implements the sequence requested for the TEE remoteproc
support.
[1] https://lore.kernel.org/lkml/[email protected]/T/
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
remoteproc: Extract the firmware load from the start
remoteproc: core: Add check on cached_table pointer
remoteproc: core: Implement the support of an alternative boot
dt-bindings: remoteproc: Add compatibility for TEE support
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 | 51 ++-
drivers/remoteproc/Kconfig | 9 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_core.c | 109 ++++-
drivers/remoteproc/stm32_rproc.c | 169 ++++++--
drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++
include/linux/remoteproc.h | 2 +
include/linux/tee_remoteproc.h | 102 +++++
8 files changed, 784 insertions(+), 56 deletions(-)
create mode 100644 drivers/remoteproc/tee_remoteproc.c
create mode 100644 include/linux/tee_remoteproc.h
--
2.25.1
From: Arnaud Pouliquen <[email protected]>
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 device offers a client
interface to load a firmware in the secure part.
This firmware could be authenticated and decrypted by the secure
trusted application.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
update from V2
- Use 'tee_rproc' prefix for all functions
- rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
- redefine fonction to better match with the rproc_ops structure format
- replace 'struct tee_rproc' parameter by 'struct rproc' parameter
- rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
and rework it to remove the cached_table management.
- introduce tee_rproc_get_context() to get the tee_rproc struct from the
rproc struct
- rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
- remove useless check on tee_rproc_ctx structure in tee_rproc_register()
and tee_rproc_unregister()
- fix test on the return of tee_rproc_ctx = devm_kzalloc()
- remove useless includes and unused tee_rproc_mem structure.
---
drivers/remoteproc/Kconfig | 9 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
include/linux/tee_remoteproc.h | 102 +++++++
4 files changed, 509 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..85299606806c 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
It's safe to say N if not interested in using RPU r5f cores.
+
+config TEE_REMOTEPROC
+ tristate "trusted firmware support by a TEE application"
+ depends on OPTEE
+ help
+ Support for trusted remote processors firmware. The firmware
+ authentication and/or decryption are managed by a trusted application.
+ 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..ac727e062d00
--- /dev/null
+++ b/drivers/remoteproc/tee_remoteproc.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) STMicroelectronics 2023 - 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 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
+{
+ struct tee_rproc *entry, *tmp;
+
+ list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
+ if (entry->rproc == rproc)
+ return entry;
+ }
+
+ return NULL;
+}
+
+int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ struct tee_ioctl_invoke_arg arg;
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = tee_rproc_get_context(rproc);
+ 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);
+
+ 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_ioctl_invoke_arg arg;
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = tee_rproc_get_context(rproc);
+ int ret;
+
+ if (!trproc)
+ return ERR_PTR(-EINVAL);
+
+ 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. */
+ trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
+ if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
+
+struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
+ const struct firmware *fw)
+{
+ struct tee_rproc *trproc = tee_rproc_get_context(rproc);
+ size_t table_sz;
+
+ if (!trproc)
+ return ERR_PTR(-EINVAL);
+
+ if (!trproc->rsc_table)
+ trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
+
+ return trproc->rsc_table;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
+
+int tee_rproc_start(struct rproc *rproc)
+{
+ struct tee_ioctl_invoke_arg arg;
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = tee_rproc_get_context(rproc);
+ int ret;
+
+ if (!trproc)
+ return -EINVAL;
+
+ 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;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_start);
+
+int tee_rproc_stop(struct rproc *rproc)
+{
+ struct tee_ioctl_invoke_arg arg;
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc = tee_rproc_get_context(rproc);
+ int ret;
+
+ if (!trproc)
+ return -EINVAL;
+
+ 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;
+ }
+ if (trproc->rsc_table)
+ iounmap(trproc->rsc_table);
+ trproc->rsc_table = NULL;
+
+ 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, unsigned int rproc_id)
+{
+ struct tee_client_device *tee_device;
+ struct tee_ioctl_open_session_arg sess_arg;
+ struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
+ struct tee_rproc *trproc;
+ int ret;
+
+ /*
+ * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
+ * probed or service not available in the secure firmware)
+ * Assumption here is that the TEE bus is not probed.
+ */
+ if (!tee_rproc_ctx)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
+ if (!trproc)
+ return ERR_PTR(-ENOMEM);
+
+ tee_device = to_tee_client_device(tee_rproc_ctx->dev);
+ memset(&sess_arg, 0, sizeof(sess_arg));
+
+ /* Open session with rproc_tee load the OP-TEE Trusted Application */
+ 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);
+ return ERR_PTR(-EINVAL);
+ }
+
+ trproc->parent = dev;
+ trproc->rproc_id = rproc_id;
+ trproc->session_id = sess_arg.session;
+
+ list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
+
+ return trproc;
+}
+EXPORT_SYMBOL_GPL(tee_rproc_register);
+
+int tee_rproc_unregister(struct tee_rproc *trproc)
+{
+ 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);
+
+ 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
new file mode 100644
index 000000000000..7c9e91e989ba
--- /dev/null
+++ b/include/linux/tee_remoteproc.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright(c) 2023 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
+ * @rsc_table: Resource table virtual address.
+ */
+struct tee_rproc {
+ struct list_head node;
+ struct rproc *rproc;
+ struct device *parent;
+ u32 rproc_id;
+ u32 session_id;
+ struct resource_table *rsc_table;
+};
+
+#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
+
+struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
+int tee_rproc_unregister(struct tee_rproc *trproc);
+
+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);
+struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
+ const struct firmware *fw);
+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, unsigned int rproc_id)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+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;
+}
+
+static inline struct resource_table *
+tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return NULL;
+}
+#endif /* CONFIG_TEE_REMOTEPROC */
+#endif /* TEE_REMOTEPROC_H */
--
2.25.1
To prepare for the support of TEE remoteproc, create sub-functions
that can be used in both cases, with and without 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 4f469f0bcf8b..fcc0001e2657 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 err;
+}
+
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
The new TEE remoteproc device is used to manage remote firmware in a
secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
introduced to delegate the loading of the firmware to the trusted
execution context. In such cases, the firmware should be signed and
adhere to the image format defined by the TEE.
A new "to_attach" field is introduced to differentiate the use cases
"firmware loaded by the boot stage" and "firmware loaded by the TEE".
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
V2 to V3 update:
- remove stm32_rproc_tee_elf_sanity_check(), stm32_rproc_tee_elf_load()
stm32_rproc_tee_elf_find_loaded_rsc_table() and stm32_rproc_tee_start() that are bnow unused
- use new rproc::alt_boot field to sepcify that the alternate fboot method is used
- use stm32_rproc::to_attach field to differenciate attch mode from remoteproc tee boot mode.
- remove the used of stm32_rproc::fw_loaded
---
drivers/remoteproc/stm32_rproc.c | 85 +++++++++++++++++++++++++++++---
1 file changed, 79 insertions(+), 6 deletions(-)
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index fcc0001e2657..9cfcf66462e0 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -20,6 +20,7 @@
#include <linux/remoteproc.h>
#include <linux/reset.h>
#include <linux/slab.h>
+#include <linux/tee_remoteproc.h>
#include <linux/workqueue.h>
#include "remoteproc_internal.h"
@@ -49,6 +50,9 @@
#define M4_STATE_STANDBY 4
#define M4_STATE_CRASH 5
+/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
+#define STM32_MP1_M4_PROC_ID 0
+
struct stm32_syscon {
struct regmap *map;
u32 reg;
@@ -90,6 +94,8 @@ struct stm32_rproc {
struct stm32_mbox mb[MBOX_NB_MBX];
struct workqueue_struct *workqueue;
bool hold_boot_smc;
+ bool to_attach;
+ struct tee_rproc *trproc;
void __iomem *rsc_va;
};
@@ -253,10 +259,30 @@ static int stm32_rproc_release(struct rproc *rproc)
return err;
}
}
+ ddata->to_attach = false;
return err;
}
+static int stm32_rproc_tee_attach(struct rproc *rproc)
+{
+ /* Nothing to do, remote proc already started by the secured context. */
+ return 0;
+}
+
+static int stm32_rproc_tee_stop(struct rproc *rproc)
+{
+ int err;
+
+ stm32_rproc_request_shutdown(rproc);
+
+ err = tee_rproc_stop(rproc);
+ if (err)
+ return err;
+
+ return stm32_rproc_release(rproc);
+}
+
static int stm32_rproc_prepare(struct rproc *rproc)
{
struct device *dev = rproc->dev.parent;
@@ -637,10 +663,14 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
{
struct stm32_rproc *ddata = rproc->priv;
struct device *dev = rproc->dev.parent;
+ struct tee_rproc *trproc = ddata->trproc;
phys_addr_t rsc_pa;
u32 rsc_da;
int err;
+ if (trproc && !ddata->to_attach)
+ return tee_rproc_get_loaded_rsc_table(rproc, table_sz);
+
/* The resource table has already been mapped, nothing to do */
if (ddata->rsc_va)
goto done;
@@ -693,8 +723,20 @@ static const struct rproc_ops st_rproc_ops = {
.get_boot_addr = rproc_elf_get_boot_addr,
};
+static const struct rproc_ops st_rproc_tee_ops = {
+ .prepare = stm32_rproc_prepare,
+ .start = tee_rproc_start,
+ .stop = stm32_rproc_tee_stop,
+ .attach = stm32_rproc_tee_attach,
+ .kick = stm32_rproc_kick,
+ .get_loaded_rsc_table = stm32_rproc_get_loaded_rsc_table,
+ .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
+ .load = tee_rproc_load_fw,
+};
+
static const struct of_device_id stm32_rproc_match[] = {
- { .compatible = "st,stm32mp1-m4" },
+ {.compatible = "st,stm32mp1-m4",},
+ {.compatible = "st,stm32mp1-m4-tee",},
{},
};
MODULE_DEVICE_TABLE(of, stm32_rproc_match);
@@ -853,6 +895,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct stm32_rproc *ddata;
struct device_node *np = dev->of_node;
+ struct tee_rproc *trproc = NULL;
struct rproc *rproc;
unsigned int state;
int ret;
@@ -861,12 +904,33 @@ static int stm32_rproc_probe(struct platform_device *pdev)
if (ret)
return ret;
- rproc = rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
- if (!rproc)
- return -ENOMEM;
+ if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
+ /*
+ * Delegate the firmware management to the secure context.
+ * The firmware loaded has to be signed.
+ */
+ trproc = tee_rproc_register(dev, STM32_MP1_M4_PROC_ID);
+ if (IS_ERR(trproc)) {
+ dev_err_probe(dev, PTR_ERR(trproc),
+ "signed firmware not supported by TEE\n");
+ return PTR_ERR(trproc);
+ }
+ }
- ddata = rproc->priv;
+ rproc = rproc_alloc(dev, np->name,
+ trproc ? &st_rproc_tee_ops : &st_rproc_ops,
+ NULL, sizeof(*ddata));
+ if (!rproc) {
+ ret = -ENOMEM;
+ goto free_tee;
+ }
+ ddata = rproc->priv;
+ ddata->trproc = trproc;
+ if (trproc) {
+ rproc->alt_boot = true;
+ trproc->rproc = rproc;
+ }
rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
@@ -881,8 +945,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
- if (state == M4_STATE_CRUN)
+ if (state == M4_STATE_CRUN) {
rproc->state = RPROC_DETACHED;
+ ddata->to_attach = true;
+ }
rproc->has_iommu = false;
ddata->workqueue = create_workqueue(dev_name(dev));
@@ -916,6 +982,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
device_init_wakeup(dev, false);
}
rproc_free(rproc);
+free_tee:
+ if (trproc)
+ tee_rproc_unregister(trproc);
+
return ret;
}
@@ -923,6 +993,7 @@ static void stm32_rproc_remove(struct platform_device *pdev)
{
struct rproc *rproc = platform_get_drvdata(pdev);
struct stm32_rproc *ddata = rproc->priv;
+ struct tee_rproc *trproc = ddata->trproc;
struct device *dev = &pdev->dev;
if (atomic_read(&rproc->power) > 0)
@@ -937,6 +1008,8 @@ static void stm32_rproc_remove(struct platform_device *pdev)
device_init_wakeup(dev, false);
}
rproc_free(rproc);
+ if (trproc)
+ tee_rproc_unregister(trproc);
}
static int stm32_rproc_suspend(struct device *dev)
--
2.25.1
Add a check on the optional rproc->cached_table to perform the memory
copy only if it is not null.
2 use cases to support:
- starting on boot, in which case rproc->cached_table can be null,
- starting on crash recovery, where the cached table is used to save
the resource table configuration on stop and re-apply the configuration
on the re-start.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 283ca071e35c..34b0093689da 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1278,7 +1278,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
* that any subsequent changes will be applied to the loaded version.
*/
loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
- if (loaded_table) {
+ if (loaded_table && rproc->cached_table) {
memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
rproc->table_ptr = loaded_table;
}
--
2.25.1
The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
where the Cortex-M4 firmware is loaded by the Trusted execution Environment
(TEE).
For instance, this compatible is used in both the Linux and OP-TEE
device-tree:
- In OP-TEE, a node is defined in the device tree with the
st,stm32mp1-m4-tee to support signed remoteproc firmware.
Based on DT properties, OP-TEE authenticates, loads, starts, and stops
the firmware.
- On Linux, when the compatibility is set, the Cortex-M resets should not
be declared in the device tree.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
V1 to V2 updates
- update "st,stm32mp1-m4" compatible description to generalize
- remove the 'reset-names' requirement in one conditional branch, as the
property is already part of the condition test.
---
.../bindings/remoteproc/st,stm32-rproc.yaml | 51 ++++++++++++++++---
1 file changed, 43 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
index 370af61d8f28..36ea54016b76 100644
--- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
@@ -16,7 +16,12 @@ maintainers:
properties:
compatible:
- const: st,stm32mp1-m4
+ enum:
+ - st,stm32mp1-m4
+ - st,stm32mp1-m4-tee
+ description:
+ Use "st,stm32mp1-m4" for the Cortex-M4 coprocessor management by non-secure context
+ Use "st,stm32mp1-m4-tee" for the Cortex-M4 coprocessor management by secure context
reg:
description:
@@ -142,21 +147,41 @@ properties:
required:
- compatible
- reg
- - resets
allOf:
- if:
properties:
- reset-names:
- not:
- contains:
- const: hold_boot
+ compatible:
+ contains:
+ const: st,stm32mp1-m4
then:
+ if:
+ properties:
+ reset-names:
+ not:
+ contains:
+ const: hold_boot
+ then:
+ required:
+ - st,syscfg-holdboot
+ else:
+ properties:
+ st,syscfg-holdboot: false
+ required:
+ - reset-names
required:
- - st,syscfg-holdboot
- else:
+ - resets
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: st,stm32mp1-m4-tee
+ then:
properties:
st,syscfg-holdboot: false
+ reset-names: false
+ resets: false
additionalProperties: false
@@ -188,5 +213,15 @@ examples:
st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
};
+ - |
+ #include <dt-bindings/reset/stm32mp1-resets.h>
+ m4@10000000 {
+ compatible = "st,stm32mp1-m4-tee";
+ reg = <0x10000000 0x40000>,
+ <0x30000000 0x40000>,
+ <0x38000000 0x10000>;
+ st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
+ st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
+ };
...
--
2.25.1
Implement a new method to load a firmware and start the remote processor.
In this method the firmware is loaded first and then the loaded resource
table is obtained.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 86 +++++++++++++++++++++++++++-
include/linux/remoteproc.h | 2 +
2 files changed, 87 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 34b0093689da..47956e07365e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -38,6 +38,7 @@
#include <linux/virtio_ring.h>
#include <asm/byteorder.h>
#include <linux/platform_device.h>
+#include <linux/tee_remoteproc.h>
#include "remoteproc_internal.h"
@@ -1493,6 +1494,86 @@ static int rproc_set_rsc_table(struct rproc *rproc)
return 0;
}
+/*
+ * Alternative method to load a firmware and boot a remote processor with it.
+ * Similar to rproc_fw_boot but the resource table is obtained and parsed only after loading the
+ * firmware.
+ */
+static int rproc_alt_fw_boot(struct rproc *rproc, const struct firmware *fw)
+{
+ struct device *dev = &rproc->dev;
+ const char *name = rproc->firmware;
+ int ret;
+
+ dev_info(dev, "Booting a private format image %s, size %zd\n", name, fw->size);
+
+ /*
+ * if enabling an IOMMU isn't relevant for this rproc, this is
+ * just a nop
+ */
+ ret = rproc_enable_iommu(rproc);
+ if (ret) {
+ dev_err(dev, "can't enable iommu: %d\n", ret);
+ return ret;
+ }
+
+ /* Prepare rproc for firmware loading if needed */
+ ret = rproc_prepare_device(rproc);
+ if (ret) {
+ dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
+ goto disable_iommu;
+ }
+
+ /* load the image to memory */
+ ret = rproc_load_segments(rproc, fw);
+ if (ret) {
+ dev_err(dev, "Failed to load firmware: %d\n", ret);
+ return ret;
+ }
+
+ ret = rproc_set_rsc_table(rproc);
+ if (ret) {
+ dev_err(dev, "can't load resource table: %d\n", ret);
+ goto unprepare_device;
+ }
+
+ /* reset max_notifyid */
+ rproc->max_notifyid = -1;
+
+ /* reset handled vdev */
+ rproc->nb_vdev = 0;
+
+ /* handle fw resources which are required to boot rproc */
+ ret = rproc_handle_resources(rproc, rproc_loading_handlers);
+ if (ret) {
+ dev_err(dev, "Failed to process resources: %d\n", ret);
+ goto clean_up_resources;
+ }
+
+ /* Allocate carveout resources associated to rproc */
+ ret = rproc_alloc_registered_carveouts(rproc);
+ if (ret) {
+ dev_err(dev, "Failed to allocate associated carveouts: %d\n",
+ ret);
+ goto clean_up_resources;
+ }
+
+ ret = rproc_start(rproc, fw);
+ if (ret)
+ goto clean_up_resources;
+
+ return 0;
+
+clean_up_resources:
+ rproc_resource_cleanup(rproc);
+unprepare_device:
+ /* release HW resources if needed */
+ rproc_unprepare_device(rproc);
+disable_iommu:
+ rproc_disable_iommu(rproc);
+ return ret;
+}
+
static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
{
struct resource_table *table_ptr;
@@ -1957,7 +2038,10 @@ int rproc_boot(struct rproc *rproc)
goto downref_rproc;
}
- ret = rproc_fw_boot(rproc, firmware_p);
+ if (rproc->alt_boot)
+ ret = rproc_alt_fw_boot(rproc, firmware_p);
+ else
+ ret = rproc_fw_boot(rproc, firmware_p);
release_firmware(firmware_p);
}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index b4795698d8c2..ba219a77e055 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -544,6 +544,7 @@ enum rproc_features {
* @elf_machine: firmware ELF machine
* @cdev: character device of the rproc
* @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @alt_boot flag to indicate if remoteproc should use the alternate boot method.
* @features: indicate remoteproc features
*/
struct rproc {
@@ -585,6 +586,7 @@ struct rproc {
u16 elf_machine;
struct cdev cdev;
bool cdev_put_on_release;
+ bool alt_boot;
DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
};
--
2.25.1
Good morning,
On Wed, Feb 14, 2024 at 06:21:21PM +0100, Arnaud Pouliquen wrote:
> From: Arnaud Pouliquen <[email protected]>
>
> 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 device offers a client
> interface to load a firmware in the secure part.
> This firmware could be authenticated and decrypted by the secure
> trusted application.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> update from V2
> - Use 'tee_rproc' prefix for all functions
> - rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
> - redefine fonction to better match with the rproc_ops structure format
> - replace 'struct tee_rproc' parameter by 'struct rproc' parameter
> - rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
> and rework it to remove the cached_table management.
> - introduce tee_rproc_get_context() to get the tee_rproc struct from the
> rproc struct
> - rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
> - remove useless check on tee_rproc_ctx structure in tee_rproc_register()
> and tee_rproc_unregister()
> - fix test on the return of tee_rproc_ctx = devm_kzalloc()
> - remove useless includes and unused tee_rproc_mem structure.
> ---
> drivers/remoteproc/Kconfig | 9 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
> include/linux/tee_remoteproc.h | 102 +++++++
> 4 files changed, 509 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..85299606806c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
>
> It's safe to say N if not interested in using RPU r5f cores.
>
> +
> +config TEE_REMOTEPROC
> + tristate "trusted firmware support by a TEE application"
s/trusted/Trusted
And the wording will have to change a little, I will advise on this later.
> + depends on OPTEE
> + help
> + Support for trusted remote processors firmware. The firmware
> + authentication and/or decryption are managed by a trusted application.
> + 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..ac727e062d00
> --- /dev/null
> +++ b/drivers/remoteproc/tee_remoteproc.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) STMicroelectronics 2023 - 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 {
s/tee_rproc__context/tee_rproc_context
as it was in the previous patchset.
> + struct list_head sessions;
> + struct tee_context *tee_ctx;
> + struct device *dev;
> +};
> +
> +static struct tee_rproc__context *tee_rproc_ctx;
> +
> +static void 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
> +{
> + struct tee_rproc *entry, *tmp;
> +
> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
> + if (entry->rproc == rproc)
> + return entry;
> + }
> +
> + return NULL;
> +}
> +
> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct tee_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + 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);
> +
> + 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_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + int ret;
> +
> + if (!trproc)
> + return ERR_PTR(-EINVAL);
> +
> + 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. */
> + trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
> + if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
> +
> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> + const struct firmware *fw)
> +{
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + size_t table_sz;
> +
> + if (!trproc)
> + return ERR_PTR(-EINVAL);
> +
> + if (!trproc->rsc_table)
> + trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> +
> + return trproc->rsc_table;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
> +
> +int tee_rproc_start(struct rproc *rproc)
> +{
> + struct tee_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + int ret;
> +
> + if (!trproc)
> + return -EINVAL;
> +
> + 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;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_start);
> +
> +int tee_rproc_stop(struct rproc *rproc)
> +{
> + struct tee_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + int ret;
> +
> + if (!trproc)
> + return -EINVAL;
> +
> + 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;
> + }
> + if (trproc->rsc_table)
> + iounmap(trproc->rsc_table);
> + trproc->rsc_table = NULL;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_stop);
>
I was wondering where this ID is coming from - Is it the ID of the remote
processor service in the secure world or the ID of the program that provides the
service?
More comments tomorrow.
Thanks,
Mathieu
+
> +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, unsigned int rproc_id)
> +{
> + struct tee_client_device *tee_device;
> + struct tee_ioctl_open_session_arg sess_arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc;
> + int ret;
> +
> + /*
> + * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
> + * probed or service not available in the secure firmware)
> + * Assumption here is that the TEE bus is not probed.
> + */
> + if (!tee_rproc_ctx)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
> + if (!trproc)
> + return ERR_PTR(-ENOMEM);
> +
> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
> + memset(&sess_arg, 0, sizeof(sess_arg));
> +
> + /* Open session with rproc_tee load the OP-TEE Trusted Application */
> + 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);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + trproc->parent = dev;
> + trproc->rproc_id = rproc_id;
> + trproc->session_id = sess_arg.session;
> +
> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
> +
> + return trproc;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_register);
> +
> +int tee_rproc_unregister(struct tee_rproc *trproc)
> +{
> + 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);
> +
> + 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
> new file mode 100644
> index 000000000000..7c9e91e989ba
> --- /dev/null
> +++ b/include/linux/tee_remoteproc.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright(c) 2023 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
> + * @rsc_table: Resource table virtual address.
> + */
> +struct tee_rproc {
> + struct list_head node;
> + struct rproc *rproc;
> + struct device *parent;
> + u32 rproc_id;
> + u32 session_id;
> + struct resource_table *rsc_table;
> +};
> +
> +#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
> +
> +struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
> +int tee_rproc_unregister(struct tee_rproc *trproc);
> +
> +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);
> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> + const struct firmware *fw);
> +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, unsigned int rproc_id)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> +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;
> +}
> +
> +static inline struct resource_table *
> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> +{
> + /* This shouldn't be possible */
> + WARN_ON(1);
> +
> + return NULL;
> +}
> +#endif /* CONFIG_TEE_REMOTEPROC */
> +#endif /* TEE_REMOTEPROC_H */
> --
> 2.25.1
>
Hi Mathieu,
On 2/20/24 19:58, Mathieu Poirier wrote:
> Good morning,
>
> On Wed, Feb 14, 2024 at 06:21:21PM +0100, Arnaud Pouliquen wrote:
>> From: Arnaud Pouliquen <[email protected]>
>>
>> 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 device offers a client
>> interface to load a firmware in the secure part.
>> This firmware could be authenticated and decrypted by the secure
>> trusted application.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> update from V2
>> - Use 'tee_rproc' prefix for all functions
>> - rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
>> - redefine fonction to better match with the rproc_ops structure format
>> - replace 'struct tee_rproc' parameter by 'struct rproc' parameter
>> - rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
>> and rework it to remove the cached_table management.
>> - introduce tee_rproc_get_context() to get the tee_rproc struct from the
>> rproc struct
>> - rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
>> - remove useless check on tee_rproc_ctx structure in tee_rproc_register()
>> and tee_rproc_unregister()
>> - fix test on the return of tee_rproc_ctx = devm_kzalloc()
>> - remove useless includes and unused tee_rproc_mem structure.
>> ---
>> drivers/remoteproc/Kconfig | 9 +
>> drivers/remoteproc/Makefile | 1 +
>> drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
>> include/linux/tee_remoteproc.h | 102 +++++++
>> 4 files changed, 509 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..85299606806c 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
>>
>> It's safe to say N if not interested in using RPU r5f cores.
>>
>> +
>> +config TEE_REMOTEPROC
>> + tristate "trusted firmware support by a TEE application"
>
> s/trusted/Trusted
>
> And the wording will have to change a little, I will advise on this later.
>
>> + depends on OPTEE
>> + help
>> + Support for trusted remote processors firmware. The firmware
>> + authentication and/or decryption are managed by a trusted application.
>> + 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..ac727e062d00
>> --- /dev/null
>> +++ b/drivers/remoteproc/tee_remoteproc.c
>> @@ -0,0 +1,397 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) STMicroelectronics 2023 - 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 {
>
> s/tee_rproc__context/tee_rproc_context
I updated to tee_rproc_interface in a first update,as you advised. But
regarding the implementation I propose in this version, keeping the initial
name seems to me better. I have introduced the issue coming back to initial name.
>
> as it was in the previous patchset.
>
>> + struct list_head sessions;
>> + struct tee_context *tee_ctx;
>> + struct device *dev;
>> +};
>> +
>> +static struct tee_rproc__context *tee_rproc_ctx;
>> +
>> +static void 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
>> +{
>> + struct tee_rproc *entry, *tmp;
>> +
>> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
>> + if (entry->rproc == rproc)
>> + return entry;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + struct tee_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + 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);
>> +
>> + 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_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + int ret;
>> +
>> + if (!trproc)
>> + return ERR_PTR(-EINVAL);
>> +
>> + 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. */
>> + trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
>> + if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
>> +
>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
>> + const struct firmware *fw)
>> +{
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + size_t table_sz;
>> +
>> + if (!trproc)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (!trproc->rsc_table)
>> + trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
>> +
>> + return trproc->rsc_table;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
>> +
>> +int tee_rproc_start(struct rproc *rproc)
>> +{
>> + struct tee_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + int ret;
>> +
>> + if (!trproc)
>> + return -EINVAL;
>> +
>> + 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;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_start);
>> +
>> +int tee_rproc_stop(struct rproc *rproc)
>> +{
>> + struct tee_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + int ret;
>> +
>> + if (!trproc)
>> + return -EINVAL;
>> +
>> + 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;
>> + }
>> + if (trproc->rsc_table)
>> + iounmap(trproc->rsc_table);
>> + trproc->rsc_table = NULL;
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_stop);
>>
>
> I was wondering where this ID is coming from - Is it the ID of the remote
> processor service in the secure world or the ID of the program that provides the
> service?
>
> More comments tomorrow.
I assume that you are speaking about trproc->rproc_id. Yes, it is an identifier.
The rproc_id must be common between the Linux and the secure world. It is used
by the secure world to identify the remote processor and is platform dependent.
The TEE application that provides the service is identified by a UUID defined in
stm32_tee_rproc_id_table[] below.
Thanks,
Arnaud
>
> Thanks,
> Mathieu
>
> +
>> +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, unsigned int rproc_id)
>> +{
>> + struct tee_client_device *tee_device;
>> + struct tee_ioctl_open_session_arg sess_arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc;
>> + int ret;
>> +
>> + /*
>> + * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
>> + * probed or service not available in the secure firmware)
>> + * Assumption here is that the TEE bus is not probed.
>> + */
>> + if (!tee_rproc_ctx)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
>> + if (!trproc)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
>> + memset(&sess_arg, 0, sizeof(sess_arg));
>> +
>> + /* Open session with rproc_tee load the OP-TEE Trusted Application */
>> + 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);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + trproc->parent = dev;
>> + trproc->rproc_id = rproc_id;
>> + trproc->session_id = sess_arg.session;
>> +
>> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
>> +
>> + return trproc;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_register);
>> +
>> +int tee_rproc_unregister(struct tee_rproc *trproc)
>> +{
>> + 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);
>> +
>> + 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
>> new file mode 100644
>> index 000000000000..7c9e91e989ba
>> --- /dev/null
>> +++ b/include/linux/tee_remoteproc.h
>> @@ -0,0 +1,102 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright(c) 2023 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
>> + * @rsc_table: Resource table virtual address.
>> + */
>> +struct tee_rproc {
>> + struct list_head node;
>> + struct rproc *rproc;
>> + struct device *parent;
>> + u32 rproc_id;
>> + u32 session_id;
>> + struct resource_table *rsc_table;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
>> +
>> +struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
>> +int tee_rproc_unregister(struct tee_rproc *trproc);
>> +
>> +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);
>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
>> + const struct firmware *fw);
>> +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, unsigned int rproc_id)
>> +{
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
>> +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;
>> +}
>> +
>> +static inline struct resource_table *
>> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return NULL;
>> +}
>> +#endif /* CONFIG_TEE_REMOTEPROC */
>> +#endif /* TEE_REMOTEPROC_H */
>> --
>> 2.25.1
>>
On 2/14/2024 10:51 PM, Arnaud Pouliquen wrote:
> From: Arnaud Pouliquen <[email protected]>
>
> 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 device offers a client
> interface to load a firmware in the secure part.
> This firmware could be authenticated and decrypted by the secure
> trusted application.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
<...>
> +};
> +
> +static struct tee_rproc__context *tee_rproc_ctx;
> +
> +static void prepare_args(struct tee_rproc *trproc, int cmd,
> + struct tee_ioctl_invoke_arg *arg,
> + struct tee_param *param, unsigned int num_params)
s/prepare_args/tee_rproc_prepare_args ?
please align function args in nextline with (.
Eg:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/remoteproc_sysfs.c?h=v6.8-rc5#n13
> +{
> + memset(arg, 0, sizeof(*arg));
> + memset(param, 0, MAX_TEE_PARAM_ARRY_MEMBER * sizeof(*param));
> +
> + arg->func = cmd;
Regards,
Naman Jain
On 2/14/2024 10:51 PM, Arnaud Pouliquen wrote:
> Updates from the previous version [1]:
>
> This version proposes another approach based on an alternate load and boot
> of the coprocessor. Therefore, the constraint introduced by tee_remoteproc
> is that the firmware has to be authenticated and loaded before the resource
> table can be obtained.
>
> The existing boot sequence is: >
> 1) Get the resource table and store it in a cache,
> calling rproc->ops->parse_fw().
> 2) Parse the resource table and handle resources,
> calling rproc_handle_resources.
> 3) Load the firmware, calling rproc->ops->load().
> 4) Start the firmware, calling rproc->ops->start().
>
> => Steps 1 and 2 are executed in rproc_fw_boot(), while steps 3 and 4 are
> executed in rproc_start().
> => the use of rproc->ops->load() ops is mandatory
>
> The boot sequence needed for TEE boot is:
>
> 1) Load the firmware.
> 2) Get the loaded resource, no cache.
> 3) Parse the resource table and handle resources.
> 4) Start the firmware.
Hi,
What problem are we really addressing here by reordering load, parse of
FW resources?
Basically, what are the limitations of the current design you are
referring to?
I understood that TEE is designed that way.
>
> Then the crash recovery also has to be managed.For recovery, the cache is
> used to temporarily save the resource table and then reapply it on
> restart:
> 1) Stop the remote processor, calling rproc->ops->stop().
> 2) Load the firmware, calling rproc->ops->load().
> 3) Copy cached resource table.
> 4) Start the remote processor, calling rproc->ops->start().
>
> => This sequence is also needed when TEE manages the boot of the remote
> processor.
> => The rproc->ops->load() is also used in recovery sequence.
>
> Based on the sequences described above, the proposal is to:
>
> - Rework tee_rproc API to better match the rproc_ops structure.
> This allows to simply map the function to implement the load ops, which
> is not optional. The tee_rproc_load_fw() is updated in consequence.
> - Remove the call of rproc_load_segments from rproc_start() to dissociate
> the load and the start. This is necessary to implement the boot sequence
> requested for the TEE remote proc support.
> - Introduce an rproc_alt_fw_boot() function that is an alternative boot
> sequence, which implements the sequence requested for the TEE remoteproc
> support.
>
>
> [1] https://lore.kernel.org/lkml/[email protected]/T/
>
>
> 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).
s/Context/Environment?
> 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.
>
Regards,
Naman Jain
Hello Naman,
On 2/22/24 06:43, Naman Jain wrote:
> On 2/14/2024 10:51 PM, Arnaud Pouliquen wrote:
>> Updates from the previous version [1]:
>>
>> This version proposes another approach based on an alternate load and boot
>> of the coprocessor. Therefore, the constraint introduced by tee_remoteproc
>> is that the firmware has to be authenticated and loaded before the resource
>> table can be obtained.
>>
>> The existing boot sequence is: >
>> Â Â 1) Get the resource table and store it in a cache,
>> Â Â Â Â Â calling rproc->ops->parse_fw().
>> Â Â 2) Parse the resource table and handle resources,
>> Â Â Â Â Â calling rproc_handle_resources.
>> Â Â 3) Load the firmware, calling rproc->ops->load().
>> Â Â 4) Start the firmware, calling rproc->ops->start().
>> Â => Steps 1 and 2 are executed in rproc_fw_boot(), while steps 3 and 4 are
>> Â Â Â executed in rproc_start().
>> => the use of rproc->ops->load() ops is mandatory
>>
>> The boot sequence needed for TEE boot is:
>>
>> Â Â 1) Load the firmware.
>> Â Â 2) Get the loaded resource, no cache.
>> Â Â 3) Parse the resource table and handle resources.
>> Â Â 4) Start the firmware.
>
> Hi,
> What problem are we really addressing here by reordering load, parse of
> FW resources?
The feature introduced in TEE is the signature of the firmware images. That
means that before getting the resource table, we need to first authenticate the
firmware images.
Authenticating a firmware image means that we have to copy the firmware into
protected memory that cannot be corrupted by the non-secure and then verify the
signature.
The strategy implemented in OP-TEE is to load the firmware into destination
memory and then authenticate it.
This strategy avoids having a temporary copy of the whole images in a secure memory.
This strategy imposes loading the firmware images before retrieving the resource
table.
> Basically, what are the limitations of the current design you are referring to?
> I understood that TEE is designed that way.
The limitation of the current design is that we obtain the resource table before
loading the firmware. Following the current design would impose constraints in
TEE that are not straightforward. Step 1 (getting the resource table and storing
it in a cache) would require having a copy of the resource table in TEE after
authenticating the images. However, authenticating the firmware, as explained
before, depends on the strategy implemented. In TEE implementation, we load the
firmware to authenticate it in the destination memory.
Regards,
Arnaud
>
>>
>> Then the crash recovery also has to be managed.For recovery, the cache is
>> used to temporarily save the resource table and then reapply it on
>> restart:
>> Â Â 1) Stop the remote processor, calling rproc->ops->stop().
>> Â Â 2) Load the firmware, calling rproc->ops->load().
>> Â Â 3) Copy cached resource table.
>> Â Â 4) Start the remote processor, calling rproc->ops->start().
>>
>> => This sequence is also needed when TEE manages the boot of the remote
>> Â Â Â processor.
>> => The rproc->ops->load() is also used in recovery sequence.
>>
>> Based on the sequences described above, the proposal is to:
>>
>> - Rework tee_rproc API to better match the rproc_ops structure.
>> Â Â This allows to simply map the function to implement the load ops, which
>> Â Â is not optional. The tee_rproc_load_fw() is updated in consequence.
>> - Remove the call of rproc_load_segments from rproc_start() to dissociate
>> Â Â the load and the start. This is necessary to implement the boot sequence
>> Â Â requested for the TEE remote proc support.
>> - Introduce an rproc_alt_fw_boot() function that is an alternative boot
>> Â Â sequence, which implements the sequence requested for the TEE remoteproc
>> Â Â support.
>>
>>
>> [1]
>> https://lore.kernel.org/lkml/[email protected]/T/
>>
>>
>> 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).
>
> s/Context/Environment?
>
>> 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.
>>
>
> Regards,
> Naman Jain
>
On 2/22/2024 2:17 PM, Arnaud POULIQUEN wrote:
> Hello Naman,
>
> On 2/22/24 06:43, Naman Jain wrote:
>> On 2/14/2024 10:51 PM, Arnaud Pouliquen wrote:
>>> Updates from the previous version [1]:
>>>
>>> This version proposes another approach based on an alternate load and boot
>>> of the coprocessor. Therefore, the constraint introduced by tee_remoteproc
>>> is that the firmware has to be authenticated and loaded before the resource
>>> table can be obtained.
>>>
>>> The existing boot sequence is: >
>>> Â Â 1) Get the resource table and store it in a cache,
>>> Â Â Â Â Â calling rproc->ops->parse_fw().
>>> Â Â 2) Parse the resource table and handle resources,
>>> Â Â Â Â Â calling rproc_handle_resources.
>>> Â Â 3) Load the firmware, calling rproc->ops->load().
>>> Â Â 4) Start the firmware, calling rproc->ops->start().
>>> Â => Steps 1 and 2 are executed in rproc_fw_boot(), while steps 3 and 4 are
>>> Â Â Â executed in rproc_start().
>>> => the use of rproc->ops->load() ops is mandatory
>>>
>>> The boot sequence needed for TEE boot is:
>>>
>>> Â Â 1) Load the firmware.
>>> Â Â 2) Get the loaded resource, no cache.
>>> Â Â 3) Parse the resource table and handle resources.
>>> Â Â 4) Start the firmware.
>>
>> Hi,
>> What problem are we really addressing here by reordering load, parse of
>> FW resources?
>
> The feature introduced in TEE is the signature of the firmware images. That
> means that before getting the resource table, we need to first authenticate the
> firmware images.
> Authenticating a firmware image means that we have to copy the firmware into
> protected memory that cannot be corrupted by the non-secure and then verify the
> signature.
> The strategy implemented in OP-TEE is to load the firmware into destination
> memory and then authenticate it.
> This strategy avoids having a temporary copy of the whole images in a secure memory.
> This strategy imposes loading the firmware images before retrieving the resource
> table.
>
>> Basically, what are the limitations of the current design you are referring to?
>> I understood that TEE is designed that way.
>
> The limitation of the current design is that we obtain the resource table before
> loading the firmware. Following the current design would impose constraints in
> TEE that are not straightforward. Step 1 (getting the resource table and storing
> it in a cache) would require having a copy of the resource table in TEE after
> authenticating the images. However, authenticating the firmware, as explained
> before, depends on the strategy implemented. In TEE implementation, we load the
> firmware to authenticate it in the destination memory.
>
> Regards,
> Arnaud
Hello Arnaud,
I think now I got your point. In TEE, you don't want to do anything(read
resource table) with FW images, until its loaded and authenticated.
Since current design was not allowing you to do it, you had to
reorganize the code so that this can be achieved.
Generally speaking, in current design, if authentication fails for some
reason later, one can handle it, but it depends on the implementation of
parse_fw op if the damage is already done.
Please correct me if this is wrong assumption.
Patch looks good to me.
Regards,
Naman Jain
On Wed, 14 Feb 2024 18:21:25 +0100, Arnaud Pouliquen wrote:
> The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
> where the Cortex-M4 firmware is loaded by the Trusted execution Environment
> (TEE).
> For instance, this compatible is used in both the Linux and OP-TEE
> device-tree:
> - In OP-TEE, a node is defined in the device tree with the
> st,stm32mp1-m4-tee to support signed remoteproc firmware.
> Based on DT properties, OP-TEE authenticates, loads, starts, and stops
> the firmware.
> - On Linux, when the compatibility is set, the Cortex-M resets should not
> be declared in the device tree.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> V1 to V2 updates
> - update "st,stm32mp1-m4" compatible description to generalize
> - remove the 'reset-names' requirement in one conditional branch, as the
> property is already part of the condition test.
> ---
> .../bindings/remoteproc/st,stm32-rproc.yaml | 51 ++++++++++++++++---
> 1 file changed, 43 insertions(+), 8 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
Hi,
On Wed, Feb 14, 2024 at 06:21:27PM +0100, Arnaud Pouliquen wrote:
> The new TEE remoteproc device is used to manage remote firmware in a
> secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
> introduced to delegate the loading of the firmware to the trusted
> execution context. In such cases, the firmware should be signed and
> adhere to the image format defined by the TEE.
>
> A new "to_attach" field is introduced to differentiate the use cases
> "firmware loaded by the boot stage" and "firmware loaded by the TEE".
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> V2 to V3 update:
> - remove stm32_rproc_tee_elf_sanity_check(), stm32_rproc_tee_elf_load()
> stm32_rproc_tee_elf_find_loaded_rsc_table() and stm32_rproc_tee_start() that are bnow unused
> - use new rproc::alt_boot field to sepcify that the alternate fboot method is used
> - use stm32_rproc::to_attach field to differenciate attch mode from remoteproc tee boot mode.
> - remove the used of stm32_rproc::fw_loaded
> ---
> drivers/remoteproc/stm32_rproc.c | 85 +++++++++++++++++++++++++++++---
> 1 file changed, 79 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index fcc0001e2657..9cfcf66462e0 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -20,6 +20,7 @@
> #include <linux/remoteproc.h>
> #include <linux/reset.h>
> #include <linux/slab.h>
> +#include <linux/tee_remoteproc.h>
> #include <linux/workqueue.h>
>
> #include "remoteproc_internal.h"
> @@ -49,6 +50,9 @@
> #define M4_STATE_STANDBY 4
> #define M4_STATE_CRASH 5
>
> +/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
> +#define STM32_MP1_M4_PROC_ID 0
> +
> struct stm32_syscon {
> struct regmap *map;
> u32 reg;
> @@ -90,6 +94,8 @@ struct stm32_rproc {
> struct stm32_mbox mb[MBOX_NB_MBX];
> struct workqueue_struct *workqueue;
> bool hold_boot_smc;
> + bool to_attach;
> + struct tee_rproc *trproc;
> void __iomem *rsc_va;
> };
>
> @@ -253,10 +259,30 @@ static int stm32_rproc_release(struct rproc *rproc)
> return err;
> }
> }
> + ddata->to_attach = false;
>
> return err;
> }
>
> +static int stm32_rproc_tee_attach(struct rproc *rproc)
> +{
> + /* Nothing to do, remote proc already started by the secured context. */
> + return 0;
> +}
> +
> +static int stm32_rproc_tee_stop(struct rproc *rproc)
> +{
> + int err;
> +
> + stm32_rproc_request_shutdown(rproc);
> +
> + err = tee_rproc_stop(rproc);
> + if (err)
> + return err;
> +
> + return stm32_rproc_release(rproc);
> +}
> +
> static int stm32_rproc_prepare(struct rproc *rproc)
> {
> struct device *dev = rproc->dev.parent;
> @@ -637,10 +663,14 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> {
> struct stm32_rproc *ddata = rproc->priv;
> struct device *dev = rproc->dev.parent;
> + struct tee_rproc *trproc = ddata->trproc;
> phys_addr_t rsc_pa;
> u32 rsc_da;
> int err;
>
> + if (trproc && !ddata->to_attach)
> + return tee_rproc_get_loaded_rsc_table(rproc, table_sz);
> +
Why do we need a flag at all? Why can't st_rproc_tee_ops::get_loaded_rsc_table
be set to tee_rproc_get_loaded_rsc_table()?
> /* The resource table has already been mapped, nothing to do */
> if (ddata->rsc_va)
> goto done;
> @@ -693,8 +723,20 @@ static const struct rproc_ops st_rproc_ops = {
> .get_boot_addr = rproc_elf_get_boot_addr,
> };
>
> +static const struct rproc_ops st_rproc_tee_ops = {
> + .prepare = stm32_rproc_prepare,
> + .start = tee_rproc_start,
> + .stop = stm32_rproc_tee_stop,
> + .attach = stm32_rproc_tee_attach,
> + .kick = stm32_rproc_kick,
> + .get_loaded_rsc_table = stm32_rproc_get_loaded_rsc_table,
> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
> + .load = tee_rproc_load_fw,
> +};
> +
> static const struct of_device_id stm32_rproc_match[] = {
> - { .compatible = "st,stm32mp1-m4" },
> + {.compatible = "st,stm32mp1-m4",},
> + {.compatible = "st,stm32mp1-m4-tee",},
> {},
> };
> MODULE_DEVICE_TABLE(of, stm32_rproc_match);
> @@ -853,6 +895,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct stm32_rproc *ddata;
> struct device_node *np = dev->of_node;
> + struct tee_rproc *trproc = NULL;
> struct rproc *rproc;
> unsigned int state;
> int ret;
> @@ -861,12 +904,33 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - rproc = rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
> - if (!rproc)
> - return -ENOMEM;
This patch doesn't apply to rproc-next - please rebase.
> + if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
> + /*
> + * Delegate the firmware management to the secure context.
> + * The firmware loaded has to be signed.
> + */
> + trproc = tee_rproc_register(dev, STM32_MP1_M4_PROC_ID);
> + if (IS_ERR(trproc)) {
> + dev_err_probe(dev, PTR_ERR(trproc),
> + "signed firmware not supported by TEE\n");
> + return PTR_ERR(trproc);
> + }
> + }
>
> - ddata = rproc->priv;
> + rproc = rproc_alloc(dev, np->name,
> + trproc ? &st_rproc_tee_ops : &st_rproc_ops,
> + NULL, sizeof(*ddata));
> + if (!rproc) {
> + ret = -ENOMEM;
> + goto free_tee;
> + }
>
> + ddata = rproc->priv;
> + ddata->trproc = trproc;
My opinion hasn't changed from the previous patchet, i.e tee_rproc should be
folded in struct rproc as rproc::tee_interface.
More comments to come shortly...
> + if (trproc) {
> + rproc->alt_boot = true;
> + trproc->rproc = rproc;
> + }
> rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
>
> ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
> @@ -881,8 +945,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> if (ret)
> goto free_rproc;
>
> - if (state == M4_STATE_CRUN)
> + if (state == M4_STATE_CRUN) {
> rproc->state = RPROC_DETACHED;
> + ddata->to_attach = true;
> + }
>
> rproc->has_iommu = false;
> ddata->workqueue = create_workqueue(dev_name(dev));
> @@ -916,6 +982,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> device_init_wakeup(dev, false);
> }
> rproc_free(rproc);
> +free_tee:
> + if (trproc)
> + tee_rproc_unregister(trproc);
> +
> return ret;
> }
>
> @@ -923,6 +993,7 @@ static void stm32_rproc_remove(struct platform_device *pdev)
> {
> struct rproc *rproc = platform_get_drvdata(pdev);
> struct stm32_rproc *ddata = rproc->priv;
> + struct tee_rproc *trproc = ddata->trproc;
> struct device *dev = &pdev->dev;
>
> if (atomic_read(&rproc->power) > 0)
> @@ -937,6 +1008,8 @@ static void stm32_rproc_remove(struct platform_device *pdev)
> device_init_wakeup(dev, false);
> }
> rproc_free(rproc);
> + if (trproc)
> + tee_rproc_unregister(trproc);
> }
>
> static int stm32_rproc_suspend(struct device *dev)
> --
> 2.25.1
>
Hello Mathieu,
On 2/22/24 20:02, Mathieu Poirier wrote:
> Hi,
>
> On Wed, Feb 14, 2024 at 06:21:27PM +0100, Arnaud Pouliquen wrote:
>> The new TEE remoteproc device is used to manage remote firmware in a
>> secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
>> introduced to delegate the loading of the firmware to the trusted
>> execution context. In such cases, the firmware should be signed and
>> adhere to the image format defined by the TEE.
>>
>> A new "to_attach" field is introduced to differentiate the use cases
>> "firmware loaded by the boot stage" and "firmware loaded by the TEE".
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> V2 to V3 update:
>> - remove stm32_rproc_tee_elf_sanity_check(), stm32_rproc_tee_elf_load()
>> stm32_rproc_tee_elf_find_loaded_rsc_table() and stm32_rproc_tee_start() that are bnow unused
>> - use new rproc::alt_boot field to sepcify that the alternate fboot method is used
>> - use stm32_rproc::to_attach field to differenciate attch mode from remoteproc tee boot mode.
>> - remove the used of stm32_rproc::fw_loaded
>> ---
>> drivers/remoteproc/stm32_rproc.c | 85 +++++++++++++++++++++++++++++---
>> 1 file changed, 79 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index fcc0001e2657..9cfcf66462e0 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -20,6 +20,7 @@
>> #include <linux/remoteproc.h>
>> #include <linux/reset.h>
>> #include <linux/slab.h>
>> +#include <linux/tee_remoteproc.h>
>> #include <linux/workqueue.h>
>>
>> #include "remoteproc_internal.h"
>> @@ -49,6 +50,9 @@
>> #define M4_STATE_STANDBY 4
>> #define M4_STATE_CRASH 5
>>
>> +/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
>> +#define STM32_MP1_M4_PROC_ID 0
>> +
>> struct stm32_syscon {
>> struct regmap *map;
>> u32 reg;
>> @@ -90,6 +94,8 @@ struct stm32_rproc {
>> struct stm32_mbox mb[MBOX_NB_MBX];
>> struct workqueue_struct *workqueue;
>> bool hold_boot_smc;
>> + bool to_attach;
>> + struct tee_rproc *trproc;
>> void __iomem *rsc_va;
>> };
>>
>> @@ -253,10 +259,30 @@ static int stm32_rproc_release(struct rproc *rproc)
>> return err;
>> }
>> }
>> + ddata->to_attach = false;
>>
>> return err;
>> }
>>
>> +static int stm32_rproc_tee_attach(struct rproc *rproc)
>> +{
>> + /* Nothing to do, remote proc already started by the secured context. */
>> + return 0;
>> +}
>> +
>> +static int stm32_rproc_tee_stop(struct rproc *rproc)
>> +{
>> + int err;
>> +
>> + stm32_rproc_request_shutdown(rproc);
>> +
>> + err = tee_rproc_stop(rproc);
>> + if (err)
>> + return err;
>> +
>> + return stm32_rproc_release(rproc);
>> +}
>> +
>> static int stm32_rproc_prepare(struct rproc *rproc)
>> {
>> struct device *dev = rproc->dev.parent;
>> @@ -637,10 +663,14 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>> {
>> struct stm32_rproc *ddata = rproc->priv;
>> struct device *dev = rproc->dev.parent;
>> + struct tee_rproc *trproc = ddata->trproc;
>> phys_addr_t rsc_pa;
>> u32 rsc_da;
>> int err;
>>
>> + if (trproc && !ddata->to_attach)
>> + return tee_rproc_get_loaded_rsc_table(rproc, table_sz);
>> +
>
> Why do we need a flag at all? Why can't st_rproc_tee_ops::get_loaded_rsc_table
> be set to tee_rproc_get_loaded_rsc_table()?
This function is used to retrieve the address of the resource table in 3 cases
- attach to a firmware started by the boot loader (U-boot).
- load of the firmware by OP-TEE.
- crash recovery on a signed firmware started by the boot loader.
The flag is used to differentiate the attch from the other uses cases
For instance we support this use case.
1) attach to the firmware on boot
2) crash during runtime
2a) stop the firmware by OP-TEE( ddata->to_attach set to 0)
2b) load the firmware by OP-TEE
2c) get the loaded resource table from OP-TEE (we can not guaranty
that the firmware loaded on recovery is the same)
2d) restart the firmware by OP-TEE
>
>> /* The resource table has already been mapped, nothing to do */
>> if (ddata->rsc_va)
>> goto done;
>> @@ -693,8 +723,20 @@ static const struct rproc_ops st_rproc_ops = {
>> .get_boot_addr = rproc_elf_get_boot_addr,
>> };
>>
>> +static const struct rproc_ops st_rproc_tee_ops = {
>> + .prepare = stm32_rproc_prepare,
>> + .start = tee_rproc_start,
>> + .stop = stm32_rproc_tee_stop,
>> + .attach = stm32_rproc_tee_attach,
>> + .kick = stm32_rproc_kick,
>> + .get_loaded_rsc_table = stm32_rproc_get_loaded_rsc_table,
>> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
>> + .load = tee_rproc_load_fw,
>> +};
>> +
>> static const struct of_device_id stm32_rproc_match[] = {
>> - { .compatible = "st,stm32mp1-m4" },
>> + {.compatible = "st,stm32mp1-m4",},
>> + {.compatible = "st,stm32mp1-m4-tee",},
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, stm32_rproc_match);
>> @@ -853,6 +895,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>> struct device *dev = &pdev->dev;
>> struct stm32_rproc *ddata;
>> struct device_node *np = dev->of_node;
>> + struct tee_rproc *trproc = NULL;
>> struct rproc *rproc;
>> unsigned int state;
>> int ret;
>> @@ -861,12 +904,33 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>> if (ret)
>> return ret;
>>
>> - rproc = rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
>> - if (!rproc)
>> - return -ENOMEM;
>
> This patch doesn't apply to rproc-next - please rebase.
Yes, sure. I forgot to mention in my cover letter that my series has been
applied and tested on 841c35169323 (Linux 6.8-rc4).
>
>
>> + if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
>> + /*
>> + * Delegate the firmware management to the secure context.
>> + * The firmware loaded has to be signed.
>> + */
>> + trproc = tee_rproc_register(dev, STM32_MP1_M4_PROC_ID);
>> + if (IS_ERR(trproc)) {
>> + dev_err_probe(dev, PTR_ERR(trproc),
>> + "signed firmware not supported by TEE\n");
>> + return PTR_ERR(trproc);
>> + }
>> + }
>>
>> - ddata = rproc->priv;
>> + rproc = rproc_alloc(dev, np->name,
>> + trproc ? &st_rproc_tee_ops : &st_rproc_ops,
>> + NULL, sizeof(*ddata));
>> + if (!rproc) {
>> + ret = -ENOMEM;
>> + goto free_tee;
>> + }
>>
>> + ddata = rproc->priv;
>> + ddata->trproc = trproc;
>
> My opinion hasn't changed from the previous patchet, i.e tee_rproc should be
> folded in struct rproc as rproc::tee_interface.
Sure, I will do it in next version
>
> More comments to come shortly...
>
Thanks!
Arnaud
>> + if (trproc) {
>> + rproc->alt_boot = true;
>> + trproc->rproc = rproc;
>> + }
>> rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
>>
>> ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
>> @@ -881,8 +945,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>> if (ret)
>> goto free_rproc;
>>
>> - if (state == M4_STATE_CRUN)
>> + if (state == M4_STATE_CRUN) {
>> rproc->state = RPROC_DETACHED;
>> + ddata->to_attach = true;
>> + }
>>
>> rproc->has_iommu = false;
>> ddata->workqueue = create_workqueue(dev_name(dev));
>> @@ -916,6 +982,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>> device_init_wakeup(dev, false);
>> }
>> rproc_free(rproc);
>> +free_tee:
>> + if (trproc)
>> + tee_rproc_unregister(trproc);
>> +
>> return ret;
>> }
>>
>> @@ -923,6 +993,7 @@ static void stm32_rproc_remove(struct platform_device *pdev)
>> {
>> struct rproc *rproc = platform_get_drvdata(pdev);
>> struct stm32_rproc *ddata = rproc->priv;
>> + struct tee_rproc *trproc = ddata->trproc;
>> struct device *dev = &pdev->dev;
>>
>> if (atomic_read(&rproc->power) > 0)
>> @@ -937,6 +1008,8 @@ static void stm32_rproc_remove(struct platform_device *pdev)
>> device_init_wakeup(dev, false);
>> }
>> rproc_free(rproc);
>> + if (trproc)
>> + tee_rproc_unregister(trproc);
>> }
>>
>> static int stm32_rproc_suspend(struct device *dev)
>> --
>> 2.25.1
>>
>
On 2/22/24 10:55, Naman Jain wrote:
> On 2/22/2024 2:17 PM, Arnaud POULIQUEN wrote:
>> Hello Naman,
>>
>> On 2/22/24 06:43, Naman Jain wrote:
>>> On 2/14/2024 10:51 PM, Arnaud Pouliquen wrote:
>>>> Updates from the previous version [1]:
>>>>
>>>> This version proposes another approach based on an alternate load and boot
>>>> of the coprocessor. Therefore, the constraint introduced by tee_remoteproc
>>>> is that the firmware has to be authenticated and loaded before the resource
>>>> table can be obtained.
>>>>
>>>> The existing boot sequence is: >
>>>> Â Â Â 1) Get the resource table and store it in a cache,
>>>> Â Â Â Â Â Â calling rproc->ops->parse_fw().
>>>> Â Â Â 2) Parse the resource table and handle resources,
>>>> Â Â Â Â Â Â calling rproc_handle_resources.
>>>> Â Â Â 3) Load the firmware, calling rproc->ops->load().
>>>> Â Â Â 4) Start the firmware, calling rproc->ops->start().
>>>> Â Â => Steps 1 and 2 are executed in rproc_fw_boot(), while steps 3 and 4 are
>>>> Â Â Â Â executed in rproc_start().
>>>> => the use of rproc->ops->load() ops is mandatory
>>>>
>>>> The boot sequence needed for TEE boot is:
>>>>
>>>> Â Â Â 1) Load the firmware.
>>>> Â Â Â 2) Get the loaded resource, no cache.
>>>> Â Â Â 3) Parse the resource table and handle resources.
>>>> Â Â Â 4) Start the firmware.
>>>
>>> Hi,
>>> What problem are we really addressing here by reordering load, parse of
>>> FW resources?
>>
>> The feature introduced in TEE is the signature of the firmware images. That
>> means that before getting the resource table, we need to first authenticate the
>> firmware images.
>> Authenticating a firmware image means that we have to copy the firmware into
>> protected memory that cannot be corrupted by the non-secure and then verify the
>> signature.
>> The strategy implemented in OP-TEE is to load the firmware into destination
>> memory and then authenticate it.
>> This strategy avoids having a temporary copy of the whole images in a secure
>> memory.
>> This strategy imposes loading the firmware images before retrieving the resource
>> table.
>>
>>> Basically, what are the limitations of the current design you are referring to?
>>> I understood that TEE is designed that way.
>>
>> The limitation of the current design is that we obtain the resource table before
>> loading the firmware. Following the current design would impose constraints in
>> TEE that are not straightforward. Step 1 (getting the resource table and storing
>> it in a cache) would require having a copy of the resource table in TEE after
>> authenticating the images. However, authenticating the firmware, as explained
>> before, depends on the strategy implemented. In TEE implementation, we load the
>> firmware to authenticate it in the destination memory.
>>
>> Regards,
>> Arnaud
>
>
> Hello Arnaud,
> I think now I got your point. In TEE, you don't want to do anything(read
> resource table) with FW images, until its loaded and authenticated.
> Since current design was not allowing you to do it, you had to reorganize the
> code so that this can be achieved.
>
> Generally speaking, in current design, if authentication fails for some
> reason later, one can handle it, but it depends on the implementation of
> parse_fw op if the damage is already done.
>
> Please correct me if this is wrong assumption.
That's correct.
Regards,
Arnaud
> Patch looks good to me.
>
> Regards,
> Naman Jain
On Wed, Feb 14, 2024 at 06:21:23PM +0100, Arnaud Pouliquen wrote:
> Add a check on the optional rproc->cached_table to perform the memory
> copy only if it is not null.
>
> 2 use cases to support:
> - starting on boot, in which case rproc->cached_table can be null,
> - starting on crash recovery, where the cached table is used to save
> the resource table configuration on stop and re-apply the configuration
> on the re-start.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 283ca071e35c..34b0093689da 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1278,7 +1278,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> * that any subsequent changes will be applied to the loaded version.
> */
> loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> - if (loaded_table) {
> + if (loaded_table && rproc->cached_table) {
.. and this becomes:
if (loaded_table != rproc->cached_table)
with a detailed comment about what is going on and a reference to
tee_rproc_parse_fw().
There are other things to adjust in this patchset but starting with that will
hopefully deal with a few of them. We can address the rest at the next
iteration.
I am done reviewing this set.
Thanks,
Mathieu
> memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> rproc->table_ptr = loaded_table;
> }
> --
> 2.25.1
>
On Wed, Feb 14, 2024 at 06:21:21PM +0100, Arnaud Pouliquen wrote:
> From: Arnaud Pouliquen <[email protected]>
>
> 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 device offers a client
> interface to load a firmware in the secure part.
> This firmware could be authenticated and decrypted by the secure
> trusted application.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> update from V2
> - Use 'tee_rproc' prefix for all functions
> - rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
> - redefine fonction to better match with the rproc_ops structure format
> - replace 'struct tee_rproc' parameter by 'struct rproc' parameter
> - rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
> and rework it to remove the cached_table management.
> - introduce tee_rproc_get_context() to get the tee_rproc struct from the
> rproc struct
> - rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
> - remove useless check on tee_rproc_ctx structure in tee_rproc_register()
> and tee_rproc_unregister()
> - fix test on the return of tee_rproc_ctx = devm_kzalloc()
> - remove useless includes and unused tee_rproc_mem structure.
> ---
> drivers/remoteproc/Kconfig | 9 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
> include/linux/tee_remoteproc.h | 102 +++++++
> 4 files changed, 509 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..85299606806c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
>
> It's safe to say N if not interested in using RPU r5f cores.
>
> +
> +config TEE_REMOTEPROC
> + tristate "trusted firmware support by a TEE application"
> + depends on OPTEE
> + help
> + Support for trusted remote processors firmware. The firmware
> + authentication and/or decryption are managed by a trusted application.
> + 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..ac727e062d00
> --- /dev/null
> +++ b/drivers/remoteproc/tee_remoteproc.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) STMicroelectronics 2023 - 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 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
> +{
> + struct tee_rproc *entry, *tmp;
> +
> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
> + if (entry->rproc == rproc)
> + return entry;
> + }
> +
> + return NULL;
> +}
> +
> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct tee_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + 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);
> +
> + 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_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + int ret;
> +
> + if (!trproc)
> + return ERR_PTR(-EINVAL);
> +
> + 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. */
> + trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
> + if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
Here we are missing:
tee_rproc_parse_fw()
{
tee_rproc_load_fw()
resource_table = tee_rproc_get_loaded_rsc_table()
//check error conditions here
rproc->cached_table = resource_table;
rproc->table_ptr = resource_table;
}
This is essentially the same as rproc_elf_load_rsc_table(). That way we don't
need rproc_alt_fw_boot() and rproc_load_segments() doesn't have to be moved
around. The trusted application simply needs to know that if the firmware is
already loaded, it has to return.
> +
> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> + const struct firmware *fw)
> +{
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + size_t table_sz;
> +
> + if (!trproc)
> + return ERR_PTR(-EINVAL);
> +
> + if (!trproc->rsc_table)
> + trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> +
> + return trproc->rsc_table;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
> +
> +int tee_rproc_start(struct rproc *rproc)
> +{
> + struct tee_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + int ret;
> +
> + if (!trproc)
> + return -EINVAL;
> +
> + 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;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_start);
> +
> +int tee_rproc_stop(struct rproc *rproc)
> +{
> + struct tee_ioctl_invoke_arg arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> + int ret;
> +
> + if (!trproc)
> + return -EINVAL;
> +
> + 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;
> + }
> + if (trproc->rsc_table)
> + iounmap(trproc->rsc_table);
> + trproc->rsc_table = NULL;
> +
> + 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, unsigned int rproc_id)
> +{
> + struct tee_client_device *tee_device;
> + struct tee_ioctl_open_session_arg sess_arg;
> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> + struct tee_rproc *trproc;
> + int ret;
> +
> + /*
> + * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
> + * probed or service not available in the secure firmware)
> + * Assumption here is that the TEE bus is not probed.
> + */
> + if (!tee_rproc_ctx)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
> + if (!trproc)
> + return ERR_PTR(-ENOMEM);
> +
> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
> + memset(&sess_arg, 0, sizeof(sess_arg));
> +
> + /* Open session with rproc_tee load the OP-TEE Trusted Application */
> + 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);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + trproc->parent = dev;
> + trproc->rproc_id = rproc_id;
> + trproc->session_id = sess_arg.session;
> +
> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
> +
> + return trproc;
> +}
> +EXPORT_SYMBOL_GPL(tee_rproc_register);
> +
> +int tee_rproc_unregister(struct tee_rproc *trproc)
> +{
> + 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);
> +
> + 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
> new file mode 100644
> index 000000000000..7c9e91e989ba
> --- /dev/null
> +++ b/include/linux/tee_remoteproc.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright(c) 2023 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
> + * @rsc_table: Resource table virtual address.
> + */
> +struct tee_rproc {
> + struct list_head node;
> + struct rproc *rproc;
> + struct device *parent;
> + u32 rproc_id;
> + u32 session_id;
> + struct resource_table *rsc_table;
> +};
> +
> +#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
> +
> +struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
> +int tee_rproc_unregister(struct tee_rproc *trproc);
> +
> +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);
> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> + const struct firmware *fw);
> +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, unsigned int rproc_id)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> +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;
> +}
> +
> +static inline struct resource_table *
> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> +{
> + /* This shouldn't be possible */
> + WARN_ON(1);
> +
> + return NULL;
> +}
> +#endif /* CONFIG_TEE_REMOTEPROC */
> +#endif /* TEE_REMOTEPROC_H */
> --
> 2.25.1
>
On Fri, Feb 23, 2024 at 02:54:13PM +0100, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 2/22/24 20:02, Mathieu Poirier wrote:
> > Hi,
> >
> > On Wed, Feb 14, 2024 at 06:21:27PM +0100, Arnaud Pouliquen wrote:
> >> The new TEE remoteproc device is used to manage remote firmware in a
> >> secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
> >> introduced to delegate the loading of the firmware to the trusted
> >> execution context. In such cases, the firmware should be signed and
> >> adhere to the image format defined by the TEE.
> >>
> >> A new "to_attach" field is introduced to differentiate the use cases
> >> "firmware loaded by the boot stage" and "firmware loaded by the TEE".
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> ---
> >> V2 to V3 update:
> >> - remove stm32_rproc_tee_elf_sanity_check(), stm32_rproc_tee_elf_load()
> >> stm32_rproc_tee_elf_find_loaded_rsc_table() and stm32_rproc_tee_start() that are bnow unused
> >> - use new rproc::alt_boot field to sepcify that the alternate fboot method is used
> >> - use stm32_rproc::to_attach field to differenciate attch mode from remoteproc tee boot mode.
> >> - remove the used of stm32_rproc::fw_loaded
> >> ---
> >> drivers/remoteproc/stm32_rproc.c | 85 +++++++++++++++++++++++++++++---
> >> 1 file changed, 79 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >> index fcc0001e2657..9cfcf66462e0 100644
> >> --- a/drivers/remoteproc/stm32_rproc.c
> >> +++ b/drivers/remoteproc/stm32_rproc.c
> >> @@ -20,6 +20,7 @@
> >> #include <linux/remoteproc.h>
> >> #include <linux/reset.h>
> >> #include <linux/slab.h>
> >> +#include <linux/tee_remoteproc.h>
> >> #include <linux/workqueue.h>
> >>
> >> #include "remoteproc_internal.h"
> >> @@ -49,6 +50,9 @@
> >> #define M4_STATE_STANDBY 4
> >> #define M4_STATE_CRASH 5
> >>
> >> +/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
> >> +#define STM32_MP1_M4_PROC_ID 0
> >> +
> >> struct stm32_syscon {
> >> struct regmap *map;
> >> u32 reg;
> >> @@ -90,6 +94,8 @@ struct stm32_rproc {
> >> struct stm32_mbox mb[MBOX_NB_MBX];
> >> struct workqueue_struct *workqueue;
> >> bool hold_boot_smc;
> >> + bool to_attach;
> >> + struct tee_rproc *trproc;
> >> void __iomem *rsc_va;
> >> };
> >>
> >> @@ -253,10 +259,30 @@ static int stm32_rproc_release(struct rproc *rproc)
> >> return err;
> >> }
> >> }
> >> + ddata->to_attach = false;
> >>
> >> return err;
> >> }
> >>
> >> +static int stm32_rproc_tee_attach(struct rproc *rproc)
> >> +{
> >> + /* Nothing to do, remote proc already started by the secured context. */
> >> + return 0;
> >> +}
> >> +
> >> +static int stm32_rproc_tee_stop(struct rproc *rproc)
> >> +{
> >> + int err;
> >> +
> >> + stm32_rproc_request_shutdown(rproc);
> >> +
> >> + err = tee_rproc_stop(rproc);
> >> + if (err)
> >> + return err;
> >> +
> >> + return stm32_rproc_release(rproc);
> >> +}
> >> +
> >> static int stm32_rproc_prepare(struct rproc *rproc)
> >> {
> >> struct device *dev = rproc->dev.parent;
> >> @@ -637,10 +663,14 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> >> {
> >> struct stm32_rproc *ddata = rproc->priv;
> >> struct device *dev = rproc->dev.parent;
> >> + struct tee_rproc *trproc = ddata->trproc;
> >> phys_addr_t rsc_pa;
> >> u32 rsc_da;
> >> int err;
> >>
> >> + if (trproc && !ddata->to_attach)
> >> + return tee_rproc_get_loaded_rsc_table(rproc, table_sz);
> >> +
> >
> > Why do we need a flag at all? Why can't st_rproc_tee_ops::get_loaded_rsc_table
> > be set to tee_rproc_get_loaded_rsc_table()?
>
>
> This function is used to retrieve the address of the resource table in 3 cases
> - attach to a firmware started by the boot loader (U-boot).
> - load of the firmware by OP-TEE.
> - crash recovery on a signed firmware started by the boot loader.
>
> The flag is used to differentiate the attch from the other uses cases
> For instance we support this use case.
> 1) attach to the firmware on boot
> 2) crash during runtime
> 2a) stop the firmware by OP-TEE( ddata->to_attach set to 0)
> 2b) load the firmware by OP-TEE
> 2c) get the loaded resource table from OP-TEE (we can not guaranty
> that the firmware loaded on recovery is the same)
> 2d) restart the firmware by OP-TEE
This is not maintainable and needs to be broken down into smaller
building blocks. The introduction of tee_rproc_parse_fw() should help dealing
with some of the complexity.
>
> >
> >> /* The resource table has already been mapped, nothing to do */
> >> if (ddata->rsc_va)
> >> goto done;
> >> @@ -693,8 +723,20 @@ static const struct rproc_ops st_rproc_ops = {
> >> .get_boot_addr = rproc_elf_get_boot_addr,
> >> };
> >>
> >> +static const struct rproc_ops st_rproc_tee_ops = {
> >> + .prepare = stm32_rproc_prepare,
> >> + .start = tee_rproc_start,
> >> + .stop = stm32_rproc_tee_stop,
> >> + .attach = stm32_rproc_tee_attach,
> >> + .kick = stm32_rproc_kick,
> >> + .get_loaded_rsc_table = stm32_rproc_get_loaded_rsc_table,
> >> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
> >> + .load = tee_rproc_load_fw,
> >> +};
> >> +
> >> static const struct of_device_id stm32_rproc_match[] = {
> >> - { .compatible = "st,stm32mp1-m4" },
> >> + {.compatible = "st,stm32mp1-m4",},
> >> + {.compatible = "st,stm32mp1-m4-tee",},
> >> {},
> >> };
> >> MODULE_DEVICE_TABLE(of, stm32_rproc_match);
> >> @@ -853,6 +895,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> struct device *dev = &pdev->dev;
> >> struct stm32_rproc *ddata;
> >> struct device_node *np = dev->of_node;
> >> + struct tee_rproc *trproc = NULL;
> >> struct rproc *rproc;
> >> unsigned int state;
> >> int ret;
> >> @@ -861,12 +904,33 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> if (ret)
> >> return ret;
> >>
> >> - rproc = rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
> >> - if (!rproc)
> >> - return -ENOMEM;
> >
> > This patch doesn't apply to rproc-next - please rebase.
>
> Yes, sure. I forgot to mention in my cover letter that my series has been
> applied and tested on 841c35169323 (Linux 6.8-rc4).
>
> >
> >
> >> + if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
> >> + /*
> >> + * Delegate the firmware management to the secure context.
> >> + * The firmware loaded has to be signed.
> >> + */
> >> + trproc = tee_rproc_register(dev, STM32_MP1_M4_PROC_ID);
> >> + if (IS_ERR(trproc)) {
> >> + dev_err_probe(dev, PTR_ERR(trproc),
> >> + "signed firmware not supported by TEE\n");
> >> + return PTR_ERR(trproc);
> >> + }
> >> + }
> >>
> >> - ddata = rproc->priv;
> >> + rproc = rproc_alloc(dev, np->name,
> >> + trproc ? &st_rproc_tee_ops : &st_rproc_ops,
> >> + NULL, sizeof(*ddata));
> >> + if (!rproc) {
> >> + ret = -ENOMEM;
> >> + goto free_tee;
> >> + }
> >>
> >> + ddata = rproc->priv;
> >> + ddata->trproc = trproc;
> >
> > My opinion hasn't changed from the previous patchet, i.e tee_rproc should be
> > folded in struct rproc as rproc::tee_interface.
>
> Sure, I will do it in next version
>
> >
> > More comments to come shortly...
> >
>
> Thanks!
> Arnaud
>
> >> + if (trproc) {
> >> + rproc->alt_boot = true;
> >> + trproc->rproc = rproc;
> >> + }
> >> rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
> >>
> >> ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
> >> @@ -881,8 +945,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> if (ret)
> >> goto free_rproc;
> >>
> >> - if (state == M4_STATE_CRUN)
> >> + if (state == M4_STATE_CRUN) {
> >> rproc->state = RPROC_DETACHED;
> >> + ddata->to_attach = true;
> >> + }
> >>
> >> rproc->has_iommu = false;
> >> ddata->workqueue = create_workqueue(dev_name(dev));
> >> @@ -916,6 +982,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> device_init_wakeup(dev, false);
> >> }
> >> rproc_free(rproc);
> >> +free_tee:
> >> + if (trproc)
> >> + tee_rproc_unregister(trproc);
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -923,6 +993,7 @@ static void stm32_rproc_remove(struct platform_device *pdev)
> >> {
> >> struct rproc *rproc = platform_get_drvdata(pdev);
> >> struct stm32_rproc *ddata = rproc->priv;
> >> + struct tee_rproc *trproc = ddata->trproc;
> >> struct device *dev = &pdev->dev;
> >>
> >> if (atomic_read(&rproc->power) > 0)
> >> @@ -937,6 +1008,8 @@ static void stm32_rproc_remove(struct platform_device *pdev)
> >> device_init_wakeup(dev, false);
> >> }
> >> rproc_free(rproc);
> >> + if (trproc)
> >> + tee_rproc_unregister(trproc);
> >> }
> >>
> >> static int stm32_rproc_suspend(struct device *dev)
> >> --
> >> 2.25.1
> >>
> >
Hello Mathieu,
On 2/23/24 19:27, Mathieu Poirier wrote:
> On Wed, Feb 14, 2024 at 06:21:21PM +0100, Arnaud Pouliquen wrote:
>> From: Arnaud Pouliquen <[email protected]>
>>
>> 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 device offers a client
>> interface to load a firmware in the secure part.
>> This firmware could be authenticated and decrypted by the secure
>> trusted application.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> update from V2
>> - Use 'tee_rproc' prefix for all functions
>> - rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
>> - redefine fonction to better match with the rproc_ops structure format
>> - replace 'struct tee_rproc' parameter by 'struct rproc' parameter
>> - rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
>> and rework it to remove the cached_table management.
>> - introduce tee_rproc_get_context() to get the tee_rproc struct from the
>> rproc struct
>> - rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
>> - remove useless check on tee_rproc_ctx structure in tee_rproc_register()
>> and tee_rproc_unregister()
>> - fix test on the return of tee_rproc_ctx = devm_kzalloc()
>> - remove useless includes and unused tee_rproc_mem structure.
>> ---
>> drivers/remoteproc/Kconfig | 9 +
>> drivers/remoteproc/Makefile | 1 +
>> drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
>> include/linux/tee_remoteproc.h | 102 +++++++
>> 4 files changed, 509 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..85299606806c 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
>>
>> It's safe to say N if not interested in using RPU r5f cores.
>>
>> +
>> +config TEE_REMOTEPROC
>> + tristate "trusted firmware support by a TEE application"
>> + depends on OPTEE
>> + help
>> + Support for trusted remote processors firmware. The firmware
>> + authentication and/or decryption are managed by a trusted application.
>> + 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..ac727e062d00
>> --- /dev/null
>> +++ b/drivers/remoteproc/tee_remoteproc.c
>> @@ -0,0 +1,397 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) STMicroelectronics 2023 - 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 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
>> +{
>> + struct tee_rproc *entry, *tmp;
>> +
>> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
>> + if (entry->rproc == rproc)
>> + return entry;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + struct tee_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + 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);
>> +
>> + 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_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + int ret;
>> +
>> + if (!trproc)
>> + return ERR_PTR(-EINVAL);
>> +
>> + 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. */
>> + trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
>> + if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
>
> Here we are missing:
>
> tee_rproc_parse_fw()
Please tell me if I'm wrong but the aim of this ops is to parse the firmware
before loading it in case of some resources shoukd be needed before.
but here to parse it we load. I'm not sure that this function makes sense here.
> {
> tee_rproc_load_fw()
> resource_table = tee_rproc_get_loaded_rsc_table()
>
> //check error conditions here
>
> rproc->cached_table = resource_table;
> rproc->table_ptr = resource_table;
This seems to me that it is not possible regarding
the "memcpy(loaded_table, rproc->cached_table, rproc->table_sz)"[1] in
rproc_start() and the kfree(rproc->cached_table) [2] in rproc_shutdown(). We
would copy with the same source and destibnation address.
In this case a memory should be allocated for the rproc->cached_table.
> }
>
> This is essentially the same as rproc_elf_load_rsc_table(). That way we don't
> need rproc_alt_fw_boot() and rproc_load_segments() doesn't have to be moved
> around.
The trusted application simply needs to know that if the firmware is
> already loaded, it has to return.
Today trying to load twice time is considered as an error in OP-TEE [3].
As it is a constraint introduced by Linux, I would prefer treated it in the Linux.
What about introduce a "fw_loaded" flag in tee_rproc__context?
[1]
https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/remoteproc/remoteproc_core.c#L1289
[2]
https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/remoteproc/remoteproc_core.c#L2024
[3]
https://elixir.bootlin.com/op-tee/latest/source/ta/remoteproc/src/remoteproc_core.c#L896
Regards,
Arnaud
>
>> +
>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
>> + const struct firmware *fw)
>> +{
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + size_t table_sz;
>> +
>> + if (!trproc)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (!trproc->rsc_table)
>> + trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
>> +
>> + return trproc->rsc_table;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
>> +
>> +int tee_rproc_start(struct rproc *rproc)
>> +{
>> + struct tee_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + int ret;
>> +
>> + if (!trproc)
>> + return -EINVAL;
>> +
>> + 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;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_start);
>> +
>> +int tee_rproc_stop(struct rproc *rproc)
>> +{
>> + struct tee_ioctl_invoke_arg arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>> + int ret;
>> +
>> + if (!trproc)
>> + return -EINVAL;
>> +
>> + 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;
>> + }
>> + if (trproc->rsc_table)
>> + iounmap(trproc->rsc_table);
>> + trproc->rsc_table = NULL;
>> +
>> + 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, unsigned int rproc_id)
>> +{
>> + struct tee_client_device *tee_device;
>> + struct tee_ioctl_open_session_arg sess_arg;
>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>> + struct tee_rproc *trproc;
>> + int ret;
>> +
>> + /*
>> + * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
>> + * probed or service not available in the secure firmware)
>> + * Assumption here is that the TEE bus is not probed.
>> + */
>> + if (!tee_rproc_ctx)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
>> + if (!trproc)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
>> + memset(&sess_arg, 0, sizeof(sess_arg));
>> +
>> + /* Open session with rproc_tee load the OP-TEE Trusted Application */
>> + 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);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + trproc->parent = dev;
>> + trproc->rproc_id = rproc_id;
>> + trproc->session_id = sess_arg.session;
>> +
>> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
>> +
>> + return trproc;
>> +}
>> +EXPORT_SYMBOL_GPL(tee_rproc_register);
>> +
>> +int tee_rproc_unregister(struct tee_rproc *trproc)
>> +{
>> + 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);
>> +
>> + 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
>> new file mode 100644
>> index 000000000000..7c9e91e989ba
>> --- /dev/null
>> +++ b/include/linux/tee_remoteproc.h
>> @@ -0,0 +1,102 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright(c) 2023 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
>> + * @rsc_table: Resource table virtual address.
>> + */
>> +struct tee_rproc {
>> + struct list_head node;
>> + struct rproc *rproc;
>> + struct device *parent;
>> + u32 rproc_id;
>> + u32 session_id;
>> + struct resource_table *rsc_table;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
>> +
>> +struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
>> +int tee_rproc_unregister(struct tee_rproc *trproc);
>> +
>> +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);
>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
>> + const struct firmware *fw);
>> +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, unsigned int rproc_id)
>> +{
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
>> +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;
>> +}
>> +
>> +static inline struct resource_table *
>> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return NULL;
>> +}
>> +#endif /* CONFIG_TEE_REMOTEPROC */
>> +#endif /* TEE_REMOTEPROC_H */
>> --
>> 2.25.1
>>
Good morning,
On Wed, Feb 28, 2024 at 09:20:28AM +0100, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
>
> On 2/23/24 19:27, Mathieu Poirier wrote:
> > On Wed, Feb 14, 2024 at 06:21:21PM +0100, Arnaud Pouliquen wrote:
> >> From: Arnaud Pouliquen <[email protected]>
> >>
> >> 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 device offers a client
> >> interface to load a firmware in the secure part.
> >> This firmware could be authenticated and decrypted by the secure
> >> trusted application.
> >>
> >> Signed-off-by: Arnaud Pouliquen <[email protected]>
> >> ---
> >> update from V2
> >> - Use 'tee_rproc' prefix for all functions
> >> - rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
> >> - redefine fonction to better match with the rproc_ops structure format
> >> - replace 'struct tee_rproc' parameter by 'struct rproc' parameter
> >> - rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
> >> and rework it to remove the cached_table management.
> >> - introduce tee_rproc_get_context() to get the tee_rproc struct from the
> >> rproc struct
> >> - rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
> >> - remove useless check on tee_rproc_ctx structure in tee_rproc_register()
> >> and tee_rproc_unregister()
> >> - fix test on the return of tee_rproc_ctx = devm_kzalloc()
> >> - remove useless includes and unused tee_rproc_mem structure.
> >> ---
> >> drivers/remoteproc/Kconfig | 9 +
> >> drivers/remoteproc/Makefile | 1 +
> >> drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
> >> include/linux/tee_remoteproc.h | 102 +++++++
> >> 4 files changed, 509 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..85299606806c 100644
> >> --- a/drivers/remoteproc/Kconfig
> >> +++ b/drivers/remoteproc/Kconfig
> >> @@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
> >>
> >> It's safe to say N if not interested in using RPU r5f cores.
> >>
> >> +
> >> +config TEE_REMOTEPROC
> >> + tristate "trusted firmware support by a TEE application"
> >> + depends on OPTEE
> >> + help
> >> + Support for trusted remote processors firmware. The firmware
> >> + authentication and/or decryption are managed by a trusted application.
> >> + 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..ac727e062d00
> >> --- /dev/null
> >> +++ b/drivers/remoteproc/tee_remoteproc.c
> >> @@ -0,0 +1,397 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2023 - 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 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
> >> +{
> >> + struct tee_rproc *entry, *tmp;
> >> +
> >> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
> >> + if (entry->rproc == rproc)
> >> + return entry;
> >> + }
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> >> + 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);
> >> +
> >> + 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_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> >> + int ret;
> >> +
> >> + if (!trproc)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + 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. */
> >> + trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
> >> + if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
> >
> > Here we are missing:
> >
> > tee_rproc_parse_fw()
>
> Please tell me if I'm wrong but the aim of this ops is to parse the firmware
> before loading it in case of some resources shoukd be needed before.
> but here to parse it we load. I'm not sure that this function makes sense here.
>
The documentation for rproc_ops::parse_fw [1] indicate the operation
is to extract the resource table from the firmware image, something that is
echoed in by the implementation of rproc_elf_load_rsc_table(). No matter what
the secure side needs or how it does it, this function should return the address
of the resource table.
[1]. https://elixir.bootlin.com/linux/v6.8-rc6/source/include/linux/remoteproc.h#L370
> > {
> > tee_rproc_load_fw()
> > resource_table = tee_rproc_get_loaded_rsc_table()
> >
> > //check error conditions here
> >
> > rproc->cached_table = resource_table;
> > rproc->table_ptr = resource_table;
>
> This seems to me that it is not possible regarding
> the "memcpy(loaded_table, rproc->cached_table, rproc->table_sz)"[1] in
> rproc_start() and the kfree(rproc->cached_table) [2] in rproc_shutdown(). We
> would copy with the same source and destibnation address.
> In this case a memory should be allocated for the rproc->cached_table.
>
I touched base on how to fix the handling of the resource table in rproc_start()
in an earlier reply, but because of the kfree() in rproc_shutdown, I agree
rproc->cached_table should be set to NULL. That said, there needs to be a
detailed explanation of what is happening in rproc_start().
>
> > }
> >
> > This is essentially the same as rproc_elf_load_rsc_table(). That way we don't
> > need rproc_alt_fw_boot() and rproc_load_segments() doesn't have to be moved
> > around.
>
> The trusted application simply needs to know that if the firmware is
> > already loaded, it has to return.
>
> Today trying to load twice time is considered as an error in OP-TEE [3].
That can be fixed.
> As it is a constraint introduced by Linux, I would prefer treated it in the Linux.
Things in the remoteproc subsystem have been working just fine for well over a
decade. I'd say this is introduced by contraints from an image that is handled
by the secure world.
>
> What about introduce a "fw_loaded" flag in tee_rproc__context?
I'd like to avoid that since the flag is associated with the specific way the
trusted application works. That flag surely won't apply, or new flags will be
introduced, when another trusted application to handle the remote processor's
firmware is introduced.
>
>
> [1]
> https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/remoteproc/remoteproc_core.c#L1289
> [2]
> https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/remoteproc/remoteproc_core.c#L2024
> [3]
> https://elixir.bootlin.com/op-tee/latest/source/ta/remoteproc/src/remoteproc_core.c#L896
>
>
> Regards,
> Arnaud
>
> >
> >> +
> >> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> >> + const struct firmware *fw)
> >> +{
> >> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> >> + size_t table_sz;
> >> +
> >> + if (!trproc)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + if (!trproc->rsc_table)
> >> + trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> >> +
> >> + return trproc->rsc_table;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
> >> +
> >> +int tee_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> >> + int ret;
> >> +
> >> + if (!trproc)
> >> + return -EINVAL;
> >> +
> >> + 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;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_start);
> >> +
> >> +int tee_rproc_stop(struct rproc *rproc)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
> >> + int ret;
> >> +
> >> + if (!trproc)
> >> + return -EINVAL;
> >> +
> >> + 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;
> >> + }
> >> + if (trproc->rsc_table)
> >> + iounmap(trproc->rsc_table);
> >> + trproc->rsc_table = NULL;
> >> +
> >> + 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, unsigned int rproc_id)
> >> +{
> >> + struct tee_client_device *tee_device;
> >> + struct tee_ioctl_open_session_arg sess_arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc;
> >> + int ret;
> >> +
> >> + /*
> >> + * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
> >> + * probed or service not available in the secure firmware)
> >> + * Assumption here is that the TEE bus is not probed.
> >> + */
> >> + if (!tee_rproc_ctx)
> >> + return ERR_PTR(-EPROBE_DEFER);
> >> +
> >> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
> >> + if (!trproc)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
> >> + memset(&sess_arg, 0, sizeof(sess_arg));
> >> +
> >> + /* Open session with rproc_tee load the OP-TEE Trusted Application */
> >> + 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);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + trproc->parent = dev;
> >> + trproc->rproc_id = rproc_id;
> >> + trproc->session_id = sess_arg.session;
> >> +
> >> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
> >> +
> >> + return trproc;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_register);
> >> +
> >> +int tee_rproc_unregister(struct tee_rproc *trproc)
> >> +{
> >> + 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);
> >> +
> >> + 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
> >> new file mode 100644
> >> index 000000000000..7c9e91e989ba
> >> --- /dev/null
> >> +++ b/include/linux/tee_remoteproc.h
> >> @@ -0,0 +1,102 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright(c) 2023 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
> >> + * @rsc_table: Resource table virtual address.
> >> + */
> >> +struct tee_rproc {
> >> + struct list_head node;
> >> + struct rproc *rproc;
> >> + struct device *parent;
> >> + u32 rproc_id;
> >> + u32 session_id;
> >> + struct resource_table *rsc_table;
> >> +};
> >> +
> >> +#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
> >> +
> >> +struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
> >> +int tee_rproc_unregister(struct tee_rproc *trproc);
> >> +
> >> +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);
> >> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> >> + const struct firmware *fw);
> >> +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, unsigned int rproc_id)
> >> +{
> >> + return ERR_PTR(-ENODEV);
> >> +}
> >> +
> >> +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;
> >> +}
> >> +
> >> +static inline struct resource_table *
> >> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return NULL;
> >> +}
> >> +#endif /* CONFIG_TEE_REMOTEPROC */
> >> +#endif /* TEE_REMOTEPROC_H */
> >> --
> >> 2.25.1
> >>
On 2/23/24 19:37, Mathieu Poirier wrote:
> On Fri, Feb 23, 2024 at 02:54:13PM +0100, Arnaud POULIQUEN wrote:
>> Hello Mathieu,
>>
>> On 2/22/24 20:02, Mathieu Poirier wrote:
>>> Hi,
>>>
>>> On Wed, Feb 14, 2024 at 06:21:27PM +0100, Arnaud Pouliquen wrote:
>>>> The new TEE remoteproc device is used to manage remote firmware in a
>>>> secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
>>>> introduced to delegate the loading of the firmware to the trusted
>>>> execution context. In such cases, the firmware should be signed and
>>>> adhere to the image format defined by the TEE.
>>>>
>>>> A new "to_attach" field is introduced to differentiate the use cases
>>>> "firmware loaded by the boot stage" and "firmware loaded by the TEE".
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>>>> ---
>>>> V2 to V3 update:
>>>> - remove stm32_rproc_tee_elf_sanity_check(), stm32_rproc_tee_elf_load()
>>>> stm32_rproc_tee_elf_find_loaded_rsc_table() and stm32_rproc_tee_start() that are bnow unused
>>>> - use new rproc::alt_boot field to sepcify that the alternate fboot method is used
>>>> - use stm32_rproc::to_attach field to differenciate attch mode from remoteproc tee boot mode.
>>>> - remove the used of stm32_rproc::fw_loaded
>>>> ---
>>>> drivers/remoteproc/stm32_rproc.c | 85 +++++++++++++++++++++++++++++---
>>>> 1 file changed, 79 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>>>> index fcc0001e2657..9cfcf66462e0 100644
>>>> --- a/drivers/remoteproc/stm32_rproc.c
>>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>>> @@ -20,6 +20,7 @@
>>>> #include <linux/remoteproc.h>
>>>> #include <linux/reset.h>
>>>> #include <linux/slab.h>
>>>> +#include <linux/tee_remoteproc.h>
>>>> #include <linux/workqueue.h>
>>>>
>>>> #include "remoteproc_internal.h"
>>>> @@ -49,6 +50,9 @@
>>>> #define M4_STATE_STANDBY 4
>>>> #define M4_STATE_CRASH 5
>>>>
>>>> +/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
>>>> +#define STM32_MP1_M4_PROC_ID 0
>>>> +
>>>> struct stm32_syscon {
>>>> struct regmap *map;
>>>> u32 reg;
>>>> @@ -90,6 +94,8 @@ struct stm32_rproc {
>>>> struct stm32_mbox mb[MBOX_NB_MBX];
>>>> struct workqueue_struct *workqueue;
>>>> bool hold_boot_smc;
>>>> + bool to_attach;
>>>> + struct tee_rproc *trproc;
>>>> void __iomem *rsc_va;
>>>> };
>>>>
>>>> @@ -253,10 +259,30 @@ static int stm32_rproc_release(struct rproc *rproc)
>>>> return err;
>>>> }
>>>> }
>>>> + ddata->to_attach = false;
>>>>
>>>> return err;
>>>> }
>>>>
>>>> +static int stm32_rproc_tee_attach(struct rproc *rproc)
>>>> +{
>>>> + /* Nothing to do, remote proc already started by the secured context. */
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int stm32_rproc_tee_stop(struct rproc *rproc)
>>>> +{
>>>> + int err;
>>>> +
>>>> + stm32_rproc_request_shutdown(rproc);
>>>> +
>>>> + err = tee_rproc_stop(rproc);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + return stm32_rproc_release(rproc);
>>>> +}
>>>> +
>>>> static int stm32_rproc_prepare(struct rproc *rproc)
>>>> {
>>>> struct device *dev = rproc->dev.parent;
>>>> @@ -637,10 +663,14 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>>>> {
>>>> struct stm32_rproc *ddata = rproc->priv;
>>>> struct device *dev = rproc->dev.parent;
>>>> + struct tee_rproc *trproc = ddata->trproc;
>>>> phys_addr_t rsc_pa;
>>>> u32 rsc_da;
>>>> int err;
>>>>
>>>> + if (trproc && !ddata->to_attach)
>>>> + return tee_rproc_get_loaded_rsc_table(rproc, table_sz);
>>>> +
>>>
>>> Why do we need a flag at all? Why can't st_rproc_tee_ops::get_loaded_rsc_table
>>> be set to tee_rproc_get_loaded_rsc_table()?
>>
>>
>> This function is used to retrieve the address of the resource table in 3 cases
>> - attach to a firmware started by the boot loader (U-boot).
>> - load of the firmware by OP-TEE.
>> - crash recovery on a signed firmware started by the boot loader.
>>
>> The flag is used to differentiate the attch from the other uses cases
>> For instance we support this use case.
>> 1) attach to the firmware on boot
>> 2) crash during runtime
>> 2a) stop the firmware by OP-TEE( ddata->to_attach set to 0)
>> 2b) load the firmware by OP-TEE
>> 2c) get the loaded resource table from OP-TEE (we can not guaranty
>> that the firmware loaded on recovery is the same)
>> 2d) restart the firmware by OP-TEE
>
> This is not maintainable and needs to be broken down into smaller
> building blocks. The introduction of tee_rproc_parse_fw() should help dealing
> with some of the complexity.
The use cases I mentioned are supported by the legacy, if firmware is not
authenticated by a Trusted Application.
No problem to addressed this in a second step.
I will remove this constrain from this series in next version.
Regards,
Arnaud
>
>>
>>>
>>>> /* The resource table has already been mapped, nothing to do */
>>>> if (ddata->rsc_va)
>>>> goto done;
>>>> @@ -693,8 +723,20 @@ static const struct rproc_ops st_rproc_ops = {
>>>> .get_boot_addr = rproc_elf_get_boot_addr,
>>>> };
>>>>
>>>> +static const struct rproc_ops st_rproc_tee_ops = {
>>>> + .prepare = stm32_rproc_prepare,
>>>> + .start = tee_rproc_start,
>>>> + .stop = stm32_rproc_tee_stop,
>>>> + .attach = stm32_rproc_tee_attach,
>>>> + .kick = stm32_rproc_kick,
>>>> + .get_loaded_rsc_table = stm32_rproc_get_loaded_rsc_table,
>>>> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
>>>> + .load = tee_rproc_load_fw,
>>>> +};
>>>> +
>>>> static const struct of_device_id stm32_rproc_match[] = {
>>>> - { .compatible = "st,stm32mp1-m4" },
>>>> + {.compatible = "st,stm32mp1-m4",},
>>>> + {.compatible = "st,stm32mp1-m4-tee",},
>>>> {},
>>>> };
>>>> MODULE_DEVICE_TABLE(of, stm32_rproc_match);
>>>> @@ -853,6 +895,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>>> struct device *dev = &pdev->dev;
>>>> struct stm32_rproc *ddata;
>>>> struct device_node *np = dev->of_node;
>>>> + struct tee_rproc *trproc = NULL;
>>>> struct rproc *rproc;
>>>> unsigned int state;
>>>> int ret;
>>>> @@ -861,12 +904,33 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - rproc = rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
>>>> - if (!rproc)
>>>> - return -ENOMEM;
>>>
>>> This patch doesn't apply to rproc-next - please rebase.
>>
>> Yes, sure. I forgot to mention in my cover letter that my series has been
>> applied and tested on 841c35169323 (Linux 6.8-rc4).
>>
>>>
>>>
>>>> + if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
>>>> + /*
>>>> + * Delegate the firmware management to the secure context.
>>>> + * The firmware loaded has to be signed.
>>>> + */
>>>> + trproc = tee_rproc_register(dev, STM32_MP1_M4_PROC_ID);
>>>> + if (IS_ERR(trproc)) {
>>>> + dev_err_probe(dev, PTR_ERR(trproc),
>>>> + "signed firmware not supported by TEE\n");
>>>> + return PTR_ERR(trproc);
>>>> + }
>>>> + }
>>>>
>>>> - ddata = rproc->priv;
>>>> + rproc = rproc_alloc(dev, np->name,
>>>> + trproc ? &st_rproc_tee_ops : &st_rproc_ops,
>>>> + NULL, sizeof(*ddata));
>>>> + if (!rproc) {
>>>> + ret = -ENOMEM;
>>>> + goto free_tee;
>>>> + }
>>>>
>>>> + ddata = rproc->priv;
>>>> + ddata->trproc = trproc;
>>>
>>> My opinion hasn't changed from the previous patchet, i.e tee_rproc should be
>>> folded in struct rproc as rproc::tee_interface.
>>
>> Sure, I will do it in next version
>>
>>>
>>> More comments to come shortly...
>>>
>>
>> Thanks!
>> Arnaud
>>
>>>> + if (trproc) {
>>>> + rproc->alt_boot = true;
>>>> + trproc->rproc = rproc;
>>>> + }
>>>> rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
>>>>
>>>> ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
>>>> @@ -881,8 +945,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>>> if (ret)
>>>> goto free_rproc;
>>>>
>>>> - if (state == M4_STATE_CRUN)
>>>> + if (state == M4_STATE_CRUN) {
>>>> rproc->state = RPROC_DETACHED;
>>>> + ddata->to_attach = true;
>>>> + }
>>>>
>>>> rproc->has_iommu = false;
>>>> ddata->workqueue = create_workqueue(dev_name(dev));
>>>> @@ -916,6 +982,10 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>>> device_init_wakeup(dev, false);
>>>> }
>>>> rproc_free(rproc);
>>>> +free_tee:
>>>> + if (trproc)
>>>> + tee_rproc_unregister(trproc);
>>>> +
>>>> return ret;
>>>> }
>>>>
>>>> @@ -923,6 +993,7 @@ static void stm32_rproc_remove(struct platform_device *pdev)
>>>> {
>>>> struct rproc *rproc = platform_get_drvdata(pdev);
>>>> struct stm32_rproc *ddata = rproc->priv;
>>>> + struct tee_rproc *trproc = ddata->trproc;
>>>> struct device *dev = &pdev->dev;
>>>>
>>>> if (atomic_read(&rproc->power) > 0)
>>>> @@ -937,6 +1008,8 @@ static void stm32_rproc_remove(struct platform_device *pdev)
>>>> device_init_wakeup(dev, false);
>>>> }
>>>> rproc_free(rproc);
>>>> + if (trproc)
>>>> + tee_rproc_unregister(trproc);
>>>> }
>>>>
>>>> static int stm32_rproc_suspend(struct device *dev)
>>>> --
>>>> 2.25.1
>>>>
>>>
Hello Mathieu,
On 2/29/24 17:19, Mathieu Poirier wrote:
> Good morning,
>
> On Wed, Feb 28, 2024 at 09:20:28AM +0100, Arnaud POULIQUEN wrote:
>> Hello Mathieu,
>>
>>
>> On 2/23/24 19:27, Mathieu Poirier wrote:
>>> On Wed, Feb 14, 2024 at 06:21:21PM +0100, Arnaud Pouliquen wrote:
>>>> From: Arnaud Pouliquen <[email protected]>
>>>>
>>>> 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 device offers a client
>>>> interface to load a firmware in the secure part.
>>>> This firmware could be authenticated and decrypted by the secure
>>>> trusted application.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>>>> ---
>>>> update from V2
>>>> - Use 'tee_rproc' prefix for all functions
>>>> - rename tee_rproc_get_loaded_rsc_table to tee_rproc_find_loaded_rsc_table
>>>> - redefine fonction to better match with the rproc_ops structure format
>>>> - replace 'struct tee_rproc' parameter by 'struct rproc' parameter
>>>> - rename 'rproc_tee_get_rsc_table()' to tee_rproc_get_loaded_rsc_table()
>>>> and rework it to remove the cached_table management.
>>>> - introduce tee_rproc_get_context() to get the tee_rproc struct from the
>>>> rproc struct
>>>> - rename tee_rproc_get_loaded_rsc_table() to tee_rproc_find_loaded_rsc_table()
>>>> - remove useless check on tee_rproc_ctx structure in tee_rproc_register()
>>>> and tee_rproc_unregister()
>>>> - fix test on the return of tee_rproc_ctx = devm_kzalloc()
>>>> - remove useless includes and unused tee_rproc_mem structure.
>>>> ---
>>>> drivers/remoteproc/Kconfig | 9 +
>>>> drivers/remoteproc/Makefile | 1 +
>>>> drivers/remoteproc/tee_remoteproc.c | 397 ++++++++++++++++++++++++++++
>>>> include/linux/tee_remoteproc.h | 102 +++++++
>>>> 4 files changed, 509 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..85299606806c 100644
>>>> --- a/drivers/remoteproc/Kconfig
>>>> +++ b/drivers/remoteproc/Kconfig
>>>> @@ -365,6 +365,15 @@ config XLNX_R5_REMOTEPROC
>>>>
>>>> It's safe to say N if not interested in using RPU r5f cores.
>>>>
>>>> +
>>>> +config TEE_REMOTEPROC
>>>> + tristate "trusted firmware support by a TEE application"
>>>> + depends on OPTEE
>>>> + help
>>>> + Support for trusted remote processors firmware. The firmware
>>>> + authentication and/or decryption are managed by a trusted application.
>>>> + 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..ac727e062d00
>>>> --- /dev/null
>>>> +++ b/drivers/remoteproc/tee_remoteproc.c
>>>> @@ -0,0 +1,397 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + * Copyright (C) STMicroelectronics 2023 - 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 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 struct tee_rproc *tee_rproc_get_context(struct rproc *rproc)
>>>> +{
>>>> + struct tee_rproc *entry, *tmp;
>>>> +
>>>> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
>>>> + if (entry->rproc == rproc)
>>>> + return entry;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
>>>> +{
>>>> + struct tee_ioctl_invoke_arg arg;
>>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>>>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>>>> + 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);
>>>> +
>>>> + 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_ioctl_invoke_arg arg;
>>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>>>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>>>> + int ret;
>>>> +
>>>> + if (!trproc)
>>>> + return ERR_PTR(-EINVAL);
>>>> +
>>>> + 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. */
>>>> + trproc->rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
>>>> + if (IS_ERR_OR_NULL(trproc->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 trproc->rsc_table;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
>>>
>>> Here we are missing:
>>>
>>> tee_rproc_parse_fw()
>>
>> Please tell me if I'm wrong but the aim of this ops is to parse the firmware
>> before loading it in case of some resources shoukd be needed before.
>> but here to parse it we load. I'm not sure that this function makes sense here.
>>
>
> The documentation for rproc_ops::parse_fw [1] indicate the operation
> is to extract the resource table from the firmware image, something that is
> echoed in by the implementation of rproc_elf_load_rsc_table(). No matter what
> the secure side needs or how it does it, this function should return the address
> of the resource table.
>
> [1]. https://elixir.bootlin.com/linux/v6.8-rc6/source/include/linux/remoteproc.h#L370
>
>>> {
>>> tee_rproc_load_fw()
>>> resource_table = tee_rproc_get_loaded_rsc_table()
>>>
>>> //check error conditions here
>>>
>>> rproc->cached_table = resource_table;
>>> rproc->table_ptr = resource_table;
>>
>> This seems to me that it is not possible regarding
>> the "memcpy(loaded_table, rproc->cached_table, rproc->table_sz)"[1] in
>> rproc_start() and the kfree(rproc->cached_table) [2] in rproc_shutdown(). We
>> would copy with the same source and destibnation address.
>> In this case a memory should be allocated for the rproc->cached_table.
>>
>
> I touched base on how to fix the handling of the resource table in rproc_start()
> in an earlier reply, but because of the kfree() in rproc_shutdown, I agree
> rproc->cached_table should be set to NULL. That said, there needs to be a
> detailed explanation of what is happening in rproc_start().
>
>>
>>> }
>>>
>>> This is essentially the same as rproc_elf_load_rsc_table(). That way we don't
>>> need rproc_alt_fw_boot() and rproc_load_segments() doesn't have to be moved
>>> around.
>>
>> The trusted application simply needs to know that if the firmware is
>>> already loaded, it has to return.
>>
>> Today trying to load twice time is considered as an error in OP-TEE [3].
>
> That can be fixed.
>
>> As it is a constraint introduced by Linux, I would prefer treated it in the Linux.
>
> Things in the remoteproc subsystem have been working just fine for well over a
> decade. I'd say this is introduced by contraints from an image that is handled
> by the secure world.
>
>>
>> What about introduce a "fw_loaded" flag in tee_rproc__context?
>
> I'd like to avoid that since the flag is associated with the specific way the
> trusted application works. That flag surely won't apply, or new flags will be
> introduced, when another trusted application to handle the remote processor's
> firmware is introduced.
>
Let's move in this direction, I can try to change the behaviour in OP-TEE on
Load and if it is not accepted we can rediscuss the use of a flag.
I will come back with a new version based on your proposals.
Thanks,
Arnaud
>>
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/remoteproc/remoteproc_core.c#L1289
>> [2]
>> https://elixir.bootlin.com/linux/v6.8-rc6/source/drivers/remoteproc/remoteproc_core.c#L2024
>> [3]
>> https://elixir.bootlin.com/op-tee/latest/source/ta/remoteproc/src/remoteproc_core.c#L896
>>
>>
>> Regards,
>> Arnaud
>>
>>>
>>>> +
>>>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
>>>> + const struct firmware *fw)
>>>> +{
>>>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>>>> + size_t table_sz;
>>>> +
>>>> + if (!trproc)
>>>> + return ERR_PTR(-EINVAL);
>>>> +
>>>> + if (!trproc->rsc_table)
>>>> + trproc->rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
>>>> +
>>>> + return trproc->rsc_table;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
>>>> +
>>>> +int tee_rproc_start(struct rproc *rproc)
>>>> +{
>>>> + struct tee_ioctl_invoke_arg arg;
>>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>>>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>>>> + int ret;
>>>> +
>>>> + if (!trproc)
>>>> + return -EINVAL;
>>>> +
>>>> + 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;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tee_rproc_start);
>>>> +
>>>> +int tee_rproc_stop(struct rproc *rproc)
>>>> +{
>>>> + struct tee_ioctl_invoke_arg arg;
>>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>>>> + struct tee_rproc *trproc = tee_rproc_get_context(rproc);
>>>> + int ret;
>>>> +
>>>> + if (!trproc)
>>>> + return -EINVAL;
>>>> +
>>>> + 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;
>>>> + }
>>>> + if (trproc->rsc_table)
>>>> + iounmap(trproc->rsc_table);
>>>> + trproc->rsc_table = NULL;
>>>> +
>>>> + 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, unsigned int rproc_id)
>>>> +{
>>>> + struct tee_client_device *tee_device;
>>>> + struct tee_ioctl_open_session_arg sess_arg;
>>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
>>>> + struct tee_rproc *trproc;
>>>> + int ret;
>>>> +
>>>> + /*
>>>> + * The device is not probed by the TEE bus. We ignore the reason (bus could be not yet
>>>> + * probed or service not available in the secure firmware)
>>>> + * Assumption here is that the TEE bus is not probed.
>>>> + */
>>>> + if (!tee_rproc_ctx)
>>>> + return ERR_PTR(-EPROBE_DEFER);
>>>> +
>>>> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
>>>> + if (!trproc)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
>>>> + memset(&sess_arg, 0, sizeof(sess_arg));
>>>> +
>>>> + /* Open session with rproc_tee load the OP-TEE Trusted Application */
>>>> + 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);
>>>> + return ERR_PTR(-EINVAL);
>>>> + }
>>>> +
>>>> + trproc->parent = dev;
>>>> + trproc->rproc_id = rproc_id;
>>>> + trproc->session_id = sess_arg.session;
>>>> +
>>>> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
>>>> +
>>>> + return trproc;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tee_rproc_register);
>>>> +
>>>> +int tee_rproc_unregister(struct tee_rproc *trproc)
>>>> +{
>>>> + 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);
>>>> +
>>>> + 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/tee_remoteproc.h b/include/linux/tee_remoteproc.h
>>>> new file mode 100644
>>>> index 000000000000..7c9e91e989ba
>>>> --- /dev/null
>>>> +++ b/include/linux/tee_remoteproc.h
>>>> @@ -0,0 +1,102 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright(c) 2023 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
>>>> + * @rsc_table: Resource table virtual address.
>>>> + */
>>>> +struct tee_rproc {
>>>> + struct list_head node;
>>>> + struct rproc *rproc;
>>>> + struct device *parent;
>>>> + u32 rproc_id;
>>>> + u32 session_id;
>>>> + struct resource_table *rsc_table;
>>>> +};
>>>> +
>>>> +#if IS_ENABLED(CONFIG_TEE_REMOTEPROC)
>>>> +
>>>> +struct tee_rproc *tee_rproc_register(struct device *dev, unsigned int rproc_id);
>>>> +int tee_rproc_unregister(struct tee_rproc *trproc);
>>>> +
>>>> +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);
>>>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
>>>> + const struct firmware *fw);
>>>> +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, unsigned int rproc_id)
>>>> +{
>>>> + return ERR_PTR(-ENODEV);
>>>> +}
>>>> +
>>>> +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;
>>>> +}
>>>> +
>>>> +static inline struct resource_table *
>>>> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
>>>> +{
>>>> + /* This shouldn't be possible */
>>>> + WARN_ON(1);
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +#endif /* CONFIG_TEE_REMOTEPROC */
>>>> +#endif /* TEE_REMOTEPROC_H */
>>>> --
>>>> 2.25.1
>>>>