From adbc2d654f5e126a1c241d86e2885e1fa920c967 Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 14:39:08 +1000 Subject: [PATCH 1/7] tool(cleanup): add attribute-as-number parsing fn Let's stop duplicating all the `sdf_parse_number` checks around everywhere. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/cspace.rs | 6 +- tool/microkit/src/sdf/domains.rs | 38 ++++----- tool/microkit/src/sdf/iommu.rs | 8 +- tool/microkit/src/sdf/memory_region.rs | 39 +++++----- tool/microkit/src/sdf/pd_vm.rs | 102 +++++++++---------------- tool/microkit/src/sdf/util.rs | 73 +++++++++++++++--- 6 files changed, 146 insertions(+), 120 deletions(-) diff --git a/tool/microkit/src/sdf/cspace.rs b/tool/microkit/src/sdf/cspace.rs index 61021f8be..7e37cc6e7 100644 --- a/tool/microkit/src/sdf/cspace.rs +++ b/tool/microkit/src/sdf/cspace.rs @@ -7,7 +7,9 @@ use std::rc::Rc; use super::consts::*; -use super::util::{check_attributes, checked_lookup, loc_string, sdf_parse_number, value_error}; +use super::util::{ + check_attributes, checked_lookup, loc_string, sdf_required_attribute_as_number, value_error, +}; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; #[derive(Debug, PartialEq, Eq, Copy, Clone)] @@ -47,7 +49,7 @@ impl CapMap { let pd = Rc::from(checked_lookup(xml_sdf, node, "pd")?); - let slot = sdf_parse_number(checked_lookup(xml_sdf, node, "slot")?, node)?; + let slot: u64 = sdf_required_attribute_as_number(xml_sdf, node, "slot")?; if slot == 0 { return Err(value_error( diff --git a/tool/microkit/src/sdf/domains.rs b/tool/microkit/src/sdf/domains.rs index 2c062b180..32c3382a5 100644 --- a/tool/microkit/src/sdf/domains.rs +++ b/tool/microkit/src/sdf/domains.rs @@ -9,7 +9,10 @@ use std::num::NonZero; use sel4_capdl_initializer_types::{DomainSchedDuration, DomainSchedEntry}; -use super::util::{check_attributes, checked_lookup, loc_string, sdf_parse_number, value_error}; +use super::util::{ + check_attributes, checked_lookup, loc_string, parse_number, sdf_attribute_as_number, + value_error, +}; use super::{SdfNode, SystemDescriptionFile}; use crate::Config; @@ -144,12 +147,9 @@ impl Domains { let name = checked_lookup(xml_sdf, node, "name")?.to_string(); - let domain_id = node - .attribute("id") - .map(|s| sdf_parse_number(s, node)) - .transpose()? - .map(|n| { - if n >= config.num_domains.into() { + let domain_id = sdf_attribute_as_number(xml_sdf, node, "id")? + .map(|n: u8| { + if n >= config.num_domains { Err(value_error( xml_sdf, node, @@ -160,8 +160,7 @@ impl Domains { ), )) } else { - Ok(n.try_into() - .expect("num_domains is u8 so by if above this is OK")) + Ok(n) } }) .transpose()?; @@ -177,18 +176,13 @@ impl Domains { ) -> Result { check_attributes(xml_sdf, node, &["index_shift", "start_index"])?; - let schedule_start_index = node - .attribute("start_index") - .map(|s| sdf_parse_number(s, node)) - .transpose()? + let schedule_start_index: u64 = sdf_attribute_as_number(xml_sdf, node, "start_index")? // The domain schedule is only started when the start index is Some(...) // so even when not specified we default to a start index of zero. .unwrap_or(0); - let schedule_index_shift = node - .attribute("index_shift") - .map(|s| sdf_parse_number(s, node)) - .transpose()?; + let schedule_index_shift: Option = + sdf_attribute_as_number(xml_sdf, node, "index_shift")?; let mut schedule = vec![]; @@ -291,7 +285,15 @@ impl Domains { ) })?; - let duration_int = sdf_parse_number(duration_raw, node)?; + let duration_int = parse_number(duration_raw).map_err(|err| { + format!( + "Error: failed to parse integer '{}' on element '{}': {}: {}", + duration_raw, + node.tag_name(), + err, + loc_string(xml_sdf, node.range().start), + ) + })?; let duration = NonZero::new(duration_int).ok_or_else(|| { value_error( xml_sdf, diff --git a/tool/microkit/src/sdf/iommu.rs b/tool/microkit/src/sdf/iommu.rs index 78514fa12..42aaa3cab 100644 --- a/tool/microkit/src/sdf/iommu.rs +++ b/tool/microkit/src/sdf/iommu.rs @@ -10,7 +10,9 @@ use std::str::FromStr; use super::memory_region::SysIOMap; use super::pci::{PciDevice, PciDeviceParseError}; -use super::util::{check_attributes, checked_lookup, loc_string, sdf_parse_number, value_error}; +use super::util::{ + check_attributes, checked_lookup, loc_string, sdf_required_attribute_as_number, value_error, +}; use super::{SdfNode, SystemDescriptionFile}; use crate::{sel4::Arch, Config}; @@ -104,8 +106,8 @@ impl IOAddressSpace { // http://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/vt-directed-io-spec.pdf let domain_id = match config.arch { Arch::X86_64 => { - let domain_id = - sdf_parse_number(checked_lookup(xml_sdf, node, "domain_id")?, node)?; + let domain_id = sdf_required_attribute_as_number(xml_sdf, node, "domain_id")?; + if !domain_ids.insert(domain_id) { return Err(value_error( xml_sdf, diff --git a/tool/microkit/src/sdf/memory_region.rs b/tool/microkit/src/sdf/memory_region.rs index e20077119..eeec6ec06 100644 --- a/tool/microkit/src/sdf/memory_region.rs +++ b/tool/microkit/src/sdf/memory_region.rs @@ -13,7 +13,10 @@ use sel4_capdl_initializer_types::FillEntryContentBootInfoId; use super::iommu::IommuDeviceIdentifier; use super::util::location_suffix_format; -use super::util::{check_attributes, checked_lookup, sdf_parse_number, value_error}; +use super::util::{ + check_attributes, checked_lookup, sdf_attribute_as_number, sdf_required_attribute_as_number, + value_error, +}; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; use crate::util::get_full_path; @@ -269,7 +272,7 @@ impl SysMap { check_attributes(xml_sdf, node, &attrs)?; let mr = checked_lookup(xml_sdf, node, "mr")?.to_string(); - let vaddr = sdf_parse_number(checked_lookup(xml_sdf, node, "vaddr")?, node)?; + let vaddr: u64 = sdf_required_attribute_as_number(xml_sdf, node, "vaddr")?; if vaddr >= max_vaddr { return Err(value_error( @@ -344,7 +347,7 @@ impl SysIOMap { check_attributes(xml_sdf, node, &attrs)?; let mr = checked_lookup(xml_sdf, node, "mr")?.to_string(); - let iovaddr = sdf_parse_number(checked_lookup(xml_sdf, node, "iovaddr")?, node)?; + let iovaddr = sdf_required_attribute_as_number(xml_sdf, node, "iovaddr")?; if iovaddr > x86_io_address_space::CAPDL_MAX_IOVA { return Err(value_error( @@ -394,11 +397,8 @@ impl SysMemoryRegion { prefill_bootinfo_maybe: Option, page_size: u64, ) -> Result { - match checked_lookup(xml_sdf, node, "size") { - Ok(size_str) => { - // Size explicitly specified - let size_parsed = sdf_parse_number(size_str, node)?; - + match sdf_attribute_as_number::(xml_sdf, node, "size")? { + Some(size_parsed) => { if !size_parsed.is_multiple_of(page_size) { return Err(value_error( xml_sdf, @@ -427,7 +427,7 @@ impl SysMemoryRegion { } } - Err(_) => { + None => { if prefill_bootinfo_maybe.is_some() { Ok(page_size) } else { @@ -468,12 +468,13 @@ impl SysMemoryRegion { let name = checked_lookup(xml_sdf, node, "name")?; let mut page_size_specified_by_user = false; - let page_size = if let Some(xml_page_size) = node.attribute("page_size") { - page_size_specified_by_user = true; - sdf_parse_number(xml_page_size, node)? - } else { - config.page_sizes()[0] - }; + let page_size = + if let Some(page_size) = sdf_attribute_as_number(xml_sdf, node, "page_size")? { + page_size_specified_by_user = true; + page_size + } else { + config.page_sizes()[0] + }; let page_size_valid = config.page_sizes().contains(&page_size); if !page_size_valid { @@ -558,12 +559,10 @@ impl SysMemoryRegion { page_size, )?; - let phys_addr = if let Some(xml_phys_addr) = node.attribute("phys_addr") { - SysMemoryRegionPaddr::Specified(sdf_parse_number(xml_phys_addr, node)?) - } else { + let phys_addr = sdf_attribute_as_number(xml_sdf, node, "phys_addr")? + .map(SysMemoryRegionPaddr::Specified) // At this point it is unsure whether this MR is a subject of a setvar region_paddr. - SysMemoryRegionPaddr::Unspecified - }; + .unwrap_or(SysMemoryRegionPaddr::Unspecified); if let SysMemoryRegionPaddr::Specified(sdf_paddr) = phys_addr { if !sdf_paddr.is_multiple_of(page_size) { diff --git a/tool/microkit/src/sdf/pd_vm.rs b/tool/microkit/src/sdf/pd_vm.rs index 91fb588ec..789377ade 100644 --- a/tool/microkit/src/sdf/pd_vm.rs +++ b/tool/microkit/src/sdf/pd_vm.rs @@ -17,7 +17,8 @@ use super::irq::{SysIrq, SysIrqKind}; use super::memory_region::SysMap; use super::pci::PciDevice; use super::util::{ - check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_parse_number, value_error, + check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_attribute_as_number, + sdf_required_attribute_as_number, value_error, }; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; @@ -164,7 +165,7 @@ impl ProtectionDomain { let name = Rc::from(checked_lookup(xml_sdf, node, "name")?); let (id, setvar_id) = if is_child { - let id = sdf_parse_number(checked_lookup(xml_sdf, node, "id")?, node)?; + let id = sdf_required_attribute_as_number(xml_sdf, node, "id")?; let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); (Some(id), setvar_id) } else { @@ -172,16 +173,10 @@ impl ProtectionDomain { }; // If we do not have an explicit budget the period is equal to the default budget. - let budget = if let Some(xml_budget) = node.attribute("budget") { - sdf_parse_number(xml_budget, node)? - } else { - BUDGET_DEFAULT - }; - let period = if let Some(xml_period) = node.attribute("period") { - sdf_parse_number(xml_period, node)? - } else { - budget - }; + let budget: u64 = + sdf_attribute_as_number(xml_sdf, node, "budget")?.unwrap_or(BUDGET_DEFAULT); + let period: u64 = sdf_attribute_as_number(xml_sdf, node, "period")?.unwrap_or(budget); + if budget > period { return Err(value_error( xml_sdf, @@ -205,11 +200,8 @@ impl ProtectionDomain { false }; - let stack_size = if let Some(xml_stack_size) = node.attribute("stack_size") { - sdf_parse_number(xml_stack_size, node)? - } else { - PD_DEFAULT_STACK_SIZE - }; + let stack_size: u64 = + sdf_attribute_as_number(xml_sdf, node, "stack_size")?.unwrap_or(PD_DEFAULT_STACK_SIZE); let smc = if let Some(xml_smc) = node.attribute("smc") { match str_to_bool(xml_smc) { @@ -242,11 +234,7 @@ impl ProtectionDomain { } } - let cpu = CpuCore( - sdf_parse_number(node.attribute("cpu").unwrap_or("0"), node)? - .try_into() - .expect("cpu core must be between 0 and 255"), - ); + let cpu = CpuCore(sdf_attribute_as_number(xml_sdf, node, "cpu")?.unwrap_or(0u8)); if cpu.0 >= config.num_cores { return Err(value_error( @@ -316,12 +304,8 @@ impl ProtectionDomain { let mut virtual_machine = None; let mut cspace = None; - // Default to minimum priority - let priority = if let Some(xml_priority) = node.attribute("priority") { - sdf_parse_number(xml_priority, node)? - } else { - 0 - }; + // Defaults to minimum priority + let priority: u64 = sdf_attribute_as_number(xml_sdf, node, "priority")?.unwrap_or(0); if priority > PD_MAX_PRIORITY as u64 { return Err(value_error( @@ -650,8 +634,7 @@ impl ProtectionDomain { checked_add_setvar(&mut setvars, setvar, xml_sdf, &*child)?; } - let addr = - sdf_parse_number(checked_lookup(xml_sdf, &*child, "addr")?, &*child)?; + let addr: u64 = sdf_required_attribute_as_number(xml_sdf, &*child, "addr")?; if let Some(setvar_addr) = child.attribute("setvar_addr") { let setvar = SysSetVar { @@ -903,16 +886,10 @@ impl VirtualMachine { let sched_params = if config.arch == Arch::Aarch64 { // If we do not have an explicit budget the period is equal to the default budget. - let budget = if let Some(xml_budget) = node.attribute("budget") { - sdf_parse_number(xml_budget, node)? - } else { - BUDGET_DEFAULT - }; - let period = if let Some(xml_period) = node.attribute("period") { - sdf_parse_number(xml_period, node)? - } else { - budget - }; + let budget: u64 = + sdf_attribute_as_number(xml_sdf, node, "budget")?.unwrap_or(BUDGET_DEFAULT); + let period: u64 = sdf_attribute_as_number(xml_sdf, node, "period")?.unwrap_or(budget); + if budget > period { return Err(value_error( xml_sdf, @@ -922,16 +899,10 @@ impl VirtualMachine { } // Default to minimum priority - let priority = if let Some(xml_priority) = node.attribute("priority") { - sdf_parse_number(xml_priority, node)? - } else { - 0 - }; + let priority: u8 = sdf_attribute_as_number(xml_sdf, node, "priority")?.unwrap_or(0); Some(SchedulingParams { - // This downcast is safe as we have checked that this is less than - // the maximum PD priority, which fits in a u8. - priority: priority as u8, + priority, budget, period, }) @@ -971,26 +942,23 @@ impl VirtualMachine { let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); - let cpu = if let Some(cpu) = child.attribute("cpu") { - let cpu_value: u8 = sdf_parse_number(cpu, node)? - .try_into() - .expect("cpu # fits in u8"); - - if cpu_value >= config.num_cores { - return Err(value_error( - xml_sdf, - &*child, - format!( - "cpu core must be less than {}, got {}", - config.num_cores, cpu_value - ), - )); - } + let cpu = + if let Some(cpu_value) = sdf_attribute_as_number(xml_sdf, node, "cpu")? { + if cpu_value >= config.num_cores { + return Err(value_error( + xml_sdf, + &*child, + format!( + "cpu core must be less than {}, got {}", + config.num_cores, cpu_value + ), + )); + } - Some(CpuCore(cpu_value)) - } else { - None - }; + Some(CpuCore(cpu_value)) + } else { + None + }; vcpus.push(VirtualCpu { id, setvar_id, cpu }); } diff --git a/tool/microkit/src/sdf/util.rs b/tool/microkit/src/sdf/util.rs index b28479e85..6a4b51463 100644 --- a/tool/microkit/src/sdf/util.rs +++ b/tool/microkit/src/sdf/util.rs @@ -4,14 +4,75 @@ // SPDX-License-Identifier: BSD-2-Clause // +use std::num::ParseIntError; + use super::{SdfLocation, SdfNode, SysSetVar, SystemDescriptionFile}; +/// Parse an 'attribute' of an `SdfNode` as a number of type T. +/// If the attribute does not exist, return an Optional value. +pub fn sdf_attribute_as_number( + sdf: &SystemDescriptionFile, + node: &dyn SdfNode, + attribute: &str, +) -> Result, String> { + let Some(value_str) = node.attribute(attribute) else { + return Ok(None); + }; + + parse_number(value_str).map(|v| Some(v)).map_err(|err| { + format!( + "Error: failed to parse integer '{}' on element '{}': {}: {}", + value_str, + node.tag_name(), + err, + loc_string(sdf, node.range().start), + ) + }) +} + +/// Parse an 'attribute' of an `SdfNode` as a number of type T. +/// If the attribute does not exist, return a neatly formatted error. +pub fn sdf_required_attribute_as_number( + sdf: &SystemDescriptionFile, + node: &dyn SdfNode, + attribute: &str, +) -> Result { + sdf_attribute_as_number(sdf, node, attribute)?.ok_or_else(|| { + format!( + "Error: Missing required attribute '{}' on element '{}': {}", + attribute, + node.tag_name(), + loc_string(sdf, node.range().start), + ) + }) +} + +/// This is annoying. Essentially, we can't do a generic over any number type +/// in rust, so we need to implement this marker trait which has the functions +/// we need. This is similar to the rust-num crate, but specialised for what +/// we need it for. +pub(super) trait IsNum: Sized { + fn from_str_radix(src: &str, radix: u32) -> Result; +} + +impl IsNum for u64 { + fn from_str_radix(src: &str, radix: u32) -> Result { + Self::from_str_radix(src, radix) + } +} + +impl IsNum for u8 { + fn from_str_radix(src: &str, radix: u32) -> Result { + Self::from_str_radix(src, radix) + } +} + /// The purpose of this function is to parse an integer that could /// either be in decimal or hex format, unlike the normal parsing /// functionality that the Rust standard library provides. /// This also removes any underscores that may be present in the number /// Always returns a base 10 integer. -pub fn sdf_parse_number(s: &str, node: &dyn SdfNode) -> Result { +pub fn parse_number(s: &str) -> Result { let mut to_parse = s.to_string(); to_parse.retain(|c| c != '_'); @@ -20,15 +81,7 @@ pub fn sdf_parse_number(s: &str, node: &dyn SdfNode) -> Result { None => (to_parse.as_str(), 10), }; - match u64::from_str_radix(final_str, base) { - Ok(value) => Ok(value), - Err(err) => Err(format!( - "Error: failed to parse integer '{}' on element '{}': {}", - s, - node.tag_name(), - err - )), - } + T::from_str_radix(final_str, base) } pub fn loc_string(xml_sdf: &SystemDescriptionFile, pos: SdfLocation) -> String { From ff51ccdff070810b16dc7fe0481dba2f917c5bcb Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 14:44:43 +1000 Subject: [PATCH 2/7] tool: catch edge-case with VM priority >255 Somehow this was missed, so this would instead fail at runtime. Add tests to check this. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/pd_vm.rs | 8 ++++++++ .../tests/sdf/pd_invalid_priority.system | 11 +++++++++++ .../tests/sdf/vm_with_priority_invalid.system | 13 +++++++++++++ tool/microkit/tests/test.rs | 18 ++++++++++++++++++ 4 files changed, 50 insertions(+) create mode 100644 tool/microkit/tests/sdf/pd_invalid_priority.system create mode 100644 tool/microkit/tests/sdf/vm_with_priority_invalid.system diff --git a/tool/microkit/src/sdf/pd_vm.rs b/tool/microkit/src/sdf/pd_vm.rs index 789377ade..544e0bb74 100644 --- a/tool/microkit/src/sdf/pd_vm.rs +++ b/tool/microkit/src/sdf/pd_vm.rs @@ -901,6 +901,14 @@ impl VirtualMachine { // Default to minimum priority let priority: u8 = sdf_attribute_as_number(xml_sdf, node, "priority")?.unwrap_or(0); + if priority > PD_MAX_PRIORITY { + return Err(value_error( + xml_sdf, + node, + format!("priority must be between 0 and {PD_MAX_PRIORITY}"), + )); + } + Some(SchedulingParams { priority, budget, diff --git a/tool/microkit/tests/sdf/pd_invalid_priority.system b/tool/microkit/tests/sdf/pd_invalid_priority.system new file mode 100644 index 000000000..e55ffea2a --- /dev/null +++ b/tool/microkit/tests/sdf/pd_invalid_priority.system @@ -0,0 +1,11 @@ + + + + + + + diff --git a/tool/microkit/tests/sdf/vm_with_priority_invalid.system b/tool/microkit/tests/sdf/vm_with_priority_invalid.system new file mode 100644 index 000000000..c20dc2734 --- /dev/null +++ b/tool/microkit/tests/sdf/vm_with_priority_invalid.system @@ -0,0 +1,13 @@ + + + + + + + + + + diff --git a/tool/microkit/tests/test.rs b/tool/microkit/tests/test.rs index 33758ba24..de844a301 100644 --- a/tool/microkit/tests/test.rs +++ b/tool/microkit/tests/test.rs @@ -765,6 +765,15 @@ mod protection_domain { "Error: cpu core must be less than 1, got 10 on element 'protection_domain':", ) } + + #[test] + fn test_invalid_priority() { + check_error( + &DEFAULT_AARCH64_KERNEL_CONFIG, + "pd_invalid_priority.system", + "Error: priority must be between 0 and 254 on element 'protection_domain':", + ); + } } #[cfg(test)] @@ -1009,6 +1018,15 @@ mod virtual_machine { check_success(&DEFAULT_AARCH64_KERNEL_CONFIG, "vm_with_priority.system") } + #[test] + fn test_vm_with_priority_invalid_aarch64() { + check_error( + &DEFAULT_AARCH64_KERNEL_CONFIG, + "vm_with_priority_invalid.system", + "Error: priority must be between 0 and 254 on element 'virtual_machine':", + ) + } + #[test] fn test_vm_valid_x86_64() { check_success(&DEFAULT_X86_64_KERNEL_CONFIG, "vm_valid.system") From 3419ef89fe2901b3e40d5a5a3985b725f6ac803d Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 15:06:45 +1000 Subject: [PATCH 3/7] tool(cleanup): add attribute-as-bool parsing fn Stop duplicating this logic everywhere. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/channels.rs | 27 +++----------- tool/microkit/src/sdf/memory_region.rs | 21 +++-------- tool/microkit/src/sdf/pd_vm.rs | 50 +++----------------------- tool/microkit/src/sdf/util.rs | 49 +++++++++++++++++++++++++ tool/microkit/src/util.rs | 8 ----- tool/microkit/tests/test.rs | 6 ++-- 6 files changed, 66 insertions(+), 95 deletions(-) diff --git a/tool/microkit/src/sdf/channels.rs b/tool/microkit/src/sdf/channels.rs index 2eabfa02e..50712766e 100644 --- a/tool/microkit/src/sdf/channels.rs +++ b/tool/microkit/src/sdf/channels.rs @@ -9,11 +9,11 @@ use std::rc::Rc; use super::consts::*; use super::pd_vm::ProtectionDomain; -use super::util::{check_attributes, checked_lookup, loc_string, value_error}; +use super::util::{ + check_attributes, checked_lookup, loc_string, sdf_attribute_as_bool, value_error, +}; use super::{SdfNode, SystemDescriptionFile}; -use crate::util::str_to_bool; - #[derive(Debug, Clone)] pub struct ChannelEnd { pub pd: Rc, @@ -61,25 +61,8 @@ impl ChannelEnd { return Err(value_error(xml_sdf, node, "id must be >= 0".to_string())); } - let notify = node - .attribute("notify") - .map(str_to_bool) - .unwrap_or(Some(true)) - .ok_or_else(|| { - value_error( - xml_sdf, - node, - "notify must be 'true' or 'false'".to_string(), - ) - })?; - - let pp = node - .attribute("pp") - .map(str_to_bool) - .unwrap_or(Some(false)) - .ok_or_else(|| { - value_error(xml_sdf, node, "pp must be 'true' or 'false'".to_string()) - })?; + let notify = sdf_attribute_as_bool(xml_sdf, node, "notify")?.unwrap_or(true); + let pp = sdf_attribute_as_bool(xml_sdf, node, "pp")?.unwrap_or(false); if let Some(pd) = pds.get(end_pd) { let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); diff --git a/tool/microkit/src/sdf/memory_region.rs b/tool/microkit/src/sdf/memory_region.rs index eeec6ec06..92174aa01 100644 --- a/tool/microkit/src/sdf/memory_region.rs +++ b/tool/microkit/src/sdf/memory_region.rs @@ -14,14 +14,13 @@ use sel4_capdl_initializer_types::FillEntryContentBootInfoId; use super::iommu::IommuDeviceIdentifier; use super::util::location_suffix_format; use super::util::{ - check_attributes, checked_lookup, sdf_attribute_as_number, sdf_required_attribute_as_number, - value_error, + check_attributes, checked_lookup, sdf_attribute_as_bool, sdf_attribute_as_number, + sdf_required_attribute_as_number, value_error, }; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; use crate::util::get_full_path; use crate::util::round_up; -use crate::util::str_to_bool; use crate::{Config, PageSize}; #[repr(u8)] @@ -307,21 +306,9 @@ impl SysMap { )); } - let cached = if let Some(xml_cached) = node.attribute("cached") { - match str_to_bool(xml_cached) { - Some(val) => val, - None => { - return Err(value_error( - xml_sdf, - node, - "cached must be 'true' or 'false'".to_string(), - )) - } - } - } else { + let cached = sdf_attribute_as_bool(xml_sdf, node, "cached")? // Default to cached - true - }; + .unwrap_or(true); Ok(SysMap { mr, diff --git a/tool/microkit/src/sdf/pd_vm.rs b/tool/microkit/src/sdf/pd_vm.rs index 544e0bb74..fc8ef47f8 100644 --- a/tool/microkit/src/sdf/pd_vm.rs +++ b/tool/microkit/src/sdf/pd_vm.rs @@ -17,13 +17,12 @@ use super::irq::{SysIrq, SysIrqKind}; use super::memory_region::SysMap; use super::pci::PciDevice; use super::util::{ - check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_attribute_as_number, - sdf_required_attribute_as_number, value_error, + check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_attribute_as_bool, + sdf_attribute_as_number, sdf_required_attribute_as_number, value_error, }; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; use crate::sel4::{Arch, ArmRiscvIrqTrigger, X86IoapicIrqPolarity, X86IoapicIrqTrigger}; -use crate::util::str_to_bool; use crate::Config; #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] @@ -185,38 +184,12 @@ impl ProtectionDomain { )); } - let passive = if let Some(xml_passive) = node.attribute("passive") { - match str_to_bool(xml_passive) { - Some(val) => val, - None => { - return Err(value_error( - xml_sdf, - node, - "passive must be 'true' or 'false'".to_string(), - )) - } - } - } else { - false - }; + let passive = sdf_attribute_as_bool(xml_sdf, node, "passive")?.unwrap_or(false); let stack_size: u64 = sdf_attribute_as_number(xml_sdf, node, "stack_size")?.unwrap_or(PD_DEFAULT_STACK_SIZE); - let smc = if let Some(xml_smc) = node.attribute("smc") { - match str_to_bool(xml_smc) { - Some(val) => val, - None => { - return Err(value_error( - xml_sdf, - node, - "smc must be 'true' or 'false'".to_string(), - )) - } - } - } else { - false - }; + let smc = sdf_attribute_as_bool(xml_sdf, node, "smc")?.unwrap_or(false); if smc { match config.arm_smc { @@ -316,20 +289,7 @@ impl ProtectionDomain { } // FPU is enabled by default - let fpu = if let Some(xml_fpu) = node.attribute("fpu") { - match str_to_bool(xml_fpu) { - Some(val) => val, - None => { - return Err(value_error( - xml_sdf, - node, - "fpu must be 'true' or 'false'".to_string(), - )) - } - } - } else { - true - }; + let fpu = sdf_attribute_as_bool(xml_sdf, node, "fpu")?.unwrap_or(true); for child in node.children() { match child.tag_name() { diff --git a/tool/microkit/src/sdf/util.rs b/tool/microkit/src/sdf/util.rs index 6a4b51463..be94e47bd 100644 --- a/tool/microkit/src/sdf/util.rs +++ b/tool/microkit/src/sdf/util.rs @@ -47,6 +47,46 @@ pub fn sdf_required_attribute_as_number( }) } +/// Parse an 'attribute' of an `SdfNode` as a boolean. +/// If the attribute does not exist, return an Optional value. +pub fn sdf_attribute_as_bool( + sdf: &SystemDescriptionFile, + node: &dyn SdfNode, + attribute: &str, +) -> Result, String> { + let Some(value_str) = node.attribute(attribute) else { + return Ok(None); + }; + + parse_bool(value_str).map(Some).map_err(|_| { + format!( + "Error: '{}' must be 'true' or 'false', got '{}' on element '{}': {}", + attribute, + value_str, + node.tag_name(), + loc_string(sdf, node.range().start), + ) + }) +} + +/// Parse an 'attribute' of an `SdfNode` as a boolean. +/// If the attribute does not exist, return a neatly formatted error. +#[expect(unused)] +pub fn sdf_required_attribute_as_bool( + sdf: &SystemDescriptionFile, + node: &dyn SdfNode, + attribute: &str, +) -> Result { + sdf_attribute_as_bool(sdf, node, attribute)?.ok_or_else(|| { + format!( + "Error: Missing required attribute '{}' on element '{}': {}", + attribute, + node.tag_name(), + loc_string(sdf, node.range().start), + ) + }) +} + /// This is annoying. Essentially, we can't do a generic over any number type /// in rust, so we need to implement this marker trait which has the functions /// we need. This is similar to the rust-num crate, but specialised for what @@ -84,6 +124,15 @@ pub fn parse_number(s: &str) -> Result { T::from_str_radix(final_str, base) } +// Parse a string as a boolean with the values "true" or "false" +pub fn parse_bool(s: &str) -> Result { + match s { + "true" => Ok(true), + "false" => Ok(false), + _ => Err(()), + } +} + pub fn loc_string(xml_sdf: &SystemDescriptionFile, pos: SdfLocation) -> String { format!("{}:{}:{}", xml_sdf.filename.display(), pos.row, pos.col) } diff --git a/tool/microkit/src/util.rs b/tool/microkit/src/util.rs index f5672b25c..24ebb6470 100644 --- a/tool/microkit/src/util.rs +++ b/tool/microkit/src/util.rs @@ -17,14 +17,6 @@ pub fn lsb(x: u64) -> u64 { x.trailing_zeros() as u64 } -pub fn str_to_bool(s: &str) -> Option { - match s { - "true" => Some(true), - "false" => Some(false), - _ => None, - } -} - pub const fn kb(n: u64) -> u64 { n * 1024 } diff --git a/tool/microkit/tests/test.rs b/tool/microkit/tests/test.rs index de844a301..93af9f059 100644 --- a/tool/microkit/tests/test.rs +++ b/tool/microkit/tests/test.rs @@ -1219,7 +1219,7 @@ mod channel { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "ch_end_invalid_pp.system", - "Error: pp must be 'true' or 'false' on element 'end': ", + "Error: 'pp' must be 'true' or 'false', got 'no' on element 'end': ", ) } @@ -1228,7 +1228,7 @@ mod channel { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "ch_end_invalid_notify.system", - "Error: notify must be 'true' or 'false' on element 'end': ", + "Error: 'notify' must be 'true' or 'false', got 'no' on element 'end': ", ) } @@ -1474,7 +1474,7 @@ mod system { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "wrong_fpu_flag_value.system", - "Error: fpu must be 'true' or 'false'", + "Error: 'fpu' must be 'true' or 'false', got 'foo' on element 'protection_domain': ", ) } From 77d4b45a3fe6300ad8ec2c2430276d06ff04f827 Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 15:23:35 +1000 Subject: [PATCH 4/7] tool(util): implement IsNum for i64 Implement the IsNum trait via a macro to stop duplicating the same code so often. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/util.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tool/microkit/src/sdf/util.rs b/tool/microkit/src/sdf/util.rs index be94e47bd..cb0e2d211 100644 --- a/tool/microkit/src/sdf/util.rs +++ b/tool/microkit/src/sdf/util.rs @@ -95,17 +95,19 @@ pub(super) trait IsNum: Sized { fn from_str_radix(src: &str, radix: u32) -> Result; } -impl IsNum for u64 { - fn from_str_radix(src: &str, radix: u32) -> Result { - Self::from_str_radix(src, radix) - } +macro_rules! impl_is_num { + ($t:ty) => { + impl IsNum for $t { + fn from_str_radix(src: &str, radix: u32) -> Result { + Self::from_str_radix(src, radix) + } + } + }; } -impl IsNum for u8 { - fn from_str_radix(src: &str, radix: u32) -> Result { - Self::from_str_radix(src, radix) - } -} +impl_is_num!(u64); +impl_is_num!(u8); +impl_is_num!(i64); /// The purpose of this function is to parse an integer that could /// either be in decimal or hex format, unlike the normal parsing From 84f343ec9b14dff4782516b8808d785e73aae366 Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 15:27:32 +1000 Subject: [PATCH 5/7] tool(cleanup): use sdf_attribute_as_number helper Previously, this was using `::parse()` or `::parse()` which will panic with no nice error message. The code surrounding these should be refactored too, as the checks about "> 0" and range checks can be done as helpers. This is done as a later commit, for now, it is left as 'i64' and casts are done later. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/channels.rs | 5 +-- tool/microkit/src/sdf/pd_vm.rs | 56 ++++++++++++++----------------- 2 files changed, 28 insertions(+), 33 deletions(-) diff --git a/tool/microkit/src/sdf/channels.rs b/tool/microkit/src/sdf/channels.rs index 50712766e..4e93f42d0 100644 --- a/tool/microkit/src/sdf/channels.rs +++ b/tool/microkit/src/sdf/channels.rs @@ -10,7 +10,8 @@ use std::rc::Rc; use super::consts::*; use super::pd_vm::ProtectionDomain; use super::util::{ - check_attributes, checked_lookup, loc_string, sdf_attribute_as_bool, value_error, + check_attributes, checked_lookup, loc_string, sdf_attribute_as_bool, + sdf_required_attribute_as_number, value_error, }; use super::{SdfNode, SystemDescriptionFile}; @@ -47,7 +48,7 @@ impl ChannelEnd { check_attributes(xml_sdf, node, &["pd", "id", "pp", "notify", "setvar_id"])?; let end_pd = checked_lookup(xml_sdf, node, "pd")?; - let end_id = checked_lookup(xml_sdf, node, "id")?.parse::().unwrap(); + let end_id: i64 = sdf_required_attribute_as_number(xml_sdf, node, "id")?; if end_id > PD_MAX_ID as i64 { return Err(value_error( diff --git a/tool/microkit/src/sdf/pd_vm.rs b/tool/microkit/src/sdf/pd_vm.rs index fc8ef47f8..89cb67b65 100644 --- a/tool/microkit/src/sdf/pd_vm.rs +++ b/tool/microkit/src/sdf/pd_vm.rs @@ -340,9 +340,8 @@ impl ProtectionDomain { maps.push(map); } "irq" => { - let id = checked_lookup(xml_sdf, &*child, "id")? - .parse::() - .unwrap(); + let id: i64 = sdf_required_attribute_as_number(xml_sdf, &*child, "id")?; + if id > PD_MAX_ID as i64 { return Err(value_error( xml_sdf, @@ -362,7 +361,7 @@ impl ProtectionDomain { checked_add_setvar(&mut setvars, setvar, xml_sdf, &*child)?; } - if let Some(irq_str) = child.attribute("irq") { + if let Some(irq) = sdf_attribute_as_number(xml_sdf, &*child, "irq")? { if config.arch == Arch::X86_64 { return Err(value_error( xml_sdf, @@ -373,7 +372,7 @@ impl ProtectionDomain { // ARM and RISC-V interrupts must have an "irq" attribute. check_attributes(xml_sdf, &*child, &["irq", "id", "setvar_id", "trigger"])?; - let irq = irq_str.parse::().unwrap(); + let trigger = if let Some(trigger_str) = child.attribute("trigger") { match trigger_str { "level" => ArmRiscvIrqTrigger::Level, @@ -395,7 +394,9 @@ impl ProtectionDomain { kind: SysIrqKind::Conventional { irq, trigger }, }; irqs.push(irq); - } else if let Some(pin_str) = child.attribute("pin") { + } else if let Some(pin) = + sdf_attribute_as_number::(xml_sdf, &*child, "pin")? + { if config.arch != Arch::X86_64 { return Err(value_error( xml_sdf, @@ -419,12 +420,10 @@ impl ProtectionDomain { ], )?; - let ioapic = if let Some(ioapic_str) = child.attribute("ioapic") { - ioapic_str.parse::().unwrap() - } else { - // Default to the first unit. - 0 - }; + // Default to the first unit. + let ioapic: i64 = + sdf_attribute_as_number(xml_sdf, &*child, "ioapic")?.unwrap_or(0); + if ioapic < 0 { return Err(value_error( xml_sdf, @@ -433,7 +432,6 @@ impl ProtectionDomain { )); } - let pin = pin_str.parse::().unwrap(); if pin < 0 { return Err(value_error( xml_sdf, @@ -474,9 +472,10 @@ impl ProtectionDomain { // Default to normal polarity X86IoapicIrqPolarity::HighTriggered }; - let vector = checked_lookup(xml_sdf, &*child, "vector")? - .parse::() - .unwrap(); + + let vector: i64 = + sdf_required_attribute_as_number(xml_sdf, &*child, "vector")?; + if !(0..=X86_IRQ_VECTOR_MAX).contains(&vector) { return Err(value_error( xml_sdf, @@ -515,9 +514,8 @@ impl ProtectionDomain { let pci_device = PciDevice::from_str(pcidev_str) .map_err(|err| value_error(xml_sdf, &*child, err.to_string()))?; - let handle = checked_lookup(xml_sdf, &*child, "handle")? - .parse::() - .unwrap(); + let handle: i64 = + sdf_required_attribute_as_number(xml_sdf, &*child, "handle")?; if handle < 0 { return Err(value_error( xml_sdf, @@ -526,9 +524,9 @@ impl ProtectionDomain { )); } - let vector = checked_lookup(xml_sdf, &*child, "vector")? - .parse::() - .unwrap(); + let vector: i64 = + sdf_required_attribute_as_number(xml_sdf, &*child, "vector")?; + if !(0..=X86_IRQ_VECTOR_MAX).contains(&vector) { return Err(value_error( xml_sdf, @@ -568,9 +566,8 @@ impl ProtectionDomain { &["id", "setvar_id", "setvar_addr", "addr", "size"], )?; - let id = checked_lookup(xml_sdf, &*child, "id")? - .parse::() - .unwrap(); + let id: i64 = sdf_required_attribute_as_number(xml_sdf, &*child, "id")?; + if id > PD_MAX_ID as i64 { return Err(value_error( xml_sdf, @@ -604,9 +601,7 @@ impl ProtectionDomain { checked_add_setvar(&mut setvars, setvar, xml_sdf, &*child)?; } - let size = checked_lookup(xml_sdf, &*child, "size")? - .parse::() - .unwrap(); + let size: i64 = sdf_required_attribute_as_number(xml_sdf, &*child, "size")?; if size <= 0 { return Err(value_error( xml_sdf, @@ -885,9 +880,8 @@ impl VirtualMachine { match child_name { "vcpu" => { check_attributes(xml_sdf, &*child, &["id", "setvar_id", "cpu"])?; - let id = checked_lookup(xml_sdf, &*child, "id")? - .parse::() - .unwrap(); + let id = sdf_required_attribute_as_number(xml_sdf, &*child, "id")?; + if id > VCPU_MAX_ID { return Err(value_error( xml_sdf, From f3b7fe3fdfbdf3f18fde0fbf6f800637becdd997 Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 16:09:11 +1000 Subject: [PATCH 6/7] tool(cleanup): make parse_attribute type generic This allows arbitrary types to work and be parsed without needing to duplicate the code for each type. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/channels.rs | 10 +-- tool/microkit/src/sdf/cspace.rs | 4 +- tool/microkit/src/sdf/domains.rs | 10 ++- tool/microkit/src/sdf/iommu.rs | 4 +- tool/microkit/src/sdf/memory_region.rs | 27 ++++---- tool/microkit/src/sdf/pd_vm.rs | 87 ++++++++++++-------------- tool/microkit/src/sdf/util.rs | 78 ++++++++++------------- tool/microkit/tests/test.rs | 20 +++--- 8 files changed, 112 insertions(+), 128 deletions(-) diff --git a/tool/microkit/src/sdf/channels.rs b/tool/microkit/src/sdf/channels.rs index 4e93f42d0..2940663e6 100644 --- a/tool/microkit/src/sdf/channels.rs +++ b/tool/microkit/src/sdf/channels.rs @@ -10,8 +10,8 @@ use std::rc::Rc; use super::consts::*; use super::pd_vm::ProtectionDomain; use super::util::{ - check_attributes, checked_lookup, loc_string, sdf_attribute_as_bool, - sdf_required_attribute_as_number, value_error, + check_attributes, checked_lookup, loc_string, sdf_parse_attribute, + sdf_parse_required_attribute, value_error, }; use super::{SdfNode, SystemDescriptionFile}; @@ -48,7 +48,7 @@ impl ChannelEnd { check_attributes(xml_sdf, node, &["pd", "id", "pp", "notify", "setvar_id"])?; let end_pd = checked_lookup(xml_sdf, node, "pd")?; - let end_id: i64 = sdf_required_attribute_as_number(xml_sdf, node, "id")?; + let end_id: i64 = sdf_parse_required_attribute(xml_sdf, node, "id")?; if end_id > PD_MAX_ID as i64 { return Err(value_error( @@ -62,8 +62,8 @@ impl ChannelEnd { return Err(value_error(xml_sdf, node, "id must be >= 0".to_string())); } - let notify = sdf_attribute_as_bool(xml_sdf, node, "notify")?.unwrap_or(true); - let pp = sdf_attribute_as_bool(xml_sdf, node, "pp")?.unwrap_or(false); + let notify = sdf_parse_attribute(xml_sdf, node, "notify")?.unwrap_or(true); + let pp = sdf_parse_attribute(xml_sdf, node, "pp")?.unwrap_or(false); if let Some(pd) = pds.get(end_pd) { let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); diff --git a/tool/microkit/src/sdf/cspace.rs b/tool/microkit/src/sdf/cspace.rs index 7e37cc6e7..50f096520 100644 --- a/tool/microkit/src/sdf/cspace.rs +++ b/tool/microkit/src/sdf/cspace.rs @@ -8,7 +8,7 @@ use std::rc::Rc; use super::consts::*; use super::util::{ - check_attributes, checked_lookup, loc_string, sdf_required_attribute_as_number, value_error, + check_attributes, checked_lookup, loc_string, sdf_parse_required_attribute, value_error, }; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; @@ -49,7 +49,7 @@ impl CapMap { let pd = Rc::from(checked_lookup(xml_sdf, node, "pd")?); - let slot: u64 = sdf_required_attribute_as_number(xml_sdf, node, "slot")?; + let slot: u64 = sdf_parse_required_attribute(xml_sdf, node, "slot")?; if slot == 0 { return Err(value_error( diff --git a/tool/microkit/src/sdf/domains.rs b/tool/microkit/src/sdf/domains.rs index 32c3382a5..792e032dd 100644 --- a/tool/microkit/src/sdf/domains.rs +++ b/tool/microkit/src/sdf/domains.rs @@ -10,8 +10,7 @@ use std::num::NonZero; use sel4_capdl_initializer_types::{DomainSchedDuration, DomainSchedEntry}; use super::util::{ - check_attributes, checked_lookup, loc_string, parse_number, sdf_attribute_as_number, - value_error, + check_attributes, checked_lookup, loc_string, parse_number, sdf_parse_attribute, value_error, }; use super::{SdfNode, SystemDescriptionFile}; @@ -147,7 +146,7 @@ impl Domains { let name = checked_lookup(xml_sdf, node, "name")?.to_string(); - let domain_id = sdf_attribute_as_number(xml_sdf, node, "id")? + let domain_id = sdf_parse_attribute(xml_sdf, node, "id")? .map(|n: u8| { if n >= config.num_domains { Err(value_error( @@ -176,13 +175,12 @@ impl Domains { ) -> Result { check_attributes(xml_sdf, node, &["index_shift", "start_index"])?; - let schedule_start_index: u64 = sdf_attribute_as_number(xml_sdf, node, "start_index")? + let schedule_start_index: u64 = sdf_parse_attribute(xml_sdf, node, "start_index")? // The domain schedule is only started when the start index is Some(...) // so even when not specified we default to a start index of zero. .unwrap_or(0); - let schedule_index_shift: Option = - sdf_attribute_as_number(xml_sdf, node, "index_shift")?; + let schedule_index_shift: Option = sdf_parse_attribute(xml_sdf, node, "index_shift")?; let mut schedule = vec![]; diff --git a/tool/microkit/src/sdf/iommu.rs b/tool/microkit/src/sdf/iommu.rs index 42aaa3cab..a388f22ca 100644 --- a/tool/microkit/src/sdf/iommu.rs +++ b/tool/microkit/src/sdf/iommu.rs @@ -11,7 +11,7 @@ use std::str::FromStr; use super::memory_region::SysIOMap; use super::pci::{PciDevice, PciDeviceParseError}; use super::util::{ - check_attributes, checked_lookup, loc_string, sdf_required_attribute_as_number, value_error, + check_attributes, checked_lookup, loc_string, sdf_parse_required_attribute, value_error, }; use super::{SdfNode, SystemDescriptionFile}; @@ -106,7 +106,7 @@ impl IOAddressSpace { // http://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/vt-directed-io-spec.pdf let domain_id = match config.arch { Arch::X86_64 => { - let domain_id = sdf_required_attribute_as_number(xml_sdf, node, "domain_id")?; + let domain_id = sdf_parse_required_attribute(xml_sdf, node, "domain_id")?; if !domain_ids.insert(domain_id) { return Err(value_error( diff --git a/tool/microkit/src/sdf/memory_region.rs b/tool/microkit/src/sdf/memory_region.rs index 92174aa01..557354220 100644 --- a/tool/microkit/src/sdf/memory_region.rs +++ b/tool/microkit/src/sdf/memory_region.rs @@ -14,8 +14,8 @@ use sel4_capdl_initializer_types::FillEntryContentBootInfoId; use super::iommu::IommuDeviceIdentifier; use super::util::location_suffix_format; use super::util::{ - check_attributes, checked_lookup, sdf_attribute_as_bool, sdf_attribute_as_number, - sdf_required_attribute_as_number, value_error, + check_attributes, checked_lookup, sdf_parse_attribute, sdf_parse_required_attribute, + value_error, }; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; @@ -271,7 +271,7 @@ impl SysMap { check_attributes(xml_sdf, node, &attrs)?; let mr = checked_lookup(xml_sdf, node, "mr")?.to_string(); - let vaddr: u64 = sdf_required_attribute_as_number(xml_sdf, node, "vaddr")?; + let vaddr: u64 = sdf_parse_required_attribute(xml_sdf, node, "vaddr")?; if vaddr >= max_vaddr { return Err(value_error( @@ -306,7 +306,7 @@ impl SysMap { )); } - let cached = sdf_attribute_as_bool(xml_sdf, node, "cached")? + let cached = sdf_parse_attribute(xml_sdf, node, "cached")? // Default to cached .unwrap_or(true); @@ -334,7 +334,7 @@ impl SysIOMap { check_attributes(xml_sdf, node, &attrs)?; let mr = checked_lookup(xml_sdf, node, "mr")?.to_string(); - let iovaddr = sdf_required_attribute_as_number(xml_sdf, node, "iovaddr")?; + let iovaddr = sdf_parse_required_attribute(xml_sdf, node, "iovaddr")?; if iovaddr > x86_io_address_space::CAPDL_MAX_IOVA { return Err(value_error( @@ -384,7 +384,7 @@ impl SysMemoryRegion { prefill_bootinfo_maybe: Option, page_size: u64, ) -> Result { - match sdf_attribute_as_number::(xml_sdf, node, "size")? { + match sdf_parse_attribute::(xml_sdf, node, "size")? { Some(size_parsed) => { if !size_parsed.is_multiple_of(page_size) { return Err(value_error( @@ -455,13 +455,12 @@ impl SysMemoryRegion { let name = checked_lookup(xml_sdf, node, "name")?; let mut page_size_specified_by_user = false; - let page_size = - if let Some(page_size) = sdf_attribute_as_number(xml_sdf, node, "page_size")? { - page_size_specified_by_user = true; - page_size - } else { - config.page_sizes()[0] - }; + let page_size = if let Some(page_size) = sdf_parse_attribute(xml_sdf, node, "page_size")? { + page_size_specified_by_user = true; + page_size + } else { + config.page_sizes()[0] + }; let page_size_valid = config.page_sizes().contains(&page_size); if !page_size_valid { @@ -546,7 +545,7 @@ impl SysMemoryRegion { page_size, )?; - let phys_addr = sdf_attribute_as_number(xml_sdf, node, "phys_addr")? + let phys_addr = sdf_parse_attribute(xml_sdf, node, "phys_addr")? .map(SysMemoryRegionPaddr::Specified) // At this point it is unsure whether this MR is a subject of a setvar region_paddr. .unwrap_or(SysMemoryRegionPaddr::Unspecified); diff --git a/tool/microkit/src/sdf/pd_vm.rs b/tool/microkit/src/sdf/pd_vm.rs index 89cb67b65..fdf123d5e 100644 --- a/tool/microkit/src/sdf/pd_vm.rs +++ b/tool/microkit/src/sdf/pd_vm.rs @@ -17,8 +17,8 @@ use super::irq::{SysIrq, SysIrqKind}; use super::memory_region::SysMap; use super::pci::PciDevice; use super::util::{ - check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_attribute_as_bool, - sdf_attribute_as_number, sdf_required_attribute_as_number, value_error, + check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_parse_attribute, + sdf_parse_required_attribute, value_error, }; use super::{SdfLocation, SdfNode, SystemDescriptionFile}; @@ -164,7 +164,7 @@ impl ProtectionDomain { let name = Rc::from(checked_lookup(xml_sdf, node, "name")?); let (id, setvar_id) = if is_child { - let id = sdf_required_attribute_as_number(xml_sdf, node, "id")?; + let id = sdf_parse_required_attribute(xml_sdf, node, "id")?; let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); (Some(id), setvar_id) } else { @@ -172,9 +172,8 @@ impl ProtectionDomain { }; // If we do not have an explicit budget the period is equal to the default budget. - let budget: u64 = - sdf_attribute_as_number(xml_sdf, node, "budget")?.unwrap_or(BUDGET_DEFAULT); - let period: u64 = sdf_attribute_as_number(xml_sdf, node, "period")?.unwrap_or(budget); + let budget: u64 = sdf_parse_attribute(xml_sdf, node, "budget")?.unwrap_or(BUDGET_DEFAULT); + let period: u64 = sdf_parse_attribute(xml_sdf, node, "period")?.unwrap_or(budget); if budget > period { return Err(value_error( @@ -184,12 +183,12 @@ impl ProtectionDomain { )); } - let passive = sdf_attribute_as_bool(xml_sdf, node, "passive")?.unwrap_or(false); + let passive = sdf_parse_attribute(xml_sdf, node, "passive")?.unwrap_or(false); let stack_size: u64 = - sdf_attribute_as_number(xml_sdf, node, "stack_size")?.unwrap_or(PD_DEFAULT_STACK_SIZE); + sdf_parse_attribute(xml_sdf, node, "stack_size")?.unwrap_or(PD_DEFAULT_STACK_SIZE); - let smc = sdf_attribute_as_bool(xml_sdf, node, "smc")?.unwrap_or(false); + let smc = sdf_parse_attribute(xml_sdf, node, "smc")?.unwrap_or(false); if smc { match config.arm_smc { @@ -207,7 +206,7 @@ impl ProtectionDomain { } } - let cpu = CpuCore(sdf_attribute_as_number(xml_sdf, node, "cpu")?.unwrap_or(0u8)); + let cpu = CpuCore(sdf_parse_attribute(xml_sdf, node, "cpu")?.unwrap_or(0u8)); if cpu.0 >= config.num_cores { return Err(value_error( @@ -278,7 +277,7 @@ impl ProtectionDomain { let mut cspace = None; // Defaults to minimum priority - let priority: u64 = sdf_attribute_as_number(xml_sdf, node, "priority")?.unwrap_or(0); + let priority: u64 = sdf_parse_attribute(xml_sdf, node, "priority")?.unwrap_or(0); if priority > PD_MAX_PRIORITY as u64 { return Err(value_error( @@ -289,7 +288,7 @@ impl ProtectionDomain { } // FPU is enabled by default - let fpu = sdf_attribute_as_bool(xml_sdf, node, "fpu")?.unwrap_or(true); + let fpu = sdf_parse_attribute(xml_sdf, node, "fpu")?.unwrap_or(true); for child in node.children() { match child.tag_name() { @@ -340,7 +339,7 @@ impl ProtectionDomain { maps.push(map); } "irq" => { - let id: i64 = sdf_required_attribute_as_number(xml_sdf, &*child, "id")?; + let id: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "id")?; if id > PD_MAX_ID as i64 { return Err(value_error( @@ -361,7 +360,7 @@ impl ProtectionDomain { checked_add_setvar(&mut setvars, setvar, xml_sdf, &*child)?; } - if let Some(irq) = sdf_attribute_as_number(xml_sdf, &*child, "irq")? { + if let Some(irq) = sdf_parse_attribute(xml_sdf, &*child, "irq")? { if config.arch == Arch::X86_64 { return Err(value_error( xml_sdf, @@ -394,9 +393,7 @@ impl ProtectionDomain { kind: SysIrqKind::Conventional { irq, trigger }, }; irqs.push(irq); - } else if let Some(pin) = - sdf_attribute_as_number::(xml_sdf, &*child, "pin")? - { + } else if let Some(pin) = sdf_parse_attribute::(xml_sdf, &*child, "pin")? { if config.arch != Arch::X86_64 { return Err(value_error( xml_sdf, @@ -422,7 +419,7 @@ impl ProtectionDomain { // Default to the first unit. let ioapic: i64 = - sdf_attribute_as_number(xml_sdf, &*child, "ioapic")?.unwrap_or(0); + sdf_parse_attribute(xml_sdf, &*child, "ioapic")?.unwrap_or(0); if ioapic < 0 { return Err(value_error( @@ -473,8 +470,7 @@ impl ProtectionDomain { X86IoapicIrqPolarity::HighTriggered }; - let vector: i64 = - sdf_required_attribute_as_number(xml_sdf, &*child, "vector")?; + let vector: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "vector")?; if !(0..=X86_IRQ_VECTOR_MAX).contains(&vector) { return Err(value_error( @@ -514,8 +510,7 @@ impl ProtectionDomain { let pci_device = PciDevice::from_str(pcidev_str) .map_err(|err| value_error(xml_sdf, &*child, err.to_string()))?; - let handle: i64 = - sdf_required_attribute_as_number(xml_sdf, &*child, "handle")?; + let handle: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "handle")?; if handle < 0 { return Err(value_error( xml_sdf, @@ -524,8 +519,7 @@ impl ProtectionDomain { )); } - let vector: i64 = - sdf_required_attribute_as_number(xml_sdf, &*child, "vector")?; + let vector: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "vector")?; if !(0..=X86_IRQ_VECTOR_MAX).contains(&vector) { return Err(value_error( @@ -566,7 +560,7 @@ impl ProtectionDomain { &["id", "setvar_id", "setvar_addr", "addr", "size"], )?; - let id: i64 = sdf_required_attribute_as_number(xml_sdf, &*child, "id")?; + let id: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "id")?; if id > PD_MAX_ID as i64 { return Err(value_error( @@ -591,7 +585,7 @@ impl ProtectionDomain { checked_add_setvar(&mut setvars, setvar, xml_sdf, &*child)?; } - let addr: u64 = sdf_required_attribute_as_number(xml_sdf, &*child, "addr")?; + let addr: u64 = sdf_parse_required_attribute(xml_sdf, &*child, "addr")?; if let Some(setvar_addr) = child.attribute("setvar_addr") { let setvar = SysSetVar { @@ -601,7 +595,7 @@ impl ProtectionDomain { checked_add_setvar(&mut setvars, setvar, xml_sdf, &*child)?; } - let size: i64 = sdf_required_attribute_as_number(xml_sdf, &*child, "size")?; + let size: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "size")?; if size <= 0 { return Err(value_error( xml_sdf, @@ -842,8 +836,8 @@ impl VirtualMachine { let sched_params = if config.arch == Arch::Aarch64 { // If we do not have an explicit budget the period is equal to the default budget. let budget: u64 = - sdf_attribute_as_number(xml_sdf, node, "budget")?.unwrap_or(BUDGET_DEFAULT); - let period: u64 = sdf_attribute_as_number(xml_sdf, node, "period")?.unwrap_or(budget); + sdf_parse_attribute(xml_sdf, node, "budget")?.unwrap_or(BUDGET_DEFAULT); + let period: u64 = sdf_parse_attribute(xml_sdf, node, "period")?.unwrap_or(budget); if budget > period { return Err(value_error( @@ -854,7 +848,7 @@ impl VirtualMachine { } // Default to minimum priority - let priority: u8 = sdf_attribute_as_number(xml_sdf, node, "priority")?.unwrap_or(0); + let priority: u8 = sdf_parse_attribute(xml_sdf, node, "priority")?.unwrap_or(0); if priority > PD_MAX_PRIORITY { return Err(value_error( @@ -880,7 +874,7 @@ impl VirtualMachine { match child_name { "vcpu" => { check_attributes(xml_sdf, &*child, &["id", "setvar_id", "cpu"])?; - let id = sdf_required_attribute_as_number(xml_sdf, &*child, "id")?; + let id = sdf_parse_required_attribute(xml_sdf, &*child, "id")?; if id > VCPU_MAX_ID { return Err(value_error( @@ -904,23 +898,22 @@ impl VirtualMachine { let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); - let cpu = - if let Some(cpu_value) = sdf_attribute_as_number(xml_sdf, node, "cpu")? { - if cpu_value >= config.num_cores { - return Err(value_error( - xml_sdf, - &*child, - format!( - "cpu core must be less than {}, got {}", - config.num_cores, cpu_value - ), - )); - } + let cpu = if let Some(cpu_value) = sdf_parse_attribute(xml_sdf, node, "cpu")? { + if cpu_value >= config.num_cores { + return Err(value_error( + xml_sdf, + &*child, + format!( + "cpu core must be less than {}, got {}", + config.num_cores, cpu_value + ), + )); + } - Some(CpuCore(cpu_value)) - } else { - None - }; + Some(CpuCore(cpu_value)) + } else { + None + }; vcpus.push(VirtualCpu { id, setvar_id, cpu }); } diff --git a/tool/microkit/src/sdf/util.rs b/tool/microkit/src/sdf/util.rs index cb0e2d211..25c4254d8 100644 --- a/tool/microkit/src/sdf/util.rs +++ b/tool/microkit/src/sdf/util.rs @@ -4,13 +4,21 @@ // SPDX-License-Identifier: BSD-2-Clause // +use std::fmt::Display; use std::num::ParseIntError; use super::{SdfLocation, SdfNode, SysSetVar, SystemDescriptionFile}; -/// Parse an 'attribute' of an `SdfNode` as a number of type T. +/// This is a helper trait so that we can have a generic attribute parsing +/// function that auto-infers the type. +pub(super) trait ParseableAttribute: Sized { + fn type_name() -> &'static str; + fn parse(s: &str) -> Result; +} + +/// Parse an 'attribute' of an `SdfNode` as a type T. /// If the attribute does not exist, return an Optional value. -pub fn sdf_attribute_as_number( +pub fn sdf_parse_attribute( sdf: &SystemDescriptionFile, node: &dyn SdfNode, attribute: &str, @@ -19,10 +27,12 @@ pub fn sdf_attribute_as_number( return Ok(None); }; - parse_number(value_str).map(|v| Some(v)).map_err(|err| { + T::parse(value_str).map(|v| Some(v)).map_err(|err| { format!( - "Error: failed to parse integer '{}' on element '{}': {}: {}", + "Error: failed to parse attribute `{}=\"{}\"` as {} on element <{}>: {}: {}", + attribute, value_str, + T::type_name(), node.tag_name(), err, loc_string(sdf, node.range().start), @@ -30,16 +40,16 @@ pub fn sdf_attribute_as_number( }) } -/// Parse an 'attribute' of an `SdfNode` as a number of type T. +/// Parse an 'attribute' of an `SdfNode` as a type T. /// If the attribute does not exist, return a neatly formatted error. -pub fn sdf_required_attribute_as_number( +pub fn sdf_parse_required_attribute( sdf: &SystemDescriptionFile, node: &dyn SdfNode, attribute: &str, ) -> Result { - sdf_attribute_as_number(sdf, node, attribute)?.ok_or_else(|| { + sdf_parse_attribute(sdf, node, attribute)?.ok_or_else(|| { format!( - "Error: Missing required attribute '{}' on element '{}': {}", + "Error: missing required attribute '{}' on element '{}': {}", attribute, node.tag_name(), loc_string(sdf, node.range().start), @@ -47,44 +57,24 @@ pub fn sdf_required_attribute_as_number( }) } -/// Parse an 'attribute' of an `SdfNode` as a boolean. -/// If the attribute does not exist, return an Optional value. -pub fn sdf_attribute_as_bool( - sdf: &SystemDescriptionFile, - node: &dyn SdfNode, - attribute: &str, -) -> Result, String> { - let Some(value_str) = node.attribute(attribute) else { - return Ok(None); - }; +impl ParseableAttribute for N { + fn type_name() -> &'static str { + "integer" + } - parse_bool(value_str).map(Some).map_err(|_| { - format!( - "Error: '{}' must be 'true' or 'false', got '{}' on element '{}': {}", - attribute, - value_str, - node.tag_name(), - loc_string(sdf, node.range().start), - ) - }) + fn parse(s: &str) -> Result { + parse_number(s) + } } -/// Parse an 'attribute' of an `SdfNode` as a boolean. -/// If the attribute does not exist, return a neatly formatted error. -#[expect(unused)] -pub fn sdf_required_attribute_as_bool( - sdf: &SystemDescriptionFile, - node: &dyn SdfNode, - attribute: &str, -) -> Result { - sdf_attribute_as_bool(sdf, node, attribute)?.ok_or_else(|| { - format!( - "Error: Missing required attribute '{}' on element '{}': {}", - attribute, - node.tag_name(), - loc_string(sdf, node.range().start), - ) - }) +impl ParseableAttribute for bool { + fn type_name() -> &'static str { + "boolean" + } + + fn parse(s: &str) -> Result { + parse_bool(s).map_err(|_ | "must be 'true' or 'false'") + } } /// This is annoying. Essentially, we can't do a generic over any number type @@ -228,7 +218,7 @@ pub fn checked_lookup<'a>( } else { let pos = node.range().start; Err(format!( - "Error: Missing required attribute '{}' on element '{}': {}:{}:{}", + "Error: missing required attribute '{}' on element '{}': {}:{}:{}", attribute, node.tag_name(), xml_sdf.filename.display(), diff --git a/tool/microkit/tests/test.rs b/tool/microkit/tests/test.rs index 93af9f059..098e15a6b 100644 --- a/tool/microkit/tests/test.rs +++ b/tool/microkit/tests/test.rs @@ -148,7 +148,7 @@ fn check_error(kernel_config: &sel4::Config, test_name: &str, expected_err: &str fn check_missing(kernel_config: &sel4::Config, test_name: &str, attr: &str, element: &str) { let expected_error = - format!("Error: Missing required attribute '{attr}' on element '{element}'"); + format!("Error: missing required attribute '{attr}' on element '{element}'"); check_error(kernel_config, test_name, expected_error.as_str()); } @@ -158,7 +158,11 @@ mod memory_region { #[test] fn test_malformed_size() { - check_error(&DEFAULT_AARCH64_KERNEL_CONFIG, "mr_malformed_size.system", "Error: failed to parse integer '0x200_000sd' on element 'memory_region': invalid digit found in string") + check_error( + &DEFAULT_AARCH64_KERNEL_CONFIG, + "mr_malformed_size.system", + r#"Error: failed to parse attribute `size="0x200_000sd"` as integer on element : invalid digit found in string:"#, + ) } #[test] @@ -1219,7 +1223,7 @@ mod channel { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "ch_end_invalid_pp.system", - "Error: 'pp' must be 'true' or 'false', got 'no' on element 'end': ", + r#"Error: failed to parse attribute `pp="no"` as boolean on element : must be 'true' or 'false': "#, ) } @@ -1228,7 +1232,7 @@ mod channel { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "ch_end_invalid_notify.system", - "Error: 'notify' must be 'true' or 'false', got 'no' on element 'end': ", + r#"Error: failed to parse attribute `notify="no"` as boolean on element : must be 'true' or 'false': "#, ) } @@ -1304,7 +1308,7 @@ mod domains { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "domain_no_pd_domain.system", - "Error: Missing required attribute 'domain' on element 'protection_domain': domain_no_pd_domain.system:15:5", + "Error: missing required attribute 'domain' on element 'protection_domain': domain_no_pd_domain.system:15:5", ) } @@ -1313,7 +1317,7 @@ mod domains { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "domain_invalid_start_index.system", - "Error: failed to parse integer 'zzzz' on element 'domain_schedule': invalid digit found in string", + r#"Error: failed to parse attribute `start_index="zzzz"` as integer on element : invalid digit found in string:"#, ) } @@ -1332,7 +1336,7 @@ mod domains { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "domain_invalid_shift.system", - "Error: failed to parse integer 'zzzz' on element 'domain_schedule': invalid digit found in string", + r#"Error: failed to parse attribute `index_shift="zzzz"` as integer on element : invalid digit found in string:"#, ) } @@ -1474,7 +1478,7 @@ mod system { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "wrong_fpu_flag_value.system", - "Error: 'fpu' must be 'true' or 'false', got 'foo' on element 'protection_domain': ", + r#"Error: failed to parse attribute `fpu="foo"` as boolean on element : must be 'true' or 'false':"#, ) } From 74c6b494c78f34ea8f4eede2db9f893582e47283 Mon Sep 17 00:00:00 2001 From: Julia Vassiliki Date: Thu, 13 Aug 2026 16:50:29 +1000 Subject: [PATCH 7/7] tool(cleanup): impl ParseAttribute on PciDevice This means we can use the sdf_parse_attribute** functions for PCI devices, which cleans up some code. Not sure of the best way to deal with IommuDeviceIdentifier however in this framework. I think if I passed "config" to parse then it might make more sense, but that means I need an extra argument for all sdf_parse_attribute calls. Unless it forms a part of the SystemDescription struct which currently only contains a file, which might be a good idea. Signed-off-by: Julia Vassiliki --- tool/microkit/src/sdf/iommu.rs | 4 ++-- tool/microkit/src/sdf/pci.rs | 11 ++++++++--- tool/microkit/src/sdf/pd_vm.rs | 9 +++------ tool/microkit/src/sdf/util.rs | 16 ++++++++++++---- tool/microkit/tests/test.rs | 14 +++++++------- 5 files changed, 32 insertions(+), 22 deletions(-) diff --git a/tool/microkit/src/sdf/iommu.rs b/tool/microkit/src/sdf/iommu.rs index a388f22ca..4ceac10c8 100644 --- a/tool/microkit/src/sdf/iommu.rs +++ b/tool/microkit/src/sdf/iommu.rs @@ -6,12 +6,12 @@ use std::collections::BTreeSet; use std::fmt; -use std::str::FromStr; use super::memory_region::SysIOMap; use super::pci::{PciDevice, PciDeviceParseError}; use super::util::{ check_attributes, checked_lookup, loc_string, sdf_parse_required_attribute, value_error, + ParseableAttribute, }; use super::{SdfNode, SystemDescriptionFile}; @@ -43,7 +43,7 @@ impl IommuDeviceIdentifier { s: &str, ) -> Result { match config.arch { - Arch::X86_64 => PciDevice::from_str(s) + Arch::X86_64 => PciDevice::parse(s) .map(IommuDeviceIdentifier::X86Pci) .map_err(IommuDeviceIdentifierParseError::Pci), Arch::Aarch64 | Arch::Riscv64 => Err(IommuDeviceIdentifierParseError::UnsupportedArch( diff --git a/tool/microkit/src/sdf/pci.rs b/tool/microkit/src/sdf/pci.rs index 851908340..9a909a020 100644 --- a/tool/microkit/src/sdf/pci.rs +++ b/tool/microkit/src/sdf/pci.rs @@ -6,10 +6,11 @@ use std::fmt; use std::ops::Deref; -use std::str::FromStr; use sel4_capdl_initializer_types::object; +use super::util::ParseableAttribute; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PciDevice(pub object::PCIDevice); @@ -88,10 +89,14 @@ impl fmt::Display for PciDeviceParseError { } } -impl FromStr for PciDevice { +impl ParseableAttribute for PciDevice { type Err = PciDeviceParseError; - fn from_str(s: &str) -> Result { + fn type_name() -> &'static str { + "pci device" + } + + fn parse(s: &str) -> Result { let (bus_str, device_function_str) = s.split_once(':').ok_or(PciDeviceParseError::Malformed)?; let (device_str, function_str) = device_function_str diff --git a/tool/microkit/src/sdf/pd_vm.rs b/tool/microkit/src/sdf/pd_vm.rs index fdf123d5e..91fd1d6f8 100644 --- a/tool/microkit/src/sdf/pd_vm.rs +++ b/tool/microkit/src/sdf/pd_vm.rs @@ -7,7 +7,6 @@ use std::fmt; use std::path::{Path, PathBuf}; use std::rc::Rc; -use std::str::FromStr; use super::channels::Channel; use super::consts::*; @@ -15,7 +14,6 @@ use super::cspace::{CSpace, CapMap}; use super::domains::Domains; use super::irq::{SysIrq, SysIrqKind}; use super::memory_region::SysMap; -use super::pci::PciDevice; use super::util::{ check_attributes, checked_add_setvar, checked_lookup, loc_string, sdf_parse_attribute, sdf_parse_required_attribute, value_error, @@ -491,7 +489,9 @@ impl ProtectionDomain { }, }; irqs.push(irq); - } else if let Some(pcidev_str) = child.attribute("pcidev") { + } else if let Some(pci_device) = + sdf_parse_attribute(xml_sdf, &*child, "pcidev")? + { if config.arch != Arch::X86_64 { return Err(value_error( xml_sdf, @@ -507,9 +507,6 @@ impl ProtectionDomain { &["id", "setvar_id", "pcidev", "handle", "vector"], )?; - let pci_device = PciDevice::from_str(pcidev_str) - .map_err(|err| value_error(xml_sdf, &*child, err.to_string()))?; - let handle: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "handle")?; if handle < 0 { return Err(value_error( diff --git a/tool/microkit/src/sdf/util.rs b/tool/microkit/src/sdf/util.rs index 25c4254d8..fa99d2218 100644 --- a/tool/microkit/src/sdf/util.rs +++ b/tool/microkit/src/sdf/util.rs @@ -11,9 +11,13 @@ use super::{SdfLocation, SdfNode, SysSetVar, SystemDescriptionFile}; /// This is a helper trait so that we can have a generic attribute parsing /// function that auto-infers the type. +/// This is like FromStr trait but it allows for our own custom implementations +/// of from_str on integers and others. pub(super) trait ParseableAttribute: Sized { + type Err: Display; + fn type_name() -> &'static str; - fn parse(s: &str) -> Result; + fn parse(s: &str) -> Result; } /// Parse an 'attribute' of an `SdfNode` as a type T. @@ -58,22 +62,26 @@ pub fn sdf_parse_required_attribute( } impl ParseableAttribute for N { + type Err = ParseIntError; + fn type_name() -> &'static str { "integer" } - fn parse(s: &str) -> Result { + fn parse(s: &str) -> Result { parse_number(s) } } impl ParseableAttribute for bool { + type Err = &'static str; + fn type_name() -> &'static str { "boolean" } - fn parse(s: &str) -> Result { - parse_bool(s).map_err(|_ | "must be 'true' or 'false'") + fn parse(s: &str) -> Result { + parse_bool(s).map_err(|_| "must be 'true' or 'false'") } } diff --git a/tool/microkit/tests/test.rs b/tool/microkit/tests/test.rs index 098e15a6b..8f9a2fbb4 100644 --- a/tool/microkit/tests/test.rs +++ b/tool/microkit/tests/test.rs @@ -576,7 +576,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_bus_less_than_0.system", - "Error: PCI bus must be within [0..255] on element 'irq'", + r#"Error: failed to parse attribute `pcidev="-1:0.0"` as pci device on element : PCI bus must be within [0..255]:"#, ) } @@ -585,7 +585,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_dev_less_than_0.system", - "Error: PCI device must be within [0..31] on element 'irq'", + r#"Error: failed to parse attribute `pcidev="0:-1.0"` as pci device on element : PCI device must be within [0..31]"#, ) } @@ -594,7 +594,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_func_less_than_0.system", - "Error: PCI function must be within [0..7] on element 'irq'", + r#"Error: failed to parse attribute `pcidev="0:0.-1"` as pci device on element : PCI function must be within [0..7]:"#, ) } @@ -603,7 +603,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_bus_greater_than_255.system", - "Error: PCI bus must be within [0..255] on element 'irq'", + r#"Error: failed to parse attribute `pcidev="256:0.0"` as pci device on element : PCI bus must be within [0..255]"#, ) } @@ -612,7 +612,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_dev_greater_than_31.system", - "Error: PCI device must be within [0..31] on element 'irq'", + r#"Error: failed to parse attribute `pcidev="0:32.0"` as pci device on element : PCI device must be within [0..31]:"#, ) } @@ -621,7 +621,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_func_greater_than_7.system", - "Error: PCI function must be within [0..7] on element 'irq'", + r#"Error: failed to parse attribute `pcidev="0:0.8"` as pci device on element : PCI function must be within [0..7]:"#, ) } @@ -657,7 +657,7 @@ mod protection_domain { check_error( &DEFAULT_X86_64_KERNEL_CONFIG, "irq_msi_pci_invalid.system", - "Error: expected PCI address in bus:device.function form on element 'irq'", + r#"Error: failed to parse attribute `pcidev="0:0:0"` as pci device on element : expected PCI address in bus:device.function form:"#, ) }