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.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS 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 2DB9CC48BE8 for ; Tue, 15 Jun 2021 04:45:18 +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 1601A61403 for ; Tue, 15 Jun 2021 04:45:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1601A61403 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org 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 63C8C803B2; Tue, 15 Jun 2021 06:45:14 +0200 (CEST) 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="nBMf3UlJ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id BA971801BE; Tue, 15 Jun 2021 06:45:11 +0200 (CEST) Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) (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 0E550801BE for ; Tue, 15 Jun 2021 06:45:07 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pj1-x1030.google.com with SMTP id bb10-20020a17090b008ab029016eef083425so612685pjb.5 for ; Mon, 14 Jun 2021 21:45:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=RuoIoGYzbGyWuMTYlob/6ep4O/0R8QGBsG1ELZVfpxk=; b=nBMf3UlJHCWRdHvo+zDs1zGxjHZoMdIZqs0UIc/l+SmjSFOymTFSaKVIblGMnjRnJs nQr6HedL63lEJjLiXxyHp2CFjnbfodBQ8olbrZSO6JoS0PTHWu9+QiQR1Cpcdt9bUngA Vd/Durno6ydPzmaNLGrOwNT+vDHNE/lIUeYAoVn1G5XH8lB1QimVt9fF/7CH3L++kn4U 9X8uqAO+rHYFu7VMWfAiML3v7k76BTVJgpuL30tvs3IG7OGNPo8Naad90LiDPsC7RRvl Jaireis01UB0qqLDYJ3udxWlk+S4W+zSOM1MBeeGK1kDpULsWwPZ03ByX4AhqUatlXdL t4mQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=RuoIoGYzbGyWuMTYlob/6ep4O/0R8QGBsG1ELZVfpxk=; b=naf0g1z5a/FYsZhew/va/ai2NBUABpjCvc0UUkXFRu2juAqjgjGjSJxt8yYcxZ4uXM 3hGxOETcQLYtirbGnlpyJRjeX/flsU2Ye4KhCOj1STSxmF6Ho9rlxp6zluaAbKoLD1Iz kq/MaGGjUGurIC/Nmi7K3Hfhdm+/n99OVRUX/KNvLHt7nWjwaLi+jN+ll0wYnyRQnqwh DV8d7A7trerGeRrr3RTMXiuPw4RwRQTtgiL5wT7c7NlFfkXYzzLh93jSC6AuW+NhuGja G/WC7jMNqV23PXnTSvYTMuctaEc+iYiNpbmpL/n5AGcYnimkPuzGcn4UGgFLi2Ar/LgC rbmQ== X-Gm-Message-State: AOAM533WsGzVEBNFp7AQiu4S8DxmIv8uYuOKM6uRFFiU9Anq2CSr1qF8 vFxb9ZBElraJREqTnsvElO+RPQ== X-Google-Smtp-Source: ABdhPJyFvP9A2dEqQAlo06u0CKnlU3x39o8nedzj1vezCdmUgCQvNKg8zjShynFshRF8n8AEvty8jQ== X-Received: by 2002:a17:90b:3749:: with SMTP id ne9mr2800697pjb.77.1623732303855; Mon, 14 Jun 2021 21:45:03 -0700 (PDT) Received: from laputa (p3dd30534.tkyea130.ap.so-net.ne.jp. [61.211.5.52]) by smtp.gmail.com with ESMTPSA id b65sm2879944pfa.32.2021.06.14.21.45.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 14 Jun 2021 21:45:03 -0700 (PDT) Date: Tue, 15 Jun 2021 13:44:58 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: xypron.glpk@gmx.de, masami.hiramatsu@linaro.org, Simon Glass , Mario Six , Michal Simek , Alexander Graf , u-boot@lists.denx.de Subject: Re: [PATCH] efi_loader: FMP cleanups Message-ID: <20210615044458.GA54329@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , xypron.glpk@gmx.de, masami.hiramatsu@linaro.org, Simon Glass , Mario Six , Michal Simek , Alexander Graf , u-boot@lists.denx.de References: <20210614151015.99992-1-ilias.apalodimas@linaro.org> <20210615015101.GA46087@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote: > Akashi-san, > > On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote: > > Ilias, > > > > In this patch, you are trying to address a couple of independent > > issues in a single commit. > > Please split. > > (Heinrich doesn't like that.) Any comment? > > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote: > > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at > > > the same time. Moreover we only install those if a CapsuleUpdate is > > > requested. Since we now have an ESRT table, it makes more sense to > > > unconditionally install the FMP, so any userspace applications (e.g fwupd) > > > can make use of them and trigger an update. > > > > > > While at it clean up the FMP installation as well. Chapter 23 of the EFI > > > spec (rev 2.9) says: > > > "A specific updatable hardware firmware store must be represented by > > > exactly one FMP instance". > > > This is not the case for us, since both of our FMP protocols can be > > > installed at the same time and are controlled by a single 'dfu_alt_info' > > > env variable. > > > So make the config option a choice and allow the user to install one > > > of them at any given time. > > > > I'd like to say nak in some respects: > > - Although I do understand the UEFI requirement that you mentioned above, > > FIT and RAW FMP drivers can handle *different* firmware even though > > they share the same dfu_alt_info. > > How ? One idea that I can imagine is that we may be able to utilize "update_image_index", which is currently not used effectively, in order to specify which firmware in dfu_alt_info be handled by either FIT FMP or RAW FMP. > > We should not impose unnecessary restriction if we manage to have some > > workaround to meet the requirement. > > It's not the updating part only. It's that the .get_image_info also relies on > the same env variable. The above idea can and should be applied to GetImageInfo implementation at the same time. > Specifically in the fwupd case on an RPI4 with the > dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were > populated only one was reported. Probably because this really does give you > 2 ways of updating the same flash. > > > (I still believe that the firmware definition for ESRT should exist > > elsewhere other than in FMP themselves.) > > That's a whole different story, and if we have that, then .get_image_info > should change as well instead of using the DFU information. I don't think so as I mentioned above. > Because right > now we enabled security (or think we have), while allowing users to set an env > variable which is not authenticated, and completely change what the > firmware reports (or updates). This is the point that I mentioned earlier in our private chat, and it's a "whole different" story in this context. > We can always add a huge warning saying > something along the lines of "If you really care this should come with a > CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1". > > The spec is pretty clear and we allow users to *break* it with a config > option. Arguably this is fine, since the code continues to work fine and > you can perform the updates, but in essence RAW and FITs are used to update > the same medium right now. You can't have a capsule with half it's contents > describing something RAW and the other half being a FIT. You have a FIT based > capsule or a RAW based capsule. See above. > > - We should allow users to add their own FMP drivers and so not call > > [arch_]efi_load_capsule_drivers() unconditionally > > even if you don't like "__weak" attribute. > > I am fine with the __weak attribute. On the other hand I consider the > current code the defacto way users would use to update their firmware. That's > why I removed the __weak attribute. If a hardware vendor was to update > their special PCI option ROM or a flash that lives on the secure world they > should install their FMPs on a different handle and leave the current code > as is. And we should provide an option that disables these existing handle. > > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will > > leave some test cases in pytest skipped. > > Yea that's unfortunate, but maybe we can just add an extra config on the > sandbox? Please add another patch that is missing. -Takahiro Akashi > Thanks > /Ilias > > > > > -Takahiro Akashi > > > > > The overall changes show up in fwupd > > > > > > pre-patch: > > > fwupdmgr get-devices > > > No detected devices > > > > > > post-patch (with FIT FMP installed): > > > fwupdmgr get-devices > > > WARNING: Required efivarfs filesystem was not found > > > See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information. > > > Unknown Product > > > │ > > > └─Unknown Firmware: > > > Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6 > > > Current version: 0 > > > Vendor: DMI:U-Boot > > > Update Error: Not updatable as efivarfs was not found > > > GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147 > > > 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware > > > 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147} > > > Device Flags: • Internal device > > > • System requires external power source > > > • Needs a reboot after installation > > > • Device is usable for the duration of the update > > > > > > Signed-off-by: Ilias Apalodimas > > > --- > > > configs/sandbox64_defconfig | 1 - > > > configs/sandbox_defconfig | 1 - > > > configs/xilinx_zynqmp_virt_defconfig | 1 - > > > include/efi_loader.h | 1 + > > > lib/efi_loader/Kconfig | 48 +++++++++++++++------------- > > > lib/efi_loader/efi_capsule.c | 22 ++++--------- > > > lib/efi_loader/efi_setup.c | 4 +++ > > > 7 files changed, 37 insertions(+), 41 deletions(-) > > > > > > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig > > > index 9a373bab6fe3..af18b6c7826e 100644 > > > --- a/configs/sandbox64_defconfig > > > +++ b/configs/sandbox64_defconfig > > > @@ -233,7 +233,6 @@ CONFIG_LZ4=y > > > CONFIG_ERRNO_STR=y > > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y > > > CONFIG_EFI_CAPSULE_ON_DISK=y > > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y > > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y > > > CONFIG_EFI_SECURE_BOOT=y > > > CONFIG_TEST_FDTDEC=y > > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig > > > index bdbf714e2bd9..24313fdfa53d 100644 > > > --- a/configs/sandbox_defconfig > > > +++ b/configs/sandbox_defconfig > > > @@ -280,7 +280,6 @@ CONFIG_LZ4=y > > > CONFIG_ERRNO_STR=y > > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y > > > CONFIG_EFI_CAPSULE_ON_DISK=y > > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y > > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y > > > CONFIG_EFI_SECURE_BOOT=y > > > CONFIG_TEST_FDTDEC=y > > > diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig > > > index e939b04ef6a5..0c2d1a70a5a1 100644 > > > --- a/configs/xilinx_zynqmp_virt_defconfig > > > +++ b/configs/xilinx_zynqmp_virt_defconfig > > > @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y > > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y > > > CONFIG_EFI_CAPSULE_ON_DISK=y > > > CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y > > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y > > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > index 0a9c82a257e1..b81180cfda8b 100644 > > > --- a/include/efi_loader.h > > > +++ b/include/efi_loader.h > > > @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void); > > > * - error code otherwise. > > > */ > > > efi_status_t efi_esrt_populate(void); > > > +efi_status_t efi_load_capsule_drivers(void); > > > #endif /* _EFI_LOADER_H */ > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > index 6242caceb7f9..da6f5faf5adb 100644 > > > --- a/lib/efi_loader/Kconfig > > > +++ b/lib/efi_loader/Kconfig > > > @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT > > > Select this option if you want to enable capsule-based > > > firmware update using Firmware Management Protocol. > > > > > > +choice EFI_CAPSULE_TYPE > > > + prompt "Capsule type (RAW/FIT)" > > > + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT > > > + > > > +config EFI_CAPSULE_FIRMWARE_FIT > > > + bool "FMP driver for FIT images" > > > + depends on FIT > > > + select UPDATE_FIT > > > + select DFU > > > + select EFI_CAPSULE_FIRMWARE > > > + help > > > + Select this option if you want to enable firmware management protocol > > > + driver for FIT image > > > + > > > +config EFI_CAPSULE_FIRMWARE_RAW > > > + bool "FMP driver for raw images" > > > + select DFU_WRITE_ALT > > > + select DFU > > > + select EFI_CAPSULE_FIRMWARE > > > + help > > > + Select this option if you want to enable firmware management protocol > > > + driver for raw image > > > + > > > +endchoice > > > + > > > config EFI_CAPSULE_AUTHENTICATE > > > bool "Update Capsule authentication" > > > depends on EFI_CAPSULE_FIRMWARE > > > @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE > > > Select this option if you want to enable capsule > > > authentication > > > > > > -config EFI_CAPSULE_FIRMWARE_FIT > > > - bool "FMP driver for FIT image" > > > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT > > > - depends on FIT > > > - select UPDATE_FIT > > > - select DFU > > > - select EFI_CAPSULE_FIRMWARE > > > - default n > > > - help > > > - Select this option if you want to enable firmware management protocol > > > - driver for FIT image > > > - > > > -config EFI_CAPSULE_FIRMWARE_RAW > > > - bool "FMP driver for raw image" > > > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT > > > - select DFU > > > - select DFU_WRITE_ALT > > > - select EFI_CAPSULE_FIRMWARE > > > - default n > > > - help > > > - Select this option if you want to enable firmware management protocol > > > - driver for raw image > > > - > > > config EFI_DEVICE_PATH_TO_TEXT > > > bool "Device path to text protocol" > > > default y > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > index 9ead0d2c7816..3b4214a8d4cc 100644 > > > --- a/lib/efi_loader/efi_capsule.c > > > +++ b/lib/efi_loader/efi_capsule.c > > > @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void) > > > } > > > > > > /** > > > - * arch_efi_load_capsule_drivers - initialize capsule drivers > > > + * efi_load_capsule_drivers - initialize capsule drivers > > > * > > > - * Architecture or board specific initialization routine > > > + * Generic FMP drivers backed by DFU > > > * > > > * Return: status code > > > */ > > > -efi_status_t __weak arch_efi_load_capsule_drivers(void) > > > +efi_status_t efi_load_capsule_drivers(void) > > > { > > > - __maybe_unused efi_handle_t handle; > > > + __maybe_unused efi_handle_t handle = NULL; > > > efi_status_t ret = EFI_SUCCESS; > > > > > > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) { > > > - handle = NULL; > > > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) > > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces( > > > &handle, &efi_guid_firmware_management_protocol, > > > &efi_fmp_fit, NULL)); > > > - } > > > > > > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) { > > > - handle = NULL; > > > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) > > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces( > > > - &efi_root, > > > + &handle, > > > &efi_guid_firmware_management_protocol, > > > &efi_fmp_raw, NULL)); > > > - } > > > > > > return ret; > > > } > > > @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void) > > > > > > index = get_last_capsule(); > > > > > > - /* Load capsule drivers */ > > > - ret = arch_efi_load_capsule_drivers(); > > > - if (ret != EFI_SUCCESS) > > > - return ret; > > > > > > /* > > > * Find capsules on disk. > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c > > > index 3c5cf9a4357e..0106efdc161b 100644 > > > --- a/lib/efi_loader/efi_setup.c > > > +++ b/lib/efi_loader/efi_setup.c > > > @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void) > > > if (ret != EFI_SUCCESS) > > > goto out; > > > > > > + ret = efi_load_capsule_drivers(); > > > + if (ret != EFI_SUCCESS) > > > + goto out; > > > + > > > #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO) > > > ret = efi_gop_register(); > > > if (ret != EFI_SUCCESS) > > > -- > > > 2.32.0.rc0 > > >