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 26099
Build 24630: 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
66

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
66

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

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