diff --git a/tool/microkit/src/sdf/channels.rs b/tool/microkit/src/sdf/channels.rs index 2eabfa02e..2940663e6 100644 --- a/tool/microkit/src/sdf/channels.rs +++ b/tool/microkit/src/sdf/channels.rs @@ -9,11 +9,12 @@ 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_parse_attribute, + sdf_parse_required_attribute, value_error, +}; use super::{SdfNode, SystemDescriptionFile}; -use crate::util::str_to_bool; - #[derive(Debug, Clone)] pub struct ChannelEnd { pub pd: Rc, @@ -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_parse_required_attribute(xml_sdf, node, "id")?; if end_id > PD_MAX_ID as i64 { return Err(value_error( @@ -61,25 +62,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_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 61021f8be..50f096520 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_parse_required_attribute, 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_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 2c062b180..792e032dd 100644 --- a/tool/microkit/src/sdf/domains.rs +++ b/tool/microkit/src/sdf/domains.rs @@ -9,7 +9,9 @@ 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_parse_attribute, value_error, +}; use super::{SdfNode, SystemDescriptionFile}; use crate::Config; @@ -144,12 +146,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_parse_attribute(xml_sdf, node, "id")? + .map(|n: u8| { + if n >= config.num_domains { Err(value_error( xml_sdf, node, @@ -160,8 +159,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 +175,12 @@ 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_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 = node - .attribute("index_shift") - .map(|s| sdf_parse_number(s, node)) - .transpose()?; + let schedule_index_shift: Option = sdf_parse_attribute(xml_sdf, node, "index_shift")?; let mut schedule = vec![]; @@ -291,7 +283,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..4ceac10c8 100644 --- a/tool/microkit/src/sdf/iommu.rs +++ b/tool/microkit/src/sdf/iommu.rs @@ -6,11 +6,13 @@ 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_number, value_error}; +use super::util::{ + check_attributes, checked_lookup, loc_string, sdf_parse_required_attribute, value_error, + ParseableAttribute, +}; use super::{SdfNode, SystemDescriptionFile}; use crate::{sel4::Arch, Config}; @@ -41,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( @@ -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_parse_required_attribute(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..557354220 100644 --- a/tool/microkit/src/sdf/memory_region.rs +++ b/tool/microkit/src/sdf/memory_region.rs @@ -13,12 +13,14 @@ 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_parse_attribute, sdf_parse_required_attribute, + 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)] @@ -269,7 +271,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_parse_required_attribute(xml_sdf, node, "vaddr")?; if vaddr >= max_vaddr { return Err(value_error( @@ -304,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_parse_attribute(xml_sdf, node, "cached")? // Default to cached - true - }; + .unwrap_or(true); Ok(SysMap { mr, @@ -344,7 +334,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_parse_required_attribute(xml_sdf, node, "iovaddr")?; if iovaddr > x86_io_address_space::CAPDL_MAX_IOVA { return Err(value_error( @@ -394,11 +384,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_parse_attribute::(xml_sdf, node, "size")? { + Some(size_parsed) => { if !size_parsed.is_multiple_of(page_size) { return Err(value_error( xml_sdf, @@ -427,7 +414,7 @@ impl SysMemoryRegion { } } - Err(_) => { + None => { if prefill_bootinfo_maybe.is_some() { Ok(page_size) } else { @@ -468,9 +455,9 @@ 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") { + let page_size = if let Some(page_size) = sdf_parse_attribute(xml_sdf, node, "page_size")? { page_size_specified_by_user = true; - sdf_parse_number(xml_page_size, node)? + page_size } else { config.page_sizes()[0] }; @@ -558,12 +545,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_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. - 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/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 91fb588ec..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,14 +14,13 @@ 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_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}; use crate::sel4::{Arch, ArmRiscvIrqTrigger, X86IoapicIrqPolarity, X86IoapicIrqTrigger}; -use crate::util::str_to_bool; use crate::Config; #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] @@ -164,7 +162,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_parse_required_attribute(xml_sdf, node, "id")?; let setvar_id = node.attribute("setvar_id").map(ToOwned::to_owned); (Some(id), setvar_id) } else { @@ -172,16 +170,9 @@ 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_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( xml_sdf, @@ -190,41 +181,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_parse_attribute(xml_sdf, node, "passive")?.unwrap_or(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_parse_attribute(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_parse_attribute(xml_sdf, node, "smc")?.unwrap_or(false); if smc { match config.arm_smc { @@ -242,11 +204,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_parse_attribute(xml_sdf, node, "cpu")?.unwrap_or(0u8)); if cpu.0 >= config.num_cores { return Err(value_error( @@ -316,12 +274,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_parse_attribute(xml_sdf, node, "priority")?.unwrap_or(0); if priority > PD_MAX_PRIORITY as u64 { return Err(value_error( @@ -332,20 +286,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_parse_attribute(xml_sdf, node, "fpu")?.unwrap_or(true); for child in node.children() { match child.tag_name() { @@ -396,9 +337,8 @@ impl ProtectionDomain { maps.push(map); } "irq" => { - let id = checked_lookup(xml_sdf, &*child, "id")? - .parse::() - .unwrap(); + let id: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "id")?; + if id > PD_MAX_ID as i64 { return Err(value_error( xml_sdf, @@ -418,7 +358,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_parse_attribute(xml_sdf, &*child, "irq")? { if config.arch == Arch::X86_64 { return Err(value_error( xml_sdf, @@ -429,7 +369,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, @@ -451,7 +391,7 @@ 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_parse_attribute::(xml_sdf, &*child, "pin")? { if config.arch != Arch::X86_64 { return Err(value_error( xml_sdf, @@ -475,12 +415,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_parse_attribute(xml_sdf, &*child, "ioapic")?.unwrap_or(0); + if ioapic < 0 { return Err(value_error( xml_sdf, @@ -489,7 +427,6 @@ impl ProtectionDomain { )); } - let pin = pin_str.parse::().unwrap(); if pin < 0 { return Err(value_error( xml_sdf, @@ -530,9 +467,9 @@ impl ProtectionDomain { // Default to normal polarity X86IoapicIrqPolarity::HighTriggered }; - let vector = checked_lookup(xml_sdf, &*child, "vector")? - .parse::() - .unwrap(); + + let vector: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "vector")?; + if !(0..=X86_IRQ_VECTOR_MAX).contains(&vector) { return Err(value_error( xml_sdf, @@ -552,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, @@ -568,12 +507,7 @@ 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 = checked_lookup(xml_sdf, &*child, "handle")? - .parse::() - .unwrap(); + let handle: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "handle")?; if handle < 0 { return Err(value_error( xml_sdf, @@ -582,9 +516,8 @@ impl ProtectionDomain { )); } - let vector = checked_lookup(xml_sdf, &*child, "vector")? - .parse::() - .unwrap(); + let vector: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "vector")?; + if !(0..=X86_IRQ_VECTOR_MAX).contains(&vector) { return Err(value_error( xml_sdf, @@ -624,9 +557,8 @@ impl ProtectionDomain { &["id", "setvar_id", "setvar_addr", "addr", "size"], )?; - let id = checked_lookup(xml_sdf, &*child, "id")? - .parse::() - .unwrap(); + let id: i64 = sdf_parse_required_attribute(xml_sdf, &*child, "id")?; + if id > PD_MAX_ID as i64 { return Err(value_error( xml_sdf, @@ -650,8 +582,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_parse_required_attribute(xml_sdf, &*child, "addr")?; if let Some(setvar_addr) = child.attribute("setvar_addr") { let setvar = SysSetVar { @@ -661,9 +592,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_parse_required_attribute(xml_sdf, &*child, "size")?; if size <= 0 { return Err(value_error( xml_sdf, @@ -903,16 +832,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_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( xml_sdf, @@ -922,16 +845,18 @@ 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_parse_attribute(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 { - // 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, }) @@ -946,9 +871,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_parse_required_attribute(xml_sdf, &*child, "id")?; + if id > VCPU_MAX_ID { return Err(value_error( xml_sdf, @@ -971,11 +895,7 @@ 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"); - + 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, diff --git a/tool/microkit/src/sdf/util.rs b/tool/microkit/src/sdf/util.rs index b28479e85..fa99d2218 100644 --- a/tool/microkit/src/sdf/util.rs +++ b/tool/microkit/src/sdf/util.rs @@ -4,14 +4,115 @@ // SPDX-License-Identifier: BSD-2-Clause // +use std::fmt::Display; +use std::num::ParseIntError; + 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; +} + +/// Parse an 'attribute' of an `SdfNode` as a type T. +/// If the attribute does not exist, return an Optional value. +pub fn sdf_parse_attribute( + sdf: &SystemDescriptionFile, + node: &dyn SdfNode, + attribute: &str, +) -> Result, String> { + let Some(value_str) = node.attribute(attribute) else { + return Ok(None); + }; + + T::parse(value_str).map(|v| Some(v)).map_err(|err| { + format!( + "Error: failed to parse attribute `{}=\"{}\"` as {} on element <{}>: {}: {}", + attribute, + value_str, + T::type_name(), + node.tag_name(), + err, + loc_string(sdf, node.range().start), + ) + }) +} + +/// Parse an 'attribute' of an `SdfNode` as a type T. +/// If the attribute does not exist, return a neatly formatted error. +pub fn sdf_parse_required_attribute( + sdf: &SystemDescriptionFile, + node: &dyn SdfNode, + attribute: &str, +) -> Result { + sdf_parse_attribute(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 N { + type Err = ParseIntError; + + fn type_name() -> &'static str { + "integer" + } + + 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'") + } +} + +/// 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; +} + +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_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 /// 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,14 +121,15 @@ 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) +} + +// 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(()), } } @@ -124,7 +226,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/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/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..8f9a2fbb4 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] @@ -572,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]:"#, ) } @@ -581,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]"#, ) } @@ -590,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]:"#, ) } @@ -599,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]"#, ) } @@ -608,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]:"#, ) } @@ -617,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]:"#, ) } @@ -653,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:"#, ) } @@ -765,6 +769,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 +1022,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") @@ -1201,7 +1223,7 @@ mod channel { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "ch_end_invalid_pp.system", - "Error: pp must be 'true' or 'false' on element 'end': ", + r#"Error: failed to parse attribute `pp="no"` as boolean on element : must be 'true' or 'false': "#, ) } @@ -1210,7 +1232,7 @@ mod channel { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "ch_end_invalid_notify.system", - "Error: notify must be 'true' or 'false' on element 'end': ", + r#"Error: failed to parse attribute `notify="no"` as boolean on element : must be 'true' or 'false': "#, ) } @@ -1286,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", ) } @@ -1295,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:"#, ) } @@ -1314,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:"#, ) } @@ -1456,7 +1478,7 @@ mod system { check_error( &DEFAULT_AARCH64_KERNEL_CONFIG, "wrong_fpu_flag_value.system", - "Error: fpu must be 'true' or 'false'", + r#"Error: failed to parse attribute `fpu="foo"` as boolean on element : must be 'true' or 'false':"#, ) }