Page MenuHomeFreeBSD

Make loader.efi dual boot, step 1
ClosedPublic

Authored by imp on Jan 6 2018, 5:26 PM.

Details

Reviewers
tsoome
kevans
rgrimes
Group Reviewers
bhyve
Summary

Minor refactoring of currdev assignment. Remove redunant device device type.

Start to simplify the searching of block devices. Move the loops into
efipart.c. Add back pointers for the devsw in pdinfo_t and introduce a
pd_parent so we can represent partitions more easily. This is simply a
refactor, no new functionality.

Add a quick sanity chceck to the zfs and disk partition code. We
require loader.conf to exist on the partition we're considering. If
the currdev from the image descriptor isn't sane, and it has a disk
parent (and is therefore a slice), try all the sibling slices to see
if one of them is same. Add some comments to flesh out what we're
doing and call out some missing pieces of the puzzle.

Sponsored by: Netflix

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 15530
Build 15571: arc lint + arc unit

Event Timeline

imp created this revision.Jan 6 2018, 5:26 PM
imp added a reviewer: tsoome.Jan 6 2018, 5:26 PM
cem added a subscriber: cem.Jan 6 2018, 6:12 PM

What's dual boot mean in this context? Thanks.

imp added a comment.Jan 6 2018, 6:38 PM
In D13784#288697, @cem wrote:

What's dual boot mean in this context? Thanks.

To act as either a loader kicked off by boot1.efi, or a loader living in the ESP kicked off directly by UEFI.

More patches will be needed to get full functionality, but this gets us to at least parity with the functionality we have today, modulo the absolutely insane "look at every disk in the system" stuff....

imp updated this revision to Diff 37593.Jan 6 2018, 7:15 PM

Update to actually implement timeout.

imp updated this revision to Diff 37598.Jan 7 2018, 12:32 AM

-2 now means always break to OK in case of failure. -1 means never
break. 0 means poll once and > 0 means wait up to that many seconds
for a key stroke.

kevans added a subscriber: kevans.Jan 11 2018, 9:21 PM
kevans added inline comments.Jan 11 2018, 9:28 PM
stand/efi/loader/main.c
294

laoder.conf => loader.conf =)

imp added inline comments.Jan 11 2018, 9:37 PM
stand/efi/loader/main.c
294

/boot/loader.rc I think is better.

kevans added inline comments.Mar 5 2018, 3:45 AM
stand/efi/loader/main.c
294

On further thought, I think it might be best to drop this sanity check until loader grows GELI support... I'll do some more testing tomorrow with it installed as loader.efi on my non-GELI ZFS laptop, but this test would actively break my primary laptop whether it's installed as loader.efi or to ESP.

kevans added inline comments.Mar 5 2018, 3:49 AM
stand/efi/loader/main.c
294

Not 100% true since I switch between encrypted and unencrypted /boot

tsoome added inline comments.Mar 5 2018, 7:26 AM
stand/libsa/stand.h
396 ↗(On Diff #37598)

are your sources a bit old? getsecs() seems to be there already...

kevans added a comment.Mar 5 2018, 4:19 PM

0

stand/efi/loader/main.c
274

ZFS breaks because this no longer sets currdev.d_type. zfs_fmtdev @ stand/zfs/zfs.c:703 checks dev->d_type rather than dev->d_dev->dv_type.

tsoome added inline comments.Mar 5 2018, 4:35 PM
stand/efi/loader/main.c
578

what will happen if fail_timeout is set to garbage?

imp added inline comments.Mar 5 2018, 6:07 PM
stand/efi/loader/main.c
274

Then I'll have to fix ZFS to cope.

294

/ needs to be readable. If it isn't readable, we can't use it. GELI will have to have keys loaded by the time this is called for it to work.

578

fail timeout will be 0.

stand/libsa/stand.h
396 ↗(On Diff #37598)

I've already merged this part of the diffs for other things. I published this review in January.

imp updated this revision to Diff 39966.Mar 5 2018, 6:15 PM

Rebase and address review comments

kevans accepted this revision.Mar 7 2018, 4:05 PM

With a patch to make the sibling-partition search logic work for ZFS, this seems to DTRT for me on all of my current devices- the only exception is that it doesn't do this bit [1] that "finds the best text mode" - I'm not sure if that's a good thing or a bad thing, though. =)

[1] https://svnweb.freebsd.org/base/head/stand/efi/boot1/boot1.c?view=markup#l450

This revision is now accepted and ready to land.Mar 7 2018, 4:05 PM
imp added a comment.Mar 7 2018, 4:07 PM

With a patch to make the sibling-partition search logic work for ZFS, this seems to DTRT for me on all of my current devices- the only exception is that it doesn't do this bit [1] that "finds the best text mode" - I'm not sure if that's a good thing or a bad thing, though. =)

Just to be clear, this is with the patch you sent me privately applied ti the current review, correct?

kevans added a comment.Mar 7 2018, 4:10 PM
In D13784#306813, @imp wrote:

With a patch to make the sibling-partition search logic work for ZFS, this seems to DTRT for me on all of my current devices- the only exception is that it doesn't do this bit [1] that "finds the best text mode" - I'm not sure if that's a good thing or a bad thing, though. =)

Just to be clear, this is with the patch you sent me privately applied ti the current review, correct?

Right- I assume you'll want to massage the patch a little bit, but with that patch applied it's all OK.

tsoome accepted this revision.Mar 7 2018, 4:13 PM
imp updated this revision to Diff 40038.Mar 7 2018, 4:43 PM

update with kevan's zfs stuff

This revision now requires review to proceed.Mar 7 2018, 4:43 PM
kevans accepted this revision.Mar 7 2018, 4:57 PM

New version tests good for me as-is, both on my GELI ZFS and non-GELI ZFS setups.

This revision is now accepted and ready to land.Mar 7 2018, 4:57 PM
tsoome added inline comments.Mar 8 2018, 9:05 AM
stand/efi/libefi/efizfs.c
68

hm, better return bool there.

tsoome added inline comments.Mar 8 2018, 10:30 AM
stand/efi/loader/main.c
286

while there, I did change int to bool... as we only return 0 or ENOENT and this function is bool by nature...

288–289

leftover.

405

small nit.

warning: format '%d' expects type 'int', but argument 2 has type 'time_t' [-Wformat]

perhaps use: (int)(now - then) + fail_timeout

kevans added inline comments.Mar 8 2018, 3:35 PM
stand/efi/loader/main.c
405

Now that you mention it, that should probably be something more like: fail_timeout - (int)(now - then) -- this counter is probably going the wrong way, though I've not paid attention, since now - then is always increasing and fail_timeout remains constant.

tsoome added inline comments.Mar 8 2018, 5:33 PM
stand/efi/loader/main.c
97

Note, this statement is only true when you start loader manually with chain command.

The problem is that by default, the img->DeviceHandle is handle for ESP of the booted disk, meaning that we should first find the boot disk itself, then we can check if the handle is for partition on that disk.

imp added inline comments.Mar 8 2018, 10:49 PM
stand/efi/loader/main.c
97

How do we find that disk?

imp added inline comments.Mar 8 2018, 10:56 PM
stand/efi/loader/main.c
97

Or put another way, is this a problem? If we were chain loaded, it's the right answer. If we weren't, we have to go ahunting, and that's done elsewhere.

imp updated this revision to Diff 40090.Mar 8 2018, 10:57 PM

Bool for what makes sense
fix count down

This revision now requires review to proceed.Mar 8 2018, 10:57 PM
imp marked 14 inline comments as done.Mar 8 2018, 10:59 PM
imp added inline comments.
stand/efi/loader/main.c
286

I don't want to break things that have been tested, so declining this for now.

imp marked an inline comment as done.Mar 9 2018, 2:06 AM

The current version fails as /boot/loader.efi loaded from /boot/boot1.efi, so some more work is needed here.

tsoome added inline comments.Mar 9 2018, 5:32 AM
stand/efi/loader/main.c
97

We can find this disk by fetching the devicepath, then chop off the last item (partition), then fetch handle for reminder. This is what we do in efipart.

It may or may not be issue; it really depends on how the firmware will present and order the disks - if we will always get boot disk first or not. It can be quite annoying if booted from usb disk, on disk move etc - all the scenarios where the boot variable can not be set up yet.

PS: it is not showstopper - I also did test this patch on illumos port and I was able to boot without boot1. I am in process of building such disk check (as in illumos I plan to use partition start LBA for time being).

kevans added a comment.Mar 9 2018, 4:42 PM
In D13784#307322, @imp wrote:

The current version fails as /boot/loader.efi loaded from /boot/boot1.efi, so some more work is needed here.

This version also seems to fail for me with loader.efi on the ESP...that wasn't a fun recovery. =)

kevans added a comment.Mar 9 2018, 4:50 PM
In D13784#307322, @imp wrote:

The current version fails as /boot/loader.efi loaded from /boot/boot1.efi, so some more work is needed here.

This version also seems to fail for me with loader.efi on the ESP...that wasn't a fun recovery. =)

Scratch that- actually unrelated. It *does* work on ESP on my disk, but none of these work on USB ESP.

imp updated this revision to Diff 40122.Mar 10 2018, 12:51 AM

Fix the type-punning better. Things work for me better now.

rgrimes edited reviewers, added: bhyve; removed: rgrimes.Mar 10 2018, 5:11 AM
rgrimes resigned from this revision.Mar 10 2018, 5:12 AM

Translate reviewer rgrimes -> bhyve triggered by touching stand/userboot.

imp updated this revision to Diff 40127.Mar 10 2018, 6:01 AM

Rebased and cleaned up some trivial stuff based on divergent lines on
two machines that were almost the same. Now they are the same.

tsoome added inline comments.Mar 10 2018, 6:08 AM
stand/efi/loader/main.c
391

Isnt it better to have this and efi_setenv_freebsd_wcs in libefi/env.c?

658

This should not be arbitrary value, it should come from conout->QueryMode() - see command_mode() below.

kevans added inline comments.Mar 10 2018, 2:11 PM
stand/efi/libefi/efizfs.c
73

return (false)

imp updated this revision to Diff 40220.Mar 12 2018, 9:01 PM

Refactor commit, plus add some additional refactoring

  • Minor cosmetic changes.
  • We can't use d_opendata for blkio storage.
  • Make struct libi386_devdesc match the struct devdesc better
  • Use the actual struct devdesc at the start of all *_devdesc structs
  • Remove d_type from devdesc. It's not needed as we can fetch it from
  • GC unused routines.
  • Use the one-line-per-file pattern here, and sort the file names.
  • Move the env convenience routines out of boot1.c.
  • Star BootCurrent entry when booting.
  • Print the load and device path as well as BootCurrent and BootOrder
  • Refactor currdev setting
imp updated this revision to Diff 40223.Mar 12 2018, 9:46 PM

rebase

imp updated this revision to Diff 40224.Mar 12 2018, 9:48 PM

fix overlooked review comment

tsoome added inline comments.Mar 14 2018, 10:11 AM
stand/efi/loader/main.c
79

those 2 are not needed as we have them in efienv.c

kevans accepted this revision.Apr 11 2018, 7:44 PM
This revision is now accepted and ready to land.Apr 11 2018, 7:44 PM