Page MenuHomeFreeBSD

bhyve nvme ramdisk backend cli option is broken after configuration management refactor.

Authored by chuck on Jun 10 2021, 1:23 PM.



The bhyve NVMe backend should support a device path of 'ram=<size_in_MiB>'
and this no longer works following the recent configuration management

Test Plan

I've tested that bhyve now correctly sets the ram configuration variable
when parsing legacy options:

# usr.sbin/bhyve/bhyve -o config.dump=1 -s 7,nvme,ram=16384 xx

# usr.sbin/bhyve/bhyve -o config.dump=1 -s 7,nvme,/dev/gpt/rootfs xx

and with some other options, for completeness:

# usr.sbin/bhyve/bhyve -o config.dump=1 -s 7,nvme,ram=1234,direct,ro,sectorsize=512/512 xx

Diff Detail

rG FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thank you for finding this bug!

What would you think of the following? It should work the same but avoids duplicating some of the logic found elsewhere:

diff --git a/usr.sbin/bhyve/pci_nvme.c b/usr.sbin/bhyve/pci_nvme.c
index 0abc0415a1d..d31a977c7bf 100644
--- a/usr.sbin/bhyve/pci_nvme.c
+++ b/usr.sbin/bhyve/pci_nvme.c
@@ -2800,11 +2800,30 @@ pci_nvme_init(struct vmctx *ctx, struct pci_devinst *pi, nvlist_t *nvl)        return (error);

+static int
+pci_nvme_legacy_config(nvlist_t *nvl, const char *opts)
+       char *cp;
+       if (opts == NULL)
+               return (0);
+       if (strncmp(opts, "ram=", 4) == 0) {
+               cp = strchr(opts, ',');
+               if (cp) {
+                       *cp = '\0';
+                       cp++;
+               }
+               set_config_value_node(nvl, "ram", opts + 4);
+               return (pci_parse_legacy_config(nvl, cp));
+       } else
+               return (blockif_legacy_config(nvl, opts));

 struct pci_devemu pci_de_nvme = {
        .pe_emu =       "nvme",
        .pe_init =      pci_nvme_init,
-       .pe_legacy_config = blockif_legacy_config,
+       .pe_legacy_config = pci_nvme_legacy_config,
        .pe_barwrite =  pci_nvme_write,
        .pe_barread =   pci_nvme_read

I think I like Chuck's shorter version a bit better, but Chuck's version needs to use strndup() instead of modifying the 'cosnt char *' input. (strchr's weird signature breaks the compiler noticing since it accepts a const char and silently "casts" away the const via the char * return type). Sorry for the regression though. The commit log should use a 'Fixes:' tag that references the original commit.

In D30717#690975, @jhb wrote:

I think I like Chuck's shorter version a bit better

Fine with me (modulo the *cp issue). Should I close this and let Chuck fix it?

In D30717#690975, @jhb wrote:

I think I like Chuck's shorter version a bit better

Fine with me (modulo the *cp issue). Should I close this and let Chuck fix it?

Perhaps I'll commandeer this review to keep all the history in one place unless there are objections

Commandeering seems sensible to me.

Fine with me, thanks for doing this.

chuck edited reviewers, added:; removed: chuck.

New version with change to strndup

I am used to contributing to illumos so please feel free to ignore these comments.


These could be scoped to the if block.


No check for NULL from strndup()

This revision is now accepted and ready to land.Jun 15 2021, 4:35 PM
This revision was automatically updated to reflect the committed changes.