Page MenuHomeFreeBSD

Dual-purpose loader.efi
AbandonedPublic

Authored by eric_metricspace.net on Dec 15 2017, 3:36 AM.

Details

Reviewers
imp
allanjude
Summary

This patch augments the logic for detecting the default filesystem, allowing loader.efi to function both as a replacement for boot1.efi as well as in its legacy capacity. This is intended to be the final fallback mechanism for loader.efi in the future, and the legacy compatibility is designed to facilitate a smooth transition.

This patch also allows GELI integration to proceed.

Test Plan

Testing has been done with both legacy and new operation on UFS and ZFS.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

imp accepted this revision.Dec 15 2017, 5:24 AM

As you may have guessed, I have issues with the extensive searching contemplated by this patch. I don't think it's a good idea. I've never had a system that was "saved" by it and had several where the wrong partition was "guessed" in an over zealous attempt to be bootable at all cost, but it wound up choosing something bad. I really do not wish to preserve it and wish to actively and specifically break anybody that relies on it.
There's parts I like that are nicely separate that I'll commit. Thanks for that cleanup.

stand/efi/libefi/devpath.c
97 ↗(On Diff #36621)

This can fail, and will cause a fault in the next line.

stand/efi/loader/main.c
182

space star, not star space, is the general convention.

224–259

I don't like searching like this. Load the file or don't, but don't guess at the system might be OK before trying.

248

Again, I hate these checks for an installed system. They aren't needed.

250

{ goes on the next line.

263–343

And this is a third copy of the same thing.

284

Can't we just load the kernel as the check? I know that will require that we rearrange things, but I do not think we should probe like this. We should try to load the kernel or have it fail.

303

I do not like this searching of all the devices. It actively gets in the way of properly implementing EFIBOOTMGR stuff, which is next on my hit parade.

305

you can't assume GPT partitioning.

337

and this is an even worse version of the above since it's simply a copy that doesn't have the calls to check_preferred.

381

I like this cleanup.

451

Again, I don't like this structure of searching. At most we should search for other partitions on the disk we booted from, and then stop.

526

We can likely lose this.

541

here is where we should look for BootCurrent and then BootXXXX. If we find BootXXXX, we should see if it contains additional paths. If so, we should jump to the additional paths straight away.

551

At the end of this loop, we should see if we have an arg that specifies a kernel to boot directly.

707

I think expanding the searching is not a good idea. It will get in the way of properly implementing the efibootmgr protocol where the searching is done in the BIOS not in the loader and if we can't boot this device, we should go to the next one. I know that's not what FreeBSD has done traditionally, but the traditional behavior is (a) not useful and (b) violates the standard.

This revision is now accepted and ready to land.Dec 15 2017, 5:24 AM
imp requested changes to this revision.Dec 15 2017, 5:25 AM

Sorry, that last one is 'request changes'

This revision now requires changes to proceed.Dec 15 2017, 5:25 AM
imp added a comment.Dec 15 2017, 5:37 AM

I just read my comments, and I think they are overly critical. I think this is a good next step, but with less searching...

The idea here is to implement what will eventually be a last-resort fallback mechanism (in the case of a blank install, or someone's EFI vars getting wiped, or something) as a means of starting the transition away from boot1. The legacy search behavior *should* be subsumed into the find_currdev_all path, but I'm unwilling to remove the legacy path completely at this point.

I'm personally most interested in getting GELI integrated; thus I didn't work on the EFI vars here, and I don't plan to. My aim here is to get something in place which can start the transition away from boot1 without breaking current functionality, which can be useful in some form in the long run, and which will allow GELI integration to proceed.

eric_metricspace.net marked 6 inline comments as done.Dec 15 2017, 1:29 PM
eric_metricspace.net added inline comments.
stand/efi/loader/main.c
263–343

This is the existing behavior, which is here to avoid breaking anyone's system. I think this needs to stay in place until the next major version.

541

I'm not going to do EFI vars handling in this changeset, but I agree, that ought to be the way it works eventually.

imp added inline comments.Dec 15 2017, 3:16 PM
stand/efi/loader/main.c
263–343

Normally I'd be right there with you on compatibility.

However, compatibility breaks things, including any possibility of efibootmgr failing over to the next thing. I'm not at all comfortable retaining a feature that's little used, causes actual problems today, is of at best dubious value and demonstrably gets in the way of bringing in standards compliant behavior.

541

Yea, I'm actually working on making that work now that I have efibootmgr in the tree and generating proper paths. I'll see if I can fast track it, as well as a trimmed down searching that goes with it. That will get you onto the GELI stuff I broke :)

eric_metricspace.net marked 2 inline comments as done.Dec 16 2017, 12:27 AM

I'm taking the broader architecture discussion here to -arch

eric_metricspace.net marked an inline comment as done.Dec 16 2017, 12:38 AM
eric_metricspace.net added inline comments.
stand/efi/loader/main.c
224–259

That's not really possible given the architecture of loader. It expects you to set the variables, then run a load procedure. The purpose here is to get a good guess of the boot partition.

imp added inline comments.Dec 16 2017, 12:47 AM
stand/efi/loader/main.c
224–259

That's possible through UEFI environment variables. Failure to do so and looking too hard violates the UEFI spec.

eric_metricspace.net marked 8 inline comments as done.Dec 16 2017, 12:52 AM
eric_metricspace.net added inline comments.
stand/efi/loader/main.c
263–343

Anyone with a legacy setup isn't going to have EFI vars set. If this is done as a fallback, it shouldn't cause problems.

303

This search will end up being a fallback mechanism. EFI bootmgr should take precedence, if the variables are actually defined.

451

tsoome will almost certainly complain about that, on behalf of everyone who boots from different disks from their ESP

707

The plan is this will only run if no previous mechanism (command-line args, efibootmgr) has succeeded.

cy added a subscriber: cy.Dec 16 2017, 1:47 AM
eric_metricspace.net marked 4 inline comments as done.Dec 19 2017, 2:48 AM
eric_metricspace.net edited the test plan for this revision. (Show Details)
eric_metricspace.net updated this revision to Diff 36746.

Simplified search procedure.

eric_metricspace.net marked 5 inline comments as done.Dec 19 2017, 2:49 AM

Fixed efi_zfs_is_preferred so ZFS preferred volumes are correctly detected again.

Accidental early rebase