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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Etienne,

[...]

> > > +
> > > +#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)?

That should be a user selected option.  If the dt entry is there we
should scan it as we do today.  However if the DT entry is not there I
believe we should try to scan the device from the tree bus.

>
> 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

Yea I know there's  a PR fixing that but was posted on the initial
fTPM project  [1].  We need to refresh that

[1] https://github.com/microsoft/MSRSec/pull/34

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

Yea don't add the ftpm now.  I only wanted to convert this to an
array, so we plug in new devices easier in the future.

Cheers
/Ilias
>
> Best regards,
> etienne
>
> >
> > >               if (ret)
> > >                       return ret;
> > >       }
> > > +#endif
> > [...]
> >
> >
> > Thanks!
> > /Ilias