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=-20.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,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 38A6BC07E95 for ; Tue, 20 Jul 2021 16:14:43 +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 2866660720 for ; Tue, 20 Jul 2021 16:14:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2866660720 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=siemens.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E60A282CE8; Tue, 20 Jul 2021 18:14:39 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=siemens.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 65C8082D22; Tue, 20 Jul 2021 18:14:38 +0200 (CEST) Received: from lizzard.sbs.de (lizzard.sbs.de [194.138.37.39]) (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 8B33982CE8 for ; Tue, 20 Jul 2021 18:14:34 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=siemens.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=jan.kiszka@siemens.com Received: from mail2.sbs.de (mail2.sbs.de [192.129.41.66]) by lizzard.sbs.de (8.15.2/8.15.2) with ESMTPS id 16KGEWlQ011240 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 20 Jul 2021 18:14:32 +0200 Received: from [139.22.32.146] ([139.22.32.146]) by mail2.sbs.de (8.15.2/8.15.2) with ESMTP id 16KGEVZn016460; Tue, 20 Jul 2021 18:14:31 +0200 Subject: Re: [PATCH v2 4/5] watchdog: rti_wdt: Add support for loading firmware From: Jan Kiszka To: Simon Glass Cc: Tom Rini , Lokesh Vutla , U-Boot Mailing List , Le Jin , Bao Cheng Su , Nian Gao , Chao Zeng References: <88d7d3e323c27417d7109b8a92bf53a08ad77654.1622626660.git.jan.kiszka@siemens.com> <96039724-9a5f-dbb8-d46b-b268a0d9a8c2@ti.com> <20210607114007.GD9516@bill-the-cat> <20210611140831.GU9516@bill-the-cat> <54a75091-0499-8902-b7f5-b1f75ae0091c@siemens.com> <53a5e923-04a1-3ca6-14ed-d051c123fab3@siemens.com> <48724115-33f5-78b0-73ec-42f25a3b4340@siemens.com> <6e6c7e49-a885-f8ed-ca27-bc91dbca3874@siemens.com> Message-ID: Date: Tue, 20 Jul 2021 18:14:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <6e6c7e49-a885-f8ed-ca27-bc91dbca3874@siemens.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 On 20.07.21 14:57, Jan Kiszka wrote: > On 14.07.21 16:15, Simon Glass wrote: >> Hi Jan, >> >> On Wed, 14 Jul 2021 at 03:53, Jan Kiszka wrote: >>> >>> On 05.07.21 17:29, Simon Glass wrote: >>>> Hi Jan, >>>> >>>> On Sun, 27 Jun 2021 at 23:40, Jan Kiszka wrote: >>>>> >>>>> On 27.06.21 20:18, Simon Glass wrote: >>>>>> Hi Jan, >>>>>> >>>>>> On Sun, 27 Jun 2021 at 12:01, Jan Kiszka wrote: >>>>>>> >>>>>>> On 26.06.21 20:29, Simon Glass wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Fri, 11 Jun 2021 at 08:08, Tom Rini wrote: >>>>>>>>> >>>>>>>>> On Fri, Jun 11, 2021 at 07:14:21PM +0530, Lokesh Vutla wrote: >>>>>>>>>> Hi Tom, >>>>>>>>>> >>>>>>>>>> On 09/06/21 6:47 pm, Jan Kiszka wrote: >>>>>>>>>>> On 07.06.21 13:44, Jan Kiszka wrote: >>>>>>>>>>>> On 07.06.21 13:40, Tom Rini wrote: >>>>>>>>>>>>> On Mon, Jun 07, 2021 at 03:33:52PM +0530, Lokesh Vutla wrote: >>>>>>>>>>>>>> +Tom, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Tom, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 02/06/21 3:07 pm, Jan Kiszka wrote: >>>>>>>>>>>>>>> From: Jan Kiszka >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> To avoid the need of extra boot scripting on AM65x for loading a >>>>>>>>>>>>>>> watchdog firmware, add the required rproc init and loading logic for the >>>>>>>>>>>>>>> first R5F core to the watchdog start handler. In case the R5F cluster is >>>>>>>>>>>>>>> in lock-step mode, also initialize the second core. The firmware itself >>>>>>>>>>>>>>> is embedded into U-Boot binary to ease access to it and ensure it is >>>>>>>>>>>>>>> properly hashed in case of secure boot. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> One possible firmware source is https://github.com/siemens/k3-rti-wdt. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Jan Kiszka >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> drivers/watchdog/Kconfig | 20 ++++++++++++ >>>>>>>>>>>>>>> drivers/watchdog/Makefile | 5 +++ >>>>>>>>>>>>>>> drivers/watchdog/rti_wdt.c | 58 ++++++++++++++++++++++++++++++++++- >>>>>>>>>>>>>>> drivers/watchdog/rti_wdt_fw.S | 20 ++++++++++++ >>>>>>>>>>>>>>> 4 files changed, 102 insertions(+), 1 deletion(-) >>>>>>>>>>>>>>> create mode 100644 drivers/watchdog/rti_wdt_fw.S >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>>>>>>>>>>>>>> index f0ff2612a6..1a1fddfe9f 100644 >>>>>>>>>>>>>>> --- a/drivers/watchdog/Kconfig >>>>>>>>>>>>>>> +++ b/drivers/watchdog/Kconfig >>>>>>>>>>>>>>> @@ -209,6 +209,26 @@ config WDT_K3_RTI >>>>>>>>>>>>>>> Say Y here if you want to include support for the K3 watchdog >>>>>>>>>>>>>>> timer (RTI module) available in the K3 generation of processors. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> +if WDT_K3_RTI >>>>>>>>>>>>>>> + >>>>>>>>>>>>>>> +config WDT_K3_RTI_LOAD_FW >>>>>>>>>>>>>>> + bool "Load watchdog firmware" >>>>>>>>>>>>>>> + depends on REMOTEPROC >>>>>>>>>>>>>>> + help >>>>>>>>>>>>>>> + Automatically load the specified firmware image into the MCU R5F >>>>>>>>>>>>>>> + core 0. On the AM65x, this firmware is supposed to handle the expiry >>>>>>>>>>>>>>> + of the watchdog timer, typically by resetting the system. >>>>>>>>>>>>>>> + >>>>>>>>>>>>>>> +config WDT_K3_RTI_FW_FILE >>>>>>>>>>>>>>> + string "Watchdog firmware image file" >>>>>>>>>>>>>>> + default "k3-rti-wdt.fw" >>>>>>>>>>>>>>> + depends on WDT_K3_RTI_LOAD_FW >>>>>>>>>>>>>>> + help >>>>>>>>>>>>>>> + Firmware image to be embedded into U-Boot and loaded on watchdog >>>>>>>>>>>>>>> + start. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I need your input on this proach. Is it okay to include the linker file unders >>>>>>>>>>>>>> drivers? >>>>>>>>>>>>> >>>>>>>>>>>>> Maybe? I suppose the first thing that springs to mind is why aren't we >>>>>>>>>>>>> using binman and including this blob (which I happily see is GPLv2) >>>>>>>>>>>>> similar to how we do things with x86 for one example. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> See https://www.mail-archive.com/u-boot@lists.denx.de/msg377894.html >>>>>>>>>>>> >>>>>>>>>>>> Jan >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Did this help to answer open questions? Otherwise, please let me know. >>>>>>>>>>> >>>>>>>>>>> I'd also like to avoid that his patch alone blocks 1-3 of the series >>>>>>>>>>> needless - but I would also not mind getting everything in at once. >>>>>>>>>> >>>>>>>>>> Can you provide your reviewed-by if you are okay with this approach? >>>>>>>>> >>>>>>>>> I was kind of hoping Simon would chime in here on binman usage. So, >>>>>>>>> re-re-reading the above URL, yes, fsloader wouldn't be the right choice >>>>>>>>> for watchdog firmware. But I think binman_entry_find() and related >>>>>>>>> could work, in general, for this case of "need firmware blob embedded in >>>>>>>>> to image". That said, this isn't just any firmware blob, it's the >>>>>>>>> watchdog firmware. The less reliance on other things the safer it is. >>>>>>>>> That means this would be an exception to the general firmware blob >>>>>>>>> loading rule and yeah, OK, we can do it this way. Sorry for the delay. >>>>>>>> >>>>>>>> Yes I've been a little tied up recently. But I think this should use >>>>>>>> binman. We really don't want to be building binary firmware into >>>>>>>> U-Boot itself! >>>>>>>> >>>>>>>> Also Tom says, see x86 for a load of binaries of different types and >>>>>>>> how they are accessed at runttime. This is what binman is for. >>>>>>>> >>>>>>> >>>>>>> Please take the time and study my arguments. I'm open for better >>>>>>> proposals, but they need to be concrete and addressing my points. >>>>>> >>>>>> Do you mean 'properly hashed' and 'easy access', or something else? >>>>>> What can binman not do? >>>>> >>>>> Binman itself can stick things into binary images. But that is at most >>>>> half of the tasks needed here. I would need concrete guidance how to >>>>> >>>>> - access that binary from u-boot proper in a reasonably simple way >>>> >>>> I thought you wanted to access it from SPL. For that you would use >>>> linker symbols. See 'Access to binman entry offsets at run time >>>> (symbols)' in the binman docs for that. >>>> >>>> But for U-Boot proper, the section is 'Access to binman entry offsets >>>> at run time (fdt)'. There is no mention of the runtime library that >>>> now exists (binman.h) so I will send a patch for that. But basically >>>> you call binman_entry_find("name", &entry) and it returns the offset >>>> and size for you. >>>> >>>>> - make sure the binary can be signed and the signature is evaluated >>>>> before using it >>>> >>>> Do you mean signed or hashed? I think you mean hashed. If you trust >>>> the U-Boot binary then presumably you can put the firmware in a >>>> similar place do you can trust that as well. After all, you seem happy >>>> enough to link it into U-Boot. >>>> >>>> If not, you can ask binman to add a hash: >>>> >>>> my-firmware { >>>> type = "blob"; >>>> hash { >>>> algo = "sha256"; >>>> }; >>>> }; >>>> >>>> Then you can add support for that to some sort of helper function in >>>> binman.c, e.g.: >>>> >>>> ofnode node, hashnode; >>>> const u8 *hash; >>>> int size; >>>> >>>> node = binman_section_find_node("name"); >>>> if (!ofnode_valid(node)) >>>> ...return error >>>> hashnode = ofnode_read_prop(node, "hash"); >>>> if (!ofnode_valid(hashnode)) >>>> ...return error >>>> hash = ofnode_read_prop(hashnode, "value", &size); >>>> >>>> /* Hash the firmware...need to read it from flash into fwdata/fwlen >>>> using binman_entry_find() ...then: */ >>>> u8 digest[SHA256_SUM_LEN]; >>>> ret = hash_block("sha256", fwdata, fwlen, digest, sizeof(digest); >>>> if (ret) >>>> return log_msg_ret("hash", ret); >>>> >>>> /* compare the hashes */ >>>> if (size != sizeof(digest) || memcmp(hash, digest)) >>>> return log_msg_ret("cmp", ret); >>>> >>> >>> Yes, it will likely be a hash that will be signed, and all that will be >>> checked by SPL when loading proper. The infrastructure for that should >>> be there, just not yet plugged for the way of loading things like we do >>> (one of my todos). >>> >>> Obviously, what would be simplest for that is to have the watchdog blob >>> embedded, rather than separately hashed, as that would Just Work. I >>> didn't fully grasp yet what you propose, but it seems right now it will >>> complicate things. I'm not saying it will make it impossible, just harder. >> >> I'm not even sure that is true. In binman, add the entry: >> >> watchdog-firmware { >> type = "blob"; >> filename = "..."; >> }; >> >> In the C code, declare a symbol that will get the image position of the entry: >> >> binman_sym_declare(unsigned long, watchdog_firmware, image_pos); >> >> then read that value when you need it: >> >> int check_it(..) >> { >> ulong pos = binman_sym(ulong, watchdog_firmware, image_pos); >> >> // read from flash offset @pos into a buffer >> >> // check the hash >> }; > > This function is the extra boilerplate code that is not needed when the > blob is simply part of the U-Boot binary that is loaded and checked by > SPL. The worst part of this: It requires special handling of different > storage media. We currently only have OSPI for out board, but you may > also imagine versions that load U-Boot from filesystems (the TI EVM does > that). So this is the extra complexity without extra value I'm always > talking about. > > But I'm happy to take concrete suggestions where to add the firmware > into our board and where to add the necessary code in a simple way. > > What we do so far: U-Boot proper and DTBs go into a fit image that SPL > will load (and later also validate). It would be simple to do > > diff --git a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > index 96647719df..d3242af70c 100644 > --- a/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > +++ b/arch/arm/dts/k3-am65-iot2050-boot-image.dtsi > @@ -60,6 +60,11 @@ > filename = "arch/arm/dts/k3-am6548-iot2050-advanced.dtb"; > }; > }; > + > + k3-rti-firmware { > + type = "blob"; > + filename = CONFIG_WDT_K3_RTI_FW_FILE; > + }; > }; > > configurations { > > in [1] (used by [2]). > > But now: How do I communicate that blob address from SPL to U-Boot > proper so that I can skip the extra medium-specific loading part and > also reuse the fit image validation of SPL? If there is a simple way for > that, I could indeed switch the model. > OK, I think I found a trick (outside of binman): Making the firmware an additional loadable in the u-boot fit image, and then picking its location up from /fit-images/k3-rti-wdt-firmware in rti_wdt_load_fw(). That should be nicer then object embedded - without requiring the complexity of separate image loading and validating. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux