Page MenuHomeFreeBSD

bhyve: Support a _VARS.fd file for bootrom

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



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 <>
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

rS FreeBSD src repository
Lint Skipped
Unit Tests Skipped

Event Timeline

Note that the UEFI firmware used with this needs to have the flash variable driver added in, as in:

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


You need to break the line at the sentence stop.

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

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 added inline comments.

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...."


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.


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


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


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.


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


Opened varfd, but check status of fd?


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.


"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 added inline comments.

Used .Nm and split up the sentence.


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.


This will vary depending on which commands are sent.


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.


definitely wrong, thanks for catching that


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

LGTM! Thank you!

I have merged your PR:
I'm gonna commit the -devel port in few minutes. 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? 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 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.


@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.

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


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.

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.


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 added inline comments.

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

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

Has this been committed yet?

Now patch fail
CURRENT 359419
1 out of 3 hunks failed--saving rejects to usr.sbin/bhyve/bhyve.8.rej - new date
2 out of 4 hunks failed--saving rejects to usr.sbin/bhyve/bootrom.c.rej

@scottph Are you still interested in working on this?

In D19976#515732, @mat wrote:

Has this been committed yet?

No, I'm fairly sure it hasn't been committed.

bcran edited reviewers, added: scottph; removed: bcran.

@scottph has indicated he no longer has the time/interest in working on this, so I'll take over and work to get it committed.