Hello Alex and Patrick,
(my apologies for my previous malformed post)
On Fri, 3 Sept 2021 at 18:43, Alex G. <mr.nuke...@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
