Details

Reviewers

tsoome
kevans
rgrimes

Group Reviewers

bhyve

Commits

rS332416: Refactor currdev setting

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.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 14160
Build 14330: arc lint + arc unit

Event Timeline

imp created this revision.Jan 6 2018, 5:26 PM

Harbormaster completed remote builds in B14160: Diff 37591.Jan 6 2018, 5:26 PM

imp added a reviewer: tsoome.Jan 6 2018, 5:26 PM

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

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

Update to actually implement timeout.

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

Harbormaster completed remote builds in B14166: Diff 37598.Jan 7 2018, 12:32 AM

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
223	laoder.conf => loader.conf =)

imp added inline comments.Jan 11 2018, 9:37 PM

stand/efi/loader/main.c
223	/boot/loader.rc I think is better.

kevans added inline comments.Mar 5 2018, 3:45 AM

stand/efi/loader/main.c
223	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
223	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	are your sources a bit old? getsecs() seems to be there already...

stand/efi/loader/main.c
203	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
484	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
203	Then I'll have to fix ZFS to cope.
223	/ 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.
484	fail timeout will be 0.
stand/libsa/stand.h
396	I've already merged this part of the diffs for other things. I published this review in January.

Rebase and address review comments

Harbormaster completed remote builds in B15376: Diff 39966.Mar 5 2018, 6:15 PM

Herald added a subscriber: delphij. · View Herald TranscriptMar 5 2018, 6:15 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

In D13784#306810, @kevans 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?

In D13784#306813, @imp wrote:

In D13784#306810, @kevans 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

update with kevan's zfs stuff

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

Harbormaster completed remote builds in B15410: Diff 40038.Mar 7 2018, 4:43 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
69 ↗	(On Diff #40038)	hm, better return bool there.

tsoome added inline comments.Mar 8 2018, 10:30 AM

stand/efi/loader/main.c
226	while there, I did change int to bool... as we only return 0 or ENOENT and this function is bool by nature...
229	leftover.
339	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
339	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
81	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
81	How do we find that disk?

imp added inline comments.Mar 8 2018, 10:56 PM

stand/efi/loader/main.c
81	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.

Bool for what makes sense
fix count down

This revision now requires review to proceed.Mar 8 2018, 10:57 PM

Harbormaster completed remote builds in B15444: Diff 40090.Mar 8 2018, 10:58 PM

imp marked 14 inline comments as done.Mar 8 2018, 10:59 PM

imp added inline comments.

stand/efi/loader/main.c
226	I don't want to break things that have been tested, so declining this for now.

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

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

In D13784#307376, @kevans wrote:

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.

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

Herald added a reviewer: rgrimes. · View Herald TranscriptMar 10 2018, 12:51 AM

Harbormaster completed remote builds in B15463: Diff 40122.Mar 10 2018, 12:51 AM

rgrimes edited reviewers, added: bhyve; removed: rgrimes.Mar 10 2018, 5:11 AM

Herald added a reviewer: rgrimes. · View Herald TranscriptMar 10 2018, 5:11 AM

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

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

Harbormaster completed remote builds in B15467: Diff 40127.Mar 10 2018, 6:01 AM

tsoome added inline comments.Mar 10 2018, 6:08 AM

stand/efi/loader/main.c
325	Isnt it better to have this and efi_setenv_freebsd_wcs in libefi/env.c?
526	This should not be arbitrary value, it should come from conout->QueryMode() - see command_mode() below.