Skip to content

nvme: fix controller init for Windows Server 2025 / Windows 11 24H2 #1199

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.

Sign up for GitHub

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

Draft
daberkow wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

nvme: fix controller init for Windows Server 2025 / Windows 11 24H2 #1199

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 27 additions & 19 deletions lib/propolis/src/hw/nvme/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ struct NvmeCtrl {
/// Doorbell Buffer Config state
doorbell_buf: Option<queue::DoorbellBuffer>,

/// 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>,
Comment on lines +203 to +205

@iximeow iximeow Aug 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.


/// MSI-X Interrupt Handle to signal VM
msix_hdl: Option<pci::MsixHdl>,

Expand Down Expand Up @@ -539,6 +543,12 @@ impl NvmeCtrl {
self.ctrl.cc = Configuration(0);
self.ctrl.csts = Status(0);

// Parked Async Event Requests are implicitly aborted by the reset;
// their queues are already torn down so there is nothing to complete.
for permit in self.parked_aers.drain(..) {
permit.abandon();
}

// Other bits which are cleared on reset
self.doorbell_buf = None;

Expand Down Expand Up @@ -925,6 +935,7 @@ impl PciNvme {
device_id: DeviceId::new(),
ctrl: CtrlState { cap, vs, cc, csts, ..Default::default() },
doorbell_buf: None,
parked_aers: Vec::new(),
msix_hdl: None,
cqs: Default::default(),
sqs: Default::default(),
Expand Down Expand Up @@ -1044,22 +1055,21 @@ impl PciNvme {
ro.write_u32(state.ctrl.csts.0);
}
CtrlrReg::AdminQueueAttr => {
// 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).
Comment on lines +1058 to +1062

@iximeow iximeow Aug 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

let state = self.state.lock().unwrap();
if !state.ctrl.cc.enabled() {
ro.write_u32(state.ctrl.aqa.0);
}
ro.write_u32(state.ctrl.aqa.0);
}
CtrlrReg::AdminSubQAddr => {
let state = self.state.lock().unwrap();
if !state.ctrl.cc.enabled() {
ro.write_u64(state.ctrl.admin_sq_base);
}
ro.write_u64(state.ctrl.admin_sq_base);
}
CtrlrReg::AdminCompQAddr => {
let state = self.state.lock().unwrap();
if !state.ctrl.cc.enabled() {
ro.write_u64(state.ctrl.admin_cq_base);
}
ro.write_u64(state.ctrl.admin_cq_base);
}
CtrlrReg::Reserved => {
ro.fill(0);
Expand Down Expand Up @@ -1361,16 +1371,14 @@ impl PciNvme {
state.acmd_delete_io_sq(sqid, self)
}
AdminCmd::AsyncEventReq => {
// async event requests do not appear to be an optional
// feature but are not yet supported. The only
// command-specific error we could return is "async event
// limit exceeded".
//
// qemu's emulated NVMe also does not support async events
// but returns invalid opcode with the do-not-retry flag
// 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

@iximeow iximeow Aug 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

// 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);
Comment on lines +1374 to +1380

@iximeow iximeow Aug 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@iximeow iximeow Aug 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

continue;
}
AdminCmd::DoorbellBufCfg(cmd) => {
state.acmd_doorbell_buf_cfg(&cmd)
Expand Down
Loading