On 4/1/24 17:46, Mathieu Poirier wrote: > On Fri, Mar 29, 2024 at 11:57:43AM +0100, Arnaud POULIQUEN wrote: >> >> >> On 3/27/24 18:14, Mathieu Poirier wrote: >>> On Tue, Mar 26, 2024 at 08:31:33PM +0100, Arnaud POULIQUEN wrote: >>>> >>>> >>>> On 3/25/24 17:51, Mathieu Poirier wrote: >>>>> On Fri, Mar 08, 2024 at 03:47:08PM +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. >>>>>> >>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx> >>>>>> --- >>>>>> Updates from V3: >>>>>> - remove support of the attach use case. Will be addressed in a separate >>>>>> thread, >>>>>> - add st_rproc_tee_ops::parse_fw ops, >>>>>> - inverse call of devm_rproc_alloc()and tee_rproc_register() to manage cross >>>>>> reference between the rproc struct and the tee_rproc struct in tee_rproc.c. >>>>>> --- >>>>>> drivers/remoteproc/stm32_rproc.c | 60 +++++++++++++++++++++++++++++--- >>>>>> 1 file changed, 56 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c >>>>>> index 8cd838df4e92..13df33c78aa2 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 */ >>>>> >>>>> Why is this the case? At least from the kernel side it is possible to call >>>>> tee_rproc_register() with any kind of value, why is there a need to be any >>>>> kind of alignment with the TEE? >>>> >>>> >>>> The use of the proc_id is to identify a processor in case of multi co-processors. >>>> >>> >>> That is well understood. >>> >>>> For instance we can have a system with A DSP and a modem. We would use the same >>>> TEE service, but >>> >>> That too. >>> >>>> the TEE driver will probably be different, same for the signature key. >>> >>> What TEE driver are we talking about here? >> >> In OP-TEE remoteproc frameork is divided in 2 or 3 layers: >> >> - the remoteproc Trusted Application (TA) [1] which is platform agnostic >> - The remoteproc Pseudo Trusted Application (PTA) [2] which is platform >> dependent and can rely on the proc ID to retrieve the context. >> - the remoteproc driver (optional for some platforms) [3], which is in charge >> of DT parsing and platform configuration. >> > > That part makes sense. > >> Here TEE driver can be interpreted by remote PTA and/or platform driver. >> > > I have to guess PTA means "Platform Trusted Application" but I have no > guarantee, adding to the level of (already high) confusion brought on by this > patchset. As mentioned above, PTA is Pseudo Trusted Application. It is an interface exposed by OP-TEE core to the OP-TEE application. In this case PTA is used to implement the platform part. If you need more details about the remoteproc framework in OP-TEE, there is a link in the to a presentation in the cover letter. > >> [1] >> https://elixir.bootlin.com/op-tee/latest/source/ta/remoteproc/src/remoteproc_core.c >> [2] >> https://elixir.bootlin.com/op-tee/latest/source/core/pta/stm32mp/remoteproc_pta.c >> [3] >> https://elixir.bootlin.com/op-tee/latest/source/core/drivers/remoteproc/stm32_remoteproc.c >> >>> >>>> In such case the proc ID allows to identify the the processor you want to address. >>>> >>> >>> That too is well understood, but there is no alignment needed with the TEE, i.e >>> the TEE application is not expecting a value of '0'. We could set >>> STM32_MP1_M4_PROC_ID to 0xDEADBEEF and things would work. This driver won't go >>> anywhere for as long as it is not the case. >> >> >> Here I suppose that you do not challenge the rproc_ID use in general, but for >> the stm32mp1 platform as we have only one remote processor. I'm right? > > That is correct - I understand the need for an rproc_ID. The problem is with > the comment that states that '0' is aligned with the TEE definitions, which in > my head means hard coded value and a big red flag. What it should say is > "aligned with the TEE device tree definition". > >> >> In OP-TEE the check is done here: >> https://elixir.bootlin.com/op-tee/latest/source/core/drivers/remoteproc/stm32_remoteproc.c#L64 >> >> If driver does not register the proc ID an error is returned indicating that the >> feature is not supported. >> >> In case of stm32mp1 yes we could consider it as useless as we have only one >> remote proc. >> >> Nevertheless I can not guaranty that a customer will not add an external >> companion processor that uses OP-TEE to authenticate the associated firmware. As >> the trusted Application is the unique entry point. he will need the proc_id to >> identify the target at PTA level. >> >> So from my point of view having a proc ID on stm32MP1 (and on stm32mp2 that will >> reuse same driver) aligned between Linux and OP-TEE is useful. > > I agree, for as long as it is not hard coded. The way remote processors are > discovered in the DT is perfectly acceptable, i.e the first remote processor is > for application X, the second for application Y... > >> >> That said using a TEE_REMOTEPROC_DEFAULT_ID is something that could be >> more generic (for linux and OP-TEE). This ID could be reuse in the stm32mp >> driver and platform drivers with an unique internal remote processor. >> > > I can't find the definition of TEE_REMOTEPROC_DEFAULT_ID anywhere, something > that doesn't help the confusion I referred to above. The TEE_REMOTEPROC_DEFAULT_ID does not yet exist; it is a proposal. Nevertheless I also had in mind the addition of a "proc ID" property in the DT. I will proceed in this way I think I now have all the information needed to prepare a new revision. Thanks for the time passed on this series and your advises Regards, Arnaud > >> It that solution would be ok for you? >> >> Regards, >> Arnaud >> >> >>> >>>> >>>>> >>>>>> +#define STM32_MP1_M4_PROC_ID 0 >>>>>> + >>>>>> struct stm32_syscon { >>>>>> struct regmap *map; >>>>>> u32 reg; >>>>>> @@ -257,6 +261,19 @@ static int stm32_rproc_release(struct rproc *rproc) >>>>>> 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; >>>>>> @@ -693,8 +710,19 @@ 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, >>>>>> + .kick = stm32_rproc_kick, >>>>>> + .load = tee_rproc_load_fw, >>>>>> + .parse_fw = tee_rproc_parse_fw, >>>>>> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table, >>>>>> +}; >>>>>> + >>>>>> 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 +881,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,9 +890,26 @@ static int stm32_rproc_probe(struct platform_device *pdev) >>>>>> if (ret) >>>>>> return ret; >>>>>> >>>>>> - rproc = devm_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. >>>>>> + */ >>>>>> + rproc = devm_rproc_alloc(dev, np->name, &st_rproc_tee_ops, NULL, sizeof(*ddata)); >>>>>> + if (!rproc) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + trproc = tee_rproc_register(dev, rproc, 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); >>>>>> + } >>>>>> + } else { >>>>>> + rproc = devm_rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata)); >>>>>> + if (!rproc) >>>>>> + return -ENOMEM; >>>>>> + } >>>>>> >>>>>> ddata = rproc->priv; >>>>>> >>>>>> @@ -915,6 +961,9 @@ static int stm32_rproc_probe(struct platform_device *pdev) >>>>>> dev_pm_clear_wake_irq(dev); >>>>>> device_init_wakeup(dev, false); >>>>>> } >>>>>> + if (trproc) >>>>> >>>>> if (rproc->tee_interface) >>>>> >>>>> >>>>> I am done reviewing this set. >>>> >>>> Thank for your review! >>>> Arnaud >>>> >>>>> >>>>> Thanks, >>>>> Mathieu >>>>> >>>>>> + tee_rproc_unregister(trproc); >>>>>> + >>>>>> return ret; >>>>>> } >>>>>> >>>>>> @@ -935,6 +984,9 @@ static void stm32_rproc_remove(struct platform_device *pdev) >>>>>> dev_pm_clear_wake_irq(dev); >>>>>> device_init_wakeup(dev, false); >>>>>> } >>>>>> + if (rproc->tee_interface) >>>>>> + tee_rproc_unregister(rproc->tee_interface); >>>>>> + >>>>>> } >>>>>> >>>>>> static int stm32_rproc_suspend(struct device *dev) >>>>>> -- >>>>>> 2.25.1 >>>>>>
