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@lin...
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