ZFS EFI Boot Support #2

[smh]

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

[smh]

Move .PATH include for zfs

[smh]

Standardise on EFI_ZFS_BOOT vs ZFS_EFI_BOOT

[smh]

Make efi_zfs_probe EFI_ZFS_BOOT conditional

[smh]

Cleanup libzfs.h includes

[alvisen_gmail.com]

I am now using this on 4 machines with success.

[smh]

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:

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

[kmoore]

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?

[smh]

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.

[kmoore]

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.

[eric_metricspace.net]

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?

[alvisen_gmail.com]

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:

1. What type of ZFS install e.g. single disk, mirror, RAIDZ
2. Any ARC of LOG devices
3. Do you have any UFS partitions?
4. 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)

[alvisen_gmail.com]

Patch does not apply after r292563, r292576, and r292584

[smh]

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

[alvisen_gmail.com]

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.

[smh]

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

[alvisen_gmail.com]

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

[eric_metricspace.net]

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.

[smh]

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:

1. ZFS single disk
2. ZFS mirrored disk
3. ZFS raidz2
4. ZFS raidz2 with log and cache devices.
5. 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

[emaste]

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]

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.

[smh]

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

[emaste]

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]

sys/boot/efi/boot1/boot1.c

473 ↗

(On Diff #11973)

trailing whitespace

[emaste]

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]

sys/boot/efi/boot1/boot1.c

84 ↗

(On Diff #11973)

Malloc() is usually provided by libstand iianm.

[andrew]

sys/boot/efi/boot1/boot1.c

84 ↗

(On Diff #11973)

boot1 doesn't use libstand.

[smh]

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.

[smh]

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.

[tsoome]

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]

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.

[smh]

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.

[smh]

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]

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.

[smh]

Address comment feedback from emaste.

[smh]

style nit highlighted by emaste.

[emaste]

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

1. migration to libstand
2. warning flags, prototypes
3. introduction of module support and moving UFS
4. 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

[smh]

In D4515#102680, @emaste wrote:

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

1. migration to libstand

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

2. warning flags, prototypes

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

3. introduction of module support and moving UFS
4. 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]

sys/boot/zfs/zfsimpl.c

1870 ↗

(On Diff #12055)

is this change intentional?

[smh]

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.

[smh]

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.

[smh]

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

[smh]

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.

[smh]

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