[PATCH v4] tee: optee: rework TA bus scanning code
Patrick DELAUNAY
patrick.delaunay at foss.st.com
Thu Sep 22 20:15:37 CEST 2022
Hi,
On 9/22/22 13:27, Simon Glass wrote:
> Hi Etienne,
>
> On Thu, 22 Sept 2022 at 10:52, Etienne Carriere
> <etienne.carriere at linaro.org> wrote:
>> Hello Patrick and all,
>>
>> On Mon, 19 Sept 2022 at 16:49, Patrick DELAUNAY
>> <patrick.delaunay at 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 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.
>>>>
>> 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
More information about the U-Boot
mailing list