Core: Introduce stm32mp remoteproc PTA and driver #6470

arnopo · 2023-11-17T10:38:49Z

This series introduces the stm32mp remoteproc PTA and the stm32_remoteproc driver , which enable the authentication of signed images for the stm32mp157 platform.

The remote proc TA implements platform-specific requirements associated to the remoteproc PTA API introduced in Introduce remote proc trusted application #6300. The API commit is also part of this PR for build purposes, and should be removed if Introduce remote proc trusted application #6300 is merged.
The stm32mp_remoteproc driver is responsible for relying on the device tree, which is very similar to the Linux one. Note that the remoteproc node is a sub-node of the AHB simple bus. The AHB bus defines the dma-range property that specifoes memory translation between the Cortex-M4 mapping and the Cortex-A7 mapping. This is necessary to find the address translation between the address found in the ELF file and the address to load the segment. For instance, the RETRAM address is 0 from the Cortex-M4 perspective and 0x38000000 from the Cortex-A7 perspective.

For more details a presentation is available here: LHR23-214 OP-TEE to extend the root of trust to the coprocessor

Related pull request:

[RFC PATCH] introduction of a remoteproc framework to load signed coprocessor firmware images #6027
Introduce remote proc trusted application #6300
Code associated to the Linux Kernel is proposed as RFC for upstream:

https://lore.kernel.org/lkml/20230523091350.292221-1-arnaud.pouliquen@foss.st.com/

etienne-lms

Commit "drivers: Add stm32mp1 remoteproc driver" should define a default value for CFG_DRIVERS_REMOTEPROC and provide an inline description comment.

Ditto for commit "pta: stm32mp: add new remoteproc pta" regarding CFG_RPROC_PTA.

My main concern here is detailed in my review comment on stm32_rproc_get() function (see #6470 (comment)).
(edited: add a link to my comment)

core/drivers/remoteproc/stm32_remoteproc.c

core/pta/stm32mp/remoteproc_pta.c

arnopo · 2023-12-06T14:40:54Z

Commit "drivers: Add stm32mp1 remoteproc driver" should define a default value for CFG_DRIVERS_REMOTEPROC and provide an inline description comment.

Ditto for commit "pta: stm32mp: add new remoteproc pta" regarding CFG_RPROC_PTA.

Please could you detail where I should declare default value? I was expecting that platforms declare it if supported, else config was set to n by default.

arnopo

I have taken into account @etienne-lms comment or responded to them.
However, I have not yet take into account #6470 (comment). I would waiting feedback from
@jforissier, @jenswi-linaro on the way I propose to use UUID for the platform PTA.

core/drivers/remoteproc/stm32_remoteproc.c

+	return TEE_ERROR_ACCESS_DENIED;
+}
+
+TEE_Result stm32_rproc_unmap(uint32_t firmware_id, paddr_t pa, size_t size)


core/drivers/remoteproc/stm32_remoteproc.c

core/pta/stm32mp/remoteproc_pta.c

etienne-lms

I have few comments on the fixup commits. I'm fine if you squash and rebase once addressed.

etienne-lms · 2023-12-08T15:05:04Z

core/drivers/remoteproc/stm32_remoteproc.c

 	struct stm32_rproc_mem *mems = NULL;
-	vaddr_t *va = NULL;
+	paddr_t pa  = virt_to_phys(va);


could you replace the 2 space chars with a single one?

etienne-lms · 2023-12-08T15:06:52Z

core/drivers/remoteproc/stm32_remoteproc.c

@@ -174,14 +174,14 @@ TEE_Result stm32_rproc_map(uint32_t firmware_id, paddr_t pa, size_t size,
 	return TEE_ERROR_ACCESS_DENIED;
 }

-TEE_Result stm32_rproc_unmap(uint32_t firmware_id, paddr_t pa, size_t size)
+TEE_Result stm32_rproc_unmap(uint32_t rproc_id, vaddr_t *va, size_t size)


this passes a va or a pointer to a va?
I think it should be either void *va or vaddr_t va here.

good catch, I will keep void * to be aligned with core_mmu_add_mapping and core_mmu_add_mapping functions

etienne-lms · 2023-12-08T15:08:16Z

core/drivers/remoteproc/stm32_remoteproc.c

 	unsigned int i = 0;

-	if (!rproc)
+	if (!rproc || !va)


!va or !pa (note: pa will be 0 if va is 0)

Right, pa seems more interesting to test as set to 0 if virt_to_phys fails.

core/drivers/remoteproc/stm32_remoteproc.c

etienne-lms · 2023-12-08T15:27:17Z

core/pta/stm32mp/remoteproc_pta.c

+						TEE_PARAM_TYPE_NONE,
+						TEE_PARAM_TYPE_NONE,
+						TEE_PARAM_TYPE_NONE);
+	struct ts_ctx *ctx;


arnopo

As requested by @etienne-lms, I have updated the PR based on comment, squashed the fixups and rebased the PR.
On top of that I have added a new fixup that adds additional fixes.

arnopo · 2023-12-12T13:30:53Z

core/drivers/remoteproc/stm32_remoteproc.c

@@ -174,14 +174,14 @@ TEE_Result stm32_rproc_map(uint32_t firmware_id, paddr_t pa, size_t size,
 	return TEE_ERROR_ACCESS_DENIED;
 }

-TEE_Result stm32_rproc_unmap(uint32_t firmware_id, paddr_t pa, size_t size)
+TEE_Result stm32_rproc_unmap(uint32_t rproc_id, vaddr_t *va, size_t size)


good catch, I will keep void * to be aligned with core_mmu_add_mapping and core_mmu_add_mapping functions

arnopo · 2023-12-12T13:31:20Z

core/drivers/remoteproc/stm32_remoteproc.c

 	struct stm32_rproc_mem *mems = NULL;
-	vaddr_t *va = NULL;
+	paddr_t pa  = virt_to_phys(va);


arnopo · 2023-12-12T13:35:18Z

core/drivers/remoteproc/stm32_remoteproc.c

 	unsigned int i = 0;

-	if (!rproc)
+	if (!rproc || !va)


Right, pa seems more interesting to test as set to 0 if virt_to_phys fails.

etienne-lms

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"drivers: Add stm32mp1 remoteproc driver" and
"pta: stm32mp: add new remoteproc pta" with a comment in each addressed.

etienne-lms · 2023-12-13T07:44:59Z

core/drivers/remoteproc/stm32_remoteproc.c

+	for (i = 0; i < nranges; i++) {
+		uint32_t da = fdt32_to_cpu(list[i++]);
+		uint32_t pa = fdt32_to_cpu(list[i++]);
+		uint32_t size = fdt32_to_cpu(list[i]);


nitpicking: here I would prefer:

for (i = 0; i < nranges; i += 3) { uint32_t da = fdt32_to_cpu(list[i]); uint32_t pa = fdt32_to_cpu(list[i + 1]); uint32_t size = fdt32_to_cpu(list[i + 2]); (...)

etienne-lms · 2023-12-13T07:47:52Z

core/pta/stm32mp/remoteproc_pta.c

+ */
+static TEE_Result
+	rproc_pta_open_session(uint32_t pt, TEE_Param params[TEE_NUM_PARAMS],
+			       void **sess_ctx __unused)


prefer:

static TEE_Result rproc_pta_open_session(uint32_t pt, TEE_Param params[TEE_NUM_PARAMS], void **sess_ctx __unused)

arnopo · 2023-12-13T09:24:22Z

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits "drivers: Add stm32mp1 remoteproc driver" and "pta: stm32mp: add new remoteproc pta" with a comment in each addressed.

Done, thanks @etienne-lms for the reviews!

etienne-lms

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>for commits
"plat-stm32mp1: Add the remoteproc TA in early-TA list" and
"dts: stm32: update m4_rproc to support the remoteproc OPTEE framework".

jforissier

Minor comments on "drivers: Add stm32mp1 remoteproc driver".

s/coherence/consistency/ in the commit description. Other comments below.

jforissier · 2023-12-13T09:38:14Z

mk/config.mk

+CFG_DRIVERS_REMOTEPROC ?= n
+
+# CFG_RPROC_PTA, when enabled, embeds remote processor management PTA service.
+CFG_RPROC_PTA ?= n


Would you mind s/CFG_RPROC_PTA/CFG_REMOTEPROC_PTA/ for consistency with CFG_DRIVERS_REMOTEPROC (and better "grepability", too)?

yes much better I also move the declaration in the patch that introduce the remoteproc PTA

jforissier · 2023-12-13T09:42:24Z

core/drivers/remoteproc/stm32_remoteproc.c

+	return res;
+}
+
+static TEE_Result __stm32_rproc_stop(struct stm32_rproc_instance *rproc)


Leading underscores/double underscores are not really recommended, why not simply rproc_stop()? It is a static function the name can remain very short and generic with no risk of collision.

jforissier

For commit " pta: stm32mp: add new remoteproc pta"

s/pta/PTA/ in the suject (the pta: prefix is OK in lowercase).

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2023-12-13T09:49:32Z

core/arch/arm/plat-stm32mp1/conf.mk

@@ -151,6 +151,7 @@ CFG_CORE_ASLR ?= n

 CFG_STM32MP_REMOTEPROC ?= n
 CFG_DRIVERS_REMOTEPROC ?= $(CFG_STM32MP_REMOTEPROC)
+CFG_RPROC_PTA ?= $(CFG_STM32MP_REMOTEPROC)


As already mentioned I would prefer CFG_REMOTEPROC_PTA

jforissier

For "plat-stm32mp1: Add the remoteproc TA in early-TA list":

s/early-TA/early TA/

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

For "dts: stm32: update m4_rproc to support the remoteproc OPTEE framework":

s/OPTEE/OP-TEE/

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

arnopo

@jforissier : I have take, into account your comments. In addition I have re-dispatched the CFG-REMOTEPROC-PTA between commits. As consequence, I don't add your tag and @etienne-lms one in commits.
Please, could you confirm that the fixups are ok for you then i will squash them add the tags in commits messages.

arnopo · 2023-12-13T10:20:09Z

core/drivers/remoteproc/stm32_remoteproc.c

+	return res;
+}
+
+static TEE_Result __stm32_rproc_stop(struct stm32_rproc_instance *rproc)


arnopo · 2023-12-13T10:28:29Z

core/arch/arm/plat-stm32mp1/conf.mk

@@ -151,6 +151,7 @@ CFG_CORE_ASLR ?= n

 CFG_STM32MP_REMOTEPROC ?= n
 CFG_DRIVERS_REMOTEPROC ?= $(CFG_STM32MP_REMOTEPROC)
+CFG_RPROC_PTA ?= $(CFG_STM32MP_REMOTEPROC)


arnopo · 2023-12-13T10:29:20Z

mk/config.mk

+CFG_DRIVERS_REMOTEPROC ?= n
+
+# CFG_RPROC_PTA, when enabled, embeds remote processor management PTA service.
+CFG_RPROC_PTA ?= n


yes much better I also move the declaration in the patch that introduce the remoteproc PTA

jforissier · 2023-12-15T08:05:41Z

@arnopo yes the fixups look good, thanks. When squashing you may also add:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

... to "drivers: Add stm32mp1 remoteproc driver".

The remoteproc PTA is charge of providing interface to authenticate firmware images and managing the remote processor live cycle. The remoteproc PTA supports platform specificity in the management of a remote processor: - firmware authentication based on a platform key, - load of the segments in remote processor memories, - start/stop of the remote processor, - remote processor addresses conversion. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

This driver is responsible for configuring the registers and memories of the remote processor. - It stores information about memories assigned to the remote processor based on the device tree. - It ensures consistency between the registered memory and the addresses of the firmware segments to be loaded. - Additionally, it is responsible for starting and stopping the remote processor core. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Add remoteproc PTA for the stm32mp1 platform. The PTA relies on the stm32_remoteproc driver for the remoteproc management. It is charge of providing interface for authenticating firmware images and managing the remote processor live cycle. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

arnopo · 2023-12-15T10:37:34Z

I rebased on top of master branch to fix merge conflic introduced by 1d12969 ("core: add CFG_AUTO_MAX_PA_BITS", 2023-12-13)

On the stm32mp1 platform, it is possible to load firmware during the bootloader stage, for instance, by U-boot. To enable this feature, The remoteproc TA should be added to the list of early-TAs. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Update device tree to support the load of the remoteproc firmware by OP-TEE. - declare m_ipc_shm memory region that can contain the remote processor resource table and trace buffer, - update reset to align declaration with the Linux devicetree To enable the load of the coprocessor firmware by OP-TEE, user have to update the m4_rproc node compatible property: -"st,stm32mp1-m4": the load is managed by Linux or U-boot, -"st,stm32mp1-m4-tee": the load is managed by OP-TEE. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

arnopo · 2023-12-15T10:44:09Z

+1 update to fix a typo in commit adding the acked-by

arnopo force-pushed the remoteproc_platform branch 2 times, most recently from 2f08ff7 to 6735610 Compare November 17, 2023 13:42

arnopo mentioned this pull request Nov 17, 2023

[RFC PATCH] introduction of a remoteproc framework to load signed coprocessor firmware images #6027

Closed

etienne-lms reviewed Nov 29, 2023

View reviewed changes

arnopo force-pushed the remoteproc_platform branch from 952fad3 to 584efe1 Compare December 6, 2023 14:57

arnopo commented Dec 6, 2023

View reviewed changes

etienne-lms reviewed Dec 8, 2023

View reviewed changes

arnopo mentioned this pull request Dec 11, 2023

Introduce remote proc trusted application #6300

Merged

arnopo force-pushed the remoteproc_platform branch 2 times, most recently from 8aab84f to 3dce5d2 Compare December 12, 2023 18:25

arnopo commented Dec 12, 2023

View reviewed changes

etienne-lms reviewed Dec 13, 2023

View reviewed changes

arnopo force-pushed the remoteproc_platform branch from 3dce5d2 to 74ff81e Compare December 13, 2023 09:23

etienne-lms reviewed Dec 13, 2023

View reviewed changes

jforissier reviewed Dec 13, 2023

View reviewed changes

arnopo force-pushed the remoteproc_platform branch from 74ff81e to bbf28bc Compare December 13, 2023 10:49

arnopo commented Dec 13, 2023

View reviewed changes

arnopo force-pushed the remoteproc_platform branch from bbf28bc to 86ac2f0 Compare December 13, 2023 13:05

arnopo force-pushed the remoteproc_platform branch from 86ac2f0 to f195d6d Compare December 15, 2023 10:09

arnopo added 3 commits December 15, 2023 11:12

arnopo force-pushed the remoteproc_platform branch from f195d6d to 1132ebf Compare December 15, 2023 10:34

arnopo added 2 commits December 15, 2023 11:40

arnopo force-pushed the remoteproc_platform branch from 1132ebf to a1ff7cd Compare December 15, 2023 10:41

jforissier merged commit 7c9920c into OP-TEE:master Dec 15, 2023
8 checks passed

Core: Introduce stm32mp remoteproc PTA and driver #6470

Core: Introduce stm32mp remoteproc PTA and driver #6470

Conversation

arnopo commented Nov 17, 2023

etienne-lms left a comment • edited Loading

Choose a reason for hiding this comment

arnopo commented Dec 6, 2023

arnopo left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnopo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnopo commented Dec 13, 2023

etienne-lms left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Dec 15, 2023

arnopo commented Dec 15, 2023

arnopo commented Dec 15, 2023

etienne-lms left a comment •

edited

Loading

arnopo left a comment •

edited

Loading