Revert efipart to use EFI_HANDLEs for partitions
Needs ReviewPublic

Authored by eric_metricspace.net on Oct 19 2017, 9:47 PM.

Details

Summary

At some point, the efipart driver started using the EFI_HANDLE for the base disk, and manually reading and parsing partition tables off the disk. This will not work with GELI, since the base disk is encrypted. This patch reverts the system to use the EFI_HANDLE for the partition, and eliminates manual parsing of partition tables completely.

Test Plan

This was tested successfully with an earlier version of the GELI work; however, merge conflicts occurred and this has not been retested.

It needs to be retested, though a basic smoke-test should suffice.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
tsoome requested changes to this revision.Oct 20 2017, 9:02 AM

This patch will need more work and cleanup, especially printing information about the devices.

sys/boot/efi/libefi/efipart.c
376 ↗(On Diff #34157)

First of all, this does not belong here, and secondly, use ucs2len instead, see efichar.c

386 ↗(On Diff #34157)

This function does not seem to be used at all.

393 ↗(On Diff #34157)

Use printf %S here and let printf to deal with conversion details.

661 ↗(On Diff #34157)

By removing this, we do not get any information about partition types etc, and this is not good for users. And we will end up having the same unmanageable mess we had with partX: devices.

One principal purpose for using disk/part interface was to reuse the partition management info to provide familiar info for users. Note that you can create and populate partition info based on media device path etc, but this will require some investigation of course.

This revision now requires changes to proceed.Oct 20 2017, 9:02 AM
eric_metricspace.net marked 3 inline comments as done.Oct 23 2017, 11:01 PM
eric_metricspace.net added inline comments.
sys/boot/efi/libefi/efipart.c
376 ↗(On Diff #34157)

This was old debug logging stuff that remained It's gone.

661 ↗(On Diff #34157)

This won't work with GELI at all, because you end up trying to parse an encrypted disk.

A better way, I suspect, would be to get the information from the device paths.

eric_metricspace.net marked an inline comment as done.Oct 23 2017, 11:02 PM
eric_metricspace.net marked an inline comment as done.

OK, I know how to deal with the partition info from disks. I'm going to add a field to the pdinfo_list which contains the partition type, and I'll pluck it out of the devpaths when we register the partitions. The partition relationships can be obtained from the pdinfo list already.

sys/boot/efi/libefi/efipart.c
419 ↗(On Diff #34157)

This stuff jumped in here prematurely. It won't do any harm, since we won't get any partitions of this type, but it's probably better to put it in with GELI.

(Sorry for the extreme delay on this one)

Restored detailed output from lsdev.

Still looking quite nice, but there is a bit of competition still, have you checked on https://reviews.freebsd.org/D13784 and how much those 2 updates are conflicting and in which order should we implement them to cause the least amount of issues on integration?

Still looking quite nice, but there is a bit of competition still, have you checked on https://reviews.freebsd.org/D13784 and how much those 2 updates are conflicting and in which order should we implement them to cause the least amount of issues on integration?

It looks like that one is doing what I wanted to do with my own dual-purpose loader patch. I'm probably going to have to redo this one after D13784 lands.

Confirmed working on UFS, ZFS, and real hardware.

Currently, something gets freed incorrectly if no partitions are selected, but I want to get this up for review.

Rebase from master and tried on real hardware

tsoome added a comment.Fri, Jun 8, 7:26 AM

Rebase from master and tried on real hardware

There is one concern - there has been some rework of usb boot images, I think this update should be tested with those new images too, otherwise we risk cutting ourselves..

imp added a comment.Fri, Jun 8, 5:43 PM

Rebase from master and tried on real hardware

There is one concern - there has been some rework of usb boot images, I think this update should be tested with those new images too, otherwise we risk cutting ourselves..

Eric is here at BSDcan. I independently asked him to test out the bsd label inside a MBR case.