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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id C33F5C54EBC for ; Sat, 7 Jan 2023 21:51:43 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E0D82854EF; Sat, 7 Jan 2023 22:51:41 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org 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=linaro.org header.i=@linaro.org header.b="DaQyoIMR"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2592B85377; Sat, 7 Jan 2023 22:51:40 +0100 (CET) Received: from mail-ed1-x530.google.com (mail-ed1-x530.google.com [IPv6:2a00:1450:4864:20::530]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 1D76F854F1 for ; Sat, 7 Jan 2023 22:51:36 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=jerome.forissier@linaro.org Received: by mail-ed1-x530.google.com with SMTP id x10so4146029edd.10 for ; Sat, 07 Jan 2023 13:51:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=KF1dLtcNrIhnqwZv27V6yDAbMgAkgP063pW8oy5qZJw=; b=DaQyoIMRmH1ClTb+WUh1SKMuMxzp++8Cd4yAxkXSujXwUBPBWqOhGkrmCihzjnatcy YhmQgGVN02Xo3OcjjN3XI2PLiO7lfsenFWvXe9R/Z81nu5p0g9UOU5Aw6vQ/IJdqNnbr IaYerP67EbMaFDydSvU8WZpMNTr40xXpT8fuSscc0IYO4j1au8c8pe5NyZaa9sTHp+6D HPzkXblcAxU1bqqmC+BfLGI8U7QFVfFixJEvjeA1A5z/sZ9tQL47EG/CoIW9ve91PyZG A5+3nzew5dRAqao2UcAzcmooh+upodelmOWQoof+hDADoZEruc+tgPW/Hsp2ZT0Lkxov 8NZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=KF1dLtcNrIhnqwZv27V6yDAbMgAkgP063pW8oy5qZJw=; b=gzKc3lvQtYhWoFGsQxhv3DyCP/nJ+IhYgnTVhX8Lxz7+Q4eZ38K8/TFgqSnzsMiKWW Fr3/XG/9ac7HQoKDDVkJSXfqyi1fxrFZ1KTuFnTdDVO79gzIje0pVlkRzxn/gX0zsp1U C815vDzyENle01QOY9Kh7JWRf+5o7+5KCWocMs8dSh57a07NGTu1wnYimS6oeZAheWge LZanBybrQs5NIWwko/jZzg/hMJZgH5BZeCgyI0rn45kBnvHRpaBZHVuWnv6y9XqhoV3Y qi8tf7mbsFhCYjA/TyqhMbkJmUIvZRRy0Smwo3dCcm94h4EA/mdJya1TRD5puX0/4dVx bDVA== X-Gm-Message-State: AFqh2krbcoQX54tHa0FEgcWtf/fO+Zhcf4uBDfeAL8SSm22eMpGiOrJH GdYKZYlJoKmI20RuMAxNCLcCdQ== X-Google-Smtp-Source: AMrXdXttc2IKYjUioSZjux5YHqkb1NJxty3BM+CCsM+B5lA7c0E2wh9H/VlywKV22gSEyBjNOvzDVA== X-Received: by 2002:aa7:c2c5:0:b0:498:2223:2df9 with SMTP id m5-20020aa7c2c5000000b0049822232df9mr2975759edp.4.1673128295636; Sat, 07 Jan 2023 13:51:35 -0800 (PST) Received: from ?IPV6:2a01:e0a:3cb:7bb0:e4fb:51a5:f121:8512? ([2a01:e0a:3cb:7bb0:e4fb:51a5:f121:8512]) by smtp.gmail.com with ESMTPSA id p18-20020a50cd92000000b0046ba536ce52sm1889103edi.95.2023.01.07.13.51.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 07 Jan 2023 13:51:35 -0800 (PST) Message-ID: Date: Sat, 7 Jan 2023 22:51:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v8 06/13] binman: Support new op-tee binary format To: Simon Glass , Quentin Schulz Cc: U-Boot Mailing List , Tom Rini , huang lin , Jeffy Chen , Kever Yang , Ivan Mikhaylov , Roger Quadros , Philippe Reynes , Alper Nebi Yasak , Peter Geis References: <20221221230726.638740-1-sjg@chromium.org> <20221221230726.638740-7-sjg@chromium.org> <86fabb1a-5087-263f-fa51-87c34d99505a@theobroma-systems.com> Content-Language: en-US From: Jerome Forissier In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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.6 at phobos.denx.de X-Virus-Status: Clean On 1/7/23 19:55, Simon Glass wrote: > Hi Quentin, > > On Mon, 2 Jan 2023 at 10:54, Quentin Schulz > wrote: >> >> Hi Simon, >> >> On 12/22/22 00:07, Simon Glass wrote: >>> OP-TEE has a format with a binary header that can be used instead of the >>> ELF file. With newer versions of OP-TEE this may be required on some >>> platforms. >>> >>> Add support for this in binman. First, add a method to obtain the ELF >>> sections from an entry, then use that in the FIT support. We then end up >>> with the ability to support both types of OP-TEE files, depending on which >>> one is passed in with the entry argument (TEE=xxx in the U-Boot build). >>> >>> Signed-off-by: Simon Glass >>> --- >>> >>> (no changes since v7) >>> >>> Changes in v7: >>> - Correct missing test coverage >>> >>> Changes in v6: >>> - Update op-tee to support new v1 binary header >>> >>> tools/binman/entries.rst | 35 ++++++++- >>> tools/binman/entry.py | 13 +++ >>> tools/binman/etype/fit.py | 69 +++++++++------- >>> tools/binman/etype/section.py | 9 +++ >>> tools/binman/etype/tee_os.py | 68 +++++++++++++++- >>> tools/binman/ftest.py | 83 ++++++++++++++++++++ >>> tools/binman/test/263_tee_os_opt.dts | 22 ++++++ >>> tools/binman/test/264_tee_os_opt_fit.dts | 33 ++++++++ >>> tools/binman/test/265_tee_os_opt_fit_bad.dts | 40 ++++++++++ >>> 9 files changed, 340 insertions(+), 32 deletions(-) >>> create mode 100644 tools/binman/test/263_tee_os_opt.dts >>> create mode 100644 tools/binman/test/264_tee_os_opt_fit.dts >>> create mode 100644 tools/binman/test/265_tee_os_opt_fit_bad.dts >>> >>> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst >>> index b2ce7960d3b..a3e4493a44f 100644 >>> --- a/tools/binman/entries.rst >>> +++ b/tools/binman/entries.rst >>> @@ -1508,12 +1508,45 @@ Entry: tee-os: Entry containing an OP-TEE Trusted OS (TEE) blob >>> >>> Properties / Entry arguments: >>> - tee-os-path: Filename of file to read into entry. This is typically >>> - called tee-pager.bin >>> + called tee.bin or tee.elf >>> >>> This entry holds the run-time firmware, typically started by U-Boot SPL. >>> See the U-Boot README for your architecture or board for how to use it. See >>> https://urldefense.com/v3/__https://github.com/OP-TEE/optee_os__;!!OOPJP91ZZw!kx6SLv4sPusmg1TyYMw-Ho5G9jxeVMf9HOYx_4yq3ZNl_TpoGJoUyICZMgEiv0Zd6l_Bl8f5OAFYyJxm8wDUevhARIs$ for more information about OP-TEE. >>> >>> +Note that if the file is in ELF format, it must go in a FIT. In that case, >>> +this entry will mark itself as absent, providing the data only through the >>> +read_elf_segments() method. >>> + >>> +Marking this entry as absent means that it if is used in the wrong context >>> +it can be automatically dropped. Thus it is possible to add anb OP-TEE entry >> >> s/anb/an/ >> >>> +like this:: >>> + >>> + binman { >>> + tee-os { >>> + }; >>> + }; >>> + >>> +and pass either an ELF or plain binary in with -a tee-os-path >>> +and have binman do the right thing: >>> + >>> + - include the entry if tee.bin is provided and it doesn't have the v1 >>> + header >>> + - drop it otherwise >>> + >> >> Is there an actual usecase for this? (sorry if this was mentioned in the >> earlier versions of the patch) Are we expecting to be able to append the >> content of tee-os to some raw binary instead of putting OP-TEE OS in a >> u-boot.itb image? > > Yes, that is my understanding. Perhaps it is for some archs or some > old versions. But in any case, we can always drop it later if not > needed, refine the warnings, etc. etc. But please let's land this mess > and move on. If I had pushed a little harder a year ago we would have > be talking about how to do this properly in binman rather than me > trying to retrofit everything. > > [..] > >> >> You can use a reference here since we have a _etype_fit target for "Flat >> Image Tree / FIT". >> >>> +the binary v1 format is provided. >>> + >>> >> >> I'm a bit worried this is OP-TEE OS specific? We could also point to the >> documentation here: >> https://optee.readthedocs.io/en/latest/architecture/core.html#partitioning-of-the-binary? > > Yes it is...I believe there will be other use cases for absent > entries....in a way it is quite a big hold in binman's functionality. > Will link to the docs. > >> >>> >>> .. _etype_text: >>> diff --git a/tools/binman/entry.py b/tools/binman/entry.py >>> index 637aece3705..de51d295891 100644 >>> --- a/tools/binman/entry.py >>> +++ b/tools/binman/entry.py >>> @@ -1290,3 +1290,16 @@ features to produce new behaviours. >>> def mark_absent(self, msg): >>> tout.info("Entry '%s' marked absent: %s" % (self._node.path, msg)) >>> self.absent = True >>> + >>> + def read_elf_segments(self): >>> + """Read segments from an entry that can generate an ELF file >>> + >>> + Returns: >>> + tuple: >>> + list of segments, each: >>> + int: Segment number (0 = first) >>> + int: Start address of segment in memory >>> + bytes: Contents of segment >>> + int: entry address of ELF file >>> + """ >>> + return None >> >> Does it really make sense to have this function available to all Entry >> objects? > > Yes we must define functions in the base class to avoid confusion. > Also it does actually get called from the FIT implementation, to see > if this function can provide the segments...failing that it tries to > parse an ELF blob. > > [..] > >>> + @staticmethod >>> + def is_optee_bin(data): >> >> Here you are checking it's a binary with v1 header, so please explicit >> in the function name. (there's already a v2 header available FWIW). > > OMG > >> >>> + return len(data) >= 8 and data[0:5] == b'OPTE\x01' >>> + >>> + def ObtainContents(self, fake_size=0): >>> + super().ObtainContents(fake_size) >> >> Do not silence the return code of the parent class here? I know it's >> only returning True ATM but nothing guarantees it'll stay this way. >> >>> + if not self.missing: >>> + if elf.is_valid(self.data): >>> + self.mark_absent('uses Elf format which must be in a FIT') >>> + elif self.is_optee_bin(self.data): >>> + self.mark_absent('uses v1 format which must be in a FIT') >> >> What should this support then if neither ELF not binary with v1 header >> are supported? I don't see support for binary with v2 header anywhere so >> I'm quite confused by what this piece of code is supposed to handle? >> >> I'm also very much against displaying a warning if the user has TEE set >> in their environment and the file exists but it won't be used in the >> final image. If it's not compatible based on the binman DT node, error >> out. If it's the wrong file or it's missing, error out. Is there some >> scenario where displaying a warning (and/or removing the entry with >> mark_absent like you did here) make sense? > > We need to deal with this later, sorry. I have come to the end of the > time I can bear to spend dealing with the strangeness of OP-TEE. If > you are able to tidy this up after it lands, or have ideas on how to > do better, please let me know. There is nothing strange in OP-TEE. There is a tee.bin file, it needs to be loaded into memory at a specific address that you can find by reading a header (yes those things exist in OS binaries, which are not all pure ELF files). As shown in make_atf_fit.py, it takes only a couple of lines of Python to get this address. Since you are calling OP-TEE strange let me say I don't understand a thing about all the U-Boot specific tooling that you are putting in place (this whole binman thing in particular), so as a newbie in this project I should probably be allowed to call that weird, too ;-) But that's not the point. The point is, with the current patch set I can't build a working FIT image for RockPi4 that includes OP-TEE. Note I have not tried v9 but I see nothing significant changed regarding the handling of tee.bin (please correct me if I'm wrong). Therefore merging this patch set in its current state would clearly be a regression and prevent us from upgrading U-Boot in the projects I'm working on... >> In any case, thanks Simon and Jerome for looking into this, it's a >> bigger task than I had anticipated but am looking forward to this >> Rockchip-specific behavior to be dropped from mainline :) > > Thanks. This has been beyond painful. We have to stop all these > scripts and strange workflows...it is making firmware impossible to > understand. Refactoring is certainly good from time to time. But you have to make sure the feature set is the same, and ideally the usage is unchanged (or only marginally). Otherwise you're transferring the pain to your users... Thanks, -- Jerome > > Regards, > Simon