9067 Support PMBR customisation with EFI label

Reviews
Diff

- Download Diff

Information
Submitter:	Andy Fiddaman
Repository:	illumos-gate
Branch:	master
Bugs:	9067, 9116
Depends On:
Commit:	6fbc984...
Reviewers
Groups:	general
People:	hadfl

Description

9067 Support PMBR customisation with EFI label

Testing Done

libefi:

reaper# grep Dell /usr/share/hwdata/efi.fixes | tail -1
sys.manufacturer="Dell Inc." sys.product="PowerEdge R730" mb.version=A04 pmbr_active=1 pmbr_slot=3

reaper# zpool create test c0t13d1
reaper# fdisk -W - c0t13d1p0 | tail -5
* Id    Act  Bhead  Bsect  Bcyl    Ehead  Esect  Ecyl    Rsect      Numsect
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  238   128  255    63     1023    255    63     1023    1          62499999

reaper# zpool replace test c0t{13,14}d1
reaper# fdisk -W - c0t14d1p0 | tail -5
* Id    Act  Bhead  Bsect  Bcyl    Ehead  Esect  Ecyl    Rsect      Numsect
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  238   128  255    63     1023    255    63     1023    1          62499999

reaper# sed -i '/PowerEdge/s/active=1/active=0/' /usr/share/hwdata/efi.fixes
reaper# zpool replace test c0t{14,13}d1
reaper# fdisk -W - c0t13d1p0 | tail -5
* Id    Act  Bhead  Bsect  Bcyl    Ehead  Esect  Ecyl    Rsect      Numsect
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  238   0    255    63     1023    255    63     1023    1          62499999

reaper# sed -i '/PowerEdge/s/slot=3/slot=1/' /usr/share/hwdata/efi.fixes
reaper# zpool replace test c0t{13,14}d1
reaper# fdisk -W - c0t14d1p0 | tail -5
* Id    Act  Bhead  Bsect  Bcyl    Ehead  Esect  Ecyl    Rsect      Numsect
  0     0    0      0      0       0      0      0       0          0
  238   0    255    63     1023    255    63     1023    1          62499999
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0

reaper# sed -i '/PowerEdge/s/^/#/' /usr/share/hwdata/efi.fixes
reaper# zpool replace test c0t{14,13}d1
reaper# fdisk -W - c0t13d1p0 | tail -5
* Id    Act  Bhead  Bsect  Bcyl    Ehead  Esect  Ecyl    Rsect      Numsect
  238   0    255    63     1023    255    63     1023    1          62499999
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0

fdisk:

reaper# zpool export test
reaper# fdisk -E 2:1 c0t13d1p0
reaper# fdisk -W - c0t13d1p0 | tail -5
* Id    Act  Bhead  Bsect  Bcyl    Ehead  Esect  Ecyl    Rsect      Numsect
  0     0    0      0      0       0      0      0       0          0
  0     0    0      0      0       0      0      0       0          0
  238   128  255    63     1023    255    63     1023    1          62499999
  0     0    0      0      0       0      0      0       0          0
reaper# zpool import test
reaper# zpool status test
  pool: test
 state: ONLINE
  scan: resilvered 86K in 0h0m with 0 errors on Mon Feb 12 10:46:49 2018
config:

        NAME        STATE     READ WRITE CKSUM
        test        ONLINE       0     0     0
          c0t13d1   ONLINE       0     0     0

errors: No known data errors

Issues

0
1
0
All Issues: 1

Description	From	Last Updated
I think this (and 2265) would need issue and explanation - you can combine multiple issues in single update.	Toomas Soome	Feb. 15, 2018, 4:25 p.m.

I really don't feel like providing a "default" file is correct solution here, could it be done in say zpool command instead, same way we added the -B option?

Andy Fiddaman Feb. 8, 2018, 12:30 p.m.

The problem here is that the PMBR is created in libefi::efi_write(int fd, dk_gpt_t *vtoc) which is a committed interface used by a range of things, not just zpool - for example format and fmthard both use it.

Options that I also considered:
1. add new flags to dk_gpt_t.efi_flags - for the known firmware problems I actually only need two bits, set partition active and put partition in slot 1 - and then add parameters/options to zpool to set these bits on pool creation;
2. update fdisk with new options (or possibly extend -E) so that the PMBR can be modified (by an installer, say) following pool creation.

All have pros and cons and I'd welcome any discussion around this.

Yuri Pankov Feb. 8, 2018, 12:56 p.m.

I must admit that partitioning in solarish world is one of the most unpleasing things for me, and I'm trying to look at the related code, and even think about it, as little as possible.

What I wanted to say is that creating a boot pool should be a single time operation, done mostly in the installer, so providing a tunable in /etc/default/something does look a bit weird.

Toomas Soome Feb. 8, 2018, 1:11 p.m.

The default file has obvious issue, in install image you have (possibly) read only /etc/default.

The question there is, do we have list of "known" bad behaving systems and can hide all this mess and populate PMBR based on hw info from smbios? This list can not be too long:)

Andy Fiddaman Feb. 8, 2018, 1:15 p.m.

Thanks to FreeBSD, yes - see https://github.com/citrus-it/kayak/blob/46ef74e49abff7cfeb8c482f7d54922fe2fbf91d/hwfixes.sh

Andy Fiddaman Feb. 8, 2018, 1:44 p.m.

So we have a fourth option which is to link libefi with libsmbios and put the logic there with an external known-bad-hardware database to make the entire thing automatic.
I'm actually currently leaning towards adding this function to fdisk as it could be used by installers but also by people moving disks from a good to a bad system - just fdisk -E:1,1 or something and fix-up the PMBR.

Andy Fiddaman Feb. 8, 2018, 1:48 p.m.

FWIW, FreeBSD installer does gpart set -a lenovofix /dev/... and gpart set -a active /dev/... for affected systems.

Toomas Soome Feb. 8, 2018, 1:53 p.m.

Having the utility support would be very nice for manual fix; I still would like to have it sort of done automatically for user too because it is one thing we can have automated and therefore there is no reason why we should not do it...

also keep in mind that we will get disks relabeled on zpool replace (see /etc/sysevent/*).

Toomas Soome Feb. 8, 2018, 1:55 p.m.

yea, I have long dream of having gpart ported (the command and UI logic, not geom;) and implemented, but just do not have time. We really need properly scriptable partition management tool, but thats different topic anyhow.

Yuri Pankov Feb. 8, 2018, 1:57 p.m.

Would that mean getting rid of all minors we expose now by default? :-)

Andy Fiddaman Feb. 8, 2018, 2:47 p.m.

Leave it with me - I will rework this when I get time.

Testing Done:

~		All variations of parameters in /etc/default/efi, separate and together, out-of-bounds values, non-existent and empty file, file containing random text.
	~	libefi:
-
-		Examples:



~		bloody# rm /etc/default/efi
~		bloody# LD_LIBRARY_PATH=proto/root_i386/usr/lib zpool create test c4t0d0
~		bloody# fdisk -W - c4t0d0 \| tail -5
	~	reaper# grep Dell /usr/share/hwdata/efi.fixes \| tail -1
	~	sys.manufacturer="Dell Inc." sys.product="PowerEdge R730" mb.version=A04 pmbr_active=1 pmbr_slot=3
	~
	+	reaper# zpool create test c0t13d1
	+	reaper# fdisk -W - c0t13d1p0 \| tail -5
	+	* Id Act Bhead Bsect Bcyl Ehead Esect Ecyl Rsect Numsect
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0
	+	238 128 255 63 1023 255 63 1023 1 62499999
	+
	+	reaper# zpool replace test c0t{13,14}d1
	+	reaper# fdisk -W - c0t14d1p0 \| tail -5
	+	* Id Act Bhead Bsect Bcyl Ehead Esect Ecyl Rsect Numsect
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0
	+	238 128 255 63 1023 255 63 1023 1 62499999
	+
	+	reaper# sed -i '/PowerEdge/s/active=1/active=0/' /usr/share/hwdata/efi.fixes
	+	reaper# zpool replace test c0t{14,13}d1
	+	reaper# fdisk -W - c0t13d1p0 \| tail -5
		* Id Act Bhead Bsect Bcyl Ehead Esect Ecyl Rsect Numsect
-		238 0 255 63 1023 255 63 1023 1 16777215
		0 0 0 0 0 0 0 0 0 0
		0 0 0 0 0 0 0 0 0 0
		0 0 0 0 0 0 0 0 0 0
	+	238 0 255 63 1023 255 63 1023 1 62499999
	+
	+	reaper# sed -i '/PowerEdge/s/slot=3/slot=1/' /usr/share/hwdata/efi.fixes
	+	reaper# zpool replace test c0t{13,14}d1
	+	reaper# fdisk -W - c0t14d1p0 \| tail -5
	+	* Id Act Bhead Bsect Bcyl Ehead Esect Ecyl Rsect Numsect
	+	0 0 0 0 0 0 0 0 0 0
	+	238 0 255 63 1023 255 63 1023 1 62499999
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0
	+
	+	reaper# sed -i '/PowerEdge/s/^/#/' /usr/share/hwdata/efi.fixes
	+	reaper# zpool replace test c0t{14,13}d1
	+	reaper# fdisk -W - c0t13d1p0 \| tail -5
	+	* Id Act Bhead Bsect Bcyl Ehead Esect Ecyl Rsect Numsect
	+	238 0 255 63 1023 255 63 1023 1 62499999
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0
	+	0 0 0 0 0 0 0 0 0 0



	+
	+	fdisk:



~		bloody# cat /etc/default/efi
~		PMBR_SLOT=2
~		PMBR_ACTIVE=1
	~	reaper# zpool export test
	~	reaper# fdisk -E 2:1 c0t13d1p0
	~	reaper# fdisk -W - c0t13d1p0 \| tail -5
-		bloody# LD_LIBRARY_PATH=proto/root_i386/usr/lib zpool create test c4t0d0
-		bloody# fdisk -W - c4t0d0 \| tail -5
		* Id Act Bhead Bsect Bcyl Ehead Esect Ecyl Rsect Numsect
		0 0 0 0 0 0 0 0 0 0
		0 0 0 0 0 0 0 0 0 0
~		238 128 255 63 1023 255 63 1023 1 16777215
	~	238 128 255 63 1023 255 63 1023 1 62499999
		0 0 0 0 0 0 0 0 0 0
	+	reaper# zpool import test
	+	reaper# zpool status test
	+	pool: test
	+	state: ONLINE
	+	scan: resilvered 86K in 0h0m with 0 errors on Mon Feb 12 10:46:49 2018
	+	config:
	+
	+	NAME STATE READ WRITE CKSUM
	+	test ONLINE 0 0 0
	+	c0t13d1 ONLINE 0 0 0
	+
	+	errors: No known data errors

Bugs:

+	9067

People:

+	hadfl

Change Summary:

Round 2 - different implementation with two parts.

fdisk -E extended to take arguments so that the PMBR can be modified if necessary (useful for testing, patching, or moving disks between systems).
libefi - automatically tweak the PMBR for broken systems as defined in a separate database (text-file). The benefit of doing it here is that all consumers of libefi benefit and things like replacing a disk with zpool replace work as expected - the replacement disk gets the modified PMBR too.

I'm still unsure about where the database file should be in the filesystem but I think it is right to set preserve=renamenew since anyone who has modified this file has done it to support the host system and won't want the changes to be lost on an upgrade (and likely won't care about changes in that file since it's working for them).

There was a bug in fdisk too that caused the EFI labels to always be cleared, even if the old and new PMBRs both contained an EFI partition of the same size and location. Please see testing notes for a summary of this working on a test system.

Lastly, the database file still needs a copyright adding. Since the data therein is derived from the FreeBSD installer, it likely needs to include theirs too but advice welcome.

Comments appreciated, thanks!

Commit:

-	5949c81bd28cd25545a119f3abf31350fea818b2
+	f79072b46f061a8352cc5a3b5b78981fc3bd918e

Diff:

Revision 2 (+300 -45)

Show changes

	usr/src/cmd/fdisk/fdisk.c
	usr/src/data/hwdata/Makefile
	usr/src/data/hwdata/efi.fixes
	usr/src/lib/libefi/Makefile.com
	usr/src/lib/libefi/common/rdwr_efi.c
	usr/src/man/man1m/fdisk.1m
	usr/src/man/man3ext/efi_alloc_and_init.3ext
	usr/src/pkg/manifests/system-data-hardware-registry.mf

Can't speak for the actually code but I do like the new design.

/system/boot/kernel/drv <== driver configs like sd.conf live there. Maybe somewhere close to that location would be a good place to store it?

Change Summary:

Made smbios string checks case-insensitive. Added some missing (void) casts.

Commit:

-	f79072b46f061a8352cc5a3b5b78981fc3bd918e
+	2fc497edd33a9a9ed52b295733e2b7f2e9946515

Diff:

Revision 3 (+326 -45)

Show changes

	usr/src/cmd/fdisk/fdisk.c
	usr/src/data/hwdata/Makefile
	usr/src/data/hwdata/efi.fixes
	usr/src/lib/libefi/Makefile.com
	usr/src/lib/libefi/common/rdwr_efi.c
	usr/src/man/man1m/fdisk.1m
	usr/src/man/man3ext/efi_alloc_and_init.3ext
	usr/src/pkg/manifests/system-data-hardware-registry.mf

Ship it!

Successfully installed OmniOS on my LENOVO ThinkPad X1 with an install imaged based on this code provided by Andy.

usr/src/cmd/fdisk/fdisk.c (Diff revision 3)

Show all issues

I think this (and 2265) would need issue and explanation - you can combine multiple issues in single update.

Change Summary:

Modify maximum HEAD globally in fdisk.c and adjust, include new bug number too.

Bugs:

+	9116

Commit:

-	2fc497edd33a9a9ed52b295733e2b7f2e9946515
+	77375801700bef7b7604fe84c1081e73aae16b02

Diff:

Revision 4 (+325 -45)

Show changes

	usr/src/cmd/fdisk/fdisk.c
	usr/src/data/hwdata/Makefile
	usr/src/data/hwdata/efi.fixes
	usr/src/lib/libefi/Makefile.com
	usr/src/lib/libefi/common/rdwr_efi.c
	usr/src/man/man1m/fdisk.1m
	usr/src/man/man3ext/efi_alloc_and_init.3ext
	usr/src/pkg/manifests/system-data-hardware-registry.mf

Ship it!

Ship It!

The new BSD licensed file likely needs the THIRDPARTYLICENSE stuff?

Andy Fiddaman Feb. 15, 2018, 9:16 p.m.

I don't know - would appreciate some assistance with this issue.
That file didn't exactly come from FreeBSD, it's just that the information in there about what systems are affected and what fix should be applied is derived from a file in their installer - their file looks nothing like this.

I obviously want to properly attribute the source, so what does it need?

Change Summary:

Add THIRDPARTYLICENSE for efi.fixes database. Remove duplicate MAX_ definitions from fdisk.c.

Commit:

-	77375801700bef7b7604fe84c1081e73aae16b02
+	6fbc984785709201926a739f689355d3b541f4e7

Diff:

Revision 5 (+369 -64)

Show changes

	exception_lists/copyright
	usr/src/cmd/fdisk/fdisk.c
	usr/src/data/hwdata/Makefile
	usr/src/data/hwdata/THIRDPARTYLICENSE.efifixes.tmpl
	usr/src/data/hwdata/THIRDPARTYLICENSE.efifixes.descrip
	usr/src/data/hwdata/efi.fixes
	usr/src/lib/libefi/Makefile.com
	usr/src/lib/libefi/common/rdwr_efi.c
	3 more

Ship it!

Ship It!

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

Testing Done:

Bugs:

People:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Bugs:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)