Rework UEFI ESP generation

[bcran]

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

[imp]

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]

Made changes per Warner's comments

[bcran]

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]

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]

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]

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]

tools/boot/install-boot.sh

36 ↗

(On Diff #51934)

No, but we do have die

[bcran]

Fixed latest review comments

[bcran]

Remove more of the 33292 magic numbers

[bcran]

This comment was removed by bcran.

[bcran]

Remove the unintended change of updating MAXWAIT

[imp]

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]

release/tools/vmimage.subr

38 ↗

(On Diff #51977)

Yeah, me neither.

[bcran]

Changes per @imp's comments

[bcran]

Use ${fat32min} in vmimage.subr too

[bcran]

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

[bcran]

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]

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

[bcran]

Don't run mount twice

[bcran]

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

[imp]

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]

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]

Address latest review comments

[bcran]

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

[bcran]

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

[bcran]

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]

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]

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.