Page MenuHomeFreeBSD

bhyve: Support a _VARS.fd file for bootrom
AcceptedPublic

Authored by scottph on Apr 19 2019, 9:08 PM.

Details

Summary

OVMF creates two separate .fd files, a _CODE.fd file containing
the UEFI code, and a _VARS.fd file containing a template of an
empty UEFI variable store.

OVMF decides to write variables to the memory range just below the
boot rom code if it detects a CFI flash device. So here we add
just the barest facsimile of CFI command handling to bootrom.c
that is needed to placate OVMF.

Submitted by: D Scott Phillips <d.scott.phillips@intel.com>
Sponsored by: Intel Corporation
MFC After: 1 week

Test Plan

Using the _CODE.fd and _VARS.fd files from the devel UEFI firmware build, start a VM.
At the EFI Shell:

setvar Hi -guid 135fe7b2-8573-453a-b747-69f04add0bd7 -bs -rt -nv ="There"
dmpstore -guid 135fe7b2-8573-453a-b747-69f04add0bd7 Hi

Destroy the VM (so variables cannot be persisted through memory). Again at the EFI Shell:

dmpstore -guid 135fe7b2-8573-453a-b747-69f04add0bd7 Hi

Get back the proper response of "There"

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

scottph created this revision.Apr 19 2019, 9:08 PM

Note that the UEFI firmware used with this needs to have the flash variable driver added in, as in: https://github.com/freebsd/uefi-edk2/pull/8

bcr added a subscriber: bcr.Apr 19 2019, 9:16 PM

Can you run textproc/igor and "mandoc -Tlint" over the man page and see what warnings/errors they give you? Thanks!

usr.sbin/bhyve/bhyve.8
626

You need to break the line at the sentence stop.

scottph updated this revision to Diff 56409.Apr 19 2019, 9:43 PM

bhyve.8, update .Dd, start sentences on a new line.

scottph marked an inline comment as done.Apr 19 2019, 9:44 PM
bcr accepted this revision.Apr 19 2019, 9:51 PM

Thank you for the quick edits. Approved by manpages for that part.

This revision is now accepted and ready to land.Apr 19 2019, 9:51 PM
rgrimes requested changes to this revision.Apr 20 2019, 12:45 AM
rgrimes added inline comments.
usr.sbin/bhyve/bhyve.8
626

Should bhyve be capitalized here, or is .Nm call more proper. Also I would break the still pretty long for mandoc line at the "so make sure...."

usr.sbin/bhyve/bootrom.c
62

The next two are initialized in the code, which is the prefered form rather than doing it in the declares, for consistancy could you move this initialization to be with the others below.

69

We know cmd == CFI_BCS_READ_ARRAY when we reach this line, why do we switch on it?

88

The logic in this function seems to be confusing. dir seems to be the controlling parameter, when I would expect cmd to be, default: cases are used to override the cmd in one place, and to assume a read operation for all others? This seems that errors are not actually checked for and returned, ie the function always returns 0

103

This function is long enough it should have a block comment that describes what it does, what the inputs are and what return values/error conditions exist.

112

style(9) ordering, char/uchar should be after int.

134

Opened varfd, but check status of fd?

166

It may be more useful to print 2 or more different errors here, this single error covers a lot of conditions that can cause it.

186

"Bootrom or variable file is not a multiple of the page size." is a more correct error. I think I would split this out and leave the old code untouched as possible and add a new error for var_size & PAGE_MASK

This revision now requires changes to proceed.Apr 20 2019, 12:45 AM
scottph updated this revision to Diff 56516.Apr 22 2019, 10:32 PM
scottph marked 8 inline comments as done.Apr 22 2019, 10:43 PM
scottph added inline comments.
usr.sbin/bhyve/bhyve.8
626

Used .Nm and split up the sentence.

usr.sbin/bhyve/bootrom.c
62

This was a static function-scoped variable in the last iteration. I've changed it to be part of the state struct now with the other state, because adding bounds checking caused the function to outgrow the two general purpose arguments that a handler function gets.

69

This will vary depending on which commands are sent.

88

I looked at the restructuring you suggested but I think this is the more clear way to write the handling logic. As a first decision, (1) are we being given a new command, or are we servicing a read request under the existing command.

Regarding error checking, addr is guaranteed to be in bounds by the logic in mem.c, but I think it is possible for size to go past the end of our range, so I've added a check for that.

112
134

definitely wrong, thanks for catching that

186

I've split up the checks here so that there are individual checks and error messages for each case.

araujo accepted this revision.Apr 24 2019, 2:40 PM

LGTM! Thank you!

I have merged your PR: https://github.com/freebsd/uefi-edk2/pull/8
I'm gonna commit the -devel port in few minutes.

@scott.d.phillips_intel.com I'm wondering if sysutils/uefi-edk2-bhyve will work out of the box with these new options, or do we need to update this port as well?

scottph marked 6 inline comments as done.Apr 24 2019, 5:21 PM

@scott.d.phillips_intel.com I'm wondering if sysutils/uefi-edk2-bhyve will work out of the box with these new options, or do we need to update this port as well?

A similar change to https://github.com/freebsd/uefi-edk2/pull/8 would be needed on the UDK2014.SP1 branch to add the driver that actually makes use of the writable bootrom area. I think that should be fairly easy, the only sticking point is that using the combined BHYVE_UEFI.fd file (which is the same as cat BHYVE_UEFI_VARS.fd BHYVE_UEFI_CODE.fd) as romfile would cause vmm.ko to die. Probably the best choice would be to update the old package to put BHYVE_UEFI_CODE.fd into the BHYVE_UEFI.fd path.

scottl added a subscriber: scottl.Jun 12 2019, 8:02 PM

ping!!!

@scottph is on sabbatical over the summer. I'm not sure when he's back.

He's out for a few more weeks. It's on my radar, but I've been busy with other things.

@rgrimes I believe I've addressed your comments from earlier. Would you mind taking a look at the new patch? Thanks.

rgrimes accepted this revision as: rgrimes.Fri, Aug 16, 6:38 PM

The additional note on style(9) on variable order declaration is informative only, you can choose to fix at your option.

usr.sbin/bhyve/bootrom.c
112

At line 660 of style: When declaring variables in functions declare them sorted by size,
then in alphabetical order; multiple ones per line are okay.

scottph marked an inline comment as done.Fri, Aug 16, 6:52 PM

The additional note on style(9) on variable order declaration is informative only, you can choose to fix at your option.

Cool, thanks for taking a look again so quickly.

usr.sbin/bhyve/bootrom.c
112

ah, I believe you may have missed that those are char pointers in your first reading, where sizeof(char *) == 8, sizeof(int) == 4. cf line 483 of style(9).

rgrimes accepted this revision.Fri, Aug 16, 6:55 PM
rgrimes added inline comments.
usr.sbin/bhyve/bootrom.c
112

Ineeded I did, so this is fine, sorry for the noise.

This revision is now accepted and ready to land.Fri, Aug 16, 6:55 PM