From 89824921516dbe1cb6cce482bca555da1fba15d5 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Mon, 13 Feb 2023 14:35:43 -0800 Subject: [PATCH] NVMe: don't panic if guests tries a doorbell write for admin queues while controller is disabled (#309) * nvme: use bare cast instead of try_into to better match spec The spec defines the doorbell registers as 32-bits wide but leaves the upper 16 bits as reserved. Now, a guest shouldn't be setting those upper bits but we can't control that. To better model that, just use a bare `as` cast to keep the bottom 16 bits rather than potentially failing via unwrapping a valid try_into. * nvme: don't process doorbell writes if controller disabled While the guest shouldn't be trying to write to the Admin or IO queue's doorbell registers when the controller isn't enabled, we also shouldn't just panic. Just log a warning and bail out. * nvme: remove unwraps for grabbing admin queues. Even though we should only be trying to unwrap at points we know the admin queues should exist, don't assume they are. We should treat them same as the IO queues where we'll bail with an error that gets logged instead of panic'ing. --- lib/propolis/src/hw/nvme/mod.rs | 79 +++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/lib/propolis/src/hw/nvme/mod.rs b/lib/propolis/src/hw/nvme/mod.rs index 536892bb0..48307af81 100644 --- a/lib/propolis/src/hw/nvme/mod.rs +++ b/lib/propolis/src/hw/nvme/mod.rs @@ -298,21 +298,13 @@ impl NvmeCtrl { } /// Returns a reference to the Admin [`CompQueue`]. - /// - /// # Panics - /// - /// Panics if the Admin Completion Queue hasn't been created yet. - fn get_admin_cq(&self) -> Arc { - self.get_cq(queue::ADMIN_QUEUE_ID).unwrap() + fn get_admin_cq(&self) -> Result, NvmeError> { + self.get_cq(queue::ADMIN_QUEUE_ID) } /// Returns a reference to the Admin [`SubQueue`]. - /// - /// # Panics - /// - /// Panics if the Admin Submission Queue hasn't been created yet. - fn get_admin_sq(&self) -> Arc { - self.get_sq(queue::ADMIN_QUEUE_ID).unwrap() + fn get_admin_sq(&self) -> Result, NvmeError> { + self.get_sq(queue::ADMIN_QUEUE_ID) } /// Configure Controller @@ -760,25 +752,49 @@ impl PciNvme { } CtrlrReg::DoorBellAdminSQ => { - let val = wo.read_u32().try_into().unwrap(); + // 32-bit register but ignore reserved top 16-bits + let val = wo.read_u32() as u16; let state = self.state.lock().unwrap(); - let admin_sq = state.get_admin_sq(); + + if !state.ctrl.cc.enabled() { + slog::warn!( + self.log, + "Doorbell write while controller is disabled" + ); + return Err(NvmeError::InvalidSubQueue( + queue::ADMIN_QUEUE_ID, + )); + } + + let admin_sq = state.get_admin_sq()?; admin_sq.notify_tail(val)?; // Process any new SQ entries self.process_admin_queue(state, admin_sq)?; } CtrlrReg::DoorBellAdminCQ => { - let val = wo.read_u32().try_into().unwrap(); + // 32-bit register but ignore reserved top 16-bits + let val = wo.read_u32() as u16; let state = self.state.lock().unwrap(); - let admin_cq = state.get_admin_cq(); + + if !state.ctrl.cc.enabled() { + slog::warn!( + self.log, + "Doorbell write while controller is disabled" + ); + return Err(NvmeError::InvalidCompQueue( + queue::ADMIN_QUEUE_ID, + )); + } + + let admin_cq = state.get_admin_cq()?; admin_cq.notify_head(val)?; // We may have skipped pulling entries off the admin sq // due to no available completion entry permit, so just // kick it here again in case. if admin_cq.kick() { - let admin_sq = state.get_admin_sq(); + let admin_sq = state.get_admin_sq()?; self.process_admin_queue(state, admin_sq)?; } } @@ -793,14 +809,30 @@ impl PciNvme { // // But note that we only support CAP.DSTRD = 0 let off = wo.offset() - 0x1000; + let is_cq = (off >> 2) & 0b1 == 0b1; + let qid = if is_cq { (off - 4) >> 3 } else { off >> 3 }; + + // Queue IDs should be 16-bit and we know `off <= CONTROLLER_REG_SZ (0x4000)` + let qid = qid.try_into().unwrap(); - let val: u16 = wo.read_u32().try_into().unwrap(); let state = self.state.lock().unwrap(); + if !state.ctrl.cc.enabled() { + slog::warn!( + self.log, + "Doorbell write while controller is disabled" + ); + return Err(if is_cq { + NvmeError::InvalidCompQueue(qid) + } else { + NvmeError::InvalidSubQueue(qid) + }); + } - if (off >> 2) & 0b1 == 0b1 { + // 32-bit register but ignore reserved top 16-bits + let val = wo.read_u32() as u16; + if is_cq { // Completion Queue y Head Doorbell - let y = (off - 4) >> 3; - let cq = state.get_cq(y as u16)?; + let cq = state.get_cq(qid)?; cq.notify_head(val)?; // We may have skipped pulling entries off some SQ due to this @@ -813,8 +845,7 @@ impl PciNvme { } } else { // Submission Queue y Tail Doorbell - let y = off >> 3; - let sq = state.get_sq(y as u16)?; + let sq = state.get_sq(qid)?; sq.notify_tail(val)?; // Poke block device to service new requests @@ -839,7 +870,7 @@ impl PciNvme { } // Grab the Admin CQ too - let cq = state.get_admin_cq(); + let cq = state.get_admin_cq()?; let mem = self.mem_access(); if mem.is_none() {