Page MenuHomeFreeBSD

bhyve: Fix NVMe BAR size calculation
ClosedPublic

Authored by chuck on Mar 22 2019, 5:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 12:19 AM
Unknown Object (File)
Nov 18 2024, 8:49 PM
Unknown Object (File)
Sep 24 2024, 3:35 AM
Unknown Object (File)
Sep 24 2024, 12:26 AM
Unknown Object (File)
Sep 17 2024, 11:01 AM
Unknown Object (File)
Sep 15 2024, 8:24 PM
Unknown Object (File)
Sep 10 2024, 10:05 AM
Unknown Object (File)
Sep 4 2024, 11:42 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

Updated based on feedback

This revision is now accepted and ready to land.Mar 25 2019, 10:06 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?

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.