wip: Advertise optional flr support #1858

gurasinghMS · 2025-08-13T21:04:30Z

Adding optional FLR support to the NvmeFaultController. Fixes: #1719. Description in the words of copilot:

Problem

When testing with the NVMe emulator, VFIO didn't allow OpenHCL to modify the reset_method file because the emulator didn't advertise FLR support. This prevented proper device reset functionality in virtualized environments.

Solution

Added a minimal PCI Express capability implementation that:

Advertises FLR support - Implements Device Capabilities register with FLR bit (bit 29) set
Handles FLR requests - Processes writes to Device Control register FLR trigger bit (bit 15)
Performs device reset - Resets NVMe controller state when FLR is initiated
Maintains compatibility - FLR support is optional and disabled by default

Key Components

PciExpressCapability: New capability struct implementing PCI Express Device Capabilities, Device Control, and Device Status registers
FlrHandler trait: Interface for handling FLR events with atomic signaling mechanism
Configuration options: Added flr_support flag to NvmeControllerCaps and NvmeControllerHandle
Reset integration: FLR triggers existing controller reset logic safely

Testing

Added comprehensive unit tests that verify:

FLR capability is properly advertised when enabled
No PCI Express capability appears when FLR is disabled
FLR trigger mechanism works with correct self-clearing behavior
All existing functionality remains unaffected

Usage

let caps = NvmeControllerCaps {
    msix_count: 64,
    max_io_queues: 64, 
    subsystem_id: Guid::new_random(),
    flr_support: true,  // Enable FLR support
};

Disclaimer: This is a reworked PR authored by copilot with logic fixes to how the FLR is completed and fixes to thedefinition of the DeviceConfiguration struct in PCIe capability + Moved the support to the fault emulator instead of the main one (for now).

gurasinghMS · 2025-08-13T21:11:04Z

Spec digarams (From PCIe Base Spec 6.4) for reference:

Copilot

Pull Request Overview

This PR adds optional Function Level Reset (FLR) support to the NVMe fault controller for testing purposes. This enables VFIO to modify the reset_method file by advertising FLR capability through PCI Express configuration space.

Key changes:

Implements PCI Express capability with FLR support in the fault controller
Adds comprehensive unit tests for FLR functionality
Updates configuration structures to include FLR support option

Reviewed Changes

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

Show a summary per file

File	Description
vm/devices/storage/nvme_test/src/tests/flr_tests.rs	New comprehensive test suite for FLR capability advertising and triggering
vm/devices/storage/nvme_test/src/pci.rs	Main implementation adding PCI Express capability with FLR handler and reset logic
vm/devices/pci/pci_core/src/capabilities/pci_express.rs	New PCI Express capability implementation with FLR support and extensive tests
vm/devices/pci/pci_core/src/spec.rs	PCI Express specification definitions for capability registers
vm/devices/storage/nvme_test/src/lib.rs	Updated fault configuration to use Arc for shared ownership
vm/devices/storage/nvme_resources/src/lib.rs	Added flr_support field to controller handle
vm/devices/storage/nvme_test/src/resolver.rs	Updated resolver to pass through flr_support configuration
vm/devices/storage/nvme_test/src/tests.rs	Added flr_tests module
vm/devices/storage/nvme_test/src/tests/controller_tests.rs	Updated fault configuration usage and added flr_support field
vm/devices/storage/disk_nvme/nvme_driver/src/tests.rs	Updated fault configuration usage and added flr_support field
vm/devices/pci/pci_core/src/capabilities/mod.rs	Exposed new PCI Express capability types

vm/devices/pci/pci_core/src/spec.rs

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

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

gurasinghMS · 2025-08-13T23:43:31Z

Some Open question for this PR:

Does this NVME Emulator count as a "memory based controller". In the Nvme spec, it specifies that "memory based controllers" should not be resetting the Admin Queues during controller reset (i.e. when cc.en() bit goes from 1 -> 0. Currently the emulator is clearing the Admin queues upon controller reset.
One big way that the FLR differs from a regular controller reset (in my understanding) is it should reset the device regardless of the controller state. A normal reset by setting cc.en = 0 should only be done when csts.rdy = 1 - If not, controller panics. I found 2 options while implementing this behaviour -- Option 1: call self.workers.reset().await but since we should be servicing the FLR right after the cfg_write, we can't await (cfg_write isn't async). -- Option 2: Just recreate the NvmeWorker. This has futures in it but it seems like the drop implementation for Task just removes the task so this should seemingly be OK to do.

mattkur

Looking good. Thanks for taking the time to work on this!

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

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

mattkur · 2025-08-15T17:29:27Z

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

+    /// Creates a new PCI Express capability with FLR support.
+    ///
+    /// # Arguments
+    /// * `flr_supported` - Whether Function Level Reset is supported
+    /// * `flr_handler` - Optional handler to be called when FLR is initiated
+    pub fn new(flr_supported: bool, flr_handler: Option<Arc<dyn FlrHandler>>) -> Self {


How do you envision that callers would use this if they specify flr_supported: true but flr_handler: None. I'm not sure that makes semantic sense. What do you think? (I'm happy to be wrong here, I'm still wrapping my head around the nuance of FLR behavior).

Does it make sense to even allow this behavior? It seems like even if you want a no-op, shouldn't you be forced to specify a handler?

Theoretically I think requiring FLR handler at all times makes sense to me. However, my holdout is it would add unnecessary bloat to tests down the road (specially if we expect more PCIe capabilities to be supported in the future). What if we get rid of the bool altogether and just interpret Some(FlrHandler) to mean that flr is supported?

Using above approach

Yeah, I like Some(FlrHandler) here.

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

mattkur · 2025-08-15T17:38:27Z

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

+            PciExpressCapabilityHeader::PCIE_CAPS => {
+                // PCIe Capabilities Register (16 bits) + Next Pointer (8 bits) + Capability ID (8 bits)
+                // For basic endpoint: Version=2, Device/Port Type=0 (PCI Express Endpoint)
+                let pcie_caps: u16 = 0x0002; // Version 2, Device/Port Type 0
+                (pcie_caps as u32) << 16 | (0x00 << 8) | CapabilityId::PCI_EXPRESS.0 as u32
+            }


This is not critical, but I'd prefer to see you define the appropriate struct type for this as well. We're already doing that for the device caps, etc.

Do you think this could be a follow up change to this? Since the PR has been sitting for a while I am trying to scope it to the mvp. Always happy to send out follow up changes!

Oh, yes. That can be a follow up.

chris-oo · 2025-08-18T21:32:30Z

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

+    /// Creates a new PCI Express capability with FLR support.
+    ///
+    /// # Arguments
+    /// * `flr_supported` - Whether Function Level Reset is supported
+    /// * `flr_handler` - Optional handler to be called when FLR is initiated
+    pub fn new(flr_supported: bool, flr_handler: Option<Arc<dyn FlrHandler>>) -> Self {


Does it make sense to even allow this behavior? It seems like even if you want a no-op, shouldn't you be forced to specify a handler?

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

github-actions · 2025-08-19T19:46:04Z

At least one Petri test failed.

github-actions · 2025-08-24T02:59:57Z

At least one Petri test failed.

…1913) This is a support PR being made to reduce size of #1858. Added `NvmeWorkersContext` that stores the input for `NvmeWorkers`. This will subsequently be used to create new `NvmeWorkers` when FLR is issued to the test controller.

- Add PCI Express capability ID to pci_core spec - Implement PciExpressCapability with FLR support in pci_core - Add FlrHandler trait for handling FLR events - Add flr_support flag to NvmeControllerCaps and NvmeControllerHandle - Wire FLR capability to NvmeController when enabled - Implement FLR reset logic using atomic flag mechanism - Add comprehensive unit tests for FLR functionality - Update nvme_resources to include FLR configuration option Co-authored-by: mattkur <[email protected]>

Co-authored-by: mattkur <[email protected]>

…l one

…vices

…eing isolated to the pcie flr testing

…king mut variables

…ion Pr is checked in

…n branch

vm/devices/pci/pci_core/src/capabilities/pci_express.rs

mattkur

Overall this LGTM. We discussed offline that you're going to split this PR into one for the core capability and another for the NVMe handling of it. Looking at a smaller nvme-only PR may yield some additional comments, but I think we're good to go here. Thanks for your work on this Guramrit!

Co-authored-by: Matt LaFayette (Kurjanowicz) <[email protected]>

…th some basic functionality (#1930) Adds the PCI_Express capability option for devices to advertise PCIe support. This change will also us to add FLR support to the nvme_test crate for further perf testing. PciExpressCapability: New capability struct implementing PCI Express Device Capabilities, Device Control, and Device Status registers Spec digarams (From PCIe Base Spec 6.4) for reference: <img width="1481" height="991" alt="image" src="https://github.com/user-attachments/assets/a6f49e62-af03-4573-ab60-17637154aadc" /> <img width="1489" height="919" alt="image" src="https://github.com/user-attachments/assets/21f6996c-eb8b-4fb6-b505-a113c37d9257" /> Being made as a helper PR to #1858 since that was getting a little too large/complicated --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Matt LaFayette (Kurjanowicz) <[email protected]>

gurasinghMS marked this pull request as ready for review August 13, 2025 23:30

Copilot AI review requested due to automatic review settings August 13, 2025 23:30

gurasinghMS requested review from a team as code owners August 13, 2025 23:30

Copilot AI reviewed Aug 13, 2025

View reviewed changes

gurasinghMS changed the title ~~[nvme_test] Advertise optional flr support~~ nvme_test: Advertise optional flr support Aug 14, 2025

mattkur reviewed Aug 15, 2025

View reviewed changes

chris-oo reviewed Aug 18, 2025

View reviewed changes

gurasinghMS force-pushed the advertise_flr_support branch from d30046e to ece7dac Compare August 23, 2025 23:27

gurasinghMS requested a review from a team as a code owner August 23, 2025 23:27

gurasinghMS mentioned this pull request Aug 25, 2025

nvme_test: Added NvmeWorkersContext input for the NvmeWorkers struct #1913

Merged

gurasinghMS and others added 14 commits August 25, 2025 14:47

Ran xtask fmt fix

e4705ce

Fixing build issues

7e25c82

Initial plan

466655a

Address PR feedback: improve PCI Express capability implementation

45a2414

Co-authored-by: mattkur <[email protected]>

Added a todo comment to fix the spec

e9ae16b

Initial plan

e6db4df

Initial plan

522058f

Initial plan

b8d2e27

Moved the flr logic to the nvme fault controller instead of the actua…

aaf1411

…l one

Fixed the Device Capabilites struct as per the nvme 6.4 spec

5848a4e

Fixed the Device Control struct

136537f

Remove link config and link control since that is options for PCIe de…

af2845c

…vices

Moving FLR logic to happen after a CFG write

0446d4b

gurasinghMS added 10 commits August 25, 2025 15:02

Rebasing to the Fault builder model style of fault injection

3831510

Added the find_pci_capability method as a test helper instead of it b…

efa32f5

…eing isolated to the pcie flr testing

Fixing build issues

716de0b

Small changes based on comments. Using builder notation instead of ma…

7ad0449

…king mut variables

Repsonding to comments about flr handler creation

f102776

Pausing work

067c193

Commit before merge

1efe1a7

Pausing work again. This will be completed after the Fault Configurat…

3f194fb

…ion Pr is checked in

Fixing build issues

c21cfa4

Responding to comments and build errors

8c2141e

gurasinghMS force-pushed the advertise_flr_support branch from 2919774 to 8c2141e Compare August 25, 2025 22:02

gurasinghMS and others added 2 commits August 25, 2025 15:19

Finished the merge and related changes. Back up to speed with the mai…

badcf29

…n branch

Merge branch 'main' into advertise_flr_support

ca9f04e

mattkur reviewed Aug 27, 2025

View reviewed changes

vm/devices/pci/pci_core/src/capabilities/pci_express.rs Show resolved Hide resolved

mattkur reviewed Aug 27, 2025

View reviewed changes

Apply suggestion from @mattkur

ecb8160

Co-authored-by: Matt LaFayette (Kurjanowicz) <[email protected]>

gurasinghMS changed the title ~~nvme_test: Advertise optional flr support~~ wip: Advertise optional flr support Aug 28, 2025

gurasinghMS marked this pull request as draft August 28, 2025 17:27

gurasinghMS mentioned this pull request Aug 28, 2025

pci_core: add infrastructure for devices to advertise pcie support with some basic functionality #1930

Merged

wip: Advertise optional flr support #1858

Are you sure you want to change the base?

wip: Advertise optional flr support #1858

Uh oh!

Conversation

gurasinghMS commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Key Components

Testing

Usage

Uh oh!

gurasinghMS commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gurasinghMS commented Aug 13, 2025

Uh oh!

mattkur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 19, 2025

Uh oh!

github-actions bot commented Aug 24, 2025

Uh oh!

Uh oh!

mattkur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gurasinghMS commented Aug 13, 2025 •

edited

Loading

gurasinghMS commented Aug 13, 2025 •

edited

Loading