[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 at linaro.org> wrote:
>> Hi Simon,
>>
>> On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Ilias,
>>>
>>> On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas
>>> <ilias.apalodimas at 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 at 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 at linaro.org>
>>>>>> Reviewed-by: Jens Wiklander <jens.wiklander at linaro.org>
>>>>>> Reviewed-by: Etienne Carriere <etienne.carriere at 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