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
>>>>>>

Reply via email to