Differential D10447
Unify boot1 with loader Authored by eric_metricspace.net on Apr 21 2017, 2:57 PM. Tags None Referenced Files
Details
This patch is a redo of the EFI refactoring, with a lot of the complexity removes. It basically refactors boot1 to use the same IO code as loader. This opens the way to a lot of follow-up work, including GELI support for EFI, volume selection in boot1, and other things. This has been tested on qemu so far with a fairly complex setup. It has also been on use on real hardware for several months.
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions This review is a little out of date and is much more ambitious than what I've just done in https://reviews.freebsd.org/D11820 . which also sets down this path, but not as completely. I fear that will make this a little harder, but maybe not by too much. Comment Actions Please note a couple of things:
Comment Actions Now that this is rebased to head, I can hold off a little on my changes if they would be disruptive. That was my goal in giving the heads up: is this so stale it doesn't matter, or is it close and I'll upend the apple cart. What's your timeline to having this pushed in? Comment Actions We need to restore the priority scheme that was here before. At most, we can have zfs boot environments take precedence over the old behavior (since ZFS tends to be self-describing). But the old code used to do something similar and had all kinds of problems, which is why the preferred stuff was put in to begin with. It must be restored, or we must implement the UEFI boot manager protocol (either is acceptable, but the latter is likely beyond the scope).
Comment Actions Also, this patch is too big. There's many smaller bits that can easily be committed now which would trim maybe half of the changes here from this change. Without objection, I'll do that. The interstwingled changes here will also make it hard to bisect if there's any bugs in this refactor, which experience has shown will be important for a change of this size. Comment Actions Also, the current change in size for boot1.efi from these changes is going to present problems for the upgrade path since we foolishly chose such small FAT partitions in the past, in violation of the UEFI standard (though to be honest, even at work where we have acres of disk space, I only make 50MB partitions). Some thought may need to be given to the upgrade path since the old installs won't be able to upgrade to the new boot1.efi. Comment Actions Just as an remark - the UEFI standard does not really specify the size of the ESP, there are suggestions from vendors and some limiting factors (like minimum size for FAT32). For zpool create -B patch, I did pick 256MB as an default. Comment Actions I will pull out some components of the patch and make some smaller ones. Regarding the MSDOSFS image size issue, I've had a patch to increase the size out for some time, however, there's been ongoing indecision on how to deal with that issue. It really needs to get resolved soon, so this effort can move forward. Comment Actions Since it specifies it should be FAT32, that specifies a minimum size. There's no additional minimum required by the standard, true. We're cheating in many ways and it is now going to bite us. Comment Actions Yes. I know. It should have been fixed before 11.1, honestly. I thought it had been, or I'd have driven that aspect to completion faster. I've been complaining about the size issue now for a full year. Comment Actions I'm happy to help in that process. Some of these are no-brainers (like moving devicename.c, which is 100% independent or the EFI time function enhancement). I'm happy to mine the patch for these things and commit in such a way that rebasing is easy (this is easy to do with git, since I don't have to push to make sure the rebase is easy after a change). I honestly think it would be faster for me to do that than to have more review cycles here for the stuff that's more mundane and generated no comments to date. But I'm happy to defer too. Comment Actions that's a long way of saying "I'm happy to take the non-contentious parts of this patch so you have more times to deal with the issue I outlined" Comment Actions Okay, I've broken this one up into a number of smaller ones. I will wait for the last of them to go in, then I'll rebase and we can go from there.
Comment Actions I forwarded ported this, but we're missing functionality that's been added in the past year. I'll cherry pick some of these things to reduce trivial diffs.... Comment Actions I'm still certain that load_preferred does what you're talking about.
boot1.c, line 555
That was because efi_main.c got moved out of libefi to loader. I moved it back.
Comment Actions
Yea, they need to be two separate instances of efi_main because they do different things with the arguments and in the setup in general, especially for the efi boot manager changes (which need to grab the raw arguments which cannot be assumed to be ASCII or UCS2 or anything really. So I think that's too much refactoring. It's not really needed for the rest of the stuff to work. Comment Actions Fixed issues with preferred device detection. This includes detailed analysis of the code via debug messages to make sure it's doing the right thing. Preferred devices should now be correctly detected. Also, confirmed working with the loader efi_main. This should be good to go, pending necessary testing on various setups, of course. Comment Actions Also, someone please do a check for stray debug printfs. I am notoriously bad at spotting those. Comment Actions Finally got time to do QEMU tests on this. Found some issues with setting the DeviceHandle on the loaded image. I fixed it for UFS detection. I also seem to have introduced a regression in ZFS preferred device detection, which I'm working to fix. Comment Actions Fixed issues with setting image_handle->DeviceHandle incorrectly. Correct behavior confirmed on all my QEMU tests. Comment Actions I'll take another look at it this week. I have on my plate unifying the efi_main routines that we have in the tree and I have for my uefi boot manager work since they are somewhat similar. Once that's complete, I think the biggest obstacle to getting this into the tree will be behind us since that's the biggest source of conflicts at the moment. Comment Actions Thanks for rebasing and testing functionality. I know it's taken a while, but these changes are (a) large and (b) conflict with other large work... Comment Actions Can we possibly land this and then follow up with the EFI boot selection stuff? This is holding up GELI, and I'm getting people asking me when it'll land after my vBSDcon talk. If we land this then we can move forward with GELI in parallel to your stuff. Comment Actions We can't land this first and then do my stuff. That would break already committed work in boot1's copy of efi_main.c. This commit is also kinda too big to land all at once still, but I'll pull in as much as I can as I merge the efi_main's together to make the changes more bite-sized and bisectable should there be issues. There's been much grumbling of late about huge commits landing that break things that are impossible to bisect, so I'll have to break this up to ensure I won't be fielding complaints like that. Comment Actions The unification of efi_mains is already accomplished in this patch. I suppose I can try to separate things out again, but at this point I've sunk more work into fixing conflicts than I did into the GELI work itself. Comment Actions The diffs don't make sense to me.... The latest tree (and even some fairly old trees) have changes that would require diffs to appear here that don't.... Can you make sure that things here are up to date? I've hit a snag in my efforts and was trying to bring these in when I noticed that. Checking patch sys/boot/efi/loader/main.c... Checking patch sys/boot/efi/libefi/efizfs.c... Checking patch sys/boot/efi/libefi/devpath.c... Checking patch sys/boot/efi/include/efizfs.h... Checking patch sys/boot/efi/include/efilib.h... Applied patch sys/boot/efi/loader/main.c cleanly. Applied patch sys/boot/efi/libefi/efizfs.c cleanly. Applied patch sys/boot/efi/libefi/devpath.c cleanly. Applied patch sys/boot/efi/include/efizfs.h cleanly. Applied patch sys/boot/efi/include/efilib.h cleanly. OKAY Successfully committed patch. Checking patch head/sys/boot/efi/loader/loader_efi.h... error: head/sys/boot/efi/loader/loader_efi.h: does not exist in index Checking patch head/sys/boot/efi/loader/Makefile... error: head/sys/boot/efi/loader/Makefile: does not exist in index Checking patch head/sys/boot/efi/loader/devicename.c... error: head/sys/boot/efi/loader/devicename.c: does not exist in index Checking patch head/sys/boot/efi/libefi/devicename.c... error: head/sys/boot/efi/libefi/devicename.c: does not exist in index Checking patch head/sys/boot/efi/libefi/Makefile... error: head/sys/boot/efi/libefi/Makefile: does not exist in index Checking patch head/sys/boot/efi/include/efilib.h... error: head/sys/boot/efi/include/efilib.h: does not exist in index Patch Failed! Comment Actions The raw diff also looks messed up (it doesn't seem to include the head/... paths that gave arc patch heartburn). These issues may be the source of the mismatch between what I think the path does and what eric has maintained it does.... it's a phab failure of some sort. Comment Actions i've started integrating these changes, and have noticed a few problems...
Comment Actions Thanks for the updated. I'll pull this into the work I've done (I did that with a previous revision as well) I'll look closely to see if there's anything else that's a show stopper. My quick spot check just shows niggles that can be handled after the commit and/or cleaned up prior to the commit. I'll get this in front of the boot1/loader changes I'm planning for efi boot manager. Though I'm wondering more and more why we even have boot1.... But that question isn't quite ripe to explore. Comment Actions Note: this needs to get a test before it's merged, because I did modify the code. But a basic smoke-test ought to do it. Comment Actions I wonder why we even have boot1 anymore after these changes.... It's turning into a mini loader.efi. I wonder why we don't just kill it entirely. We don't need boot volume selection (that's supposed to be the EFI boot BIOS's job). We seem to duplicate a lot of code just to load /boot/loader.efi off of UFS or ZFS partitions. However, we could just use loader.efi directly (though some of the root detection code would need to be tweaked). boot1 used to be small, limited and basically unchanging (so updates weren't needed). Now that boot1.efi has hitched its wagon to loader.efi, I wonder why have an extra boot stage that seems to be good at messing up it's partition guessing... I'm going to go ahead and commit this (after adding back the bits I keep saying were bogusly deleted), but I'm thinking strongly that we consider just moving to loader.efi installed into \efi\freebsd\loader.efi starting in FreeBSD 12 since we'll be able to point efibootmgr at it by then. Comment Actions I have had the same thought since ... long time;) the only reason given has been that it is easier to have signed boot1. I still think it will be reasonable to remove it and use just loader binary. Comment Actions That's how the EFI boot stuff originally functioned. At some point, boot1.efi got added, but in the very beginning, you just installed loader.efi to the ESP. Comment Actions My point is that boot1 was originally a tiny thing we could toss on a partition and get EFI booting. Not very well supported, but hey it did it's best and it didn't change enough to bother with. Now, it's gotten to the point where it's got everything except scripting that /boot/loader.efi has... why continue down that path... let's just use /boot/loader :) I'm thinking for us, the best transition point would be when we start having efibootmgr... which I'm doing code reviews on right now... Comment Actions Thanks for the updated. I'll pull this into the work I've done (I did that with a previous revision as well) I'll look closely to see if there's anything else that's a show stopper. My quick spot check just shows niggles that can be handled after the commit and/or cleaned up prior to the commit. I'll get this in front of the boot1/loader changes I'm planning for efi boot manager. Though I'm wondering more and more why we even have boot1.... But that question isn't quite ripe to explore. [Edit: bogus comment about FICL being there removed] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This should be sorted, it was missed when arm was added, but that shouldn't mean we do the same with AArch64.