Skip to content

nvme: drop shadow doorbell registration on controller reset - #4080

Closed
smalis-msft wants to merge 1 commit into
microsoft:mainfrom
smalis-msft:nvme-doorbell-reset
Closed

nvme: drop shadow doorbell registration on controller reset#4080
smalis-msft wants to merge 1 commit into
microsoft:mainfrom
smalis-msft:nvme-doorbell-reset

Conversation

@smalis-msft

Copy link
Copy Markdown
Contributor

Problem

multiarch::pcie::openvmm_linux_x64_pcie_hotplug intermittently times out
(CI run 30466297922). After the test's reboot, the guest hits a
BUG: kernel NULL pointer dereference in nvme_probe and never reaches
init, so pipette never reconnects and petri times out after 10 minutes.

Root cause

DoorbellMemory is shared out of NvmeWorkers via an Arc<RwLock<..>> and
was never restored on reset. Once the guest issued Doorbell Buffer Config,
replace_mem repointed it at guest pages and nothing ever pointed it back —
so after a reboot the emulator kept reading and writing the previous boot's
DBBUF pages. This violates NVMe Base 5.8, which ends the lifetime of the
Shadow Doorbell and EventIdx buffers at a Controller Level Reset.

The corruption is deterministic: on the next CC.EN 0→1,
SubmissionQueue::new (db_id 0) and CompletionQueue::new (db_id 1) each
write 0, which with a 4-byte doorbell stride zeroes the first 8 bytes of the
stale page — a full 64-bit NULL wherever the new kernel happened to place a
pointer. Whether that crashes the guest depends on what the page was reused
for, which is where the flakiness comes from.

The failing run's timeout_inspect_vmm.log shows the fingerprint directly:
the stale doorbell page reads [0, 0] while the stale EventIdx page still
holds boot 1's [11, 4], because the emulator only writes EventIdx when it
probes forward.

Fix

  • DoorbellMemory::reset() reallocates the controller-internal doorbell
    storage and clears event_idx_offset.
  • Called when a controller reset completes (poll_controller_reset) and at
    the end of a device-level reset. Both paths matter: Linux's reboot uses
    CC.SHN and leaves CC.EN set, so only the device reset runs on a guest
    reboot.
  • Mirrored in the nvme_test fault-injection copy of the emulator.

Testing

New test_shadow_doorbells_dropped_on_controller_reset verifies that a
sentinel written into the guest's DBBUF pages after a controller reset
survives a re-enable and a doorbell write. Confirmed to fail without the fix
(left: 0, right: 0x0123456789abcdef) and stable over repeated runs.
cargo nextest, clippy --all-targets, cargo doc, and cargo xtask fmt
pass for nvme and nvme_test.

Copilot AI review requested due to automatic review settings July 29, 2026 17:40
@smalis-msft
smalis-msft requested review from a team as code owners July 29, 2026 17:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an NVMe emulation correctness bug where shadow doorbell (DBBUF) and event index buffers could remain registered across a controller reset, causing the controller to read/write stale guest pages after reboot and potentially corrupting guest memory. It resets DoorbellMemory back to controller-internal storage on reset paths and adds a regression test to prevent future regressions.

Changes:

  • Add DoorbellMemory::reset() to drop shadow doorbell registration and clear event_idx_offset.
  • Invoke doorbell-memory reset after controller reset completion and after device-level worker reset.
  • Add an async regression test that verifies guest DBBUF pages are not written after a controller reset + re-enable without reconfiguring DBBUF.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vm/devices/storage/nvme/src/workers/coordinator.rs Reset shared doorbell storage when controller/device reset paths complete.
vm/devices/storage/nvme/src/queue.rs Add DoorbellMemory::reset() to restore controller-internal doorbell backing and clear event index state.
vm/devices/storage/nvme/src/tests/shadow_doorbell_tests.rs Add regression test ensuring DBBUF/event-idx buffers are not used after controller reset.
vm/devices/storage/nvme_test/src/workers/coordinator.rs Mirror the coordinator reset behavior in the fault-injection emulator copy.
vm/devices/storage/nvme_test/src/queue.rs Mirror DoorbellMemory::reset() in the fault-injection emulator copy.

Comment thread vm/devices/storage/nvme_test/src/queue.rs
Comment thread vm/devices/storage/nvme/src/queue.rs
@smalis-msft smalis-msft added the backport_1.8.2607 Change should be backported to the release/1.8.2607 branch label Jul 29, 2026
@github-actions

Copy link
Copy Markdown

@jstarks

jstarks commented Jul 30, 2026

Copy link
Copy Markdown
Member

Let's take #4011 instead.

@smalis-msft
smalis-msft deleted the nvme-doorbell-reset branch July 30, 2026 14:57
@smalis-msft smalis-msft removed the backport_1.8.2607 Change should be backported to the release/1.8.2607 branch label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants