Page MenuHomeFreeBSD

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

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

Details

Summary

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

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
pci.0.7.0.device=nvme
pci.0.7.0.ram=16384

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

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
pci.0.7.0.device=nvme
pci.0.7.0.direct=true
pci.0.7.0.ro=true
pci.0.7.0.sectorsize=512/512
pci.0.7.0.ram=1234

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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: andy_omniosce.org; removed: chuck.

New version with change to strndup

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

usr.sbin/bhyve/pci_nvme.c
2806

These could be scoped to the if block.

2817

No check for NULL from strndup()

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