bhyve nvme controller will pass unknown config if using blockif backend

[wanpengqian_gmail.com]

Summary

Currently bhyve nvme controller will parse all configs in pci_nvme.c.
while options for blockif such as nocache/nodelete/sync/ro/sectorsize
are ignored and return errors.

This patch will enable nvme controller pass unknown config
to blockif_open if using blockif backend.

This patch also remove the sectsz config for nvme controller.
after apply this patch, blockif can properly set the sectorsize, and nvme controller
can get such value from blockif backend. no need to specify sectsz in nvme controller.
(ram backend is fixed to 4096, not affect.)

Test Plan

Pass some blockif acceptable configs when using nvme controller,
check if they can be accepted correctly.

[wanpengqian_gmail.com]

When parse opts, single config can also be accepted.

[chuck]

Thank you for this contribution!

The sectsz related changes look fine to me.

I think it is reasonable to require the blockif options immediately follow the block device name/path such that blockif_open() can parse its optional parameters. This has the added benefit of simplifying the changes made here. For this to work, the easiest option would be to require the user to specify devpath and any options last. For example:

-s 0:4:0,nvme,maxq=256,/dev/zvol/zroot/vol0,nocache,sectorsize=512

In an ideal world, the user should be able to specify devpath and its options anywhere within the configuration, but that would require teaching blockif_open() a) to stop processing options when it reaches one it doesn't recognize instead of returning an error and b) returning an indication of where it stopped processing.

usr.sbin/bhyve/pci_nvme.c

38

As the user would specify the block device options with the device path, perhaps it would make sense to move this comment below the "accepted devpath" comment.
Minor grammar suggestion would be to change the wording to "block device options can also be appended"

[wanpengqian_gmail.com]

In D25396#560854, @chuck wrote:

I think it is reasonable to require the blockif options immediately follow the block device name/path such that blockif_open() can parse its optional parameters. This has the added benefit of simplifying the changes made here. For this to work, the easiest option would be to require the user to specify devpath and any options last. For example:

-s 0:4:0,nvme,maxq=256,/dev/zvol/zroot/vol0,nocache,sectorsize=512

In an ideal world, the user should be able to specify devpath and its options anywhere within the configuration, but that would require teaching blockif_open() a) to stop processing options when it reaches one it doesn't recognize instead of returning an error and b) returning an indication of where it stopped processing.

Current code, it assumes first opt as devpath(I check the manpage, do we miss the devpath keyword?). I think we should keep it more clear, such as

nvme,[controller conf|[devpath=,[blockif conf]]]

so it becomes

-s 0:4:0,nvme,maxq=256,devpath=/dev/zvol/zroot/vol0,nocache,sectorsize=512

or

-s 0:4:0,nvme,maxq=256,ser=ABCD,devpath=ram,size=4096

The nvme controller will keep searching until devpath keyword, and pass the rest to blockif or parse it as ram.
The same situcation is faced in AHCI controller.

[wanpengqian_gmail.com]

• Update comment as suggested.

[chuck]

I like your suggestion of explicitly specifying devpath but need to think about the implications especially with regards to backwards compatibility.

[wanpengqian_gmail.com]

In D25396#561806, @chuck wrote:

I like your suggestion of explicitly specifying devpath but need to think about the implications especially with regards to backwards compatibility.

So current patch, only let nvme controller parse the known opts. while leave the others to blockif, seems fare enough.
I also have a patch D24174 for ahci controller. it does the same thing.

[chuck]

As an update, there is work underway to significantly enhance bhyve configuration. Once that lands, it should make many of the concerns here easier to address.

[grehan]

In D25396#565538, @chuck wrote:

As an update, there is work underway to significantly enhance bhyve configuration. Once that lands, it should make many of the concerns here easier to address.

Is this work described anywhere ?

[jhb]

In D25396#565756, @grehan wrote:

In D25396#565538, @chuck wrote:

As an update, there is work underway to significantly enhance bhyve configuration. Once that lands, it should make many of the concerns here easier to address.

Is this work described anywhere ?

It's been described in a few meeting notes. I've been working on it this week as I hadn't yet addressed device configuration, but I've almost finished that. It will need more testing though, but it does remove most of the string parsing from various backends and permits the use of both flat key=value config files as well as richer UCL-style config files (though I haven't implemented a UCL parser). I haven't posted it to lists yet since I need to finish the rest of the device stuff first.

The WIP (missing pci_xhci and USB device updates) is here: https://github.com/freebsd/freebsd/compare/master...bsdjhb:bhyve_config

[chuck]

This now works correctly with @jhb refactoring of configuration management (621b5090487de9fed1b503769702a9a2a27cc7bb / D26035). Is it OK to close this?

[wanpengqian_gmail.com]

In D25396#694493, @chuck wrote:

This now works correctly with @jhb refactoring of configuration management (621b5090487de9fed1b503769702a9a2a27cc7bb / D26035). Is it OK to close this?

Yes, I am fine. :-)