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.pouliq...@foss.st.com>
>>>>>> ---
>>>>>> 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
>>>>>>
