Page MenuHomeFreeBSD
Differential D55469

bsdinstall: fix EFI boot entry creation
ClosedPublic
Actions

Authored by vexeduxr on Mon, Feb 23, 8:40 PM.
Tags
None
Referenced Files
F145898388: D55469.id172553.diff
Wed, Feb 25, 9:48 PM
F145895623: D55469.id172544.diff
Wed, Feb 25, 8:49 PM
F145895431: D55469.id172544.diff
Wed, Feb 25, 8:44 PM
Unknown Object (File)
Wed, Feb 25, 10:27 AM
Unknown Object (File)
Wed, Feb 25, 8:33 AM
Unknown Object (File)
Wed, Feb 25, 7:50 AM
Unknown Object (File)
Wed, Feb 25, 7:46 AM
Unknown Object (File)
Wed, Feb 25, 7:42 AM
View All 15 Files
Subscribers
cperciva
jrtc27

Details

Reviewers
imp
manu
jrtc27
Commits
rG5425bdae0970: bsdinstall: fix EFI boot entry creation
rGcfdd90abab51: bsdinstall: fix EFI boot entry creation
rG0fd91c489134: bsdinstall: fix EFI boot entry creation
Summary

update_uefi_bootentry assumes that the caller sets FREEBSD_BOOTNAME and
mntpt, which isn't the case anymore. The result is that there is no
"FreeBSD" boot entry created/updated after install. Most machines manage
to boot from the removable media path (if the loader is installed there
too), but some don't.

Take the loader's path as an argument and rename the variable used in
the ZFS mirror loop so mntpt can be reused below.

Also mark nentries as a local variable so it doesn't leak out of the
function.

PR: 293385
Fixes: 494de51bc0074472d1b01604f085daea0844f240
MFC after: 2 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 70968
Build 67851: arc lint + arc unit

Event Timeline

vexeduxr created this revision.Mon, Feb 23, 8:40 PM
Herald added a subscriber: jrtc27. · View Herald TranscriptMon, Feb 23, 8:40 PM
vexeduxr requested review of this revision.Mon, Feb 23, 8:40 PM
Harbormaster completed remote builds in B70931: Diff 172544.Mon, Feb 23, 8:40 PM
jrtc27 added a comment.Mon, Feb 23, 8:52 PM

update_uefi_bootentry assumes that the caller sets FREEBSD_BOOTNAME and
mntpt, which isn't the case anymore

It would have been helpful to reference 494de51bc0074472d1b01604f085daea0844f240 as the "anymore" point, which unfortunately seems to have been in 15.0 via 4b734c47744611d1d2bd61d15045b7903d4c35be. What is the effect of this bug?

usr.sbin/bsdinstall/scripts/bootconfig
86–88

This change isn't mentioned in the commit message

87

uefi_copy_loader takes full paths, why not do so here too?

jrtc27 added a comment.Mon, Feb 23, 8:53 PM

NB: It's also about to be in 14.4 via 829e479a0a37eb72023ce361f5b2379d82f8bc2a

vexeduxr added a reviewer: jrtc27.Mon, Feb 23, 9:46 PM
In D55469#1269123, @jrtc27 wrote:

What is the effect of this bug?

The end result is that there's no "FreeBSD" boot entry after a fresh install. The effects would vary by setup.
Assuming FreeBSD is the only OS installed, the firmware would boot from the removable media path (although apparently that's not a guarantee with some buggy BIOSes)
For dual boot setups an entry would have to be created manually.

In D55469#1269128, @jrtc27 wrote:

NB: It's also about to be in 14.4 via 829e479a0a37eb72023ce361f5b2379d82f8bc2a

Ah, I didn't realize 14.4 was so soon. I'll shorten the MFC tag.

vexeduxr updated this revision to Diff 172549.Mon, Feb 23, 9:47 PM

Provide the full loader path as an argument, instead of just the file name.

Harbormaster completed remote builds in B70936: Diff 172549.Mon, Feb 23, 9:47 PM
vexeduxr edited the summary of this revision. (Show Details)Mon, Feb 23, 9:48 PM
cperciva added a subscriber: cperciva.Mon, Feb 23, 10:14 PM

BTW we're going to need this ASAP if it's going to land in 14.4.

imp accepted this revision.Mon, Feb 23, 11:20 PM
imp added inline comments.
usr.sbin/bsdinstall/scripts/bootconfig
171

This won't work, i don't think. It's the loader in the chroot, not the loader in the esp. Efibootmgr doesn't copy, but just uses the full path to get the device underlying the file in uefi terms.

This revision is now accepted and ready to land.Mon, Feb 23, 11:20 PM
vexeduxr added inline comments.Mon, Feb 23, 11:38 PM
usr.sbin/bsdinstall/scripts/bootconfig
171

The ESP should be mounted at $BSDINSTALL_CHROOT/boot/efi at this point.

imp added inline comments.Mon, Feb 23, 11:51 PM
usr.sbin/bsdinstall/scripts/bootconfig
171

OK. The wrong comment got through. I deleted that and added a comment that said "please make a comment that this only works because the ESP is mounted". Sorry, I made an initial assessment, didn't send it, then looked again, but the updates didn't make it through. my bad.

jrtc27 added inline comments.Tue, Feb 24, 12:09 AM
usr.sbin/bsdinstall/scripts/bootconfig
171

If the zfs mirror loop above didn't reuse mntpt this would be a lot clearer by just using mntpt in place of BSDINSTALL_CHROOT/boot/efi :(

vexeduxr added inline comments.Tue, Feb 24, 12:23 AM
usr.sbin/bsdinstall/scripts/bootconfig
171

Hmm, I think it might be worth renaming the variable.

vexeduxr updated this revision to Diff 172553.Tue, Feb 24, 12:29 AM

Add comment noting that the ESP is mounted.
Rename ZFS mirror loop variable so mntpt can be reused.

This revision now requires review to proceed.Tue, Feb 24, 12:29 AM
Harbormaster completed remote builds in B70940: Diff 172553.Tue, Feb 24, 12:29 AM
vexeduxr edited the summary of this revision. (Show Details)Tue, Feb 24, 12:29 AM
imp accepted this revision.Tue, Feb 24, 5:13 PM
This revision is now accepted and ready to land.Tue, Feb 24, 5:13 PM
jrtc27 added a comment.Tue, Feb 24, 5:27 PM

Can you please include an appropriate description of the effects in the commit message?

jrtc27 added inline comments.Tue, Feb 24, 5:28 PM
usr.sbin/bsdinstall/scripts/bootconfig
156

tmpdir vs TMPDIR seems unnecessarily confusing. Maybe tmpmnt instead?

vexeduxr edited the summary of this revision. (Show Details)Tue, Feb 24, 5:41 PM
In D55469#1269614, @jrtc27 wrote:

Can you please include an appropriate description of the effects in the commit message?

Done. I lowered the verbosity a little though.

In D55469#1269177, @cperciva wrote:

BTW we're going to need this ASAP if it's going to land in 14.4.

I've shortened the MFC tag again to 2 days, should hopefully make it in before RC1.

vexeduxr updated this revision to Diff 172619.Tue, Feb 24, 5:44 PM

s/tmpdir/tmpmnt

This revision now requires review to proceed.Tue, Feb 24, 5:44 PM
Harbormaster completed remote builds in B70968: Diff 172619.Tue, Feb 24, 5:44 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Feb 24, 8:45 PM
Closed by commit rG0fd91c489134: bsdinstall: fix EFI boot entry creation (authored by vexeduxr). · Explain Why
This revision was automatically updated to reflect the committed changes.
vexeduxr added a commit: rG0fd91c489134: bsdinstall: fix EFI boot entry creation.
vexeduxr added a commit: rGcfdd90abab51: bsdinstall: fix EFI boot entry creation.Thu, Feb 26, 3:26 AM
vexeduxr added a commit: rG5425bdae0970: bsdinstall: fix EFI boot entry creation.

Revision Contents
Changeset List

PathSize
usr.sbin/
bsdinstall/
scripts/
25 lines

Diff 172619

View Options

usr.sbin/bsdinstall/scripts/bootconfig

Loading...