Controller delay

[gurasinghMS]

No description provided.

[gurasinghMS]

Note that this is doing a LOT of the heavy lifting here by sending the doorbell writes later.
An issue that I noticed here is that this might end up dinging the doorbell down the line several times for batched requests. The batched requests can be throttled slightly by toying with the doorbell register. The "value" in the doorbell register write is the new tail for that queue. If we only update the tail one at a time instead of large increments, we can throttle requests to the inner controller. That way we do a doorbell write per admin command issued. Delays in that case can also be applied on a per-command level.

[gurasinghMS]

This prevents doorbell writes doing downstream. they are instead handled in the submission queue.

[gurasinghMS]

A few things still need doing:

• Replicate the AdminQueue enable / disable flow from the actual controller
• Figure out why the I/O completion queue creation is not working (even though the test itself is passing)
• Some cleanup of unused variables and such.
• Add some comments on the what and why for the notable functions
• Add fault injection to the PCI side of the controller too!

[mattkur]

I don't think we want to make this Default

[mattkur]

(You can derive FromZeroes if you want a default zero representation)

[Copilot]

Pull Request Overview

This PR implements a fault injection framework for the NVMe controller, allowing simulation of delays and other faults for testing purposes. The implementation creates a wrapper around the existing NVMe controller that can intercept admin queue commands and inject controlled delays.

• Adds a new fault injection framework for NVMe controller testing
• Introduces NvmeControllerFaultInjection struct that wraps the existing NvmeController
• Implements admin queue command interception with configurable fault injection functions

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
vm/devices/storage/nvme_spec/src/lib.rs	Adds Default derive to Command struct to support fault injection initialization
vm/devices/storage/nvme/src/lib.rs	Exports the new fault injection module and its public interface
vm/devices/storage/nvme/src/fault_injection/pci.rs	Main implementation of NvmeControllerFaultInjection wrapper with admin queue interception
vm/devices/storage/nvme/src/fault_injection/mod.rs	Module declaration for fault injection components
vm/devices/storage/nvme/src/fault_injection/admin.rs	Admin queue fault injection handler and state management
vm/devices/storage/nvme/Cargo.toml	Reorders task_control dependency for consistency
vm/devices/storage/disk_nvme/nvme_driver/src/tests.rs	Adds test case demonstrating 5-second admin command delay injection

[Copilot]

Remove commented-out import that is not being used.

Suggested change

// use crate::fault_injection::queue::SubmissionQueueFaultInjection;

[Copilot]

Remove or address the TODO comment about whether the Command event is needed.

[Copilot]

Remove or address the TODO comment about event handling complexity.

Suggested change

	// TODO: Why is the inner controller handling 3 different types of events? This seems to work for now.
	let next_command = state
	.admin_sq
	.next(&self.config.mem)
	.map(Event::Command)
	.await;
	// Refactored to handle events explicitly for better clarity and maintainability.
	let next_command = state
	.admin_sq
	.next(&self.config.mem)
	.await
	.map(Event::Command)?;
	// Additional event handling logic can be added here in the future.

[Copilot]

Remove the 'DONE:' comment as it appears to be a development note that should not remain in production code.

Suggested change

	// DONE: Always read from inner.
	fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult {
	let mut inner = self.inner.lock();
	inner.pci_cfg_read(offset, value)
	}
	// DONE: Copy any writes going to inner.
	fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult {
	let mut inner = self.inner.lock();
	inner.pci_cfg_read(offset, value)
	}

[Copilot]

Remove the 'DONE:' comment as it appears to be a development note that should not remain in production code.

Suggested change

	// DONE: Always read from inner.
	fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult {
	let mut inner = self.inner.lock();
	inner.pci_cfg_read(offset, value)
	}
	// DONE: Copy any writes going to inner.
	fn pci_cfg_read(&mut self, offset: u16, value: &mut u32) -> IoResult {
	let mut inner = self.inner.lock();
	inner.pci_cfg_read(offset, value)
	}

[Copilot]

The comment mentions extending to return Vec but the method signature returns u16. Either update the comment to reflect current implementation or consider the future extension.

Suggested change

	/// Returns the doorbells that need to be fault injected to. In this case it is just the admin submission queue
	/// TODO: Can be extended in the future to return a Vec<u16> of doorbell indices.
	/// Returns the doorbell index that needs to be fault injected. Currently, this is limited to the admin submission queue.

[Copilot]

The function documentation describes parameters that don't exist (fn_name, input) and mentions undefined types (FaultInjectionAction). Update the documentation to match the actual implementation.

Suggested change

	/// A fault injection function where fn_name is the name of the function being invoked and
	/// input is the input provided to that function
	/// this controller can respond with types of actions: FaultInjectionAction
	/// This function is used to mock the behavior of the NVMe controller for testing purposes.
	/// I might have to wrap this in a struct to control some Mesh::Cell objects.
	/// Simulates a delay in the NVMe controller's response to administrative commands.
	/// This function introduces a 5-second delay using the provided `driver` parameter.
	/// It is used to mock the behavior of the NVMe controller for testing purposes.
	/// Note: This function may need to be extended or wrapped in a struct for more complex use cases.

[Copilot]

Remove or address the TODO comment about making the driver parameter a reference.

Suggested change

	async fn fault_controller(driver: VmTaskDriver) {
	info!("DELAYING ADMIN COMMANDS FOR 5 SECONDS");
	// TODO: This should be a reference
	PolledTimer::new(&driver).sleep(Duration::new(5, 0)).await;
	async fn fault_controller(driver: &VmTaskDriver) {
	info!("DELAYING ADMIN COMMANDS FOR 5 SECONDS");
	// TODO: This should be a reference
	PolledTimer::new(driver).sleep(Duration::new(5, 0)).await;