bhyve: add varfile option to nvlist of lpc device

[corvink]

Summary

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

[jhb]

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

454

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

usr.sbin/bhyve/bootrom.c

206–220

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

[corvink]

• 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

[corvink]

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

454

Thanks. I wasn't aware of this rule.

usr.sbin/bhyve/bootrom.c

206–220

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.

[corvink]

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

[jhb]

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

usr.sbin/bhyve/bootrom.c

206–220

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

212

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

[corvink]

• do not strdup varfile
• remove const from romfile

[andy_omniosce.org]

usr.sbin/bhyve/bootrom.c

213

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

[corvink]

• check if lpc.bootrom is NULL before strdup it

[corvink]

usr.sbin/bhyve/bootrom.c

213

Thanks for catching that.

[jhb]

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.

[markj]

usr.sbin/bhyve/bootrom.c

314

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

[rew]

usr.sbin/bhyve/pci_lpc.c

109

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]

• rebase on main
• check for romfile/varfile == NULL

[corvink]

usr.sbin/bhyve/bootrom.c

314

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.

[rew]

usr.sbin/bhyve/pci_lpc.c

217–229

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.

[corvink]

• check if lpc.bootrom exists before calling bootrom_loadrom

[corvink]

• do not leak varfd in bootrom_loadrom

[corvink]

usr.sbin/bhyve/bootrom.c

314

Man page of mmap states

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

[rew]

usr.sbin/bhyve/pci_lpc.c

224–226

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.

[jhb]

usr.sbin/bhyve/pci_lpc.c

224–226

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

[rew]

looks like this is good to go?

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