nvme: honor Set/Get Features 0Bh async event config#3673
Open
jeffbromberger wants to merge 5 commits into
Open
nvme: honor Set/Get Features 0Bh async event config#3673jeffbromberger wants to merge 5 commits into
jeffbromberger wants to merge 5 commits into
Conversation
The NVMe Base specification's "Feature Support Requirements" table
(Base 2.0c section 3.1.2.1.1 / Base 2.3 section 3.1.3.6 Figure 32)
lists FID 0Bh (Asynchronous Event Configuration) as mandatory for
I/O controllers, but the admin handler's default arm rejected every
Set Features 0Bh with INVALID_FIELD_IN_COMMAND. Initiators that
strictly follow the spec may refuse to allocate any AER resources
when this Feature cannot be configured, which breaks all subsequent
AEN delivery (including the changed-namespace AEN raised by
namespace hot-add).
Record Set Features FID 0Bh CDW11 on AdminState, return it from Get
Features, and consult bit 8 ("Attached Namespace Attribute Notices",
NVMe Base 2.0c section 5.21.1.11 / Base 2.3 section 5.2.26.1.5)
before firing the changed-namespace AEN. Per spec, "If this bit is
cleared to '0', then the controller shall not send the Attached
Namespace Attribute Changed asynchronous event to the host."
The cached mask defaults to all bits set so that the pre-existing
behavior (every AEN class enabled) is preserved for any host that
does not call Set Features 0Bh explicitly. A host that does call it
gets exactly the value it last wrote -- no bits added, no bits
silently dropped -- in line with the per-bit normative "shall not
report" language in the spec.
A new Cdw11FeatureAsyncEventConfig bitfield in nvme_spec describes
the CDW11 layout up through the NVMe 1.4-era notice classes
(SMART/Health byte + the six commonly-exercised notice bits). Bits
above are kept as opaque reserved space so the controller can store
and round-trip an arbitrary mask without acting on every bit.
Two unit tests pin the contract:
* test_set_get_features_async_event_config -- Set Features 0Bh
succeeds and Get Features 0Bh echoes the configured mask back
byte-exactly.
* test_async_event_config_masks_namespace_aen -- with bit 8
cleared, a namespace add does not fire an AEN within a generous
wait; toggling bit 8 back on without re-triggering the change
causes the previously-suppressed AEN to be delivered, with the
correct event_type (NOTICE) and log_page_identifier
(CHANGED_NAMESPACE_LIST) so a spec-compliant host knows which
log page to read.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for NVMe Asynchronous Event Configuration (Feature ID 0x0B) so the controller accepts Set/Get Features for AEC, persists the mask, and uses it to gate namespace-change AEN delivery.
Changes:
- Define the CDW11 bit layout for Asynchronous Event Configuration in
nvme_spec. - Store/echo the configured AEC CDW11 value via Set/Get Features and consult it when firing namespace-change AENs.
- Add regression tests for AEC Set/Get round-tripping and for mask-based suppression/reenable of the namespace-change AEN.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| vm/devices/storage/nvme_spec/src/lib.rs | Adds a typed bitfield for AEC CDW11 (FID 0x0B) to decode the AEN enable mask. |
| vm/devices/storage/nvme/src/workers/admin.rs | Persists AEC mask, implements Set/Get Features for FID 0x0B, and gates changed-namespace AEN delivery on bit 8. |
| vm/devices/storage/nvme/src/tests/controller_tests.rs | Adds tests verifying AEC Set/Get behavior and that bit 8 suppresses/enables the namespace-change AEN. |
Address review comments on PR microsoft#3673: * Use `u32::MAX` in place of `!0u32` when initializing `AdminState::async_event_config`. Equivalent value, easier to recognize as `all bits set`. * Update the Get Features comment for FID 0Bh to match the actual default. The cache is initialized to all bits set (not zero), so that a host which never issues Set Features 0Bh sees every notification class reported as enabled. * Rename the trailing field of `Cdw11FeatureAsyncEventConfig` from `_rsvd` to `_higher_bits`. The `_rsvd` name implied `ignore / must be zero`, but per the field's documentation the bits in this range are stored verbatim so Set Features / Get Features round-trip arbitrary host-supplied values. No functional change. All 13 nvme + nvme_spec unit tests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jstarks
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implement spec-mandated handling of NVMe Set/Get Features 0Bh (Asynchronous Event Configuration). Today the admin handler's default arm rejects this Feature with
INVALID_FIELD_IN_COMMAND, which the NVMe Base specification doesn't permit for I/O controllers and which has the practical effect of breaking AEN delivery against any spec-strict initiator.This PR:
Cdw11FeatureAsyncEventConfigtonvme_spec(CDW11 layout per NVMe Base 2.0c section 5.21.1.11 / Base 2.3 section 5.2.26.1.5).AdminStateand returns it from Get Features.shall not reportrequirement when the host has explicitly disabled that class.Why
NVMe Base 2.0c section 3.1.2.1.1 / Base 2.3 section 3.1.3.6 (
Feature Support Requirements, Figure 32) lists FID 0Bh as mandatory for I/O controllers. Rejecting it withINVALID_FIELD_IN_COMMANDis a spec violation.Initiators that strictly follow the spec gate their Asynchronous Event Request allocation on this Set Features command succeeding. When the command fails they may never queue any AERs, which means any AEN the controller subsequently tries to fire -- including the changed-namespace AEN that drives namespace hot-add notification -- has no outstanding completion slot to land in and is dropped.
Testing
Pre-commit checklist (per
.github/copilot-instructions.md) green onnvmeandnvme_spec:cargo check -p nvme -p nvme_speccargo clippy --all-targets -p nvme -p nvme_speccargo doc --no-deps -p nvme -p nvme_speccargo test -p nvme -p nvme_spec-- 13 passed (11 pre-existing + 2 new)cargo xtask fmt --fix-- no diffs on re-runThe two new tests drive the production
NvmeControllerand admin worker through real BAR0 / SQ / CQ / MSI-X paths (no mocks):test_set_get_features_async_event_config-- Set Features 0Bh succeeds; Get Features 0Bh echoes the configured mask back byte-exactly.test_async_event_config_masks_namespace_aen-- with bit 8 cleared, a namespace add does not fire an AEN within 500 ms; toggling bit 8 back on without re-triggering the change causes the previously-suppressed event to be delivered with the correctevent_type(NOTICE) andlog_page_identifier(CHANGED_NAMESPACE_LIST).Additionally verified end-to-end against a real NVMe initiator: a Windows guest running on an OpenVMM build with this patch now receives the namespace-changed AEN immediately on a runtime
add-nsoperation, and the new namespace appears in the guest as a fresh disk without any manual scan-for-changes. The cached mask post-initiator-init matches exactly what the initiator computed and sent (0x11F= SMART/Health bits + bit 8).