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 <[email protected]> wrote: > > > > > > > > On 9/3/21 10:53 AM, Simon Glass wrote: > > > Hi Takahiro, > > > > > > On Thu, 2 Sept 2021 at 20:27, AKASHI Takahiro > > > <[email protected]> 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 > > >>> <[email protected]> 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 <[email protected]> > > >>>>> --- > > >>>>> 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. -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
