Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nvme: Increase the NVMe controller register size to 0x4000 to match spec requirements for MLBAR. #126

merged 3 commits into from May 2, 2022


Copy link

@luqmana luqmana commented May 2, 2022

The NVMe spec implies [1] a minimum size of 0x4000 for the controller register space. Windows thus expects BAR0 & !(0x4000 - 1) == BAR0 & !0xF to hold true. Which is all fine and well except Propolis sets the size to 0x2000 meaning we end up in this failing scenario where BAR0 = 0xFEDFE004 and so the above relation no longer holds true.

TL;DR: propolis set the NVMe controller register space to half the size it should at least be.

[1] Bits 13:04 should be set to 0.

See NVMe 1.0e Section 2.1.10 Offset 10h: MLBAR (BAR0) - Memory Register Base Address, lower 32 bits:

Bits Type Reset Description
31:14 RW 0h Base Address (BA): Base address of register memory space. For controllers that support a larger number of doorbell registers or have vendor specific space following the doorbell registers, more bits are allowed to be RO such that more memory space is consumed.
13:04 RO 0h Reserved

…pec requirements for MLBAR.

With that, we can boot successfully on Windows now with an NVMe boot
Copy link

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find

propolis/src/hw/nvme/ Outdated Show resolved Hide resolved
@luqmana luqmana merged commit 2129d16 into master May 2, 2022
@luqmana luqmana deleted the luqmana/nvme-up-bar-size branch May 2, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants