Details

Reviewers

dim
delphij

Commits

rS294060: Modularise EFI boot loader

Summary

Based off https://reviews.freebsd.org/D4104

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.

On top of the version @ https://reviews.freebsd.org/D4104 this revision:

Fixes style(9) issues
Fixes strcpy null termination.
Fixes strdup allocation size and null termination.
Fixes none single vdev booting e.g. raidz by taking a copy of dev_info_t in probe.
Is loud about allocation failures apart from Malloc
Pays attention to MK_ZFS
Module structure cleanup
Removed unused vars and funcs
Improved error checking
Enabled compiler warnings
Fixed void pointer arithmetic warning in lz4.c
Output probe progress and summary
Fixed _MSC_EXTENSIONS #if vs #ifdef checks
Fix strncmp
Use optimised versions of other support methods based on libkern / libstand
Added missing copyright headers
Updated copyright headers for new files to match the prefered header
Standardise on EFI_ZFS_BOOT vs ZFS_EFI_BOOT
Make efi_zfs_probe EFI_ZFS_BOOT conditional
Cleanup libzfs.h includes

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 Not Applicable

Unit

Tests Not Applicable

Event Timeline

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

Move .PATH include for zfs

smh marked an inline comment as done.Dec 13 2015, 11:40 PM

smh added a reviewer: dim.

smh added a reviewer: delphij.Dec 13 2015, 11:42 PM

Standardise on EFI_ZFS_BOOT vs ZFS_EFI_BOOT

Make efi_zfs_probe EFI_ZFS_BOOT conditional

Cleanup libzfs.h includes

martymac added a subscriber: martymac.Dec 15 2015, 6:07 AM

smh updated this object.Dec 15 2015, 8:28 PM

I am now using this on 4 machines with success.

In D4515#97151, @alvisen_gmail.com wrote:

I am now using this on 4 machines with success.

Thanks for the feedback, appreciated :)

It would be really good if you could confirm your setup so we can tick off various tested scenarios, if we could gather:

What type of ZFS install e.g. single disk, mirror, RAIDZ
Any ARC of LOG devices
Do you have any UFS partitions?
Number of installed devices and partition count.

Just a FYI. We've been using the EFI+ZFS patches on PC-BSD for a while. They work on the installed system, but we've seen that they break the USB memory stick creation (UFS filesystem). Most systems won't even bring up the loader. Has anybody here tested this on FreeBSD yet, if the standard USB images from 'make release" works?

In D4515#97264, @kmoore wrote:

Just a FYI. We've been using the EFI+ZFS patches on PC-BSD for a while. They work on the installed system, but we've seen that they break the USB memory stick creation (UFS filesystem). Most systems won't even bring up the loader. Has anybody here tested this on FreeBSD yet, if the standard USB images from 'make release" works?

Not tested from USB images but mfsbsd from an ISO works with this patch.

In D4515#97270, @smh wrote:

In D4515#97264, @kmoore wrote:

Just a FYI. We've been using the EFI+ZFS patches on PC-BSD for a while. They work on the installed system, but we've seen that they break the USB memory stick creation (UFS filesystem). Most systems won't even bring up the loader. Has anybody here tested this on FreeBSD yet, if the standard USB images from 'make release" works?

Not tested from USB images but mfsbsd from an ISO works with this patch.

Yea, I should have mentioned that. With the patch ISO's work fine still, we've only seen the problems on .img USB sticks.

Thanks for taking care of the remaining cleanup.

Regarding testing LOG and ARC, there were reports of successful tests using those features on freebsd-hackers.

Regarding USB sticks, could the issue perhaps be the lack of a GUID Partition Table? Does anyone have any output from a failed attempt?

In D4515#97212, @smh wrote:

In D4515#97151, @alvisen_gmail.com wrote:

I am now using this on 4 machines with success.

Thanks for the feedback, appreciated :)

It would be really good if you could confirm your setup so we can tick off various tested scenarios, if we could gather:

What type of ZFS install e.g. single disk, mirror, RAIDZ

Any ARC of LOG devices

Do you have any UFS partitions?

Number of installed devices and partition count.

They are all booting to a single disk ZFS pool. No ARC or LOG devices.
2 desktops and 2 laptops.

Home desktop machine:
Single SSD with single ZFS partition. Used to boot CSM, now booting UEFI.

=>       34  351651821  ada0  GPT  (168G)
         34          6        - free -  (3.0K)
         40        128     1  freebsd-boot  (64K)
        168        896     3  efi  (448K)
       1064  351650784     2  freebsd-zfs  (168G)
  351651848          7        - free -  (3.5K)

Work desktop:
Booting off SSD, single disk ZFS, second spinning rust disk with UFS. Used to boot CSM, now UEFI.

=>       40  117231328  ada0  GPT  (56G)
         40       1024     1  freebsd-boot  (512K)
       1064        984        - free -  (492K)
       2048    4194304     2  freebsd-swap  (2.0G)
    4196352  113033216     3  freebsd-zfs  (54G)
  117229568       1800     4  efi  (900K)

=>       40  976773088  ada1  GPT  (466G)
         40  976773080     1  freebsd-ufs  (466G)
  976773120          8        - free -  (4.0K)

Old laptop (Acer aspire E 11(E3-112)):
Single SSD, UEFI only, but /boot on UFS. Now completely ZFS:

=>       40  117231328  ada0  GPT  (56G)
         40       1024     1  freebsd-boot  (512K)
       1064     131072     2  efi  (64M)
     132136        984        - free -  (492K)
     133120    4061184     4  freebsd-ufs  (1.9G)
    4194304       1064        - free -  (532K)
    4195368  113036000     3  freebsd-zfs  (54G)

My new laptop (Lenovo Thinkpad Yoga 12):
Single SSD, UEFI only. Tripple booting with Windows 8.1, OpenBSD and FreeBSD.
Boot1 recognizes the OpenBSD partition as UFS, but loads loader.efi from ZFS:

=>       34  500118125  ada0  GPT  (238G)
         34       2014        - free -  (1.0M)
       2048    2048000     1  !de94bba4-06d1-4d40-a16a-bfd50179d6ac  (1.0G)
    2050048     532480     2  efi  (260M)
    2582528     262144     3  ms-reserved  (128M)
    2844672  237172734     4  ms-basic-data  (113G)
  240017406          2        - free -  (1.0K)
  240017408   83886080     6  !824cc7a0-36a8-11e3-890a-952519ad3f61  (40G)
  323903488  148883456     7  freebsd-zfs  (71G)
  472786944   27330560     5  !de94bba4-06d1-4d40-a16a-bfd50179d6ac  (13G)
  500117504        655        - free -  (328K)

jpm820_gmail.com added a subscriber: jpm820_gmail.com.Dec 23 2015, 2:43 PM

Patch does not apply after r292563, r292576, and r292584

Fix merge conflicts introduced by r292563, r292576 and r292584

This replaces the manual extraction of the error from an EFI status code with the new macro EFI_ERROR_CODE(status).

In D4515#100635, @smh wrote:

Fix merge conflicts introduced by r292563, r292576 and r292584

This replaces the manual extraction of the error from an EFI status code with the new macro EFI_ERROR_CODE(status).

Thanks!

Applied to r292987, built world and confirmed working as before.

Fix UFS booting
Improve error reporting with more use of EFI_ERROR_CODE
Improve error checking of fsread call

Patch is offset by r293001 and r293165. Builds fine though.

allanjude added a subscriber: allanjude.Jan 5 2016, 9:23 PM

will added a subscriber: will.Jan 5 2016, 9:26 PM

I closed the other review. What exactly remains to be done on this?

If we need testing of intent logs/l2arc, I'm going to be getting some new hardware in the near future, and I should be able to test it on that.

In D4515#101927, @eric_metricspace.net wrote:

I closed the other review.

Cool thx.

What exactly remains to be done on this?

I'm happy with the current state of this, it passes all of my tests including:

ZFS single disk
ZFS mirrored disk
ZFS raidz2
ZFS raidz2 with log and cache devices.
UFS disk

I just need to get some positive reviews from others now.

If we need testing of intent logs/l2arc, I'm going to be getting some new hardware in the near future, and I should be able to test it on that.

Just confirmed that working here:

zpool status
  pool: tank
 state: ONLINE
  scan: none requested
config:

        NAME        STATE     READ WRITE CKSUM
        tank        ONLINE       0     0     0
          raidz2-0  ONLINE       0     0     0
            nvd0p4  ONLINE       0     0     0
            nvd1p4  ONLINE       0     0     0
            nvd2p4  ONLINE       0     0     0
            nvd3p4  ONLINE       0     0     0
            nvd4p4  ONLINE       0     0     0
        logs
          nvd5p1    ONLINE       0     0     0
        cache
          nvd5p2    ONLINE       0     0     0

tsoome added a subscriber: tsoome.Jan 6 2016, 7:08 PM

emaste added inline comments.Jan 6 2016, 7:19 PM

sys/boot/efi/Makefile
19 ↗	(On Diff #11928)	CFLAGSWARN vs CWARNFLAGS?
sys/boot/efi/boot1/Makefile
5–8 ↗	(On Diff #11928)	Can you explain why individual warning flags are turned on here?
sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11928)	What's this capital-M Malloc used for?
94–95 ↗	(On Diff #11928)	can we link against libstand for these?
sys/boot/efi/include/arm64/efibind.h
42 ↗	(On Diff #11928)	these cleanups ought to be committed first/separately
sys/boot/efi/include/efierr.h
32–34 ↗	(On Diff #11928)	I had started this in a WIP branch; based on discussion on IRC I'll make sure it's equivalent to what you have in this review and commit.

smh marked 4 inline comments as done.Jan 6 2016, 7:57 PM

smh added inline comments.

sys/boot/efi/Makefile
19 ↗	(On Diff #11928)	good spot
sys/boot/efi/boot1/Makefile
5–8 ↗	(On Diff #11928)	All the flags from kern.mk
sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11928)	Without it build fails with: --- boot1.efi --- if [ `objdump -t boot1.sym \| fgrep 'UND' \| wc -l` != 0 ]; then objdump -t boot1.sym \| fgrep 'UND'; exit 1; fi 0000000000000000 UND 0000000000000000 Malloc
94–95 ↗	(On Diff #11928)	I tried that but it got ugly quickly unfortunately, so not without significant extra work
sys/boot/efi/include/arm64/efibind.h
42 ↗	(On Diff #11928)	Agreed will do.

Sync with latest head version which now includes the ancillary changes, such as style and warning fixes originally part of this review.

emaste added inline comments.Jan 6 2016, 9:13 PM

sys/boot/efi/boot1/Makefile
5–8 ↗	(On Diff #11973)	is there not a WARNS= we can use somehow that gets (most of) what we want?
28 ↗	(On Diff #11973)	Aside, I wonder if we can do MK_UFS eventually?
sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11973)	Oh, required by the ZFS module I guess?
402–403 ↗	(On Diff #11973)	Instead I think we want to print the message, delay (5 seconds?) and then exit the EFI application. But that can happen in a separate change.

emaste added inline comments.Jan 6 2016, 9:15 PM

sys/boot/efi/boot1/boot1.c
473 ↗	(On Diff #11973)	trailing whitespace

I built a QEMU VM image with this change incorporated, and run BOOTx64 from the EFI shell and it failed with Error reported: Unsupported. I'll investigate.

allanjude added inline comments.Jan 6 2016, 10:08 PM

sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11973)	Malloc() is usually provided by libstand iianm.

andrew added inline comments.Jan 6 2016, 11:44 PM

sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11973)	boot1 doesn't use libstand.

smh marked 2 inline comments as done.Jan 7 2016, 9:43 AM

smh added inline comments.

sys/boot/efi/boot1/Makefile
5–8 ↗	(On Diff #11973)	I didn't think so but on closer inspection WARNS = 6 is close, I'll give that a shot, see what the results of that are.
sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11973)	zfs.c and zfsimp.c both use malloc so yer that will be it.
84 ↗	(On Diff #11973)	For some arches it actually does e.g. i386, I'm currently testing to see if its worth using on all platforms to eliminate the custom string / memory functions.
402–403 ↗	(On Diff #11973)	sounds good.

In D4515#102069, @emaste wrote:

I built a QEMU VM image with this change incorporated, and run BOOTx64 from the EFI shell and it failed with Error reported: Unsupported. I'll investigate.

I've reproduced this on the latest version, not clear if there's a merge error as my last backport to 10.2 works under qemu works as expected but I'm also investigating too.

I personally do feel a bit concerned about the duplicated local instances of common functions. The following is not strictly about this uefi port, but a bit more generic, but sometimes it is good to think about big picture as well:

In my illumos port I actually did convert gptzfsboot to use libstand and common functions, simplifying the code a lot - obvisously the down side is image size growth, but that size is not really an issue *if* the stage2 is stored on either zfs bootblock area (3.5MB) or on dedicated partition. It also used to be issue with 64k segment size limit (for code relocation), but this limit is also gone for now.

For example from my current build the size on illumos is:
tsoome@beastie:~$ ls -l /boot/gptzfsboot
-r--r--r-- 1 root sys 108544 jaan 6 23:06 /boot/gptzfsboot

thats using libstand, common bits and "normal" devsw to implement the file system support in stage2. My own initial motivation was to add gzip compression support for zfs - so I was in need to reuse gzip bits.

Obviously it is not just as simple as "lets not care that much about stage2 size" (or stage1 in case of uefi), but then again, lets try to avoid the need for reinventing the wheel over and over;)

eric_metricspace.net added inline comments.Jan 8 2016, 1:28 PM

sys/boot/efi/boot1/boot1.c
84 ↗	(On Diff #11973)	The issue (and the reason the original boot1.efi provided its own versions of some functions) is that boot1 has to be compiled for the EFI (win32) ABI, unlike the rest of the system.

Resync with r293422 (required due to boot1 size increase).

Switch to using libstand, which was already a dependency for i386 and arm, for string and memory functions. This failed during previously due the boot1 size increase fixed by r293422.

To use this your head source must >= r293422

In D4515#102069, @emaste wrote:

I built a QEMU VM image with this change incorporated, and run BOOTx64 from the EFI shell and it failed with Error reported: Unsupported. I'll investigate.

This should be fixed by r293422.

sys/boot/efi/boot1/boot1.c
83 ↗	(On Diff #12046)	I've just updated to use libstand, which was a dependency for some arches anyway, so it would be good if you could confirm this works as expected where you thought this was an issue Eric. I have confirmed using qemu on amd64 that all is good.

emaste added inline comments.Jan 8 2016, 2:50 PM

sys/boot/efi/boot1/Makefile
69–70 ↗	(On Diff #12046)	I wouldn't list these all here since they'll get out of date if any changes are made. just the "string and memory functions" text explains the use.

Address comment feedback from emaste.

smh marked an inline comment as done.Jan 8 2016, 2:55 PM

style nit highlighted by emaste.

You could commit this in three or four pieces if you wish:

migration to libstand
warning flags, prototypes
introduction of module support and moving UFS
ZFS support

(or #3 and #4 together if it's more than a trivial amount of effort to do them separately)

Overall I'm happy with this change but still don't like the way warnings are individually handled here.

sys/boot/efi/boot1/Makefile.fat
4 ↗	(On Diff #12048)	looks like this snuck in accidentally

In D4515#102680, @emaste wrote:

You could commit this in three or four pieces if you wish:

migration to libstand

Committed: https://svnweb.freebsd.org/changeset/base/293460

warning flags, prototypes

Review for this is now here: https://reviews.freebsd.org/D4839

introduction of module support and moving UFS

ZFS support

(or #3 and #4 together if it's more than a trivial amount of effort to do them separately)

I'll see how difficult this is, should be OK.

Overall I'm happy with this change but still don't like the way warnings are individually handled here.

I'm going remove the individual warnings in favour of WARNS from https://reviews.freebsd.org/D4839, hopefully the ZFS code won't be too troublesome.

sys/boot/efi/boot1/Makefile
5–8 ↗	(On Diff #12055)	This turned out not to be easily possible due to fixes require to dependences for WARN >= 2, so for the time being keeping it local.
28–31 ↗	(On Diff #12055)	Possibly, but quite a bit relies on it so its going to be none trivial.
sys/boot/efi/boot1/boot1.c
93–94 ↗	(On Diff #12055)	This is now done, the problem cause was the image size.

allanjude added inline comments.Jan 10 2016, 5:45 AM

sys/boot/zfs/zfsimpl.c
1870 ↗	(On Diff #12055)	is this change intentional?

smh marked 4 inline comments as done.Jan 10 2016, 5:19 PM

smh added inline comments.

sys/boot/zfs/zfsimpl.c
1870 ↗	(On Diff #12055)	Yes as its not used externally and was causing a warning due to lack of prototype.

Looking to get this committed over the next day or so in the hope I can get it MFC'ed before 10.3 freeze.

Currently this blocking on https://reviews.freebsd.org/D4839 so if anyone could find some time today to review that would be most appreciated.

Remove changes covered by https://reviews.freebsd.org/D4839
Sync up with latest code which also removes changes covered by r293460

Sync with latest HEAD code that enabled WARNS.

Fix a number of compile warnings the merge introduced.

Restore Malloc and add Free to fix issues with unaligned buffers.

Also fix fsread which has always been invaliding reporting success on probe after the first valid ufs partition was detected.

Update to head r294041
Convert file_loadraw name param to const fixing compiler warning.

Closed by commit rS294060: Modularise EFI boot loader (authored by smh). · Explain WhyJan 15 2016, 1:23 AM

This revision was automatically updated to reflect the committed changes.

smh mentioned this in rS294060: Modularise EFI boot loader.

ZFS EFI Boot Support #2
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 12336

head/sys/boot/efi/boot1/Makefile

head/sys/boot/efi/boot1/boot1.c

head/sys/boot/efi/boot1/boot_module.h

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

head/sys/boot/efi/include/efilib.h

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

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

ZFS EFI Boot Support #2ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 12336

head/sys/boot/efi/boot1/Makefile

head/sys/boot/efi/boot1/boot1.c

head/sys/boot/efi/boot1/boot_module.h

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

head/sys/boot/efi/include/efilib.h

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

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

ZFS EFI Boot Support #2
ClosedPublic
Actions

Revision Contents
Changeset List