Page MenuHomeFreeBSD

bhyve: Fix NVMe BAR size calculation
ClosedPublic

Authored by chuck on Mar 22 2019, 5:22 AM.

Details

Summary

The NVMe specification defines bits 13:4 of BAR0 as Reserved (i.e. 0x0).
Most drivers do not enforce this, but the Windows NVMe driver does and
will refuse to start the device (i.e. error 10) if any of these bits are
set.

The current BAR size calculation tries to minimize the amount of memory
the device reserves by scaling the BAR size by the maximum number of
queues supported by the device. But unless the device supports a large
number of queue pairs (over 1536), it will reserve too little memory.

The fix is to allocate a minimum of 16K bytes for BAR0.

Test Plan

Boot Windows Server 2016
Open device manager and see device is present and not errored
Use Windows to label drive and create a file system

Diff Detail

Repository
rS 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

chuck created this revision.Mar 22 2019, 5:22 AM
jhb added inline comments.Mar 22 2019, 4:28 PM
usr.sbin/bhyve/pci_nvme.c
1861 ↗(On Diff #55341)

style wants you to use 4 space indent for all of the continuation lines. However, I think it would be more readable to just split this up into two lines:

pci_membar_sz = sizeof(struct nvme_registers) + ...
pci_membar_sz = MAX(pci_membar_sz, NVME_MMIO_SPACE_MIN);

I would perhaps simplify the comment as well to just say:

* The specification requires a minimum memory I/O window size of 16k.
* Certain versions of Windows refuse to start a device with a smaller window.
chuck updated this revision to Diff 55373.Mar 23 2019, 2:30 AM

Updated based on feedback

chuck marked an inline comment as done.Mar 23 2019, 2:31 AM
araujo accepted this revision.Mar 23 2019, 3:09 AM
jhb accepted this revision.Mar 25 2019, 10:06 PM
This revision is now accepted and ready to land.Mar 25 2019, 10:06 PM
imp accepted this revision.Mar 31 2019, 4:11 PM

one quick Q. if you know the windows versions, document it in the comment. the code is good to go. If you know, update the comment. If you don't know, that's cool too. either way, I no need to do a round trip through the review. you can just tweak that and commit.

usr.sbin/bhyve/pci_nvme.c
1857 ↗(On Diff #55373)

Do you know which ones?

chuck added a comment.EditedApr 4 2019, 3:30 PM
In D19676#423809, @imp wrote:

one quick Q. if you know the windows versions, document it in the comment.

Empirically, this fixes Windows server 2016 and 2019, but my assumption is all versions of Windows need this as I'd be surprised to find different Windows NVMe drivers.

But I see what you mean by the comment. Is there a better way to phrase this?

This revision was automatically updated to reflect the committed changes.