vmotherboard, pcie: add Root Complex Integrated Endpoint (RCiEP) infra#3529
Draft
jstarks wants to merge 2 commits into
Draft
vmotherboard, pcie: add Root Complex Integrated Endpoint (RCiEP) infra#3529jstarks wants to merge 2 commits into
jstarks wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds infrastructure to support PCIe Root Complex Integrated Endpoints (RCiEPs) in the OpenVMM PCIe topology model, enabling Type 0 devices to live directly on a root complex start bus alongside root ports (e.g., AMD IOMMU).
Changes:
- Extend
GenericPcieRootComplexto host and route ECAM accesses to RCiEPs on the start bus, and add astart_deviceparameter to offset root port device numbering. - Extend vmotherboard PCIe wiring to register RCiEP placements (builder/services + resolver + conflict reason).
- Add a new conflict reason for enumerators that do not support RCiEPs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm_core/vmotherboard/src/chipset/mod.rs | Adds RciepNotSupported conflict reason and display text. |
| vmm_core/vmotherboard/src/chipset/builder/mod.rs | Adds builder plumbing to register RCiEP placements into the PCIe bus resolver. |
| vmm_core/vmotherboard/src/chipset/backing/arc_mutex/services.rs | Exposes a services API to register a static RCiEP placement for a device. |
| vmm_core/vmotherboard/src/chipset/backing/arc_mutex/pci.rs | Extends PCIe resolver/trait to support RCiEP registration with a default “not supported” error. |
| vmm_core/vmotherboard/src/chipset/backing/arc_mutex/device.rs | Adds on_pcie_root_complex(...) placement option and wiring to register RCiEPs. |
| vmm_core/vmotherboard/src/base_chipset.rs | Implements add_rciep for the root complex enumerator adapter. |
| vm/devices/pci/pcie/src/root.rs | Implements RCiEP storage + ECAM routing; adds start_device offset for root port devfns; updates unit test instantiation call site. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
vm/devices/pci/pcie/src/root.rs:342
add_rciepindexesself.devices[device as usize]without validatingdevice < 32, so an invalid device number will panic. This should be handled as a normal error (e.g., return an error indicating invalid device number) rather than panicking.
pub fn add_rciep(
&mut self,
device: u8,
name: impl Into<Arc<str>>,
dev: Box<dyn GenericPciBusDevice>,
) -> Result<(), Arc<str>> {
let name = name.into();
match &self.devices[device as usize] {
BusDevice::Empty => {}
BusDevice::RootPort { name: existing, .. }
| BusDevice::Rciep { name: existing, .. } => {
return Err(existing.clone());
}
}
self.devices[device as usize] = BusDevice::Rciep { name, dev };
vm/devices/pci/pcie/src/root.rs:783
- The saved-state field
PortSavedState.port_numberpreviously represented the root-port key (devfn, i.e.device<<3), but it now represents the raw device number and restore indexes directly intodevices[...]. This changes the meaning of an existing protobuf field and will break restore of older saved states. Preserve the old encoding (or add explicit backwards-compat translation during restore) so snapshots from previous builds can still be restored.
/// Saved state for a single root port.
#[derive(Protobuf)]
#[mesh(package = "pcie.root")]
pub struct PortSavedState {
/// The PCI device number (0-31) of this port.
#[mesh(1)]
pub port_number: u8,
/// The root port Type 1 configuration space state.
#[mesh(2)]
pub cfg_space: RootPortCfgSpaceSavedState,
}
/// Saved state for the GenericPcieRootComplex.
#[derive(Protobuf, SavedStateRoot)]
#[mesh(package = "pcie.root")]
pub struct SavedState {
/// The lowest valid bus number under the root complex.
#[mesh(1)]
pub start_bus: u8,
/// The highest valid bus number under the root complex.
#[mesh(2)]
pub end_bus: u8,
/// Saved state for each root port.
#[mesh(3)]
pub ports: Vec<PortSavedState>,
}
}
impl SaveRestore for GenericPcieRootComplex {
type SavedState = state::SavedState;
fn save(&mut self) -> Result<Self::SavedState, SaveError> {
// Save all root ports in device-number order (already ordered by array index).
let mut ports = Vec::new();
for (i, d) in self.devices.iter_mut().enumerate() {
if let BusDevice::RootPort { port, .. } = d {
ports.push(state::PortSavedState {
port_number: i as u8,
cfg_space: port.port.cfg_space.save()?,
});
}
}
Ok(state::SavedState {
start_bus: self.start_bus,
end_bus: self.end_bus,
ports,
})
}
fn restore(&mut self, state: Self::SavedState) -> Result<(), RestoreError> {
let state::SavedState {
start_bus,
end_bus,
ports,
} = state;
// Validate that bus numbers match
if start_bus != self.start_bus || end_bus != self.end_bus {
return Err(RestoreError::InvalidSavedState(anyhow::anyhow!(
"bus number mismatch: saved ({}-{}), current ({}-{})",
start_bus,
end_bus,
self.start_bus,
self.end_bus
)));
}
// Validate port count matches
let root_port_count = self
.devices
.iter()
.filter(|d| matches!(d, BusDevice::RootPort { .. }))
.count();
if ports.len() != root_port_count {
return Err(RestoreError::InvalidSavedState(anyhow::anyhow!(
"root port count mismatch: saved {}, current {}",
ports.len(),
root_port_count
)));
}
let mut seen_ports = HashSet::with_capacity(ports.len());
// Restore each saved port by explicit port number.
for port_state in ports {
if !seen_ports.insert(port_state.port_number) {
return Err(RestoreError::InvalidSavedState(anyhow::anyhow!(
"duplicate root port {} in saved state",
port_state.port_number
)));
}
match &mut self.devices[port_state.port_number as usize] {
BusDevice::RootPort { port, .. } => {
port.port.cfg_space.restore(port_state.cfg_space)?;
}
BusDevice::Rciep { .. } | BusDevice::Empty => {
return Err(RestoreError::InvalidSavedState(anyhow::anyhow!(
"root port {} not found",
port_state.port_number
)));
}
}
}
Ok(())
Comment on lines
260
to
+270
| pub fn add_pcie_device( | ||
| &mut self, | ||
| port: u8, | ||
| name: impl AsRef<str>, | ||
| dev: Box<dyn GenericPciBusDevice>, | ||
| ) -> Result<(), Arc<str>> { | ||
| let (_port_name, root_port) = self.ports.get_mut(&port).ok_or_else(|| -> Arc<str> { | ||
| tracing::error!( | ||
| "GenericPcieRootComplex: port {:#x} not found for device '{}'", | ||
| port, | ||
| name.as_ref() | ||
| ); | ||
| format!("Port {:#x} not found", port).into() | ||
| })?; | ||
|
|
||
| match root_port.connect_device(name, dev) { | ||
| Ok(()) => Ok(()), | ||
| Err(existing_device) => { | ||
| tracing::warn!( | ||
| "GenericPcieRootComplex: failed to connect device to port {:#x}, existing device: '{}'", | ||
| port, | ||
| existing_device | ||
| ); | ||
| Err(existing_device) | ||
| } | ||
| } | ||
| let root_port = match &mut self.devices[port as usize] { | ||
| BusDevice::RootPort { port, .. } => port, | ||
| BusDevice::Rciep { name: existing, .. } => return Err(existing.clone()), | ||
| BusDevice::Empty => return Err(format!("device {port} is not a root port").into()), | ||
| }; |
Comment on lines
+92
to
+103
| /// Try to add a Root Complex Integrated Endpoint (RCiEP) at the given | ||
| /// device/function on the start bus of the root complex. | ||
| /// | ||
| /// Not all enumerators support RCiEPs — only root complexes do. | ||
| /// The default implementation returns an error. | ||
| fn add_rciep( | ||
| &mut self, | ||
| _device: u8, | ||
| name: Arc<str>, | ||
| _device_handle: Weak<CloseableMutex<dyn ChipsetDevice>>, | ||
| ) -> Result<(), PcieConflict> { | ||
| Err(PcieConflict { |
Comment on lines
+322
to
+343
| /// Attach a Root Complex Integrated Endpoint (RCiEP) at the given | ||
| /// PCI device number (0-31) on the start bus of this root complex. | ||
| /// | ||
| /// RCiEPs are Type 0 PCI functions that appear directly on the start | ||
| /// bus alongside root ports (e.g., an AMD IOMMU at device 0). | ||
| /// They do not sit behind a downstream port and have a fixed BDF. | ||
| pub fn add_rciep( | ||
| &mut self, | ||
| device: u8, | ||
| name: impl Into<Arc<str>>, | ||
| dev: Box<dyn GenericPciBusDevice>, | ||
| ) -> Result<(), Arc<str>> { | ||
| let name = name.into(); | ||
| match &self.devices[device as usize] { | ||
| BusDevice::Empty => {} | ||
| BusDevice::RootPort { name: existing, .. } | ||
| | BusDevice::Rciep { name: existing, .. } => { | ||
| return Err(existing.clone()); | ||
| } | ||
| } | ||
| self.devices[device as usize] = BusDevice::Rciep { name, dev }; | ||
| Ok(()) |
Some PCIe devices — most notably an AMD IOMMU — live on the start bus of a root complex as Type 0 PCI functions alongside (not behind) root ports. The existing PCIe switch infra only knew how to attach devices behind downstream ports, so there was no way to wire up an RCiEP. Teach the generic root complex (GenericPcieRootComplex) to hold a map of RCiEPs indexed by combined device/function, and route ECAM accesses on the start bus that don't hit a root port through to a registered RCiEP. Also let new() take a start_device parameter so callers can reserve low device numbers on the start bus for RCiEPs (e.g., put the IOMMU at device 0 and start root ports at device 1). Plumb a matching path through the vmotherboard builder: an on_pcie_root_complex(enumerator_id, device, function) placement method, a register_static_pcie_rciep services helper, a WeakMutexPcieRciepEntry type, an rcieps list on BusResolverWeakMutexPcie, and an add_rciep method on the RegisterWeakMutexPcie trait (with a default impl that returns a new RciepNotSupported PcieConflictReason so non-RC enumerators fail loudly). No callers yet — that comes in a follow-up. This change is purely additive infrastructure.
Comment on lines
332
to
+342
| pub fn add_pcie_device( | ||
| &mut self, | ||
| port: u8, | ||
| name: impl AsRef<str>, | ||
| dev: Box<dyn GenericPciBusDevice>, | ||
| ) -> Result<(), Arc<str>> { | ||
| let (_port_name, root_port) = self.ports.get_mut(&port).ok_or_else(|| -> Arc<str> { | ||
| tracing::error!( | ||
| "GenericPcieRootComplex: port {:#x} not found for device '{}'", | ||
| port, | ||
| name.as_ref() | ||
| ); | ||
| format!("Port {:#x} not found", port).into() | ||
| })?; | ||
| let root_port = match &mut self.devices[port as usize] { | ||
| BusDevice::RootPort { port, .. } => port, | ||
| BusDevice::Rciep { name: existing, .. } => return Err(existing.clone()), | ||
| BusDevice::Empty => return Err(format!("device {port} is not a root port").into()), | ||
| }; |
Comment on lines
+407
to
+414
| match &self.devices[device as usize] { | ||
| BusDevice::Empty => {} | ||
| BusDevice::RootPort { name: existing, .. } | ||
| | BusDevice::Rciep { name: existing, .. } => { | ||
| return Err(existing.clone()); | ||
| } | ||
| } | ||
| self.devices[device as usize] = BusDevice::Rciep { name, dev }; |
Comment on lines
919
to
943
| let mut seen_ports = HashSet::with_capacity(ports.len()); | ||
|
|
||
| // Restore each saved port by explicit port number. | ||
| for port_state in ports { | ||
| if !seen_ports.insert(port_state.port_number) { | ||
| return Err(RestoreError::InvalidSavedState(anyhow::anyhow!( | ||
| "duplicate root port {} in saved state", | ||
| port_state.port_number | ||
| ))); | ||
| } | ||
|
|
||
| if let Some((_, root_port)) = self.ports.get_mut(&port_state.port_number) { | ||
| root_port.port.cfg_space.restore(port_state.cfg_space)?; | ||
| root_port.port.restore_cxl_component_registers_state( | ||
| port_state.cxl_component_registers, | ||
| )?; | ||
| } else { | ||
| return Err(RestoreError::InvalidSavedState(anyhow::anyhow!( | ||
| "root port {} not found", | ||
| port_state.port_number | ||
| ))); | ||
| match &mut self.devices[port_state.port_number as usize] { | ||
| BusDevice::RootPort { port, .. } => { | ||
| port.port.cfg_space.restore(port_state.cfg_space)?; | ||
| port.port.restore_cxl_component_registers_state( | ||
| port_state.cxl_component_registers, | ||
| )?; | ||
| } | ||
| BusDevice::Rciep { .. } | BusDevice::Empty => { | ||
| return Err(RestoreError::InvalidSavedState(anyhow::anyhow!( | ||
| "root port {} not found", | ||
| port_state.port_number | ||
| ))); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some PCIe devices — most notably an AMD IOMMU — live on the start bus of a root complex as Type 0 PCI functions alongside (not behind) root ports. The existing PCIe switch infra only knew how to attach devices behind downstream ports, so there was no way to wire up an RCiEP.
Teach the generic root complex (GenericPcieRootComplex) to hold RCiEPs indexed by PCI device number (0-31), and route ECAM accesses on the start bus that don't hit a root port through to a registered RCiEP at function 0. Also add a builder API with a
first_port_device_numberoption so callers can reserve low device numbers on the start bus for RCiEPs (e.g., put the IOMMU at device 0 and start root ports at device 1).No callers yet — that comes in a follow-up. This change is purely additive infrastructure.