From 8d3c5c0fc9fe10445889eb893b1d03e66303a56f Mon Sep 17 00:00:00 2001 From: Callum Date: Mon, 20 Jul 2026 21:09:37 +1000 Subject: [PATCH] IOMMU: Style Fix ups (minor) Improve variable naming such that it is consistent between the sdf api and the code. Signed-off-by: Callum --- tool/microkit/src/capdl/builder.rs | 77 ++++++++++++++---------------- tool/microkit/src/capdl/memory.rs | 6 +-- tool/microkit/src/sdf.rs | 58 ++++++++++------------ tool/microkit/tests/test.rs | 2 +- 4 files changed, 65 insertions(+), 78 deletions(-) diff --git a/tool/microkit/src/capdl/builder.rs b/tool/microkit/src/capdl/builder.rs index 0f4876f70..612c1d499 100644 --- a/tool/microkit/src/capdl/builder.rs +++ b/tool/microkit/src/capdl/builder.rs @@ -24,8 +24,8 @@ use crate::{ }, elf::ElfFile, sdf::{ - CapMapType, CpuCore, IommuDeviceIdentifier, Map, SystemDescription, BUDGET_DEFAULT, - MONITOR_DOMAIN, MONITOR_PD_NAME, MONITOR_PRIORITY, + CapMapType, CpuCore, Map, SystemDescription, BUDGET_DEFAULT, MONITOR_DOMAIN, + MONITOR_PD_NAME, MONITOR_PRIORITY, }, sel4::{Arch, Config, PageSize}, util::{ranges_overlap, round_down, round_up}, @@ -1216,46 +1216,19 @@ pub fn build_capdl_spec( } // ********************************* - // Step 5. Handle extra cap mappings + // Step 5. Create IOMMU Address Spaces // ********************************* - - for (pd_dest_idx, pd) in system.protection_domains.iter().enumerate() { - for cap_map in pd.cap_maps.iter() { - // TODO: Once we add more CapMap options, they might not all have - // the pd_name. But for now, they do. - let pd_src_shadow_cspace = &pd_shadow_cspaces[&cap_map.pd.unwrap()]; - - let cap_map_obj = match cap_map.cap_type { - CapMapType::Tcb => capdl_util_make_tcb_cap(pd_src_shadow_cspace.tcb), - CapMapType::Sc => capdl_util_make_sc_cap(pd_src_shadow_cspace.sched_context), - CapMapType::VSpace => capdl_util_make_page_table_cap(pd_src_shadow_cspace.vspace), - }; - - // Map this into the destination pd's cspace and the specified slot. - pd_shadow_cspaces[&pd_dest_idx].insert_cap_into_root_cnode( - &mut spec_container, - cap_map.slot as u32, - cap_map_obj, - ); - } - } - - // ********************************* - // Step 6. Create IOMMU Address Spaces - // ********************************* - let mut iospace_by_device: HashMap = HashMap::new(); + let mut iospace_by_device: HashMap<&str, AddressSpace> = HashMap::new(); for iomap in system.iomaps.iter() { - let address_space = iospace_by_device - .entry(iomap.identifier) - .or_insert_with(|| { - create_iospace( - &mut spec_container, - kernel_config, - &iomap.device, - iomap.identifier, - iomap.domain_id, - ) - }); + let address_space = iospace_by_device.entry(&iomap.name).or_insert_with(|| { + create_iospace( + &mut spec_container, + kernel_config, + &iomap.name, + iomap.identifier, + iomap.domain_id, + ) + }); let page_size_bytes = mr_name_to_frames .get(&iomap.mr) .ok_or(format!( @@ -1279,6 +1252,30 @@ pub fn build_capdl_spec( )?; } + // ********************************* + // Step 6. Handle extra cap mappings + // ********************************* + for (pd_dest_idx, pd) in system.protection_domains.iter().enumerate() { + for cap_map in pd.cap_maps.iter() { + // TODO: Once we add more CapMap options, they might not all have + // the pd_name. But for now, they do. + let pd_src_shadow_cspace = &pd_shadow_cspaces[&cap_map.pd.unwrap()]; + + let cap_map_obj = match cap_map.cap_type { + CapMapType::Tcb => capdl_util_make_tcb_cap(pd_src_shadow_cspace.tcb), + CapMapType::Sc => capdl_util_make_sc_cap(pd_src_shadow_cspace.sched_context), + CapMapType::VSpace => capdl_util_make_page_table_cap(pd_src_shadow_cspace.vspace), + }; + + // Map this into the destination pd's cspace and the specified slot. + pd_shadow_cspaces[&pd_dest_idx].insert_cap_into_root_cnode( + &mut spec_container, + cap_map.slot as u32, + cap_map_obj, + ); + } + } + // ********************************* // Step 7. Emit a domain schedule // ********************************* diff --git a/tool/microkit/src/capdl/memory.rs b/tool/microkit/src/capdl/memory.rs index 1d089c80a..67a73626f 100644 --- a/tool/microkit/src/capdl/memory.rs +++ b/tool/microkit/src/capdl/memory.rs @@ -405,14 +405,14 @@ impl AddressSpace { pub fn create_iospace( spec_container: &mut CapDLSpecContainer, sel4_config: &Config, - device_name: &str, + name: &str, device_identifier: IommuDeviceIdentifier, domain_id: Option, ) -> AddressSpace { let IommuDeviceIdentifier::X86Pci(pci_device) = device_identifier; let root = spec_container.add_root_object(CapDLNamedObject { - name: format!("{}_{}", get_iopt_level_name(0), device_name).into(), + name: format!("{}_{}", get_iopt_level_name(0), name).into(), object: Object::IOSpace(object::IOSpace { slots: vec![], domain_id: domain_id.unwrap().into(), @@ -421,7 +421,7 @@ pub fn create_iospace( }); let address_space = AddressSpace::IOSpace { - name: device_name.to_string(), + name: name.to_string(), root, device: device_identifier, }; diff --git a/tool/microkit/src/sdf.rs b/tool/microkit/src/sdf.rs index 201faba1b..ca0e2dd1a 100644 --- a/tool/microkit/src/sdf.rs +++ b/tool/microkit/src/sdf.rs @@ -28,7 +28,6 @@ use sel4_capdl_initializer_types::{ use std::collections::{hash_map, HashMap, HashSet}; use std::fmt; use std::fs; -use std::hash::{Hash, Hasher}; use std::num::NonZero; use std::ops::Deref; use std::path::{Path, PathBuf}; @@ -100,14 +99,6 @@ fn loc_string(xml_sdf: &XmlSystemDescription, pos: roxmltree::TextPos) -> String #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PciDevice(pub object::PCIDevice); -impl Hash for PciDevice { - fn hash(&self, state: &mut H) { - self.0.bus.hash(state); - self.0.device.hash(state); - self.0.function.hash(state); - } -} - impl Deref for PciDevice { type Target = object::PCIDevice; @@ -229,7 +220,7 @@ impl FromStr for PciDevice { // This can be extended in future to support devices on an SMMU enabled Arm device // or IOMMU enabled RISC-V device. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum IommuDeviceIdentifier { X86Pci(PciDevice), } @@ -340,7 +331,7 @@ impl SysIOMapPerms { #[derive(Debug, PartialEq, Eq, Clone)] pub struct SysIOMap { - pub device: String, + pub name: String, pub mr: String, pub identifier: IommuDeviceIdentifier, pub domain_id: Option, @@ -766,7 +757,7 @@ impl SysIOMap { _config: &Config, xml_sdf: &XmlSystemDescription, node: &roxmltree::Node, - device: &str, + name: &str, identifier: IommuDeviceIdentifier, domain_id: Option, ) -> Result { @@ -806,7 +797,7 @@ impl SysIOMap { }; Ok(SysIOMap { - device: device.to_string(), + name: name.to_string(), mr, identifier, domain_id, @@ -829,9 +820,9 @@ impl IOAddressSpace { config: &Config, xml_sdf: &XmlSystemDescription, node: &roxmltree::Node, - device_names: &mut HashSet, + names: &mut HashSet, domain_ids: &mut HashSet, - iommu_device_identifiers: &mut HashSet, + iommu_device_identifiers: &mut Vec, ) -> Result { let pos = xml_sdf.doc.text_pos_at(node.range().start); if !config.iommu { @@ -842,12 +833,12 @@ impl IOAddressSpace { } check_attributes(xml_sdf, node, &["name", "peripheral_id", "domain_id"])?; - let device_name = checked_lookup(xml_sdf, node, "name")?; - if !device_names.insert(device_name.to_string()) { + let name = checked_lookup(xml_sdf, node, "name")?; + if !names.insert(name.to_string()) { return Err(value_error( xml_sdf, node, - format!("duplicate device name '{device_name}'"), + format!("duplicate name '{name}'"), )); } @@ -884,27 +875,22 @@ impl IOAddressSpace { format!("failed to parse device peripheral_id '{identifier_str}': {err}"), ) })?; - if !iommu_device_identifiers.insert(identifier) { + if iommu_device_identifiers.contains(&identifier) { return Err(value_error( xml_sdf, node, format!("duplicate device peripheral_id '{identifier}'"), )); } + iommu_device_identifiers.push(identifier); let mut iomaps = Vec::new(); for child in node.children().filter(|node| node.is_element()) { match child.tag_name().name() { "iomap" => { - let iomap = SysIOMap::from_xml( - config, - xml_sdf, - &child, - device_name, - identifier, - domain_id, - )?; + let iomap = + SysIOMap::from_xml(config, xml_sdf, &child, name, identifier, domain_id)?; iomaps.push(iomap); } _ => { @@ -2568,10 +2554,13 @@ fn check_io_maps( mrs: &[SysMemoryRegion], iomaps: &[SysIOMap], ) -> Result<(), String> { - let mut by_device: HashMap> = HashMap::new(); + let mut by_device: HashMap<&str, Vec<&SysIOMap>> = HashMap::new(); for iomap in iomaps { - by_device.entry(iomap.identifier).or_default().push(iomap); + by_device + .entry(iomap.name.as_str()) + .or_default() + .push(iomap); } if iomaps.iter().any(|iomap| { @@ -2583,8 +2572,9 @@ fn check_io_maps( ); } - for (identifier, maps) in by_device { - let address_space = identifier.to_string(); + for maps in by_device.into_values() { + let last = maps.iter().last().unwrap(); + let address_space = last.identifier.to_string(); check_maps( xml_sdf, mrs, @@ -2774,9 +2764,9 @@ pub fn parse( let mut root_pds = vec![]; let mut mrs = vec![]; let mut iomaps = vec![]; - let mut device_names = HashSet::new(); + let mut io_address_space_names = HashSet::new(); let mut iommu_domain_ids = HashSet::new(); - let mut iommu_device_identifiers = HashSet::new(); + let mut iommu_device_identifiers = Vec::new(); let mut channels = vec![]; let mut domains = Domains::default(); let system = doc @@ -2816,7 +2806,7 @@ pub fn parse( config, &xml_sdf, &child, - &mut device_names, + &mut io_address_space_names, &mut iommu_domain_ids, &mut iommu_device_identifiers, )? diff --git a/tool/microkit/tests/test.rs b/tool/microkit/tests/test.rs index 96e9d11f7..1f1442588 100644 --- a/tool/microkit/tests/test.rs +++ b/tool/microkit/tests/test.rs @@ -972,7 +972,7 @@ mod iommu { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "iommu_duplicate_device.system", - "Error: duplicate device name 'test_device' on element 'io_address_space':", + "Error: duplicate name 'test_device' on element 'io_address_space':", ) }