Page MenuHomeFreeBSD

The efifat files are no longer used: remove the code to build them.
ClosedPublic

Authored by bcran on Jun 8 2019, 9:19 PM.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24774
Build 23533: arc lint + arc unit

Event Timeline

bcran created this revision.Jun 8 2019, 9:19 PM
imp accepted this revision.Jun 8 2019, 10:45 PM
This revision is now accepted and ready to land.Jun 8 2019, 10:45 PM
imp added a reviewer: emaste.Jun 8 2019, 10:46 PM

Added emaste as a sanity check for the 'not used' bit. I'd like to see this die in fire.

tsoome accepted this revision.Jun 9 2019, 3:34 AM
emaste accepted this revision.Jun 9 2019, 3:39 PM

I approve, this should go.

I'm aware of some out-of-tree build scripts that do something like:

mkimg -s gpt -b $ROOTDIR/boot/pmbr \
  -p efi:=$ROOTDIR/boot/boot1.efifat \
  -p freebsd-boot:=$ROOTDIR/boot/gptboot \
  -p freebsd-ufs:=ufs -p freebsd-swap::1G \
  -o $image

but really they need to just generate the EFI filesystem at the same time they generate the main (UFS in this case) filesystem.

https://wiki.freebsd.org/UEFI needs an update (should probably just reference uefi(8) instead).

emaste added a comment.Jun 9 2019, 3:43 PM

There's random bits of boot1.efifat lore scattered around, e.g. Google turned up https://ashish.blog/2018/06/freebsd-uefi-boot/ and https://gist.github.com/tehpeh/a5676b711db94f6c1b9af1ec3356a13c. Hopefully a relnotes entry and a mailing-list heads-up is sufficiently discoverable by authors of those sorts of docs.

imp added inline comments.Jun 10 2019, 12:31 AM
stand/efi/boot1/Makefile
64

This needs an entry in OBSOLETE_FILES.
Also don't we need to actually delete them from the tree too?

bcran added inline comments.Jun 10 2019, 12:36 AM
stand/efi/boot1/Makefile
64

Oh, yeah it needs an entry in OBSOLETE_FILES and removed from OPTIONAL_OBSOLETE_FILES.
And yes, the .tmpl.xz files need deleted.

bcran updated this revision to Diff 58465.Jun 10 2019, 2:18 AM

Removed tmpl.xz files and updated ObsoleteFiles.inc and OptionalObsoleteFiles.inc

This revision now requires review to proceed.Jun 10 2019, 2:18 AM

I approve, this should go.
I'm aware of some out-of-tree build scripts that do something like:

mkimg -s gpt -b $ROOTDIR/boot/pmbr \
  -p efi:=$ROOTDIR/boot/boot1.efifat \
  -p freebsd-boot:=$ROOTDIR/boot/gptboot \
  -p freebsd-ufs:=ufs -p freebsd-swap::1G \
  -o $image

but really they need to just generate the EFI filesystem at the same time they generate the main (UFS in this case) filesystem.

Is there a way to build the efi fs this without root? I build images in a Jenkins instance to test FreeBSD/arm64 on various simulators and would prefer to not need to give the Jenkins user sudo access.

bcran added a comment.Jun 11 2019, 2:57 PM

Is there a way to build the efi fs this without root? I build images in a Jenkins instance to test FreeBSD/arm64 on various simulators and would prefer to not need to give the Jenkins user sudo access.

There's a review from July 2018 at D16438 to add msdos support to makefs, but it seems to have stalled. But otherwise, root access will be needed.

Is there a way to build the efi fs this without root? I build images in a Jenkins instance to test FreeBSD/arm64 on various simulators and would prefer to not need to give the Jenkins user sudo access.

There's a review from July 2018 at D16438 to add msdos support to makefs, but it seems to have stalled. But otherwise, root access will be needed.

I will take a renewed effort to get the makefs msdos support into the tree, but perhaps in the immediate term we can move the efifat goop into tools/? Keep the ObsoleteFiles.inc change and never install these for regular builds, but leave them around for people with special-purpose scripts?

bcran added a comment.Jun 19 2019, 4:48 PM

I will take a renewed effort to get the makefs msdos support into the tree, but perhaps in the immediate term we can move the efifat goop into tools/? Keep the ObsoleteFiles.inc change and never install these for regular builds, but leave them around for people with special-purpose scripts?

I'm not sure it's worth moving them, but I can commit the OptionalObsoleteFiles.inc and ObsoleteFiles.inc changes if you think that makes sense.

I'm not sure it's worth moving them, but I can commit the OptionalObsoleteFiles.inc and ObsoleteFiles.inc changes if you think that makes sense.

Are they already unhooked from install?

Are they already unhooked from install?

It seems not - they're optionally not installed if NOFAT is defined (rS348722/rS348763). There are several ways it could be accomplished (e.g. change to #ifdef EFIFAT) but my suggestion is that by default we do not install them and move the entries to ObsoleteFiles.inc unconditionally. Folks with out-of-tree uses can just do something like (cd stand/efi/boot1 && make -DEFIFAT)

imp added a comment.Jun 19 2019, 5:34 PM

Are they already unhooked from install?

It seems not - they're optionally not installed if NOFAT is defined (rS348722/rS348763). There are several ways it could be accomplished (e.g. change to #ifdef EFIFAT) but my suggestion is that by default we do not install them and move the entries to ObsoleteFiles.inc unconditionally. Folks with out-of-tree uses can just do something like (cd stand/efi/boot1 && make -DEFIFAT)

Yea, that's recent to reuse boot1's makefile for gptboot, but have the FAT stuff not done. It's not supposed to be a user controlled knob. If you want *THAT* then you could add a MK_LOADER_EFIFAT_BLOBS and have it default to NO.

Yea, that's recent to reuse boot1's makefile for gptboot, but have the FAT stuff not done. It's not supposed to be a user controlled knob. If you want *THAT* then you could add a MK_LOADER_EFIFAT_BLOBS and have it default to NO.

Well, we can't still install these files and also move them unconditionally to ObsoleteFiles.inc (as in this review) - my suggestion is that we don't even need to go as far as the machinery for WITH_LOADER_EFIFAT_BLOBS, just make it not installed by default anywhere (and add to ObsoloeteFiles.inc), and have a temporary backdoor for folks doing odd things.

imp accepted this revision.Aug 16 2019, 7:31 PM

kill them with fire :)

This revision is now accepted and ready to land.Aug 16 2019, 7:31 PM
emaste requested changes to this revision.Aug 16 2019, 8:54 PM

This change is not valid, it removes the templates but leaves the Makefile rules in place to install them.

Again, what I am suggesting is that we instead:

  • stop installing the EFI FAT fs images
  • add to obsoletefiles etc.
  • leave the templates and Makefile bits in place here or moved to tools/ for those who have out-of-tree needs, until msdos makefs support is in
This revision now requires changes to proceed.Aug 16 2019, 8:54 PM
bcran updated this revision to Diff 61267.Aug 25 2019, 9:01 PM

Since makefs fat support has been committed, proceed with removing all the EFI FAT code.

bcran added a comment.Aug 25 2019, 9:02 PM
  • leave the templates and Makefile bits in place here or moved to tools/ for those who have out-of-tree needs, until msdos makefs support is in

Sorry for the long delay in updating this review. Could you take a look at the latest changes please? I'm not sure if other work needs committed before we can remove the boot1/efifat stuff altogether? If so, I'll keep this on hold and proceed with that.

imp added inline comments.Aug 25 2019, 9:27 PM
ObsoleteFiles.inc
42 ↗(On Diff #61267)

This file is not obsolete.

stand/efi/Makefile
12 ↗(On Diff #61267)

This is not right. It boot1.efi needs to still be built.

bcran updated this revision to Diff 61270.Aug 25 2019, 10:37 PM

Keep building boot1.efi

bcran marked 2 inline comments as done.Aug 25 2019, 10:38 PM
bcran added inline comments.
stand/efi/Makefile
12 ↗(On Diff #61267)

I thought it wasn't needed since we use loader.efi now. I've updated the patch to continue building it.

Is stand/efi/boot1/Makefile.depend supposed to be deleted?

bcran marked an inline comment as done.Aug 28 2019, 1:49 AM

Is stand/efi/boot1/Makefile.depend supposed to be deleted?

I deliberately deleted it since it claims to be auto-generated, and appeared to be unused. But now I see many other directories have Makefile.depend files too. So to be safe, I'll restore it.

bcran updated this revision to Diff 61384.Aug 28 2019, 1:55 AM

Restore Makefile.depend

emaste accepted this revision.Aug 30 2019, 1:33 AM
This revision is now accepted and ready to land.Aug 30 2019, 1:33 AM
tsoome accepted this revision.Aug 30 2019, 5:20 AM
bcran closed this revision.Sep 4 2019, 9:45 PM
danfe added a subscriber: danfe.EditedOct 25 2019, 7:22 AM

I've stumbled upon this just few days ago when make delete-old suggested to remove the old trusty /boot/boot1.efifat (which I had to refuse). I know this differential is closed, but I have some questions and don't want to spam the mailing lists.

@imp wrote:

I'd like to see this die in fire.

May I ask what's wrong with it? I've been dding this image to my p1 ever since I had switched to GPT/UEFI on my drives, and it always worked just fine.

@emaste wrote:

https://wiki.freebsd.org/UEFI needs an update (should probably just reference uefi(8) instead).

I've checked with the wiki, looks like I now have to create partition manually with newfs_msdos -F 32 -c 1 and then copy loader.efi as /mnt/EFI/BOOT/BOOTX64.efi. This is not the same as my current layout created via boot1.efifat, which lists two files:

% ls -l /mnt/tmp/efi/boot/
total 385
-rwxr-xr-x  1 root  wheel  393216 Apr 16  2018 BOOTx64.efi
-rwxr-xr-x  1 root  wheel      12 Apr 16  2018 startup.nsh

Also, example in the wiki creates a whopping 40M partition to hold 411K loader file, why is that? /boot/boot1.efifat was of much more reasonable 800K.

I've stumbled upon this just few days ago when make delete-old suggested to remove the old trusty /boot/boot1.efifat (which I had to refuse). I know this differential is closed, but I have some questions and don't want to spam the mailing lists.

@imp wrote:

I'd like to see this die in fire.

My I ask what's wrong with it? I've been dding this image to my p1 ever since I had switched to GPT/UEFI on my drives, and it always worked just fine.

It is bad for multiboot setups. With mount ESP and copy files we can handle both single OS and multiple OS cases. But the pain point is, FreeBSD does not have decent way to keep boot programs up to date automatically and yes, mount + copy is more pain than single dd command.

@emaste wrote:

https://wiki.freebsd.org/UEFI needs an update (should probably just reference uefi(8) instead).

I've checked with the wiki, looks like I now have to create partition manually with newfs_msdos -F 32 -c 1 and then copy loader.efi as /mnt/EFI/BOOT/BOOTX64.efi. This is not the same as my current layout created via boot1.efifat, which lists two files:

% ls -l /mnt/tmp/efi/boot/
total 385
-rwxr-xr-x  1 root  wheel  393216 Apr 16  2018 BOOTx64.efi
-rwxr-xr-x  1 root  wheel      12 Apr 16  2018 startup.nsh

Also, example in the wiki creates a whopping 40M partition to hold 411K loader file, why is that? /boot/boot1.efifat was of much more reasonable 800K.

This is because there are systems which will not read ESP with FAT12/FAT16. Welcome to hell of x86. FAT32 needs minimum ~ 32MB. Note that vendors recommend 128-256MB (ish), and that is partly because enough space will make it possible to implement fall back situations and some hw vendors do provide firmware updates (and tools) as EFI applications, to be installed to ESP.

With "manual" creation of ESP you can still have the 800KB, but you can also have larger one… Note:

-r-xr-xr-x 1 root sys 518144 okt 11 21:41 loader32.efi
-r-xr-xr-x 1 root sys 577536 okt 11 21:41 loader64.efi

The 32 and 64 bit variants sum above 800K and if I want to keep also the "known good" backup copy...

Hope this helps.

danfe added a comment.Oct 25 2019, 8:15 AM
@tsoome wrote:

Hope this helps.

It does, thanks! However, I'm still not sure if I need to create startup.nsh or not. Quick googling suggests that one might get boot problems in some environments if this file is missing. Wiki page does not mention it.

@tsoome wrote:

Hope this helps.

It does, thanks! However, I'm still not sure if I need to create startup.nsh or not. Quick googling suggests that one might get boot problems in some environments if this file is missing. Wiki page does not mention it.

Yes it is situational, I have never needed it myself.

imp added a comment.Oct 26 2019, 2:06 AM
@tsoome wrote:

Hope this helps.

It does, thanks! However, I'm still not sure if I need to create startup.nsh or not. Quick googling suggests that one might get boot problems in some environments if this file is missing. Wiki page does not mention it.

Yes it is situational, I have never needed it myself.

It used to help in some weird situations exacerbated by bugs in the bios and in boot1.efi. things have changed enough that it's likely never needed.

@imp wrote:

It used to help in some weird situations exacerbated by bugs in the bios and in boot1.efi. things have changed enough that it's likely never needed.

I see; I've already found the commit when it was added, rS297871.