nvme: report a fixed maximum namespace ID in Identify Controller#3740
Open
jstarks wants to merge 2 commits into
Open
nvme: report a fixed maximum namespace ID in Identify Controller#3740jstarks wants to merge 2 commits into
jstarks wants to merge 2 commits into
Conversation
The NN field of Identify Controller specifies the maximum valid namespace ID for the NVM subsystem, a fixed property of the subsystem that a host reads once to bound its NSID scans. The emulator was instead deriving NN from the highest currently-present namespace ID, which is wrong on three counts: it changes as namespaces are added and removed, it reports zero when no namespaces are present (telling the host there are no valid NSIDs at all), and it can differ between the PF and its VFs even though they share one subsystem. Replace the dynamic value with a fixed MAX_NSID constant (1024), reported identically by every controller. Since NN now defines a hard bound on valid namespace IDs, also validate add_namespace against it and reject out-of-range or zero IDs rather than accepting a namespace the guest could never address. The NsidConflict error becomes an AddNamespaceError enum covering both the existing conflict case and the new range check.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR corrects NVMe Identify Controller reporting by making the NN field (maximum valid NSID for the subsystem) a fixed constant rather than deriving it from currently-present namespaces, and tightens namespace hot-add validation accordingly.
Changes:
- Introduces a fixed
MAX_NSID(1024) and reports it via Identify ControllerNNfor all controllers. - Validates
add_namespaceNSIDs against1..=MAX_NSIDand replacesNsidConflictwith a richerAddNamespaceError. - Adds unit tests covering NSID validation and fixed
NNbehavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/storage/nvme/src/workers/coordinator.rs | Updates client/request plumbing to return AddNamespaceError from namespace hot-add. |
| vm/devices/storage/nvme/src/workers/admin.rs | Implements AddNamespaceError, enforces NSID range, and reports nn = MAX_NSID. |
| vm/devices/storage/nvme/src/workers.rs | Re-exports AddNamespaceError instead of NsidConflict. |
| vm/devices/storage/nvme/src/tests/controller_tests.rs | Adds tests for NSID validation and fixed Identify Controller NN. |
| vm/devices/storage/nvme/src/resolver.rs | Updates resolver error handling to wrap AddNamespaceError. |
| vm/devices/storage/nvme/src/lib.rs | Adds MAX_NSID constant and re-exports AddNamespaceError. |
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.
The NN field of Identify Controller specifies the maximum valid namespace ID for the NVM subsystem, a fixed property of the subsystem that a host reads once to bound its NSID scans. The emulator was instead deriving NN from the highest currently-present namespace ID, which is wrong on three counts: it changes as namespaces are added and removed, it reports zero when no namespaces are present (telling the host there are no valid NSIDs at all), and it can differ between the PF and its VFs even though they share one subsystem.
Replace the dynamic value with a fixed MAX_NSID constant (1024), reported identically by every controller. Since NN now defines a hard bound on valid namespace IDs, also validate add_namespace against it and reject out-of-range or zero IDs rather than accepting a namespace the guest could never address. The NsidConflict error becomes an AddNamespaceError enum covering both the existing conflict case and the new range check.