bhyve: Support a _VARS.fd file for bootrom

[bcran]

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 <[e-mail]>
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"

[araujo]

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]

In D19976#430794, @araujo wrote:

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

[araujo]

ping!!!

[bcran]

In D19976#456807, @araujo wrote:

ping!!!

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

[scottl]

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

[scottph]

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

[rgrimes]

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

200

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]

In D19976#463043, @rgrimes wrote:

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

200

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]

usr.sbin/bhyve/bootrom.c

201

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

[mat]

Has this been committed yet?

[f0andrey_gmail.com]

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

[bcran]

@scottph Are you still interested in working on this?

[bcran]

In D19976#515732, @mat wrote:

Has this been committed yet?

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

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

[bcran]

Now the firmware work has been committed, I'll switch back to working on this.

[bcran]

Update code to work against FreeBSD-CURRENT.

[bcran]

Fix tabs vs spaces.

[bcran]

Add back checks for var_size and total_size.

[0mp]

usr.sbin/bhyve/bhyve.8

503

929

[bcran]

Fix romfile[,varfile] in man page.

[0mp]

Just one more issue with the manual page.

usr.sbin/bhyve/bhyve.8

929

It would be nice to stylize /usr with Pa.

[pauamma_gundo.com]

Only addressing the manual page change, the rest is beyond me.

usr.sbin/bhyve/bhyve.8

510

Nitpick: "persisted" as a passive may confuse EFL users. Perhaps "written back" or "saved" instead?

[bcran]

usr.sbin/bhyve/bhyve.8

510

Good point! I've changed it to "saved".

[bcran]

man page updates.

[pauamma_gundo.com]

Manual page change LGTM now.

[corvink]

usr.sbin/bhyve/bootrom.c

284

gpa_alloctop should be decreased.

[imp]

usr.sbin/bhyve/bootrom.c

209

You can put all 3 assignments on one line. It might help with what feels a bit repetitive about this clause.

238

Since the 'else' clause has {} around it, I'd be tempted to put {} here for consistency, but it's up to you (it's allowed, but not required by style(9)).

239

i wonder how this can happen since we just opened the file....

[imp]

I didn't review the man page, but it looks like it's received a lot of attention.
In general, this code looks good to my eye and my questions / suggestions are minor.

usr.sbin/bhyve/bootrom.c

284

what's the thinking there?

[corvink]

usr.sbin/bhyve/bootrom.c

284

bootrom_alloc is used to allocate space in the bootrom segment. It uses gpa_alloctop and gpa_allocbottom for allocation. Since varfile is allocated in the bootrom segment, it should keep those values consistent.

Maybe it would be even better to reuse bootrom_alloc.

[bcran]

Put allocations on one line.
Added {}.
Removed gpa variable, and used gpa_alloctop instead.

[bcran]

usr.sbin/bhyve/bootrom.c

239

Someone just deleted the file?

[corvink]

usr.sbin/bhyve/bootrom.c

205–211

I'd prefer to move parsing to pci_lpc where romfile is parsed.

However, if you prefer to parse it at this place:
strsep should set varfile to NULL if it doesn't exists.

Another possible solution:

varfile = strdup(romfile);
romfile = strsep(&varfile, ",");

209

romfile_dup is unused.

284–285

Would be a good idea to check if there's enough space left.

[bcran]

usr.sbin/bhyve/bootrom.c

205–211

Thanks, fixed.
I'd prefer not to move the parsing at this time, since I didn't write this code and it's been in review for such a long time. Would you mind if we postponed that change?

[bcran]

Remove romfile_dup. Fix strdup/strsep.

[corvink]

usr.sbin/bhyve/bootrom.c

205–211

From my side, it's ok if parsing isn't moved.

Btw: You should be able to remove the if statement because varfile should be set to NULL by strsep if "," isn't found.