Page MenuHomeFreeBSD
Differential D3065

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

Authored by allanjude on Jul 13 2015, 3:24 AM.
Tags
None
Referenced Files
F103584825: D3065.diff
Tue, Nov 26, 6:55 PM
F103532833: D3065.diff
Tue, Nov 26, 4:44 AM
Unknown Object (File)
Sun, Nov 24, 10:53 PM
Unknown Object (File)
Mon, Nov 11, 2:56 PM
Unknown Object (File)
Fri, Nov 8, 2:52 AM
Unknown Object (File)
Thu, Nov 7, 6:07 AM
Unknown Object (File)
Tue, Nov 5, 1:29 AM
Unknown Object (File)
Oct 18 2024, 9:27 PM
View All Files
Subscribers
ae
imp

Details

Reviewers
scottl
eadler
trasz
wblock
marcel
imp
mav
pjd
bcr
Group Reviewers
manpages
Commits
rS285594: Add a new option to gpart(8) to fix Lenovo BIOS boot issue
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 - subversion
Lint
Lint Passed
Unit
No Test Coverage

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.
Herald added a reviewer: manpages. · View Herald TranscriptJul 13 2015, 3:24 AM
Herald added a subscriber: imp. · View Herald Transcript
allanjude added inline comments.Jul 13 2015, 3:26 AM
sys/geom/part/g_part_gpt.c
998

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
allanjude added a comment.Jul 13 2015, 5:53 PM
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

  1. 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
In D3065#60653, @allanjude 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.

allanjude added a comment.Jul 13 2015, 6:02 PM
In D3065#60654, @ae wrote:
In D3065#60653, @allanjude 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

Don't forget to bump .Dd

sys/geom/part/g_part_gpt.c
1054

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

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

Also:
s/2nd/second/

938

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

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
allanjude added a comment.Jul 15 2015, 12:10 AM
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

"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
marcel added a comment.Jul 15 2015, 1:54 AM

You can add me as approver.

Closed by commit rS285594: Add a new option to gpart(8) to fix Lenovo BIOS boot issue (authored by allanjude). · Explain WhyJul 15 2015, 2:24 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sbin/
geom/
class/
part/
5 lines
sys/
geom/
part/
17 lines

Diff 6907

View Options

sbin/geom/class/part/gpart.8

Loading...
View Options

sys/geom/part/g_part_gpt.c

Loading...