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=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 B6BF5C433F5 for ; Tue, 7 Sep 2021 03:59:08 +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 DDD4760EE6 for ; Tue, 7 Sep 2021 03:59:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org DDD4760EE6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org 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 40DAD83200; Tue, 7 Sep 2021 05:59:04 +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="vxMFOmJV"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 25B8B83200; Tue, 7 Sep 2021 05:59:01 +0200 (CEST) Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) (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 80BBB82E4C for ; Tue, 7 Sep 2021 05:58:55 +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-pf1-x436.google.com with SMTP id g14so7093580pfm.1 for ; Mon, 06 Sep 2021 20:58:55 -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:in-reply-to; bh=/PtXK9UQezmTFEqv14MJYeeyycpa51Gi0kiJC9eu/os=; b=vxMFOmJVHoptkcESxOQxkzF8Gjg4gFYaTMb2Cbx9AYwayrneQs8nbPIvNXv+i5a18z mIzg7IXTfxTnxdj0SFvV0TNoLoRtHeUWlJHV3qQ+UmD1flkw+bjsp9mWTgbgvz8pICaz SFtIC/aktmBT4bAdA8/M037SuSCyZE3GT9Ymp6T0PeNx44xZy1mVvXFUEtDX678kuqdI /VhfDqlwMt17DG8BEdX3LZVh+OmcJS4q72SJKHZwIuWDBjKuDOQjMjg3QYBtIZA3razu 03gZSCCN5iWiyQvv/4zYxMSMwDzqihQXmgEVLeP+xcJosFZdg5KRrb0u6b6V6mrE83a7 CatA== 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 :in-reply-to; bh=/PtXK9UQezmTFEqv14MJYeeyycpa51Gi0kiJC9eu/os=; b=tZXjxSjYRli290d9xinlg3VKPUXNmU4a24MfCLQ3Z3wZOKCsOc0FBUndEUFQfAJ/hQ u2nVvSqsO+zBwCYnyCCEY0frhr/BTGN+io56cgr/FyXyXbXELcs2cv1a5WBrjIBFZn0z fPqVKQLiHRP5ScHJK+o30eV6xjpfz1pAo5ibVs4TE9aTBqVWrcUpiGgBNah7uyVcxo0m RRCpiTle4Hs1pSQiB2zSCIkA4uB737t4eeEXO4Ft8ZJ28zEsgXuj97a1nma2jKkJRR2l AOXwzSD/DUGWdTvnF17cNBH+hLHlGeWAz4PL3BZ9DQxoD5q42V5EMdpp+n2H6nZi/0pp HU1g== X-Gm-Message-State: AOAM533RxBByoOsv8AhhAEGbjf0hsro376A0uZ6zwx2xqilFqmBU2ixH aqL0QqNG+7Hp0R3rU0BR3jgZdg== X-Google-Smtp-Source: ABdhPJzV72ecUW6z6tLNO35S2/ImnUlptZVsfDpq1Ol5mt8kPjzbLybbJEpAz6kq9fv1PrRKmTe+0A== X-Received: by 2002:a65:6554:: with SMTP id a20mr15183911pgw.107.1630987133359; Mon, 06 Sep 2021 20:58:53 -0700 (PDT) Received: from laputa (p784a2304.tkyea130.ap.so-net.ne.jp. [120.74.35.4]) by smtp.gmail.com with ESMTPSA id x7sm9024141pfn.54.2021.09.06.20.58.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Sep 2021 20:58:52 -0700 (PDT) Date: Tue, 7 Sep 2021 12:58:47 +0900 From: AKASHI Takahiro To: Simon Glass Cc: Heinrich Schuchardt , Steffen Jaeckel , Klaus Heinrich Kiwi , Ilias Apalodimas , Michal Simek , Tom Rini , Alexandru Gagniuc , U-Boot Mailing List , Masahisa Kojima , Bin Meng Subject: Re: [PATCH] RFC: Support an EFI-loader bootflow Message-ID: <20210907035847.GA49004@laputa> Mail-Followup-To: AKASHI Takahiro , Simon Glass , Heinrich Schuchardt , Steffen Jaeckel , Klaus Heinrich Kiwi , Ilias Apalodimas , Michal Simek , Tom Rini , Alexandru Gagniuc , U-Boot Mailing List , Masahisa Kojima , Bin Meng References: <20210828203521.111877-1-sjg@chromium.org> <20210831061436.GA69817@laputa> <20210903022739.GB47953@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 Mon, Sep 06, 2021 at 04:41:55AM -0600, Simon Glass wrote: > Hi Heinrich, > > On Fri, 3 Sept 2021 at 10:30, Heinrich Schuchardt wrote: > > > > > > > > On 9/3/21 10:53 AM, Simon Glass wrote: > > > Hi Takahiro, > > > > > > On Thu, 2 Sept 2021 at 20:27, AKASHI Takahiro > > > wrote: > > >> > > >> Simon, > > >> > > >> On Thu, Sep 02, 2021 at 10:40:57AM -0600, Simon Glass wrote: > > >>> Hi Takahiro, > > >>> > > >>> On Tue, 31 Aug 2021 at 00:14, AKASHI Takahiro > > >>> wrote: > > >>>> > > >>>> Simon, > > >>>> > > >>>> On Sat, Aug 28, 2021 at 02:35:21PM -0600, Simon Glass wrote: > > >>>>> This is just a demonstration of how to support EFI loader using bootflow. > > >>>>> Various things need cleaning up, not least that the naming needs to be > > >>>>> finalised. I will deal with that in the v2 series. > > >>>>> > > >>>>> In order to support multiple methods of booting from the same device, we > > >>>>> should probably separate out the different implementations (syslinux, > > >>>>> EFI loader > > >>>> > > >>>> I still believe that we'd better add "removable media" support > > >>>> to UEFI boot manager first (and then probably call this functionality > > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > >> > > >>>> from bootflow?). > > >>>> > > >>>> I admit that, in this case, we will have an issue that we will not > > >>>> recognize any device which is plugged in dynamically after UEFI > > >>>> subsystem is initialized. But this issue will potentially exist > > >>>> even with your approach. > > >>> > > >>> That can be fixed by dropping the UEFI tables and using driver model > > >>> instead. I may have mentioned that :-) > > >> > > >> I'm afraid that you don't get my point above. > > >> > > >>>> > > >>>>> and soon bootmgr, > > >>>> > > >>>> What will you expect in UEFI boot manager case? > > >>>> Boot parameters (options) as well as the boot order are well defined > > >>>> by BootXXXX and BootOrder variables. How are they fit into your scheme? > > >>> > > >>> I haven't looked at boot manager yet, but I can't imagine it > > >>> presenting an insurmountable challenge. > > >> > > >> I don't say it's challenging. > > >> Since you have not yet explained your idea about how to specify > > >> the *boot order* in your scheme, I wonder how "BootXXXX"/"BootOrder" > > >> be treated and honored. > > >> There might be a parallel world again. > > > > > > Well as I mentioned, I haven't looked at it yet. The original question > > > was how to do EFI LOADER and I did a patch to show that. > > > > > > Are we likely to see mixed-boot environments, that use distro boot for > > > some OSes and EFI for others? I hope not as it would be confusing. EFI > > > is the parallel world, as I see it. > > > > > > It should be easy enough for the 'bootmgr' bootflow to read the EFI > > > variables and select the correct ordering. As I understand it, EFI > > > does not support lazy boot, so it is acceptable to probe all the > > > devices before selecting one? > > > > > >> > > >>>> > > >>>> But anyway, we can use the following commands to run a specific > > >>>> boot flow in UEFI world: > > >>>> => efidebug boot next 1(or whatever else); bootefi bootmgr > > >>> > > >>> OK. > > >>> > > >>> As you probably noticed I was trying to have bootflow connect directly > > >>> to the code that does the booting so that 'CONFIG_CMDLINE' can be > > >>> disabled (e.g. for security reasons) and the boot will still work. > > >> > > >> # Maybe, it sounds kinda chicken and egg. > > >> > > >> Even now, you can code this way :) > > >> > > >> efi_set_variable(u"BootNext", ..., u"Boot0001"); > > >> do_efibootmgr(); > > >> > > >> That's it. My concern is what I mentioned above. > > > > > > OK. But then you would need to export those functions. I think it > > > would be better to split up the logic a bit and move things out of the > > > cmd/ directory (at some point). > > > > > >> > > >> Just a note: > > >> In the current distro_bootcmd, UEFI boot manager is also called > > >> *every time* one of boot media in "boot_targets" is scanned/enumerated. > > >> But it will make little sense because the current boot manager only > > >> allows/requires users to specify both the boot device and the image file > > >> path explicitly in a boot option, i.e. "BootXXXX" variable, and tries > > >> all the boot options in "BootOrder" until it successfully launches > > >> one of those images. > > > > > > Yes, is the idea of lazy boot entirely impossible? Or is it still > > > possible to do that to some extent, e.g. by scanning until you find > > > the first thing in the boot order? > > > > > >> > > >>>> > > >>>>> Chromium OS, Android, VBE) into pluggable > > >>>>> drivers and number them as we do with partitions. For now the sequence > > >>>>> number is used to determine both the partition number and the > > >>>>> implementation to use. > > >>>>> > > >>>>> The same boot command is used as before ('bootflow scan -lb') so there is > > >>>>> no change to that. It can boot both Fedora 31 and 34, for example. > > >>>>> > > >>>>> Signed-off-by: Simon Glass > > >>>>> --- > > >>>>> See u-boot-dm/bmea for the tree containing this patch and the series > > >>>>> that it relies on: > > >>>>> > > >>>>> https://patchwork.ozlabs.org/project/uboot/list/?series=258654&state=* > > >>>>> > > >>> > > >>> [..] > > >>> > > >>>>> +static int efiload_read_file(struct blk_desc *desc, int partnum, > > >>>>> + struct bootflow *bflow) > > >>>>> +{ > > >>>>> + const struct udevice *media_dev; > > >>>>> + int size = bflow->size; > > >>>>> + char devnum_str[9]; > > >>>>> + char dirname[200]; > > >>>>> + loff_t bytes_read; > > >>>>> + char *last_slash; > > >>>>> + ulong addr; > > >>>>> + char *buf; > > >>>>> + int ret; > > >>>>> + > > >>>>> + /* Sadly FS closes the file after fs_size() so we must redo this */ > > >>>>> + ret = fs_set_blk_dev_with_part(desc, partnum); > > >>>>> + if (ret) > > >>>>> + return log_msg_ret("set", ret); > > >>>>> + > > >>>>> + buf = malloc(size + 1); > > >>>>> + if (!buf) > > >>>>> + return log_msg_ret("buf", -ENOMEM); > > >>>>> + addr = map_to_sysmem(buf); > > >>>>> + > > >>>>> + ret = fs_read(bflow->fname, addr, 0, 0, &bytes_read); > > >>>>> + if (ret) { > > >>>>> + free(buf); > > >>>>> + return log_msg_ret("read", ret); > > >>>>> + } > > >>>>> + if (size != bytes_read) > > >>>>> + return log_msg_ret("bread", -EINVAL); > > >>>>> + buf[size] = '\0'; > > >>>>> + bflow->state = BOOTFLOWST_LOADED; > > >>>>> + bflow->buf = buf; > > >>>>> + > > >>>>> + /* > > >>>>> + * This is a horrible hack to tell EFI about this boot device. Once we > > >>>>> + * unify EFI with the rest of U-Boot we can clean this up. The same hack > > >>>>> + * exists in multiple places, e.g. in the fs, tftp and load commands. > > >>>> > > >>>> Which part do you call a "horrible hack"? efi_set_bootdev()? > > >>>> In fact, there are a couple of reason why we need to call this function: > > >>>> 1. to remember a device to create a dummy device path for the loaded > > >>>> image later, > > >>>> 2. to remember a size of loaded image which is used for sanity check and > > >>>> image authentication later, > > >>>> 3. to avoid those parameters being remembered accidentally by "loading" dtb > > >>>> and/or other binaries than the image itself, > > >>>> > > >>>> I hope that (1) and (2) will be avoidable if we modify the current > > >>>> implementation (and bootefi syntax), and then we won't need (3). > > >>> > > >>> Yes thank you...I do understand why it is needed now, but it is > > >>> basically due to the the fat that EFI has its own driver structures. > > >>> Once we stop those, it will go away. > > >> > > >> Here, my point is, even under the current implementation, we will > > >> be able to eliminate efi_set_bootdev() with some tweaks. > > >> > > >> In other words, even you could integrate UEFI into the device model, > > >> the issue (2), for example, would still remain unsolved. > > >> In case of (2), we use the *size* information for sanity check against > > >> image's header information as well as calculating a hash value for > > >> UEFI secure boot when efi_load_image() is called. Even if the integration > > >> is done, we need to pass on the size information to "bootefi " > > >> command implicitly or explicitly. > > > > > > If you look at 'struct bootflow' you can see that it stores the size. > > > It can easily store other info as needed by EFI. So I don't think we > > > need that hack in the end, when things are using bootflow. > > > > > >> > > >>>> > > >>>>> + * Once we can clean up the EFI code to make proper use of driver model, > > >>>>> + * this can go away. > > >>>> > > >>>> My point is, however, that this kind of cleanup is irrelevant to > > >>>> whether we use driver model or not. > > >>> > > >>> Are you sure? Without driver model how are you going to reference a > > >>> udevice? If not that, how are you going to reference a device? The > > >>> tables in the UEFI implementation were specifically added to avoid > > >>> relying on driver model. It is a crying shame that I did not push back > > >>> harder on this at the time. > > >> > > >> I hope you will get my point in the previous comment now. > > > > > > Well yes I do, but I don't understand why there is such resistance to > > > sorting out the EFI implementation? It is quite a mess at the moment. > > > I think the first step is to drop the non-DM code, but the second is > > > to drop the parallel tables that EFI keeps about each device. > > > > > > Concerning these "parallel" tables. Please, have a look at the UEFI spec > > to understand what a handle, a protocol interface, and an event are. > > > > I also gave as short overview in > > https://archive.fosdem.org/2020/schedule/event/firmware_duwu/ starting > > at slide 10 or 08:00 of the video. > > > > Handles may refer to devices, to drivers or anything else. > > Handles are both to be created by U-Boot and by EFI binaries that we > > execute. > > > > On these handles protocol interfaces can be installed. These may be > > those defined in the UEFI spec or something completely independent. The > > protocol interfaces contain pointers to functions and additional data. > > > > Further objects are events. These may contain references to an event > > handler function that is called when the event is triggered. > > Thank you for the info. I did read the entire spec some years ago > (about 6 I think) and have dug into bits of it since. I do find the > naming confusing so I cannot claim to understand it. The use of void * > everywhere is a pain.. I will doubtless take another look. > > U-Boot does have devices, which have a (single) uclass and API. It > also has drivers. After all, the EFI tables are created from U-Boot > data structures, so presumably there is some correspondence. > > > > > None of the above objects matches one to one to objects in the driver > > model. But we can use some integration: > > > > When a U-Boot device is probed we must create the UEFI handle and > > install the expected protocols on it. > > > > When a U-Boot device is removed we must destroy the handle. There are > > certain conditions under which a protocol must not be removed from a > > handle and the handle cannot be destroyed. In this case removing a > > device should not be possible. > > I wish this had been done from the beginning, but we may as well start > now. It is quite a complicated task at this point. > > Can we pick one data structure that we can make ephemeral, using only > the DM structs and features? Can we look at what features need to be > added (such as partition devices?) and add those to U-Boot? > > What would be easiest as a starting point? How about struct > efi_system_partition? I have already proposed it as part of my more ambitious approach[1], in particular, patch#11,#12 and #13. If you want, I'm willing to re-post those (11-13 only). But it is not so much beneficial on its own because there are a lot of U-Boot interfaces that still take parameters, blk_desc + part_num. > Or perhaps could we drop efi_disk_register() and have it scan the disk > 'on the fly' when needed? Patch#13 does this. Heinrich, instead, suggested another way[2]. (Both have a lack for "removing-device" support.) Either way, we need to have some sort of mechanism (callback or binding) so that we can bridge UEFI world and U-Boot environment. -Takahiro Akashi [1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html > There must be something that can be done here. I am sure there is a > mismatch of concepts/API, but some of these mismatches are features > that U-Boot could add and some can be 'thinned down' so that UEFI is > not such a lot of code. > > Regards, > Simon