From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1B62DC433EF for ; Mon, 6 Sep 2021 16:54:33 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1468A60FBF for ; Mon, 6 Sep 2021 16:54:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1468A60FBF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=foss.st.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5859482CDE; Mon, 6 Sep 2021 18:54:29 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=foss.st.com header.i=@foss.st.com header.b="F2HikHJM"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 960B383200; Mon, 6 Sep 2021 18:54:26 +0200 (CEST) Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 1A76682C84 for ; Mon, 6 Sep 2021 18:54:23 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=prvs=388317fed3=patrick.delaunay@foss.st.com Received: from pps.filterd (m0046668.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.16.1.2/8.16.0.43) with SMTP id 186DXZhK008038; Mon, 6 Sep 2021 18:53:15 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=cuSvc41LfhBg3qjT0TwxcPX7H+Ho8rtbKinV0X3qwYg=; b=F2HikHJM/sSF1Yn73F+V66WbSwiahNNztkhSAladdyaOdygqwxJSszi7l94mp00Leu/J 3sVN65NhfVad/FFLQX95/3hI2Bc9NryLZkAJSDlRumw0lyrsC8Y4FEM7ILezlooi606I zOizLxDfJBARUOqAvl0vI132RACkRvsCpijn41WSyfDBtP5N/bDvCqTi5rF1EaOlrRpi ww2aamCg+FvxF9qvc6J4yPAB7cDI/kqDcGHW+GC+YF3lfrIiWFqlayOcY/esBk2UIVeA aTBIPrFbjljcOd0ea/w8EquXDT5VvdYRRhHNhSKQeGNJdBhnsQd6DWwoZRqWI8p5cd9C Jw== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com with ESMTP id 3awkysgspd-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 06 Sep 2021 18:53:15 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id A0B9310002A; Mon, 6 Sep 2021 18:53:10 +0200 (CEST) Received: from Webmail-eu.st.com (sfhdag1node3.st.com [10.75.127.3]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 7881F22D618; Mon, 6 Sep 2021 18:53:10 +0200 (CEST) Received: from lmecxl0994.lme.st.com (10.75.127.49) by SFHDAG1NODE3.st.com (10.75.127.3) with Microsoft SMTP Server (TLS) id 15.0.1497.18; Mon, 6 Sep 2021 18:53:09 +0200 Subject: Re: [PATCH 1/2] lib: optee: remove the duplicate CONFIG_OPTEE To: Etienne Carriere , Alex G. CC: U-Boot Mailing List , Andre Przywara , Bin Meng , Bryan O'Donoghue , Christian Gmeiner , Heinrich Schuchardt , Jens Wiklander , Kever Yang , Masahisa Kojima , Michael Walle , Michal Simek , Ovidiu Panait , =?UTF-8?Q?Pali_Roh=c3=a1r?= , Philipp Tomsich , Philippe Reynes , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Samuel Holland , Sean Anderson , Simon Glass , Stefan Roese , Steffen Jaeckel , Tero Kristo , U-Boot STM32 References: <20210902115512.1.I1c6536da7609f8240549cccae2708e075dc9fcf3@changeid> <569089c3-8936-2a47-930e-218805064413@gmail.com> From: Patrick DELAUNAY Message-ID: Date: Mon, 6 Sep 2021 18:53:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Originating-IP: [10.75.127.49] X-ClientProxiedBy: SFHDAG1NODE2.st.com (10.75.127.2) To SFHDAG1NODE3.st.com (10.75.127.3) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475 definitions=2021-09-06_08,2021-09-03_01,2020-04-07_01 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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. 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