boot1 generate-fat: generate all templates at once
ClosedPublic

Authored by emaste on May 26 2017, 1:47 PM.

Details

Summary

I want to commit D9680. I think it makes sense to have generate-fat create all templates at once and would like to make that change in advance.
Also fix a longstanding bug that the BOOT1_OFFSET for all archs is taken from the last template generated. This did not cause trouble because it's the same on all archs in practice.

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.
emaste created this revision.May 26 2017, 1:47 PM
allanjude added inline comments.May 26 2017, 4:14 PM
sys/boot/efi/boot1/generate-fat.sh
71 ↗(On Diff #28852)

Do we still want to uuencode stuff? I thought we decided binaries made more sense.

emaste added inline comments.May 26 2017, 4:15 PM
sys/boot/efi/boot1/generate-fat.sh
71 ↗(On Diff #28852)

I was thinking I'd make that change along with the size bump from 128k to 512k, to avoid extra churn.

imp added inline comments.May 27 2017, 12:03 AM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

what would cause the offset to be different?

Also, when can we nuke this from orbit and use makefs instead?

emaste added inline comments.
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

I wouldn't expect them to be different, but nothing guaranteed that before, and if it did happen the boot file would just be inserted at the wrong offset on some architecture(s).

@smahadevan_freebsdfoundation.org is now at the "it compiles and starts, but segfaults" stage of adding msdos support to makefs.

imp added inline comments.May 27 2017, 5:59 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

There's nothing in the UEFI standard that would make the offset different. There's no need, imho, to make things overly complicated like this, especially since the 'future proof' argument is counterweighted by ongoing work to move to makefs.

emaste added inline comments.May 27 2017, 6:04 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

The offset has nothing to do with the UEFI standard; it's just the offset in the filesystem image where msdosfs(5) happened to put the first block of a dummy file.

I don't see this as making things overly complicated, just addressing a longstanding bug which would have been rather hard to diagnose if we tripped over it.

imp added inline comments.May 27 2017, 6:06 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

But the template will be the same on all systems.... It's just an MSDOSFS we're kludgily plopping data into.... We totally control that... Just think it will be more complicated code that eventually will be ripped out....

emaste added inline comments.May 27 2017, 6:06 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

Well, excluding that UEFI incorporates FAT by reference I mean.

emaste added inline comments.May 27 2017, 6:14 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

OK, but relative to the refactoring here to loop over all archs this offset issue is a 2 line change (one in generate-fat.sh and one in Makefile).

To avoid this would be even more complicated, with something like:

BOOT1_KNOWN_OFFSET=2d
echo "BOOT1_OFFSET=0x$BOOT1_KNOWN_OFFSET" >> Makefile.fat

before the loop and then

if [ $BOOT1_OFFSET != 0x$BOOT1_KNOWN_OFFSET ]; then
        echo "Bad offset" >&2
        exit 1
fi

inside the loop.

emaste added inline comments.May 30 2017, 11:13 PM
sys/boot/efi/boot1/generate-fat.sh
71 ↗(On Diff #28852)

That is, this change is a pretty substantial change to the template generation script, but only a trivial change to the Makefile and Makefile.fat, and there's no need to regenerate the .uu files.

Removing the .uu files and replacing them with .bz2 is a much larger change to the templates (and only a minor change to the generate-fat script and Makefile), and can be combined with a size bump so there's only one instance of churn.

emaste added inline comments.May 30 2017, 11:14 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

Quick update: @smahadevan_freebsdfoundation.org is now at the "it creates a FAT filesystem image and the contents of the desired files are found within that image, but there are no directory entries" :)

In other words it's getting close. We'll still have the bootstrapping issues to deal with, however.

emaste added a subscriber: eric_metricspace.net.

@emaste: Do you want to subsume D9680 into this?

Dropping the uuencode, and increasing the template file size to 512kb?

ngie added inline comments.Jul 2 2017, 9:07 AM
sys/boot/efi/boot1/generate-fat.sh
18 ↗(On Diff #28852)

-ne 0 is a better idiom.

19 ↗(On Diff #28852)

I generally use ${0##*/} instead of $0 to strip the dirname.

37–40 ↗(On Diff #28852)

The cleanup for mounting/unmounting/removing the directory should really be done unconditionally in a trap'ed function, e.g.,

cleanup_stub()
{
    umount stub
    mdconfig -d -u $DEVICE
    rmdir stub
}
trap cleanup_stub EXIT INT TERM
63 ↗(On Diff #28852)

Hardcoding 512 for the byte sector size isn't a good practice.

emaste added inline comments.Sat, Sep 9, 7:12 PM
sys/boot/efi/boot1/generate-fat.sh
18 ↗(On Diff #28852)

I'm not sure I agree - if id -u did happen to produce some non-numeric output we'd get

[: foo: bad number

and the script continues on.

63 ↗(On Diff #28852)

Not introduced in this change; could be addressed in a subsequent change.

emaste updated this revision to Diff 32855.Sat, Sep 9, 7:15 PM

Update usage message as suggested by @ngie

emaste added inline comments.Sat, Sep 9, 7:18 PM
sys/boot/efi/boot1/Makefile.fat
4–7 ↗(On Diff #28852)

It's not the same on all systems: the filename is different.

emaste updated this revision to Diff 32856.Sat, Sep 9, 7:31 PM

disallow arch-specific differences in fs images by enforcing known 0x2d offset

allanjude accepted this revision.Sat, Sep 9, 8:49 PM

In the future we should do away with the uuencode, and maybe switch to xz (doesn't save any space over bzip, but is one of the only remaining uses of bzip in buildworld)

This revision is now accepted and ready to land.Sat, Sep 9, 8:49 PM
emaste added a comment.Sat, Sep 9, 8:52 PM

maybe switch to xz (doesn't save any space over bzip, but is one of the only remaining uses of bzip in buildworld)

Ah, good point. I planned to do the size change + switch away from uuencoding in one step (to reduce repo churn), we could switch to xz as well.

imp added a comment.Sat, Sep 9, 9:35 PM

Once again I ask: where are we with makefs and FAT filesystems? :) Then we could kill these dreadful templates once and for all.

imp added a comment.Sat, Sep 9, 9:36 PM

Note: my question isn't a 'no' here unless the answer to the question is 'it totally works, what rock have you been under to not know that." :)

In D10931#255107, @imp wrote:

Once again I ask: where are we with makefs and FAT filesystems? :)

Probably a couple of months, certainly in by 12. I don't want to hold this up on that though.

Also note that bsdinstall could invoke makefs_msdos, there's no real need for the templates for the installer. @allanjude is planning to look at that.

If I'm not mistaken, this should work as a precursor to my GELI patch series. I will apply this, then attempt a build with boot1_refactor also applied. That should tell us whether it does the job.

I'm pretty sure the 128k I highlighted needs to be increased to 512k.

sys/boot/efi/boot1/generate-fat.sh
17 ↗(On Diff #32856)

Needs to be increased to 512k

emaste added inline comments.Sun, Sep 10, 2:20 AM
sys/boot/efi/boot1/generate-fat.sh
17 ↗(On Diff #32856)

Yes - I plan to commit this change first, followed by a subsequent one to increase the size to 512k, switch from bzip2 to xz, and stop uuencoding the files (so that the template files only change once to avoid unnecessary repo churn).

eric_metricspace.net added inline comments.
sys/boot/efi/boot1/generate-fat.sh
17 ↗(On Diff #32856)

OK, then that's ok

imp accepted this revision.Sun, Sep 10, 5:34 AM

This looks good, so far as it goes... But maybe effort would be better fixing bsdinstall to not suck so bad as to need this as one file it DDs into a partition.

See https://reviews.freebsd.org/D12302 for a first cut at fixing it

sys/boot/efi/boot1/generate-fat.sh
17 ↗(On Diff #32856)

Why not 50MB? 512k is way way way too small and the longer we keep doing that, the longer we'll keep setting ourselves up for more pain.

45 ↗(On Diff #32856)

Doesn't this need to be /dev/$DEVICE ? Well, OK, /dev/ is added by default, but it's better to be explicit.

63 ↗(On Diff #28852)

IMHO, no sense polishing this turd... better to spend time on makefs + FAT or better yet: bsdinstall doing the newfs_msdos and copy. What we're doing now is nuts and the sooner it dies the better.

This revision was automatically updated to reflect the committed changes.

I'm unsure as to what needs to happen now. Do I need to do anything to my patches yet?

I'm unsure as to what needs to happen now. Do I need to do anything to my patches yet?

We have D9860 open still which needs one more round of questions answered and updates, and then I'll commit that. After that I think it's all your patches.

imp added a comment.Wed, Sep 13, 12:16 AM

I'm unsure as to what needs to happen now. Do I need to do anything to my patches yet?

We have D9860 open still which needs one more round of questions answered and updates, and then I'll commit that. After that I think it's all your patches.

That number doesn't sound right... It's showing closed something maybe related to network code.

imp added a comment.Wed, Sep 13, 12:16 AM

I'm unsure as to what needs to happen now. Do I need to do anything to my patches yet?

We have D9860 open still which needs one more round of questions answered and updates, and then I'll commit that. After that I think it's all your patches.

That number doesn't sound right... It's showing closed something maybe related to network code.

Indeed, make that D9680.