Page MenuHomeFreeBSD

bhyve: add varfile option to nvlist of lpc device
ClosedPublic

Authored by corvink on Dec 14 2021, 11:17 AM.
Tags
Referenced Files
Unknown Object (File)
Thu, Nov 28, 10:44 PM
Unknown Object (File)
Tue, Nov 26, 2:16 AM
Unknown Object (File)
Fri, Nov 22, 7:25 AM
Unknown Object (File)
Tue, Nov 19, 1:30 AM
Unknown Object (File)
Nov 5 2024, 6:23 AM
Unknown Object (File)
Oct 30 2024, 9:45 PM
Unknown Object (File)
Oct 25 2024, 2:58 PM
Unknown Object (File)
Oct 25 2024, 1:57 PM

Details

Summary

Create two seperate nvlist entries for romfile and varfile instead of
using one for both.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

I would perhaps avoid converting "lpc.bootrom" to a new node and instead leave it as-is for the path to the boot ROM and just add a new "lpc.bootvars" or "lpc.nvram" or some such that holds the path to the vars file.

usr.sbin/bhyve/bhyve_config.5
448

FYI: In manpages we start sentences on a new line.

usr.sbin/bhyve/bootrom.c
205–216

So this won't work as well as you might hope as if a variable has any expansions, then it uses a thread-local buffer (so you can really only query one variable at a time). Perhaps structure this as:

romfile = get_config_value_node(nvl, "romfile");
if (romfile == NULL)
    return (-1);

fd = open(romfile, O_RDONLY);
...

varfile = get_config_value_node(nvl, "varfile");
if (varfile != NULL) {
    ...
  • start every sentence on a new line in manpages
  • do not convert "lpc.bootrom" into a node
  • only query one nvlist variable at the same time
In D33433#756746, @jhb wrote:

I would perhaps avoid converting "lpc.bootrom" to a new node and instead leave it as-is for the path to the boot ROM and just add a new "lpc.bootvars" or "lpc.nvram" or some such that holds the path to the vars file.

I wanna add support for Qemu's FwCfg to bhyve (see D31578). Therefore, I'd like to add a flag which decides whether to use Bhyve's FwCtl or Qemu's FwCfg. I got some feedback that this flag should be added to the -l bootrom option. My idea was that I convert lpc.bootrom into a node and add this flag to this new node as lpc.bootrom.fwcfg.

usr.sbin/bhyve/bhyve_config.5
448

Thanks. I wasn't aware of this rule.

usr.sbin/bhyve/bootrom.c
205–216

romfile is used by some debug logs after varfile is opened. Additionally, if someone adds new changes, this behaviour might be overlooked. For that reasons, I prefer to strdup the variables. If you don't like it, I will revert it and use your solution.

@jhb @markj Any suggestions on which config nodes to use for romfile and varsfile? D31578 depends on that decision.

I think for your other review, using 'lpc.fwcfg' for the node name makes sense.

usr.sbin/bhyve/bootrom.c
205–216

strdup() of romfile makes sense if it used multiple times. If you are only going to use varfile once, I'd rather just get it the one place it is used. That is the style more commonly used in other places in bhyve (fetching config items as-needed vs fetching them all at once up front).

211

I would drop the const to avoid the cast (which really needs __DECONST()) for free.

  • do not strdup varfile
  • remove const from romfile
andy_omniosce.org added inline comments.
usr.sbin/bhyve/bootrom.c
212

get_config_value_node() can return NULL, particularly since you're now calling bootrom_load() even if lpc.bootrom is not present. How about something like this?

char *romfile = get_config_value_node(nvl, "bootrom");
if (romfile != NULL)
    romfile = strdup(romfile);
if (romfile == NULL)
    return (-1);
  • check if lpc.bootrom is NULL before strdup it
corvink added inline comments.
usr.sbin/bhyve/bootrom.c
212

Thanks for catching that.

jhb added inline comments.
usr.sbin/bhyve/bootrom.c
194

A general note that '*const' seems a bit odd style-wise (vs * const). But it's also true that nowhere else in bhyve (and rarely in FreeBSD) uses the const on the pointer (vs the const on what the pointer points to). I would probably leave it out to be consistent with the rest of usr.sbin/bhyve.

This revision is now accepted and ready to land.Jan 18 2022, 7:01 PM
markj added inline comments.
usr.sbin/bhyve/bootrom.c
309

Isn't varfd being leaked here? Not a new bug in any case.

rew added inline comments.
usr.sbin/bhyve/pci_lpc.c
107

I don't think libnv will like it when varfile is NULL, it looks like it will put the nvlist in an error state.

Maybe something like? Should lpc.bootrom get a check like this too?

corvink marked an inline comment as done.
  • rebase on main
  • check for romfile/varfile == NULL
This revision now requires review to proceed.Jan 27 2022, 10:10 AM
corvink added inline comments.
usr.sbin/bhyve/bootrom.c
309

I'm not sure. Bhyve has to keep the file open. Therefore, it uses var.mmap. I'm not sure if bhyve could close varfd while keeping var.mmap open.

usr.sbin/bhyve/pci_lpc.c
208–219

This doesn't look correct.

If a bootrom isn't being used, bootrom_loadrom() will exit with an error.

Either need to do something like this or have bootrom_loadrom() return success when a bootrom is not being used.

  • check if lpc.bootrom exists before calling bootrom_loadrom
  • do not leak varfd in bootrom_loadrom
corvink added inline comments.
usr.sbin/bhyve/bootrom.c
309

Man page of mmap states

The close(2) system call does not unmap pages, see munmap(2) for further information.

usr.sbin/bhyve/pci_lpc.c
214–216

why so much const? as opposed to const nvlist_t *nvl?

also curious, why not declare nvl at the beginning of the function? I see there are other occurrences like this within the patch..I'm only commenting on this one.

I don't mean to nitpick here.

usr.sbin/bhyve/pci_lpc.c
214–216

It is true that style(9) discourages C++ style variable declarations in the middle of scopes (though it has recently gained some exceptions for things like declaring a an integrator in a for statement).

looks like this is good to go?

I'll bring this in this week if no one beats me to it.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 3 2022, 7:56 AM
This revision was automatically updated to reflect the committed changes.