Page MenuHomeFreeBSD

Add a new option to gpart(8) to fix Lenovo BIOS boot issue
ClosedPublic

Authored by allanjude on Jul 13 2015, 3:24 AM.

Details

Summary

Add an option to gpart(8) to location the fake GPT (0xee) entry in the pMBR in a non-default location
Writing the 0xee entry to the 2nd partition slot, instead of the first, allows afflicted Lenovo's (x220, t420, t520) to boot GPT in BIOS mode

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude updated this revision to Diff 6886.Jul 13 2015, 3:24 AM
allanjude retitled this revision from to Add a new option to gpart(8) to fix Lenovo BIOS boot issue.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: pjd, mav, trasz, scottl, imp, wblock, bcr, eadler.
allanjude added inline comments.Jul 13 2015, 3:26 AM
sys/geom/part/g_part_gpt.c
998 ↗(On Diff #6886)

When 'gpart recover' is used, it will undo the lenovo fix. the pmbr is not backed up, so there is no way to tell if the fix had previously been applied

ae added a reviewer: marcel.Jul 13 2015, 5:40 PM
ae added a subscriber: ae.Jul 13 2015, 5:50 PM

I prefer to not introduce new option. I think it will be better add new scheme-specific attribute. Something like

  1. gpart set -a lenovofix ada0

or

  1. gpart set -a pmbrentry -i 2 ada0
In D3065#60637, @ae wrote:

I prefer to not introduce new option. I think it will be better add new scheme-specific attribute. Something like

  1. gpart set -a lenovofix ada0 or
  2. gpart set -a pmbrentry -i 2 ada0

The advantage to doing this as a 'set' command is that it makes it easy to apply after-the-fact as well, instead of only at create time, which resolves the potential issue with 'gpart recover'

I don't think it will be possible to overload the -i flag like in your second example, but considering the use case, the first example seems like it should work.

ae added a comment.Jul 13 2015, 5:59 PM

I don't think it will be possible to overload the -i flag like in your second example, but considering the use case, the first example seems like it should work.

In g_part_gpt_setunset() you have the attribute name and pointer to partition entry baseentry, where you know the number of partition entry.

In D3065#60654, @ae wrote:

I don't think it will be possible to overload the -i flag like in your second example, but considering the use case, the first example seems like it should work.

In g_part_gpt_setunset() you have the attribute name and pointer to partition entry baseentry, where you know the number of partition entry.

Right, but in the case of 'gpart set -a <flag> -i <index> ada0'

the -i refers to the <index>'th GPT partition, which is unrelated to the fake pMBR partition table.

But I think 'gpart set -a lenovofix ada0' makes sense, and I'll rewrite the patch to do it that way.

allanjude updated this revision to Diff 6907.Jul 14 2015, 5:06 AM

Rewrite the patch to use 'gpart set -a lenovofix <provider>'
This is cleaner than adding a new flag to 'gpart create' and allows it to be done at any time

ae added a comment.Jul 14 2015, 5:40 AM

Maybe you need to mention that after apply lenovofix, you will lose all PMBR entries. So if you have configured bootcamp, or have set/unset "active" flag - all these changes will be lost. Also you forgot to check/handle, what type of action user asks - set or unset.

sbin/geom/class/part/gpart.8
928 ↗(On Diff #6907)

Don't forget to bump .Dd

sys/geom/part/g_part_gpt.c
1054 ↗(On Diff #6907)

This isn't necessary, you didn't touch PMBR's magic.

wblock added inline comments.Jul 14 2015, 7:08 AM
sbin/geom/class/part/gpart.8
937 ↗(On Diff #6907)

Is "pMBR" the preferred capitalization? It is not listed anywhere else in gpart.8.

Also:
s/2nd/second/

938 ↗(On Diff #6907)

Start a new sentence on a new line. The moose out front should have told you.

marcel edited edge metadata.Jul 14 2015, 2:19 PM

As for bootcamp: we don't create it and one doesn't need the lenovo fix on machines that have bootcamp. I don't think we should over- engineer a pure theoretical case. Likewise for the active flag -- that is, if the active flag must be set on Lenovo machines, it better be set when the lenovo fix is applied.

While technically speaking the above is not perfect, we're talking about tweaks to make certain machines boot (whether active flag or moving the 0xee partition to the second entry). It's more concerning that recover will undo the tweak, though.

sbin/geom/class/part/gpart.8
937 ↗(On Diff #6907)

It's PMBR, with a capital P. It's probably also better not to call it a "GPT PMBR", but rather just "PMBR" or alternatively "Protective MBR".

allanjude updated this revision to Diff 6939.Jul 14 2015, 11:32 PM
allanjude marked 5 inline comments as done.
allanjude edited edge metadata.

Update with feedback from everyone

  • do not rewrite the DOSMAGIC
  • unset now restores an ordinary PMBR
  • man page fixes
marcel accepted this revision.Jul 14 2015, 11:52 PM
marcel edited edge metadata.

Thanks Allan!

This revision is now accepted and ready to land.Jul 14 2015, 11:52 PM
In D3065#60972, @marcel wrote:

Thanks Allan!

As I am currently only a doc committer, I need an 'Approved By:' to commit this. Are you willing to provide that?

wblock accepted this revision.Jul 15 2015, 12:34 AM
wblock edited edge metadata.

One note, but man page is otherwise good. Remember to test with igor and mandoc -Tlint before commit.

sbin/geom/class/part/gpart.8
939 ↗(On Diff #6939)

"in" seems... not right. I'd suggest "on" or "with" instead.

allanjude updated this revision to Diff 6942.Jul 15 2015, 12:36 AM
allanjude edited edge metadata.

Updated with feedback from wblock

This revision now requires review to proceed.Jul 15 2015, 12:36 AM
wblock accepted this revision.Jul 15 2015, 12:45 AM
wblock edited edge metadata.

Man page is good.

This revision is now accepted and ready to land.Jul 15 2015, 12:45 AM

You can add me as approver.

This revision was automatically updated to reflect the committed changes.