Hi Takahiro, On Sun, 5 Sept 2021 at 19:47, AKASHI Takahiro <takahiro.aka...@linaro.org> wrote: > > Hi Simon, > > On Fri, Sep 03, 2021 at 02:53:52AM -0600, 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. > > Yeah, I absolutely agree here. > That is why I constantly insist "removable media" support should be > implemented in UEFI boot manager in the first place. Please remember > that "removable media" support is part of UEFI specification, > UEFI boot manager and hence "EFI LOADER". > > # By "removable media" support, I mean that the image to be loaded > # is determined by searching for the default file path, or /EFI/BOOT/xxx.efi, > # in the system partition. The media can be, say, a fixed (non-removable) > # HD on the system. > # This is definitely part of UEFI environment. >
OK. > > 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). > > If we don't take "mixed-boot environments", adding efibootmger to > bootflow mechanism, or implementing efibootmger using bootflow, > makes little sense. I'm not convinced of that, actually. Let's leave the discussion for when bootflow is a little closer. I think it should be possible to boot without needing CONFIG_CMDLINE, for example. But even without that, a unified approach to what is available to boot and what to select could be quite helpful I think. It can support custom flows too, such as Chrome OS. > > > > > > > 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? > > If I correctly understand what 'lazy boot' means here, you can do > all the enumeration/scanning first, then call UEFI boot manager: > => scsi rescan; usb start; mmc rescan; (and whatever else); bootefi bootmger I mean scan the first device, try to boot it. If that doesn't work, scan the next one. It can make the boot a lot faster. > > The order of scanning commands doesn't matter as "BootOrder" determine > the priorities among BootXXXX (boot options). > What the current UEFI cannot provides is to detect a new device that > is attached to the board once UEFI subsystem is initialized. > > That issue, which I have mentioned in the ML from time to time, should be > separately resolved as part of "integration" efforts between UEFI objects > and driver model. Yes, this limitation is a side effect of the implementation method. I will try to watch out for additional 'parallel' tables being added, but I would like to see some effort to reduce what is there. > > > > > > > > > > > > > > > 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. > > I'm afraid that you miss the context of my discussion. > We support a command sequence like the following: > => load scsi 0:1 50000000 /hellowworld.efi > => bootefi 50000000 > > distro_bootcmd does use this method to add "removable media" support, > but even if the distro script is re-implemented using bootflow machanism, > we cannot remove "horrible hack" in load command as long as we continue > to use load command. Actually 'bootflow' does not use the load command so I think it is a path forward here. > > > > > > > > > > > > > > > + * 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. > > As you know, I have a positive opinion to the integration. > What I object to here is to integrate UEFI boot manager into bootflow. Well I have not looked at this yet and it is not my immediate focus. For now I can just make bootflow use the 'bootmgr' command in that case. > > > 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. > > As Heinrich suggested in his reply, there are many concepts that might > not be easily mapped to U-Boot counterparts. If our only concern is on > the boot flow, it would be easier, but we would also like to support > a lot of API, either boot/runtime services or protocols. Filling > semantic gaps would make it harder to completely drop some sort of > "parallel tables". But I'm not asking for that. In fact I sent Heinrich an email recently asking for just one thing to be removed, as a starting point. > > I think that the first thing that we should tackle is to eliminate > the issue, as I mentioned above, regarding "block devices". > It would shed light on how we could go further on this issue. Do you mean drop the parallel tables for block devices? If so, yes that sounds good. Regards, Simon