[PATCH v4] tee: optee: rework TA bus scanning code

[PATCH v4] tee: optee: rework TA bus scanning code

[Patrick DELAUNAY]

Hi Simon,

On 9/12/22 20:31, Simon Glass wrote:
> Hi Ilias,
>
> On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>> Hi Simon,
>>
>> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Ilias,
>>>
>>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
>>> <ilias.apalodimas@linaro.org> wrote:
>>>> Hi Simon,
>>>>
>>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
>>>>> <ilias.apalodimas@linaro.org> wrote:
>>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
>>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus 
>>>>>> whichwe can
>>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth 
>>>>>> noting
>>>>>> that we already have a workaround for RNG. The details are in
>>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
>>>>>>
>>>>>> So let's add a list of devices based on U-Boot Kconfig options 
>>>>>> that we will
>>>>>> scan until we properly implement the tee-bus functionality.
>>>>>>
>>>>>> While at it change the behaviour of the tee core itself wrt to device
>>>>>> binding. If some device binding fails, print a warning instead of
>>>>>> disabling OP-TEE.
>>>>>>
>>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
>>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
>>>>>> ---
>>>>>> Changes since v3:
>>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since 
>>>>>> it's not
>>>>>> really needed
>>>>>> - Changed the style of the optee_bus_probe[] definition to
>>>>>> {.drv_name = xxx, .dev_name = yyy }
>>>>>>
>>>>>> Changes since v2:
>>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
>>>>>>
>>>>>> Changes since v1:
>>>>>> - remove a macro and use ARRAY_SIZE directly
>>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
>>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
>>>>>> index a89d62aaf0b3..c201a4635e6b 100644
>>>>>> --- a/drivers/tee/optee/core.c
>>>>>> +++ b/drivers/tee/optee/core.c
>>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
>>>>>> optee_invoke_fn *invoke_fn;
>>>>>> };
>>>>>>
>>>>>> +static const struct {
>>>>>> + const char *drv_name;
>>>>>> + const char *dev_name;
>>>>>> +} optee_bus_probe[] = {
>>>>>> +#ifdef CONFIG_RNG_OPTEE
>>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
>>>>>> +#endif
>>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
>>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
>>>>>> +#endif
>>>>>> +};
>>>>>> +
>>>>>> struct rpc_param {
>>>>>> u32 a0;
>>>>>> u32 a1;
>>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
>>>>>> {
>>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
>>>>>> u32 sec_caps;
>>>>>> - struct udevice *child;
>>>>>> - int ret;
>>>>>> + int ret, i;
>>>>>>
>>>>>> if (!is_optee_api(pdata->invoke_fn)) {
>>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
>>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
>>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
>>>>>> * only bind the drivers associated to the supported OP-TEETA
>>>>>> */
>>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
>>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
>>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
>>>>>> + optee_bus_probe[i].dev_name, NULL);
>>>>>> if (ret)
>>>>>> - return ret;
>>>>>> + dev_warn(dev, "Failed to bind device %s\n",
>>>>>> + optee_bus_probe[i].dev_name);
>>>>> Please add device tree nodes for these and all this code can go away.
>>>> That's the exact opposite of what the commit message describes. OP-TEE
>>>> supports a scannable bus ifor TAs that behave like hardware blocks and
>>>> doesn't need a DT entry. Since it's really the TAs compilation decision
>>>> to support that or not having them as a DT node is not always the right
>>>> choice.
>>> This is continuing the perversion of how things are supposed to work
>>> in driver model.
>> Which is not the only thing we need to keep in mind though.
>>
>>> We need to talk about this because it is simply the wrong way to be
>>> approaching this.
>> This is already part of other software components though, e.g it's
>> already in the kernel. So I don't think it's the wrong approach.
>>
>>> There is nothing wrong with putting things in the DT
>>> and this is how U-Boot works. For now, please create a binding and get
>>> it reviewed. You don't need all the internal objects but you do need
>>> an OP-TEE driver and node, as we have with PCI.
>> Some things *are* working without a DT entry. You had similar
>> concerns on FF-A (where you requested a DT node again) and people gave
>> the exact same response. As long as a bus is scanable in any way,
>> it's preferable to than adding a DT entry. Moreover this code does
>> not prevent anyone from adding a DT entry.
>>
>> To make things even worse if the TA is compiled as 'scanable' and has
>> a DT entry, it might cause issues down the road when being probed by
>> the kernel. So really this is just a patch that makes u-boot behave
>> and plug in properly to the rest of the ecosystem
> Calling device_bind() is supposed to be used in extremis. I don't see
> any scanning of an OP-TEE bus here. I just see it binding two child
> devices which are hard-coded in U-Boot. What am I missing?


The tee bus is supported in Linux kernel (each TA have a UUID and

is discoverable by the TEE driver).


see drivers/tee/optee/core.c::optee_bus_scan()

and "struct tee_client_driver" with TA UUID


It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation

=> TA support was hardcoded, under the associated CONFIG

       and the probe failed when the TA is not present.


       for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c


The TEE bus feature is added by the Etienne in the serie [1].


This bus is more flexible and avoid OP-TEE to dynamically modify the 
device tree

to add/remove the supported SW component (TA support are activated 
during OP-TEE

compilation) as the binding is managed dynamically in OP-TEE as it is 
done in Linux.


For information, on STM32MP15 platform, I have the trace "can't open 
session:" for

RNG TA for each 'rng' command when this TA is not supported in OP-TEE but

OP-TEE RNG driver is activated in U-Boot, because the driver is binding.


[1] drivers: tee: optee: remove unused probe local variable

http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*


> This appears to be a Linaro binding, so you should be able to update
> it easily enough.
>
> Regards,
> Simon

Regards
Patrick

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Etienne Carriere]

Hello Patrick and all,

On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
<patrick.delaunay@foss.st.com> wrote:
>
>
> Hi Simon,
>
> On 9/12/22 20:31, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >> Hi Simon,
> >>
> >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> >>> Hi Ilias,
> >>>
> >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> >>> <ilias.apalodimas@linaro.org> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> >>>>> <ilias.apalodimas@linaro.org> wrote:
> >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> >>>>>> whichwe can
> >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> >>>>>> noting
> >>>>>> that we already have a workaround for RNG. The details are in
> >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> >>>>>>
> >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> >>>>>> that we will
> >>>>>> scan until we properly implement the tee-bus functionality.
> >>>>>>
> >>>>>> While at it change the behaviour of the tee core itself wrt to device
> >>>>>> binding. If some device binding fails, print a warning instead of
> >>>>>> disabling OP-TEE.
> >>>>>>
> >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> >>>>>> ---
> >>>>>> Changes since v3:
> >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> >>>>>> it's not
> >>>>>> really needed
> >>>>>> - Changed the style of the optee_bus_probe[] definition to
> >>>>>> {.drv_name = xxx, .dev_name = yyy }
> >>>>>>
> >>>>>> Changes since v2:
> >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> >>>>>>
> >>>>>> Changes since v1:
> >>>>>> - remove a macro and use ARRAY_SIZE directly
> >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> >>>>>> --- a/drivers/tee/optee/core.c
> >>>>>> +++ b/drivers/tee/optee/core.c
> >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> >>>>>> optee_invoke_fn *invoke_fn;
> >>>>>> };
> >>>>>>
> >>>>>> +static const struct {
> >>>>>> + const char *drv_name;
> >>>>>> + const char *dev_name;
> >>>>>> +} optee_bus_probe[] = {
> >>>>>> +#ifdef CONFIG_RNG_OPTEE
> >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> >>>>>> +#endif
> >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> >>>>>> +#endif
> >>>>>> +};
> >>>>>> +
> >>>>>> struct rpc_param {
> >>>>>> u32 a0;
> >>>>>> u32 a1;
> >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> >>>>>> {
> >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> >>>>>> u32 sec_caps;
> >>>>>> - struct udevice *child;
> >>>>>> - int ret;
> >>>>>> + int ret, i;
> >>>>>>
> >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> >>>>>> * only bind the drivers associated to the supported OP-TEETA
> >>>>>> */
> >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> >>>>>> +
> >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> >>>>>> + optee_bus_probe[i].dev_name, NULL);
> >>>>>> if (ret)
> >>>>>> - return ret;
> >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> >>>>>> + optee_bus_probe[i].dev_name);
> >>>>> Please add device tree nodes for these and all this code can go away.
> >>>> That's the exact opposite of what the commit message describes. OP-TEE
> >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> >>>> to support that or not having them as a DT node is not always the right
> >>>> choice.
> >>> This is continuing the perversion of how things are supposed to work
> >>> in driver model.
> >> Which is not the only thing we need to keep in mind though.
> >>
> >>> We need to talk about this because it is simply the wrong way to be
> >>> approaching this.
> >> This is already part of other software components though, e.g it's
> >> already in the kernel. So I don't think it's the wrong approach.
> >>
> >>> There is nothing wrong with putting things in the DT
> >>> and this is how U-Boot works. For now, please create a binding and get
> >>> it reviewed. You don't need all the internal objects but you do need
> >>> an OP-TEE driver and node, as we have with PCI.
> >> Some things *are* working without a DT entry. You had similar
> >> concerns on FF-A (where you requested a DT node again) and people gave
> >> the exact same response. As long as a bus is scanable in any way,
> >> it's preferable to than adding a DT entry. Moreover this code does
> >> not prevent anyone from adding a DT entry.
> >>
> >> To make things even worse if the TA is compiled as 'scanable' and has
> >> a DT entry, it might cause issues down the road when being probed by
> >> the kernel. So really this is just a patch that makes u-boot behave
> >> and plug in properly to the rest of the ecosystem
> > Calling device_bind() is supposed to be used in extremis. I don't see
> > any scanning of an OP-TEE bus here. I just see it binding two child
> > devices which are hard-coded in U-Boot. What am I missing?
>
>
> The tee bus is supported in Linux kernel (each TA have a UUID and
> is discoverable by the TEE driver).
>
> see drivers/tee/optee/core.c::optee_bus_scan()
> and "struct tee_client_driver" with TA UUID
> It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
>
> => TA support was hardcoded, under the associated CONFIG
>        and the probe failed when the TA is not present.
>        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
>
> The TEE bus feature is added by the Etienne in the serie [1].
> This bus is more flexible and avoid OP-TEE to dynamically modify the
> device tree
> to add/remove the supported SW component (TA support are activated
> during OP-TEE
> compilation) as the binding is managed dynamically in OP-TEE as it is
> done in Linux.
>
> For information, on STM32MP15 platform, I have the trace "can't open
> session:" for
> RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
>
> [1] drivers: tee: optee: remove unused probe local variable
>
> http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
>
> > This appears to be a Linaro binding, so you should be able to update
> > it easily enough.
> >

Discussing with Patrick, he made a suggestion and showed me I was
wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
OP-TEE exposes 2 levels of service discovery, so-called devices
enumeration and device-with-supplicant enumeration. The later are
OP-TEE services that depend on RPC service exposed to OP-TEE by in the
caller OS (U-Boot or Linux kernel). The former are services without
such dependencies. When i posted OP-TEE services discovery in U-Boot,
I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
dependencies).
I made it intentionally as U-Boot tee-supplicant does not implement
all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
on tee-supplicant support, it is not enumerated/discovered.

The point is the U-Boot tee-suppl. does implement the few RPC services
FTPM TA needs (that are memory allocation and RPMB access).
So Patrick argued that U-Boot can as well enumerate OP-TEE service
*with* tee-suppl. devices. The optee ftpm driver can register to this
service discovery and will operate properly.
What puzzled me was that discovery of OP-TEE services that require
tee-suppl. services not available in U-Boot would end in a failure of
the service, but as Patrick rightly said that it makes no sense for
one of add implement u-boot a driver for an OP-TEE service if that
service lacks some U-Boot tee-suppl. supports.

All in one, my apologies Ilias for this mistake. A change in
tee/optee/core.c to also bind services enumerated by OP-TEE command
PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
functional FTPM TA service. I'll post a change for that.

Regards,
Etienne

> > Regards,
> > Simon
>
> Regards
> Patrick

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Sumit Garg]

On Thu, 22 Sept 2022 at 14:22, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Patrick and all,
>
> On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
> <patrick.delaunay@foss.st.com> wrote:
> >
> >
> > Hi Simon,
> >
> > On 9/12/22 20:31, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > >> Hi Simon,
> > >>
> > >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> > >>> Hi Ilias,
> > >>>
> > >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > >>> <ilias.apalodimas@linaro.org> wrote:
> > >>>> Hi Simon,
> > >>>>
> > >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > >>>>> <ilias.apalodimas@linaro.org> wrote:
> > >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> > >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> > >>>>>> whichwe can
> > >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> > >>>>>> noting
> > >>>>>> that we already have a workaround for RNG. The details are in
> > >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > >>>>>>
> > >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> > >>>>>> that we will
> > >>>>>> scan until we properly implement the tee-bus functionality.
> > >>>>>>
> > >>>>>> While at it change the behaviour of the tee core itself wrt to device
> > >>>>>> binding. If some device binding fails, print a warning instead of
> > >>>>>> disabling OP-TEE.
> > >>>>>>
> > >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > >>>>>> ---
> > >>>>>> Changes since v3:
> > >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> > >>>>>> it's not
> > >>>>>> really needed
> > >>>>>> - Changed the style of the optee_bus_probe[] definition to
> > >>>>>> {.drv_name = xxx, .dev_name = yyy }
> > >>>>>>
> > >>>>>> Changes since v2:
> > >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > >>>>>>
> > >>>>>> Changes since v1:
> > >>>>>> - remove a macro and use ARRAY_SIZE directly
> > >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> > >>>>>> --- a/drivers/tee/optee/core.c
> > >>>>>> +++ b/drivers/tee/optee/core.c
> > >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> > >>>>>> optee_invoke_fn *invoke_fn;
> > >>>>>> };
> > >>>>>>
> > >>>>>> +static const struct {
> > >>>>>> + const char *drv_name;
> > >>>>>> + const char *dev_name;
> > >>>>>> +} optee_bus_probe[] = {
> > >>>>>> +#ifdef CONFIG_RNG_OPTEE
> > >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > >>>>>> +#endif
> > >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> > >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > >>>>>> +#endif
> > >>>>>> +};
> > >>>>>> +
> > >>>>>> struct rpc_param {
> > >>>>>> u32 a0;
> > >>>>>> u32 a1;
> > >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > >>>>>> {
> > >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> > >>>>>> u32 sec_caps;
> > >>>>>> - struct udevice *child;
> > >>>>>> - int ret;
> > >>>>>> + int ret, i;
> > >>>>>>
> > >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> > >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> > >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > >>>>>> * only bind the drivers associated to the supported OP-TEETA
> > >>>>>> */
> > >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > >>>>>> +
> > >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > >>>>>> + optee_bus_probe[i].dev_name, NULL);
> > >>>>>> if (ret)
> > >>>>>> - return ret;
> > >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> > >>>>>> + optee_bus_probe[i].dev_name);
> > >>>>> Please add device tree nodes for these and all this code can go away.
> > >>>> That's the exact opposite of what the commit message describes. OP-TEE
> > >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> > >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> > >>>> to support that or not having them as a DT node is not always the right
> > >>>> choice.
> > >>> This is continuing the perversion of how things are supposed to work
> > >>> in driver model.
> > >> Which is not the only thing we need to keep in mind though.
> > >>
> > >>> We need to talk about this because it is simply the wrong way to be
> > >>> approaching this.
> > >> This is already part of other software components though, e.g it's
> > >> already in the kernel. So I don't think it's the wrong approach.
> > >>
> > >>> There is nothing wrong with putting things in the DT
> > >>> and this is how U-Boot works. For now, please create a binding and get
> > >>> it reviewed. You don't need all the internal objects but you do need
> > >>> an OP-TEE driver and node, as we have with PCI.
> > >> Some things *are* working without a DT entry. You had similar
> > >> concerns on FF-A (where you requested a DT node again) and people gave
> > >> the exact same response. As long as a bus is scanable in any way,
> > >> it's preferable to than adding a DT entry. Moreover this code does
> > >> not prevent anyone from adding a DT entry.
> > >>
> > >> To make things even worse if the TA is compiled as 'scanable' and has
> > >> a DT entry, it might cause issues down the road when being probed by
> > >> the kernel. So really this is just a patch that makes u-boot behave
> > >> and plug in properly to the rest of the ecosystem
> > > Calling device_bind() is supposed to be used in extremis. I don't see
> > > any scanning of an OP-TEE bus here. I just see it binding two child
> > > devices which are hard-coded in U-Boot. What am I missing?
> >
> >
> > The tee bus is supported in Linux kernel (each TA have a UUID and
> > is discoverable by the TEE driver).
> >
> > see drivers/tee/optee/core.c::optee_bus_scan()
> > and "struct tee_client_driver" with TA UUID
> > It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
> >
> > => TA support was hardcoded, under the associated CONFIG
> >        and the probe failed when the TA is not present.
> >        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
> >
> > The TEE bus feature is added by the Etienne in the serie [1].
> > This bus is more flexible and avoid OP-TEE to dynamically modify the
> > device tree
> > to add/remove the supported SW component (TA support are activated
> > during OP-TEE
> > compilation) as the binding is managed dynamically in OP-TEE as it is
> > done in Linux.
> >
> > For information, on STM32MP15 platform, I have the trace "can't open
> > session:" for
> > RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> > OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
> >
> > [1] drivers: tee: optee: remove unused probe local variable
> >
> > http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
> >
> > > This appears to be a Linaro binding, so you should be able to update
> > > it easily enough.
> > >
>
> Discussing with Patrick, he made a suggestion and showed me I was
> wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
> OP-TEE exposes 2 levels of service discovery, so-called devices
> enumeration and device-with-supplicant enumeration. The later are
> OP-TEE services that depend on RPC service exposed to OP-TEE by in the
> caller OS (U-Boot or Linux kernel). The former are services without
> such dependencies. When i posted OP-TEE services discovery in U-Boot,
> I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
> dependencies).
> I made it intentionally as U-Boot tee-supplicant does not implement
> all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
> on tee-supplicant support, it is not enumerated/discovered.
>
> The point is the U-Boot tee-suppl. does implement the few RPC services
> FTPM TA needs (that are memory allocation and RPMB access).
> So Patrick argued that U-Boot can as well enumerate OP-TEE service
> *with* tee-suppl. devices. The optee ftpm driver can register to this
> service discovery and will operate properly.
> What puzzled me was that discovery of OP-TEE services that require
> tee-suppl. services not available in U-Boot would end in a failure of
> the service, but as Patrick rightly said that it makes no sense for
> one of add implement u-boot a driver for an OP-TEE service if that
> service lacks some U-Boot tee-suppl. supports.

+1

>
> All in one, my apologies Ilias for this mistake. A change in
> tee/optee/core.c to also bind services enumerated by OP-TEE command
> PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
> functional FTPM TA service. I'll post a change for that.
>

I think that should be gated under CONFIG_SUPPORT_EMMC_RPMB as if
that's unavailable FTPM TA service can't be functional.

-Sumit

> Regards,
> Etienne
>
> > > Regards,
> > > Simon
> >
> > Regards
> > Patrick

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Simon Glass]

Hi Etienne,

On Thu, 22 Sept 2022 at 10:52, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Patrick and all,
>
> On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
> <patrick.delaunay@foss.st.com> wrote:
> >
> >
> > Hi Simon,
> >
> > On 9/12/22 20:31, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > >> Hi Simon,
> > >>
> > >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> > >>> Hi Ilias,
> > >>>
> > >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > >>> <ilias.apalodimas@linaro.org> wrote:
> > >>>> Hi Simon,
> > >>>>
> > >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > >>>>> <ilias.apalodimas@linaro.org> wrote:
> > >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> > >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> > >>>>>> whichwe can
> > >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> > >>>>>> noting
> > >>>>>> that we already have a workaround for RNG. The details are in
> > >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > >>>>>>
> > >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> > >>>>>> that we will
> > >>>>>> scan until we properly implement the tee-bus functionality.
> > >>>>>>
> > >>>>>> While at it change the behaviour of the tee core itself wrt to device
> > >>>>>> binding. If some device binding fails, print a warning instead of
> > >>>>>> disabling OP-TEE.
> > >>>>>>
> > >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > >>>>>> ---
> > >>>>>> Changes since v3:
> > >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> > >>>>>> it's not
> > >>>>>> really needed
> > >>>>>> - Changed the style of the optee_bus_probe[] definition to
> > >>>>>> {.drv_name = xxx, .dev_name = yyy }
> > >>>>>>
> > >>>>>> Changes since v2:
> > >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > >>>>>>
> > >>>>>> Changes since v1:
> > >>>>>> - remove a macro and use ARRAY_SIZE directly
> > >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> > >>>>>> --- a/drivers/tee/optee/core.c
> > >>>>>> +++ b/drivers/tee/optee/core.c
> > >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> > >>>>>> optee_invoke_fn *invoke_fn;
> > >>>>>> };
> > >>>>>>
> > >>>>>> +static const struct {
> > >>>>>> + const char *drv_name;
> > >>>>>> + const char *dev_name;
> > >>>>>> +} optee_bus_probe[] = {
> > >>>>>> +#ifdef CONFIG_RNG_OPTEE
> > >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > >>>>>> +#endif
> > >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> > >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > >>>>>> +#endif
> > >>>>>> +};
> > >>>>>> +
> > >>>>>> struct rpc_param {
> > >>>>>> u32 a0;
> > >>>>>> u32 a1;
> > >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > >>>>>> {
> > >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> > >>>>>> u32 sec_caps;
> > >>>>>> - struct udevice *child;
> > >>>>>> - int ret;
> > >>>>>> + int ret, i;
> > >>>>>>
> > >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> > >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> > >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > >>>>>> * only bind the drivers associated to the supported OP-TEETA
> > >>>>>> */
> > >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > >>>>>> +
> > >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > >>>>>> + optee_bus_probe[i].dev_name, NULL);
> > >>>>>> if (ret)
> > >>>>>> - return ret;
> > >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> > >>>>>> + optee_bus_probe[i].dev_name);
> > >>>>> Please add device tree nodes for these and all this code can go away.
> > >>>> That's the exact opposite of what the commit message describes. OP-TEE
> > >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> > >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> > >>>> to support that or not having them as a DT node is not always the right
> > >>>> choice.
> > >>> This is continuing the perversion of how things are supposed to work
> > >>> in driver model.
> > >> Which is not the only thing we need to keep in mind though.
> > >>
> > >>> We need to talk about this because it is simply the wrong way to be
> > >>> approaching this.
> > >> This is already part of other software components though, e.g it's
> > >> already in the kernel. So I don't think it's the wrong approach.
> > >>
> > >>> There is nothing wrong with putting things in the DT
> > >>> and this is how U-Boot works. For now, please create a binding and get
> > >>> it reviewed. You don't need all the internal objects but you do need
> > >>> an OP-TEE driver and node, as we have with PCI.
> > >> Some things *are* working without a DT entry. You had similar
> > >> concerns on FF-A (where you requested a DT node again) and people gave
> > >> the exact same response. As long as a bus is scanable in any way,
> > >> it's preferable to than adding a DT entry. Moreover this code does
> > >> not prevent anyone from adding a DT entry.
> > >>
> > >> To make things even worse if the TA is compiled as 'scanable' and has
> > >> a DT entry, it might cause issues down the road when being probed by
> > >> the kernel. So really this is just a patch that makes u-boot behave
> > >> and plug in properly to the rest of the ecosystem
> > > Calling device_bind() is supposed to be used in extremis. I don't see
> > > any scanning of an OP-TEE bus here. I just see it binding two child
> > > devices which are hard-coded in U-Boot. What am I missing?
> >
> >
> > The tee bus is supported in Linux kernel (each TA have a UUID and
> > is discoverable by the TEE driver).
> >
> > see drivers/tee/optee/core.c::optee_bus_scan()
> > and "struct tee_client_driver" with TA UUID
> > It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
> >
> > => TA support was hardcoded, under the associated CONFIG
> >        and the probe failed when the TA is not present.
> >        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
> >
> > The TEE bus feature is added by the Etienne in the serie [1].
> > This bus is more flexible and avoid OP-TEE to dynamically modify the
> > device tree
> > to add/remove the supported SW component (TA support are activated
> > during OP-TEE
> > compilation) as the binding is managed dynamically in OP-TEE as it is
> > done in Linux.
> >
> > For information, on STM32MP15 platform, I have the trace "can't open
> > session:" for
> > RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> > OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
> >
> > [1] drivers: tee: optee: remove unused probe local variable
> >
> > http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
> >
> > > This appears to be a Linaro binding, so you should be able to update
> > > it easily enough.
> > >
>
> Discussing with Patrick, he made a suggestion and showed me I was
> wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
> OP-TEE exposes 2 levels of service discovery, so-called devices
> enumeration and device-with-supplicant enumeration. The later are
> OP-TEE services that depend on RPC service exposed to OP-TEE by in the
> caller OS (U-Boot or Linux kernel). The former are services without
> such dependencies. When i posted OP-TEE services discovery in U-Boot,
> I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
> dependencies).
> I made it intentionally as U-Boot tee-supplicant does not implement
> all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
> on tee-supplicant support, it is not enumerated/discovered.
>
> The point is the U-Boot tee-suppl. does implement the few RPC services
> FTPM TA needs (that are memory allocation and RPMB access).
> So Patrick argued that U-Boot can as well enumerate OP-TEE service
> *with* tee-suppl. devices. The optee ftpm driver can register to this
> service discovery and will operate properly.
> What puzzled me was that discovery of OP-TEE services that require
> tee-suppl. services not available in U-Boot would end in a failure of
> the service, but as Patrick rightly said that it makes no sense for
> one of add implement u-boot a driver for an OP-TEE service if that
> service lacks some U-Boot tee-suppl. supports.
>
> All in one, my apologies Ilias for this mistake. A change in
> tee/optee/core.c to also bind services enumerated by OP-TEE command
> PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
> functional FTPM TA service. I'll post a change for that.

Thank you for the update.

I talked with Ilias about this at osfc. The problem I think is that it
is not possible to reference a particular random-number generator
unless it is present in the device tree.

Here are some things that can produce random numbers, i.e. are in the
RNG uclass (which we should rename to RAND or RANDOM to fit with the
other naming there):
- software
- SoC
- TPM
- OP-TEE

For choosing which one to use, we can use aliases. so rand0 is the
first device. That alias can point into OP-TEE if desitred.

If we need a particular device to specify which random-number
generator device it should use, that can be done with a phandle, like:

board-sysinfo {
   compatible = "vendor,sysinfo-board";
   ramdom = <&optee_rng>;
}

So it should work like PCI, where we scan the bus and attach the
correct device tree subnode to the OP-TEE devices. The PCI code for
this is at
pci_bind_bus_devices() where it calls pci_bus_find_devfn().

This means we need to have compatible strings and bindings for any
devices on the bus which can be used as above. If the subnodes are
missing, then scanning will result in binding them in any case.

Regards,
Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Etienne Carriere]

On Thu, 22 Sept 2022 at 13:27, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Etienne,
>
> On Thu, 22 Sept 2022 at 10:52, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Patrick and all,
> >
> > On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
> > <patrick.delaunay@foss.st.com> wrote:
> > >
> > >
> > > Hi Simon,
> > >
> > > On 9/12/22 20:31, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > >> Hi Simon,
> > > >>
> > > >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> > > >>> Hi Ilias,
> > > >>>
> > > >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > > >>> <ilias.apalodimas@linaro.org> wrote:
> > > >>>> Hi Simon,
> > > >>>>
> > > >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > > >>>>> Hi,
> > > >>>>>
> > > >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > > >>>>> <ilias.apalodimas@linaro.org> wrote:
> > > >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> > > >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> > > >>>>>> whichwe can
> > > >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> > > >>>>>> noting
> > > >>>>>> that we already have a workaround for RNG. The details are in
> > > >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > >>>>>>
> > > >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> > > >>>>>> that we will
> > > >>>>>> scan until we properly implement the tee-bus functionality.
> > > >>>>>>
> > > >>>>>> While at it change the behaviour of the tee core itself wrt to device
> > > >>>>>> binding. If some device binding fails, print a warning instead of
> > > >>>>>> disabling OP-TEE.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > >>>>>> ---
> > > >>>>>> Changes since v3:
> > > >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> > > >>>>>> it's not
> > > >>>>>> really needed
> > > >>>>>> - Changed the style of the optee_bus_probe[] definition to
> > > >>>>>> {.drv_name = xxx, .dev_name = yyy }
> > > >>>>>>
> > > >>>>>> Changes since v2:
> > > >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > >>>>>>
> > > >>>>>> Changes since v1:
> > > >>>>>> - remove a macro and use ARRAY_SIZE directly
> > > >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> > > >>>>>> --- a/drivers/tee/optee/core.c
> > > >>>>>> +++ b/drivers/tee/optee/core.c
> > > >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> > > >>>>>> optee_invoke_fn *invoke_fn;
> > > >>>>>> };
> > > >>>>>>
> > > >>>>>> +static const struct {
> > > >>>>>> + const char *drv_name;
> > > >>>>>> + const char *dev_name;
> > > >>>>>> +} optee_bus_probe[] = {
> > > >>>>>> +#ifdef CONFIG_RNG_OPTEE
> > > >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > >>>>>> +#endif
> > > >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> > > >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > >>>>>> +#endif
> > > >>>>>> +};
> > > >>>>>> +
> > > >>>>>> struct rpc_param {
> > > >>>>>> u32 a0;
> > > >>>>>> u32 a1;
> > > >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > > >>>>>> {
> > > >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> > > >>>>>> u32 sec_caps;
> > > >>>>>> - struct udevice *child;
> > > >>>>>> - int ret;
> > > >>>>>> + int ret, i;
> > > >>>>>>
> > > >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> > > >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> > > >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > > >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > > >>>>>> * only bind the drivers associated to the supported OP-TEETA
> > > >>>>>> */
> > > >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > >>>>>> +
> > > >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > >>>>>> + optee_bus_probe[i].dev_name, NULL);
> > > >>>>>> if (ret)
> > > >>>>>> - return ret;
> > > >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> > > >>>>>> + optee_bus_probe[i].dev_name);
> > > >>>>> Please add device tree nodes for these and all this code can go away.
> > > >>>> That's the exact opposite of what the commit message describes. OP-TEE
> > > >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> > > >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> > > >>>> to support that or not having them as a DT node is not always the right
> > > >>>> choice.
> > > >>> This is continuing the perversion of how things are supposed to work
> > > >>> in driver model.
> > > >> Which is not the only thing we need to keep in mind though.
> > > >>
> > > >>> We need to talk about this because it is simply the wrong way to be
> > > >>> approaching this.
> > > >> This is already part of other software components though, e.g it's
> > > >> already in the kernel. So I don't think it's the wrong approach.
> > > >>
> > > >>> There is nothing wrong with putting things in the DT
> > > >>> and this is how U-Boot works. For now, please create a binding and get
> > > >>> it reviewed. You don't need all the internal objects but you do need
> > > >>> an OP-TEE driver and node, as we have with PCI.
> > > >> Some things *are* working without a DT entry. You had similar
> > > >> concerns on FF-A (where you requested a DT node again) and people gave
> > > >> the exact same response. As long as a bus is scanable in any way,
> > > >> it's preferable to than adding a DT entry. Moreover this code does
> > > >> not prevent anyone from adding a DT entry.
> > > >>
> > > >> To make things even worse if the TA is compiled as 'scanable' and has
> > > >> a DT entry, it might cause issues down the road when being probed by
> > > >> the kernel. So really this is just a patch that makes u-boot behave
> > > >> and plug in properly to the rest of the ecosystem
> > > > Calling device_bind() is supposed to be used in extremis. I don't see
> > > > any scanning of an OP-TEE bus here. I just see it binding two child
> > > > devices which are hard-coded in U-Boot. What am I missing?
> > >
> > >
> > > The tee bus is supported in Linux kernel (each TA have a UUID and
> > > is discoverable by the TEE driver).
> > >
> > > see drivers/tee/optee/core.c::optee_bus_scan()
> > > and "struct tee_client_driver" with TA UUID
> > > It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
> > >
> > > => TA support was hardcoded, under the associated CONFIG
> > >        and the probe failed when the TA is not present.
> > >        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
> > >
> > > The TEE bus feature is added by the Etienne in the serie [1].
> > > This bus is more flexible and avoid OP-TEE to dynamically modify the
> > > device tree
> > > to add/remove the supported SW component (TA support are activated
> > > during OP-TEE
> > > compilation) as the binding is managed dynamically in OP-TEE as it is
> > > done in Linux.
> > >
> > > For information, on STM32MP15 platform, I have the trace "can't open
> > > session:" for
> > > RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> > > OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
> > >
> > > [1] drivers: tee: optee: remove unused probe local variable
> > >
> > > http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
> > >
> > > > This appears to be a Linaro binding, so you should be able to update
> > > > it easily enough.
> > > >
> >
> > Discussing with Patrick, he made a suggestion and showed me I was
> > wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
> > OP-TEE exposes 2 levels of service discovery, so-called devices
> > enumeration and device-with-supplicant enumeration. The later are
> > OP-TEE services that depend on RPC service exposed to OP-TEE by in the
> > caller OS (U-Boot or Linux kernel). The former are services without
> > such dependencies. When i posted OP-TEE services discovery in U-Boot,
> > I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
> > dependencies).
> > I made it intentionally as U-Boot tee-supplicant does not implement
> > all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
> > on tee-supplicant support, it is not enumerated/discovered.
> >
> > The point is the U-Boot tee-suppl. does implement the few RPC services
> > FTPM TA needs (that are memory allocation and RPMB access).
> > So Patrick argued that U-Boot can as well enumerate OP-TEE service
> > *with* tee-suppl. devices. The optee ftpm driver can register to this
> > service discovery and will operate properly.
> > What puzzled me was that discovery of OP-TEE services that require
> > tee-suppl. services not available in U-Boot would end in a failure of
> > the service, but as Patrick rightly said that it makes no sense for
> > one of add implement u-boot a driver for an OP-TEE service if that
> > service lacks some U-Boot tee-suppl. supports.
> >
> > All in one, my apologies Ilias for this mistake. A change in
> > tee/optee/core.c to also bind services enumerated by OP-TEE command
> > PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
> > functional FTPM TA service. I'll post a change for that.
>
> Thank you for the update.
>
> I talked with Ilias about this at osfc. The problem I think is that it
> is not possible to reference a particular random-number generator
> unless it is present in the device tree.
>
> Here are some things that can produce random numbers, i.e. are in the
> RNG uclass (which we should rename to RAND or RANDOM to fit with the
> other naming there):
> - software
> - SoC
> - TPM
> - OP-TEE
>
> For choosing which one to use, we can use aliases. so rand0 is the
> first device. That alias can point into OP-TEE if desitred.
>
> If we need a particular device to specify which random-number
> generator device it should use, that can be done with a phandle, like:
>
> board-sysinfo {
>    compatible = "vendor,sysinfo-board";
>    ramdom = <&optee_rng>;
> }
>
> So it should work like PCI, where we scan the bus and attach the
> correct device tree subnode to the OP-TEE devices. The PCI code for
> this is at
> pci_bind_bus_devices() where it calls pci_bus_find_devfn().
>
> This means we need to have compatible strings and bindings for any
> devices on the bus which can be used as above. If the subnodes are
> missing, then scanning will result in binding them in any case.

If I understand correctly:
- having the DT node we can bind an OP-TEE service to would allow a
platform to define it as prefered device for a given feature (as for
the rng in your description above);
- if there is no DT node to bind to, then the discovered service can
still be bound to a driver in U-Boot, the restriction is that we
cannot define such as a prefered device.
These 2 cases would remain valid, right?

For the OP-TEE services for which there is no compatible node, maybe
we could bind their device to the OP-TEE DT node itself.

Regards,
etienne


>
> Regards,
> Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Patrick DELAUNAY]

Hi,

On 9/22/22 13:27, Simon Glass wrote:
> Hi Etienne,
>
> On Thu, 22 Sept 2022 at 10:52, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
>> Hello Patrick and all,
>>
>> On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
>> <patrick.delaunay@foss.st.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 9/12/22 20:31, Simon Glass wrote:
>>>> Hi Ilias,
>>>>
>>>> On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
>>>> <ilias.apalodimas@linaro.org> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
>>>>>> Hi Ilias,
>>>>>>
>>>>>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
>>>>>> <ilias.apalodimas@linaro.org> wrote:
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
>>>>>>>> <ilias.apalodimas@linaro.org> wrote:
>>>>>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
>>>>>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
>>>>>>>>> whichwe can
>>>>>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
>>>>>>>>> noting
>>>>>>>>> that we already have a workaround for RNG. The details are in
>>>>>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
>>>>>>>>>
>>>>>>>>> So let's add a list of devices based on U-Boot Kconfig options
>>>>>>>>> that we will
>>>>>>>>> scan until we properly implement the tee-bus functionality.
>>>>>>>>>
>>>>>>>>> While at it change the behaviour of the tee core itself wrt to device
>>>>>>>>> binding. If some device binding fails, print a warning instead of
>>>>>>>>> disabling OP-TEE.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
>>>>>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
>>>>>>>>> ---
>>>>>>>>> Changes since v3:
>>>>>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
>>>>>>>>> it's not
>>>>>>>>> really needed
>>>>>>>>> - Changed the style of the optee_bus_probe[] definition to
>>>>>>>>> {.drv_name = xxx, .dev_name = yyy }
>>>>>>>>>
>>>>>>>>> Changes since v2:
>>>>>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
>>>>>>>>>
>>>>>>>>> Changes since v1:
>>>>>>>>> - remove a macro and use ARRAY_SIZE directly
>>>>>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
>>>>>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
>>>>>>>>> index a89d62aaf0b3..c201a4635e6b 100644
>>>>>>>>> --- a/drivers/tee/optee/core.c
>>>>>>>>> +++ b/drivers/tee/optee/core.c
>>>>>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
>>>>>>>>> optee_invoke_fn *invoke_fn;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> +static const struct {
>>>>>>>>> + const char *drv_name;
>>>>>>>>> + const char *dev_name;
>>>>>>>>> +} optee_bus_probe[] = {
>>>>>>>>> +#ifdef CONFIG_RNG_OPTEE
>>>>>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
>>>>>>>>> +#endif
>>>>>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
>>>>>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
>>>>>>>>> +#endif
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> struct rpc_param {
>>>>>>>>> u32 a0;
>>>>>>>>> u32 a1;
>>>>>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
>>>>>>>>> {
>>>>>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
>>>>>>>>> u32 sec_caps;
>>>>>>>>> - struct udevice *child;
>>>>>>>>> - int ret;
>>>>>>>>> + int ret, i;
>>>>>>>>>
>>>>>>>>> if (!is_optee_api(pdata->invoke_fn)) {
>>>>>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
>>>>>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
>>>>>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
>>>>>>>>> * only bind the drivers associated to the supported OP-TEETA
>>>>>>>>> */
>>>>>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
>>>>>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
>>>>>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
>>>>>>>>> + optee_bus_probe[i].dev_name, NULL);
>>>>>>>>> if (ret)
>>>>>>>>> - return ret;
>>>>>>>>> + dev_warn(dev, "Failed to bind device %s\n",
>>>>>>>>> + optee_bus_probe[i].dev_name);
>>>>>>>> Please add device tree nodes for these and all this code can go away.
>>>>>>> That's the exact opposite of what the commit message describes. OP-TEE
>>>>>>> supports a scannable bus ifor TAs that behave like hardware blocks and
>>>>>>> doesn't need a DT entry. Since it's really the TAs compilation decision
>>>>>>> to support that or not having them as a DT node is not always the right
>>>>>>> choice.
>>>>>> This is continuing the perversion of how things are supposed to work
>>>>>> in driver model.
>>>>> Which is not the only thing we need to keep in mind though.
>>>>>
>>>>>> We need to talk about this because it is simply the wrong way to be
>>>>>> approaching this.
>>>>> This is already part of other software components though, e.g it's
>>>>> already in the kernel. So I don't think it's the wrong approach.
>>>>>
>>>>>> There is nothing wrong with putting things in the DT
>>>>>> and this is how U-Boot works. For now, please create a binding and get
>>>>>> it reviewed. You don't need all the internal objects but you do need
>>>>>> an OP-TEE driver and node, as we have with PCI.
>>>>> Some things *are* working without a DT entry. You had similar
>>>>> concerns on FF-A (where you requested a DT node again) and people gave
>>>>> the exact same response. As long as a bus is scanable in any way,
>>>>> it's preferable to than adding a DT entry. Moreover this code does
>>>>> not prevent anyone from adding a DT entry.
>>>>>
>>>>> To make things even worse if the TA is compiled as 'scanable' and has
>>>>> a DT entry, it might cause issues down the road when being probed by
>>>>> the kernel. So really this is just a patch that makes u-boot behave
>>>>> and plug in properly to the rest of the ecosystem
>>>> Calling device_bind() is supposed to be used in extremis. I don't see
>>>> any scanning of an OP-TEE bus here. I just see it binding two child
>>>> devices which are hard-coded in U-Boot. What am I missing?
>>>
>>> The tee bus is supported in Linux kernel (each TA have a UUID and
>>> is discoverable by the TEE driver).
>>>
>>> see drivers/tee/optee/core.c::optee_bus_scan()
>>> and "struct tee_client_driver" with TA UUID
>>> It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
>>>
>>> => TA support was hardcoded, under the associated CONFIG
>>>         and the probe failed when the TA is not present.
>>>         for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
>>>
>>> The TEE bus feature is added by the Etienne in the serie [1].
>>> This bus is more flexible and avoid OP-TEE to dynamically modify the
>>> device tree
>>> to add/remove the supported SW component (TA support are activated
>>> during OP-TEE
>>> compilation) as the binding is managed dynamically in OP-TEE as it is
>>> done in Linux.
>>>
>>> For information, on STM32MP15 platform, I have the trace "can't open
>>> session:" for
>>> RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
>>> OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
>>>
>>> [1] drivers: tee: optee: remove unused probe local variable
>>>
>>> http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
>>>
>>>> This appears to be a Linaro binding, so you should be able to update
>>>> it easily enough.
>>>>
>> Discussing with Patrick, he made a suggestion and showed me I was
>> wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
>> OP-TEE exposes 2 levels of service discovery, so-called devices
>> enumeration and device-with-supplicant enumeration. The later are
>> OP-TEE services that depend on RPC service exposed to OP-TEE by in the
>> caller OS (U-Boot or Linux kernel). The former are services without
>> such dependencies. When i posted OP-TEE services discovery in U-Boot,
>> I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
>> dependencies).
>> I made it intentionally as U-Boot tee-supplicant does not implement
>> all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
>> on tee-supplicant support, it is not enumerated/discovered.
>>
>> The point is the U-Boot tee-suppl. does implement the few RPC services
>> FTPM TA needs (that are memory allocation and RPMB access).
>> So Patrick argued that U-Boot can as well enumerate OP-TEE service
>> *with* tee-suppl. devices. The optee ftpm driver can register to this
>> service discovery and will operate properly.
>> What puzzled me was that discovery of OP-TEE services that require
>> tee-suppl. services not available in U-Boot would end in a failure of
>> the service, but as Patrick rightly said that it makes no sense for
>> one of add implement u-boot a driver for an OP-TEE service if that
>> service lacks some U-Boot tee-suppl. supports.
>>
>> All in one, my apologies Ilias for this mistake. A change in
>> tee/optee/core.c to also bind services enumerated by OP-TEE command
>> PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
>> functional FTPM TA service. I'll post a change for that.
> Thank you for the update.
>
> I talked with Ilias about this at osfc. The problem I think is that it
> is not possible to reference a particular random-number generator
> unless it is present in the device tree.
>
> Here are some things that can produce random numbers, i.e. are in the
> RNG uclass (which we should rename to RAND or RANDOM to fit with the
> other naming there):
> - software
> - SoC
> - TPM
> - OP-TEE
>
> For choosing which one to use, we can use aliases. so rand0 is the
> first device. That alias can point into OP-TEE if desitred.
>
> If we need a particular device to specify which random-number
> generator device it should use, that can be done with a phandle, like:
>
> board-sysinfo {
>     compatible = "vendor,sysinfo-board";
>     ramdom = <&optee_rng>;
> }


Yes, I agree


Today the "TA" drivers are bound without any node...

So phandle support is not possible.


But the driver for TA can be associated to the OP-TEE node with the 
simple patch


--------------------------- drivers/tee/optee/core.c 
---------------------------
index 9240277579b..96f08074443 100644
@@ -92,7 +92,8 @@ static int bind_service_list(struct udevice *dev, 
struct tee_shm *service_list,
          if (!service)
              continue;

-        ret = device_bind_driver(dev, service->driver_name, 
service->driver_name, NULL);
+        ret = device_bind_driver_to_node(dev, service->driver_name, 
service->driver_name,
+                         dev_ofnode(dev), NULL);
          if (ret) {
              dev_warn(dev, "%s was not bound: %d, ignored\n", 
service->driver_name, ret);
              continue;
@@ -832,7 +833,8 @@ static int optee_probe(struct udevice *dev)
           * Discovery of TAs on the TEE bus is not supported in U-Boot:
           * only bind the drivers associated to the supported OP-TEE TA
           */
-        ret = device_bind_driver(dev, "optee-rng", "optee-rng", NULL);
+        ret = device_bind_driver_to_node(dev, "optee-rng", "optee-rng",
+                         dev_ofnode(dev), NULL);
          if (ret)
              return ret;
      }


=> the RNG provide by TA RNG in OP-TEE is accessible with a phandle to 
optee,

       for example:


     firmware {
         optee: optee {
             method = "smc";
             compatible = "linaro,optee-tz";
         };

     };

board-sysinfo {
    compatible = "vendor,sysinfo-board";
    ramdom = <&optee>;
}



> So it should work like PCI, where we scan the bus and attach the
> correct device tree subnode to the OP-TEE devices. The PCI code for
> this is at
> pci_bind_bus_devices() where it calls pci_bus_find_devfn().


I don't know the full driver model support for PCI but it seens

the compatible is optional.

=> doc/develop/driver-model/pci-info.rst

"If PCI devices are not listed in the device tree, U_BOOT_PCI_DEVICE can 
be used

to specify the driver to use for the device."


>
> This means we need to have compatible strings and bindings for any
> devices on the bus which can be used as above. If the subnodes are
> missing, then scanning will result in binding them in any case.


with my proposal, subnode is not needed

if only one instance of each UCLASS is exported by OP-TEE.


we can identify the handle with the parent node = optee.

we don't need to modify the kernel device tree.


I just test it

      aliases {
          i2c3 = &i2c4;
          rng1 = &optee;
      };


With OP-TEE providing RNG and without discovery.

and phandle is working to force sequence number

   STM32MP> dm uclass

   ...

   uclass 83: rng
   0     optee-rng @ dbb4b230, seq 1
   ...


if you think it is a good idea I can push this patch.

Even I don't sure that solve all the issues.


Regards

Patrick


>
> Regards,
> Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Etienne Carriere]

Hello Sumit,


On Thu, 22 Sept 2022 at 12:15, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Thu, 22 Sept 2022 at 14:22, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Patrick and all,
> >
> > On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
> > <patrick.delaunay@foss.st.com> wrote:
> > >
> > >
> > > Hi Simon,
> > >
> > > On 9/12/22 20:31, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > >> Hi Simon,
> > > >>
> > > >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> > > >>> Hi Ilias,
> > > >>>
> > > >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > > >>> <ilias.apalodimas@linaro.org> wrote:
> > > >>>> Hi Simon,
> > > >>>>
> > > >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > > >>>>> Hi,
> > > >>>>>
> > > >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > > >>>>> <ilias.apalodimas@linaro.org> wrote:
> > > >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> > > >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> > > >>>>>> whichwe can
> > > >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> > > >>>>>> noting
> > > >>>>>> that we already have a workaround for RNG. The details are in
> > > >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > >>>>>>
> > > >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> > > >>>>>> that we will
> > > >>>>>> scan until we properly implement the tee-bus functionality.
> > > >>>>>>
> > > >>>>>> While at it change the behaviour of the tee core itself wrt to device
> > > >>>>>> binding. If some device binding fails, print a warning instead of
> > > >>>>>> disabling OP-TEE.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > >>>>>> ---
> > > >>>>>> Changes since v3:
> > > >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> > > >>>>>> it's not
> > > >>>>>> really needed
> > > >>>>>> - Changed the style of the optee_bus_probe[] definition to
> > > >>>>>> {.drv_name = xxx, .dev_name = yyy }
> > > >>>>>>
> > > >>>>>> Changes since v2:
> > > >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > >>>>>>
> > > >>>>>> Changes since v1:
> > > >>>>>> - remove a macro and use ARRAY_SIZE directly
> > > >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> > > >>>>>> --- a/drivers/tee/optee/core.c
> > > >>>>>> +++ b/drivers/tee/optee/core.c
> > > >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> > > >>>>>> optee_invoke_fn *invoke_fn;
> > > >>>>>> };
> > > >>>>>>
> > > >>>>>> +static const struct {
> > > >>>>>> + const char *drv_name;
> > > >>>>>> + const char *dev_name;
> > > >>>>>> +} optee_bus_probe[] = {
> > > >>>>>> +#ifdef CONFIG_RNG_OPTEE
> > > >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > >>>>>> +#endif
> > > >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> > > >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > >>>>>> +#endif
> > > >>>>>> +};
> > > >>>>>> +
> > > >>>>>> struct rpc_param {
> > > >>>>>> u32 a0;
> > > >>>>>> u32 a1;
> > > >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > > >>>>>> {
> > > >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> > > >>>>>> u32 sec_caps;
> > > >>>>>> - struct udevice *child;
> > > >>>>>> - int ret;
> > > >>>>>> + int ret, i;
> > > >>>>>>
> > > >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> > > >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> > > >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > > >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > > >>>>>> * only bind the drivers associated to the supported OP-TEETA
> > > >>>>>> */
> > > >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > >>>>>> +
> > > >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > >>>>>> + optee_bus_probe[i].dev_name, NULL);
> > > >>>>>> if (ret)
> > > >>>>>> - return ret;
> > > >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> > > >>>>>> + optee_bus_probe[i].dev_name);
> > > >>>>> Please add device tree nodes for these and all this code can go away.
> > > >>>> That's the exact opposite of what the commit message describes. OP-TEE
> > > >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> > > >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> > > >>>> to support that or not having them as a DT node is not always the right
> > > >>>> choice.
> > > >>> This is continuing the perversion of how things are supposed to work
> > > >>> in driver model.
> > > >> Which is not the only thing we need to keep in mind though.
> > > >>
> > > >>> We need to talk about this because it is simply the wrong way to be
> > > >>> approaching this.
> > > >> This is already part of other software components though, e.g it's
> > > >> already in the kernel. So I don't think it's the wrong approach.
> > > >>
> > > >>> There is nothing wrong with putting things in the DT
> > > >>> and this is how U-Boot works. For now, please create a binding and get
> > > >>> it reviewed. You don't need all the internal objects but you do need
> > > >>> an OP-TEE driver and node, as we have with PCI.
> > > >> Some things *are* working without a DT entry. You had similar
> > > >> concerns on FF-A (where you requested a DT node again) and people gave
> > > >> the exact same response. As long as a bus is scanable in any way,
> > > >> it's preferable to than adding a DT entry. Moreover this code does
> > > >> not prevent anyone from adding a DT entry.
> > > >>
> > > >> To make things even worse if the TA is compiled as 'scanable' and has
> > > >> a DT entry, it might cause issues down the road when being probed by
> > > >> the kernel. So really this is just a patch that makes u-boot behave
> > > >> and plug in properly to the rest of the ecosystem
> > > > Calling device_bind() is supposed to be used in extremis. I don't see
> > > > any scanning of an OP-TEE bus here. I just see it binding two child
> > > > devices which are hard-coded in U-Boot. What am I missing?
> > >
> > >
> > > The tee bus is supported in Linux kernel (each TA have a UUID and
> > > is discoverable by the TEE driver).
> > >
> > > see drivers/tee/optee/core.c::optee_bus_scan()
> > > and "struct tee_client_driver" with TA UUID
> > > It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
> > >
> > > => TA support was hardcoded, under the associated CONFIG
> > >        and the probe failed when the TA is not present.
> > >        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
> > >
> > > The TEE bus feature is added by the Etienne in the serie [1].
> > > This bus is more flexible and avoid OP-TEE to dynamically modify the
> > > device tree
> > > to add/remove the supported SW component (TA support are activated
> > > during OP-TEE
> > > compilation) as the binding is managed dynamically in OP-TEE as it is
> > > done in Linux.
> > >
> > > For information, on STM32MP15 platform, I have the trace "can't open
> > > session:" for
> > > RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> > > OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
> > >
> > > [1] drivers: tee: optee: remove unused probe local variable
> > >
> > > http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
> > >
> > > > This appears to be a Linaro binding, so you should be able to update
> > > > it easily enough.
> > > >
> >
> > Discussing with Patrick, he made a suggestion and showed me I was
> > wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
> > OP-TEE exposes 2 levels of service discovery, so-called devices
> > enumeration and device-with-supplicant enumeration. The later are
> > OP-TEE services that depend on RPC service exposed to OP-TEE by in the
> > caller OS (U-Boot or Linux kernel). The former are services without
> > such dependencies. When i posted OP-TEE services discovery in U-Boot,
> > I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
> > dependencies).
> > I made it intentionally as U-Boot tee-supplicant does not implement
> > all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
> > on tee-supplicant support, it is not enumerated/discovered.
> >
> > The point is the U-Boot tee-suppl. does implement the few RPC services
> > FTPM TA needs (that are memory allocation and RPMB access).
> > So Patrick argued that U-Boot can as well enumerate OP-TEE service
> > *with* tee-suppl. devices. The optee ftpm driver can register to this
> > service discovery and will operate properly.
> > What puzzled me was that discovery of OP-TEE services that require
> > tee-suppl. services not available in U-Boot would end in a failure of
> > the service, but as Patrick rightly said that it makes no sense for
> > one of add implement u-boot a driver for an OP-TEE service if that
> > service lacks some U-Boot tee-suppl. supports.
>
> +1
>
> >
> > All in one, my apologies Ilias for this mistake. A change in
> > tee/optee/core.c to also bind services enumerated by OP-TEE command
> > PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
> > functional FTPM TA service. I'll post a change for that.
> >
>
> I think that should be gated under CONFIG_SUPPORT_EMMC_RPMB as if
> that's unavailable FTPM TA service can't be functional.

I think it should rather be CONFIG_TPM2_FTPM_TEE that should depend on
CONFIG_SUPPORT_EMMC_RPMB, not the optee service discovery itself.
Note also current tee-supplicant in U-Boot also implements another RPC
for OP-TEE: an I2C  interface (CONFIG_OPTEE && CONFIG_DM_I2C). An
OP-TEE service based on this I2C RPC interface could well be
discovered and used by U-Boot.

Regards,
Etienne

>
> -Sumit
>
> > Regards,
> > Etienne
> >
> > > > Regards,
> > > > Simon
> > >
> > > Regards
> > > Patrick

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Sumit Garg]

On Fri, 23 Sept 2022 at 11:16, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Sumit,
>
>
> On Thu, 22 Sept 2022 at 12:15, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Thu, 22 Sept 2022 at 14:22, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Hello Patrick and all,
> > >
> > > On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
> > > <patrick.delaunay@foss.st.com> wrote:
> > > >
> > > >
> > > > Hi Simon,
> > > >
> > > > On 9/12/22 20:31, Simon Glass wrote:
> > > > > Hi Ilias,
> > > > >
> > > > > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >> Hi Simon,
> > > > >>
> > > > >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> > > > >>> Hi Ilias,
> > > > >>>
> > > > >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > > > >>> <ilias.apalodimas@linaro.org> wrote:
> > > > >>>> Hi Simon,
> > > > >>>>
> > > > >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > > > >>>>> Hi,
> > > > >>>>>
> > > > >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > > > >>>>> <ilias.apalodimas@linaro.org> wrote:
> > > > >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> > > > >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> > > > >>>>>> whichwe can
> > > > >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> > > > >>>>>> noting
> > > > >>>>>> that we already have a workaround for RNG. The details are in
> > > > >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > > >>>>>>
> > > > >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> > > > >>>>>> that we will
> > > > >>>>>> scan until we properly implement the tee-bus functionality.
> > > > >>>>>>
> > > > >>>>>> While at it change the behaviour of the tee core itself wrt to device
> > > > >>>>>> binding. If some device binding fails, print a warning instead of
> > > > >>>>>> disabling OP-TEE.
> > > > >>>>>>
> > > > >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > >>>>>> ---
> > > > >>>>>> Changes since v3:
> > > > >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> > > > >>>>>> it's not
> > > > >>>>>> really needed
> > > > >>>>>> - Changed the style of the optee_bus_probe[] definition to
> > > > >>>>>> {.drv_name = xxx, .dev_name = yyy }
> > > > >>>>>>
> > > > >>>>>> Changes since v2:
> > > > >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > > >>>>>>
> > > > >>>>>> Changes since v1:
> > > > >>>>>> - remove a macro and use ARRAY_SIZE directly
> > > > >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > > >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> > > > >>>>>>
> > > > >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > > >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> > > > >>>>>> --- a/drivers/tee/optee/core.c
> > > > >>>>>> +++ b/drivers/tee/optee/core.c
> > > > >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> > > > >>>>>> optee_invoke_fn *invoke_fn;
> > > > >>>>>> };
> > > > >>>>>>
> > > > >>>>>> +static const struct {
> > > > >>>>>> + const char *drv_name;
> > > > >>>>>> + const char *dev_name;
> > > > >>>>>> +} optee_bus_probe[] = {
> > > > >>>>>> +#ifdef CONFIG_RNG_OPTEE
> > > > >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > > >>>>>> +#endif
> > > > >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> > > > >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > > >>>>>> +#endif
> > > > >>>>>> +};
> > > > >>>>>> +
> > > > >>>>>> struct rpc_param {
> > > > >>>>>> u32 a0;
> > > > >>>>>> u32 a1;
> > > > >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > > > >>>>>> {
> > > > >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> > > > >>>>>> u32 sec_caps;
> > > > >>>>>> - struct udevice *child;
> > > > >>>>>> - int ret;
> > > > >>>>>> + int ret, i;
> > > > >>>>>>
> > > > >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> > > > >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> > > > >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > > > >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > > > >>>>>> * only bind the drivers associated to the supported OP-TEETA
> > > > >>>>>> */
> > > > >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > > >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > > >>>>>> +
> > > > >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > > >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > > >>>>>> + optee_bus_probe[i].dev_name, NULL);
> > > > >>>>>> if (ret)
> > > > >>>>>> - return ret;
> > > > >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> > > > >>>>>> + optee_bus_probe[i].dev_name);
> > > > >>>>> Please add device tree nodes for these and all this code can go away.
> > > > >>>> That's the exact opposite of what the commit message describes. OP-TEE
> > > > >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> > > > >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> > > > >>>> to support that or not having them as a DT node is not always the right
> > > > >>>> choice.
> > > > >>> This is continuing the perversion of how things are supposed to work
> > > > >>> in driver model.
> > > > >> Which is not the only thing we need to keep in mind though.
> > > > >>
> > > > >>> We need to talk about this because it is simply the wrong way to be
> > > > >>> approaching this.
> > > > >> This is already part of other software components though, e.g it's
> > > > >> already in the kernel. So I don't think it's the wrong approach.
> > > > >>
> > > > >>> There is nothing wrong with putting things in the DT
> > > > >>> and this is how U-Boot works. For now, please create a binding and get
> > > > >>> it reviewed. You don't need all the internal objects but you do need
> > > > >>> an OP-TEE driver and node, as we have with PCI.
> > > > >> Some things *are* working without a DT entry. You had similar
> > > > >> concerns on FF-A (where you requested a DT node again) and people gave
> > > > >> the exact same response. As long as a bus is scanable in any way,
> > > > >> it's preferable to than adding a DT entry. Moreover this code does
> > > > >> not prevent anyone from adding a DT entry.
> > > > >>
> > > > >> To make things even worse if the TA is compiled as 'scanable' and has
> > > > >> a DT entry, it might cause issues down the road when being probed by
> > > > >> the kernel. So really this is just a patch that makes u-boot behave
> > > > >> and plug in properly to the rest of the ecosystem
> > > > > Calling device_bind() is supposed to be used in extremis. I don't see
> > > > > any scanning of an OP-TEE bus here. I just see it binding two child
> > > > > devices which are hard-coded in U-Boot. What am I missing?
> > > >
> > > >
> > > > The tee bus is supported in Linux kernel (each TA have a UUID and
> > > > is discoverable by the TEE driver).
> > > >
> > > > see drivers/tee/optee/core.c::optee_bus_scan()
> > > > and "struct tee_client_driver" with TA UUID
> > > > It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
> > > >
> > > > => TA support was hardcoded, under the associated CONFIG
> > > >        and the probe failed when the TA is not present.
> > > >        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
> > > >
> > > > The TEE bus feature is added by the Etienne in the serie [1].
> > > > This bus is more flexible and avoid OP-TEE to dynamically modify the
> > > > device tree
> > > > to add/remove the supported SW component (TA support are activated
> > > > during OP-TEE
> > > > compilation) as the binding is managed dynamically in OP-TEE as it is
> > > > done in Linux.
> > > >
> > > > For information, on STM32MP15 platform, I have the trace "can't open
> > > > session:" for
> > > > RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> > > > OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
> > > >
> > > > [1] drivers: tee: optee: remove unused probe local variable
> > > >
> > > > http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
> > > >
> > > > > This appears to be a Linaro binding, so you should be able to update
> > > > > it easily enough.
> > > > >
> > >
> > > Discussing with Patrick, he made a suggestion and showed me I was
> > > wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
> > > OP-TEE exposes 2 levels of service discovery, so-called devices
> > > enumeration and device-with-supplicant enumeration. The later are
> > > OP-TEE services that depend on RPC service exposed to OP-TEE by in the
> > > caller OS (U-Boot or Linux kernel). The former are services without
> > > such dependencies. When i posted OP-TEE services discovery in U-Boot,
> > > I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
> > > dependencies).
> > > I made it intentionally as U-Boot tee-supplicant does not implement
> > > all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
> > > on tee-supplicant support, it is not enumerated/discovered.
> > >
> > > The point is the U-Boot tee-suppl. does implement the few RPC services
> > > FTPM TA needs (that are memory allocation and RPMB access).
> > > So Patrick argued that U-Boot can as well enumerate OP-TEE service
> > > *with* tee-suppl. devices. The optee ftpm driver can register to this
> > > service discovery and will operate properly.
> > > What puzzled me was that discovery of OP-TEE services that require
> > > tee-suppl. services not available in U-Boot would end in a failure of
> > > the service, but as Patrick rightly said that it makes no sense for
> > > one of add implement u-boot a driver for an OP-TEE service if that
> > > service lacks some U-Boot tee-suppl. supports.
> >
> > +1
> >
> > >
> > > All in one, my apologies Ilias for this mistake. A change in
> > > tee/optee/core.c to also bind services enumerated by OP-TEE command
> > > PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
> > > functional FTPM TA service. I'll post a change for that.
> > >
> >
> > I think that should be gated under CONFIG_SUPPORT_EMMC_RPMB as if
> > that's unavailable FTPM TA service can't be functional.
>
> I think it should rather be CONFIG_TPM2_FTPM_TEE that should depend on
> CONFIG_SUPPORT_EMMC_RPMB, not the optee service discovery itself.

This sounds reasonable to me.

> Note also current tee-supplicant in U-Boot also implements another RPC
> for OP-TEE: an I2C  interface (CONFIG_OPTEE && CONFIG_DM_I2C). An
> OP-TEE service based on this I2C RPC interface could well be
> discovered and used by U-Boot.

Agree.

-Sumit

>
> Regards,
> Etienne
>
> >
> > -Sumit
> >
> > > Regards,
> > > Etienne
> > >
> > > > > Regards,
> > > > > Simon
> > > >
> > > > Regards
> > > > Patrick

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Etienne Carriere]

On Mon, 26 Sept 2022 at 09:06, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Fri, 23 Sept 2022 at 11:16, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Sumit,
> >
> >
> > On Thu, 22 Sept 2022 at 12:15, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Thu, 22 Sept 2022 at 14:22, Etienne Carriere
> > > <etienne.carriere@linaro.org> wrote:
> > > >
> > > > Hello Patrick and all,
> > > >
> > > > On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
> > > > <patrick.delaunay@foss.st.com> wrote:
> > > > >
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On 9/12/22 20:31, Simon Glass wrote:
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >> Hi Simon,
> > > > > >>
> > > > > >> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> > > > > >>> Hi Ilias,
> > > > > >>>
> > > > > >>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > > > > >>> <ilias.apalodimas@linaro.org> wrote:
> > > > > >>>> Hi Simon,
> > > > > >>>>
> > > > > >>>> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > > > > >>>>> Hi,
> > > > > >>>>>
> > > > > >>>>> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > > > > >>>>> <ilias.apalodimas@linaro.org> wrote:
> > > > > >>>>>> Late versions of OP-TEE support a pseudo bus. TAs that behave as
> > > > > >>>>>> hardware blocks (e.g TPM, RNG etc) present themselves on a bus
> > > > > >>>>>> whichwe can
> > > > > >>>>>> scan. Unfortunately U-Boot doesn't support that yet. It's worth
> > > > > >>>>>> noting
> > > > > >>>>>> that we already have a workaround for RNG. The details are in
> > > > > >>>>>> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > > > >>>>>>
> > > > > >>>>>> So let's add a list of devices based on U-Boot Kconfig options
> > > > > >>>>>> that we will
> > > > > >>>>>> scan until we properly implement the tee-bus functionality.
> > > > > >>>>>>
> > > > > >>>>>> While at it change the behaviour of the tee core itself wrt to device
> > > > > >>>>>> binding. If some device binding fails, print a warning instead of
> > > > > >>>>>> disabling OP-TEE.
> > > > > >>>>>>
> > > > > >>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > >>>>>> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > >>>>>> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > > >>>>>> ---
> > > > > >>>>>> Changes since v3:
> > > > > >>>>>> - Use NULL instead of a child ptr on device_bind_driver(), since
> > > > > >>>>>> it's not
> > > > > >>>>>> really needed
> > > > > >>>>>> - Changed the style of the optee_bus_probe[] definition to
> > > > > >>>>>> {.drv_name = xxx, .dev_name = yyy }
> > > > > >>>>>>
> > > > > >>>>>> Changes since v2:
> > > > > >>>>>> - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > > > >>>>>>
> > > > > >>>>>> Changes since v1:
> > > > > >>>>>> - remove a macro and use ARRAY_SIZE directly
> > > > > >>>>>> drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > > > >>>>>> 1 file changed, 19 insertions(+), 5 deletions(-)
> > > > > >>>>>>
> > > > > >>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > > > >>>>>> index a89d62aaf0b3..c201a4635e6b 100644
> > > > > >>>>>> --- a/drivers/tee/optee/core.c
> > > > > >>>>>> +++ b/drivers/tee/optee/core.c
> > > > > >>>>>> @@ -31,6 +31,18 @@ struct optee_pdata {
> > > > > >>>>>> optee_invoke_fn *invoke_fn;
> > > > > >>>>>> };
> > > > > >>>>>>
> > > > > >>>>>> +static const struct {
> > > > > >>>>>> + const char *drv_name;
> > > > > >>>>>> + const char *dev_name;
> > > > > >>>>>> +} optee_bus_probe[] = {
> > > > > >>>>>> +#ifdef CONFIG_RNG_OPTEE
> > > > > >>>>>> + { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > > > >>>>>> +#endif
> > > > > >>>>>> +#ifdef CONFIG_TPM2_FTPM_TEE
> > > > > >>>>>> + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > > > >>>>>> +#endif
> > > > > >>>>>> +};
> > > > > >>>>>> +
> > > > > >>>>>> struct rpc_param {
> > > > > >>>>>> u32 a0;
> > > > > >>>>>> u32 a1;
> > > > > >>>>>> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > > > > >>>>>> {
> > > > > >>>>>> struct optee_pdata *pdata = dev_get_plat(dev);
> > > > > >>>>>> u32 sec_caps;
> > > > > >>>>>> - struct udevice *child;
> > > > > >>>>>> - int ret;
> > > > > >>>>>> + int ret, i;
> > > > > >>>>>>
> > > > > >>>>>> if (!is_optee_api(pdata->invoke_fn)) {
> > > > > >>>>>> dev_err(dev, "OP-TEE api uid mismatch\n");
> > > > > >>>>>> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > > > > >>>>>> * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > > > > >>>>>> * only bind the drivers associated to the supported OP-TEETA
> > > > > >>>>>> */
> > > > > >>>>>> - if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > > > >>>>>> - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > > > >>>>>> +
> > > > > >>>>>> + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > > > >>>>>> + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > > > >>>>>> + optee_bus_probe[i].dev_name, NULL);
> > > > > >>>>>> if (ret)
> > > > > >>>>>> - return ret;
> > > > > >>>>>> + dev_warn(dev, "Failed to bind device %s\n",
> > > > > >>>>>> + optee_bus_probe[i].dev_name);
> > > > > >>>>> Please add device tree nodes for these and all this code can go away.
> > > > > >>>> That's the exact opposite of what the commit message describes. OP-TEE
> > > > > >>>> supports a scannable bus ifor TAs that behave like hardware blocks and
> > > > > >>>> doesn't need a DT entry. Since it's really the TAs compilation decision
> > > > > >>>> to support that or not having them as a DT node is not always the right
> > > > > >>>> choice.
> > > > > >>> This is continuing the perversion of how things are supposed to work
> > > > > >>> in driver model.
> > > > > >> Which is not the only thing we need to keep in mind though.
> > > > > >>
> > > > > >>> We need to talk about this because it is simply the wrong way to be
> > > > > >>> approaching this.
> > > > > >> This is already part of other software components though, e.g it's
> > > > > >> already in the kernel. So I don't think it's the wrong approach.
> > > > > >>
> > > > > >>> There is nothing wrong with putting things in the DT
> > > > > >>> and this is how U-Boot works. For now, please create a binding and get
> > > > > >>> it reviewed. You don't need all the internal objects but you do need
> > > > > >>> an OP-TEE driver and node, as we have with PCI.
> > > > > >> Some things *are* working without a DT entry. You had similar
> > > > > >> concerns on FF-A (where you requested a DT node again) and people gave
> > > > > >> the exact same response. As long as a bus is scanable in any way,
> > > > > >> it's preferable to than adding a DT entry. Moreover this code does
> > > > > >> not prevent anyone from adding a DT entry.
> > > > > >>
> > > > > >> To make things even worse if the TA is compiled as 'scanable' and has
> > > > > >> a DT entry, it might cause issues down the road when being probed by
> > > > > >> the kernel. So really this is just a patch that makes u-boot behave
> > > > > >> and plug in properly to the rest of the ecosystem
> > > > > > Calling device_bind() is supposed to be used in extremis. I don't see
> > > > > > any scanning of an OP-TEE bus here. I just see it binding two child
> > > > > > devices which are hard-coded in U-Boot. What am I missing?
> > > > >
> > > > >
> > > > > The tee bus is supported in Linux kernel (each TA have a UUID and
> > > > > is discoverable by the TEE driver).
> > > > >
> > > > > see drivers/tee/optee/core.c::optee_bus_scan()
> > > > > and "struct tee_client_driver" with TA UUID
> > > > > It wasn't supported in U-Boot is the first TEE/OP-TEE driver implementation
> > > > >
> > > > > => TA support was hardcoded, under the associated CONFIG
> > > > >        and the probe failed when the TA is not present.
> > > > >        for example, I add this binding for TA_RNG in drivers/rng/optee_rng.c
> > > > >
> > > > > The TEE bus feature is added by the Etienne in the serie [1].
> > > > > This bus is more flexible and avoid OP-TEE to dynamically modify the
> > > > > device tree
> > > > > to add/remove the supported SW component (TA support are activated
> > > > > during OP-TEE
> > > > > compilation) as the binding is managed dynamically in OP-TEE as it is
> > > > > done in Linux.
> > > > >
> > > > > For information, on STM32MP15 platform, I have the trace "can't open
> > > > > session:" for
> > > > > RNG TA for each 'rng' command when this TA is not supported in OP-TEE but
> > > > > OP-TEE RNG driver is activated in U-Boot, because the driver is binding.
> > > > >
> > > > > [1] drivers: tee: optee: remove unused probe local variable
> > > > >
> > > > > http://patchwork.ozlabs.org/project/uboot/list/?series=311351&state=*
> > > > >
> > > > > > This appears to be a Linaro binding, so you should be able to update
> > > > > > it easily enough.
> > > > > >
> > > >
> > > > Discussing with Patrick, he made a suggestion and showed me I was
> > > > wrong in OP-TEE tee-supplicant enumeration constraints in U-Boot.
> > > > OP-TEE exposes 2 levels of service discovery, so-called devices
> > > > enumeration and device-with-supplicant enumeration. The later are
> > > > OP-TEE services that depend on RPC service exposed to OP-TEE by in the
> > > > caller OS (U-Boot or Linux kernel). The former are services without
> > > > such dependencies. When i posted OP-TEE services discovery in U-Boot,
> > > > I made U-Boot to enumerate OP-TEE "devices" (without tee-suppl.
> > > > dependencies).
> > > > I made it intentionally as U-Boot tee-supplicant does not implement
> > > > all OP-TEE RPC services as Linux kernel. Since FTPM TA service relies
> > > > on tee-supplicant support, it is not enumerated/discovered.
> > > >
> > > > The point is the U-Boot tee-suppl. does implement the few RPC services
> > > > FTPM TA needs (that are memory allocation and RPMB access).
> > > > So Patrick argued that U-Boot can as well enumerate OP-TEE service
> > > > *with* tee-suppl. devices. The optee ftpm driver can register to this
> > > > service discovery and will operate properly.
> > > > What puzzled me was that discovery of OP-TEE services that require
> > > > tee-suppl. services not available in U-Boot would end in a failure of
> > > > the service, but as Patrick rightly said that it makes no sense for
> > > > one of add implement u-boot a driver for an OP-TEE service if that
> > > > service lacks some U-Boot tee-suppl. supports.
> > >
> > > +1
> > >
> > > >
> > > > All in one, my apologies Ilias for this mistake. A change in
> > > > tee/optee/core.c to also bind services enumerated by OP-TEE command
> > > > PTA_CMD_GET_DEVICES_SUPPL should enable full dynamic discovery of
> > > > functional FTPM TA service. I'll post a change for that.

FYI, I have posted
https://lore.kernel.org/u-boot/20220928074725.2228353-1-etienne.carriere@linaro.org/T/#u

br,
etienne

> > > >
> > >
> > > I think that should be gated under CONFIG_SUPPORT_EMMC_RPMB as if
> > > that's unavailable FTPM TA service can't be functional.
> >
> > I think it should rather be CONFIG_TPM2_FTPM_TEE that should depend on
> > CONFIG_SUPPORT_EMMC_RPMB, not the optee service discovery itself.
>
> This sounds reasonable to me.
>
> > Note also current tee-supplicant in U-Boot also implements another RPC
> > for OP-TEE: an I2C  interface (CONFIG_OPTEE && CONFIG_DM_I2C). An
> > OP-TEE service based on this I2C RPC interface could well be
> > discovered and used by U-Boot.
>
> Agree.
>
> -Sumit
>
> >
> > Regards,
> > Etienne
> >
> > >
> > > -Sumit
> > >
> > > > Regards,
> > > > Etienne
> > > >
> > > > > > Regards,
> > > > > > Simon
> > > > >
> > > > > Regards
> > > > > Patrick

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Ilias Apalodimas]

Hi Simon,

> > > > > >
> > > > > > Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> > > > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> > > > > > scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> > > > > > that we already have a workaround for RNG.  The details are in
> > > > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > > > >
> > > > > > So let's add a list of devices based on U-Boot Kconfig options that we will
> > > > > > scan until we properly implement the tee-bus functionality.
> > > > > >
> > > > > > While at it change the behaviour of the tee core itself wrt to device
> > > > > > binding.  If some device binding fails, print a warning instead of
> > > > > > disabling OP-TEE.
> > > > > >
> > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > > > ---
> > > > > > Changes since v3:
> > > > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> > > > > > really needed
> > > > > > - Changed the style of the optee_bus_probe[] definition to
> > > > > >   {.drv_name = xxx, .dev_name = yyy }
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > > > >
> > > > > > Changes since v1:
> > > > > > - remove a macro and use ARRAY_SIZE directly
> > > > > >  drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > > > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > > > > >

[...]

> >
> > Some things *are* working without a DT entry.  You had similar
> > concerns on FF-A (where you requested a DT node again) and people gave
> > the exact same response.  As long as a bus is scanable in any way,
> > it's preferable to than adding a DT entry.  Moreover this code does
> > not prevent anyone from adding a DT entry.
> >
> > To make things even worse if the TA is compiled as 'scanable' and has
> > a DT entry, it might cause issues down the road when being probed by
> > the kernel.  So really this is just a patch that makes u-boot behave
> > and plug in properly to the rest of the ecosystem
> 
> Calling device_bind() is supposed to be used in extremis. I don't see
> any scanning of an OP-TEE bus here. I just see it binding two child
> devices which are hard-coded in U-Boot. What am I missing?

The commit description describes the current state of U-Boot

> 
> This appears to be a Linaro binding, so you should be able to update
> it easily enough.

Linaro binding?  The DT is governed by a spec, we commit everything
upstream there.  OTOH I still don't see what we need to put in there.
As we discussed this is a bus that can be used to scan devices.


Thanks
/Ilias
> 
> Regards,
> Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Simon Glass]

Hi Ilias,

On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> > > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> > > > > scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> > > > > that we already have a workaround for RNG.  The details are in
> > > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > > >
> > > > > So let's add a list of devices based on U-Boot Kconfig options that we will
> > > > > scan until we properly implement the tee-bus functionality.
> > > > >
> > > > > While at it change the behaviour of the tee core itself wrt to device
> > > > > binding.  If some device binding fails, print a warning instead of
> > > > > disabling OP-TEE.
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > > ---
> > > > > Changes since v3:
> > > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> > > > > really needed
> > > > > - Changed the style of the optee_bus_probe[] definition to
> > > > >   {.drv_name = xxx, .dev_name = yyy }
> > > > >
> > > > > Changes since v2:
> > > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > > >
> > > > > Changes since v1:
> > > > > - remove a macro and use ARRAY_SIZE directly
> > > > >  drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > > > index a89d62aaf0b3..c201a4635e6b 100644
> > > > > --- a/drivers/tee/optee/core.c
> > > > > +++ b/drivers/tee/optee/core.c
> > > > > @@ -31,6 +31,18 @@ struct optee_pdata {
> > > > >         optee_invoke_fn *invoke_fn;
> > > > >  };
> > > > >
> > > > > +static const struct {
> > > > > +       const char *drv_name;
> > > > > +       const char *dev_name;
> > > > > +} optee_bus_probe[] = {
> > > > > +#ifdef CONFIG_RNG_OPTEE
> > > > > +       { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > > > +#endif
> > > > > +#ifdef CONFIG_TPM2_FTPM_TEE
> > > > > +       { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > > > +#endif
> > > > > +};
> > > > > +
> > > > >  struct rpc_param {
> > > > >         u32     a0;
> > > > >         u32     a1;
> > > > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > > > >  {
> > > > >         struct optee_pdata *pdata = dev_get_plat(dev);
> > > > >         u32 sec_caps;
> > > > > -       struct udevice *child;
> > > > > -       int ret;
> > > > > +       int ret, i;
> > > > >
> > > > >         if (!is_optee_api(pdata->invoke_fn)) {
> > > > >                 dev_err(dev, "OP-TEE api uid mismatch\n");
> > > > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > > > >          * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > > > >          * only bind the drivers associated to the supported OP-TEE TA
> > > > >          */
> > > > > -       if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > > > -               ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > > > +
> > > > > +       for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > > > +               ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > > > +                                        optee_bus_probe[i].dev_name, NULL);
> > > > >                 if (ret)
> > > > > -                       return ret;
> > > > > +                       dev_warn(dev, "Failed to bind device %s\n",
> > > > > +                                optee_bus_probe[i].dev_name);
> > > >
> > > > Please add device tree nodes for these and all this code can go away.
> > >
> > > That's the exact opposite of what the commit message describes.  OP-TEE
> > > supports a scannable bus ifor TAs that  behave like hardware blocks and
> > > doesn't need a DT entry.  Since it's really the TAs compilation decision
> > > to support that or not having them as a DT node is not always the right
> > > choice.
> >
> > This is continuing the perversion of how things are supposed to work
> > in driver model.
>
> Which is not the only thing we need to keep in mind though.
>
> >
> > We need to talk about this because it is simply the wrong way to be
> > approaching this.
>
> This is already part of other software components though, e.g it's
> already in the kernel.  So I don't think it's the wrong approach.
>
> > There is nothing wrong with putting things in the DT
> > and this is how U-Boot works. For now, please create a binding and get
> > it reviewed. You don't need all the internal objects but you do need
> > an OP-TEE driver and node, as we have with PCI.
>
> Some things *are* working without a DT entry.  You had similar
> concerns on FF-A (where you requested a DT node again) and people gave
> the exact same response.  As long as a bus is scanable in any way,
> it's preferable to than adding a DT entry.  Moreover this code does
> not prevent anyone from adding a DT entry.
>
> To make things even worse if the TA is compiled as 'scanable' and has
> a DT entry, it might cause issues down the road when being probed by
> the kernel.  So really this is just a patch that makes u-boot behave
> and plug in properly to the rest of the ecosystem

Calling device_bind() is supposed to be used in extremis. I don't see
any scanning of an OP-TEE bus here. I just see it binding two child
devices which are hard-coded in U-Boot. What am I missing?

This appears to be a Linaro binding, so you should be able to update
it easily enough.

Regards,
Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Ilias Apalodimas]

Hi Simon,

On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> > > > scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> > > > that we already have a workaround for RNG.  The details are in
> > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > > >
> > > > So let's add a list of devices based on U-Boot Kconfig options that we will
> > > > scan until we properly implement the tee-bus functionality.
> > > >
> > > > While at it change the behaviour of the tee core itself wrt to device
> > > > binding.  If some device binding fails, print a warning instead of
> > > > disabling OP-TEE.
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > ---
> > > > Changes since v3:
> > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> > > > really needed
> > > > - Changed the style of the optee_bus_probe[] definition to
> > > >   {.drv_name = xxx, .dev_name = yyy }
> > > >
> > > > Changes since v2:
> > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > > >
> > > > Changes since v1:
> > > > - remove a macro and use ARRAY_SIZE directly
> > > >  drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > > index a89d62aaf0b3..c201a4635e6b 100644
> > > > --- a/drivers/tee/optee/core.c
> > > > +++ b/drivers/tee/optee/core.c
> > > > @@ -31,6 +31,18 @@ struct optee_pdata {
> > > >         optee_invoke_fn *invoke_fn;
> > > >  };
> > > >
> > > > +static const struct {
> > > > +       const char *drv_name;
> > > > +       const char *dev_name;
> > > > +} optee_bus_probe[] = {
> > > > +#ifdef CONFIG_RNG_OPTEE
> > > > +       { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > > +#endif
> > > > +#ifdef CONFIG_TPM2_FTPM_TEE
> > > > +       { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > > +#endif
> > > > +};
> > > > +
> > > >  struct rpc_param {
> > > >         u32     a0;
> > > >         u32     a1;
> > > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > > >  {
> > > >         struct optee_pdata *pdata = dev_get_plat(dev);
> > > >         u32 sec_caps;
> > > > -       struct udevice *child;
> > > > -       int ret;
> > > > +       int ret, i;
> > > >
> > > >         if (!is_optee_api(pdata->invoke_fn)) {
> > > >                 dev_err(dev, "OP-TEE api uid mismatch\n");
> > > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > > >          * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > > >          * only bind the drivers associated to the supported OP-TEE TA
> > > >          */
> > > > -       if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > > -               ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > > +               ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > > +                                        optee_bus_probe[i].dev_name, NULL);
> > > >                 if (ret)
> > > > -                       return ret;
> > > > +                       dev_warn(dev, "Failed to bind device %s\n",
> > > > +                                optee_bus_probe[i].dev_name);
> > >
> > > Please add device tree nodes for these and all this code can go away.
> >
> > That's the exact opposite of what the commit message describes.  OP-TEE
> > supports a scannable bus ifor TAs that  behave like hardware blocks and
> > doesn't need a DT entry.  Since it's really the TAs compilation decision
> > to support that or not having them as a DT node is not always the right
> > choice.
>
> This is continuing the perversion of how things are supposed to work
> in driver model.

Which is not the only thing we need to keep in mind though.

>
> We need to talk about this because it is simply the wrong way to be
> approaching this.

This is already part of other software components though, e.g it's
already in the kernel.  So I don't think it's the wrong approach.

> There is nothing wrong with putting things in the DT
> and this is how U-Boot works. For now, please create a binding and get
> it reviewed. You don't need all the internal objects but you do need
> an OP-TEE driver and node, as we have with PCI.

Some things *are* working without a DT entry.  You had similar
concerns on FF-A (where you requested a DT node again) and people gave
the exact same response.  As long as a bus is scanable in any way,
it's preferable to than adding a DT entry.  Moreover this code does
not prevent anyone from adding a DT entry.

To make things even worse if the TA is compiled as 'scanable' and has
a DT entry, it might cause issues down the road when being probed by
the kernel.  So really this is just a patch that makes u-boot behave
and plug in properly to the rest of the ecosystem

Thanks
/Ilias
>
> Regards,
> Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Simon Glass]

Hi Ilias,

On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> > Hi,
> >
> > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> > > scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> > > that we already have a workaround for RNG.  The details are in
> > > commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> > >
> > > So let's add a list of devices based on U-Boot Kconfig options that we will
> > > scan until we properly implement the tee-bus functionality.
> > >
> > > While at it change the behaviour of the tee core itself wrt to device
> > > binding.  If some device binding fails, print a warning instead of
> > > disabling OP-TEE.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > > Changes since v3:
> > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> > > really needed
> > > - Changed the style of the optee_bus_probe[] definition to
> > >   {.drv_name = xxx, .dev_name = yyy }
> > >
> > > Changes since v2:
> > > - Fixed typo on driver name ftpm-tee -> ftpm_tee
> > >
> > > Changes since v1:
> > > - remove a macro and use ARRAY_SIZE directly
> > >  drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > index a89d62aaf0b3..c201a4635e6b 100644
> > > --- a/drivers/tee/optee/core.c
> > > +++ b/drivers/tee/optee/core.c
> > > @@ -31,6 +31,18 @@ struct optee_pdata {
> > >         optee_invoke_fn *invoke_fn;
> > >  };
> > >
> > > +static const struct {
> > > +       const char *drv_name;
> > > +       const char *dev_name;
> > > +} optee_bus_probe[] = {
> > > +#ifdef CONFIG_RNG_OPTEE
> > > +       { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > > +#endif
> > > +#ifdef CONFIG_TPM2_FTPM_TEE
> > > +       { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > > +#endif
> > > +};
> > > +
> > >  struct rpc_param {
> > >         u32     a0;
> > >         u32     a1;
> > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> > >  {
> > >         struct optee_pdata *pdata = dev_get_plat(dev);
> > >         u32 sec_caps;
> > > -       struct udevice *child;
> > > -       int ret;
> > > +       int ret, i;
> > >
> > >         if (!is_optee_api(pdata->invoke_fn)) {
> > >                 dev_err(dev, "OP-TEE api uid mismatch\n");
> > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> > >          * in U-Boot, the discovery of TA on the TEE bus is not supported:
> > >          * only bind the drivers associated to the supported OP-TEE TA
> > >          */
> > > -       if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > > -               ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > > +               ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > > +                                        optee_bus_probe[i].dev_name, NULL);
> > >                 if (ret)
> > > -                       return ret;
> > > +                       dev_warn(dev, "Failed to bind device %s\n",
> > > +                                optee_bus_probe[i].dev_name);
> >
> > Please add device tree nodes for these and all this code can go away.
>
> That's the exact opposite of what the commit message describes.  OP-TEE
> supports a scannable bus ifor TAs that  behave like hardware blocks and
> doesn't need a DT entry.  Since it's really the TAs compilation decision
> to support that or not having them as a DT node is not always the right
> choice.

This is continuing the perversion of how things are supposed to work
in driver model.

We need to talk about this because it is simply the wrong way to be
approaching this. There is nothing wrong with putting things in the DT
and this is how U-Boot works. For now, please create a binding and get
it reviewed. You don't need all the internal objects but you do need
an OP-TEE driver and node, as we have with PCI.

Regards,
Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Ilias Apalodimas]

Hi Simon,

On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote:
> Hi,
> 
> On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> > scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> > that we already have a workaround for RNG.  The details are in
> > commit 70812bb83da6 ("tee: optee: bind rng optee driver")
> >
> > So let's add a list of devices based on U-Boot Kconfig options that we will
> > scan until we properly implement the tee-bus functionality.
> >
> > While at it change the behaviour of the tee core itself wrt to device
> > binding.  If some device binding fails, print a warning instead of
> > disabling OP-TEE.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v3:
> > - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> > really needed
> > - Changed the style of the optee_bus_probe[] definition to
> >   {.drv_name = xxx, .dev_name = yyy }
> >
> > Changes since v2:
> > - Fixed typo on driver name ftpm-tee -> ftpm_tee
> >
> > Changes since v1:
> > - remove a macro and use ARRAY_SIZE directly
> >  drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index a89d62aaf0b3..c201a4635e6b 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -31,6 +31,18 @@ struct optee_pdata {
> >         optee_invoke_fn *invoke_fn;
> >  };
> >
> > +static const struct {
> > +       const char *drv_name;
> > +       const char *dev_name;
> > +} optee_bus_probe[] = {
> > +#ifdef CONFIG_RNG_OPTEE
> > +       { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> > +#endif
> > +#ifdef CONFIG_TPM2_FTPM_TEE
> > +       { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> > +#endif
> > +};
> > +
> >  struct rpc_param {
> >         u32     a0;
> >         u32     a1;
> > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
> >  {
> >         struct optee_pdata *pdata = dev_get_plat(dev);
> >         u32 sec_caps;
> > -       struct udevice *child;
> > -       int ret;
> > +       int ret, i;
> >
> >         if (!is_optee_api(pdata->invoke_fn)) {
> >                 dev_err(dev, "OP-TEE api uid mismatch\n");
> > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
> >          * in U-Boot, the discovery of TA on the TEE bus is not supported:
> >          * only bind the drivers associated to the supported OP-TEE TA
> >          */
> > -       if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> > -               ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> > +               ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> > +                                        optee_bus_probe[i].dev_name, NULL);
> >                 if (ret)
> > -                       return ret;
> > +                       dev_warn(dev, "Failed to bind device %s\n",
> > +                                optee_bus_probe[i].dev_name);
> 
> Please add device tree nodes for these and all this code can go away.

That's the exact opposite of what the commit message describes.  OP-TEE
supports a scannable bus ifor TAs that  behave like hardware blocks and 
doesn't need a DT entry.  Since it's really the TAs compilation decision
to support that or not having them as a DT node is not always the right
choice.

Thanks
/Ilias

> 
> Regards,
> Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Simon Glass]

Hi,

On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> that we already have a workaround for RNG.  The details are in
> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
>
> So let's add a list of devices based on U-Boot Kconfig options that we will
> scan until we properly implement the tee-bus functionality.
>
> While at it change the behaviour of the tee core itself wrt to device
> binding.  If some device binding fails, print a warning instead of
> disabling OP-TEE.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v3:
> - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> really needed
> - Changed the style of the optee_bus_probe[] definition to
>   {.drv_name = xxx, .dev_name = yyy }
>
> Changes since v2:
> - Fixed typo on driver name ftpm-tee -> ftpm_tee
>
> Changes since v1:
> - remove a macro and use ARRAY_SIZE directly
>  drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index a89d62aaf0b3..c201a4635e6b 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -31,6 +31,18 @@ struct optee_pdata {
>         optee_invoke_fn *invoke_fn;
>  };
>
> +static const struct {
> +       const char *drv_name;
> +       const char *dev_name;
> +} optee_bus_probe[] = {
> +#ifdef CONFIG_RNG_OPTEE
> +       { .drv_name = "optee-rng", .dev_name = "optee-rng" },
> +#endif
> +#ifdef CONFIG_TPM2_FTPM_TEE
> +       { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
> +#endif
> +};
> +
>  struct rpc_param {
>         u32     a0;
>         u32     a1;
> @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
>  {
>         struct optee_pdata *pdata = dev_get_plat(dev);
>         u32 sec_caps;
> -       struct udevice *child;
> -       int ret;
> +       int ret, i;
>
>         if (!is_optee_api(pdata->invoke_fn)) {
>                 dev_err(dev, "OP-TEE api uid mismatch\n");
> @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
>          * in U-Boot, the discovery of TA on the TEE bus is not supported:
>          * only bind the drivers associated to the supported OP-TEE TA
>          */
> -       if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
> -               ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
> +
> +       for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
> +               ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
> +                                        optee_bus_probe[i].dev_name, NULL);
>                 if (ret)
> -                       return ret;
> +                       dev_warn(dev, "Failed to bind device %s\n",
> +                                optee_bus_probe[i].dev_name);

Please add device tree nodes for these and all this code can go away.

Regards,
Simon

Re: [PATCH v4] tee: optee: rework TA bus scanning code

[Patrick DELAUNAY]

Hi,

On 9/6/22 11:37, Ilias Apalodimas wrote:
> Late versions of OP-TEE support a pseudo bus.  TAs that behave as
> hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
> scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
> that we already have a workaround for RNG.  The details are in
> commit 70812bb83da6 ("tee: optee: bind rng optee driver")
>
> So let's add a list of devices based on U-Boot Kconfig options that we will
> scan until we properly implement the tee-bus functionality.
>
> While at it change the behaviour of the tee core itself wrt to device
> binding.  If some device binding fails, print a warning instead of
> disabling OP-TEE.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v3:
> - Use NULL instead of a child ptr on device_bind_driver(), since it's not
> really needed
> - Changed the style of the optee_bus_probe[] definition to
>    {.drv_name = xxx, .dev_name = yyy }
>
> Changes since v2:
> - Fixed typo on driver name ftpm-tee -> ftpm_tee
>
> Changes since v1:
> - remove a macro and use ARRAY_SIZE directly
>   drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

[PATCH v4] tee: optee: rework TA bus scanning code

[Ilias Apalodimas]

Late versions of OP-TEE support a pseudo bus.  TAs that behave as
hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can
scan.  Unfortunately U-Boot doesn't support that yet. It's worth noting
that we already have a workaround for RNG.  The details are in
commit 70812bb83da6 ("tee: optee: bind rng optee driver")

So let's add a list of devices based on U-Boot Kconfig options that we will
scan until we properly implement the tee-bus functionality.

While at it change the behaviour of the tee core itself wrt to device
binding.  If some device binding fails, print a warning instead of
disabling OP-TEE.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v3:
- Use NULL instead of a child ptr on device_bind_driver(), since it's not
really needed
- Changed the style of the optee_bus_probe[] definition to
  {.drv_name = xxx, .dev_name = yyy }

Changes since v2:
- Fixed typo on driver name ftpm-tee -> ftpm_tee

Changes since v1:
- remove a macro and use ARRAY_SIZE directly
 drivers/tee/optee/core.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index a89d62aaf0b3..c201a4635e6b 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -31,6 +31,18 @@ struct optee_pdata {
 	optee_invoke_fn *invoke_fn;
 };
 
+static const struct {
+	const char *drv_name;
+	const char *dev_name;
+} optee_bus_probe[] = {
+#ifdef CONFIG_RNG_OPTEE
+	{ .drv_name = "optee-rng", .dev_name = "optee-rng" },
+#endif
+#ifdef CONFIG_TPM2_FTPM_TEE
+	{ .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" },
+#endif
+};
+
 struct rpc_param {
 	u32	a0;
 	u32	a1;
@@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev)
 {
 	struct optee_pdata *pdata = dev_get_plat(dev);
 	u32 sec_caps;
-	struct udevice *child;
-	int ret;
+	int ret, i;
 
 	if (!is_optee_api(pdata->invoke_fn)) {
 		dev_err(dev, "OP-TEE api uid mismatch\n");
@@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev)
 	 * in U-Boot, the discovery of TA on the TEE bus is not supported:
 	 * only bind the drivers associated to the supported OP-TEE TA
 	 */
-	if (IS_ENABLED(CONFIG_RNG_OPTEE)) {
-		ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
+
+	for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) {
+		ret = device_bind_driver(dev, optee_bus_probe[i].drv_name,
+					 optee_bus_probe[i].dev_name, NULL);
 		if (ret)
-			return ret;
+			dev_warn(dev, "Failed to bind device %s\n",
+				 optee_bus_probe[i].dev_name);
 	}
 
 	return 0;
-- 
2.37.2