From a0a09fc6eceac1f54dfcee5cf37493b3e53a3a59 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Sep 2025 19:51:33 +0000 Subject: [PATCH 1/5] Initial plan From 9c7badbd5a21cc71cbe9bcca9e853fcd6ca6ee47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Sep 2025 20:16:11 +0000 Subject: [PATCH 2/5] nvme_test: update shadow doorbell handling to mirror nvme crate Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com> --- Cargo.lock | 1 - vm/devices/storage/nvme_test/Cargo.toml | 1 - vm/devices/storage/nvme_test/src/pci.rs | 8 +- vm/devices/storage/nvme_test/src/queue.rs | 451 ++++++++++-------- .../storage/nvme_test/src/workers/admin.rs | 164 +++---- .../nvme_test/src/workers/coordinator.rs | 21 +- .../storage/nvme_test/src/workers/io.rs | 92 ++-- 7 files changed, 352 insertions(+), 386 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8e6734c771..e76a9464f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4865,7 +4865,6 @@ dependencies = [ "chipset_device", "device_emulators", "disk_backend", - "event-listener", "futures", "futures-concurrency", "guestmem", diff --git a/vm/devices/storage/nvme_test/Cargo.toml b/vm/devices/storage/nvme_test/Cargo.toml index 1598dcad6d..1e0f49d914 100644 --- a/vm/devices/storage/nvme_test/Cargo.toml +++ b/vm/devices/storage/nvme_test/Cargo.toml @@ -27,7 +27,6 @@ inspect.workspace = true mesh.workspace = true pal_async.workspace = true async-trait.workspace = true -event-listener.workspace = true futures.workspace = true futures-concurrency.workspace = true parking_lot.workspace = true diff --git a/vm/devices/storage/nvme_test/src/pci.rs b/vm/devices/storage/nvme_test/src/pci.rs index c1d4815195..2c9ccb76d7 100644 --- a/vm/devices/storage/nvme_test/src/pci.rs +++ b/vm/devices/storage/nvme_test/src/pci.rs @@ -233,15 +233,15 @@ impl NvmeFaultController { if addr >= 0x1000 { // Doorbell write. let base = addr - 0x1000; - let index = base >> DOORBELL_STRIDE_BITS; - if (index << DOORBELL_STRIDE_BITS) != base { + let db_id = base >> DOORBELL_STRIDE_BITS; + if (db_id << DOORBELL_STRIDE_BITS) != base { return IoResult::Err(InvalidRegister); } let Ok(data) = data.try_into() else { return IoResult::Err(IoError::InvalidAccessSize); }; - let data = u32::from_ne_bytes(data); - self.workers.doorbell(index, data); + let value = u32::from_ne_bytes(data); + self.workers.doorbell(db_id, value); return IoResult::Ok; } diff --git a/vm/devices/storage/nvme_test/src/queue.rs b/vm/devices/storage/nvme_test/src/queue.rs index 0b603c1c94..7b46ae9266 100644 --- a/vm/devices/storage/nvme_test/src/queue.rs +++ b/vm/devices/storage/nvme_test/src/queue.rs @@ -3,266 +3,338 @@ //! NVMe submission and completion queue types. +use crate::DOORBELL_STRIDE_BITS; use crate::spec; use guestmem::GuestMemory; use guestmem::GuestMemoryError; use inspect::Inspect; +use parking_lot::RwLock; use std::sync::Arc; -use std::sync::atomic::AtomicU32; use std::sync::atomic::Ordering; +use std::task::Context; +use std::task::Poll; +use std::task::Waker; +use std::task::ready; use thiserror::Error; use vmcore::interrupt::Interrupt; -pub const ILLEGAL_DOORBELL_VALUE: u32 = 0xffffffff; - -#[derive(Default, Inspect)] -#[inspect(transparent)] -pub struct DoorbellRegister { - #[inspect(hex)] - value: AtomicU32, - #[inspect(skip)] - event: event_listener::Event, +pub struct DoorbellMemory { + mem: GuestMemory, + offset: u64, + event_idx_offset: Option, + wakers: Vec>, } -impl DoorbellRegister { - pub fn new() -> Self { - Self::default() +pub struct InvalidDoorbell; + +impl DoorbellMemory { + pub fn new(num_qids: u16) -> Self { + Self { + mem: GuestMemory::allocate((num_qids as usize) << DOORBELL_STRIDE_BITS), + offset: 0, + event_idx_offset: None, + wakers: (0..num_qids).map(|_| None).collect(), + } } - pub fn write(&self, value: u32) { - self.value.store(value, Ordering::SeqCst); - self.event.notify(usize::MAX); + /// Update the memory used to store the doorbell values. This is used to + /// support shadow doorbells, where the values are directly in guest memory. + pub fn replace_mem( + &mut self, + mem: GuestMemory, + offset: u64, + event_idx_offset: Option, + ) -> Result<(), GuestMemoryError> { + // Copy the current doorbell values into the new memory. + let len = self.wakers.len() << DOORBELL_STRIDE_BITS; + let mut current = vec![0; len]; + self.mem.read_at(self.offset, &mut current)?; + mem.write_at(offset, ¤t)?; + if let Some(event_idx_offset) = event_idx_offset { + // Catch eventidx up to the current doorbell value. + mem.write_at(event_idx_offset, ¤t)?; + } + self.mem = mem; + self.offset = offset; + self.event_idx_offset = event_idx_offset; + Ok(()) } - pub fn read(&self) -> u32 { - self.value.load(Ordering::SeqCst) + pub fn try_write(&self, db_id: u16, value: u32) -> Result<(), InvalidDoorbell> { + if (db_id as usize) >= self.wakers.len() { + return Err(InvalidDoorbell); + } + self.write(db_id, value); + Ok(()) } - pub async fn wait_read(&self, value: u32) -> u32 { - let v = self.read(); - if value != v { - return v; + fn write(&self, db_id: u16, value: u32) { + assert!((db_id as usize) < self.wakers.len()); + let addr = self + .offset + .wrapping_add((db_id as u64) << DOORBELL_STRIDE_BITS); + if let Err(err) = self.mem.write_plain(addr, &value) { + tracelimit::error_ratelimited!( + error = &err as &dyn std::error::Error, + "failed to write doorbell memory" + ); } - loop { - let listener = self.event.listen(); - let v = self.read(); - if value != v { - break v; - } - listener.await; + if let Some(waker) = &self.wakers[db_id as usize] { + waker.wake_by_ref(); } } + + fn read(&self, db_id: u16) -> Option { + assert!((db_id as usize) < self.wakers.len()); + self.mem + .read_plain( + self.offset + .wrapping_add((db_id as u64) << DOORBELL_STRIDE_BITS), + ) + .inspect_err(|err| { + tracelimit::error_ratelimited!( + error = err as &dyn std::error::Error, + "failed to read doorbell memory" + ); + }) + .ok() + } + + fn has_event_idx(&self) -> bool { + self.event_idx_offset.is_some() + } + + fn write_event_idx(&self, db_id: u16, val: u32) { + assert!((db_id as usize) < self.wakers.len()); + if let Err(err) = self.mem.write_plain( + self.event_idx_offset + .unwrap() + .wrapping_add((db_id as u64) << DOORBELL_STRIDE_BITS), + &val, + ) { + tracelimit::error_ratelimited!( + error = &err as &dyn std::error::Error, + "failed to read event_idx memory" + ) + } + } + + fn read_event_idx(&self, db_id: u16) -> Option { + assert!((db_id as usize) < self.wakers.len()); + self.mem + .read_plain( + self.event_idx_offset? + .wrapping_add((db_id as u64) << DOORBELL_STRIDE_BITS), + ) + .inspect_err(|err| { + tracelimit::error_ratelimited!( + error = err as &dyn std::error::Error, + "failed to read doorbell memory" + ); + }) + .ok() + } } -#[derive(Copy, Clone, Default, Inspect, Debug)] -pub struct ShadowDoorbell { +#[derive(Inspect)] +#[inspect(extra = "Self::inspect_shadow")] +struct DoorbellState { #[inspect(hex)] - pub shadow_db_gpa: u64, + current: u32, #[inspect(hex)] - pub event_idx_gpa: u64, + event_idx: u32, + db_id: u16, + db_offset: u64, + #[inspect(hex)] + len: u32, + #[inspect(skip)] + doorbells: Arc>, + #[inspect(skip)] + registered_waker: Option, } -impl ShadowDoorbell { - // See NVMe Spec version 2.0a, Section 5.8 -- Doorbell Buffer Config Command for - // an explanation of this math. - pub fn new( - shadow_db_evt_idx_base: ShadowDoorbell, - qid: u16, - is_sq: bool, - doorbell_stride_bits: usize, - ) -> ShadowDoorbell { - let offset = match is_sq { - true => 0u64, - false => 1u64, - }; - let shadow_db_gpa = shadow_db_evt_idx_base.shadow_db_gpa - + (qid as u64 * 2 + offset) * (4 << (doorbell_stride_bits - 2)); - let event_idx_gpa = shadow_db_evt_idx_base.event_idx_gpa - + (qid as u64 * 2 + offset) * (4 << (doorbell_stride_bits - 2)); - ShadowDoorbell { - shadow_db_gpa, - event_idx_gpa, +impl DoorbellState { + fn inspect_shadow(&self, resp: &mut inspect::Response<'_>) { + resp.field_with("doorbell", || { + self.doorbells.read().read(self.db_id).map(inspect::AsHex) + }) + .field_with("shadow_event_idx", || { + self.doorbells + .read() + .read_event_idx(self.db_id) + .map(inspect::AsHex) + }); + } + + fn new(doorbells: Arc>, db_id: u16, len: u32) -> Self { + Self { + current: 0, + event_idx: 0, + len, + doorbells, + registered_waker: None, + db_id, + db_offset: (db_id as u64) << DOORBELL_STRIDE_BITS, + } + } + + fn probe_inner(&mut self, update_event_idx: bool) -> Option { + // Try to read forward. + let doorbell = self.doorbells.read(); + let val = doorbell.read(self.db_id)?; + if val != self.current { + return Some(val); + } + + if self.event_idx == val || !update_event_idx || !doorbell.has_event_idx() { + return None; + } + + // Update the event index so that the guest will write the real doorbell + // on the next update. + doorbell.write_event_idx(self.db_id, val); + self.event_idx = val; + + // Double check after a memory barrier. + std::sync::atomic::fence(Ordering::SeqCst); + let val = doorbell.read(self.db_id)?; + if val != self.current { Some(val) } else { None } + } + + fn probe(&mut self, update_event_idx: bool) -> Result { + // If shadow doorbells are in use, use that instead of what was written to the doorbell + // register, as it may be more current. + if let Some(val) = self.probe_inner(update_event_idx) { + if val >= self.len { + return Err(QueueError::InvalidDoorbell { val, len: self.len }); + } + self.current = val; + Ok(true) + } else { + Ok(false) } } + + fn poll(&mut self, cx: &mut Context<'_>) -> Poll> { + // Ensure we get woken up whenever the doorbell is written to. + if self + .registered_waker + .as_ref() + .is_none_or(|w| !cx.waker().will_wake(w)) + { + let _old_waker = + self.doorbells.write().wakers[self.db_id as usize].replace(cx.waker().clone()); + self.registered_waker = Some(cx.waker().clone()); + } + if !self.probe(true)? { + return Poll::Pending; + } + Poll::Ready(Ok(())) + } } #[derive(Inspect)] pub struct SubmissionQueue { - #[inspect(hex)] - cached_tail: u32, - tail: Arc, + tail: DoorbellState, + mem: GuestMemory, #[inspect(hex)] head: u32, #[inspect(hex)] gpa: u64, - #[inspect(hex)] - len: u32, - #[inspect(with = "Option::is_some")] - shadow_db_evt_idx: Option, - #[inspect(hex)] - evt_idx: u32, } #[derive(Debug, Error)] pub enum QueueError { - #[error("invalid doorbell tail {0:#x}")] - InvalidTail(u32), - #[error("invalid doorbell head {0:#x}")] - InvalidHead(u32), + #[error("invalid doorbell value {val:#x}, len {len:#x}")] + InvalidDoorbell { val: u32, len: u32 }, #[error("queue access error")] Memory(#[source] GuestMemoryError), } impl SubmissionQueue { - pub fn new( - tail: Arc, - gpa: u64, - len: u16, - shadow_db_evt_idx: Option, - ) -> Self { - tail.write(0); + pub fn new(cq: &CompletionQueue, db_id: u16, gpa: u64, len: u16) -> Self { + let doorbells = cq.head.doorbells.clone(); + let mem = cq.mem.clone(); + doorbells.read().write(db_id, 0); Self { - cached_tail: 0, - tail, + tail: DoorbellState::new(doorbells, db_id, len.into()), head: 0, gpa, - len: len.into(), - shadow_db_evt_idx, - evt_idx: 0, + mem, } } /// This function returns a future for the next entry in the submission queue. It also /// has a side effect of updating the tail. - /// - /// Note that this function returns a future that must be cancellable, which means that the - /// parts after an await may never run. The tail update side effect is benign, so - /// that can happen before the await. - pub async fn next(&mut self, mem: &GuestMemory) -> Result { - // If shadow doorbells are in use, use that instead of what was written to the doorbell - // register, as it may be more current. - if let Some(shadow_db_evt_idx) = self.shadow_db_evt_idx { - let shadow_tail = mem - .read_plain(shadow_db_evt_idx.shadow_db_gpa) - .map_err(QueueError::Memory)?; - - // ILLEGAL_DOORBELL_VALUE is the initial state. The guest will overwrite - // it when it first uses the shadow. - if shadow_tail != ILLEGAL_DOORBELL_VALUE { - self.cached_tail = shadow_tail; - self.tail.write(self.cached_tail); - } - } - while self.cached_tail == self.head { - self.cached_tail = self.tail.wait_read(self.cached_tail).await; + pub fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll> { + let tail = self.tail.current; + if tail == self.head { + ready!(self.tail.poll(cx))?; } - if self.cached_tail >= self.len { - return Err(QueueError::InvalidTail(self.cached_tail)); - } - let command: spec::Command = mem - .read_plain(self.gpa.wrapping_add(self.head as u64 * 64)) + let command: spec::Command = self + .mem + .read_plain( + self.gpa + .wrapping_add(self.head as u64 * size_of::() as u64), + ) .map_err(QueueError::Memory)?; - self.head = advance(self.head, self.len); - Ok(command) + self.head = advance(self.head, self.tail.len); + Poll::Ready(Ok(command)) } pub fn sqhd(&self) -> u16 { self.head as u16 } - - /// This function lets the driver know what doorbell value we consumed, allowing - /// it to elide the next ring, maybe. - pub fn advance_evt_idx(&mut self, mem: &GuestMemory) -> Result<(), QueueError> { - self.evt_idx = advance(self.evt_idx, self.len); - if let Some(shadow_db_evt_idx) = self.shadow_db_evt_idx { - mem.write_plain(shadow_db_evt_idx.event_idx_gpa, &self.evt_idx) - .map_err(QueueError::Memory)?; - } - Ok(()) - } - - /// This function updates the shadow doorbell values of a queue that is - /// potentially already in use. - pub fn update_shadow_db(&mut self, mem: &GuestMemory, sdb: ShadowDoorbell) { - self.shadow_db_evt_idx = Some(sdb); - self.evt_idx = self.cached_tail; - // Write the illegal value out to the buffer, so that we can tell - // if Linux has ever written a valid value. - let _ = mem.write_plain(sdb.shadow_db_gpa, &ILLEGAL_DOORBELL_VALUE); - } } #[derive(Inspect)] pub struct CompletionQueue { #[inspect(hex)] tail: u32, - #[inspect(hex)] - cached_head: u32, - head: Arc, + head: DoorbellState, phase: bool, #[inspect(hex)] gpa: u64, - #[inspect(hex)] - len: u32, #[inspect(with = "Option::is_some")] interrupt: Option, - shadow_db_evt_idx: Option, + mem: GuestMemory, } impl CompletionQueue { pub fn new( - head: Arc, + doorbells: Arc>, + db_id: u16, + mem: GuestMemory, interrupt: Option, gpa: u64, len: u16, - shadow_db_evt_idx: Option, ) -> Self { - head.write(0); + doorbells.read().write(db_id, 0); Self { tail: 0, - cached_head: 0, - head, + head: DoorbellState::new(doorbells, db_id, len.into()), phase: true, gpa, - len: len.into(), interrupt, - shadow_db_evt_idx, + mem, } } /// Wait for free completions. - pub async fn wait_ready(&mut self, mem: &GuestMemory) -> Result<(), QueueError> { - let next_tail = advance(self.tail, self.len); - // If shadow doorbells are in use, use that instead of what was written to the doorbell - // register, as it may be more current. - if let Some(shadow_db_evt_idx) = self.shadow_db_evt_idx { - let shadow_head = mem - .read_plain(shadow_db_evt_idx.shadow_db_gpa) - .map_err(QueueError::Memory)?; - - // ILLEGAL_DOORBELL_VALUE is the initial state. The guest will overwrite - // it when it first uses the shadow. - if shadow_head != ILLEGAL_DOORBELL_VALUE { - self.cached_head = shadow_head; - self.head.write(self.cached_head); - } - } - while self.cached_head == next_tail { - self.cached_head = self.head.wait_read(self.cached_head).await; + pub fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + let next_tail = advance(self.tail, self.head.len); + if self.head.current == next_tail { + ready!(self.head.poll(cx))?; } - if self.cached_head >= self.len { - return Err(QueueError::InvalidHead(self.cached_head)); - } - Ok(()) + Poll::Ready(Ok(())) } - pub fn write( - &mut self, - mem: &GuestMemory, - mut data: spec::Completion, - ) -> Result { - if self.cached_head == advance(self.tail, self.len) { + pub fn write(&mut self, mut data: spec::Completion) -> Result { + let next = advance(self.tail, self.head.len); + // Check the doorbell register instead of requiring the caller to + // go around the slow path and call `poll_ready`. + if self.head.current == next && !self.head.probe(false)? { return Ok(false); } data.status.set_phase(self.phase); @@ -273,52 +345,27 @@ impl CompletionQueue { // This is necessary to ensure the guest can observe the full completion // once it observes the phase bit change (which is in the high part). let [low, high]: [u64; 2] = zerocopy::transmute!(data); - let gpa = self.gpa.wrapping_add(self.tail as u64 * 16); - mem.write_plain(gpa, &low).map_err(QueueError::Memory)?; + let gpa = self + .gpa + .wrapping_add(self.tail as u64 * size_of::() as u64); + self.mem + .write_plain(gpa, &low) + .map_err(QueueError::Memory)?; std::sync::atomic::fence(Ordering::Release); - mem.write_plain(gpa + 8, &high) + self.mem + .write_plain(gpa + 8, &high) .map_err(QueueError::Memory)?; std::sync::atomic::fence(Ordering::Release); if let Some(interrupt) = &self.interrupt { interrupt.deliver(); } - self.tail = advance(self.tail, self.len); + self.tail = next; if self.tail == 0 { self.phase = !self.phase; } Ok(true) } - - /// This method updates the EVT_IDX field to match the shadow doorbell - /// value, thus signalling to the guest driver that the next completion - /// removed should involve a doorbell ring. In this emulator, such - /// a thing (the ring) is only necessary when the number of un-spoken-for - /// completion queue entries is getting small. (Completion queue entries - /// are spoken for when a command is removed from the SQ). - pub fn catch_up_evt_idx( - &mut self, - force: bool, - io_outstanding: u32, - mem: &GuestMemory, - ) -> Result<(), QueueError> { - if let Some(shadow_db_evt_idx) = self.shadow_db_evt_idx { - if force | (io_outstanding >= self.len - 3) { - mem.write_plain(shadow_db_evt_idx.event_idx_gpa, &self.cached_head) - .map_err(QueueError::Memory)?; - } - } - Ok(()) - } - - /// This function updates the shadow doorbell values of a queue that is - /// potentially already in use. - pub fn update_shadow_db(&mut self, mem: &GuestMemory, sdb: ShadowDoorbell) { - self.shadow_db_evt_idx = Some(sdb); - // Write the illegal value out to the buffer, so that we can tell - // if Linux has ever written a valid value. - let _ = mem.write_plain(sdb.shadow_db_gpa, &ILLEGAL_DOORBELL_VALUE); - } } fn advance(n: u32, l: u32) -> u32 { diff --git a/vm/devices/storage/nvme_test/src/workers/admin.rs b/vm/devices/storage/nvme_test/src/workers/admin.rs index ed95821b22..a8d1aba83c 100644 --- a/vm/devices/storage/nvme_test/src/workers/admin.rs +++ b/vm/devices/storage/nvme_test/src/workers/admin.rs @@ -19,9 +19,8 @@ use crate::error::NvmeError; use crate::namespace::Namespace; use crate::prp::PrpRange; use crate::queue::CompletionQueue; -use crate::queue::DoorbellRegister; +use crate::queue::DoorbellMemory; use crate::queue::QueueError; -use crate::queue::ShadowDoorbell; use crate::queue::SubmissionQueue; use crate::spec; use disk_backend::Disk; @@ -39,9 +38,11 @@ use pal_async::task::Spawn; use pal_async::task::Task; use pal_async::timer::PolledTimer; use parking_lot::Mutex; +use parking_lot::RwLock; use std::collections::BTreeMap; use std::collections::btree_map; use std::future::pending; +use std::future::poll_fn; use std::io::Cursor; use std::io::Write; use std::sync::Arc; @@ -72,7 +73,7 @@ pub struct AdminConfig { #[inspect(skip)] pub interrupts: Vec, #[inspect(skip)] - pub doorbells: Vec>, + pub doorbells: Arc>, #[inspect(display)] pub subsystem_id: Guid, pub max_sqs: u16, @@ -102,8 +103,6 @@ pub struct AdminState { io_cqs: Vec>, #[inspect(skip)] sq_delete_response: mesh::Receiver, - #[inspect(with = "Option::is_some")] - shadow_db_evt_gpa_base: Option, #[inspect(iter_by_index)] asynchronous_event_requests: Vec, #[inspect( @@ -132,7 +131,6 @@ struct IoSq { driver: VmTaskDriver, pending_delete_cid: Option, cqid: Option, - shadow_db_evt_idx: Option, } #[derive(Inspect)] @@ -143,7 +141,6 @@ struct IoCq { len: u16, interrupt: Option, sqid: Option, - shadow_db_evt_idx: Option, } impl AdminState { @@ -168,19 +165,21 @@ impl AdminState { }) .collect(); + let admin_cq = CompletionQueue::new( + handler.config.doorbells.clone(), + 1, + handler.config.mem.clone(), + Some(handler.config.interrupts[0].clone()), + acq, + acqs, + ); + let mut state = Self { - admin_sq: SubmissionQueue::new(handler.config.doorbells[0].clone(), asq, asqs, None), - admin_cq: CompletionQueue::new( - handler.config.doorbells[1].clone(), - Some(handler.config.interrupts[0].clone()), - acq, - acqs, - None, - ), + admin_sq: SubmissionQueue::new(&admin_cq, 0, asq, asqs), + admin_cq, io_sqs: Vec::new(), io_cqs: Vec::new(), sq_delete_response: Default::default(), - shadow_db_evt_gpa_base: None, asynchronous_event_requests: Vec::new(), changed_namespaces: Vec::new(), notified_changed_namespaces: false, @@ -207,9 +206,6 @@ impl AdminState { /// Caller must ensure that no queues are active. fn set_max_queues(&mut self, handler: &AdminHandler, num_sqs: u16, num_cqs: u16) { - let num_qids = 2 + num_sqs.max(num_cqs) * 2; - assert!(handler.config.doorbells.len() >= num_qids as usize); - self.io_sqs.truncate(num_sqs.into()); self.io_sqs .extend((self.io_sqs.len()..num_sqs.into()).map(|i| { @@ -233,7 +229,6 @@ impl AdminState { )), pending_delete_cid: None, cqid: None, - shadow_db_evt_idx: None, driver, } })); @@ -410,12 +405,11 @@ impl AdminHandler { let event = loop { // Wait for there to be room for a completion for the next // command or the completed sq deletion. - state.admin_cq.wait_ready(&self.config.mem).await?; + poll_fn(|cx| state.admin_cq.poll_ready(cx)).await?; if !state.changed_namespaces.is_empty() && !state.notified_changed_namespaces { if let Some(cid) = state.asynchronous_event_requests.pop() { state.admin_cq.write( - &self.config.mem, spec::Completion { dw0: spec::AsynchronousEventRequestDw0::new() .with_event_type(spec::AsynchronousEventType::NOTICE.0) @@ -435,7 +429,7 @@ impl AdminHandler { } } - let next_command = state.admin_sq.next(&self.config.mem).map(Event::Command); + let next_command = poll_fn(|cx| state.admin_sq.poll_next(cx)).map(Event::Command); let sq_delete_complete = async { let Some(sqid) = state.sq_delete_response.next().await else { pending().await @@ -461,10 +455,6 @@ impl AdminHandler { state: &mut AdminState, event: Result, ) -> Result<(), QueueError> { - // For the admin queue, update Evt_IDX at the beginning of command - // processing, just to keep it simple. - state.admin_sq.advance_evt_idx(&self.config.mem)?; - let (command_processed, cid, result) = match event? { Event::Command(command) => { let mut command = command?; @@ -516,7 +506,7 @@ impl AdminHandler { let result = match opcode { spec::AdminOpcode::IDENTIFY => self - .handle_identify(&command) + .handle_identify(state, &command) .map(|()| Some(Default::default())), spec::AdminOpcode::GET_FEATURES => { self.handle_get_features(state, &command).await.map(Some) @@ -543,9 +533,13 @@ impl AdminHandler { spec::AdminOpcode::GET_LOG_PAGE => self .handle_get_log_page(state, &command) .map(|()| Some(Default::default())), - spec::AdminOpcode::DOORBELL_BUFFER_CONFIG => self - .handle_doorbell_buffer_config(state, &command) - .map(|()| Some(Default::default())), + spec::AdminOpcode::DOORBELL_BUFFER_CONFIG + if self.supports_shadow_doorbells(state) => + { + self.handle_doorbell_buffer_config(state, &command) + .await + .map(|()| Some(Default::default())) + } opcode => { tracelimit::warn_ratelimited!(?opcode, "unsupported opcode"); Err(spec::Status::INVALID_COMMAND_OPCODE.into()) @@ -657,13 +651,15 @@ impl AdminHandler { } } - state.admin_cq.write(&self.config.mem, completion)?; - // Again, for simplicity, update EVT_IDX here. - state.admin_cq.catch_up_evt_idx(true, 0, &self.config.mem)?; + state.admin_cq.write(completion)?; Ok(()) } - fn handle_identify(&mut self, command: &spec::Command) -> Result<(), NvmeError> { + fn handle_identify( + &mut self, + state: &AdminState, + command: &spec::Command, + ) -> Result<(), NvmeError> { let cdw10: spec::Cdw10Identify = command.cdw10.into(); // All identify results are 4096 bytes. let mut buf = [0u64; 512]; @@ -671,7 +667,7 @@ impl AdminHandler { match spec::Cns(cdw10.cns()) { spec::Cns::CONTROLLER => { let id = spec::IdentifyController::mut_from_prefix(buf).unwrap().0; // TODO: zerocopy: from-prefix (mut_from_prefix): use-rest-of-range (https://github.com/microsoft/openvmm/issues/759) - *id = self.identify_controller(); + *id = self.identify_controller(state); write!( Cursor::new(&mut id.subnqn[..]), @@ -717,8 +713,7 @@ impl AdminHandler { Ok(()) } - fn identify_controller(&self) -> spec::IdentifyController { - let oacs = spec::OptionalAdminCommandSupport::from(0).with_doorbell_buffer_config(true); + fn identify_controller(&self, state: &AdminState) -> spec::IdentifyController { spec::IdentifyController { vid: VENDOR_ID, ssvid: VENDOR_ID, @@ -749,7 +744,8 @@ impl AdminHandler { .with_present(true) .with_broadcast_flush_behavior(spec::BroadcastFlushBehavior::NOT_SUPPORTED.0), cntrltype: spec::ControllerType::IO_CONTROLLER, - oacs, + oacs: spec::OptionalAdminCommandSupport::new() + .with_doorbell_buffer_config(self.supports_shadow_doorbells(state)), ..FromZeros::new_zeroed() } } @@ -880,22 +876,11 @@ impl AdminHandler { return Err(spec::Status::INVALID_QUEUE_SIZE.into()); } - let mut shadow_db_evt_idx: Option = None; - if let Some(shadow_db_evt_gpa_base) = state.shadow_db_evt_gpa_base { - shadow_db_evt_idx = Some(ShadowDoorbell::new( - shadow_db_evt_gpa_base, - cqid, - false, - DOORBELL_STRIDE_BITS.into(), - )); - } - *io_queue = Some(IoCq { gpa, len: len0 + 1, interrupt, sqid: None, - shadow_db_evt_idx, }); Ok(()) } @@ -948,19 +933,8 @@ impl AdminHandler { return Err(spec::Status::INVALID_QUEUE_SIZE.into()); } - if let Some(shadow_db_evt_gpa_base) = state.shadow_db_evt_gpa_base { - sq.shadow_db_evt_idx = Some(ShadowDoorbell::new( - shadow_db_evt_gpa_base, - sqid, - true, - DOORBELL_STRIDE_BITS.into(), - )); - } - cq.sqid = Some(sqid); sq.cqid = Some(cqid); - let sq_tail = self.config.doorbells[sqid as usize * 2].clone(); - let cq_head = self.config.doorbells[cqid as usize * 2 + 1].clone(); let interrupt = cq .interrupt .map(|iv| self.config.interrupts[iv as usize].clone()); @@ -969,14 +943,14 @@ impl AdminHandler { let cq_gpa = cq.gpa; let cq_len = cq.len; let state = IoState::new( + &self.config.mem, + self.config.doorbells.clone(), sq_gpa, sq_len, - sq_tail, - sq.shadow_db_evt_idx, + sqid, cq_gpa, cq_len, - cq_head, - cq.shadow_db_evt_idx, + cqid, interrupt, namespaces, ); @@ -1122,57 +1096,33 @@ impl AdminHandler { Ok(()) } - fn handle_doorbell_buffer_config( + fn supports_shadow_doorbells(&self, state: &AdminState) -> bool { + let num_queues = state.io_sqs.len().max(state.io_cqs.len()) + 1; + let len = num_queues * (2 << DOORBELL_STRIDE_BITS); + // The spec only allows a single shadow doorbell page. + len <= PAGE_SIZE + } + + async fn handle_doorbell_buffer_config( &self, state: &mut AdminState, command: &spec::Command, ) -> Result<(), NvmeError> { + // Validated by caller. + assert!(self.supports_shadow_doorbells(state)); + let shadow_db_gpa = command.dptr[0]; let event_idx_gpa = command.dptr[1]; - - if (shadow_db_gpa == 0) - || (shadow_db_gpa & 0xfff != 0) - || (event_idx_gpa == 0) - || (event_idx_gpa & 0xfff != 0) - || (shadow_db_gpa == event_idx_gpa) - { - return Err(spec::Status::INVALID_FIELD_IN_COMMAND.into()); + if (shadow_db_gpa | event_idx_gpa) & !PAGE_MASK != 0 { + return Err(NvmeError::from(spec::Status::INVALID_FIELD_IN_COMMAND)); } - // Stash the base values for use in data queue creation. - let sdb_base = ShadowDoorbell { - shadow_db_gpa, - event_idx_gpa, - }; - state.shadow_db_evt_gpa_base = Some(sdb_base); - - // Update the admin queue to use shadow doorbells. - state.admin_sq.update_shadow_db( - &self.config.mem, - ShadowDoorbell::new(sdb_base, 0, true, DOORBELL_STRIDE_BITS.into()), - ); - state.admin_cq.update_shadow_db( - &self.config.mem, - ShadowDoorbell::new(sdb_base, 0, false, DOORBELL_STRIDE_BITS.into()), - ); + self.config + .doorbells + .write() + .replace_mem(self.config.mem.clone(), shadow_db_gpa, Some(event_idx_gpa)) + .map_err(|err| NvmeError::new(spec::Status::DATA_TRANSFER_ERROR, err))?; - // Update any data queues with the new shadow doorbell base. - for (qid, sq) in state.io_sqs.iter_mut().enumerate() { - if !sq.task.has_state() { - continue; - } - let gm = self.config.mem.clone(); - - // Data queue pairs are qid + 1, because the admin queue isn't in this vector. - let sq_sdb = - ShadowDoorbell::new(sdb_base, qid as u16 + 1, true, DOORBELL_STRIDE_BITS.into()); - let cq_sdb = - ShadowDoorbell::new(sdb_base, qid as u16 + 1, false, DOORBELL_STRIDE_BITS.into()); - - sq.task.update_with(move |sq, sq_state| { - sq.update_shadow_db(&gm, sq_state.unwrap(), sq_sdb, cq_sdb); - }); - } Ok(()) } diff --git a/vm/devices/storage/nvme_test/src/workers/coordinator.rs b/vm/devices/storage/nvme_test/src/workers/coordinator.rs index 7016857eab..b5c8542876 100644 --- a/vm/devices/storage/nvme_test/src/workers/coordinator.rs +++ b/vm/devices/storage/nvme_test/src/workers/coordinator.rs @@ -8,7 +8,8 @@ use super::admin::AdminConfig; use super::admin::AdminHandler; use super::admin::AdminState; use super::admin::NsidConflict; -use crate::queue::DoorbellRegister; +use crate::queue::DoorbellMemory; +use crate::queue::InvalidDoorbell; use disk_backend::Disk; use futures::FutureExt; use futures::StreamExt; @@ -23,7 +24,7 @@ use mesh::rpc::RpcSend; use nvme_resources::fault::FaultConfiguration; use pal_async::task::Spawn; use pal_async::task::Task; -use parking_lot::Mutex; +use parking_lot::RwLock; use std::future::pending; use std::sync::Arc; use task_control::TaskControl; @@ -38,7 +39,7 @@ pub struct NvmeWorkersContext<'a> { pub interrupts: Vec, pub max_sqs: u16, pub max_cqs: u16, - pub qe_sizes: Arc>, + pub qe_sizes: Arc>, pub subsystem_id: Guid, pub fault_configuration: FaultConfiguration, } @@ -46,7 +47,7 @@ pub struct NvmeWorkersContext<'a> { pub struct NvmeWorkers { _task: Task<()>, send: mesh::Sender, - doorbells: Vec>, + doorbells: Arc>, state: EnableState, } @@ -78,9 +79,7 @@ impl NvmeWorkers { } = context; let num_qids = 2 + max_sqs.max(max_cqs) * 2; - let doorbells: Vec<_> = (0..num_qids) - .map(|_| Arc::new(DoorbellRegister::new())) - .collect(); + let doorbells = Arc::new(RwLock::new(DoorbellMemory::new(num_qids))); let driver = driver_source.simple(); let handler: AdminHandler = AdminHandler::new( @@ -118,11 +117,9 @@ impl NvmeWorkers { } } - pub fn doorbell(&self, index: u16, value: u32) { - if let Some(doorbell) = self.doorbells.get(index as usize) { - doorbell.write(value); - } else { - tracelimit::warn_ratelimited!(index, value, "unknown doorbell"); + pub fn doorbell(&self, db_id: u16, value: u32) { + if let Err(InvalidDoorbell) = self.doorbells.read().try_write(db_id, value) { + tracelimit::error_ratelimited!(db_id, "write to invalid doorbell index"); } } diff --git a/vm/devices/storage/nvme_test/src/workers/io.rs b/vm/devices/storage/nvme_test/src/workers/io.rs index 3247889fe2..2cbae195e1 100644 --- a/vm/devices/storage/nvme_test/src/workers/io.rs +++ b/vm/devices/storage/nvme_test/src/workers/io.rs @@ -7,9 +7,8 @@ use crate::error::CommandResult; use crate::error::NvmeError; use crate::namespace::Namespace; use crate::queue::CompletionQueue; -use crate::queue::DoorbellRegister; +use crate::queue::DoorbellMemory; use crate::queue::QueueError; -use crate::queue::ShadowDoorbell; use crate::queue::SubmissionQueue; use crate::spec; use crate::spec::nvm; @@ -17,9 +16,11 @@ use crate::workers::MAX_DATA_TRANSFER_SIZE; use futures_concurrency::future::Race; use guestmem::GuestMemory; use inspect::Inspect; +use parking_lot::RwLock; use std::collections::BTreeMap; use std::future::Future; use std::future::pending; +use std::future::poll_fn; use std::pin::Pin; use std::sync::Arc; use task_control::AsyncRun; @@ -59,20 +60,39 @@ enum IoQueueState { impl IoState { pub fn new( + mem: &GuestMemory, + doorbell: Arc>, sq_gpa: u64, sq_len: u16, - sq_tail: Arc, - sq_sdb_idx_gpas: Option, + sq_id: u16, cq_gpa: u64, cq_len: u16, - cq_head: Arc, - cq_sdb_idx_gpas: Option, + cq_id: u16, interrupt: Option, namespaces: BTreeMap>, ) -> Self { Self { - sq: SubmissionQueue::new(sq_tail, sq_gpa, sq_len, sq_sdb_idx_gpas), - cq: CompletionQueue::new(cq_head, interrupt, cq_gpa, cq_len, cq_sdb_idx_gpas), + sq: SubmissionQueue::new( + &CompletionQueue::new( + doorbell.clone(), + cq_id * 2 + 1, + mem.clone(), + interrupt.clone(), + cq_gpa, + cq_len, + ), + sq_id * 2, + sq_gpa, + sq_len, + ), + cq: CompletionQueue::new( + doorbell, + cq_id * 2 + 1, + mem.clone(), + interrupt, + cq_gpa, + cq_len, + ), namespaces, ios: FuturesUnordered::new(), io_count: 0, @@ -103,14 +123,12 @@ struct IoResult { cid: u16, opcode: nvm::NvmOpcode, result: Result, - advance_evt_idx: bool, } impl AsyncRun for IoHandler { async fn run(&mut self, stop: &mut StopTask<'_>, state: &mut IoState) -> Result<(), Cancelled> { - let mem = self.mem.clone(); stop.until_stopped(async { - if let Err(err) = self.process(state, &mem).await { + if let Err(err) = self.process(state).await { tracing::error!(error = &err as &dyn std::error::Error, "io handler failed"); } }) @@ -148,11 +166,7 @@ impl IoHandler { } } - async fn process( - &mut self, - state: &mut IoState, - mem: &GuestMemory, - ) -> Result<(), HandlerError> { + async fn process(&mut self, state: &mut IoState) -> Result<(), HandlerError> { loop { let deleting = match state.queue_state { IoQueueState::Active => { @@ -160,7 +174,7 @@ impl IoHandler { // to post an immediate result or to post an IO completion. It's not // strictly necessary to start a new IO, but handling that special // case is not worth the complexity. - state.cq.wait_ready(mem).await?; + poll_fn(|cx| state.cq.poll_ready(cx)).await?; false } IoQueueState::Deleting => { @@ -181,7 +195,7 @@ impl IoHandler { let next_sqe = async { if state.io_count < MAX_IO_QUEUE_DEPTH && !deleting { - Event::Sq(state.sq.next(&self.mem).await) + Event::Sq(poll_fn(|cx| state.sq.poll_next(cx)).await) } else { pending().await } @@ -198,12 +212,6 @@ impl IoHandler { let event = (next_sqe, next_io_completion).race().await; let (cid, result) = match event { Event::Io(io_result) => { - if io_result.advance_evt_idx { - let result = state.sq.advance_evt_idx(&self.mem); - if result.is_err() { - tracelimit::warn_ratelimited!("failure to advance evt_idx"); - } - } state.io_count -= 1; let result = match io_result.result { Ok(cr) => cr, @@ -226,21 +234,6 @@ impl IoHandler { if let Some(ns) = state.namespaces.get(&command.nsid) { let ns = ns.clone(); - // If the queue depth is low, immediately update the evt_idx, so that - // the guest driver will ring the doorbell again. If the queue depth is - // high, defer this until I/O completion, on the theory that high queue - // depth workloads won't wait before enqueuing more work. - // - // TODO: Update later after performance testing, perhaps to something - // like to 2*(number of VPs)/(number of queue pairs). - let mut advance_evt_idx = true; - if state.io_count <= 1 { - let result = state.sq.advance_evt_idx(&self.mem); - if result.is_err() { - tracelimit::warn_ratelimited!("failure to advance evt_idx"); - } - advance_evt_idx = false; - } let io = Box::pin(async move { let result = ns.nvm_command(MAX_DATA_TRANSFER_SIZE, &command).await; IoResult { @@ -248,7 +241,6 @@ impl IoHandler { opcode: nvm::NvmOpcode(command.cdw0.opcode()), cid, result, - advance_evt_idx, } }); state.ios.push(io); @@ -256,10 +248,6 @@ impl IoHandler { continue; } - let result = state.sq.advance_evt_idx(&self.mem); - if result.is_err() { - tracelimit::warn_ratelimited!("failure to advance evt_idx"); - } (cid, spec::Status::INVALID_NAMESPACE_OR_FORMAT.into()) } }; @@ -272,25 +260,11 @@ impl IoHandler { cid, status: spec::CompletionStatus::new().with_status(result.status.0), }; - if !state.cq.write(&self.mem, completion)? { + if !state.cq.write(completion)? { assert!(deleting); tracelimit::warn_ratelimited!("dropped i/o completion during queue deletion"); } - state - .cq - .catch_up_evt_idx(false, state.io_count as u32, &self.mem)?; } Ok(()) } - - pub fn update_shadow_db( - &mut self, - mem: &GuestMemory, - state: &mut IoState, - sq_sdb: ShadowDoorbell, - cq_sdb: ShadowDoorbell, - ) { - state.sq.update_shadow_db(mem, sq_sdb); - state.cq.update_shadow_db(mem, cq_sdb); - } } From c72910303f5c799996b87ee1c5901091a54d2d85 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Sep 2025 21:02:47 +0000 Subject: [PATCH 3/5] Fix SubmissionQueue::new signature to take doorbells parameter directly Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com> --- vm/devices/storage/nvme_test/src/queue.rs | 10 +++++++--- vm/devices/storage/nvme_test/src/workers/admin.rs | 8 +++++++- vm/devices/storage/nvme_test/src/workers/io.rs | 14 +------------- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/vm/devices/storage/nvme_test/src/queue.rs b/vm/devices/storage/nvme_test/src/queue.rs index 7b46ae9266..5ac0d3b8b5 100644 --- a/vm/devices/storage/nvme_test/src/queue.rs +++ b/vm/devices/storage/nvme_test/src/queue.rs @@ -252,9 +252,13 @@ pub enum QueueError { } impl SubmissionQueue { - pub fn new(cq: &CompletionQueue, db_id: u16, gpa: u64, len: u16) -> Self { - let doorbells = cq.head.doorbells.clone(); - let mem = cq.mem.clone(); + pub fn new( + doorbells: Arc>, + db_id: u16, + gpa: u64, + len: u16, + mem: GuestMemory, + ) -> Self { doorbells.read().write(db_id, 0); Self { tail: DoorbellState::new(doorbells, db_id, len.into()), diff --git a/vm/devices/storage/nvme_test/src/workers/admin.rs b/vm/devices/storage/nvme_test/src/workers/admin.rs index a8d1aba83c..ab704c7e1d 100644 --- a/vm/devices/storage/nvme_test/src/workers/admin.rs +++ b/vm/devices/storage/nvme_test/src/workers/admin.rs @@ -175,7 +175,13 @@ impl AdminState { ); let mut state = Self { - admin_sq: SubmissionQueue::new(&admin_cq, 0, asq, asqs), + admin_sq: SubmissionQueue::new( + handler.config.doorbells.clone(), + 0, + asq, + asqs, + handler.config.mem.clone(), + ), admin_cq, io_sqs: Vec::new(), io_cqs: Vec::new(), diff --git a/vm/devices/storage/nvme_test/src/workers/io.rs b/vm/devices/storage/nvme_test/src/workers/io.rs index 2cbae195e1..03a3f8c944 100644 --- a/vm/devices/storage/nvme_test/src/workers/io.rs +++ b/vm/devices/storage/nvme_test/src/workers/io.rs @@ -72,19 +72,7 @@ impl IoState { namespaces: BTreeMap>, ) -> Self { Self { - sq: SubmissionQueue::new( - &CompletionQueue::new( - doorbell.clone(), - cq_id * 2 + 1, - mem.clone(), - interrupt.clone(), - cq_gpa, - cq_len, - ), - sq_id * 2, - sq_gpa, - sq_len, - ), + sq: SubmissionQueue::new(doorbell.clone(), sq_id * 2, sq_gpa, sq_len, mem.clone()), cq: CompletionQueue::new( doorbell, cq_id * 2 + 1, From 4d8ec6f4df3e1fa09152adb14919b849bfb3bfb0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Sep 2025 21:18:05 +0000 Subject: [PATCH 4/5] Move CompletionQueue::new() inside Self{} creation for consistency Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com> --- .../storage/nvme_test/src/workers/admin.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/vm/devices/storage/nvme_test/src/workers/admin.rs b/vm/devices/storage/nvme_test/src/workers/admin.rs index ab704c7e1d..1ba6959f35 100644 --- a/vm/devices/storage/nvme_test/src/workers/admin.rs +++ b/vm/devices/storage/nvme_test/src/workers/admin.rs @@ -165,15 +165,6 @@ impl AdminState { }) .collect(); - let admin_cq = CompletionQueue::new( - handler.config.doorbells.clone(), - 1, - handler.config.mem.clone(), - Some(handler.config.interrupts[0].clone()), - acq, - acqs, - ); - let mut state = Self { admin_sq: SubmissionQueue::new( handler.config.doorbells.clone(), @@ -182,7 +173,14 @@ impl AdminState { asqs, handler.config.mem.clone(), ), - admin_cq, + admin_cq: CompletionQueue::new( + handler.config.doorbells.clone(), + 1, + handler.config.mem.clone(), + Some(handler.config.interrupts[0].clone()), + acq, + acqs, + ), io_sqs: Vec::new(), io_cqs: Vec::new(), sq_delete_response: Default::default(), From 06f64c65b8c5b6b87deaa1cdd5fe709f0a6e4aba Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Sep 2025 21:33:03 +0000 Subject: [PATCH 5/5] Update nvme_test shadow doorbell tests to match new DoorbellMemory API Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com> --- .../src/tests/shadow_doorbell_tests.rs | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/vm/devices/storage/nvme_test/src/tests/shadow_doorbell_tests.rs b/vm/devices/storage/nvme_test/src/tests/shadow_doorbell_tests.rs index 7bd6e478be..a4bf449623 100644 --- a/vm/devices/storage/nvme_test/src/tests/shadow_doorbell_tests.rs +++ b/vm/devices/storage/nvme_test/src/tests/shadow_doorbell_tests.rs @@ -1,10 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::DOORBELL_STRIDE_BITS; use crate::PAGE_SIZE64; use crate::prp::PrpRange; -use crate::queue::ShadowDoorbell; use crate::spec; use crate::tests::controller_tests::instantiate_and_build_admin_queue; use crate::tests::controller_tests::wait_for_msi; @@ -170,22 +168,19 @@ async fn test_setup_sq_ring_with_shadow(driver: DefaultDriver) { let sq_buf = PrpRange::new(vec![SQ_BASE], 0, PAGE_SIZE64).unwrap(); let gm = test_memory(); let int_controller = TestPciInterruptController::new(); - let sdb_base = ShadowDoorbell { - shadow_db_gpa: DOORBELL_BUFFER_BASE, - event_idx_gpa: EVT_IDX_BUFFER_BASE, - }; - let sq_sdb = ShadowDoorbell::new(sdb_base, 0, true, DOORBELL_STRIDE_BITS.into()); let mut backoff = Backoff::new(&driver); // Check that the old value was 0, just to be sure. - let sdb = gm.read_plain::(sq_sdb.shadow_db_gpa).unwrap(); + let sdb = gm.read_plain::(DOORBELL_BUFFER_BASE).unwrap(); assert_eq!(sdb, 0); let mut nvmec = setup_shadow_doorbells(driver.clone(), &cq_buf, &sq_buf, &gm, &int_controller, None).await; - let sdb = gm.read_plain::(sq_sdb.shadow_db_gpa).unwrap(); - assert_eq!(sdb, crate::queue::ILLEGAL_DOORBELL_VALUE); + let sdb = gm.read_plain::(DOORBELL_BUFFER_BASE).unwrap(); + // The doorbell value should be current (one admin queue command has been + // issued). + assert_eq!(sdb, 1); /* From the NVMe Spec (ver. 2.0a): B.5 Updating Controller Doorbell Properties using a Shadow Doorbell Buffer @@ -231,15 +226,16 @@ async fn test_setup_sq_ring_with_shadow(driver: DefaultDriver) { let new_sq_db = 2u32; // Update the shadow. - gm.write_plain::(sq_sdb.shadow_db_gpa, &new_sq_db) + gm.write_plain::(DOORBELL_BUFFER_BASE, &new_sq_db) .unwrap(); // Ring the admin queue doorbell. nvmec.write_bar0(0x1000, new_sq_db.as_bytes()).unwrap(); backoff.back_off().await; - let sq_evt_idx = gm.read_plain::(sq_sdb.event_idx_gpa).unwrap(); - assert_eq!(sq_evt_idx, 2); + // The current implementation will advance the event index immediately. + let sq_evt_idx = gm.read_plain::(EVT_IDX_BUFFER_BASE).unwrap(); + assert_eq!(sq_evt_idx, new_sq_db); } #[async_test]