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

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

[Patrick Delaunay]

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

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 arch/arm/mach-rockchip/sdram.c |  2 +-
 common/spl/Kconfig             |  6 +++---
 common/spl/Makefile            |  2 +-
 common/spl/spl.c               |  2 +-
 configs/evb-rk3229_defconfig   |  2 +-
 configs/evb-rk3288_defconfig   |  2 +-
 configs/odroid-go2_defconfig   |  2 +-
 include/configs/mx7_common.h   |  2 +-
 include/tee/optee.h            |  6 +++---
 lib/Makefile                   |  2 +-
 lib/optee/Kconfig              | 24 ++++++++++++++----------
 lib/optee/Makefile             |  2 +-
 lib/optee/optee.c              |  2 ++
 13 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c
index 28c379ef07..705ec7ba64 100644
--- a/arch/arm/mach-rockchip/sdram.c
+++ b/arch/arm/mach-rockchip/sdram.c
@@ -45,7 +45,7 @@ int dram_init_banksize(void)
 	gd->bd->bi_dram[0].start = 0x200000;
 	gd->bd->bi_dram[0].size = top - gd->bd->bi_dram[0].start;
 #else
-#ifdef CONFIG_SPL_OPTEE
+#ifdef CONFIG_SPL_OPTEE_IMAGE
 	struct tos_parameter_t *tos_parameter;
 
 	tos_parameter = (struct tos_parameter_t *)(CONFIG_SYS_SDRAM_BASE +
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index c155a3b5fc..cbf32badd3 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -1333,11 +1333,11 @@ config SPL_AM33XX_ENABLE_RTC32K_OSC
 	  Enable access to the AM33xx RTC and select the external 32kHz clock
 	  source.
 
-config SPL_OPTEE
-	bool "Support OP-TEE Trusted OS"
+config SPL_OPTEE_IMAGE
+	bool "Support OP-TEE Trusted OS image in SPL"
 	depends on ARM
 	help
-	  OP-TEE is an open source Trusted OS  which is loaded by SPL.
+	  OP-TEE is an open source Trusted OS which is loaded by SPL.
 	  More detail at: https://github.com/OP-TEE/optee_os
 
 config SPL_OPENSBI
diff --git a/common/spl/Makefile b/common/spl/Makefile
index c576a78126..b20f6fbe1a 100644
--- a/common/spl/Makefile
+++ b/common/spl/Makefile
@@ -22,7 +22,7 @@ obj-$(CONFIG_$(SPL_TPL_)UBI) += spl_ubi.o
 obj-$(CONFIG_$(SPL_TPL_)NET_SUPPORT) += spl_net.o
 obj-$(CONFIG_$(SPL_TPL_)MMC_SUPPORT) += spl_mmc.o
 obj-$(CONFIG_$(SPL_TPL_)ATF) += spl_atf.o
-obj-$(CONFIG_$(SPL_TPL_)OPTEE) += spl_optee.o
+obj-$(CONFIG_$(SPL_TPL_)OPTEE_IMAGE) += spl_optee.o
 obj-$(CONFIG_$(SPL_TPL_)OPENSBI) += spl_opensbi.o
 obj-$(CONFIG_$(SPL_TPL_)USB_STORAGE) += spl_usb.o
 obj-$(CONFIG_$(SPL_TPL_)FS_FAT) += spl_fat.o
diff --git a/common/spl/spl.c b/common/spl/spl.c
index d55d3c2848..4439cde1ee 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -773,7 +773,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 		spl_invoke_atf(&spl_image);
 		break;
 #endif
-#if CONFIG_IS_ENABLED(OPTEE)
+#if CONFIG_IS_ENABLED(OPTEE_IMAGE)
 	case IH_OS_TEE:
 		debug("Jumping to U-Boot via OP-TEE\n");
 		spl_board_prepare_for_optee(spl_image.fdt_addr);
diff --git a/configs/evb-rk3229_defconfig b/configs/evb-rk3229_defconfig
index 02e19fa10c..f83041c768 100644
--- a/configs/evb-rk3229_defconfig
+++ b/configs/evb-rk3229_defconfig
@@ -24,7 +24,7 @@ CONFIG_DISPLAY_BOARDINFO_LATE=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000
-CONFIG_SPL_OPTEE=y
+CONFIG_SPL_OPTEE_IMAGE=y
 CONFIG_CMD_GPT=y
 CONFIG_CMD_MMC=y
 CONFIG_CMD_USB_MASS_STORAGE=y
diff --git a/configs/evb-rk3288_defconfig b/configs/evb-rk3288_defconfig
index 658ddc9750..c4990a3070 100644
--- a/configs/evb-rk3288_defconfig
+++ b/configs/evb-rk3288_defconfig
@@ -22,7 +22,7 @@ CONFIG_SILENT_CONSOLE=y
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
-CONFIG_SPL_OPTEE=y
+CONFIG_SPL_OPTEE_IMAGE=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
 CONFIG_CMD_I2C=y
diff --git a/configs/odroid-go2_defconfig b/configs/odroid-go2_defconfig
index aafec84f10..551f95f4f9 100644
--- a/configs/odroid-go2_defconfig
+++ b/configs/odroid-go2_defconfig
@@ -95,7 +95,7 @@ CONFIG_DEBUG_UART_SHIFT=2
 CONFIG_DEBUG_UART_SKIP_INIT=y
 CONFIG_SOUND=y
 CONFIG_SYSRESET=y
-CONFIG_OPTEE=y
+CONFIG_OPTEE_LIB=y
 CONFIG_DM_THERMAL=y
 CONFIG_USB=y
 CONFIG_USB_EHCI_HCD=y
diff --git a/include/configs/mx7_common.h b/include/configs/mx7_common.h
index 3d87690382..629b2c7c52 100644
--- a/include/configs/mx7_common.h
+++ b/include/configs/mx7_common.h
@@ -50,7 +50,7 @@
  * initialization since it was already done by ATF or OPTEE
  */
 #if (CONFIG_OPTEE_TZDRAM_SIZE != 0)
-#ifndef CONFIG_OPTEE
+#ifndef CONFIG_OPTEE_IMAGE
 #define CONFIG_SKIP_LOWLEVEL_INIT
 #endif
 #endif
diff --git a/include/tee/optee.h b/include/tee/optee.h
index ebdfe5e98d..2928597b61 100644
--- a/include/tee/optee.h
+++ b/include/tee/optee.h
@@ -43,7 +43,7 @@ optee_image_get_load_addr(const struct image_header *hdr)
 	return optee_image_get_entry_point(hdr) - sizeof(struct optee_header);
 }
 
-#if defined(CONFIG_OPTEE)
+#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);
 #else
@@ -57,7 +57,7 @@ static inline int optee_verify_image(struct optee_header *hdr,
 
 #endif
 
-#if defined(CONFIG_OPTEE)
+#if defined(CONFIG_OPTEE_IMAGE)
 int optee_verify_bootm_image(unsigned long image_addr,
 			     unsigned long image_load_addr,
 			     unsigned long image_len);
@@ -70,7 +70,7 @@ static inline int optee_verify_bootm_image(unsigned long image_addr,
 }
 #endif
 
-#if defined(CONFIG_OPTEE) && defined(CONFIG_OF_LIBFDT)
+#if defined(CONFIG_OPTEE_LIB) && defined(CONFIG_OF_LIBFDT)
 int optee_copy_fdt_nodes(void *new_blob);
 #else
 static inline int optee_copy_fdt_nodes(void *new_blob)
diff --git a/lib/Makefile b/lib/Makefile
index 8ba745faa0..47c5ac19cb 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -16,7 +16,7 @@ obj-$(CONFIG_FIT) += libfdt/
 obj-$(CONFIG_OF_LIVE) += of_live.o
 obj-$(CONFIG_CMD_DHRYSTONE) += dhry/
 obj-$(CONFIG_ARCH_AT91) += at91/
-obj-$(CONFIG_OPTEE) += optee/
+obj-$(CONFIG_OPTEE_LIB) += optee/
 obj-$(CONFIG_ASN1_DECODER) += asn1_decoder.o
 obj-y += crypto/
 
diff --git a/lib/optee/Kconfig b/lib/optee/Kconfig
index c398f9b953..e2a0541537 100644
--- a/lib/optee/Kconfig
+++ b/lib/optee/Kconfig
@@ -1,23 +1,27 @@
-config OPTEE
+config OPTEE_LIB
+	bool "Support OPTEE library"
+	default y if OPTEE || OPTEE_IMAGE
+	help
+	  Selecting this option will enable the shared OPTEE library code.
+
+config OPTEE_IMAGE
 	bool "Support OPTEE images"
 	help
-	  U-Boot can be configured to boot OPTEE images.
-	  Selecting this option will enable shared OPTEE library code and
-          enable an OPTEE specific bootm command that will perform additional
-          OPTEE specific checks before booting an OPTEE image created with
-          mkimage.
+	  Selecting this option to boot OPTEE images.
+	  This option enable the OPTEE specific checks done before booting
+	  an OPTEE image created with mkimage
 
 config OPTEE_LOAD_ADDR
 	hex "OPTEE load address"
 	default 0x00000000
-	depends on OPTEE
+	depends on OPTEE_LIB
 	help
 	  The load address of the bootable OPTEE binary.
 
 config OPTEE_TZDRAM_SIZE
 	hex "Amount of Trust-Zone RAM for the OPTEE image"
 	default 0x0000000
-	depends on OPTEE
+	depends on OPTEE_LIB
 	help
 	  The size of pre-allocated Trust Zone DRAM to allocate for the OPTEE
 	  runtime.
@@ -25,7 +29,7 @@ config OPTEE_TZDRAM_SIZE
 config OPTEE_TZDRAM_BASE
 	hex "Base address of Trust-Zone RAM for the OPTEE image"
 	default 0x00000000
-	depends on OPTEE
+	depends on OPTEE_LIB
 	help
 	  The base address of pre-allocated Trust Zone DRAM for
 	  the OPTEE runtime.
@@ -33,7 +37,7 @@ config OPTEE_TZDRAM_BASE
 config BOOTM_OPTEE
 	bool "Support OPTEE bootm command"
 	select BOOTM_LINUX
-	depends on OPTEE
+	select OPTEE_IMAGE
 	default n
 	help
 	  Select this command to enable chain-loading of a Linux kernel
diff --git a/lib/optee/Makefile b/lib/optee/Makefile
index b692311864..9befe606d8 100644
--- a/lib/optee/Makefile
+++ b/lib/optee/Makefile
@@ -2,4 +2,4 @@
 #
 # (C) Copyright 2017 Linaro
 
-obj-$(CONFIG_OPTEE) += optee.o
+obj-y += optee.o
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
 
 #if defined(CONFIG_OF_LIBFDT)
 static int optee_copy_firmware_node(ofnode node, void *fdt_blob)
-- 
2.25.1

[PATCH 2/2] tee: add a stub for tee_find_device

[Patrick Delaunay]

Add stub for tee_find_device function when CONFIG_TEE is not activated
to simplify the caller code.

This patch allows to remove the CONFIG_IS_ENABLED(OPTEE) tests
for stm32 platform.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 arch/arm/mach-stm32mp/fdt.c        |  1 -
 board/st/common/stm32mp_mtdparts.c |  3 +--
 include/tee.h                      | 11 +++++++++++
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-stm32mp/fdt.c b/arch/arm/mach-stm32mp/fdt.c
index a19e954cf7..91330a68a4 100644
--- a/arch/arm/mach-stm32mp/fdt.c
+++ b/arch/arm/mach-stm32mp/fdt.c
@@ -341,7 +341,6 @@ int ft_system_setup(void *blob, struct bd_info *bd)
 	 * when FIP is not used by TF-A
 	 */
 	if (CONFIG_IS_ENABLED(STM32MP15x_STM32IMAGE) &&
-	    CONFIG_IS_ENABLED(OPTEE) &&
 	    !tee_find_device(NULL, NULL, NULL, NULL))
 		stm32_fdt_disable_optee(blob);
 
diff --git a/board/st/common/stm32mp_mtdparts.c b/board/st/common/stm32mp_mtdparts.c
index 8b636d62fa..18878424c7 100644
--- a/board/st/common/stm32mp_mtdparts.c
+++ b/board/st/common/stm32mp_mtdparts.c
@@ -119,8 +119,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
 	}
 
 #ifdef CONFIG_STM32MP15x_STM32IMAGE
-	if (!serial && CONFIG_IS_ENABLED(OPTEE) &&
-	    tee_find_device(NULL, NULL, NULL, NULL))
+	if (!serial && tee_find_device(NULL, NULL, NULL, NULL))
 		tee = true;
 #endif
 
diff --git a/include/tee.h b/include/tee.h
index 2ef29bfc8f..44e9cd4321 100644
--- a/include/tee.h
+++ b/include/tee.h
@@ -307,11 +307,22 @@ bool tee_shm_is_registered(struct tee_shm *shm, struct udevice *dev);
  * Returns a probed TEE device of the first TEE device matched by the
  * match() callback or NULL.
  */
+#if CONFIG_IS_ENABLED(TEE)
 struct udevice *tee_find_device(struct udevice *start,
 				int (*match)(struct tee_version_data *vers,
 					     const void *data),
 				const void *data,
 				struct tee_version_data *vers);
+#else
+static inline struct udevice *tee_find_device(struct udevice *start,
+					      int (*match)(struct tee_version_data *vers,
+							   const void *data),
+					      const void *data,
+					      struct tee_version_data *vers)
+{
+	return NULL;
+}
+#endif
 
 /**
  * tee_get_version() - Query capabilities of TEE device
-- 
2.25.1

Re: [PATCH 2/2] tee: add a stub for tee_find_device

[Etienne Carriere]

Hello Patrick,

On Thu, 2 Sept 2021 at 11:56, Patrick Delaunay <patrick.delaunay@foss.st.com>
wrote:

> Add stub for tee_find_device function when CONFIG_TEE is not activated
> to simplify the caller code.
>
> This patch allows to remove the CONFIG_IS_ENABLED(OPTEE) tests
> for stm32 platform.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
>  arch/arm/mach-stm32mp/fdt.c        |  1 -
>  board/st/common/stm32mp_mtdparts.c |  3 +--
>  include/tee.h                      | 11 +++++++++++
>  3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/fdt.c b/arch/arm/mach-stm32mp/fdt.c
> index a19e954cf7..91330a68a4 100644
> --- a/arch/arm/mach-stm32mp/fdt.c
> +++ b/arch/arm/mach-stm32mp/fdt.c
> @@ -341,7 +341,6 @@ int ft_system_setup(void *blob, struct bd_info *bd)
>          * when FIP is not used by TF-A
>          */
>         if (CONFIG_IS_ENABLED(STM32MP15x_STM32IMAGE) &&
> -           CONFIG_IS_ENABLED(OPTEE) &&
>             !tee_find_device(NULL, NULL, NULL, NULL))
>                 stm32_fdt_disable_optee(blob);
>
> diff --git a/board/st/common/stm32mp_mtdparts.c
> b/board/st/common/stm32mp_mtdparts.c
> index 8b636d62fa..18878424c7 100644
> --- a/board/st/common/stm32mp_mtdparts.c
> +++ b/board/st/common/stm32mp_mtdparts.c
> @@ -119,8 +119,7 @@ void board_mtdparts_default(const char **mtdids, const
> char **mtdparts)
>         }
>
>  #ifdef CONFIG_STM32MP15x_STM32IMAGE
> -       if (!serial && CONFIG_IS_ENABLED(OPTEE) &&
> -           tee_find_device(NULL, NULL, NULL, NULL))
> +       if (!serial && tee_find_device(NULL, NULL, NULL, NULL))
>                 tee = true;
>  #endif
>
> diff --git a/include/tee.h b/include/tee.h
> index 2ef29bfc8f..44e9cd4321 100644
> --- a/include/tee.h
> +++ b/include/tee.h
> @@ -307,11 +307,22 @@ bool tee_shm_is_registered(struct tee_shm *shm,
> struct udevice *dev);
>   * Returns a probed TEE device of the first TEE device matched by the
>   * match() callback or NULL.
>   */
> +#if CONFIG_IS_ENABLED(TEE)
>  struct udevice *tee_find_device(struct udevice *start,
>                                 int (*match)(struct tee_version_data *vers,
>                                              const void *data),
>                                 const void *data,
>                                 struct tee_version_data *vers);
> +#else
> +static inline struct udevice *tee_find_device(struct udevice *start,
> +                                             int (*match)(struct
> tee_version_data *vers,
> +                                                          const void
> *data),
> +                                             const void *data,
> +                                             struct tee_version_data
> *vers)
> +{
> +       return NULL;
> +}
> +#endif
>
>  /**
>   * tee_get_version() - Query capabilities of TEE device
> --
> 2.25.1
>
>
Acked-by: Etienne Carriere <etienne.carriere@inaro.org>

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

[Alex G.]

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

Re: [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@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

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

[Patrick DELAUNAY]

Hi,

On 9/4/21 10:31 AM, Etienne Carriere wrote:
> Hello Alex and Patrick,
>
> (my apologies for my previous malformed post)
>
>
> On Fri, 3 Sept 2021 at 18:43, Alex G. <mr.nuke.me@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".

OP-TEE driver => communication with already loaded OP-TEE based on TEE 
u-class

OP-TEE library => library for U-Boot helper function to use with OP-TEE 
support , which allows

   - to load OP-TEE image (in U-Boot proper / bootm command)

   - to update Linux DT tree with OP-TEE nodes


so OP-TEE ibrary (for bootm support) can be activated without OP-TEE 
driver support....

even it is strange (for me also)


The OP-TEE library is not used by OP-TEE driver

but I agree, it is odds to don't have optee_copy_firmware_node() not 
depending of CONFIG_OPTEE


but I found at least one board wich enable the OP-TEE library without 
CONFIG_TEE  and so without OP-TEE driver

=> configs/odroid-go2_defconfig


you can see this patchset as a first step of dependancy cleanup...


for me it was the more simple migration path to avoid issue with 
existing code

and more easy to review.


At my first try, I don't create CONFIG_OPTEE_LIB and CONFIG_OPTEE_IMAGE

but when I compile all the boards to check if the u-boot and SPL size change

(to be sure that this migration don't break any boards)

I have several issue in particular with the

include/configs/mx7_common.h

index 3d87690382..629b2c7c52 100644
   * initialization since it was already done by ATF or OPTEE
   */
  #if (CONFIG_OPTEE_TZDRAM_SIZE != 0)
#ifndef CONFIG_OPTEE
  #define CONFIG_SKIP_LOWLEVEL_INIT
  #endif
  #endif

=> CONFIG_OPTEE_TZDRAM_SIZE can be defined and CONFIG_OPTEE not defined !?


I separate CONFIG_OPTEE_LIB and CONFIG_OPTEE_IMAGE  to solve this issue....


a other issue can be see in

------------------------- configs/odroid-go2_defconfig 
-------------------------
index aafec84f10..551f95f4f9 100644
@@ -95,7 +95,7 @@ CONFIG_DEBUG_UART_SHIFT=2
  CONFIG_DEBUG_UART_SKIP_INIT=y
  CONFIG_SOUND=y
  CONFIG_SYSRESET=y
-CONFIG_OPTEE=y
+CONFIG_OPTEE_LIB=y

=> if I activate CONFIG_OPTEE_IMAGE instead of CONFIG_OPTEE_LIB the 
board size increase.


>> Introducing OPTEE_LIB then, makes sense to me, provided that OPTEE
>> depends on OPTEE_LIB, but I'm not sure about OPTEE_IMAGE.


this config OPTEE_IMAGE / OPTEE_LIB also allows to don't break the 
existing dependency for

- OPTEE_LOAD_ADDR

- OPTEE_TZDRAM_SIZE

- OPTEE_TZDRAM_BASE


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


I agree for me, the next step of the migration could be to split the 
optee lib to 2 files

lib/optee/optee_image.c

lib/optee/optee_fdt.c


with

lib/Makefile:

obj-$(CONFIG_OPTEE_LIB) += optee/


lib/optee/Makefile:
-obj-y += optee.o

+obj-$(CONFIG_OPTEE_IMAGE) += optee_image.o

+obj-$(CONFIG_OF_LIBFDT) += optee_fdt.o

=> I don't to it yet to after more easy review of the current patch....

        but if you prefer, I can do it in v2


or the optee_copy_fdt_nodes() should be copied in drivers/tee/optee.c

(but this modification required to have the driver activated in all 
boards expecting this feature)


PS: for SPL support, in future, we could manage SPL support for each 
option:

obj-$(CONFIG_$(SPL_)OPTEE_LIB) += optee/

obj-$(CONFIG_$(SPL_)OPTEE_IMAGE) += optee_image.o

obj-$(CONFIG_$(SPL_)OF_LIBFDT) += optee_fdt.o


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


it is not possible, it is used for U-Boot proper on other platforms

configs/warp7_bl33_defconfig:70:CONFIG_OPTEE_TZDRAM_BASE=0x9e000000
configs/warp7_defconfig:76:CONFIG_OPTEE_TZDRAM_BASE=0x9d000000

board/warp7/warp7.c:37:#ifdef CONFIG_OPTEE_TZDRAM_SIZE
board/warp7/warp7.c:38:        gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE;
board/warp7/warp7.c:118:#ifdef CONFIG_OPTEE_TZDRAM_SIZE
board/warp7/warp7.c:122:    optee_start = optee_end - 
CONFIG_OPTEE_TZDRAM_SIZE;
board/technexion/pico-imx7d/pico-imx7d.c:55:#ifdef CONFIG_OPTEE_TZDRAM_SIZE
board/technexion/pico-imx7d/pico-imx7d.c:56: gd->ram_size -= 
CONFIG_OPTEE_TZDRAM_SIZE;
configs/warp7_bl33_defconfig:69:CONFIG_OPTEE_TZDRAM_SIZE=0x02000000
configs/warp7_defconfig:75:CONFIG_OPTEE_TZDRAM_SIZE=0x3000000
include/configs/mx7_common.h:52:#if (CONFIG_OPTEE_TZDRAM_SIZE != 0)


And for me this configuration (size of memory used by OPTEE) is more a 
system configuration

depending of the OP-TEE firmware used than a Device Tree configuration 
at SPL level


=> if the OP-TEE size change for 2 FW (OP-TEE with or wihout secure UI 
for example)

       on the same board, the the SPL device tree should not change....

       (the kernel device tree can be hardcoded or updated by OP-TEE)


PS: for the TF-A case it is done in a secure FW configuration file => in 
the FIP

       this information is no hardcoded information in BL2


     in SPL, the load address / entry point it is already provided by 
FIT for OPTEE image

      (=> optee_image_get_load_addr / optee_image_get_entry_point)

      no need to have this information in DT (optee base address)

tools/default_image.c:119

     if (params->os == IH_OS_TEE) {
         addr = optee_image_get_load_addr(hdr);
         ep = optee_image_get_entry_point(hdr);

     }


     for CONFIG_OPTEE_TZDRAM_SIZE, I think that can be also found by 
parsing the OP-TEE header

=> see : init_mem_usage

     the OPTEE should be access to this memory .....

     and it can change the firewall configuration is it is necessary

     for the shared memory for example


=> no need to update first stage boot loader = SPL (with the risk to 
brick the device)

      when only OP-TEE firmware change


> regards,
> etienne
>
>>     - Remove optee_verify_bootm_image()

but it is used in

common/bootm_os.c:491:    ret = 
optee_verify_bootm_image(images->os.image_start,


for the use case OPTEE loaded and started by U-Boot , with bootm command 
= CONFIG_OPTEE_IMAGE

(not supported by stm32mp15)


>>
>>     - No need for CONFIG_OPTEE_IMAGE
>>
activate by CONFIG_BOOTM_OPTEE

activated in

configs/warp7_defconfig:77:CONFIG_BOOTM_OPTEE=y


and this option can be used to correctly handle the macro 
CONFIG_IS_ENABLED(OPTEE_IMAGE) as it is expected

=> in U-Boot proper : test  CONFIG_OPTEE_IMAGE

=> in SPL : test CONFIG_SPL_OPTEE_IMAGE (existing)

as it is almost the same meaning..... support of OP-TEE image load


And this marco is already used in SPL context

common/spl/spl.c:776:#if CONFIG_IS_ENABLED(OPTEE)


>>
>> Alex


Patrick

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

[Alex G.]

On 9/6/21 11:53 AM, Patrick DELAUNAY wrote:
>>
>>> 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.
> 
> 
> it is not possible, it is used for U-Boot proper on other platforms
> 
> board/warp7/warp7.c:38:        gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE;
> board/warp7/warp7.c:122:    optee_start = optee_end - CONFIG_OPTEE_TZDRAM_SIZE;
> board/technexion/pico-imx7d/pico-imx7d.c:56: gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE;
> include/configs/mx7_common.h:52:#if (CONFIG_OPTEE_TZDRAM_SIZE != 0)

I have an idea how to work around that.


> And for me this configuration (size of memory used by OPTEE) is more a 
> system configuration
> depending of the OP-TEE firmware used than a Device Tree configuration 
> at SPL level
> 
> PS: for the TF-A case it is done in a secure FW configuration file => in 
> the FIP
>        this information is no hardcoded information in BL2
>      in SPL, the load address / entry point it is already provided by 
> FIT for OPTEE image
> 
>       (=> optee_image_get_load_addr / optee_image_get_entry_point)
>       no need to have this information in DT (optee base address)
> 
> tools/default_image.c:119
> 
>      if (params->os == IH_OS_TEE) {
>          addr = optee_image_get_load_addr(hdr);
>          ep = optee_image_get_entry_point(hdr);
> 
>      }

The OPTEE entry point is available:
1) in both FIT and uImage files.
2) As the optee reserved-memory node in DT
3) Via CONFIG_OPTEE_TZDRAM_BASE

On the one hand, (1) and (2) together could hint that the OPTEE image is 
incompatible with the board, so they are not completely redundant.
On the other hand, there is no point in (3) given that the information 
could be obtained in at least two other ways.


> 
>      for CONFIG_OPTEE_TZDRAM_SIZE, I think that can be also found by 
> parsing the OP-TEE header
> 
> => see : init_mem_usage
> 
>      the OPTEE should be access to this memory .....
>      and it can change the firewall configuration is it is necessary
>      for the shared memory for example
> 
> 
> => no need to update first stage boot loader = SPL (with the risk to 
> brick the device)
>       when only OP-TEE firmware change

I see your point. It's a packaging issue, which we could solve with FIT, 
but not with uImage. Though, how often does an OP-TEE update change the 
TZDRAM location?


>>>     - Remove optee_verify_bootm_image()
> 
> but it is used in
> 
> common/bootm_os.c:491:    ret = 
> optee_verify_boot_image(images->os.image_start,

Yes. It only checks if the OP-TEE image fits within some hardcoded, and 
potentially wrong, boundaries. Which is contrary to your arguments from 
a few paragraphs ago. Just don't call optee_verify_boot_image in bootm_os.c.

Alex

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

[Alex G.]

On 9/6/21 5:39 PM, Alex G. wrote:
> 
> 
> On 9/6/21 11:53 AM, Patrick DELAUNAY wrote:
>>>
>>>> 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.
>>
>>
>> it is not possible, it is used for U-Boot proper on other platforms
>>
>> board/warp7/warp7.c:38:        gd->ram_size -= CONFIG_OPTEE_TZDRAM_SIZE;
>> board/warp7/warp7.c:122:    optee_start = optee_end - 
>> CONFIG_OPTEE_TZDRAM_SIZE;
>> board/technexion/pico-imx7d/pico-imx7d.c:56: gd->ram_size -= 
>> CONFIG_OPTEE_TZDRAM_SIZE;
>> include/configs/mx7_common.h:52:#if (CONFIG_OPTEE_TZDRAM_SIZE != 0)
> 
> I have an idea how to work around that.

This is what I had in mind [1]. With this, optee_verify_bootm_image() is 
just three or four checks.

I propose that code which would have been under CONFIG_OPTEE_IMAGE, is 
instead moved under CONFIG_BOOTM_OPTEE. Thus there's no need for 
CONFIG_OPTEE_IMAGE in the first place, and all the other changes make sense.

Alex


[1] https://lists.denx.de/pipermail/u-boot/2021-September/459981.html

Re: [PATCH 2/2] tee: add a stub for tee_find_device

[Jens Wiklander]

Hi Patrick,

On Thu, Sep 2, 2021 at 11:56 AM Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
> Add stub for tee_find_device function when CONFIG_TEE is not activated
> to simplify the caller code.
>
> This patch allows to remove the CONFIG_IS_ENABLED(OPTEE) tests
> for stm32 platform.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
>  arch/arm/mach-stm32mp/fdt.c        |  1 -
>  board/st/common/stm32mp_mtdparts.c |  3 +--
>  include/tee.h                      | 11 +++++++++++
>  3 files changed, 12 insertions(+), 3 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

On Thu, Sep 02, 2021 at 11:56:16AM +0200, 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
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] tee: add a stub for tee_find_device

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Thu, Sep 02, 2021 at 11:56:17AM +0200, Patrick Delaunay wrote:

> Add stub for tee_find_device function when CONFIG_TEE is not activated
> to simplify the caller code.
> 
> This patch allows to remove the CONFIG_IS_ENABLED(OPTEE) tests
> for stm32 platform.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Acked-by: Etienne Carriere <etienne.carriere@inaro.org>
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]