[PATCH 1/2] lib: optee: remove the duplicate CONFIG_OPTEE

[Etienne Carriere]

Hello Alex and Patrick,

(my apologies for my previous malformed post)


On Fri, 3 Sept 2021 at 18:43, Alex G. <mr.nuke.me at gmail.com> wrote:
>
> Hi Patrick
>
> On 9/2/21 4:56 AM, Patrick Delaunay wrote:
> > The configuration CONFIG_OPTEE is defined 2 times:
> > 1- in lib/optee/Kconfig for support of OPTEE images loaded by bootm command
> > 2- in drivers/tee/optee/Kconfig for support of OP-TEE driver.
> >
> > It is abnormal to have the same CONFIG define for 2 purpose;
> > and it is difficult to managed correctly their dependencies.

+1

> >
> > Moreover CONFIG_SPL_OPTEE is defined in common/spl/Kconfig
> > to manage OPTEE image load in SPL.
> >
> > This definition causes an issue with the macro CONFIG_IS_ENABLED(OPTEE)
> > to test the availability of the OP-TEE driver.
> >
> > This patch cleans the configuration dependency with:
> > - CONFIG_OPTEE_IMAGE (renamed) => support of OP-TEE image in U-Boot
> > - CONFIG_SPL_OPTEE_IMAGE (renamed) => support of OP-TEE image in SPL
> > - CONFIG_OPTEE (same) => support of OP-TEE driver in U-Boot
> > - CONFIG_OPTEE_LIB (new) => support of OP-TEE library
> >
> > After this patch, the macro have the correct behavior:
> > - CONFIG_IS_ENABLED(OPTEE_IMAGE) => Load of OP-TEE image is supported
> > - CONFIG_IS_ENABLED(OPTEE) => OP-TEE driver is supported
>
> It seems a little odd to have both OPTEE_LIB and OPTEE_IMAGE, since they
> are both used together to support booting with OP-TEE. What also seems
> odd is that "OP-TEE driver in U-Boot" does not depend on "OP-TEE library".
>
> Introducing OPTEE_LIB then, makes sense to me, provided that OPTEE
> depends on OPTEE_LIB, but I'm not sure about OPTEE_IMAGE.
>
> > diff --git a/lib/optee/optee.c b/lib/optee/optee.c
> > index 672690dc53..5676785cb5 100644
> > --- a/lib/optee/optee.c
> > +++ b/lib/optee/optee.c
> > @@ -20,6 +20,7 @@
> >       "\n\theader lo=0x%08x hi=0x%08x size=0x%08lx arch=0x%08x" \
> >       "\n\tuimage params 0x%08lx-0x%08lx\n"
> >
> > +#if defined(CONFIG_OPTEE_IMAGE)
> >   int optee_verify_image(struct optee_header *hdr, unsigned long tzdram_start,
> >                      unsigned long tzdram_len, unsigned long image_len)
> >   {
> > @@ -70,6 +71,7 @@ error:
> >
> >       return ret;
> >   }
> > +#endif
>
> One the idea of having CONFIGs is to include/exclude code via
> obj-$(CONFIG_FOO)+=code.c. This prevents the proliferation of #ifdefs.
> It's fairly counterintuitive to have a CONFIG_OPTEE_IMAGE in a file
> controlled by CONFIG_OPTEE_LIB.
>
> Going to optee_verify_image() itself. It essentially checks against
> OPTEE_TZDRAM_(BASE/SIZE). But those are a derived from devicetree, not
> Kconfig. So it seems the motivation behing optee_verify_bootm_image() is
> flawed. Also the error message is not very helpful.

The 2 functions are related to CONFIG_BOOTM_OPTEE. They could depend on.
My 2 cents.
If preserving the optee_verify_xxx() functions, they could move to a
specific source lib/optee/optee_image.c

>
> In fact, the SPL boot path for OP-TEE doesn't use this function. That's
> intentional.
>
> Here's what I suggest:
>    - Remove OPTEE_TZDRAM_BASE and _SIZE

There is some legacy here, board/warp7and board/technexion/pico-imx7d.

regards,
etienne

>
>    - Remove optee_verify_bootm_image()
>
>    - No need for CONFIG_OPTEE_IMAGE
>
>
>
> Alex