Page MenuHomeFreeBSD

mkimg: Ensure GPT Entry Array is at least 16k
ClosedPublic

Authored by imp on Oct 17 2023, 2:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 1, 7:58 AM
Unknown Object (File)
Sat, Nov 30, 5:57 AM
Unknown Object (File)
Fri, Nov 29, 7:26 AM
Unknown Object (File)
Wed, Nov 27, 9:21 PM
Unknown Object (File)
Tue, Nov 26, 3:14 PM
Unknown Object (File)
Sun, Nov 24, 1:15 AM
Unknown Object (File)
Fri, Nov 22, 6:08 PM
Unknown Object (File)
Tue, Nov 19, 8:32 PM
Subscribers

Details

Summary

The GPT standard requires that we reserve either 34 512 sectors, or 6 4k
sectors before starting partitions. However, we don't currently don't
enforce these minimums. This is 2 sectors + 16k of space.

Sponsored by: Netflix

Test Plan

The old code produces

00000200  45 46 49 20 50 41 52 54  00 00 01 00 5c 00 00 00  |EFI PART....\...|
00000210  ea c7 2f 95 00 00 00 00  01 00 00 00 00 00 00 00  |../.............|
00000220  04 60 09 00 00 00 00 00  03 00 00 00 00 00 00 00  |.`..............|
00000230  02 60 09 00 00 00 00 00  12 7a a9 63 4a f6 ed 11  |.`.......z.cJ...|
00000240  a7 d3 e0 d5 5e 1e 73 bd  02 00 00 00 00 00 00 00  |....^.s.........|
00000250  04 00 00 00 80 00 00 00  58 9c b0 81 00 00 00 00  |........X.......|

Which has 3 is the FirstLBA and 4 as the number of entries.

The correct output I claim is:

00000200  45 46 49 20 50 41 52 54  00 00 01 00 5c 00 00 00  |EFI PART....\...|
00000210  d8 2e 74 5a 00 00 00 00  01 00 00 00 00 00 00 00  |..tZ............|
00000220  42 60 09 00 00 00 00 00  22 00 00 00 00 00 00 00  |B`......".......|
00000230  21 60 09 00 00 00 00 00  84 7b f1 cc 98 6c ee 11  |!`.......{...l..|
00000240  a8 54 a0 36 9f 09 4f 1e  02 00 00 00 00 00 00 00  |.T.6..O.........|
00000250  80 00 00 00 80 00 00 00  71 5c 49 7b 00 00 00 00  |........q\I{....|

which has first LBA is 0x22 / 34 and num entires of 0x80 / 128. These are the desired values.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp requested review of this revision.Oct 17 2023, 2:28 AM
imp created this revision.

Include sys/param.h for MIN macro

braino: min will return the smaller number. We want it to return the
larger of the computed size and the minimal size.

imp edited the test plan for this revision. (Show Details)
emaste added inline comments.
sys/sys/disk/gpt.h
87
This revision is now accepted and ready to land.Oct 17 2023, 3:49 AM
usr.bin/mkimg/gpt.c
131

I find this function a little confusing as written, but that predates and is independent of your change.

usr.bin/mkimg/gpt.c
131

Yea... we need a roundup macro...

jrtc27 added inline comments.
usr.bin/mkimg/gpt.c
29

param comes before all headers other than cdefs, though not really sure why cdefs is included in the first place (left over from $FreeBSD$?), and there should be a blank line between the sys/ and non-sys/ includes anyway, so this isn't exactly the most style-conforming already

131

howmany from sys/param.h? #define howmany(x, y) (((x)+((y)-1))/(y))

usr.bin/mkimg/gpt.c
29

Indeed cdefs is not needed. Changing the first include to #include <sys/param.h> results in an identical binary (other than debug info).

It would be really good to get (some version of) this into the tree and on the build machine before the next RC build.
CC releng

imp retitled this revision from mkimg: Conform to the GPT standard by reserving at least 16k for GPT entries to mkimg: Ensure GPT Entry Array is at least 16k.Oct 17 2023, 2:37 PM
imp edited the test plan for this revision. (Show Details)

Update headers included, use howmany and improve (I claim) the comments
and commit message (which isn't updated automatically, alas)

This revision now requires review to proceed.Oct 17 2023, 2:39 PM

which isn't updated automatically, alas

You can edit it through the Phab web UI though.

my comment in gpt.h still not addressed but gpt_tblsize is definitely an improvement

usr.bin/mkimg/gpt.c
133

Thanks, variable name ents was part of the confusion before

141

maybe also homany(GPT_MIN_RESERVED, secsz)?

This revision is now accepted and ready to land.Oct 17 2023, 2:47 PM
imp marked 4 inline comments as done.Oct 17 2023, 3:15 PM
imp added inline comments.
usr.bin/mkimg/gpt.c
141

No. Sector sizes are required to be a power of 2. There's no rounding needed. I think elsewhere we don't bother... And I'd like to keep the number of 'one more things' to a minimum.

Not sure what a 16k sector size would do, or how larger sector sizes would work here. But both a 16k and 64k sector size would result in this returning 1. The standard is silent on sector sizes that aren't 512 or 4k.

address comments about gpt.h

This revision now requires review to proceed.Oct 17 2023, 3:18 PM
emaste added inline comments.
usr.bin/mkimg/gpt.c
141

It's not secsz I was thinking about, but GPT_MIN_RESERVED. 16384 is (obviously) a power of two, but the constant is defined elsewhere and so it's not immediately obvious when looking at this expression.

This revision is now accepted and ready to land.Oct 17 2023, 3:23 PM
This revision was automatically updated to reflect the committed changes.