[PATCH] RFC: Support an EFI-loader bootflow
Simon Glass
sjg at chromium.org
Thu Sep 30 06:08:48 CEST 2021
Hi Takahiro,
On Mon, 6 Sept 2021 at 21:58, AKASHI Takahiro
<takahiro.akashi at 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.glpk at 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.akashi at 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.akashi at 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 <sjg at 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?
- 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
More information about the U-Boot
mailing list