nvme: report a fixed maximum namespace ID in Identify Controller

vm/devices/storage/nvme/src/lib.rs

@@ -53,7 +53,7 @@ mod tests;
 
 pub use pci::NvmeController;
 pub use pci::NvmeControllerCaps;
-pub use workers::NsidConflict;
+pub use workers::AddNamespaceError;
 pub use workers::NvmeControllerClient;
 
 use guestmem::ranges::PagedRange;
@@ -67,6 +67,11 @@ const VENDOR_ID: u16 = 0x1414;
 const DEVICE_ID: u16 = 0xc03e;
 const NVME_VERSION: u32 = 0x00020000;
 const MAX_QES: u16 = 256;
+/// Maximum valid namespace ID for the NVM subsystem, reported in the `NN`
+/// field of Identify Controller. This is a fixed property of the subsystem
+/// (the size of the NSID address space), identical across all controllers,
+/// and is independent of how many namespaces are currently present.
+const MAX_NSID: u32 = 1024;
 const BAR0_LEN: u64 = 0x10000;
 const IOSQES: u8 = 6;
 const IOCQES: u8 = 4;

vm/devices/storage/nvme/src/resolver.rs

@@ -3,7 +3,7 @@
 
 //! Resource resolver for the nvme controller.
 
-use crate::NsidConflict;
+use crate::AddNamespaceError;
 use crate::NvmeController;
 use crate::NvmeControllerCaps;
 use crate::NvmeControllerClient;
@@ -44,7 +44,7 @@ pub enum Error {
         source: ResolveError,
     },
     #[error(transparent)]
-    NsidConflict(NsidConflict),
+    AddNamespace(AddNamespaceError),
 }
 
 #[async_trait]
@@ -89,7 +89,7 @@ impl AsyncResolveResource<PciDeviceHandleKind, NvmeControllerHandle> for NvmeCon
                 .client()
                 .add_namespace(nsid, disk.0)
                 .await
-                .map_err(Error::NsidConflict)?;
+                .map_err(Error::AddNamespace)?;
         }
 
         if let Some(requests) = resource.requests {

vm/devices/storage/nvme/src/tests/controller_tests.rs

@@ -2,7 +2,9 @@
 // Licensed under the MIT License.
 
 use super::test_helpers::TestNvmeMmioRegistration;
+use crate::AddNamespaceError;
 use crate::BAR0_LEN;
+use crate::MAX_NSID;
 use crate::NvmeController;
 use crate::NvmeControllerCaps;
 use crate::PAGE_SIZE64;
@@ -969,3 +971,145 @@ async fn test_async_event_config_masks_namespace_aen(driver: DefaultDriver) {
         "AEN log_page_identifier must point at CHANGED_NAMESPACE_LIST"
     );
 }
+
+/// `add_namespace` validates the NSID against the subsystem's valid range
+/// (`1..=MAX_NSID`) and rejects duplicates. Verify each failure mode maps to
+/// the right `AddNamespaceError` variant and that the boundary NSIDs are
+/// accepted.
+#[async_test]
+async fn test_add_namespace_validation(driver: DefaultDriver) {
+    let gm = test_memory();
+    let nvmec = instantiate_controller(driver, &gm, None);
+    let client = nvmec.client();
+
+    // NSID 0 is reserved and must be rejected as out of range.
+    let err = client
+        .add_namespace(0, ram_disk(1 << 20, false).unwrap())
+        .await
+        .unwrap_err();
+    assert!(
+        matches!(err, AddNamespaceError::OutOfRange(0)),
+        "NSID 0 should be OutOfRange, got {err:?}"
+    );
+
+    // An NSID above the subsystem maximum must be rejected.
+    let err = client
+        .add_namespace(MAX_NSID + 1, ram_disk(1 << 20, false).unwrap())
+        .await
+        .unwrap_err();
+    assert!(
+        matches!(err, AddNamespaceError::OutOfRange(n) if n == MAX_NSID + 1),
+        "NSID above MAX_NSID should be OutOfRange, got {err:?}"
+    );
+
+    // The boundary values 1 and MAX_NSID are valid.
+    client
+        .add_namespace(1, ram_disk(1 << 20, false).unwrap())
+        .await
+        .unwrap();
+    client
+        .add_namespace(MAX_NSID, ram_disk(1 << 20, false).unwrap())
+        .await
+        .unwrap();
+
+    // Re-adding an existing NSID must be reported as a conflict.
+    let err = client
+        .add_namespace(1, ram_disk(1 << 20, false).unwrap())
+        .await
+        .unwrap_err();
+    assert!(
+        matches!(err, AddNamespaceError::Conflict(1)),
+        "duplicate NSID should be Conflict, got {err:?}"
+    );
+}
+
+/// Issue an IDENTIFY CONTROLLER command into admin slot `slot`, write the
+/// result to `data_gpa`, and return the reported `NN` (maximum valid NSID)
+/// field.
+async fn identify_controller_nn(
+    nvmec: &mut NvmeController,
+    gm: &GuestMemory,
+    asq: &PrpRange,
+    acq: &PrpRange,
+    int_controller: &TestPciInterruptController,
+    driver: DefaultDriver,
+    slot: u32,
+    data_gpa: u64,
+) -> u32 {
+    let mut entry = spec::Command::new_zeroed();
+    entry.cdw0.set_opcode(spec::AdminOpcode::IDENTIFY.0);
+    entry.cdw10 = u32::from(spec::Cdw10Identify::new().with_cns(spec::Cns::CONTROLLER.0));
+    entry.dptr[0] = data_gpa;
+
+    write_command_to_queue(gm, asq, slot as usize, &entry);
+    nvmec.write_bar0(0x1000, (slot + 1).as_bytes()).unwrap();
+    wait_for_msi(driver, int_controller, 1000, 0xfeed0000, 0x1111).await;
+
+    let cqe = read_completion_from_queue(gm, acq, slot as usize);
+    assert_eq!(cqe.status.status(), spec::Status::SUCCESS.0);
+
+    gm.read_plain::<spec::IdentifyController>(data_gpa)
+        .unwrap()
+        .nn
+}
+
+/// The `NN` field of Identify Controller reports the fixed size of the NSID
+/// address space (`MAX_NSID`), independent of how many namespaces are
+/// actually present.
+#[async_test]
+async fn test_identify_reports_fixed_nn(driver: DefaultDriver) {
+    let acq = PrpRange::new(vec![0], 0, PAGE_SIZE64).unwrap();
+    let asq = PrpRange::new(vec![0x1000], 0, PAGE_SIZE64).unwrap();
+    let gm = test_memory();
+    let int_controller = TestPciInterruptController::new();
+
+    let mut nvmec = instantiate_and_build_admin_queue(
+        &acq,
+        64,
+        &asq,
+        64,
+        true,
+        Some(&int_controller),
+        driver.clone(),
+        &gm,
+    )
+    .await;
+
+    // No namespaces present: NN still reports the fixed subsystem maximum.
+    let nn = identify_controller_nn(
+        &mut nvmec,
+        &gm,
+        &asq,
+        &acq,
+        &int_controller,
+        driver.clone(),
+        0,
+        0x8000,
+    )
+    .await;
+    assert_eq!(nn, MAX_NSID, "NN must report MAX_NSID with no namespaces");
+
+    // Add a namespace; NN must remain MAX_NSID rather than tracking the
+    // highest present NSID.
+    nvmec
+        .client()
+        .add_namespace(1, ram_disk(1 << 20, false).unwrap())
+        .await
+        .unwrap();
+
+    let nn = identify_controller_nn(
+        &mut nvmec,
+        &gm,
+        &asq,
+        &acq,
+        &int_controller,
+        driver.clone(),
+        1,
+        0x9000,
+    )
+    .await;
+    assert_eq!(
+        nn, MAX_NSID,
+        "NN must remain MAX_NSID after adding a namespace"
+    );
+}

vm/devices/storage/nvme/src/workers.rs

@@ -7,7 +7,7 @@ mod admin;
 mod coordinator;
 mod io;
 
-pub use admin::NsidConflict;
+pub use admin::AddNamespaceError;
 pub use coordinator::NvmeControllerClient;
 pub use coordinator::NvmeWorkers;
 

vm/devices/storage/nvme/src/workers/admin.rs

@@ -8,6 +8,7 @@ use super::MAX_DATA_TRANSFER_SIZE;
 use super::io::IoHandler;
 use super::io::IoState;
 use crate::DOORBELL_STRIDE_BITS;
+use crate::MAX_NSID;
 use crate::MAX_QES;
 use crate::NVME_VERSION;
 use crate::PAGE_MASK;
@@ -344,10 +345,17 @@ enum Event {
     NamespaceChange(u32),
 }
 
-/// Error returned when adding a namespace with a conflicting ID.
+/// Error returned when a namespace cannot be added.
 #[derive(Debug, Error)]
-#[error("namespace id conflict for {0}")]
-pub struct NsidConflict(u32);
+pub enum AddNamespaceError {
+    /// A namespace with this ID already exists.
+    #[error("namespace id conflict for {0}")]
+    Conflict(u32),
+    /// The namespace ID is outside the valid range supported by the
+    /// subsystem (see the `NN` field of Identify Controller).
+    #[error("namespace id {0} is out of range (must be 1..={MAX_NSID})")]
+    OutOfRange(u32),
+}
 
 impl AdminHandler {
     pub fn new(driver: VmTaskDriver, config: AdminConfig) -> Self {
@@ -363,14 +371,17 @@ impl AdminHandler {
         state: Option<&mut AdminState>,
         nsid: u32,
         disk: Disk,
-    ) -> Result<(), NsidConflict> {
+    ) -> Result<(), AddNamespaceError> {
+        if nsid == 0 || nsid > MAX_NSID {
+            return Err(AddNamespaceError::OutOfRange(nsid));
+        }
         let namespace = &*match self.namespaces.entry(nsid) {
             btree_map::Entry::Vacant(entry) => entry.insert(Arc::new(Namespace::new(
                 self.config.mem.clone(),
                 nsid,
                 disk,
             ))),
-            btree_map::Entry::Occupied(_) => return Err(NsidConflict(nsid)),
+            btree_map::Entry::Occupied(_) => return Err(AddNamespaceError::Conflict(nsid)),
         };
 
         if let Some(state) = state {
@@ -591,17 +602,27 @@ impl AdminHandler {
                 }
             }
             spec::Cns::NAMESPACE => {
+                if command.nsid == 0 || command.nsid > MAX_NSID {
+                    return Err(spec::Status::INVALID_NAMESPACE_OR_FORMAT.into());
+                }
                 if let Some(ns) = self.namespaces.get(&command.nsid) {
                     ns.identify(buf);
                 } else {
-                    tracelimit::warn_ratelimited!(nsid = command.nsid, "unknown namespace id");
+                    // Valid but inactive namespace: return a zero-filled
+                    // structure (the buffer is already zeroed).
+                    tracing::debug!(nsid = command.nsid, "inactive namespace id");
                 }
             }
             spec::Cns::DESCRIPTOR_NAMESPACE => {
+                if command.nsid == 0 || command.nsid > MAX_NSID {
+                    return Err(spec::Status::INVALID_NAMESPACE_OR_FORMAT.into());
+                }
                 if let Some(ns) = self.namespaces.get(&command.nsid) {
                     ns.namespace_id_descriptor(buf);
                 } else {
-                    tracelimit::warn_ratelimited!(nsid = command.nsid, "unknown namespace id");
+                    // Valid but inactive namespace: return a zero-filled
+                    // structure (the buffer is already zeroed).
+                    tracing::debug!(nsid = command.nsid, "inactive namespace id");
                 }
             }
             cns => {
@@ -628,7 +649,7 @@ impl AdminHandler {
                 .with_min(IOCQES)
                 .with_max(IOCQES),
             frmw: spec::FirmwareUpdates::new().with_ffsro(true).with_nofs(1),
-            nn: self.namespaces.keys().copied().max().unwrap_or(0),
+            nn: MAX_NSID,
             ieee: [0x74, 0xe2, 0x8c], // Microsoft
             fr: (*b"v1.00000").into(),
             mn: (*b"MSFT NVMe Accelerator v1.0              ").into(),

vm/devices/storage/nvme/src/workers/coordinator.rs

@@ -4,10 +4,10 @@
 //! Coordinator between queues and hot add/remove of namespaces.
 
 use super::IoQueueEntrySizes;
+use super::admin::AddNamespaceError;
 use super::admin::AdminConfig;
 use super::admin::AdminHandler;
 use super::admin::AdminState;
-use super::admin::NsidConflict;
 use crate::queue::DoorbellMemory;
 use crate::queue::InvalidDoorbell;
 use disk_backend::Disk;
@@ -185,7 +185,7 @@ pub struct NvmeControllerClient {
 
 impl NvmeControllerClient {
     /// Adds a namespace.
-    pub async fn add_namespace(&self, nsid: u32, disk: Disk) -> Result<(), NsidConflict> {
+    pub async fn add_namespace(&self, nsid: u32, disk: Disk) -> Result<(), AddNamespaceError> {
         self.send
             .call(CoordinatorRequest::AddNamespace, (nsid, disk))
             .await
@@ -212,7 +212,7 @@ struct Coordinator {
 
 enum CoordinatorRequest {
     EnableAdmin(Rpc<EnableAdminParams, ()>),
-    AddNamespace(Rpc<(u32, Disk), Result<(), NsidConflict>>),
+    AddNamespace(Rpc<(u32, Disk), Result<(), AddNamespaceError>>),
     RemoveNamespace(Rpc<u32, bool>),
     Inspect(inspect::Deferred),
     ControllerReset(Rpc<(), ()>),

vm/devices/storage/nvme_test/src/lib.rs

@@ -23,14 +23,19 @@ pub use workers::NvmeFaultControllerClient;
 
 use guestmem::ranges::PagedRange;
 use nvme_spec as spec;
-use workers::NsidConflict;
+use workers::AddNamespaceError;
 
 // Device configuration shared by PCI and NVMe.
 const DOORBELL_STRIDE_BITS: u8 = 2;
 const VENDOR_ID: u16 = 0x1414;
 const DEVICE_ID: u16 = 0xc03e;
 const NVME_VERSION: u32 = 0x00020000;
 const MAX_QES: u16 = 256;
+/// Maximum valid namespace ID for the NVM subsystem, reported in the `NN`
+/// field of Identify Controller. This is a fixed property of the subsystem
+/// (the size of the NSID address space), identical across all controllers,
+/// and is independent of how many namespaces are currently present.
+const MAX_NSID: u32 = 1024;
 const BAR0_LEN: u64 = 0x10000;
 const IOSQES: u8 = 6;
 const IOCQES: u8 = 4;

vm/devices/storage/nvme_test/src/resolver.rs

@@ -3,7 +3,7 @@
 
 //! Resource resolver for the nvme controller.
 
-use crate::NsidConflict;
+use crate::AddNamespaceError;
 use crate::NvmeFaultController;
 use crate::NvmeFaultControllerCaps;
 use async_trait::async_trait;
@@ -39,7 +39,7 @@ pub enum Error {
         source: ResolveError,
     },
     #[error(transparent)]
-    NsidConflict(NsidConflict),
+    AddNamespace(AddNamespaceError),
 }
 
 #[async_trait]
@@ -97,7 +97,7 @@ impl AsyncResolveResource<PciDeviceHandleKind, NvmeFaultControllerHandle>
                 .client()
                 .add_namespace(nsid, disk.0)
                 .await
-                .map_err(Error::NsidConflict)?;
+                .map_err(Error::AddNamespace)?;
         }
         Ok(controller.into())
     }

vm/devices/storage/nvme_test/src/tests/controller_tests.rs

@@ -359,6 +359,10 @@ async fn test_send_identify_no_fault(driver: DefaultDriver) {
 async fn test_send_identify_with_sq_fault(driver: DefaultDriver) {
     let mut faulty_identify = Command::new_zeroed();
     faulty_identify.cdw0.set_cid(10);
+    // Use a valid Identify CONTROLLER request (which ignores NSID) so the
+    // command still completes successfully; this test only verifies that the
+    // fault overwrote the command (observed via the changed CID).
+    faulty_identify.cdw10 = u32::from(spec::Cdw10Identify::new().with_cns(spec::Cns::CONTROLLER.0));
 
     let fault_configuration = FaultConfiguration::new(CellUpdater::new(true).cell())
         .with_admin_queue_fault(

vm/devices/storage/nvme_test/src/workers.rs

@@ -7,7 +7,7 @@ mod admin;
 mod coordinator;
 mod io;
 
-pub use admin::NsidConflict;
+pub use admin::AddNamespaceError;
 pub use coordinator::NvmeFaultControllerClient;
 pub use coordinator::NvmeWorkers;