Page MenuHomeFreeBSD

Rework UEFI ESP generation
ClosedPublic

Authored by bcran on Nov 11 2018, 7:07 PM.

Details

Summary

Currently, the installer uses pre-created 800KB FAT12 filesystems that it dd's onto the ESP partition.
This changeset improves that by having the installer generate a FAT32 filesystem directly onto the ESP using newfs_msdos and then copying loader.efi into /EFI/freebsd.
For live installs it then runs efibootmgr to add a FreeBSD boot entry in the BIOS.

Sponsored by: Netflix

Test Plan

Ran tinderbox build.

Tested on:

x86_64 Qemu, Mac Mini (2011?)
ARM64 Overdrive 1000
armv7 Beaglebone Black

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
imp added inline comments.Dec 11 2018, 3:20 AM
tools/boot/install-boot.sh
31–49 ↗(On Diff #51823)

This is better expressed as follows

get_uefi_bootname() {
case ${TARGET:-$(uname -m)}
amd64) echo bootx64.efi ;;
arm64) echo bootaa64.efi ;;
i386) echo bootia32.efi ;;
arm) echo bootarm.efi ;;
*) panic bogus ;;
esac
}

generally shell scripts echo things rather than set global variables. Also, using the alternative value variable substitution lets you omit the repetition.

55 ↗(On Diff #51823)

I'd add efibootfile here too, since it's just used in this function.

105–106 ↗(On Diff #51823)

Are these two locals? I'd add them to the local list if so. I don't see where else they are used.

331 ↗(On Diff #51823)

I'd also match ? here. getopts returns ? when there's no args in the getopt list that match.

tools/boot/rootgen.sh
172 ↗(On Diff #51823)

Picking one: it may make sense to define a global variable for this value, and use it throughout.

usr.sbin/bsdinstall/partedit/gpart_ops.c
34 ↗(On Diff #51823)

I'd be tempted to commit all the changes to this file separately.

718 ↗(On Diff #51823)

You don't need this space. The shell will just eat it when you use this later.

bcran marked 8 inline comments as done.Dec 12 2018, 1:53 AM
bcran updated this revision to Diff 51884.Dec 12 2018, 1:54 AM

Made changes per Warner's comments

bcran updated this revision to Diff 51934.Dec 12 2018, 9:20 PM

Switch to using $() instead of backticks.
If the installer doesn't provide any ESPs, look for unformatted 'efi' partitions instead, and use
them as ESPs.
WHen installing the UEFI boot entries, if there are multiple ESPs label each one using the device name:
e.g. FreeBSD (ada0p1)
If there's only a single ESP, just label the boot entry "FreeBSD".

jilles added a subscriber: jilles.Dec 12 2018, 11:08 PM
jilles added inline comments.
tools/boot/install-boot.sh
31–49 ↗(On Diff #51823)

Such an approach looks beautiful but may have performance and correctness implications since it runs the function in a subshell environment. I think setting variables (either named via a parameter or with a hard-coded name) is a perfectly acceptable approach.

imp added a comment.Dec 12 2018, 11:48 PM

looking better. Next round of suggestions shouldn't be so bad.

release/amd64/make-memstick.sh
44 ↗(On Diff #51934)

It might make sense to use mktemp to create a better name. That would allow multiple copies of this script to be run at the same time by different users (or the same one).
Both here and below (not specifically called out).

release/tools/vmimage.subr
36 ↗(On Diff #51934)

where is VMBASE added? This just makes an esp, but doesn't seem to create the resulting image in VMIMAGE.

tools/boot/install-boot.sh
36 ↗(On Diff #51934)

don't know if we have a 'panic' routine :)

47 ↗(On Diff #51934)

I'd be tempted to create constants for these magic numbers. Then the user of this file can also use them to prevent their proliferation.

31–49 ↗(On Diff #51823)

This function is called once, and in the context my suggestions are fine. They won't cause any sort of regression in performance, and it's more likely to be correct (though I didn't change the { to ( to enforce the new sub-shell out of a desire to be efficient).

tools/boot/rootgen.sh
411 ↗(On Diff #51934)

here, and elsewhere, should be lower-case k.

usr.sbin/bsdinstall/scripts/bootconfig
44 ↗(On Diff #51934)

how can this ever be true? We only ever set X86_BOOTMETHOD foor amd64 and i386.

bcran marked 5 inline comments as done.Dec 13 2018, 5:42 AM
bcran added inline comments.
release/tools/vmimage.subr
36 ↗(On Diff #51934)

Good point - not sure how I removed the mkimg command.

usr.sbin/bsdinstall/scripts/bootconfig
44 ↗(On Diff #51934)

I'm checking if the machine is arm64 _or_ the x86 bootmethod is UEFI.
So it's going to be true if running under arm64 or a amd64 or i386 system is booted using UEFI.

bcran marked 6 inline comments as done.Dec 13 2018, 5:48 AM
bcran added inline comments.
tools/boot/install-boot.sh
36 ↗(On Diff #51934)

No, but we do have die

bcran updated this revision to Diff 51947.Dec 13 2018, 5:49 AM
bcran marked an inline comment as done.

Fixed latest review comments

bcran updated this revision to Diff 51975.Dec 13 2018, 10:23 PM

Remove more of the 33292 magic numbers

bcran updated this revision to Diff 51976.Dec 13 2018, 10:25 PM
This comment was removed by bcran.
bcran updated this revision to Diff 51977.Dec 13 2018, 10:27 PM

Remove the unintended change of updating MAXWAIT

Harbormaster completed remote builds in B21551: Diff 51977.
imp added inline comments.Dec 14 2018, 4:35 PM
release/tools/vmimage.subr
38 ↗(On Diff #51977)

I'd think should do the ${espfilename} dance too, with the rm after... I'm not thrilled about lua here, but I'm not sure what would be better given the context this script runs in.

share/man/man8/uefi.8
27 ↗(On Diff #51977)

December :)

tools/boot/install-boot.sh
13 ↗(On Diff #51977)

Add a comment that says what units these are in?

bcran marked 5 inline comments as done.Dec 14 2018, 6:05 PM
bcran added inline comments.
release/tools/vmimage.subr
38 ↗(On Diff #51977)

Yeah, me neither.

bcran updated this revision to Diff 52009.Dec 14 2018, 6:06 PM
bcran marked an inline comment as done.

Changes per @imp's comments

bcran updated this revision to Diff 52016.Dec 14 2018, 7:25 PM

Use ${fat32min} in vmimage.subr too

bcran updated this revision to Diff 52017.Dec 14 2018, 7:37 PM

Improve the usage() output of tools/boot/install-boot.sh

bcran updated this revision to Diff 52041.Dec 15 2018, 3:32 AM

Don't use DESTDIR in tools/boot/install-root.sh: it causes bad stuff to happen
since it's that file is now sourced into other scripts. For example,
"make vm-release" installs files to / !

Improve make_esp_device to add lots of error checking. Use fstyp instead of trying to mount
the filesystem first. If the UEFI boot entry already exists, don't create another one.
If /EFI/BOOT/BOOTxx.EFI exists and is the FreeBSD boot1.efi (i.e. it contains the string
"FreeBSD EFI boot block") then delete it.

Other fixes.

bcran updated this revision to Diff 52044.Dec 15 2018, 5:48 AM

Fixed whitespace issues (tab vs space) and added informational messages

bcran updated this revision to Diff 52045.Dec 15 2018, 6:00 AM

Don't run mount twice

bcran updated this revision to Diff 52105.Dec 17 2018, 3:55 PM

Changes to fix issues found by devel/hs-ShellCheck .

imp added inline comments.Dec 17 2018, 4:31 PM
tools/boot/install-boot.sh
110 ↗(On Diff #52105)

I would be tempted to go a slightly more conservative route of renaming it instead. That would allow someone to recover back to this if they have a bad new loader.

143 ↗(On Diff #52105)

This looks weird... What are you trying to accomplish?

369 ↗(On Diff #52105)

This doesn't look right at all. srcroot seems to be /usr/src, while DESTDIR is where we install into... Is that intentional? If so, that seems like a poorly named variable.

usr.sbin/bsdinstall/partedit/gpart_ops.c
714 ↗(On Diff #52105)

Does this need protection against multiple invocations?

bcran added inline comments.Dec 17 2018, 4:37 PM
tools/boot/install-boot.sh
110 ↗(On Diff #52105)

The problem is that many systems will o my have 800KB available on the ESP. So in those situations it’s not possible to have more than one copy.

143 ↗(On Diff #52105)

Oh that needs a comment.
efibootmgr doesn’t mark new entries active, so this code figures out what number it is and runs the command to activate it.

369 ↗(On Diff #52105)

Here, srcroot is where we’re copying files from, to install onto the ESP.

bcran updated this revision to Diff 52117.Dec 17 2018, 5:41 PM

Address latest review comments

bcran edited the summary of this revision. (Show Details)Dec 17 2018, 5:42 PM
bcran edited the test plan for this revision. (Show Details)
bcran updated this revision to Diff 52118.Dec 17 2018, 5:51 PM

Check if there's space to keep BOOTxx.efi around, and if so rename is to BOOTxx-old.efi.

bcran updated this revision to Diff 52119.Dec 17 2018, 6:12 PM

Also keep a copy of loader.efi as loader-old.efi in case there's a problem with the new one.

bcran marked 3 inline comments as done.Dec 18 2018, 7:21 PM
bcran added inline comments.
tools/boot/install-boot.sh
143 ↗(On Diff #52105)

I'm trying to see if there's an existing boot entry - filtering out other OSes that use different paths to their loader. If there is, we don't need to create a new one.

bcran edited the test plan for this revision. (Show Details)Dec 19 2018, 3:56 AM
bcran marked an inline comment as done.Dec 19 2018, 3:58 AM
bcran added inline comments.
usr.sbin/bsdinstall/partedit/gpart_ops.c
714 ↗(On Diff #52105)

I don't think so: from what I've seen bsdinstall isn't designed to have multiple copies running at the same time.

imp accepted this revision.Dec 19 2018, 4:07 AM

I think this is about as good as a review this size gets :).
I'd be happier if nwhitehorn OK's it before you commit.

This revision is now accepted and ready to land.Dec 19 2018, 4:07 AM
This revision was automatically updated to reflect the committed changes.
bcran marked an inline comment as done.