Skip to content

Configurable PCI BDF Device IDs #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: gardenlinux
Choose a base branch
from
Draft

Configurable PCI BDF Device IDs #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9618d2e
pci: Allow for device ID allocation on a bus
scholzp Nov 7, 2025
9efc3ad
vmm: Allow for allocating specific PCI BDFs
scholzp Nov 7, 2025
ee66fc1
vmm: Add tests for `allocate_device_bdf` in `PciSegment`
scholzp Nov 7, 2025
fbd3b93
vmm: Introduce `addr` argument to PCI device configs and update parser
scholzp Nov 7, 2025
1ca7c38
vmm: Add tests for BDF device address parsing in configs
scholzp Nov 7, 2025
3035868
vmm: Panic if a PCI function ID other than 0 is used
scholzp Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 129 additions & 7 deletions pci/src/bus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,15 +166,42 @@ impl PciBus {
Ok(())
}

pub fn next_device_id(&mut self) -> Result<u32> {
for (idx, device_id) in self.device_ids.iter_mut().enumerate() {
if !(*device_id) {
*device_id = true;
return Ok(idx as u32);
/// Allocates a PCI device ID on the bus.
///
/// - `id`: ID to allocate on the bus. If [`None`], the next free
/// device ID on the bus is allocated, else the ID given is
Copy link
Member

@phip1611 phip1611 Nov 10, 2025

Choose a reason for hiding this comment

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

this changes should be merged into the commit where you introduced these lines, please

/// allocated
///
/// ## Errors
/// * Returns [`PciRootError::AlreadyInUsePciDeviceSlot`] in case
/// the ID requested is already allocated.
/// * Returns [`PciRootError::InvalidPciDeviceSlot`] in case the
/// requested ID exceeds the maximum number of devices allowed per
/// bus (see [`NUM_DEVICE_IDS`]).
/// * If `id` is [`None`]: Returns
/// [`PciRootError::NoPciDeviceSlotAvailable`] if no free device
/// slot is available on the bus.
pub fn allocate_device_id(&mut self, id: Option<u8>) -> Result<u32> {
if let Some(id) = id {
if (id as usize) < NUM_DEVICE_IDS {
if !self.device_ids[id as usize] {
self.device_ids[id as usize] = true;
Ok(id as u32)
} else {
Err(PciRootError::AlreadyInUsePciDeviceSlot(id as usize))
}
} else {
Err(PciRootError::InvalidPciDeviceSlot(id as usize))
Copy link
Member

@phip1611 phip1611 Nov 10, 2025

Choose a reason for hiding this comment

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

same here

}
} else {
for (idx, device_id) in self.device_ids.iter_mut().enumerate() {
if !(*device_id) {
*device_id = true;
return Ok(idx as u32);
}
}
Err(PciRootError::NoPciDeviceSlotAvailable)
}

Err(PciRootError::NoPciDeviceSlotAvailable)
}

pub fn get_device_id(&mut self, id: usize) -> Result<()> {
Expand Down Expand Up @@ -484,3 +511,98 @@ fn parse_io_config_address(config_address: u32) -> (usize, usize, usize, usize)
shift_and_mask(config_address, REGISTER_NUMBER_OFFSET, REGISTER_NUMBER_MASK),
)
}

#[cfg(test)]
mod tests {
// Note this useful idiom: importing names from outer (for mod tests) scope.
Copy link
Member

@phip1611 phip1611 Nov 10, 2025

Choose a reason for hiding this comment

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

is this comment from ChatGPT? :D

use super::*;

mod pci_bus_tests {
Copy link
Member

@phip1611 phip1611 Nov 10, 2025

Choose a reason for hiding this comment

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

you are already in pci::bus::tests, no need for another nesting level

use super::*;

#[derive(Debug)]
struct MocRelocDevice;

impl DeviceRelocation for MocRelocDevice {
fn move_bar(
&self,
_old_base: u64,
_new_base: u64,
_len: u64,
_pci_dev: &mut dyn PciDevice,
_region_type: PciBarRegionType,
) -> std::result::Result<(), std::io::Error> {
Ok(())
}
}

fn setup_bus() -> PciBus {
let pci_root = PciRoot::new(None);
let moc_device_reloc = Arc::new(MocRelocDevice {});
PciBus::new(pci_root, moc_device_reloc)
}

#[test]
// Test to acquire all IDs that can be acquired
fn allocate_device_id_next_free() {
// The first address is occupied by the root
let mut bus = setup_bus();
for expected_id in 1..NUM_DEVICE_IDS {
assert_eq!(expected_id as u32, bus.allocate_device_id(None).unwrap());
}
}

#[test]
// Test that requesting specific ID work
fn allocate_device_id_request_id() -> std::result::Result<(), Box<dyn std::error::Error>> {
Copy link
Member

@phip1611 phip1611 Nov 10, 2025

Choose a reason for hiding this comment

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

please return result::Result<(), Box<dyn error::Error>> or even Result<(), Box<dyn Error>>

// The first address is occupied by the root
let mut bus = setup_bus();
let max_id = (NUM_DEVICE_IDS - 1).try_into()?;
assert_eq!(0x01_u32, bus.allocate_device_id(Some(0x01))?);
assert_eq!(0x10_u32, bus.allocate_device_id(Some(0x10))?);
assert_eq!(max_id as u32, bus.allocate_device_id(Some(max_id))?);
Ok(())
}

#[test]
// Test that requesting the same ID twice fails
fn allocate_device_id_request_id_twice_fails()
-> std::result::Result<(), Box<dyn std::error::Error>> {
let mut bus = setup_bus();
let max_id = (NUM_DEVICE_IDS - 1).try_into()?;
bus.allocate_device_id(Some(max_id))?;
let _result = bus.allocate_device_id(Some(max_id));
assert!(matches!(
PciRootError::AlreadyInUsePciDeviceSlot(max_id.into()),
_result
));
Ok(())
}

#[test]
// Test to request an invalid ID
fn allocate_device_id_request_invalid_id_fails()
-> std::result::Result<(), Box<dyn std::error::Error>> {
let mut bus = setup_bus();
let max_id = (NUM_DEVICE_IDS + 1).try_into()?;
let _result = bus.allocate_device_id(Some(max_id));
assert!(matches!(
PciRootError::InvalidPciDeviceSlot(max_id.into()),
_result
));
Ok(())
}

#[test]
// Test to acquire an ID when all IDs were already acquired
fn allocate_device_id_none_left() {
// The first address is occupied by the root
let mut bus = setup_bus();
for expected_id in 1..NUM_DEVICE_IDS {
assert_eq!(expected_id as u32, bus.allocate_device_id(None).unwrap());
}
let _result = bus.allocate_device_id(None);
assert!(matches!(PciRootError::NoPciDeviceSlotAvailable, _result));
}
}
}
53 changes: 43 additions & 10 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,7 @@ mod unit_tests {
rng: RngConfig {
src: PathBuf::from("/dev/urandom"),
iommu: false,
bdf_device_id: None,
},
balloon: None,
fs: None,
Expand Down Expand Up @@ -1211,6 +1212,24 @@ mod unit_tests {
}"#,
true,
),
(
vec![
"cloud-hypervisor",
"--kernel",
"/path/to/kernel",
"--disk",
"path=/path/to/disk/1,addr=15.0",
"path=/path/to/disk/2",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"disks": [
{"path": "/path/to/disk/1", "bdf_device_id": 15},
{"path": "/path/to/disk/2"}
]
}"#,
true,
),
(
vec![
"cloud-hypervisor",
Expand Down Expand Up @@ -1422,6 +1441,20 @@ mod unit_tests {
}"#,
true,
),
(
vec![
"cloud-hypervisor", "--kernel", "/path/to/kernel",
"--net",
"mac=12:34:56:78:90:ab,host_mac=34:56:78:90:ab:cd,tap=tap0,ip=1.2.3.4,mask=5.6.7.8,addr=15.0",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"net": [
{"mac": "12:34:56:78:90:ab", "host_mac": "34:56:78:90:ab:cd", "tap": "tap0", "ip": "1.2.3.4", "mask": "5.6.7.8", "num_queues": 2, "queue_size": 256, "bdf_device_id": 15}
]
}"#,
true,
),
#[cfg(target_arch = "x86_64")]
(
vec![
Expand Down Expand Up @@ -1493,11 +1526,11 @@ mod unit_tests {
"--kernel",
"/path/to/kernel",
"--rng",
"src=/path/to/entropy/source",
"src=/path/to/entropy/source,addr=15.0",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"rng": {"src": "/path/to/entropy/source"}
"rng": {"src": "/path/to/entropy/source", "bdf_device_id": 15}
}"#,
true,
)]
Expand All @@ -1514,14 +1547,14 @@ mod unit_tests {
"cloud-hypervisor", "--kernel", "/path/to/kernel",
"--memory", "shared=true",
"--fs",
"tag=virtiofs1,socket=/path/to/sock1",
"tag=virtiofs1,socket=/path/to/sock1,addr=15.0",
"tag=virtiofs2,socket=/path/to/sock2",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"memory" : { "shared": true, "size": 536870912 },
"fs": [
{"tag": "virtiofs1", "socket": "/path/to/sock1"},
{"tag": "virtiofs1", "socket": "/path/to/sock1", "bdf_device_id": 15},
{"tag": "virtiofs2", "socket": "/path/to/sock2"}
]
}"#,
Expand Down Expand Up @@ -1593,13 +1626,13 @@ mod unit_tests {
"--kernel",
"/path/to/kernel",
"--pmem",
"file=/path/to/img/1,size=1G",
"file=/path/to/img/1,size=1G,addr=15.0",
"file=/path/to/img/2,size=2G",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"pmem": [
{"file": "/path/to/img/1", "size": 1073741824},
{"file": "/path/to/img/1", "size": 1073741824,"bdf_device_id": 15},
{"file": "/path/to/img/2", "size": 2147483648}
]
}"#,
Expand Down Expand Up @@ -1877,13 +1910,13 @@ mod unit_tests {
"--kernel",
"/path/to/kernel",
"--vdpa",
"path=/path/to/device/1",
"path=/path/to/device/1,addr=15.0",
"path=/path/to/device/2,num_queues=2",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"vdpa": [
{"path": "/path/to/device/1", "num_queues": 1},
{"path": "/path/to/device/1", "num_queues": 1, "bdf_device_id": 15},
{"path": "/path/to/device/2", "num_queues": 2}
]
}"#,
Expand Down Expand Up @@ -1922,11 +1955,11 @@ mod unit_tests {
"--kernel",
"/path/to/kernel",
"--vsock",
"cid=123,socket=/path/to/sock/1",
"cid=123,socket=/path/to/sock/1,addr=15.0",
],
r#"{
"payload": {"kernel": "/path/to/kernel"},
"vsock": {"cid": 123, "socket": "/path/to/sock/1"}
"vsock": {"cid": 123, "socket": "/path/to/sock/1", "bdf_device_id": 15}
}"#,
true,
),
Expand Down
Loading
Loading