Page MenuHomeFreeBSD

ZFS EFI Boot Support

Authored by on Nov 7 2015, 11:46 PM.



This patch adds ZFS support to the EFI boot and loader programs.

It modifies boot1.efi to use a modular framework, moving the existing UFS code to a UFS module. A ZFS module is provided for ZFS support. In addition to facilitating ZFS support, this also provides a foundation for future work (such as booting from encrypted disks)

It modifies loader.efi to correctly load and launch from a ZFS partition.

Test Plan

This patch has been tested extensively by several people. It has been confirmed to work in at least the following cases:

  • boot1 with ZFS
  • boot1 with UFS
  • boot1 with both ZFS and UFS
  • Grub + loader
  • Complex vdevs (mirroring, striping, logs, etc)

It has also reportedly been picked up by both NextBSD and PCBSD

Diff Detail

rS FreeBSD src repository - subversion
Lint Skipped
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Thank you very much for working on this! I hope we can commit it soon.

I'm currently on travel and won't be able to review / test fully until I return (in a week), although others may. For now I've pointed out a few minor style(9) and similar issues noted during a quick review.


We should conditionalize this (MK_ZFS?) and arrange to build a non-CDDL boot1.efi.


We'll want to clean these up -- either delete them if there's no value any longer, or make a DPRINTF macro or such.


style(9) does not want variable declarations in for loops


style(9) wants /* */ comments


The printf should be conditionalized on UFS/ZFS being enabled, or more likely just leave it -- especially if there's per-module printfs below.


Would you be willing to use a stock 2-clause FreeBSD license for consistency?

delphij requested changes to this revision.Nov 8 2015, 6:52 AM
delphij added a reviewer: delphij.

Thanks for working on this (and unifying boot code for the first time for UFS/ZFS).

I'd like to request some changes before someone proceed with an actual commit, the proposed changes are mostly cosmetic.


There are many style(9) violations in the following section, please consider fixing them before committing.


Can we just use the libc version of strchr instead of introducing yet another new strchr implementation?


Same here.


Can we make this a strlen()?


Since there is an alias for this (memcmp), I would suggest just use memcmp and remove this one.


I'd suggest some comment here describing that try_load would not return if load is succeeded.


Please use buffer == NULL. Modern compilers already have the ability to detect this kind of mistakes and doing so doesn't buy us much.


Please change this to memset().




Please revert this portion of change as it's not necessary.


and this.






Perhaps comment here that we didn't use printf("%s") because there are potential \0's, and we don't support real wide character either?


I'd explicitly truncate str[i] to char here but consider this optional.

This revision now requires changes to proceed.Nov 8 2015, 6:52 AM

Please review your code for the style consistency / conformancy and fix it.


Here and at several other places the opening brace is placed on a new line. This violates FreeBSD style and is inconsistent with the nearby code.


Here and at many other places function arguments are written in a way that is not customary for the FreeBSD code.
Usually we introduce new lines only when running over 80 symbols limit and the continuation lines have additional 4-space indentation.


Also, const could be considered unneeded in this function signature. It means that e.g. vdev can not be changed, but it does not mean that the struct that vdev points to can not be changed. Also, any change to vdev would not be visible to the callers. Perhaps you meant to write const vdev_t *vdev here? Same for the rest of parameters.


Not sure if it is really benefitial to enforce that the value of devinfocan not be changed in such a short and trivial function.
In any case, there should be a space between the type name and *.


We attach * to the variable, not to the type.

I really wanted to test this, but it fails to build for me.
Applied the patch to r291381
make buildworld results in this:

===> sys (depend)
===> sys/boot (depend)
===> sys/boot/ficl (depend)
===> sys/boot/forth (depend)
===> sys/boot/common (depend)
===> sys/boot/efi (depend)
===> sys/boot/efi/libefi (depend)
===> sys/boot/efi/loader (depend)
sh /usr/src/sys/boot/efi/loader/../../common/ /usr/src/sys/boot/efi/lo
ader/version "EFI loader" amd64
make[6]: don't know how to make /usr/src/sys/boot/efi/loader/zfs.c. Stop

make[6]: stopped in /usr/src/sys/boot/efi/loader
*** Error code 2 edited edge metadata.

General cleanup, address style issues. Also slight edits to makefiles.

Updated diff to address many issues.

Build system still needs to be adapted to use NO_ZFS. Also, the license in two of the new files needs to be updated.


That is OK, but I'm not fully aware of how best to do this.


The issue here is that this is EFI ABI code. Therefore, it has to be compiled with a different set of compiler flags. I don't think linking against libc will actually work because of that.


Yes. This was just a stock header I add to all my BSD-licensed projects.


I habitually declare everything const. I've removed those from the argument lists.

Also, it should cleanly apply to the current head and build in make world now.


This looks like it is not doing what it is supposed to do. If it passed a string that does where the first byte is not 0 it would loop forever... marked an inline comment as done. edited edge metadata.

Fixed strlen bug added inline comments.

The existence of bcmp is required by either the ZFS or UFS code. In general, the functions here are required by the filesystem code, and because of the aforementioned ABI issue, we can't just link against stdlib


Return values should be enclosed in parentheses, i.e. return (out); (and similar for other return statements).


Missing a space, i.e. this should be void *.


The second line should be indented 1 tab and 4 spaces. The EFI_SUCCESS) can then be moved to the same line.




These should be at the top of the function


There is a style bug here, the int i declaration should be in the group above.

There is a second subtle issue. You are redefining h to be an int. efi_find_handle returns an EFI_HANDLE, aka a void *. On a 64-bit machine this will truncate the pointer. When used later it will be sign extended. This is an issue if the handle has bit 31 set (as is the case on my laptop) as the handle will have the top 32 bits set incorrectly. It would also be an issue if any of the top bits were used in the handle.

The fix for this is the same as the style issue.


if (zfs_probe_dev(..) == 0) {

First off thanks Eric for working on this, just found myself in urgent need of this for a project I'm working on so based on rev #10875 so I've created a version which should address all the remaining points and a few more that I came across while reviewing the diff.

I've not tested yet and its very late here so be warned there may be some niggles outstanding but wanted to make it available for others to save any potentially duplicate effort.

Patch from svn diff -x -p is here:

Updated patch with the new files added this time and some more minor fixes:

Thanks for submitting the update. I am currently tied up dealing with year-end stuff, so not much time to work on it. I will have more time after mid next week.

Managed to back port this and the changes it relies on to 10.2 and I'm now happy to report I have a system booting of Intel P3700 nvme drives using EFI, which makes me really happy.

I did hit one gotcha, that EFI really hates active partitions, it just ignores them totally even if they contain a valid EFI boot setup, but that may well be a manufacture issue.

How to guide here:

New patch

which fixes:

  • strcpy null termination.
  • strdup allocation size and null termination.
  • none single vdev booting e.g. raidz by taking a copy of dev_info_t in probe.
  • be loud about allocation failures apart from Malloc
  • minor style fixes.

As others are interested in this and its confusing to just link patches I've created a new review which includes all my fixes here:

Once Eric gets some time to look at the feedback and updates it we can close one or the other.

delphij edited edge metadata.

Please take a look at style(9).

Please make ZFS support optional, it does not matter if it's enabled by default but we should give user the opportunity to choose.

Please reduce code elimination in UFS1_ONLY and UFS2_ONLY (also rename them to e.g. WANT_UFS1/WANT_UFS2) because there is no test if UFS1_ONLY and UFS2_ONLY were defined at the same time. If the size difference is small then we should probably just eliminate both flags and support both formats (EFI have much larger allowance for boot code size, unlike MBR/GPT).


No, I mean perhaps we can either add ${SRCTOP}/lib/libc/string/strcpy.c or #include it?


My suggestion is to make UFS[12]_ONLY more generic flags to match their actual semantics, e.g., WANT_UFS1. These should be reorganized to not duplicate code like the current construct, which is quite error prone and makes it hard to review.


A blank line is required to separate variable definitions and actual code.

A space is required after keywords [n.b. for (, not for(].


Why const here? This looks weird since you are returning plain void * and this const would be casted away upon return.


Please make this conditional -- otherwise the EFI loader would be always CDDL.


Here too.


Please remove this portion in final submission.


Please revert this change because it's a no-op.


Don't put constant as lval, it's not buying us anything and compiler should catch this.


Test of malloc result is needed because it could be NULL.


Blank line before this, space before (.


{ } should not be used here per style(9).


{ should not be used here.


Blank line.


Don't use } here.


No { here. Indent break;.


No } here, indent break.

This revision now requires changes to proceed.Dec 20 2015, 10:27 AM

Work on this has moved to Please make any requests for revision there.

Hi Eric if we're going to continue working on this on my revision of this ( then would you like to close this one so we can avoid confusion?