nvme: fix controller init for Windows Server 2025 / Windows 11 24H2 - #1199
nvme: fix controller init for Windows Server 2025 / Windows 11 24H2#1199daberkow wants to merge 1 commit into
Conversation
The rewritten stornvme in Windows Server 2025 (26100) and Windows 11 24H2 could not see any NVMe disk; Setup reports "a media driver your computer needs is missing" while Server 2019/2022 and Windows 10 install fine. Two defects in the NVMe emulation combine to cause this: 1. AQA/ASQ/ACQ reads returned nothing while the controller was enabled. The spec restricts *writes* to these registers while CC.EN=1, but reads must always return the last-written value. stornvme reads ASQ back right after its first Identify Controller completes and treats the zero readback as a fatal adapter error (CM_PROB_FAILED_START, STATUS_ADAPTER_HARDWARE_ERROR), tearing the device down after exactly one successful admin command. Make the reads unconditional. 2. Async Event Requests were completed with Invalid Opcode + DNR, on the rationale that QEMU does the same; that comment is stale (modern QEMU implements AER). stornvme ignores DNR and immediately resubmits, producing a permanent ~8k cmd/s admin loop during which boot of the installed OS never proceeds. Park AER permits, retaining them until controller reset, as the spec prescribes; no events are ever posted. Verified on bhyve/Helios: Server 2025 installs end-to-end to the logon screen (4 parked AERs per controller at steady state), Windows 11 installs to OOBE, and Server 2022 still installs on the same build. Known limitation: parked AER permits are not preserved across live migration export/import.
iximeow
left a comment
iximeow
left a comment
There was a problem hiding this comment.
as i'd mentioned in chat: thanks for taking a look at this! it's good that server 2025 is relatively close to being OK with Propolis :) i realize this is something of a "draft/wdyt", i don't necessarily expect you to do things with this review but this hopefully demonstrates the bar i'd want us to hold ourselves to when looking at changing device emulation like this. getting into it:
The rewritten stornvme
what do you mean by rewritten? i didn't (quickly!) find anything about a substantial rewrite of Windows' NVMe driver lately, so i'm not sure how to read this or of we're just seeing some relatively minor additions to the driver that happen to interact poorly with our emulated device.
and treats the zero readback as a fatal adapter error (CM_PROB_FAILED_START, STATUS_ADAPTER_HARDWARE_ERROR)
i assume this is just determined empirically from "it now does not read as zero, and does not report that the hardware has errored"? was there a more direct debugging path that got you here?
stornvme ignores DNR and immediately resubmits,
bummer. i assume you found this with a bit of dtrace on the nvme probes?
Park AER permits, retaining them until controller reset, as the spec prescribes
"Park" is not an NVMe term of art, right? i think you mean to say that this patch accepts whatever the guest might have written to AsyncEventReq, and we just do nothing to ever fulfill that async event request. "Parking" at least to me sounds like it's gesturing at some NVMe idea of an "idle" AER which is mostly just confusing.
4 parked AERs per controller at steady state
how did you measure this? it's not directly exposed here, and it's not like there's a specific probe when AERs are received, is there some careful dtrace or is this just "added some println() and only saw four requests"?
migration, testing
it seems pretty straightforward to say we should be able to migrate oustanding AERs with the rest of the controller configuration, so unless we're under time pressure lets not make NVMe migration worse off? 😁
when i'd mentioned testcases earlier, i was thinking of, basically, the shape of test environment i'd set up in #966. clearly that's not in yet, so it's hard to build on that :) i'll get that sorted next week when i'm properly back and get you something reasonable to build on. but that's really where i think we'd want to remark that "such and such behavior is important for [at least] such and such guest, and is described by NVMe rev X.Y section A.B.C, ..." rather than on (kinda arbitrary) fields of controller state.
| // These registers may only be modified while the controller | ||
| // is disabled, but reads must always return the last value | ||
| // written (NVMe 1.0e Section 3.1; e.g. Windows Server 2025's | ||
| // stornvme reads ASQ back after enabling the controller and | ||
| // treats a mismatch as a fatal adapter error). |
There was a problem hiding this comment.
what part of 1.0e section 3.1 says this, to you? I see 3.1.7 talks about AQA and in particular defines ACQS and ASQS as RW. from 3.1.7 it's not clear to me what the write semantic should be (do these hold the "next" admin queue settings, latched into place when CC.EN is next set to 1?)
the actual answer here looks to me to be 3.1.5 Offset 14h: CC - Controller Configuration where EN says these must simply not be written to. I'd actually strike the comment here (this is in the read path, you're allowed to read the register, it should read as what the device is configured for), and note this where we presumably drop the write if the controller is active.
| // Async Event Requests remain outstanding until an event | ||
| // occurs (which we never post) or the controller is | ||
| // reset; they do not receive an immediate completion. | ||
| // Completing them with an error instead sends some | ||
| // guests (e.g. Windows Server 2025's stornvme) into a | ||
| // tight resubmit loop, despite the do-not-retry flag. | ||
| state.parked_aers.push(permit); |
There was a problem hiding this comment.
I would be curious about the qemu archaeology of either growing AER support or doing what this patch proposes; if the existing comment no longer describes qemu, what was qemu's change? if it's just "qemu supports AERs", well, okay then.
| // Completing them with an error instead sends some | ||
| // guests (e.g. Windows Server 2025's stornvme) into a | ||
| // tight resubmit loop, despite the do-not-retry flag. | ||
| state.parked_aers.push(permit); |
There was a problem hiding this comment.
also we definitely need to have correct error behavior when the guest creates more than AERL-many oustanding AERs, and we really ought to make sure the AER is sensible before deciding to imply we're tolerating it by not immediately completing with an error.
| /// Async Event Requests parked until an event is posted or the | ||
| /// controller is reset (see the AsyncEventReq admin command handling). | ||
| parked_aers: Vec<queue::Permit>, |
There was a problem hiding this comment.
same nit here about "parked"; i'd just call these "outstanding AERs" and we should do ourselves the favor of at least retaining the log page an AER has registered interest in, at this point.
realistically, I think we would want something like a BTreeMap from log pages to outstanding event information or outstanding AERs. it's not like looking through a list of four entries for a matching AER is that bad, but that would help keep the semantics of things like
When the controller posts a completion queue entry for an outstanding Asynchronous Event Request command and thus reports an asynchronous event, subsequent events of that event type are automatically masked by the controller until the host clears that event.
a bit clearer.
| // set. Do the same so that guest drivers that check for | ||
| // this can detect it and stop posting async events. | ||
| cmds::Completion::generic_err(bits::STS_INVAL_OPC).dnr() | ||
| // Async Event Requests remain outstanding until an event |
There was a problem hiding this comment.
what are the log pages that Server 2025 (and Windows 11) want to monitor? in 1.0e there's Error Information, SMART / Health Information, and Firmware Slot Information which are all mandatory and i can forgive just setting up AERs for those. but you said that Windows sets up four AERs - what's the last one!
later NVMe spec versions have many more log pages defined, but i'm not sure which are madatory or not, etc
daberkow
added
bug
|
Hi, I apologize Claude wrote some of that and I fully defer to you. This was meant as more of a proof of concept and a conversation starter about this issue. With Windows 11 and Server 2025 introducing things like DirectStorage, they have rewritten a lot of the NVMe driver, and it seems to be tripping up the NVMe interface. |
2 participants
The rewritten stornvme made Windows Server 2025 (26100) and Windows 11 24H2 not see any NVMe disk; Setup reports "a media driver your computer needs is missing" while Server 2019/2022 and Windows 10 install fine. Two defects in the NVMe emulation combine to cause this:
AQA/ASQ/ACQ reads returned nothing while the controller was enabled. The spec restricts writes to these registers while CC.EN=1, but reads must always return the last-written value. stornvme reads ASQ back right after its first Identify Controller completes and treats the zero readback as a fatal adapter error (CM_PROB_FAILED_START, STATUS_ADAPTER_HARDWARE_ERROR), tearing the device down after exactly one successful admin command. Make the reads unconditional.
Async Event Requests were completed with Invalid Opcode + DNR, on the rationale that QEMU does the same; that comment is stale (modern QEMU implements AER). stornvme ignores DNR and immediately resubmits, producing a permanent ~8k cmd/s admin loop during which boot of the installed OS never proceeds. Park AER permits, retaining them until controller reset, as the spec prescribes; no events are ever posted.
Verified on bhyve/Helios: Server 2025 installs end-to-end to the logon screen (4 parked AERs per controller at steady state), Windows 11 installs to OOBE, and Server 2022 still installs on the same build.
Known limitation: parked AER permits are not preserved across live migration export/import.