Page MenuHomeFreeBSD

bhyve: Support a _VARS.fd file for bootrom
ClosedPublic

Authored by bcran on Apr 19 2019, 9:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 8:45 PM
Unknown Object (File)
Feb 24 2024, 5:23 AM
Unknown Object (File)
Feb 5 2024, 10:12 AM
Unknown Object (File)
Jan 30 2024, 6:55 AM
Unknown Object (File)
Jan 30 2024, 6:55 AM
Unknown Object (File)
Jan 30 2024, 6:55 AM
Unknown Object (File)
Jan 30 2024, 6:55 AM
Unknown Object (File)
Jan 30 2024, 6:55 AM

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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?

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

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.

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
101

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.

usr.sbin/bhyve/bootrom.c
101

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.
usr.sbin/bhyve/bootrom.c
102

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.

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

bcran edited the summary of this revision. (Show Details)

Update code to work against FreeBSD-CURRENT.

This revision now requires review to proceed.Nov 28 2021, 4:36 PM

Add back checks for var_size and total_size.

0mp requested changes to this revision.Nov 28 2021, 5:57 PM
0mp added a subscriber: 0mp.
0mp added inline comments.
usr.sbin/bhyve/bhyve.8
326
629
This revision now requires changes to proceed.Nov 28 2021, 5:57 PM

Fix romfile[,varfile] in man page.

Just one more issue with the manual page.

usr.sbin/bhyve/bhyve.8
629

It would be nice to stylize /usr with Pa.

This revision is now accepted and ready to land.Nov 28 2021, 7:49 PM

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

usr.sbin/bhyve/bhyve.8
333

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

bcran added inline comments.
usr.sbin/bhyve/bhyve.8
333

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

bcran marked an inline comment as done.

man page updates.

This revision now requires review to proceed.Nov 28 2021, 11:00 PM
corvink added inline comments.
usr.sbin/bhyve/bootrom.c
192

gpa_alloctop should be decreased.

usr.sbin/bhyve/bootrom.c
110

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

140

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

141

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

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
192

what's the thinking there?

usr.sbin/bhyve/bootrom.c
192

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 marked 6 inline comments as done.

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

usr.sbin/bhyve/bootrom.c
141

Someone just deleted the file?

usr.sbin/bhyve/bootrom.c
106–112

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, ",");
110

romfile_dup is unused.

192–193

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

bcran marked 2 inline comments as done.Dec 5 2021, 3:09 AM
bcran added inline comments.
usr.sbin/bhyve/bootrom.c
106–112

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 marked an inline comment as done.

Remove romfile_dup. Fix strdup/strsep.

usr.sbin/bhyve/bootrom.c
106–112

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.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 12 2021, 3:08 PM
This revision was automatically updated to reflect the committed changes.