Page MenuHomeFreeBSD

loader: add HTTP support using UEFI
ClosedPublic

Authored by bcran on Jun 14 2019, 8:55 PM.

Details

Summary

Add support for an HTTP "network filesystem" using the UEFI's HTTP
stack.

This also supports HTTPS, but TianoCore EDK2 implementations currently crash while fetching loader files.
Only IPv4 is supported at the moment. IPv6 support is planned for a follow-up changeset.

Note that we include some headers from the TianoCore EDK II project in stand/efi/include/Protocol verbatim, including links to the license instead of including the full text because that's their preferred way of communicating it, despite not being normal FreeBSD project practice.

Test Plan
  • Do UEFI PXE boot, see that it doesn't break - DONE.
  • Do UEFI HTTP boot, see that loader.conf &c can be fetched over http - DONE.
  • Do UEFI boot on system that doesn't support HTTP booting, ensure it doesn't hang trying to open the http protocol - DONE
  • Send out a Call For Testing to freebsd-current ML - DONE.

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

scottph created this revision.Jun 14 2019, 8:55 PM
bcran edited the summary of this revision. (Show Details)Jun 14 2019, 9:10 PM
bcran added reviewers: imp, tsoome, kevans, emaste, scottph.
bcran edited the test plan for this revision. (Show Details)Jun 14 2019, 9:14 PM
bcran edited the test plan for this revision. (Show Details)Jun 15 2019, 3:21 AM
lwhsu added a subscriber: lwhsu.Jun 16 2019, 2:03 PM
bcran edited the test plan for this revision. (Show Details)Jun 19 2019, 6:48 PM

I thought I had a computer capable of doing uefi http but I was wrong. UEFI PXE works fine though.

bcran added a comment.Jun 22 2019, 6:49 PM

I realized one issue: we should perhaps add a configuration option such as LOADER_EFIHTTP_SUPPORT to let people disable this.

imp added a comment.Jun 23 2019, 7:55 PM

This looks pretty close, but there are still a couple of issues that need to be resolved before the commit.

stand/efi/include/Protocol/Http.h
13 ↗(On Diff #58638)

I'd note in your commit message that several files were pulled in verbatim from EDK2 and they contain pointers to a URL for the license. And that we're committing them verbatim because it's the vendor's preferred method of communicating the license, even though it's a bit at odds with normal project practice.

stand/efi/include/efidevp.h
299 ↗(On Diff #58638)

I'd use C style comments here, and below. This file is a FreeBSD file.

stand/efi/include/efilib.h
47 ↗(On Diff #58638)

this seems misplaced.... why is it here? Others are in stand.h

stand/efi/libefi/efihttp.c
279 ↗(On Diff #58638)

checked too late

339 ↗(On Diff #58638)

error handling above seems thin.

443 ↗(On Diff #58638)

host not checked.
Also, consider using strdup instead.

447 ↗(On Diff #58638)

assumes c is non-null.

481 ↗(On Diff #58638)

Is there some way to time-bound this wait? And the one below?

500–501 ↗(On Diff #58638)

wouldn't it be safer to set this BEFORE you call CreateEvent? This pattern is used several times and I'll just comment once.

stand/efi/loader/conf.c
58 ↗(On Diff #58638)

All these other fsops are defined in stand.h

bcran added inline comments.Jun 23 2019, 10:56 PM
stand/efi/loader/conf.c
58 ↗(On Diff #58638)

stand.h looks to be the right place for extern struct fs_ops efihttp_fsops, but this seems to be the right place for the &efihttp_fsops?

bcran added inline comments.Jun 23 2019, 11:04 PM
stand/efi/libefi/efihttp.c
481 ↗(On Diff #58638)

Yes, I'll add a call to BS->Stall() and wait 10 seconds.

500–501 ↗(On Diff #58638)

The done? Or calling __compiler_membar()?
I'll move the done up before the CreateEvent.

imp added inline comments.Jun 24 2019, 1:29 AM
stand/efi/libefi/efihttp.c
481 ↗(On Diff #58638)

I'd cap the the upper bound to be some variable that can be set.

500–501 ↗(On Diff #58638)

shouldn't we set done to false, forcing it to be false before we create a task/thread that might change it?

stand/efi/loader/conf.c
58 ↗(On Diff #58638)

Correct on both accounts.

bcran added inline comments.Jun 24 2019, 2:14 AM
stand/efi/libefi/efihttp.c
481 ↗(On Diff #58638)

Given that the other network code doesn't do that, and was instead taking over an hour to timeout, I think simply capping it at 10 seconds is sufficient for now.

500–501 ↗(On Diff #58638)

Yes, I agree - I'll fix it.

imp added inline comments.Jun 24 2019, 2:50 AM
stand/efi/libefi/efihttp.c
481 ↗(On Diff #58638)

Some reasonable upper limit is needed for how long the network waits. Where else do we wait an unbounded amount of time?

bcran added inline comments.Jun 24 2019, 2:53 AM
stand/efi/libefi/efihttp.c
481 ↗(On Diff #58638)

We used to take a really long time for send/recv in stand/libsa/net.c to timeout. From https://svnweb.freebsd.org/base/head/stand/libsa/net.c?revision=342282&view=markup :P

Wait a maximum of 300 seconds for network send/recv in libsa
The reason for this change is that currently, a send/recv takes many hours to time out.
This is suboptimal in the bootloader because it means for example that NFS will take hours to fail before allowing subsequent access methods such as gzip to be tried.
Setting MAXWAIT to 300 seconds (5 minutes) still allows slow connections of 1Mb to be used to download a 30MB kernel file.

bcran removed a reviewer: bcran.Jun 24 2019, 3:41 AM
bcran commandeered this revision.
bcran edited the summary of this revision. (Show Details)Jun 24 2019, 3:42 AM
bcran updated this revision to Diff 58937.
  • loader: add HTTP support using UEFI
  • Updates per review feedback.
bcran marked 12 inline comments as done.Jun 24 2019, 3:44 AM
bcran marked 2 inline comments as done.Jun 24 2019, 7:03 PM
bcran updated this revision to Diff 58960.Jun 24 2019, 7:08 PM
  • Fixed a couple more issues
bcran edited the summary of this revision. (Show Details)Jun 24 2019, 7:11 PM
bcran edited the summary of this revision. (Show Details)
imp added a comment.Jun 24 2019, 7:20 PM

Looks a lot better. Found one minor thing...

stand/efi/libefi/efihttp.c
736 ↗(On Diff #58960)

no error handling here...

bcran updated this revision to Diff 58961.Jun 24 2019, 7:26 PM
  • Added error handling around realloc.
imp accepted this revision.Jun 24 2019, 7:45 PM

I'm tapped out. Can't find anything else.

This revision is now accepted and ready to land.Jun 24 2019, 7:45 PM
This revision was automatically updated to reflect the committed changes.