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

["Alex G." <mr.nuke.me@gmail.com>]

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

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
   - Remove optee_verify_bootm_image()
   - No need for CONFIG_OPTEE_IMAGE


Alex