ZFS EFI Boot Support
AbandonedPublic

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

Details

Reviewers
delphij
andrew
Summary

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

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
emaste added a subscriber: emaste.Nov 8 2015, 12:01 AM

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.

sys/boot/efi/boot1/Makefile
23–24

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

sys/boot/efi/boot1/boot1.c
215–217

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

266

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

271

style(9) wants /* */ comments

305

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

sys/boot/efi/boot1/zfs_module.c
8–18

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.

sys/boot/efi/boot1/boot1.c
75

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

95

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

104

Same here.

119

Can we make this a strlen()?

129

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

200

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

211

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

268

Please change this to memset().

sys/boot/efi/boot1/ufs_module.c
186

style(9).

sys/boot/efi/loader/devicename.c
91

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

93

and this.

101

Ditto.

103

Ditto.

sys/boot/efi/loader/main.c
68

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?

73

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
avg added a subscriber: avg.Nov 25 2015, 7:21 AM

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

sys/boot/efi/boot1/boot1.c
312

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.

sys/boot/efi/boot1/zfs_module.c
60

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.

60

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.

62

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 *.

76

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

alvisen_gmail.com added a comment.EditedNov 27 2015, 1:04 PM

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/newvers.sh /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

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

eric_metricspace.net marked 21 inline comments as done.Nov 29 2015, 12:22 AM

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.

sys/boot/efi/boot1/Makefile
23–24

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

sys/boot/efi/boot1/boot1.c
95

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.

sys/boot/efi/boot1/zfs_module.c
9–19

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

61

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

eric_metricspace.net marked 2 inline comments as done.Nov 29 2015, 12:24 AM

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

sys/boot/efi/boot1/boot1.c
127

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...

eric_metricspace.net marked an inline comment as done.

Fixed strlen bug

eric_metricspace.net marked 3 inline comments as done.Dec 7 2015, 1:18 PM
eric_metricspace.net added inline comments.
sys/boot/efi/boot1/boot1.c
129

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

andrew added inline comments.Dec 7 2015, 6:11 PM
sys/boot/efi/boot1/boot1.c
86

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

174

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

241–242

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

246–249

Ditto

sys/boot/efi/loader/main.c
152–154

These should be at the top of the function

505–506

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.

509

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

smh added a subscriber: smh.Dec 9 2015, 5:10 AM

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:

smh added a comment.Dec 9 2015, 6:21 AM

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

eric_metricspace.net marked an inline comment as done.Dec 10 2015, 4:18 AM

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.

smh added a comment.EditedDec 10 2015, 5:55 PM

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: http://blog.multiplay.co.uk/2015/12/freebsd-10-2-release-efi-zfs-root-boot/

smh added a comment.Dec 11 2015, 4:46 AM

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.
smh added a comment.EditedDec 11 2015, 3:07 PM

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:
https://reviews.freebsd.org/D4515

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

delphij requested changes to this revision.Dec 20 2015, 10:27 AM

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).

sys/boot/efi/boot1/boot1.c
95

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

sys/boot/efi/boot1/ufs_module.c
60

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.

178

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

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

179

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

sys/boot/efi/loader/Makefile
25

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

39

Here too.

91

Please remove this portion in final submission.

sys/boot/efi/loader/devicename.c
65

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

106

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

108

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

sys/boot/efi/loader/main.c
76

Blank line before this, space before (.

77

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

157

{ should not be used here.

159

Blank line.

169

Don't use } here.

170

No { here. Indent break;.

180

No } here, indent break.

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

Work on this has moved to https://reviews.freebsd.org/D4515. Please make any requests for revision there.

will added a subscriber: will.Jan 5 2016, 6:58 PM
smh added a comment.Jan 5 2016, 9:13 PM

Hi Eric if we're going to continue working on this on my revision of this (https://reviews.freebsd.org/D4515) then would you like to close this one so we can avoid confusion?