Page MenuHomeFreeBSD

Make loader.efi dual boot, step 1
ClosedPublic

Authored by imp on Jan 6 2018, 5:26 PM.
Tags
None
Referenced Files
F80138960: D13784.id37593.diff
Thu, Mar 28, 11:22 AM
Unknown Object (File)
Sun, Mar 17, 6:09 PM
Unknown Object (File)
Thu, Mar 7, 7:48 AM
Unknown Object (File)
Feb 22 2024, 3:49 AM
Unknown Object (File)
Feb 5 2024, 5:51 AM
Unknown Object (File)
Jan 27 2024, 8:15 PM
Unknown Object (File)
Jan 15 2024, 5:55 PM
Unknown Object (File)
Jan 6 2024, 2:22 PM

Details

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 Passed
Unit
No Test Coverage
Build Status
Buildable 15529
Build 15570: arc lint + arc unit

Event Timeline

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.

stand/efi/loader/main.c
294

laoder.conf => loader.conf =)

stand/efi/loader/main.c
294

/boot/loader.rc I think is better.

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.

stand/efi/loader/main.c
294

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

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

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

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.

stand/efi/loader/main.c
578

what will happen if fail_timeout is set to garbage?

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.

Rebase and address review comments

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

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:

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.

update with kevan's zfs stuff

This revision now requires review to proceed.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
stand/efi/libefi/efizfs.c
68

hm, better return bool there.

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

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.

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.

stand/efi/loader/main.c
97

How do we find that disk?

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.

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.

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

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

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.

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.

stand/efi/libefi/efizfs.c
73

return (false)

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

fix overlooked review comment

stand/efi/loader/main.c
79

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

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