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 > <takahiro.aka...@linaro.org> 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 <xypron.g...@gmx.de> > > > wrote: > > > > > > > > > > > > > > > > On 9/3/21 10:53 AM, Simon Glass wrote: > > > > > Hi Takahiro, > > > > > > > > > > On Thu, 2 Sept 2021 at 20:27, AKASHI Takahiro > > > > > <takahiro.aka...@linaro.org> 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 > > > > >>> <takahiro.aka...@linaro.org> 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 <s...@chromium.org> > > > > >>>>> --- > > > > >>>>> 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 <addr>" > > > > >> 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