Page MenuHomeFreeBSD

Split boot1 in half and use the useful half for gptboot.efi
ClosedPublic

Authored by imp on Jun 7 2019, 2:24 AM.

Details

Summary

Break out the disk selection protocol from the rest of boot1.

Segregate the disk probing and selection protocol from the rest of the
boot loader.

Create gptboot.efi

This is a primary boot loader that is intended to implement the
gptboot partition selection algorithm just like we did for BIOS
booting. While the preferred method for UEFI is to use the UEFI Boot
Manager protocol, there are situations where that can't be done: some
BIOS makers interfere with the protocol in unhelpful ways, there's a
new standard for a zero variable write from the client OS, and finally
for USB drives that might be mobile between systems with multiple
partitions there needs to be a media stable way to select.

Test Plan

Integrate it into Netflix's OCA firmware, do some upgrades, watch it boot the right partition :)

Yes, I know that some people may not like this.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp created this revision.Jun 7 2019, 2:24 AM
imp edited the test plan for this revision. (Show Details)Jun 7 2019, 2:27 AM
imp added reviewers: bcran, tsoome, kevans.
bcran added inline comments.Jun 8 2019, 2:35 AM
stand/efi/boot1/boot1.c
232 ↗(On Diff #58344)

It's possible for platforms to not produce the EFI_BLOCK_IO_PROTOCOL protocol, so we should check for errors here.

stand/efi/boot1/proto.c
43 ↗(On Diff #58344)

Can we depend on OpenProtocol being available? The UEFI Specification says that should be used in preference to HandleProtocol:

EFI 1.10 Extension
The HandleProtocol() function is still available for use by old EFI applications and drivers.
However, all new applications and drivers should use EFI_BOOT_SERVICES.OpenProtocol()
in place of HandleProtocol(). The following code fragment shows a possible implementation of
HandleProtocol() using OpenProtocol(). The variable EfiCoreImageHandle is the image
handle of the EFI core.

stand/efi/gptboot/Makefile
1 ↗(On Diff #58344)

Are MK_LOADER_ZFS=no and NOFAT=yes supposed to be left in?

stand/efi/gptboot/proto.c
114 ↗(On Diff #58344)

Typo ("parittion").

150 ↗(On Diff #58344)

Not sure the final "block" is needed?

171 ↗(On Diff #58344)

Is "WTF?" suitable for end-users to see?

194 ↗(On Diff #58344)

I do wish style(9) required brackets even when they're not needed - I don't like not having them here and in other places. Especially after the "goto fail" saga.

imp added inline comments.Jun 8 2019, 5:28 AM
stand/efi/boot1/boot1.c
232 ↗(On Diff #58344)

This loader is useless on such platforms, but having an error message is better.

stand/efi/boot1/proto.c
43 ↗(On Diff #58344)

Beyond the scope of this change. This code is copied from boot1. We use OpenProtocol in one place and HandleProtocol in 22.

stand/efi/gptboot/Makefile
1 ↗(On Diff #58344)

Yup. We absolutely do not support ZFS with this, and never will. Also, we don't need that hacky boot1.efifat thing for this. That stuff should die there and I added NOFAT so we wouldn't build it here.

stand/efi/gptboot/proto.c
150 ↗(On Diff #58344)

No, it's not.

171 ↗(On Diff #58344)

Fair point. Will revise.

194 ↗(On Diff #58344)

Yea, I guess we'll disagree here. I find the extra {} confusing for simple things like this.

imp added inline comments.Jun 8 2019, 5:34 AM
stand/efi/boot1/boot1.c
232 ↗(On Diff #58344)

also, this is just code motion, I'd like to do that as a followup.

tsoome accepted this revision.Jun 8 2019, 5:34 AM
tsoome added inline comments.
stand/efi/boot1/proto.c
43 ↗(On Diff #58344)

I agree that we should move towards OpenProtocol but not necessarily here.

The obvious question is about obsoleting the support for pre EFI 1.1, but as we already have place with OpenProtocol.... :)

This revision is now accepted and ready to land.Jun 8 2019, 5:34 AM
imp added inline comments.Jun 8 2019, 5:40 AM
stand/efi/boot1/boot1.c
232 ↗(On Diff #58344)

finally, so I remember it later, the proper check is for hsize still == 0, which it will be if EFI_BLOCK_IO_PROTOCOL isn't implemented.

imp added inline comments.Jun 8 2019, 5:43 AM
stand/efi/boot1/boot1.c
240 ↗(On Diff #58344)

And we check the status down here...

bcran added inline comments.Jun 8 2019, 6:09 AM
stand/efi/boot1/boot1.c
232 ↗(On Diff #58344)

Yeah, checking if hsize is still zero sounds like the best approach.

stand/efi/boot1/proto.c
43 ↗(On Diff #58344)

Agreed on it being out of scope for this change.

bcran accepted this revision.Jun 8 2019, 6:09 AM
imp added inline comments.Jun 8 2019, 5:23 PM
stand/efi/boot1/proto.c
43 ↗(On Diff #58344)

I'm pretty sure we couldn't boot on an EFI 1.0 system anyway... Do we know of any such beasts in the wild? My 64-bit macbook was 1.1 IIRC.

This revision was automatically updated to reflect the committed changes.