Paths

Table of Contentst

ZFS EFI Boot Support
AbandonedPublic
Actions

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

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

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

eric_metricspace.net updated this revision to Diff 10018.Nov 7 2015, 11:46 PM

Herald added subscribers: delphij, imp. · View Herald TranscriptNov 7 2015, 11:46 PM

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
7–17	Would you be willing to use a stock 2-clause FreeBSD license for consistency?

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

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
59	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.
59	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.
61	Not sure if it is really benefitial to enforce that the value of `devinfo`can not be changed in such a short and trivial function. In any case, there should be a space between the type name and `*`.
75	We attach `*` to the variable, not to the type.

alvisen_gmail.com added a subscriber: alvisen_gmail.com.Nov 27 2015, 12:10 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.

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.

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

alvisen_gmail.com added inline comments.Dec 2 2015, 4:06 PM

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

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) {`

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:

efi-zfs.patch30 KBDownload

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

efi-zfs.patch43 KBDownload

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

f0andrey_gmail.com added a subscriber: f0andrey_gmail.com.Dec 10 2015, 6:45 PM

deng.tooling_gmail.com added a subscriber: deng.tooling_gmail.com.Dec 10 2015, 11:41 PM

New patch

efi-zfs.patch43 KBDownload

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

martymac added a subscriber: martymac.Dec 15 2015, 6:05 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
61	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.
179	A blank line is required to separate variable definitions and actual code. A space is required after keywords [n.b. for (, not for(].
180	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

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?

Closing. Conduct all review on https://reviews.freebsd.org/D4515