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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9CA39C433F5 for ; Thu, 30 Sep 2021 04:24:11 +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 0E52D61880 for ; Thu, 30 Sep 2021 04:24:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 0E52D61880 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 21E6A802A1; Thu, 30 Sep 2021 06:24:09 +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="srFSvl0n"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 636B78020E; Thu, 30 Sep 2021 06:24:07 +0200 (CEST) Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) (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 3B9148020E for ; Thu, 30 Sep 2021 06:24:00 +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-x102d.google.com with SMTP id pg10so2819526pjb.5 for ; Wed, 29 Sep 2021 21:24:00 -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=sZCvAXBrLnrIN0YdhII5327S9xENC/ce+KgId+4noyE=; b=srFSvl0nHveGMwq2ngPDt/yQxxJPs/qobsBP4anr/qq9H/jFE9jvBe+kHCDnlo5Inh zNiFNbFRnBxunBTa4AdajpCwo3onLVvqH5pKgvdEBsJ6z7pnsCOXlHksM/GQDfajrKMF kzcFcEfDQfDOdBLRsl+vz8I805/k46JIed05edTYo6EqBXVsPJZTlTLT59QWgg92pieR MSWWDTMMnMncFk8mN4tbEzo2a/EPZW44CiJ/ZWx1zJBWjjAJpos0NkX2PxUo4zPbxf/k vXUMzkfP94rgz1AGUJuGV9D2X1m4PZXTAZJJI0ku/PElrbZtXS087r3zVGOdnzEO67M/ ZEOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=sZCvAXBrLnrIN0YdhII5327S9xENC/ce+KgId+4noyE=; b=6moyDejMdK13O4Wgj3tJJ8pl7xEkJIHnghfuZam5i9z1rmXVrEagcNusofZMuQeh1E yZAddm9Jnm3NCUqjmpciBQPjsApoXHQ9BORP8msRMwe8120vMTJv5I+woNr2sjGj7Pgt +iJO9dofz2XZiemO4Iwho/thH/WMlUY1VWUaAnm+gjGRFSe9tTICVPRS6FtvXoxVAVBP ve75MnTvZCCSCn/eNzEWD1UhKtq3OdBOgclQfKnFqVuQl8W7cZN0SwoDg910A/07lfig AMyf+vK+7snBmITwIKpBfCMn+rpEigg1EupR1GL7XjDLrr0dbgQWbDotiItE4Oz+0ABf Zwtw== X-Gm-Message-State: AOAM5301H79RdKSyjpQdGdXx6OAaJsFZscl8gV+P7B/aTYLryrNWifNh jsIBEw7pzAIZDoV8u0MiZnwhgQ== X-Google-Smtp-Source: ABdhPJw4NXMyxOlzx+FXviNkV+io8uXgesDVqVhaoVFxmVVmwAyH9Z1TpdTz8U3dduxhJxYNXzFe9A== X-Received: by 2002:a17:902:b286:b0:13e:1a30:ebc6 with SMTP id u6-20020a170902b28600b0013e1a30ebc6mr2289271plr.72.1632975838246; Wed, 29 Sep 2021 21:23:58 -0700 (PDT) Received: from laputa (p6e421539.tkyea130.ap.so-net.ne.jp. [110.66.21.57]) by smtp.gmail.com with ESMTPSA id z10sm1181357pfn.70.2021.09.29.21.23.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Sep 2021 21:23:56 -0700 (PDT) Date: Thu, 30 Sep 2021 13:23:50 +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: <20210930042350.GA48553@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> <20210907035847.GA49004@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 Wed, Sep 29, 2021 at 10:08:48PM -0600, Simon Glass wrote: > Hi Takahiro, > > On Mon, 6 Sept 2021 at 21:58, AKASHI Takahiro > wrote: > > > > 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. > > Ilias pointed me to your patches from a few years ago that seemed to > get stuck in review. To me they point the way forward. Do you think > you could report the whole series? I have now resumed my previous work, with focus on better integration of U-Boot block devices and UEFI disks. You will see my RFC soon (later today or tomorrow?). -Takahiro Akashi > > - Simon > > > > > -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