Page MenuHomeFreeBSD

Add a new efi-update-loader script and associated man page
Needs ReviewPublic

Authored by bcran on Mar 15 2019, 3:20 AM.

Details

Summary

The efi-update-loader script looks for efi type partitions on each disk associated with the root filesystem, looking at UFS and ZFS mirror, stripe, raid etc. configurations.
Its primary use is expected to be during 'installworld' and 'freebsd-update' to automatically update the loader that the UEFI system firmware uses to boot FreeBSD.

Unless the user specifies a device to use, it will use all such ESPs, and if there's an existing /EFI/FreeBSD/loader.efi it will copy /boot/loader.efi (or the otherwise
specified loader file) to /EFI/FreeBSD/loader.efi and backup any existing file as loader-old.efi if the new loader.efi isn't the same as the old one and there's available space to do so.
If there's a FreeBSD loader/boot1 as /EFI/BOOT/BOOT{arch}.efi it will back it up to /EFI/FreeBSD/BOOT{arch}-old.efi (if there's space) and copy the new loader to /EFI/BOOT/BOOT${arch}.efi.

Test Plan

Tested on a couple of amd64 systems, both against the real root filesystem (ZFS) and against a md configuration of zfs mirror, raidz, ufs mirror and non-redundant configurations.
Tested on an armv6 board (BeagleBone Black) and an aarch64 system (Overdrive 1000).

Note that on armv6 and other installations with MBR disk layouts, the autodetection will fail to find any devices because the partition type isn't 'efi'.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23225
Build 22268: arc lint + arc unit

Event Timeline

bcran created this revision.Mar 15 2019, 3:20 AM
bcran added a comment.Mar 15 2019, 5:09 AM

I'm thinking that instead of looking at all disks in the system it should perhaps only check the disk that contains the root filesystem.
That would avoid the case where for example the script would try and update the install media plugged into a USB port.

I'm thinking that instead of looking at all disks in the system it should perhaps only check the disk that contains the root filesystem.
That would avoid the case where for example the script would try and update the install media plugged into a USB port.

I think, what you need to establish first is what this script is for and what are the use cases. If we are running it from installer, it must only install boot programs on target disk(s). If it is meant to be used as generic update current boot disks, then it has to discover disks related to current root file system. The least we want to happen is that some random disk will get updated.

bcran added a comment.Mar 15 2019, 3:12 PM

I think, what you need to establish first is what this script is for and what are the use cases. If we are running it from installer, it must only install boot programs on target disk(s). If it is meant to be used as generic update current boot disks, then it has to discover disks related to current root file system. The least we want to happen is that some random disk will get updated.

I forgot to explain that. This script would be run from make installworld or freebsd-update to update an ESP that's expected to already have a FreeBSD boot1.efi or loader.efi on it.
I need to update the diff to look at the output of gmirror status, zpool status etc. to work out which ESP(s) to update. In a mirror situation there may be multiple ESPs which should be updated.

I think, what you need to establish first is what this script is for and what are the use cases. If we are running it from installer, it must only install boot programs on target disk(s). If it is meant to be used as generic update current boot disks, then it has to discover disks related to current root file system. The least we want to happen is that some random disk will get updated.

I forgot to explain that. This script would be run from make installworld or freebsd-update to update an ESP that's expected to already have a FreeBSD boot1.efi or loader.efi on it.
I need to update the diff to look at the output of gmirror status, zpool status etc. to work out which ESP(s) to update. In a mirror situation there may be multiple ESPs which should be updated.

Okay, thats cool - yes, in those cases we need to determine the targets. Also there are not only mirror, but raidz too (I'm using raidz on my boot pool;).

bcran updated this revision to Diff 55146.Mar 17 2019, 2:37 AM

Changed the autodetect code to look for all ESPs associated with the root filesystem.

It now understands zfs (stripe, raidz etc.), and ufs including mirror, raid, stripe etc.

bcran updated this revision to Diff 55147.Mar 17 2019, 3:06 AM
  • Fix usage output, try and improve some variable names
bcran edited the summary of this revision. (Show Details)Mar 17 2019, 3:12 AM
bcran edited the test plan for this revision. (Show Details)
bcran added a reviewer: manu.
bcran edited the summary of this revision. (Show Details)Mar 17 2019, 3:16 AM
manu added inline comments.Mar 17 2019, 3:21 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
174

You could test directly EFI/FreeBSD and mkdir -p

181

This will break system that don't have persistant variable, for those we need to use EFI/BOOT/boot${arch}.efi

221

On some system you will not have efirt for variable, can efibootmgr test this ?

imp added a comment.Mar 17 2019, 3:21 AM

partial review.... more to come

usr.sbin/efi-update-loader/efi-update-loader.8
36

It might be nice to also accept an ESP directly to install it into, as well as auto detect.
As well as having the full unix path for ESPs that are already mounted.

45

You need markup on loader.efi

62

You need markup on these file names.

usr.sbin/efi-update-loader/efi-update-loader.sh
45

I'd be tempted to line the echos up with the echo below.

78

Does this cope with boot1.ef?

79

You can return 0 here.

82

and return 1 here.

178

If you make the changes I suggested, this can turn into a simple

if bootefi_is_freebsd; then

which seems less convoluted to me.

imp added inline comments.Mar 17 2019, 3:27 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
221

It doesn't today, but we can do the right thing with efivar if uboot's EFI rejects permanent changes to variables.

kevans added inline comments.Mar 17 2019, 3:41 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
221

I think we also need to test this with bhyve+UEFI, but I suspect (IIRC) both implementations accept the UEFI vars and they just don't persist.

bcran added inline comments.Mar 17 2019, 4:17 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
221

So in that case we definitely do _not_ want to remove the EFI/BOOT/BOOT{$arch}.efi file.
And for new installs, I also need to check the changes I committed a few weeks ago to install /EFI/FreeBSD/loader.efi only. I seem to recall @imp was against having the BOOT${arch}.efi file since it's supposedly only for removable media, but there are obviously several situations where it's needed for booting from fixed media too.

imp added inline comments.Mar 17 2019, 6:01 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
34

[ -n ${verbose} ] && echo $*

should suffice. You don't need the extra stuff $@ does, nor the extra quotes, nor even the if.

bcran marked 8 inline comments as done.Mar 17 2019, 4:36 PM
bcran added inline comments.
usr.sbin/efi-update-loader/efi-update-loader.sh
78

Was "boot1.ef" a typo? It copes with boot1.efi, but not currently loader.efi. Since we'll want to keep this file, I'll update it to check for strings that are in loader.efi too.

bcran added inline comments.Mar 18 2019, 3:11 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
34

Except since the script is set -e, that then needs a || true to avoid the statement causing an error.

bcran updated this revision to Diff 55182.Mar 18 2019, 3:34 AM

Address review comments.
Only ever update /EFI/BOOT/BOOT${arch}.efi and /EFI/FreeBSD/loader.efi if they already exist and are
the BOOT${arch}.efi file is a FreeBSD boot1.efi or loader.efi.

Remove the lines about running efibootmgr.
Rework the copyloader function to better work with situations where the ESP is low on free space - for example
this will be the case with most existing installations which only have an 800KB filesystem. Since the loader.efi
is over 400KB, it won't be possible to do backups. The script now makes backups by copying the existing files to
/tmp/espback.XXXXXX and then, if there's space left, copying them back onto the ESP.

bcran edited the summary of this revision. (Show Details)Mar 18 2019, 3:35 AM
bcran marked 5 inline comments as done.
bcran added inline comments.
usr.sbin/efi-update-loader/efi-update-loader.sh
181

In the latest revision of the script I've changed it so that if /EFI/BOOT/BOOT${arch}.efi exists, it will copy loader.efi to that file.
In addition, if EFI/FreeBSD/loader.efi exists, it will also copy loader.efi to that file. I think that should work to retain any boot functionality.

bcran marked an inline comment as done.Mar 18 2019, 3:39 AM
bcran added inline comments.
usr.sbin/efi-update-loader/efi-update-loader.8
36

The script already does accept an ESP directly to install into - via the -d option.
I agree having an option to specify a unix path for an already mounted ESP would be nice to have too.

bcran added inline comments.Mar 18 2019, 3:45 AM
usr.sbin/efi-update-loader/efi-update-loader.sh
45

In the latest revision they look lined up in QtCreator, but for some reason they're wrong here - I'm using tabs, not spaces.

kevans added inline comments.Mar 19 2019, 3:14 PM
usr.sbin/efi-update-loader/efi-update-loader.sh
163

This should take into an account an ESP we're copying to already having been mounted. For some images, it'll be premounted at /boot/msdos (or something relevant, like /boot/efi or /efi) and we probably should just use that mountpoint rather than mounting/unmounting it. We'll need to be careful about not unmounting it in cleanup, too, if that's the case.

291

This block should be redundant now; we've guaranteed just above that we either have non-zero user supplied ${esps} or we failed to autodetect ESP and bailed out with the "Error: could not detect ESP containing FreeBSD loader to update"

bcran updated this revision to Diff 55265.Mar 20 2019, 12:26 AM
  • Allow an ESP that's already mounted to be updated.
bcran marked 2 inline comments as done.Mar 20 2019, 12:27 AM
bcran added inline comments.
usr.sbin/efi-update-loader/efi-update-loader.sh
163

Good point. I've fixed this in the latest revision and tested on a Beaglebone Black where the ESP is mounted on /boot/msdos.

291

Thanks, I've fixed it in the latest diff.

bcran marked 3 inline comments as done.Mar 20 2019, 12:28 AM
bcran added inline comments.
usr.sbin/efi-update-loader/efi-update-loader.8
36

I've added a -p path option to the latest revision of the script and man page.

bcran updated this revision to Diff 55268.Mar 20 2019, 3:52 AM
  • Bug fixes
bcran edited the test plan for this revision. (Show Details)Mar 20 2019, 3:53 AM
kevans added inline comments.Mar 20 2019, 5:48 PM
usr.sbin/efi-update-loader/efi-update-loader.sh
183

I think with this approach, you'll need to also set some local indicator that esppath should be cleared out just after you don't unmount/rmdir the mntpt. Otherwise, subsequent copyloader will suddenly fail as we can't clobber the already-mounted ESP that we don't want to touch.

This is likely really really edge-casey, though...

tsoome added inline comments.Mar 20 2019, 5:50 PM
usr.sbin/efi-update-loader/efi-update-loader.sh
184

what if it was mounted RO?

kevans added inline comments.Mar 20 2019, 6:52 PM
usr.sbin/efi-update-loader/efi-update-loader.sh
184

I guess we should keep track of auto-detected ESP vs. user-specified ESP. If user-specified it, we should probably re-mount it R/W and then return it to R/O when we're done, no?

bcran added inline comments.Mar 20 2019, 7:21 PM
usr.sbin/efi-update-loader/efi-update-loader.sh
183

Good point.

184

Yeah, we probably should do that, instead of erroring out if it's mounted RO.

bcran updated this revision to Diff 55304.Mar 21 2019, 1:54 AM
  • Allow use of an ESP that's mounted read-only.
bcran updated this revision to Diff 55305.Mar 21 2019, 2:08 AM
bcran marked 2 inline comments as done.

Fix bugs caused by setting ${esppath} and not unsetting ${espdev}.

I frequently cross-build amd64->aarch64 and amd64->armv7 and also do
installworld's of the aarch64 and armv7 worlds into directories trees on the
amd64 machine ( same file system as the amd64 world is on, but in something
like, for example, /usr/obj/DESTDIRs/clang-cortexA53-installworld-poud ).
(Later such are used via qemu-user-static and poudriere-devel , for example.)

I may have missed it, but will the script or its use during installworld avoid
updating the amd64 EFI area when cross-installing a world that is not
intended to be bootable? If yes, how?

I may have missed it, but will the script or its use during installworld avoid
updating the amd64 EFI area when cross-installing a world that is not
intended to be bootable? If yes, how?

I was planning to avoid it by checking ${DESTDIR}, but others have suggested not adding it to installworld at all, but having users run it as an additional manual step instead.