Page MenuHomeFreeBSD
Differential D19976

bhyve: Support a _VARS.fd file for bootrom
ClosedPublic
Actions

Authored by bcran on Apr 19 2019, 9:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Feb 21, 9:49 AM
Unknown Object (File)
Thu, Feb 19, 5:52 PM
Unknown Object (File)
Thu, Feb 19, 6:24 AM
Unknown Object (File)
Wed, Feb 18, 11:29 AM
Unknown Object (File)
Wed, Feb 18, 3:55 AM
Unknown Object (File)
Tue, Feb 17, 6:27 PM
Unknown Object (File)
Tue, Feb 17, 6:27 PM
Unknown Object (File)
Tue, Feb 17, 6:27 PM
View All Files
Subscribers
0mp
bcr
bz
corvink
dch
emaste
f0andrey_gmail.com
View All 14 Subscribers

Details

Reviewers
jhb
tychon
araujo
bcr
rgrimes
scottph
0mp
Group Reviewers
manpages
bhyve
Commits
rGd6c280fb36fe: bhyve: Support a _VARS.fd file for bootrom
rG866036f46c6e: bhyve: Support a _VARS.fd file for bootrom
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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 43016
Build 39904: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
araujo added a comment.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
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.

olevole_olevole.ru added a subscriber: olevole_olevole.ru.May 25 2019, 7:49 PM
mgrooms_shrew.net added a subscriber: mgrooms_shrew.net.Jun 3 2019, 8:14 PM
scottl added a subscriber: scottl.Jun 12 2019, 8:02 PM
araujo added a comment.Jul 24 2019, 8:18 AM

ping!!!

bcran added a comment.Jul 24 2019, 10:07 PM
In D19976#456807, @araujo wrote:

ping!!!

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

scottl added a comment.Jul 24 2019, 10:30 PM

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

scottph added a comment.Aug 16 2019, 5:09 PM

@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.Aug 16 2019, 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
201

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.Aug 16 2019, 6:52 PM
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
201

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.Aug 16 2019, 6:55 PM
rgrimes added inline comments.
usr.sbin/bhyve/bootrom.c
202

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
dch added a subscriber: dch.Nov 25 2019, 2:19 PM
mat added a subscriber: mat.Feb 4 2020, 2:06 PM

Has this been committed yet?

f0andrey_gmail.com added a subscriber: f0andrey_gmail.com.Mar 16 2020, 6:15 PM
f0andrey_gmail.com added a comment.Mar 29 2020, 10:55 PM

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 added a comment.Apr 5 2020, 6:54 PM

@scottph Are you still interested in working on this?

bcran added a comment.Apr 5 2020, 6:55 PM
In D19976#515732, @mat wrote:

Has this been committed yet?

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

bcran commandeered this revision.Apr 21 2020, 11:59 PM
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.

emaste added a subscriber: emaste.Apr 28 2020, 2:36 PM
bz added a subscriber: bz.Aug 26 2020, 4:07 PM
bcran marked an inline comment as done.Feb 18 2021, 3:46 AM

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

bcran marked 2 inline comments as done.Nov 28 2021, 4:32 PM
bcran updated this revision to Diff 99102.Nov 28 2021, 4:36 PM
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
Harbormaster completed remote builds in B43005: Diff 99102.Nov 28 2021, 4:36 PM
bcran updated this revision to Diff 99104.Nov 28 2021, 4:43 PM

Fix tabs vs spaces.

Harbormaster completed remote builds in B43006: Diff 99104.Nov 28 2021, 4:43 PM
bcran updated this revision to Diff 99105.Nov 28 2021, 5:08 PM

Add back checks for var_size and total_size.

Harbormaster completed remote builds in B43007: Diff 99105.Nov 28 2021, 5:08 PM
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
503
929
This revision now requires changes to proceed.Nov 28 2021, 5:57 PM
bcran updated this revision to Diff 99115.Nov 28 2021, 7:23 PM

Fix romfile[,varfile] in man page.

Harbormaster completed remote builds in B43014: Diff 99115.Nov 28 2021, 7:23 PM
bcran marked an inline comment as done.Nov 28 2021, 7:24 PM
0mp accepted this revision.Nov 28 2021, 7:49 PM

Just one more issue with the manual page.

usr.sbin/bhyve/bhyve.8
929

It would be nice to stylize /usr with Pa.

This revision is now accepted and ready to land.Nov 28 2021, 7:49 PM
pauamma_gundo.com added a subscriber: pauamma_gundo.com.Nov 28 2021, 7:54 PM

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 marked 3 inline comments as done.Nov 28 2021, 10:59 PM
bcran added inline comments.
usr.sbin/bhyve/bhyve.8
510

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

bcran updated this revision to Diff 99123.Nov 28 2021, 11:00 PM
bcran marked an inline comment as done.

man page updates.

This revision now requires review to proceed.Nov 28 2021, 11:00 PM
Harbormaster completed remote builds in B43016: Diff 99123.Nov 28 2021, 11:00 PM
pauamma_gundo.com added a comment.Nov 28 2021, 11:34 PM

Manual page change LGTM now.

corvink added a subscriber: corvink.Nov 29 2021, 9:29 AM
corvink added inline comments.
usr.sbin/bhyve/bootrom.c
287

gpa_alloctop should be decreased.

imp added inline comments.Nov 29 2021, 3:36 PM
usr.sbin/bhyve/bootrom.c
210

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

241

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

242

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

imp added a comment.Nov 29 2021, 3:38 PM

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
287

what's the thinking there?

corvink added inline comments.Nov 30 2021, 6:48 AM
usr.sbin/bhyve/bootrom.c
287

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 updated this revision to Diff 99275.Dec 1 2021, 3:19 AM
bcran marked 6 inline comments as done.

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

Harbormaster completed remote builds in B43075: Diff 99275.Dec 1 2021, 3:19 AM
bcran added inline comments.Dec 1 2021, 3:19 AM
usr.sbin/bhyve/bootrom.c
242

Someone just deleted the file?

corvink added inline comments.Dec 3 2021, 1:20 PM
usr.sbin/bhyve/bootrom.c
206–212

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

romfile_dup is unused.

287–288

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
206–212

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 updated this revision to Diff 99469.Dec 5 2021, 3:11 AM
bcran marked an inline comment as done.

Remove romfile_dup. Fix strdup/strsep.

Harbormaster completed remote builds in B43154: Diff 99469.Dec 5 2021, 3:11 AM
corvink added inline comments.Dec 6 2021, 7:42 AM
usr.sbin/bhyve/bootrom.c
206–212

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
Closed by commit rG866036f46c6e: bhyve: Support a _VARS.fd file for bootrom (authored by bcran). · Explain Why
This revision was automatically updated to reflect the committed changes.
bcran added a commit: rG866036f46c6e: bhyve: Support a _VARS.fd file for bootrom.
corvink added a commit: rGd6c280fb36fe: bhyve: Support a _VARS.fd file for bootrom.Dec 8 2022, 5:44 PM

Revision Contents
Changeset List

PathSize
usr.sbin/
bhyve/
23 lines
139 lines

Diff 99123

View Options

usr.sbin/bhyve/bhyve.8

Loading...
View Options

usr.sbin/bhyve/bootrom.c

Loading...