Paths

Table of Contentst

[RFC] sysutils/fwupd: Make uefi-capsules plugin work
Needs ReviewPublic
Actions

Authored by sergii.dmytruk_3mdeb.com on Sat, Feb 28, 11:51 AM.

Details

Reviewers

decke
egypcio

Summary

Fix gnu-efi producing broken binaries.

Needed to make fwupd-efi work.

Drop unused libefiboot dependency.

It's not needed since 2023.

Make fwupd work with native libefivar.

It's either native libefivar or the one from ports. Their API is incompatible. devel/efivar was needed for libefiboot, so it makes sense to not depend on it now, except that /usr/lib/libefivar can't be used if devel/efivar is installed due to -rpath. Linking to /usr/lib/libefivar.a as a workaround doesn't work, because it was not built with -fPIC.

Using devel/efivar requires different patches for fwupd and implementation of FreeBSD's ioctl() to deal with EFI variables in efivar. Both changes are available, but posting a simpler version first to get feedback about which approach is preferable.

Add patches to make uefi-capsules work on FreeBSD.

These will be sent to upstream which knows about FreeBSD, but which patches should be sent partially depends on which libefivar is going to be used in the end.

Questions to reviewers

Need to decide if the conflict with devel/efivar is acceptable or can be avoided.

Also, should bsdisks be added to the dependencies of fwupd? fwupd could be used without it, so maybe it should just be recommended.

Test Plan

Make sure dbus is running and bsdisks is installed or ESP won't be found.
bsdisks port must be the latest (reporting UUID was already upstreamed).
fwupdtool plugins should show uefi-capsules plugin as functional.
fwupdtool get-devices should show system firmware as updatable.
fwupdtool install-blob update.cap should succeed and update after a reboot should also succeed.

ESRT sometimes wasn't present (efitable -t esrt) in QEMU and failed with ENODEV on hardware, likely because EFI loader doesn't move ESRT to EFI runtime memory before calling ExitBootServices(). This is, however, not in the scope of these changes.

Diff Detail

Repository

R11 FreeBSD ports repository

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 71230
Build 68113: arc lint + arc unit

Event Timeline

sergii.dmytruk_3mdeb.com requested review of this revision.Sat, Feb 28, 11:51 AM

sergii.dmytruk_3mdeb.com created this revision.

Harbormaster completed remote builds in B71091: Diff 172924.Sat, Feb 28, 11:51 AM

sergii.dmytruk_3mdeb.com edited the summary of this revision. (Show Details)Sat, Feb 28, 11:59 AM

sergii.dmytruk_3mdeb.com edited the test plan for this revision. (Show Details)

emaste added a subscriber: emaste.Wed, Mar 4, 6:38 PM

emaste added inline comments.

devel/gnu-efi/Makefile
29–34	This comment seems a little confusing out of context -- in this diff it makes sense with the removal of the strip command, but now it's just here without context. I'd probably add something like "This port installs object files as lib/*.o. Previously it stripped those objects, but ..." to start off the comment. This (stripping .o or .a) seems to have come up a few times in different ports and I'm not sure where the idea is coming from. I think we could commit this part of this review independently first.

Updated a comment in devel/gnu-efi, also rebased

Harbormaster completed remote builds in B71230: Diff 173266.Fri, Mar 6, 1:35 PM

sergii.dmytruk_3mdeb.com added a reviewer: egypcio.Fri, Mar 6, 1:36 PM

sergii.dmytruk_3mdeb.com added inline comments.Fri, Mar 6, 1:46 PM

devel/gnu-efi/Makefile
29–34	This comment seems a little confusing out of context -- in this diff it makes sense with the removal of the strip command, but now it's just here without context. I'd probably add something like "This port installs object files as lib/*.o. Previously it stripped those objects, but ..." to start off the comment. Updated the comment per your suggestion. This (stripping .o or .a) seems to have come up a few times in different ports and I'm not sure where the idea is coming from. It's a nasty thing too. I spent several hours understanding code and debugging `objcopy` to figure out why it silently produces a broken executable (lack of an entry point in an input file (`_start` from CRT) results in `objcopy` skipping "optional" PE header). Hence felt like the comment is necessary, so nobody adds stripping again. I think we could commit this part of this review independently first. Yes, this part doesn't have to wait for other commits. Added @egypcio as a reviewer temporarily.

Sorry that it took me so long and thanks a lot for your work, that looks really good to me!

I started looking at this issue some time ago but lost motivation because it was really hard to debug. So a really big tanks for fixing it!

In D55590#1275939, @decke wrote:

Sorry that it took me so long and thanks a lot for your work, that looks really good to me!

I started looking at this issue some time ago but lost motivation because it was really hard to debug. So a really big tanks for fixing it!

Thank you for reviving and finalizing D29332 after it was abandoned!

Any comments on which way of handling libefivar is preferable? I.e., whether to add CONFLICTS = devel/efivar or make devel/efivar a dependency and add missing changes there (pointing at updated https://github.com/3mdeb/efivar could be simpler than adding a bunch of patches).

I'm also conflicted about bsdisks and RUN_DEPENDS, because it isn't really a hard dependency and it itself depends on Qt. Do OPTIONS_* make sense in this case? I suppose bsdisks and fwupd-efi could be grouped together.

I'm not very familiar with ports and have never used *BSD systems for a long time, thus don't really know what would be more idiomatic and convenient for actual users.

In D55590#1276134, @sergii.dmytruk_3mdeb.com wrote:

In D55590#1275939, @decke wrote:

Sorry that it took me so long and thanks a lot for your work, that looks really good to me!

I started looking at this issue some time ago but lost motivation because it was really hard to debug. So a really big tanks for fixing it!

Thank you for reviving and finalizing D29332 after it was abandoned!

Any comments on which way of handling libefivar is preferable? I.e., whether to add CONFLICTS = devel/efivar or make devel/efivar a dependency and add missing changes there (pointing at updated https://github.com/3mdeb/efivar could be simpler than adding a bunch of patches).

devel/efivar is the best we can get up to FreeBSD 14. As of 15.0 we have proper libefivar but as you know it's not fully compatible. I started to upstream efivar patches already so maintaining those patches in the portstree is not a big deal and better than hiding patches in some github fork.

I think we should ONLY keep FreeBSD 14 supported in fwupd if it's reasonable and does not block our progress. So far it was possible but efivar was a bit of a pain.

I'm also conflicted about bsdisks and RUN_DEPENDS, because it isn't really a hard dependency and it itself depends on Qt. Do OPTIONS_* make sense in this case? I suppose bsdisks and fwupd-efi could be grouped together.

Yeah, we can introduce more options and create a no-gui flavour but I do not consider this a high priority task and it complicates port maintenance and testing.

I added your patches on top of fwupd 2.0.20 and it builds fine for FreeBSD 15.0 but breaks building on FreeBSD 14.3. I am not sure if this was expected:

../plugins/uefi-capsule/fu-uefi-capsule-backend-freebsd.c:118:18: fatal error: variable has incomplete type 'struct efi_guid'
  118 |         struct efi_guid esrt_uuid = EFI_TABLE_ESRT;
      |                         ^
../plugins/uefi-capsule/fu-uefi-capsule-backend-freebsd.c:118:9: note: forward declaration of 'struct efi_guid'
  118 |         struct efi_guid esrt_uuid = EFI_TABLE_ESRT;
      |                ^

https://ports.bluelife.at/builds/20260310-11:47:55.92081/

I will do some runtime testing on FreeBSD 15.0 with that soon.

Rebase, add CONFLICTS_* with efivar, add RUN_DEPENDS on bsdisks

Also fix 14.x by using efi_guid_t instead of struct efi_guid.

Harbormaster completed remote builds in B71327: Diff 173540.Tue, Mar 10, 11:26 PM