[PATCH 1/2] drivers: tee: optee: discover OP-TEE services

[Etienne Carriere]

Hi Ilias,

On Mon, 6 Jun 2022 at 11:49, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Etienne,
>
> On Wed, Jun 01, 2022 at 10:27:51AM +0200, Etienne Carriere wrote:
> > This change defines resources for OP-TEE service drivers to register
> > themselves for being bound to when OP-TEE firmware reports the related
> > service is supported. OP-TEE services are discovered during optee
> > driver probe sequence. Discovery of optee services and binding to
> > related U-Boot drivers is embedded upon configuration switch
> > CONFIG_OPTEE_SERVICE_DISCOVERY.
> >
> > Cc: Jens Wiklander <jens.wiklander at linaro.org>
> > Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
> > ---
> >  drivers/tee/optee/Kconfig   |   8 ++
> >  drivers/tee/optee/core.c    | 187 +++++++++++++++++++++++++++++++++++-
> >  include/tee/optee_service.h |  29 ++++++
> >  3 files changed, 223 insertions(+), 1 deletion(-)
> >  create mode 100644 include/tee/optee_service.h
> >
> > diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> > index d03028070b..9dc65b0501 100644
> > --- a/drivers/tee/optee/Kconfig
> > +++ b/drivers/tee/optee/Kconfig
> > @@ -37,6 +37,14 @@ config OPTEE_TA_SCP03
> >
>
> [...]
>
> > +static int enum_services(struct udevice *dev, struct tee_shm **shm, size_t *count, u32 tee_sess)
> > +{
> > +     size_t shm_size = 0;
> > +     int ret;
> > +
> > +     ret = __enum_services(dev, NULL, &shm_size, tee_sess);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tee_shm_alloc(dev, shm_size, 0, shm);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to allocated shared memory: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = __enum_services(dev, *shm, &shm_size, tee_sess);
> > +     if (ret)
> > +             tee_shm_free(*shm);
>
> I'd prefer if we handled this a bit differently.  Instead of freeing the
> buffer here, just release it on bind_service_drivers() always

Ok, i'll change this in patch v3.

>
> > +     else
> > +             *count = shm_size / sizeof(struct tee_optee_ta_uuid);
> > +
> > +     return ret;
> > +}
> > +
> > +
> >  static int optee_probe(struct udevice *dev)
> >  {
> >       struct optee_pdata *pdata = dev_get_plat(dev);
> >       u32 sec_caps;
> > -     struct udevice *child;
> >       int ret;
> >
> >       if (!is_optee_api(pdata->invoke_fn)) {
> > @@ -668,15 +842,23 @@ static int optee_probe(struct udevice *dev)
> >               return -ENOENT;
> >       }
> >
> > +     ret = bind_service_drivers(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +#ifndef CONFIG_OPTEE_SERVICE_DISCOVERY
> >       /*
> >        * 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)) {
> > +             struct udevice *child;
> > +
> >               ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child);
>
> The same principle applies for fTPM.  Moreover the linux kernel supports
> bus scanning, which creates a conflict when the fTPM is added on the .dts
> (for u-boot to scan it).

Do you mean you would like fTPM driver to NOT be probed upon its
related DT compatible node and only probed from the fTPM TA discovery
(optee so-called devices enumeration)?

Another issue here is that current fTPM implementation [1] does not
set flag TA_FLAG_DEVICE_ENUM [2] that makes a built-in TA (so-called
early TA) to be enumerated by OP-TEE.

[1] https://github.com/microsoft/ms-tpm-20-ref/blob/d638536d0fe01acd5e39ffa1bd100b3da82d92c7/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47
[2] https://github.com/OP-TEE/optee_os/blob/3.17.0/lib/libutee/include/user_ta_header.h#L26-L32

>
> Can we make this a bit more generic, even though only the rng is added on
> this patch?
>
> something like
> struct devices {
>         const char *drv_name;
>         const char *dev_name;
> } tee_bus_devices = {
>         {
>                 "optee-rng",
>                 "optee-rng",
>         },
> }
> and add an array of the 'scanable' devices?  It would make adding the ftpm
> and other devices trivial

Assuming fTPM TA is enumerated, i don't think we need to add a device
name here. fTPM service could be proved straight based on the driver
name. fTPM driver in u-boot expects there is only 1 TEE firmware,
hence only 1 fTPM TA instance.

For info, i'll send a patch v3 without changes on fTPM.

Best regards,
etienne

>
> >               if (ret)
> >                       return ret;
> >       }
> > +#endif
> [...]
>
>
> Thanks!
> /Ilias