From e9e951c25d4b7bd15768f5b7688340eee2247721 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 Aug 2025 22:44:21 +0000 Subject: [PATCH 1/9] Initial plan From 10ca123fb9bb01f9588a5914ec528c643bdaa5f6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 Aug 2025 23:14:40 +0000 Subject: [PATCH 2/9] Migrate fdt parser error types to thiserror Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- Cargo.lock | 2 + openhcl/host_fdt_parser/Cargo.toml | 1 + openhcl/host_fdt_parser/src/lib.rs | 104 +++++------- support/fdt/src/parser.rs | 245 ++++++++++------------------- vm/hv1/hvdef/Cargo.toml | 1 + 5 files changed, 132 insertions(+), 221 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cffde82832..ab6d95d776 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2879,6 +2879,7 @@ dependencies = [ "igvm_defs", "inspect", "memory_range", + "thiserror 2.0.12", "tracing", ] @@ -2974,6 +2975,7 @@ dependencies = [ "bitfield-struct 0.10.1", "open_enum", "static_assertions", + "thiserror 2.0.12", "zerocopy 0.8.24", ] diff --git a/openhcl/host_fdt_parser/Cargo.toml b/openhcl/host_fdt_parser/Cargo.toml index e7a9b8969b..4411b04031 100644 --- a/openhcl/host_fdt_parser/Cargo.toml +++ b/openhcl/host_fdt_parser/Cargo.toml @@ -19,6 +19,7 @@ inspect = { workspace = true, optional = true } arrayvec.workspace = true igvm_defs.workspace = true +thiserror.workspace = true tracing = { workspace = true, optional = true } [lints] diff --git a/openhcl/host_fdt_parser/src/lib.rs b/openhcl/host_fdt_parser/src/lib.rs index 6e6a2a3d0e..734282b5ed 100644 --- a/openhcl/host_fdt_parser/src/lib.rs +++ b/openhcl/host_fdt_parser/src/lib.rs @@ -21,6 +21,7 @@ use igvm_defs::MemoryMapEntryType; #[cfg(feature = "inspect")] use inspect::Inspect; use memory_range::MemoryRange; +use thiserror::Error; /// Information about VMBUS. #[derive(Clone, Debug, PartialEq, Eq)] @@ -56,136 +57,115 @@ pub struct GicInfo { } /// Errors returned by parsing. -#[derive(Debug)] -pub struct Error<'a>(ErrorKind<'a>); - -impl Display for Error<'_> { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - f.write_fmt(format_args!("Parsing failed due to: {}", self.0)) - } -} - -impl core::error::Error for Error<'_> {} - -#[derive(Debug)] -enum ErrorKind<'a> { +#[derive(Debug, Error)] +pub enum Error<'a> { + /// Invalid device tree + #[error("invalid device tree: {0}")] Dt(fdt::parser::Error<'a>), + /// Invalid device tree node + #[error("invalid device tree node with parent {parent_name}: {error}")] Node { parent_name: &'a str, + #[source] error: fdt::parser::Error<'a>, }, + /// Required property missing + #[error("{node_name} did not have the following required property {prop_name}")] PropMissing { node_name: &'a str, prop_name: &'static str, }, + /// Reading node property failed + #[error("reading node property failed: {0}")] Prop(fdt::parser::Error<'a>), + /// Too many CPUs + #[error("device tree contained more enabled CPUs than can be parsed")] TooManyCpus, + /// Memory region not aligned + #[error("memory node {node_name} contains 4K unaligned base {base} or len {len}")] MemoryRegUnaligned { node_name: &'a str, base: u64, len: u64, }, + /// Memory regions overlap + #[error("ram at {}..{} of type {:?} overlaps ram at {}..{} of type {:?}", lower.range.start(), lower.range.end(), lower.mem_type, upper.range.start(), upper.range.end(), upper.mem_type)] MemoryRegOverlap { lower: MemoryEntry, upper: MemoryEntry, }, + /// Too many memory entries + #[error("device tree contained more memory ranges than can be parsed")] TooManyMemoryEntries, + /// Invalid u32 property value + #[error("{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}")] PropInvalidU32 { node_name: &'a str, prop_name: &'a str, expected: u32, actual: u32, }, + /// Invalid string property value + #[error("{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}")] PropInvalidStr { node_name: &'a str, prop_name: &'a str, expected: &'a str, actual: &'a str, }, + /// Unexpected VMBUS VTL + #[error("{node_name} has an unexpected vtl {vtl}")] UnexpectedVmbusVtl { node_name: &'a str, vtl: u32, }, + /// Multiple VMBUS nodes + #[error("{node_name} specifies a duplicate vmbus node")] MultipleVmbusNode { node_name: &'a str, }, + /// VMBUS ranges child/parent mismatch + #[error("vmbus {node_name} ranges child base {child_base} does not match parent {parent_base}")] VmbusRangesChildParent { node_name: &'a str, child_base: u64, parent_base: u64, }, + /// VMBUS ranges not aligned + #[error("vmbus {node_name} base {base} or len {len} not aligned to 4K")] VmbusRangesNotAligned { node_name: &'a str, base: u64, len: u64, }, + /// Too many VMBUS MMIO ranges + #[error("vmbus {node_name} has more than 2 mmio ranges {ranges}")] TooManyVmbusMmioRanges { node_name: &'a str, ranges: usize, }, + /// VMBUS MMIO overlaps RAM + #[error("vmbus mmio at {}..{} overlaps ram at {}..{}", mmio.start(), mmio.end(), ram.range.start(), ram.range.end())] VmbusMmioOverlapsRam { mmio: MemoryRange, ram: MemoryEntry, }, + /// VMBUS MMIO overlaps VMBUS MMIO + #[error("vmbus mmio at {}..{} overlaps vmbus mmio at {}..{}", mmio_a.start(), mmio_a.end(), mmio_b.start(), mmio_b.end())] VmbusMmioOverlapsVmbusMmio { mmio_a: MemoryRange, mmio_b: MemoryRange, }, + /// Command line size error + #[error("commandline too small to parse /chosen bootargs")] CmdlineSize, + /// Unexpected memory allocation mode + #[error("unexpected memory allocation mode: {mode}")] UnexpectedMemoryAllocationMode { mode: &'a str, }, } -impl Display for ErrorKind<'_> { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - ErrorKind::Dt(e) => f.write_fmt(format_args!("invalid device tree: {}", e)), - ErrorKind::Node { parent_name, error } => { - f.write_fmt(format_args!("invalid device tree node with parent {parent_name}: {error}")) - } - ErrorKind::PropMissing { - node_name, - prop_name, - } => f.write_fmt(format_args!( - "{node_name} did not have the following required property {prop_name}", - )), - ErrorKind::Prop(e) => f.write_fmt(format_args!("reading node property failed: {e}")), - ErrorKind::TooManyCpus => { - f.write_str("device tree contained more enabled CPUs than can be parsed") - } - ErrorKind::MemoryRegUnaligned { - node_name, - base, - len, - } => f.write_fmt(format_args!( - "memory node {node_name} contains 4K unaligned base {base} or len {len}" - )), - ErrorKind::MemoryRegOverlap { lower, upper, } => { - f.write_fmt(format_args!("ram at {}..{} of type {:?} overlaps ram at {}..{} of type {:?}", lower.range.start(), lower.range.end(), lower.mem_type, upper.range.start(), upper.range.end(), upper.mem_type)) - } - ErrorKind::TooManyMemoryEntries => { - f.write_str("device tree contained more memory ranges than can be parsed") - } - ErrorKind::PropInvalidU32 { node_name, prop_name, expected, actual } => f.write_fmt(format_args!("{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}")), - ErrorKind::PropInvalidStr { node_name, prop_name, expected, actual } => f.write_fmt(format_args!("{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}")), - ErrorKind::UnexpectedVmbusVtl { node_name, vtl } => f.write_fmt(format_args!("{node_name} has an unexpected vtl {vtl}")), - ErrorKind::MultipleVmbusNode { node_name } => f.write_fmt(format_args!("{node_name} specifies a duplicate vmbus node")), - ErrorKind::VmbusRangesChildParent { node_name, child_base, parent_base } => f.write_fmt(format_args!("vmbus {node_name} ranges child base {child_base} does not match parent {parent_base}")), - ErrorKind::VmbusRangesNotAligned { node_name, base, len } => f.write_fmt(format_args!("vmbus {node_name} base {base} or len {len} not aligned to 4K")), - ErrorKind::TooManyVmbusMmioRanges { node_name, ranges } => f.write_fmt(format_args!("vmbus {node_name} has more than 2 mmio ranges {ranges}")), - ErrorKind::VmbusMmioOverlapsRam { mmio, ram } => { - f.write_fmt(format_args!("vmbus mmio at {}..{} overlaps ram at {}..{}", mmio.start(), mmio.end(), ram.range.start(), ram.range.end())) - } - ErrorKind::VmbusMmioOverlapsVmbusMmio { mmio_a, mmio_b } => { - f.write_fmt(format_args!("vmbus mmio at {}..{} overlaps vmbus mmio at {}..{}", mmio_a.start(), mmio_a.end(), mmio_b.start(), mmio_b.end())) - } - ErrorKind::CmdlineSize => f.write_str("commandline too small to parse /chosen bootargs"), - ErrorKind::UnexpectedMemoryAllocationMode { mode } => f.write_fmt(format_args!("unexpected memory allocation mode: {}", mode)), - } - } -} - const COM3_REG_BASE: u64 = 0x3E8; /// Struct containing parsed device tree information. diff --git a/support/fdt/src/parser.rs b/support/fdt/src/parser.rs index 914fbe48da..6d8f7e306e 100644 --- a/support/fdt/src/parser.rs +++ b/support/fdt/src/parser.rs @@ -6,154 +6,104 @@ use super::spec; use super::spec::U32b; use super::spec::U64b; -use core::fmt::Display; use core::mem::size_of; +use thiserror::Error; use zerocopy::FromBytes; use zerocopy::Immutable; use zerocopy::KnownLayout; /// Errors returned when parsing a FDT. -#[derive(Debug)] -pub struct Error<'a>(ErrorKind<'a>); - -impl Display for Error<'_> { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - self.0.fmt(f) - } -} - -// TODO: Once core::error::Error is stablized, we can remove this feature gate. -impl core::error::Error for Error<'_> {} - -/// Types of errors when parsing a FDT. -#[derive(Debug)] -enum ErrorKind<'a> { +#[derive(Debug, Error)] +pub enum Error<'a> { /// Buffer is not aligned to u32 + #[error("Buffer is not aligned to u32")] BufferAlignment, /// Buffer too small for fixed header + #[error("Buffer too small for fixed FDT header")] NoHeader, /// Fixed header magic invalid + #[error("FDT header magic field invalid")] HeaderMagic, /// Total size described in the fixed header is greater than buffer provided + #[error("FDT header total size greater than provided buffer")] HeaderTotalSize, /// Header version is invalid + #[error("FDT header version invalid")] HeaderVersion, /// Structure block not contained within buffer + #[error("Structure block not contained within buffer")] StructureBlock, /// Structure block not aligned to u32 + #[error("Structure block offset is not aligned to u32")] StructureBlockAlignment, /// Memory reservation block not contained within buffer + #[error("Memory reservation block not contained within buffer")] MemoryReservationBlock, /// Memory reservation block did not end with an empty entry + #[error("Memory reservation block did not end with an empty entry")] MemoryReservationBlockEnd, /// Strings block not contained within buffer + #[error("Strings block not contained within buffer")] StringsBlock, /// No root node present + #[error("No root node present")] RootNode, /// More than one node at the root + #[error("More than one node at the root")] MultipleRootNodes, /// Unable to parse FDT token when parsing nodes - NodeToken(ParseTokenError), + #[error("Unable to parse FDT token when parsing nodes {0}")] + NodeToken(#[from] ParseTokenError), /// Unexpected token when parsing begin node + #[error("Unexpected token when parsing begin node {0}")] NodeBegin(u32), /// Unexpected token when parsing node properties + #[error("Unexpected token when parsing node properties {0}")] NodeProp(u32), /// Unexpected token when parsing children nodes + #[error("Unexpected token when parsing children nodes {0}")] NodeChildren(u32), /// Property data buffer len is not a multiple of requested type size + #[error("Property {prop_name} data buffer len is not multiple of type size for node {node_name}")] PropertyDataTypeBuffer { node_name: &'a str, prop_name: &'a str, }, /// Property requested at offset is larger than data buffer + #[error("Property {prop_name} requested at offset is larger than data buffer for node {node_name}")] PropertyOffset { node_name: &'a str, prop_name: &'a str, }, /// Property data is not a a valid string + #[error("Property data is not a a valid string for node {node_name}: {error}")] PropertyStr { node_name: &'a str, + #[source] error: StringError, }, /// Unable to parse FDT token when parsing properties + #[error("Unable to parse FDT token when parsing properties for node {node_name}: {error}")] PropertyTokenParse { node_name: &'a str, + #[source] error: ParseTokenError, }, /// Unexpected FDT token when parsing properties + #[error("Unexpected FDT token when parsing properties for node {node_name}: {token}")] PropertyToken { node_name: &'a str, token: u32 }, /// Property name string is not a valid string + #[error("Property name string is not a valid string for node {node_name}: {error}")] PropertyNameStr { node_name: &'a str, + #[source] error: StringError, }, /// FDT end token not present at end of structure block + #[error("FDT end token not present at end of structure block")] FdtEnd, } -impl Display for ErrorKind<'_> { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - ErrorKind::BufferAlignment => f.write_str("Buffer is not aligned to u32"), - ErrorKind::NoHeader => f.write_str("Buffer too small for fixed FDT header"), - ErrorKind::HeaderMagic => f.write_str("FDT header magic field invalid"), - ErrorKind::HeaderTotalSize => { - f.write_str("FDT header total size greater than provided buffer") - } - ErrorKind::HeaderVersion => f.write_str("FDT header version invalid"), - ErrorKind::StructureBlock => f.write_str("Structure block not contained within buffer"), - ErrorKind::StructureBlockAlignment => { - f.write_str("Structure block offset is not aligned to u32") - } - ErrorKind::MemoryReservationBlock => { - f.write_str("Memory reservation block not contained within buffer") - } - ErrorKind::MemoryReservationBlockEnd => { - f.write_str("Memory reservation block did not end with an empty entry") - } - ErrorKind::StringsBlock => f.write_str("Strings block not contained within buffer"), - ErrorKind::RootNode => f.write_str("No root node present"), - ErrorKind::MultipleRootNodes => f.write_str("More than one node at the root"), - ErrorKind::NodeToken(e) => f.write_fmt(format_args!( - "Unable to parse FDT token when parsing nodes {}", - e - )), - ErrorKind::NodeBegin(token) => f.write_fmt(format_args!( - "Unexpected token when parsing begin node {}", - token - )), - ErrorKind::NodeProp(token) => f.write_fmt(format_args!( - "Unexpected token when parsing node properties {}", - token - )), - ErrorKind::NodeChildren(token) => f.write_fmt(format_args!( - "Unexpected token when parsing children nodes {}", - token - )), - ErrorKind::PropertyDataTypeBuffer { node_name, prop_name } => f.write_fmt(format_args!( - "Property {prop_name} data buffer len is not multiple of type size for node {node_name}" - )), - ErrorKind::PropertyOffset { node_name, prop_name } => f.write_fmt(format_args!( - "Property {prop_name} requested at offset is larger than data buffer for node {node_name}" - )), - ErrorKind::PropertyStr { node_name, error } => f.write_fmt(format_args!( - "Property data is not a a valid string for node {node_name}: {error}" - )), - ErrorKind::PropertyTokenParse { node_name, error } => f.write_fmt(format_args!( - "Unable to parse FDT token when parsing properties for node {node_name}: {error}", - )), - ErrorKind::PropertyToken { node_name, token } => f.write_fmt(format_args!( - "Unexpected FDT token when parsing properties for node {node_name}: {}", - token - )), - ErrorKind::PropertyNameStr { node_name, error } => f.write_fmt(format_args!( - "Property name string is not a valid string for node {node_name}: {error}", - )), - ErrorKind::FdtEnd => f.write_str("FDT end token not present at end of structure block"), - } - } -} - /// A parser used to parse a FDT. pub struct Parser<'a> { /// The total size used by the dt. @@ -173,11 +123,11 @@ impl<'a> Parser<'a> { /// attempting to determine the overall size of a device tree. pub fn read_total_size(buf: &[u8]) -> Result> { let header = spec::Header::read_from_prefix(buf) - .map_err(|_| Error(ErrorKind::NoHeader))? + .map_err(|_| Error::NoHeader)? .0; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u32::from(header.magic) != spec::MAGIC { - Err(Error(ErrorKind::HeaderMagic)) + Err(Error::HeaderMagic) } else { Ok(u32::from(header.totalsize) as usize) } @@ -186,27 +136,27 @@ impl<'a> Parser<'a> { /// Create a new instance of a FDT parser. pub fn new(buf: &'a [u8]) -> Result> { if buf.as_ptr() as usize % size_of::() != 0 { - return Err(Error(ErrorKind::BufferAlignment)); + return Err(Error::BufferAlignment); } let header = spec::Header::read_from_prefix(buf) - .map_err(|_| Error(ErrorKind::NoHeader))? + .map_err(|_| Error::NoHeader)? .0; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u32::from(header.magic) != spec::MAGIC { - return Err(Error(ErrorKind::HeaderMagic)); + return Err(Error::HeaderMagic); } // Validate total size within buf. let total_size = u32::from(header.totalsize) as usize; if total_size > buf.len() { - return Err(Error(ErrorKind::HeaderTotalSize)); + return Err(Error::HeaderTotalSize); } if u32::from(header.version) < spec::CURRENT_VERSION || u32::from(header.last_comp_version) > spec::COMPAT_VERSION { - return Err(Error(ErrorKind::HeaderVersion)); + return Err(Error::HeaderVersion); } // Validate the mem_rsvmap region ends with an empty entry. Currently @@ -215,10 +165,10 @@ impl<'a> Parser<'a> { let mut memory_reservations_len = 0; let mut mem_rsvmap = buf .get(mem_rsvmap_offset..) - .ok_or(Error(ErrorKind::MemoryReservationBlock))?; + .ok_or(Error::MemoryReservationBlock)?; loop { let (entry, rest) = spec::ReserveEntry::read_from_prefix(mem_rsvmap) - .map_err(|_| Error(ErrorKind::MemoryReservationBlockEnd))?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) + .map_err(|_| Error::MemoryReservationBlockEnd)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u64::from(entry.address) == 0 && u64::from(entry.size) == 0 { break; @@ -230,30 +180,30 @@ impl<'a> Parser<'a> { let memory_reservations = buf .get(mem_rsvmap_offset..(mem_rsvmap_offset + memory_reservations_len)) - .ok_or(Error(ErrorKind::MemoryReservationBlock))?; + .ok_or(Error::MemoryReservationBlock)?; let struct_offset = u32::from(header.off_dt_struct) as usize; let struct_len = u32::from(header.size_dt_struct) as usize; if struct_offset % size_of::() != 0 { - return Err(Error(ErrorKind::StructureBlockAlignment)); + return Err(Error::StructureBlockAlignment); } let structure_block = buf .get(struct_offset..(struct_offset + struct_len)) - .ok_or(Error(ErrorKind::StructureBlock))?; + .ok_or(Error::StructureBlock)?; // FDT_END must be the last token in the structure block. Ignore it once // checked. let structure_block = structure_block .strip_suffix(&spec::END.to_be_bytes()) - .ok_or(Error(ErrorKind::FdtEnd))?; + .ok_or(Error::FdtEnd)?; let strings_offset = u32::from(header.off_dt_strings) as usize; let strings_len = u32::from(header.size_dt_strings) as usize; let strings_block = buf .get(strings_offset..(strings_offset + strings_len)) - .ok_or(Error(ErrorKind::StringsBlock))?; + .ok_or(Error::StringsBlock)?; Ok(Self { total_size, @@ -271,10 +221,10 @@ impl<'a> Parser<'a> { nodes: self.structure_block, }; - let root = iter.next().ok_or(Error(ErrorKind::RootNode))??; + let root = iter.next().ok_or(Error::RootNode)??; if iter.next().is_some() { - Err(Error(ErrorKind::MultipleRootNodes)) + Err(Error::MultipleRootNodes) } else { Ok(root) } @@ -322,43 +272,29 @@ impl ParsedToken<'_> { } /// Errors returned when parsing FDT tokens. -#[derive(Debug)] -enum ParseTokenError { +/// Errors returned when parsing FDT tokens. +#[derive(Debug, Error)] +pub enum ParseTokenError { /// Unknown token + #[error("Unknown FDT token {0}")] Unknown(u32), /// Buf too small + #[error("Buffer too small to read token")] BufLen, /// Buf too small for prop header + #[error("Buffer too small to read property header")] PropHeader, /// Buf too small for prop data described in prop header + #[error("Buffer too small to read property data encoded in property header")] PropData, /// Begin node name is not valid - BeginName(StringError), + #[error("Node name is not valid {0}")] + BeginName(#[from] StringError), /// Buf too small for begin node name alignment + #[error("Buffer too small for begin node name alignment")] BeginNameAlignment, } -impl Display for ParseTokenError { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - ParseTokenError::Unknown(token) => { - f.write_fmt(format_args!("Unknown FDT token {}", token)) - } - ParseTokenError::BufLen => f.write_str("Buffer too small to read token"), - ParseTokenError::PropHeader => f.write_str("Buffer too small to read property header"), - ParseTokenError::PropData => { - f.write_str("Buffer too small to read property data encoded in property header") - } - ParseTokenError::BeginName(e) => { - f.write_fmt(format_args!("Node name is not valid {}", e)) - } - ParseTokenError::BeginNameAlignment => { - f.write_str("Buffer is too small for begin node name alignment") - } - } - } -} - /// Read to the next token from `buf`, returning `(token, remaining_buffer)`. fn read_token(buf: &[u8]) -> Result<(ParsedToken<'_>, &[u8]), ParseTokenError> { let (token, rest) = U32b::read_from_prefix(buf).map_err(|_| ParseTokenError::BufLen)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) @@ -411,10 +347,10 @@ fn read_token(buf: &[u8]) -> Result<(ParsedToken<'_>, &[u8]), ParseTokenError> { } impl<'a> NodeIter<'a> { - fn parse(&mut self) -> Result>, ErrorKind<'a>> { + fn parse(&mut self) -> Result>, Error<'a>> { while !self.nodes.is_empty() { // Parse the next token. - let (token, rest) = read_token(self.nodes).map_err(ErrorKind::NodeToken)?; + let (token, rest) = read_token(self.nodes)?; debug_assert!(rest.len() % size_of::() == 0); let name = match token { @@ -423,7 +359,7 @@ impl<'a> NodeIter<'a> { continue; } ParsedToken::BeginNode { name } => name, - _ => return Err(ErrorKind::NodeBegin(token.raw())), + _ => return Err(Error::NodeBegin(token.raw())), }; self.nodes = rest; @@ -431,7 +367,7 @@ impl<'a> NodeIter<'a> { // Find if there is a properties section, which comes before children. let mut prop = self.nodes; 'prop: loop { - let (token, rest) = read_token(prop).map_err(ErrorKind::NodeToken)?; + let (token, rest) = read_token(prop)?; match token { ParsedToken::BeginNode { .. } => { // Begin node means move to parsing children nodes. @@ -442,7 +378,7 @@ impl<'a> NodeIter<'a> { break 'prop; } ParsedToken::Property { .. } | ParsedToken::Nop => {} - token => return Err(ErrorKind::NodeProp(token.raw())), + token => return Err(Error::NodeProp(token.raw())), }; prop = rest; @@ -456,7 +392,7 @@ impl<'a> NodeIter<'a> { let mut children = self.nodes; let mut begin_node_count = 0; 'children: loop { - let (token, rest) = read_token(children).map_err(ErrorKind::NodeToken)?; + let (token, rest) = read_token(children)?; match token { ParsedToken::EndNode => { if begin_node_count == 0 { @@ -471,7 +407,7 @@ impl<'a> NodeIter<'a> { begin_node_count += 1; } ParsedToken::Property { .. } | ParsedToken::Nop => {} - token => return Err(ErrorKind::NodeChildren(token.raw())), + token => return Err(Error::NodeChildren(token.raw())), }; children = rest; @@ -501,7 +437,7 @@ impl<'a> Iterator for NodeIter<'a> { type Item = Result, Error<'a>>; fn next(&mut self) -> Option { - self.parse().map_err(Error).transpose() + self.parse().transpose() } } @@ -561,11 +497,11 @@ pub struct PropertyIter<'a> { } impl<'a> PropertyIter<'a> { - fn parse(&mut self) -> Result>, ErrorKind<'a>> { + fn parse(&mut self) -> Result>, Error<'a>> { while !self.properties.is_empty() { // Parse the next token. let (token, rest) = - read_token(self.properties).map_err(|error| ErrorKind::PropertyTokenParse { + read_token(self.properties).map_err(|error| Error::PropertyTokenParse { node_name: self.node_name, error, })?; @@ -577,7 +513,7 @@ impl<'a> PropertyIter<'a> { } ParsedToken::Property { name_offset, data } => (name_offset, data, rest), _ => { - return Err(ErrorKind::PropertyToken { + return Err(Error::PropertyToken { node_name: self.node_name, token: token.raw(), }); @@ -586,7 +522,7 @@ impl<'a> PropertyIter<'a> { // Read the property name let name = string_from_offset(self.strings_block, name_off).map_err(|error| { - ErrorKind::PropertyNameStr { + Error::PropertyNameStr { node_name: self.node_name, error, } @@ -608,7 +544,7 @@ impl<'a> Iterator for PropertyIter<'a> { type Item = Result, Error<'a>>; fn next(&mut self) -> Option { - self.parse().map_err(Error).transpose() + self.parse().transpose() } } @@ -640,16 +576,16 @@ impl<'a> Property<'a> { <[T]>::ref_from_bytes(self.data) .map_err(|_| { // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) - Error(ErrorKind::PropertyDataTypeBuffer { + Error::PropertyDataTypeBuffer { node_name: self.node_name, prop_name: self.name, - }) + } })? .get(index) - .ok_or(Error(ErrorKind::PropertyOffset { + .ok_or(Error::PropertyOffset { node_name: self.node_name, prop_name: self.name, - })) + }) .copied() } @@ -670,10 +606,10 @@ impl<'a> Property<'a> { /// Read the data as a `&str`. pub fn read_str(&self) -> Result<&'a str, Error<'a>> { extract_str_from_bytes(self.data).map_err(|error| { - Error(ErrorKind::PropertyStr { + Error::PropertyStr { node_name: self.node_name, error, - }) + } }) } @@ -682,35 +618,28 @@ impl<'a> Property<'a> { Ok(<[U64b]>::ref_from_bytes(self.data) .map_err(|_| { // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) - Error(ErrorKind::PropertyDataTypeBuffer { + Error::PropertyDataTypeBuffer { node_name: self.node_name, prop_name: self.name, - }) + } })? .iter() .map(|v| v.get())) } } -/// Errors when reading a string from the FDT. -#[derive(Debug)] +/// String parsing errors +#[derive(Debug, Error)] enum StringError { /// Invalid string block offset + #[error("Invalid string block offset")] Offset, /// No null terminator found + #[error("No null terminator found")] Null, /// String is not utf8 - Utf8(core::str::Utf8Error), -} - -impl Display for StringError { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - StringError::Offset => f.write_str("Invalid string block offset"), - StringError::Null => f.write_str("No null terminator found"), - StringError::Utf8(e) => f.write_fmt(format_args!("String is not utf8 {}", e)), - } - } + #[error("String is not utf8 {0}")] + Utf8(#[from] core::str::Utf8Error), } /// An iterator to parse through memory reservations. @@ -719,13 +648,13 @@ pub struct MemoryReserveIter<'a> { } impl<'a> MemoryReserveIter<'a> { - fn parse(&mut self) -> Result, ErrorKind<'a>> { + fn parse(&mut self) -> Result, Error<'a>> { if self.memory_reservations.is_empty() { return Ok(None); } let (entry, rest) = spec::ReserveEntry::read_from_prefix(self.memory_reservations) - .map_err(|_| ErrorKind::MemoryReservationBlock)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) + .map_err(|_| Error::MemoryReservationBlock)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u64::from(entry.address) == 0 && u64::from(entry.size) == 0 { return Ok(None); @@ -741,12 +670,10 @@ impl<'a> Iterator for MemoryReserveIter<'a> { type Item = Result>; fn next(&mut self) -> Option { - self.parse().map_err(Error).transpose() + self.parse().transpose() } } -impl core::error::Error for StringError {} - /// Extract a string from bytes treated as a C String, stopping at the first null terminator. fn extract_str_from_bytes(bytes: &[u8]) -> Result<&str, StringError> { // Find the null terminator. diff --git a/vm/hv1/hvdef/Cargo.toml b/vm/hv1/hvdef/Cargo.toml index db94e9b0d1..c7b0775155 100644 --- a/vm/hv1/hvdef/Cargo.toml +++ b/vm/hv1/hvdef/Cargo.toml @@ -10,6 +10,7 @@ rust-version.workspace = true open_enum.workspace = true bitfield-struct.workspace = true static_assertions.workspace = true +thiserror.workspace = true zerocopy.workspace = true [lints] workspace = true From 4e504f492b52f80224f3a4395ec3b5b4ba19566c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 Aug 2025 23:19:51 +0000 Subject: [PATCH 3/9] Migrate host_fdt_parser error types to thiserror Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- openhcl/host_fdt_parser/src/lib.rs | 264 ++++++++++++++--------------- 1 file changed, 131 insertions(+), 133 deletions(-) diff --git a/openhcl/host_fdt_parser/src/lib.rs b/openhcl/host_fdt_parser/src/lib.rs index 734282b5ed..c70a1d84ed 100644 --- a/openhcl/host_fdt_parser/src/lib.rs +++ b/openhcl/host_fdt_parser/src/lib.rs @@ -13,7 +13,6 @@ use arrayvec::ArrayString; use arrayvec::ArrayVec; -use core::fmt::Display; use core::fmt::Write; use core::mem::size_of; use hvdef::HV_PAGE_SIZE; @@ -66,7 +65,6 @@ pub enum Error<'a> { #[error("invalid device tree node with parent {parent_name}: {error}")] Node { parent_name: &'a str, - #[source] error: fdt::parser::Error<'a>, }, /// Required property missing @@ -311,15 +309,15 @@ impl< /// Parse the given device tree. pub fn parse(dt: &'a [u8], storage: &'b mut Self) -> Result<&'b Self, Error<'a>> { - Self::parse_inner(dt, storage).map_err(Error) + Self::parse_inner(dt, storage) } - fn parse_inner(dt: &'a [u8], storage: &'b mut Self) -> Result<&'b Self, ErrorKind<'a>> { - let parser = fdt::parser::Parser::new(dt).map_err(ErrorKind::Dt)?; + fn parse_inner(dt: &'a [u8], storage: &'b mut Self) -> Result<&'b Self, Error<'a>> { + let parser = fdt::parser::Parser::new(dt).map_err(Error::Dt)?; let root = match parser.root() { Ok(v) => v, Err(e) => { - return Err(ErrorKind::Node { + return Err(Error::Node { parent_name: "", error: e, }); @@ -332,10 +330,10 @@ impl< // after all entries are parsed once sort is stabilized in core. let insert_memory_entry = |memory: &mut ArrayVec, entry: MemoryEntry| - -> Result<(), ErrorKind<'a>> { + -> Result<(), Error<'a>> { let insert_index = match memory.binary_search_by_key(&entry.range, |k| k.range) { Ok(index) => { - return Err(ErrorKind::MemoryRegOverlap { + return Err(Error::MemoryRegOverlap { lower: memory[index], upper: entry, }); @@ -345,11 +343,11 @@ impl< memory .try_insert(insert_index, entry) - .map_err(|_| ErrorKind::TooManyMemoryEntries) + .map_err(|_| Error::TooManyMemoryEntries) }; for child in root.children() { - let child = child.map_err(|error| ErrorKind::Node { + let child = child.map_err(|error| Error::Node { parent_name: root.name, error, })?; @@ -358,18 +356,18 @@ impl< "cpus" => { let address_cells = child .find_property("#address-cells") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: "#address-cells", })? .read_u32(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; // On ARM v8 64-bit systems, up to 2 address-cells values // can be provided. if address_cells > 2 { - return Err(ErrorKind::PropInvalidU32 { + return Err(Error::PropInvalidU32 { node_name: child.name, prop_name: "#address-cells", expected: 2, @@ -378,20 +376,20 @@ impl< } for cpu in child.children() { - let cpu = cpu.map_err(|error| ErrorKind::Node { + let cpu = cpu.map_err(|error| Error::Node { parent_name: child.name, error, })?; if cpu .find_property("status") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: cpu.name, prop_name: "status", })? .read_str() - .map_err(ErrorKind::Prop)? + .map_err(Error::Prop)? != "okay" { continue; @@ -402,61 +400,61 @@ impl< // correlation in the device tree about this at all. let reg_property = cpu .find_property("reg") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: cpu.name, prop_name: "reg", })?; let reg = if address_cells == 1 { - reg_property.read_u32(0).map_err(ErrorKind::Prop)? as u64 + reg_property.read_u32(0).map_err(Error::Prop)? as u64 } else { - reg_property.read_u64(0).map_err(ErrorKind::Prop)? + reg_property.read_u64(0).map_err(Error::Prop)? }; let vnode = cpu .find_property("numa-node-id") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: cpu.name, prop_name: "numa-node-id", })? .read_u32(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; storage .cpus .try_push(CpuEntry { reg, vnode }) - .map_err(|_| ErrorKind::TooManyCpus)?; + .map_err(|_| Error::TooManyCpus)?; } } "openhcl" => { let memory_allocation_mode = child .find_property("memory-allocation-mode") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: "memory-allocation-mode", })?; - match memory_allocation_mode.read_str().map_err(ErrorKind::Prop)? { + match memory_allocation_mode.read_str().map_err(Error::Prop)? { "host" => { storage.memory_allocation_mode = MemoryAllocationMode::Host; } "vtl2" => { let memory_size = child .find_property("memory-size") - .map_err(ErrorKind::Prop)? + .map_err(Error::Prop)? .map(|p| p.read_u64(0)) .transpose() - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; let mmio_size = child .find_property("mmio-size") - .map_err(ErrorKind::Prop)? + .map_err(Error::Prop)? .map(|p| p.read_u64(0)) .transpose() - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; storage.memory_allocation_mode = MemoryAllocationMode::Vtl2 { memory_size, @@ -464,19 +462,19 @@ impl< }; } mode => { - return Err(ErrorKind::UnexpectedMemoryAllocationMode { mode }); + return Err(Error::UnexpectedMemoryAllocationMode { mode }); } } storage.vtl0_alias_map = child .find_property("vtl0-alias-map") - .map_err(ErrorKind::Prop)? + .map_err(Error::Prop)? .map(|p| p.read_u64(0)) .transpose() - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; for openhcl_child in child.children() { - let openhcl_child = openhcl_child.map_err(|error| ErrorKind::Node { + let openhcl_child = openhcl_child.map_err(|error| Error::Node { parent_name: root.name, error, })?; @@ -485,8 +483,8 @@ impl< "entropy" => { let host_entropy = openhcl_child .find_property("reg") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: openhcl_child.name, prop_name: "reg", })? @@ -534,16 +532,16 @@ impl< _ if child.name.starts_with("memory@") => { let igvm_type = if let Some(igvm_type) = child .find_property(igvm_defs::dt::IGVM_DT_IGVM_TYPE_PROPERTY) - .map_err(ErrorKind::Prop)? + .map_err(Error::Prop)? { - let typ = igvm_type.read_u32(0).map_err(ErrorKind::Prop)?; + let typ = igvm_type.read_u32(0).map_err(Error::Prop)?; MemoryMapEntryType(typ as u16) } else { MemoryMapEntryType::MEMORY }; - let reg = child.find_property("reg").map_err(ErrorKind::Prop)?.ok_or( - ErrorKind::PropMissing { + let reg = child.find_property("reg").map_err(Error::Prop)?.ok_or( + Error::PropMissing { node_name: child.name, prop_name: "reg", }, @@ -551,24 +549,24 @@ impl< let vnode = child .find_property("numa-node-id") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: "numa-node-id", })? .read_u32(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; let len = reg.data.len(); let reg_tuple_size = size_of::() * 2; let number_of_ranges = len / reg_tuple_size; for i in 0..number_of_ranges { - let base = reg.read_u64(i * 2).map_err(ErrorKind::Prop)?; - let len = reg.read_u64(i * 2 + 1).map_err(ErrorKind::Prop)?; + let base = reg.read_u64(i * 2).map_err(Error::Prop)?; + let len = reg.read_u64(i * 2 + 1).map_err(Error::Prop)?; if base % HV_PAGE_SIZE != 0 || len % HV_PAGE_SIZE != 0 { - return Err(ErrorKind::MemoryRegUnaligned { + return Err(Error::MemoryRegUnaligned { node_name: child.name, base, len, @@ -589,13 +587,13 @@ impl< "chosen" => { let cmdline = child .find_property("bootargs") - .map_err(ErrorKind::Prop)? - .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) + .map_err(Error::Prop)? + .map(|prop| prop.read_str().map_err(Error::Prop)) .transpose()? .unwrap_or(""); write!(storage.command_line, "{}", cmdline) - .map_err(|_| ErrorKind::CmdlineSize)?; + .map_err(|_| Error::CmdlineSize)?; } _ if child.name.starts_with("intc@") => { validate_property_str(&child, "compatible", "arm,gic-v3")?; @@ -606,29 +604,29 @@ impl< let gic_redistributor_stride = child .find_property("redistributor-stride") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: "redistributor-stride", })? .read_u64(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; let gic_reg_property = child .find_property("reg") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: "reg", })?; let gic_distributor_base = - gic_reg_property.read_u64(0).map_err(ErrorKind::Prop)?; + gic_reg_property.read_u64(0).map_err(Error::Prop)?; let gic_distributor_size = - gic_reg_property.read_u64(1).map_err(ErrorKind::Prop)?; + gic_reg_property.read_u64(1).map_err(Error::Prop)?; let gic_redistributors_base = - gic_reg_property.read_u64(2).map_err(ErrorKind::Prop)?; + gic_reg_property.read_u64(2).map_err(Error::Prop)?; let gic_redistributors_size = - gic_reg_property.read_u64(3).map_err(ErrorKind::Prop)?; + gic_reg_property.read_u64(3).map_err(Error::Prop)?; storage.gic = Some(GicInfo { gic_distributor_base, @@ -653,7 +651,7 @@ impl< // Validate memory entries do not overlap. for (prev, next) in storage.memory.iter().zip(storage.memory.iter().skip(1)) { if prev.range.overlaps(&next.range) { - return Err(ErrorKind::MemoryRegOverlap { + return Err(Error::MemoryRegOverlap { lower: *prev, upper: *next, }); @@ -676,7 +674,7 @@ impl< for ram in storage.memory.iter() { for mmio in vmbus_vtl0_mmio { if mmio.overlaps(&ram.range) { - return Err(ErrorKind::VmbusMmioOverlapsRam { + return Err(Error::VmbusMmioOverlapsRam { mmio: *mmio, ram: *ram, }); @@ -685,7 +683,7 @@ impl< for mmio in vmbus_vtl2_mmio { if mmio.overlaps(&ram.range) { - return Err(ErrorKind::VmbusMmioOverlapsRam { + return Err(Error::VmbusMmioOverlapsRam { mmio: *mmio, ram: *ram, }); @@ -696,7 +694,7 @@ impl< for vtl0_mmio in vmbus_vtl0_mmio { for vtl2_mmio in vmbus_vtl2_mmio { if vtl0_mmio.overlaps(vtl2_mmio) { - return Err(ErrorKind::VmbusMmioOverlapsVmbusMmio { + return Err(Error::VmbusMmioOverlapsVmbusMmio { mmio_a: *vtl0_mmio, mmio_b: *vtl2_mmio, }); @@ -736,11 +734,11 @@ fn parse_compatible<'a>( vmbus_vtl2: &mut Option, pmu_gsiv: &mut Option, com3_serial: &mut bool, -) -> Result<(), ErrorKind<'a>> { +) -> Result<(), Error<'a>> { let compatible = node .find_property("compatible") - .map_err(ErrorKind::Prop)? - .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) + .map_err(Error::Prop)? + .map(|prop| prop.read_str().map_err(Error::Prop)) .transpose()? .unwrap_or(""); @@ -760,20 +758,20 @@ fn parse_compatible<'a>( Ok(()) } -fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> { +fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> { // Validate address cells and size cells are 2 let address_cells = node .find_property("#address-cells") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: node.name, prop_name: "#address-cells", })? .read_u32(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; if address_cells != 2 { - return Err(ErrorKind::PropInvalidU32 { + return Err(Error::PropInvalidU32 { node_name: node.name, prop_name: "#address-cells", expected: 2, @@ -783,16 +781,16 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result(node: &fdt::parser::Node<'a>) -> Result = - match node.find_property("ranges").map_err(ErrorKind::Prop)? { + match node.find_property("ranges").map_err(Error::Prop)? { Some(ranges) => { // Determine how many mmio ranges this describes. Valid numbers are // 0, 1 or 2. @@ -810,19 +808,19 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result 2 { - return Err(ErrorKind::TooManyVmbusMmioRanges { + return Err(Error::TooManyVmbusMmioRanges { node_name: node.name, ranges: number_of_ranges, }); } for i in 0..number_of_ranges { - let child_base = ranges.read_u64(i * 3).map_err(ErrorKind::Prop)?; - let parent_base = ranges.read_u64(i * 3 + 1).map_err(ErrorKind::Prop)?; - let len = ranges.read_u64(i * 3 + 2).map_err(ErrorKind::Prop)?; + let child_base = ranges.read_u64(i * 3).map_err(Error::Prop)?; + let parent_base = ranges.read_u64(i * 3 + 1).map_err(Error::Prop)?; + let len = ranges.read_u64(i * 3 + 2).map_err(Error::Prop)?; if child_base != parent_base { - return Err(ErrorKind::VmbusRangesChildParent { + return Err(Error::VmbusRangesChildParent { node_name: node.name, child_base, parent_base, @@ -830,7 +828,7 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result(node: &fdt::parser::Node<'a>) -> Result 1 && mmio[0].overlaps(&mmio[1]) { - return Err(ErrorKind::VmbusMmioOverlapsVmbusMmio { + return Err(Error::VmbusMmioOverlapsVmbusMmio { mmio_a: mmio[0], mmio_b: mmio[1], }); @@ -865,13 +863,13 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result( node: &fdt::parser::Node<'a>, vmbus_vtl0: &mut Option, vmbus_vtl2: &mut Option, -) -> Result<(), ErrorKind<'a>> { +) -> Result<(), Error<'a>> { // Vmbus must be under simple-bus node with empty ranges. if !node .find_property("ranges") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: node.name, prop_name: "ranges", })? @@ -899,15 +897,15 @@ fn parse_simple_bus<'a>( } for child in node.children() { - let child = child.map_err(|error| ErrorKind::Node { + let child = child.map_err(|error| Error::Node { parent_name: node.name, error, })?; let compatible = child .find_property("compatible") - .map_err(ErrorKind::Prop)? - .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) + .map_err(Error::Prop)? + .map(|prop| prop.read_str().map_err(Error::Prop)) .transpose()? .unwrap_or(""); @@ -915,31 +913,31 @@ fn parse_simple_bus<'a>( let vtl_name = igvm_defs::dt::IGVM_DT_VTL_PROPERTY; let vtl = child .find_property(vtl_name) - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: vtl_name, })? .read_u32(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; match vtl { 0 => { if vmbus_vtl0.replace(parse_vmbus(&child)?).is_some() { - return Err(ErrorKind::MultipleVmbusNode { + return Err(Error::MultipleVmbusNode { node_name: child.name, }); } } 2 => { if vmbus_vtl2.replace(parse_vmbus(&child)?).is_some() { - return Err(ErrorKind::MultipleVmbusNode { + return Err(Error::MultipleVmbusNode { node_name: child.name, }); } } _ => { - return Err(ErrorKind::UnexpectedVmbusVtl { + return Err(Error::UnexpectedVmbusVtl { node_name: child.name, vtl, }); @@ -954,40 +952,40 @@ fn parse_simple_bus<'a>( fn parse_io_bus<'a>( node: &fdt::parser::Node<'a>, com3_serial: &mut bool, -) -> Result<(), ErrorKind<'a>> { +) -> Result<(), Error<'a>> { for io_bus_child in node.children() { - let io_bus_child = io_bus_child.map_err(|error| ErrorKind::Node { + let io_bus_child = io_bus_child.map_err(|error| Error::Node { parent_name: node.name, error, })?; let compatible: &str = io_bus_child .find_property("compatible") - .map_err(ErrorKind::Prop)? - .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) + .map_err(Error::Prop)? + .map(|prop| prop.read_str().map_err(Error::Prop)) .transpose()? .unwrap_or(""); let _current_speed = io_bus_child .find_property("current-speed") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: io_bus_child.name, prop_name: "current-speed", })? .read_u32(0) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; let reg = io_bus_child .find_property("reg") - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: io_bus_child.name, prop_name: "reg", })?; - let reg_base = reg.read_u64(0).map_err(ErrorKind::Prop)?; - let _reg_len = reg.read_u64(1).map_err(ErrorKind::Prop)?; + let reg_base = reg.read_u64(0).map_err(Error::Prop)?; + let _reg_len = reg.read_u64(1).map_err(Error::Prop)?; // Linux kernel hard-codes COM3 to COM3_REG_BASE. // If work is ever done in the Linux kernel to instead @@ -1008,15 +1006,15 @@ fn parse_io_bus<'a>( fn parse_pmu_gsiv<'a>( node: &fdt::parser::Node<'a>, pmu_gsiv: &mut Option, -) -> Result<(), ErrorKind<'a>> { - let interrupts = node.find_property("interrupts").map_err(ErrorKind::Prop)?; - let interrupts = interrupts.ok_or(ErrorKind::PropMissing { +) -> Result<(), Error<'a>> { + let interrupts = node.find_property("interrupts").map_err(Error::Prop)?; + let interrupts = interrupts.ok_or(Error::PropMissing { node_name: node.name, prop_name: "interrupts", })?; if interrupts.data.len() < 3 * size_of::() { - return Err(ErrorKind::PropInvalidU32 { + return Err(Error::PropInvalidU32 { node_name: node.name, prop_name: "interrupts size", expected: 3 * size_of::() as u32, @@ -1027,21 +1025,21 @@ fn parse_pmu_gsiv<'a>( // This parser expects the PMU GSIV to be a PPI, as all platforms that // support OpenHCL should be using PPIs. const GIC_PPI: u32 = 1; - let interrupt_type = interrupts.read_u32(0).map_err(ErrorKind::Prop)?; + let interrupt_type = interrupts.read_u32(0).map_err(Error::Prop)?; if interrupt_type != GIC_PPI { - return Err(ErrorKind::PropInvalidU32 { + return Err(Error::PropInvalidU32 { node_name: node.name, prop_name: "interrupts", expected: GIC_PPI, actual: interrupt_type, }); } - let interrupt_id = interrupts.read_u32(1).map_err(ErrorKind::Prop)?; + let interrupt_id = interrupts.read_u32(1).map_err(Error::Prop)?; // Interrupt id describes the index from the PPI start of 16. It must be // smaller than 16, as PPIs only exist from 16 to 31. if interrupt_id >= 16 { - return Err(ErrorKind::PropInvalidU32 { + return Err(Error::PropInvalidU32 { node_name: node.name, prop_name: "interrupts", expected: 16, @@ -1059,18 +1057,18 @@ fn validate_property_str<'a>( child: &fdt::parser::Node<'a>, name: &'static str, expected: &'static str, -) -> Result<(), ErrorKind<'a>> { +) -> Result<(), Error<'a>> { let actual = child .find_property(name) - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: name, })? .read_str() - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; if actual != expected { - return Err(ErrorKind::PropInvalidStr { + return Err(Error::PropInvalidStr { node_name: child.name, prop_name: name, expected, @@ -1086,18 +1084,18 @@ fn validate_property_u32<'a>( name: &'static str, expected: u32, index: usize, -) -> Result<(), ErrorKind<'a>> { +) -> Result<(), Error<'a>> { let actual = child .find_property(name) - .map_err(ErrorKind::Prop)? - .ok_or(ErrorKind::PropMissing { + .map_err(Error::Prop)? + .ok_or(Error::PropMissing { node_name: child.name, prop_name: name, })? .read_u32(index) - .map_err(ErrorKind::Prop)?; + .map_err(Error::Prop)?; if actual != expected { - return Err(ErrorKind::PropInvalidU32 { + return Err(Error::PropInvalidU32 { node_name: child.name, prop_name: name, expected, @@ -1692,7 +1690,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error(ErrorKind::MemoryRegOverlap { .. })) + Err(Error::MemoryRegOverlap { .. }) )); // mem contained within another @@ -1731,7 +1729,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error(ErrorKind::MemoryRegOverlap { .. })) + Err(Error::MemoryRegOverlap { .. }) )); // mem overlaps vmbus @@ -1775,7 +1773,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error(ErrorKind::VmbusMmioOverlapsRam { .. })) + Err(Error::VmbusMmioOverlapsRam { .. }) )); // vmbus overlap each other @@ -1819,7 +1817,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error(ErrorKind::VmbusMmioOverlapsVmbusMmio { .. })) + Err(Error::VmbusMmioOverlapsVmbusMmio { .. }) )); } From 42c76bef03214d9432a4afe9a54977c87dba8f99 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 Aug 2025 23:25:36 +0000 Subject: [PATCH 4/9] Complete migration of remaining no_std error types to thiserror Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- vm/acpi_spec/src/srat.rs | 30 +++++++++--------------------- vm/hv1/hvdef/src/lib.rs | 5 ++--- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/vm/acpi_spec/src/srat.rs b/vm/acpi_spec/src/srat.rs index bf848f046b..ec9745260d 100644 --- a/vm/acpi_spec/src/srat.rs +++ b/vm/acpi_spec/src/srat.rs @@ -8,6 +8,7 @@ use super::Table; use crate::packed_nums::*; use core::mem::size_of; use static_assertions::const_assert_eq; +use thiserror::Error; use zerocopy::FromBytes; use zerocopy::Immutable; use zerocopy::IntoBytes; @@ -205,37 +206,24 @@ impl SratMemory { } } -#[derive(Debug)] +#[derive(Debug, Error)] pub enum ParseSratError { + #[error("could not read standard ACPI header")] MissingAcpiHeader, + #[error("invalid signature. expected b\"SRAT\", found {0:?}")] InvalidSignature([u8; 4]), + #[error("mismatched len. in_header: {in_header}, actual {actual}")] MismatchedLength { in_header: usize, actual: usize }, + #[error("missing fixed SRAT header")] MissingFixedHeader, + #[error("could not read APIC structure")] BadApic, + #[error("could not read MEMORY structure")] BadMemory, + #[error("unknown SRAT structure type: {0}")] UnknownType(u8), } -impl core::fmt::Display for ParseSratError { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - Self::MissingAcpiHeader => write!(f, "could not read standard ACPI header"), - Self::InvalidSignature(sig) => { - write!(f, "invalid signature. expected b\"SRAT\", found {sig:?}") - } - Self::MismatchedLength { in_header, actual } => { - write!(f, "mismatched len. in_header: {in_header}, actual {actual}") - } - Self::MissingFixedHeader => write!(f, "missing fixed SRAT header"), - Self::BadApic => write!(f, "could not read APIC structure"), - Self::BadMemory => write!(f, "could not read MEMORY structure"), - Self::UnknownType(ty) => write!(f, "unknown SRAT structure type: {ty}"), - } - } -} - -impl core::error::Error for ParseSratError {} - pub fn parse_srat<'a>( raw_srat: &'a [u8], mut on_apic: impl FnMut(&'a SratApic), diff --git a/vm/hv1/hvdef/src/lib.rs b/vm/hv1/hvdef/src/lib.rs index 65ec26f177..bf957a9639 100644 --- a/vm/hv1/hvdef/src/lib.rs +++ b/vm/hv1/hvdef/src/lib.rs @@ -12,6 +12,7 @@ use core::fmt::Debug; use core::mem::size_of; use open_enum::open_enum; use static_assertions::const_assert; +use thiserror::Error; use zerocopy::FromBytes; use zerocopy::FromZeros; use zerocopy::Immutable; @@ -423,7 +424,7 @@ impl Debug for HvStatus { // // DEVNOTE: use `NonZeroU16` to get a niche optimization, since 0 is reserved // for success. -#[derive(Copy, Clone, PartialEq, Eq, IntoBytes, Immutable, KnownLayout)] +#[derive(Copy, Clone, PartialEq, Eq, IntoBytes, Immutable, KnownLayout, Error)] #[repr(transparent)] pub struct HvError(core::num::NonZeroU16); @@ -451,8 +452,6 @@ impl core::fmt::Display for HvError { } } -impl core::error::Error for HvError {} - macro_rules! hv_error { ($ty:ty, $(#[doc = $doc:expr] $ident:ident = $val:expr),* $(,)?) => { From 33f935f878eef5e3900326b21f3dca50ed75b719 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 Aug 2025 23:26:51 +0000 Subject: [PATCH 5/9] repo: move no_std custom error types over to thiserror Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- openhcl/host_fdt_parser/src/lib.rs | 168 +++++++++++++---------------- support/fdt/src/parser.rs | 16 +-- 2 files changed, 84 insertions(+), 100 deletions(-) diff --git a/openhcl/host_fdt_parser/src/lib.rs b/openhcl/host_fdt_parser/src/lib.rs index c70a1d84ed..1467462c4a 100644 --- a/openhcl/host_fdt_parser/src/lib.rs +++ b/openhcl/host_fdt_parser/src/lib.rs @@ -96,7 +96,9 @@ pub enum Error<'a> { #[error("device tree contained more memory ranges than can be parsed")] TooManyMemoryEntries, /// Invalid u32 property value - #[error("{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}")] + #[error( + "{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}" + )] PropInvalidU32 { node_name: &'a str, prop_name: &'a str, @@ -104,7 +106,9 @@ pub enum Error<'a> { actual: u32, }, /// Invalid string property value - #[error("{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}")] + #[error( + "{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}" + )] PropInvalidStr { node_name: &'a str, prop_name: &'a str, @@ -113,15 +117,10 @@ pub enum Error<'a> { }, /// Unexpected VMBUS VTL #[error("{node_name} has an unexpected vtl {vtl}")] - UnexpectedVmbusVtl { - node_name: &'a str, - vtl: u32, - }, + UnexpectedVmbusVtl { node_name: &'a str, vtl: u32 }, /// Multiple VMBUS nodes #[error("{node_name} specifies a duplicate vmbus node")] - MultipleVmbusNode { - node_name: &'a str, - }, + MultipleVmbusNode { node_name: &'a str }, /// VMBUS ranges child/parent mismatch #[error("vmbus {node_name} ranges child base {child_base} does not match parent {parent_base}")] VmbusRangesChildParent { @@ -138,16 +137,10 @@ pub enum Error<'a> { }, /// Too many VMBUS MMIO ranges #[error("vmbus {node_name} has more than 2 mmio ranges {ranges}")] - TooManyVmbusMmioRanges { - node_name: &'a str, - ranges: usize, - }, + TooManyVmbusMmioRanges { node_name: &'a str, ranges: usize }, /// VMBUS MMIO overlaps RAM #[error("vmbus mmio at {}..{} overlaps ram at {}..{}", mmio.start(), mmio.end(), ram.range.start(), ram.range.end())] - VmbusMmioOverlapsRam { - mmio: MemoryRange, - ram: MemoryEntry, - }, + VmbusMmioOverlapsRam { mmio: MemoryRange, ram: MemoryEntry }, /// VMBUS MMIO overlaps VMBUS MMIO #[error("vmbus mmio at {}..{} overlaps vmbus mmio at {}..{}", mmio_a.start(), mmio_a.end(), mmio_b.start(), mmio_b.end())] VmbusMmioOverlapsVmbusMmio { @@ -159,9 +152,7 @@ pub enum Error<'a> { CmdlineSize, /// Unexpected memory allocation mode #[error("unexpected memory allocation mode: {mode}")] - UnexpectedMemoryAllocationMode { - mode: &'a str, - }, + UnexpectedMemoryAllocationMode { mode: &'a str }, } const COM3_REG_BASE: u64 = 0x3E8; @@ -398,13 +389,12 @@ impl< // NOTE: For x86, Underhill will need to query the hypervisor for // the vp_index to apic_id mapping. There's no // correlation in the device tree about this at all. - let reg_property = cpu - .find_property("reg") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + let reg_property = cpu.find_property("reg").map_err(Error::Prop)?.ok_or( + Error::PropMissing { node_name: cpu.name, prop_name: "reg", - })?; + }, + )?; let reg = if address_cells == 1 { reg_property.read_u32(0).map_err(Error::Prop)? as u64 @@ -592,8 +582,7 @@ impl< .transpose()? .unwrap_or(""); - write!(storage.command_line, "{}", cmdline) - .map_err(|_| Error::CmdlineSize)?; + write!(storage.command_line, "{}", cmdline).map_err(|_| Error::CmdlineSize)?; } _ if child.name.starts_with("intc@") => { validate_property_str(&child, "compatible", "arm,gic-v3")?; @@ -612,17 +601,14 @@ impl< .read_u64(0) .map_err(Error::Prop)?; - let gic_reg_property = child - .find_property("reg") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + let gic_reg_property = child.find_property("reg").map_err(Error::Prop)?.ok_or( + Error::PropMissing { node_name: child.name, prop_name: "reg", - })?; - let gic_distributor_base = - gic_reg_property.read_u64(0).map_err(Error::Prop)?; - let gic_distributor_size = - gic_reg_property.read_u64(1).map_err(Error::Prop)?; + }, + )?; + let gic_distributor_base = gic_reg_property.read_u64(0).map_err(Error::Prop)?; + let gic_distributor_size = gic_reg_property.read_u64(1).map_err(Error::Prop)?; let gic_redistributors_base = gic_reg_property.read_u64(2).map_err(Error::Prop)?; let gic_redistributors_size = @@ -798,68 +784,67 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> }); } - let mmio: ArrayVec = - match node.find_property("ranges").map_err(Error::Prop)? { - Some(ranges) => { - // Determine how many mmio ranges this describes. Valid numbers are - // 0, 1 or 2. - let ranges_tuple_size = size_of::() * 3; - let number_of_ranges = ranges.data.len() / ranges_tuple_size; - let mut mmio = ArrayVec::new(); - - if number_of_ranges > 2 { - return Err(Error::TooManyVmbusMmioRanges { - node_name: node.name, - ranges: number_of_ranges, - }); - } - - for i in 0..number_of_ranges { - let child_base = ranges.read_u64(i * 3).map_err(Error::Prop)?; - let parent_base = ranges.read_u64(i * 3 + 1).map_err(Error::Prop)?; - let len = ranges.read_u64(i * 3 + 2).map_err(Error::Prop)?; - - if child_base != parent_base { - return Err(Error::VmbusRangesChildParent { - node_name: node.name, - child_base, - parent_base, - }); - } - - if child_base % HV_PAGE_SIZE != 0 || len % HV_PAGE_SIZE != 0 { - return Err(Error::VmbusRangesNotAligned { - node_name: node.name, - base: child_base, - len, - }); - } + let mmio: ArrayVec = match node.find_property("ranges").map_err(Error::Prop)? { + Some(ranges) => { + // Determine how many mmio ranges this describes. Valid numbers are + // 0, 1 or 2. + let ranges_tuple_size = size_of::() * 3; + let number_of_ranges = ranges.data.len() / ranges_tuple_size; + let mut mmio = ArrayVec::new(); + + if number_of_ranges > 2 { + return Err(Error::TooManyVmbusMmioRanges { + node_name: node.name, + ranges: number_of_ranges, + }); + } - mmio.push( - MemoryRange::try_new(child_base..(child_base + len)).expect("valid range"), - ); - } + for i in 0..number_of_ranges { + let child_base = ranges.read_u64(i * 3).map_err(Error::Prop)?; + let parent_base = ranges.read_u64(i * 3 + 1).map_err(Error::Prop)?; + let len = ranges.read_u64(i * 3 + 2).map_err(Error::Prop)?; - // The DT ranges field might not have been sorted. Swap them if the - // low gap was described 2nd. - if number_of_ranges > 1 && mmio[0].start() > mmio[1].start() { - mmio.swap(0, 1); + if child_base != parent_base { + return Err(Error::VmbusRangesChildParent { + node_name: node.name, + child_base, + parent_base, + }); } - if number_of_ranges > 1 && mmio[0].overlaps(&mmio[1]) { - return Err(Error::VmbusMmioOverlapsVmbusMmio { - mmio_a: mmio[0], - mmio_b: mmio[1], + if child_base % HV_PAGE_SIZE != 0 || len % HV_PAGE_SIZE != 0 { + return Err(Error::VmbusRangesNotAligned { + node_name: node.name, + base: child_base, + len, }); } - mmio + mmio.push( + MemoryRange::try_new(child_base..(child_base + len)).expect("valid range"), + ); } - None => { - // No mmio is acceptable. - ArrayVec::new() + + // The DT ranges field might not have been sorted. Swap them if the + // low gap was described 2nd. + if number_of_ranges > 1 && mmio[0].start() > mmio[1].start() { + mmio.swap(0, 1); + } + + if number_of_ranges > 1 && mmio[0].overlaps(&mmio[1]) { + return Err(Error::VmbusMmioOverlapsVmbusMmio { + mmio_a: mmio[0], + mmio_b: mmio[1], + }); } - }; + + mmio + } + None => { + // No mmio is acceptable. + ArrayVec::new() + } + }; let connection_id = node .find_property("microsoft,message-connection-id") @@ -949,10 +934,7 @@ fn parse_simple_bus<'a>( Ok(()) } -fn parse_io_bus<'a>( - node: &fdt::parser::Node<'a>, - com3_serial: &mut bool, -) -> Result<(), Error<'a>> { +fn parse_io_bus<'a>(node: &fdt::parser::Node<'a>, com3_serial: &mut bool) -> Result<(), Error<'a>> { for io_bus_child in node.children() { let io_bus_child = io_bus_child.map_err(|error| Error::Node { parent_name: node.name, diff --git a/support/fdt/src/parser.rs b/support/fdt/src/parser.rs index 6d8f7e306e..c5cad5e7b7 100644 --- a/support/fdt/src/parser.rs +++ b/support/fdt/src/parser.rs @@ -64,13 +64,17 @@ pub enum Error<'a> { #[error("Unexpected token when parsing children nodes {0}")] NodeChildren(u32), /// Property data buffer len is not a multiple of requested type size - #[error("Property {prop_name} data buffer len is not multiple of type size for node {node_name}")] + #[error( + "Property {prop_name} data buffer len is not multiple of type size for node {node_name}" + )] PropertyDataTypeBuffer { node_name: &'a str, prop_name: &'a str, }, /// Property requested at offset is larger than data buffer - #[error("Property {prop_name} requested at offset is larger than data buffer for node {node_name}")] + #[error( + "Property {prop_name} requested at offset is larger than data buffer for node {node_name}" + )] PropertyOffset { node_name: &'a str, prop_name: &'a str, @@ -605,11 +609,9 @@ impl<'a> Property<'a> { /// Read the data as a `&str`. pub fn read_str(&self) -> Result<&'a str, Error<'a>> { - extract_str_from_bytes(self.data).map_err(|error| { - Error::PropertyStr { - node_name: self.node_name, - error, - } + extract_str_from_bytes(self.data).map_err(|error| Error::PropertyStr { + node_name: self.node_name, + error, }) } From f78c15d23c64d2f8aa4b3f09f65f094cd3258c87 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 11 Aug 2025 23:37:09 +0000 Subject: [PATCH 6/9] Fix StringError visibility in FDT parser Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- support/fdt/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/support/fdt/src/parser.rs b/support/fdt/src/parser.rs index c5cad5e7b7..0e629c92f4 100644 --- a/support/fdt/src/parser.rs +++ b/support/fdt/src/parser.rs @@ -632,7 +632,7 @@ impl<'a> Property<'a> { /// String parsing errors #[derive(Debug, Error)] -enum StringError { +pub enum StringError { /// Invalid string block offset #[error("Invalid string block offset")] Offset, From d96f3365ec97a20482287a19a057cba2a5064f63 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 Aug 2025 01:39:46 +0000 Subject: [PATCH 7/9] Add missing documentation for FDT error enum fields Add documentation for struct-like enum variant fields in the FDT parser to fix compilation errors with -D missing-docs. Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- support/fdt/src/parser.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/support/fdt/src/parser.rs b/support/fdt/src/parser.rs index 0e629c92f4..7ed3247a1a 100644 --- a/support/fdt/src/parser.rs +++ b/support/fdt/src/parser.rs @@ -68,7 +68,9 @@ pub enum Error<'a> { "Property {prop_name} data buffer len is not multiple of type size for node {node_name}" )] PropertyDataTypeBuffer { + /// Node name node_name: &'a str, + /// Property name prop_name: &'a str, }, /// Property requested at offset is larger than data buffer @@ -76,30 +78,43 @@ pub enum Error<'a> { "Property {prop_name} requested at offset is larger than data buffer for node {node_name}" )] PropertyOffset { + /// Node name node_name: &'a str, + /// Property name prop_name: &'a str, }, /// Property data is not a a valid string #[error("Property data is not a a valid string for node {node_name}: {error}")] PropertyStr { + /// Node name node_name: &'a str, + /// String parsing error #[source] error: StringError, }, /// Unable to parse FDT token when parsing properties #[error("Unable to parse FDT token when parsing properties for node {node_name}: {error}")] PropertyTokenParse { + /// Node name node_name: &'a str, + /// Token parsing error #[source] error: ParseTokenError, }, /// Unexpected FDT token when parsing properties #[error("Unexpected FDT token when parsing properties for node {node_name}: {token}")] - PropertyToken { node_name: &'a str, token: u32 }, + PropertyToken { + /// Node name + node_name: &'a str, + /// Token value + token: u32, + }, /// Property name string is not a valid string #[error("Property name string is not a valid string for node {node_name}: {error}")] PropertyNameStr { + /// Node name node_name: &'a str, + /// String parsing error #[source] error: StringError, }, From 51b216ee47531ba0822f366edfe4f9c258cc0ced Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 Aug 2025 02:35:19 +0000 Subject: [PATCH 8/9] Add missing documentation for host_fdt_parser error enum struct fields Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- openhcl/host_fdt_parser/src/lib.rs | 56 +++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/openhcl/host_fdt_parser/src/lib.rs b/openhcl/host_fdt_parser/src/lib.rs index 1467462c4a..43f5f44007 100644 --- a/openhcl/host_fdt_parser/src/lib.rs +++ b/openhcl/host_fdt_parser/src/lib.rs @@ -64,13 +64,17 @@ pub enum Error<'a> { /// Invalid device tree node #[error("invalid device tree node with parent {parent_name}: {error}")] Node { + /// The name of the parent node parent_name: &'a str, + /// The underlying FDT parser error error: fdt::parser::Error<'a>, }, /// Required property missing #[error("{node_name} did not have the following required property {prop_name}")] PropMissing { + /// The name of the node missing the property node_name: &'a str, + /// The name of the missing property prop_name: &'static str, }, /// Reading node property failed @@ -82,14 +86,19 @@ pub enum Error<'a> { /// Memory region not aligned #[error("memory node {node_name} contains 4K unaligned base {base} or len {len}")] MemoryRegUnaligned { + /// The name of the memory node node_name: &'a str, + /// The base address of the memory region base: u64, + /// The length of the memory region len: u64, }, /// Memory regions overlap #[error("ram at {}..{} of type {:?} overlaps ram at {}..{} of type {:?}", lower.range.start(), lower.range.end(), lower.mem_type, upper.range.start(), upper.range.end(), upper.mem_type)] MemoryRegOverlap { + /// The lower memory entry lower: MemoryEntry, + /// The upper memory entry that overlaps upper: MemoryEntry, }, /// Too many memory entries @@ -100,9 +109,13 @@ pub enum Error<'a> { "{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}" )] PropInvalidU32 { + /// The name of the node with the invalid property node_name: &'a str, + /// The name of the property with invalid value prop_name: &'a str, + /// The expected u32 value expected: u32, + /// The actual u32 value found actual: u32, }, /// Invalid string property value @@ -110,41 +123,71 @@ pub enum Error<'a> { "{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}" )] PropInvalidStr { + /// The name of the node with the invalid property node_name: &'a str, + /// The name of the property with invalid value prop_name: &'a str, + /// The expected string value expected: &'a str, + /// The actual string value found actual: &'a str, }, /// Unexpected VMBUS VTL #[error("{node_name} has an unexpected vtl {vtl}")] - UnexpectedVmbusVtl { node_name: &'a str, vtl: u32 }, + UnexpectedVmbusVtl { + /// The name of the VMBUS node + node_name: &'a str, + /// The unexpected VTL value + vtl: u32, + }, /// Multiple VMBUS nodes #[error("{node_name} specifies a duplicate vmbus node")] - MultipleVmbusNode { node_name: &'a str }, + MultipleVmbusNode { + /// The name of the duplicate VMBUS node + node_name: &'a str, + }, /// VMBUS ranges child/parent mismatch #[error("vmbus {node_name} ranges child base {child_base} does not match parent {parent_base}")] VmbusRangesChildParent { + /// The name of the VMBUS node node_name: &'a str, + /// The child base address child_base: u64, + /// The parent base address that doesn't match parent_base: u64, }, /// VMBUS ranges not aligned #[error("vmbus {node_name} base {base} or len {len} not aligned to 4K")] VmbusRangesNotAligned { + /// The name of the VMBUS node node_name: &'a str, + /// The base address that's not aligned base: u64, + /// The length that's not aligned len: u64, }, /// Too many VMBUS MMIO ranges #[error("vmbus {node_name} has more than 2 mmio ranges {ranges}")] - TooManyVmbusMmioRanges { node_name: &'a str, ranges: usize }, + TooManyVmbusMmioRanges { + /// The name of the VMBUS node + node_name: &'a str, + /// The number of MMIO ranges found + ranges: usize, + }, /// VMBUS MMIO overlaps RAM #[error("vmbus mmio at {}..{} overlaps ram at {}..{}", mmio.start(), mmio.end(), ram.range.start(), ram.range.end())] - VmbusMmioOverlapsRam { mmio: MemoryRange, ram: MemoryEntry }, + VmbusMmioOverlapsRam { + /// The VMBUS MMIO memory range + mmio: MemoryRange, + /// The RAM memory entry that overlaps + ram: MemoryEntry, + }, /// VMBUS MMIO overlaps VMBUS MMIO #[error("vmbus mmio at {}..{} overlaps vmbus mmio at {}..{}", mmio_a.start(), mmio_a.end(), mmio_b.start(), mmio_b.end())] VmbusMmioOverlapsVmbusMmio { + /// The first VMBUS MMIO memory range mmio_a: MemoryRange, + /// The second VMBUS MMIO memory range that overlaps mmio_b: MemoryRange, }, /// Command line size error @@ -152,7 +195,10 @@ pub enum Error<'a> { CmdlineSize, /// Unexpected memory allocation mode #[error("unexpected memory allocation mode: {mode}")] - UnexpectedMemoryAllocationMode { mode: &'a str }, + UnexpectedMemoryAllocationMode { + /// The unexpected memory allocation mode + mode: &'a str, + }, } const COM3_REG_BASE: u64 = 0x3E8; From 3422a04b716a2819ff64ca6cefafde89554698ca Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 14 Aug 2025 00:29:34 +0000 Subject: [PATCH 9/9] Revert flattened error hierarchies back to original Error struct + ErrorKind enum structure Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- Cargo.lock | 2 - openhcl/host_fdt_parser/Cargo.toml | 1 - openhcl/host_fdt_parser/src/lib.rs | 508 ++++++++++++++--------------- support/fdt/src/parser.rs | 272 +++++++++------ vm/hv1/hvdef/Cargo.toml | 1 - vm/hv1/hvdef/src/lib.rs | 5 +- 6 files changed, 418 insertions(+), 371 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab6d95d776..cffde82832 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2879,7 +2879,6 @@ dependencies = [ "igvm_defs", "inspect", "memory_range", - "thiserror 2.0.12", "tracing", ] @@ -2975,7 +2974,6 @@ dependencies = [ "bitfield-struct 0.10.1", "open_enum", "static_assertions", - "thiserror 2.0.12", "zerocopy 0.8.24", ] diff --git a/openhcl/host_fdt_parser/Cargo.toml b/openhcl/host_fdt_parser/Cargo.toml index 4411b04031..e7a9b8969b 100644 --- a/openhcl/host_fdt_parser/Cargo.toml +++ b/openhcl/host_fdt_parser/Cargo.toml @@ -19,7 +19,6 @@ inspect = { workspace = true, optional = true } arrayvec.workspace = true igvm_defs.workspace = true -thiserror.workspace = true tracing = { workspace = true, optional = true } [lints] diff --git a/openhcl/host_fdt_parser/src/lib.rs b/openhcl/host_fdt_parser/src/lib.rs index 43f5f44007..6e6a2a3d0e 100644 --- a/openhcl/host_fdt_parser/src/lib.rs +++ b/openhcl/host_fdt_parser/src/lib.rs @@ -13,6 +13,7 @@ use arrayvec::ArrayString; use arrayvec::ArrayVec; +use core::fmt::Display; use core::fmt::Write; use core::mem::size_of; use hvdef::HV_PAGE_SIZE; @@ -20,7 +21,6 @@ use igvm_defs::MemoryMapEntryType; #[cfg(feature = "inspect")] use inspect::Inspect; use memory_range::MemoryRange; -use thiserror::Error; /// Information about VMBUS. #[derive(Clone, Debug, PartialEq, Eq)] @@ -56,151 +56,136 @@ pub struct GicInfo { } /// Errors returned by parsing. -#[derive(Debug, Error)] -pub enum Error<'a> { - /// Invalid device tree - #[error("invalid device tree: {0}")] +#[derive(Debug)] +pub struct Error<'a>(ErrorKind<'a>); + +impl Display for Error<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_fmt(format_args!("Parsing failed due to: {}", self.0)) + } +} + +impl core::error::Error for Error<'_> {} + +#[derive(Debug)] +enum ErrorKind<'a> { Dt(fdt::parser::Error<'a>), - /// Invalid device tree node - #[error("invalid device tree node with parent {parent_name}: {error}")] Node { - /// The name of the parent node parent_name: &'a str, - /// The underlying FDT parser error error: fdt::parser::Error<'a>, }, - /// Required property missing - #[error("{node_name} did not have the following required property {prop_name}")] PropMissing { - /// The name of the node missing the property node_name: &'a str, - /// The name of the missing property prop_name: &'static str, }, - /// Reading node property failed - #[error("reading node property failed: {0}")] Prop(fdt::parser::Error<'a>), - /// Too many CPUs - #[error("device tree contained more enabled CPUs than can be parsed")] TooManyCpus, - /// Memory region not aligned - #[error("memory node {node_name} contains 4K unaligned base {base} or len {len}")] MemoryRegUnaligned { - /// The name of the memory node node_name: &'a str, - /// The base address of the memory region base: u64, - /// The length of the memory region len: u64, }, - /// Memory regions overlap - #[error("ram at {}..{} of type {:?} overlaps ram at {}..{} of type {:?}", lower.range.start(), lower.range.end(), lower.mem_type, upper.range.start(), upper.range.end(), upper.mem_type)] MemoryRegOverlap { - /// The lower memory entry lower: MemoryEntry, - /// The upper memory entry that overlaps upper: MemoryEntry, }, - /// Too many memory entries - #[error("device tree contained more memory ranges than can be parsed")] TooManyMemoryEntries, - /// Invalid u32 property value - #[error( - "{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}" - )] PropInvalidU32 { - /// The name of the node with the invalid property node_name: &'a str, - /// The name of the property with invalid value prop_name: &'a str, - /// The expected u32 value expected: u32, - /// The actual u32 value found actual: u32, }, - /// Invalid string property value - #[error( - "{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}" - )] PropInvalidStr { - /// The name of the node with the invalid property node_name: &'a str, - /// The name of the property with invalid value prop_name: &'a str, - /// The expected string value expected: &'a str, - /// The actual string value found actual: &'a str, }, - /// Unexpected VMBUS VTL - #[error("{node_name} has an unexpected vtl {vtl}")] UnexpectedVmbusVtl { - /// The name of the VMBUS node node_name: &'a str, - /// The unexpected VTL value vtl: u32, }, - /// Multiple VMBUS nodes - #[error("{node_name} specifies a duplicate vmbus node")] MultipleVmbusNode { - /// The name of the duplicate VMBUS node node_name: &'a str, }, - /// VMBUS ranges child/parent mismatch - #[error("vmbus {node_name} ranges child base {child_base} does not match parent {parent_base}")] VmbusRangesChildParent { - /// The name of the VMBUS node node_name: &'a str, - /// The child base address child_base: u64, - /// The parent base address that doesn't match parent_base: u64, }, - /// VMBUS ranges not aligned - #[error("vmbus {node_name} base {base} or len {len} not aligned to 4K")] VmbusRangesNotAligned { - /// The name of the VMBUS node node_name: &'a str, - /// The base address that's not aligned base: u64, - /// The length that's not aligned len: u64, }, - /// Too many VMBUS MMIO ranges - #[error("vmbus {node_name} has more than 2 mmio ranges {ranges}")] TooManyVmbusMmioRanges { - /// The name of the VMBUS node node_name: &'a str, - /// The number of MMIO ranges found ranges: usize, }, - /// VMBUS MMIO overlaps RAM - #[error("vmbus mmio at {}..{} overlaps ram at {}..{}", mmio.start(), mmio.end(), ram.range.start(), ram.range.end())] VmbusMmioOverlapsRam { - /// The VMBUS MMIO memory range mmio: MemoryRange, - /// The RAM memory entry that overlaps ram: MemoryEntry, }, - /// VMBUS MMIO overlaps VMBUS MMIO - #[error("vmbus mmio at {}..{} overlaps vmbus mmio at {}..{}", mmio_a.start(), mmio_a.end(), mmio_b.start(), mmio_b.end())] VmbusMmioOverlapsVmbusMmio { - /// The first VMBUS MMIO memory range mmio_a: MemoryRange, - /// The second VMBUS MMIO memory range that overlaps mmio_b: MemoryRange, }, - /// Command line size error - #[error("commandline too small to parse /chosen bootargs")] CmdlineSize, - /// Unexpected memory allocation mode - #[error("unexpected memory allocation mode: {mode}")] UnexpectedMemoryAllocationMode { - /// The unexpected memory allocation mode mode: &'a str, }, } +impl Display for ErrorKind<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + ErrorKind::Dt(e) => f.write_fmt(format_args!("invalid device tree: {}", e)), + ErrorKind::Node { parent_name, error } => { + f.write_fmt(format_args!("invalid device tree node with parent {parent_name}: {error}")) + } + ErrorKind::PropMissing { + node_name, + prop_name, + } => f.write_fmt(format_args!( + "{node_name} did not have the following required property {prop_name}", + )), + ErrorKind::Prop(e) => f.write_fmt(format_args!("reading node property failed: {e}")), + ErrorKind::TooManyCpus => { + f.write_str("device tree contained more enabled CPUs than can be parsed") + } + ErrorKind::MemoryRegUnaligned { + node_name, + base, + len, + } => f.write_fmt(format_args!( + "memory node {node_name} contains 4K unaligned base {base} or len {len}" + )), + ErrorKind::MemoryRegOverlap { lower, upper, } => { + f.write_fmt(format_args!("ram at {}..{} of type {:?} overlaps ram at {}..{} of type {:?}", lower.range.start(), lower.range.end(), lower.mem_type, upper.range.start(), upper.range.end(), upper.mem_type)) + } + ErrorKind::TooManyMemoryEntries => { + f.write_str("device tree contained more memory ranges than can be parsed") + } + ErrorKind::PropInvalidU32 { node_name, prop_name, expected, actual } => f.write_fmt(format_args!("{node_name} had an invalid u32 value for {prop_name}: expected {expected}, actual {actual}")), + ErrorKind::PropInvalidStr { node_name, prop_name, expected, actual } => f.write_fmt(format_args!("{node_name} had an invalid str value for {prop_name}: expected {expected}, actual {actual}")), + ErrorKind::UnexpectedVmbusVtl { node_name, vtl } => f.write_fmt(format_args!("{node_name} has an unexpected vtl {vtl}")), + ErrorKind::MultipleVmbusNode { node_name } => f.write_fmt(format_args!("{node_name} specifies a duplicate vmbus node")), + ErrorKind::VmbusRangesChildParent { node_name, child_base, parent_base } => f.write_fmt(format_args!("vmbus {node_name} ranges child base {child_base} does not match parent {parent_base}")), + ErrorKind::VmbusRangesNotAligned { node_name, base, len } => f.write_fmt(format_args!("vmbus {node_name} base {base} or len {len} not aligned to 4K")), + ErrorKind::TooManyVmbusMmioRanges { node_name, ranges } => f.write_fmt(format_args!("vmbus {node_name} has more than 2 mmio ranges {ranges}")), + ErrorKind::VmbusMmioOverlapsRam { mmio, ram } => { + f.write_fmt(format_args!("vmbus mmio at {}..{} overlaps ram at {}..{}", mmio.start(), mmio.end(), ram.range.start(), ram.range.end())) + } + ErrorKind::VmbusMmioOverlapsVmbusMmio { mmio_a, mmio_b } => { + f.write_fmt(format_args!("vmbus mmio at {}..{} overlaps vmbus mmio at {}..{}", mmio_a.start(), mmio_a.end(), mmio_b.start(), mmio_b.end())) + } + ErrorKind::CmdlineSize => f.write_str("commandline too small to parse /chosen bootargs"), + ErrorKind::UnexpectedMemoryAllocationMode { mode } => f.write_fmt(format_args!("unexpected memory allocation mode: {}", mode)), + } + } +} + const COM3_REG_BASE: u64 = 0x3E8; /// Struct containing parsed device tree information. @@ -346,15 +331,15 @@ impl< /// Parse the given device tree. pub fn parse(dt: &'a [u8], storage: &'b mut Self) -> Result<&'b Self, Error<'a>> { - Self::parse_inner(dt, storage) + Self::parse_inner(dt, storage).map_err(Error) } - fn parse_inner(dt: &'a [u8], storage: &'b mut Self) -> Result<&'b Self, Error<'a>> { - let parser = fdt::parser::Parser::new(dt).map_err(Error::Dt)?; + fn parse_inner(dt: &'a [u8], storage: &'b mut Self) -> Result<&'b Self, ErrorKind<'a>> { + let parser = fdt::parser::Parser::new(dt).map_err(ErrorKind::Dt)?; let root = match parser.root() { Ok(v) => v, Err(e) => { - return Err(Error::Node { + return Err(ErrorKind::Node { parent_name: "", error: e, }); @@ -367,10 +352,10 @@ impl< // after all entries are parsed once sort is stabilized in core. let insert_memory_entry = |memory: &mut ArrayVec, entry: MemoryEntry| - -> Result<(), Error<'a>> { + -> Result<(), ErrorKind<'a>> { let insert_index = match memory.binary_search_by_key(&entry.range, |k| k.range) { Ok(index) => { - return Err(Error::MemoryRegOverlap { + return Err(ErrorKind::MemoryRegOverlap { lower: memory[index], upper: entry, }); @@ -380,11 +365,11 @@ impl< memory .try_insert(insert_index, entry) - .map_err(|_| Error::TooManyMemoryEntries) + .map_err(|_| ErrorKind::TooManyMemoryEntries) }; for child in root.children() { - let child = child.map_err(|error| Error::Node { + let child = child.map_err(|error| ErrorKind::Node { parent_name: root.name, error, })?; @@ -393,18 +378,18 @@ impl< "cpus" => { let address_cells = child .find_property("#address-cells") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: "#address-cells", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; // On ARM v8 64-bit systems, up to 2 address-cells values // can be provided. if address_cells > 2 { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: child.name, prop_name: "#address-cells", expected: 2, @@ -413,20 +398,20 @@ impl< } for cpu in child.children() { - let cpu = cpu.map_err(|error| Error::Node { + let cpu = cpu.map_err(|error| ErrorKind::Node { parent_name: child.name, error, })?; if cpu .find_property("status") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: cpu.name, prop_name: "status", })? .read_str() - .map_err(Error::Prop)? + .map_err(ErrorKind::Prop)? != "okay" { continue; @@ -435,62 +420,63 @@ impl< // NOTE: For x86, Underhill will need to query the hypervisor for // the vp_index to apic_id mapping. There's no // correlation in the device tree about this at all. - let reg_property = cpu.find_property("reg").map_err(Error::Prop)?.ok_or( - Error::PropMissing { + let reg_property = cpu + .find_property("reg") + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: cpu.name, prop_name: "reg", - }, - )?; + })?; let reg = if address_cells == 1 { - reg_property.read_u32(0).map_err(Error::Prop)? as u64 + reg_property.read_u32(0).map_err(ErrorKind::Prop)? as u64 } else { - reg_property.read_u64(0).map_err(Error::Prop)? + reg_property.read_u64(0).map_err(ErrorKind::Prop)? }; let vnode = cpu .find_property("numa-node-id") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: cpu.name, prop_name: "numa-node-id", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; storage .cpus .try_push(CpuEntry { reg, vnode }) - .map_err(|_| Error::TooManyCpus)?; + .map_err(|_| ErrorKind::TooManyCpus)?; } } "openhcl" => { let memory_allocation_mode = child .find_property("memory-allocation-mode") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: "memory-allocation-mode", })?; - match memory_allocation_mode.read_str().map_err(Error::Prop)? { + match memory_allocation_mode.read_str().map_err(ErrorKind::Prop)? { "host" => { storage.memory_allocation_mode = MemoryAllocationMode::Host; } "vtl2" => { let memory_size = child .find_property("memory-size") - .map_err(Error::Prop)? + .map_err(ErrorKind::Prop)? .map(|p| p.read_u64(0)) .transpose() - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; let mmio_size = child .find_property("mmio-size") - .map_err(Error::Prop)? + .map_err(ErrorKind::Prop)? .map(|p| p.read_u64(0)) .transpose() - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; storage.memory_allocation_mode = MemoryAllocationMode::Vtl2 { memory_size, @@ -498,19 +484,19 @@ impl< }; } mode => { - return Err(Error::UnexpectedMemoryAllocationMode { mode }); + return Err(ErrorKind::UnexpectedMemoryAllocationMode { mode }); } } storage.vtl0_alias_map = child .find_property("vtl0-alias-map") - .map_err(Error::Prop)? + .map_err(ErrorKind::Prop)? .map(|p| p.read_u64(0)) .transpose() - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; for openhcl_child in child.children() { - let openhcl_child = openhcl_child.map_err(|error| Error::Node { + let openhcl_child = openhcl_child.map_err(|error| ErrorKind::Node { parent_name: root.name, error, })?; @@ -519,8 +505,8 @@ impl< "entropy" => { let host_entropy = openhcl_child .find_property("reg") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: openhcl_child.name, prop_name: "reg", })? @@ -568,16 +554,16 @@ impl< _ if child.name.starts_with("memory@") => { let igvm_type = if let Some(igvm_type) = child .find_property(igvm_defs::dt::IGVM_DT_IGVM_TYPE_PROPERTY) - .map_err(Error::Prop)? + .map_err(ErrorKind::Prop)? { - let typ = igvm_type.read_u32(0).map_err(Error::Prop)?; + let typ = igvm_type.read_u32(0).map_err(ErrorKind::Prop)?; MemoryMapEntryType(typ as u16) } else { MemoryMapEntryType::MEMORY }; - let reg = child.find_property("reg").map_err(Error::Prop)?.ok_or( - Error::PropMissing { + let reg = child.find_property("reg").map_err(ErrorKind::Prop)?.ok_or( + ErrorKind::PropMissing { node_name: child.name, prop_name: "reg", }, @@ -585,24 +571,24 @@ impl< let vnode = child .find_property("numa-node-id") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: "numa-node-id", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; let len = reg.data.len(); let reg_tuple_size = size_of::() * 2; let number_of_ranges = len / reg_tuple_size; for i in 0..number_of_ranges { - let base = reg.read_u64(i * 2).map_err(Error::Prop)?; - let len = reg.read_u64(i * 2 + 1).map_err(Error::Prop)?; + let base = reg.read_u64(i * 2).map_err(ErrorKind::Prop)?; + let len = reg.read_u64(i * 2 + 1).map_err(ErrorKind::Prop)?; if base % HV_PAGE_SIZE != 0 || len % HV_PAGE_SIZE != 0 { - return Err(Error::MemoryRegUnaligned { + return Err(ErrorKind::MemoryRegUnaligned { node_name: child.name, base, len, @@ -623,12 +609,13 @@ impl< "chosen" => { let cmdline = child .find_property("bootargs") - .map_err(Error::Prop)? - .map(|prop| prop.read_str().map_err(Error::Prop)) + .map_err(ErrorKind::Prop)? + .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) .transpose()? .unwrap_or(""); - write!(storage.command_line, "{}", cmdline).map_err(|_| Error::CmdlineSize)?; + write!(storage.command_line, "{}", cmdline) + .map_err(|_| ErrorKind::CmdlineSize)?; } _ if child.name.starts_with("intc@") => { validate_property_str(&child, "compatible", "arm,gic-v3")?; @@ -639,26 +626,29 @@ impl< let gic_redistributor_stride = child .find_property("redistributor-stride") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: "redistributor-stride", })? .read_u64(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; - let gic_reg_property = child.find_property("reg").map_err(Error::Prop)?.ok_or( - Error::PropMissing { + let gic_reg_property = child + .find_property("reg") + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: "reg", - }, - )?; - let gic_distributor_base = gic_reg_property.read_u64(0).map_err(Error::Prop)?; - let gic_distributor_size = gic_reg_property.read_u64(1).map_err(Error::Prop)?; + })?; + let gic_distributor_base = + gic_reg_property.read_u64(0).map_err(ErrorKind::Prop)?; + let gic_distributor_size = + gic_reg_property.read_u64(1).map_err(ErrorKind::Prop)?; let gic_redistributors_base = - gic_reg_property.read_u64(2).map_err(Error::Prop)?; + gic_reg_property.read_u64(2).map_err(ErrorKind::Prop)?; let gic_redistributors_size = - gic_reg_property.read_u64(3).map_err(Error::Prop)?; + gic_reg_property.read_u64(3).map_err(ErrorKind::Prop)?; storage.gic = Some(GicInfo { gic_distributor_base, @@ -683,7 +673,7 @@ impl< // Validate memory entries do not overlap. for (prev, next) in storage.memory.iter().zip(storage.memory.iter().skip(1)) { if prev.range.overlaps(&next.range) { - return Err(Error::MemoryRegOverlap { + return Err(ErrorKind::MemoryRegOverlap { lower: *prev, upper: *next, }); @@ -706,7 +696,7 @@ impl< for ram in storage.memory.iter() { for mmio in vmbus_vtl0_mmio { if mmio.overlaps(&ram.range) { - return Err(Error::VmbusMmioOverlapsRam { + return Err(ErrorKind::VmbusMmioOverlapsRam { mmio: *mmio, ram: *ram, }); @@ -715,7 +705,7 @@ impl< for mmio in vmbus_vtl2_mmio { if mmio.overlaps(&ram.range) { - return Err(Error::VmbusMmioOverlapsRam { + return Err(ErrorKind::VmbusMmioOverlapsRam { mmio: *mmio, ram: *ram, }); @@ -726,7 +716,7 @@ impl< for vtl0_mmio in vmbus_vtl0_mmio { for vtl2_mmio in vmbus_vtl2_mmio { if vtl0_mmio.overlaps(vtl2_mmio) { - return Err(Error::VmbusMmioOverlapsVmbusMmio { + return Err(ErrorKind::VmbusMmioOverlapsVmbusMmio { mmio_a: *vtl0_mmio, mmio_b: *vtl2_mmio, }); @@ -766,11 +756,11 @@ fn parse_compatible<'a>( vmbus_vtl2: &mut Option, pmu_gsiv: &mut Option, com3_serial: &mut bool, -) -> Result<(), Error<'a>> { +) -> Result<(), ErrorKind<'a>> { let compatible = node .find_property("compatible") - .map_err(Error::Prop)? - .map(|prop| prop.read_str().map_err(Error::Prop)) + .map_err(ErrorKind::Prop)? + .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) .transpose()? .unwrap_or(""); @@ -790,20 +780,20 @@ fn parse_compatible<'a>( Ok(()) } -fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> { +fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> { // Validate address cells and size cells are 2 let address_cells = node .find_property("#address-cells") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: node.name, prop_name: "#address-cells", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; if address_cells != 2 { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: node.name, prop_name: "#address-cells", expected: 2, @@ -813,16 +803,16 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> let size_cells = node .find_property("#size-cells") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: node.name, prop_name: "#size-cells", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; if size_cells != 2 { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: node.name, prop_name: "#size-cells", expected: 2, @@ -830,77 +820,78 @@ fn parse_vmbus<'a>(node: &fdt::parser::Node<'a>) -> Result> }); } - let mmio: ArrayVec = match node.find_property("ranges").map_err(Error::Prop)? { - Some(ranges) => { - // Determine how many mmio ranges this describes. Valid numbers are - // 0, 1 or 2. - let ranges_tuple_size = size_of::() * 3; - let number_of_ranges = ranges.data.len() / ranges_tuple_size; - let mut mmio = ArrayVec::new(); - - if number_of_ranges > 2 { - return Err(Error::TooManyVmbusMmioRanges { - node_name: node.name, - ranges: number_of_ranges, - }); - } - - for i in 0..number_of_ranges { - let child_base = ranges.read_u64(i * 3).map_err(Error::Prop)?; - let parent_base = ranges.read_u64(i * 3 + 1).map_err(Error::Prop)?; - let len = ranges.read_u64(i * 3 + 2).map_err(Error::Prop)?; - - if child_base != parent_base { - return Err(Error::VmbusRangesChildParent { + let mmio: ArrayVec = + match node.find_property("ranges").map_err(ErrorKind::Prop)? { + Some(ranges) => { + // Determine how many mmio ranges this describes. Valid numbers are + // 0, 1 or 2. + let ranges_tuple_size = size_of::() * 3; + let number_of_ranges = ranges.data.len() / ranges_tuple_size; + let mut mmio = ArrayVec::new(); + + if number_of_ranges > 2 { + return Err(ErrorKind::TooManyVmbusMmioRanges { node_name: node.name, - child_base, - parent_base, + ranges: number_of_ranges, }); } - if child_base % HV_PAGE_SIZE != 0 || len % HV_PAGE_SIZE != 0 { - return Err(Error::VmbusRangesNotAligned { - node_name: node.name, - base: child_base, - len, - }); + for i in 0..number_of_ranges { + let child_base = ranges.read_u64(i * 3).map_err(ErrorKind::Prop)?; + let parent_base = ranges.read_u64(i * 3 + 1).map_err(ErrorKind::Prop)?; + let len = ranges.read_u64(i * 3 + 2).map_err(ErrorKind::Prop)?; + + if child_base != parent_base { + return Err(ErrorKind::VmbusRangesChildParent { + node_name: node.name, + child_base, + parent_base, + }); + } + + if child_base % HV_PAGE_SIZE != 0 || len % HV_PAGE_SIZE != 0 { + return Err(ErrorKind::VmbusRangesNotAligned { + node_name: node.name, + base: child_base, + len, + }); + } + + mmio.push( + MemoryRange::try_new(child_base..(child_base + len)).expect("valid range"), + ); } - mmio.push( - MemoryRange::try_new(child_base..(child_base + len)).expect("valid range"), - ); - } + // The DT ranges field might not have been sorted. Swap them if the + // low gap was described 2nd. + if number_of_ranges > 1 && mmio[0].start() > mmio[1].start() { + mmio.swap(0, 1); + } - // The DT ranges field might not have been sorted. Swap them if the - // low gap was described 2nd. - if number_of_ranges > 1 && mmio[0].start() > mmio[1].start() { - mmio.swap(0, 1); - } + if number_of_ranges > 1 && mmio[0].overlaps(&mmio[1]) { + return Err(ErrorKind::VmbusMmioOverlapsVmbusMmio { + mmio_a: mmio[0], + mmio_b: mmio[1], + }); + } - if number_of_ranges > 1 && mmio[0].overlaps(&mmio[1]) { - return Err(Error::VmbusMmioOverlapsVmbusMmio { - mmio_a: mmio[0], - mmio_b: mmio[1], - }); + mmio } - - mmio - } - None => { - // No mmio is acceptable. - ArrayVec::new() - } - }; + None => { + // No mmio is acceptable. + ArrayVec::new() + } + }; let connection_id = node .find_property("microsoft,message-connection-id") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: node.name, prop_name: "microsoft,message-connection-id", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; Ok(VmbusInfo { mmio, @@ -912,12 +903,12 @@ fn parse_simple_bus<'a>( node: &fdt::parser::Node<'a>, vmbus_vtl0: &mut Option, vmbus_vtl2: &mut Option, -) -> Result<(), Error<'a>> { +) -> Result<(), ErrorKind<'a>> { // Vmbus must be under simple-bus node with empty ranges. if !node .find_property("ranges") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: node.name, prop_name: "ranges", })? @@ -928,15 +919,15 @@ fn parse_simple_bus<'a>( } for child in node.children() { - let child = child.map_err(|error| Error::Node { + let child = child.map_err(|error| ErrorKind::Node { parent_name: node.name, error, })?; let compatible = child .find_property("compatible") - .map_err(Error::Prop)? - .map(|prop| prop.read_str().map_err(Error::Prop)) + .map_err(ErrorKind::Prop)? + .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) .transpose()? .unwrap_or(""); @@ -944,31 +935,31 @@ fn parse_simple_bus<'a>( let vtl_name = igvm_defs::dt::IGVM_DT_VTL_PROPERTY; let vtl = child .find_property(vtl_name) - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: vtl_name, })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; match vtl { 0 => { if vmbus_vtl0.replace(parse_vmbus(&child)?).is_some() { - return Err(Error::MultipleVmbusNode { + return Err(ErrorKind::MultipleVmbusNode { node_name: child.name, }); } } 2 => { if vmbus_vtl2.replace(parse_vmbus(&child)?).is_some() { - return Err(Error::MultipleVmbusNode { + return Err(ErrorKind::MultipleVmbusNode { node_name: child.name, }); } } _ => { - return Err(Error::UnexpectedVmbusVtl { + return Err(ErrorKind::UnexpectedVmbusVtl { node_name: child.name, vtl, }); @@ -980,40 +971,43 @@ fn parse_simple_bus<'a>( Ok(()) } -fn parse_io_bus<'a>(node: &fdt::parser::Node<'a>, com3_serial: &mut bool) -> Result<(), Error<'a>> { +fn parse_io_bus<'a>( + node: &fdt::parser::Node<'a>, + com3_serial: &mut bool, +) -> Result<(), ErrorKind<'a>> { for io_bus_child in node.children() { - let io_bus_child = io_bus_child.map_err(|error| Error::Node { + let io_bus_child = io_bus_child.map_err(|error| ErrorKind::Node { parent_name: node.name, error, })?; let compatible: &str = io_bus_child .find_property("compatible") - .map_err(Error::Prop)? - .map(|prop| prop.read_str().map_err(Error::Prop)) + .map_err(ErrorKind::Prop)? + .map(|prop| prop.read_str().map_err(ErrorKind::Prop)) .transpose()? .unwrap_or(""); let _current_speed = io_bus_child .find_property("current-speed") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: io_bus_child.name, prop_name: "current-speed", })? .read_u32(0) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; let reg = io_bus_child .find_property("reg") - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: io_bus_child.name, prop_name: "reg", })?; - let reg_base = reg.read_u64(0).map_err(Error::Prop)?; - let _reg_len = reg.read_u64(1).map_err(Error::Prop)?; + let reg_base = reg.read_u64(0).map_err(ErrorKind::Prop)?; + let _reg_len = reg.read_u64(1).map_err(ErrorKind::Prop)?; // Linux kernel hard-codes COM3 to COM3_REG_BASE. // If work is ever done in the Linux kernel to instead @@ -1034,15 +1028,15 @@ fn parse_io_bus<'a>(node: &fdt::parser::Node<'a>, com3_serial: &mut bool) -> Res fn parse_pmu_gsiv<'a>( node: &fdt::parser::Node<'a>, pmu_gsiv: &mut Option, -) -> Result<(), Error<'a>> { - let interrupts = node.find_property("interrupts").map_err(Error::Prop)?; - let interrupts = interrupts.ok_or(Error::PropMissing { +) -> Result<(), ErrorKind<'a>> { + let interrupts = node.find_property("interrupts").map_err(ErrorKind::Prop)?; + let interrupts = interrupts.ok_or(ErrorKind::PropMissing { node_name: node.name, prop_name: "interrupts", })?; if interrupts.data.len() < 3 * size_of::() { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: node.name, prop_name: "interrupts size", expected: 3 * size_of::() as u32, @@ -1053,21 +1047,21 @@ fn parse_pmu_gsiv<'a>( // This parser expects the PMU GSIV to be a PPI, as all platforms that // support OpenHCL should be using PPIs. const GIC_PPI: u32 = 1; - let interrupt_type = interrupts.read_u32(0).map_err(Error::Prop)?; + let interrupt_type = interrupts.read_u32(0).map_err(ErrorKind::Prop)?; if interrupt_type != GIC_PPI { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: node.name, prop_name: "interrupts", expected: GIC_PPI, actual: interrupt_type, }); } - let interrupt_id = interrupts.read_u32(1).map_err(Error::Prop)?; + let interrupt_id = interrupts.read_u32(1).map_err(ErrorKind::Prop)?; // Interrupt id describes the index from the PPI start of 16. It must be // smaller than 16, as PPIs only exist from 16 to 31. if interrupt_id >= 16 { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: node.name, prop_name: "interrupts", expected: 16, @@ -1085,18 +1079,18 @@ fn validate_property_str<'a>( child: &fdt::parser::Node<'a>, name: &'static str, expected: &'static str, -) -> Result<(), Error<'a>> { +) -> Result<(), ErrorKind<'a>> { let actual = child .find_property(name) - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: name, })? .read_str() - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; if actual != expected { - return Err(Error::PropInvalidStr { + return Err(ErrorKind::PropInvalidStr { node_name: child.name, prop_name: name, expected, @@ -1112,18 +1106,18 @@ fn validate_property_u32<'a>( name: &'static str, expected: u32, index: usize, -) -> Result<(), Error<'a>> { +) -> Result<(), ErrorKind<'a>> { let actual = child .find_property(name) - .map_err(Error::Prop)? - .ok_or(Error::PropMissing { + .map_err(ErrorKind::Prop)? + .ok_or(ErrorKind::PropMissing { node_name: child.name, prop_name: name, })? .read_u32(index) - .map_err(Error::Prop)?; + .map_err(ErrorKind::Prop)?; if actual != expected { - return Err(Error::PropInvalidU32 { + return Err(ErrorKind::PropInvalidU32 { node_name: child.name, prop_name: name, expected, @@ -1718,7 +1712,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error::MemoryRegOverlap { .. }) + Err(Error(ErrorKind::MemoryRegOverlap { .. })) )); // mem contained within another @@ -1757,7 +1751,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error::MemoryRegOverlap { .. }) + Err(Error(ErrorKind::MemoryRegOverlap { .. })) )); // mem overlaps vmbus @@ -1801,7 +1795,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error::VmbusMmioOverlapsRam { .. }) + Err(Error(ErrorKind::VmbusMmioOverlapsRam { .. })) )); // vmbus overlap each other @@ -1845,7 +1839,7 @@ mod tests { let mut parsed = TestParsedDeviceTree::new(); assert!(matches!( TestParsedDeviceTree::parse(&dt, &mut parsed), - Err(Error::VmbusMmioOverlapsVmbusMmio { .. }) + Err(Error(ErrorKind::VmbusMmioOverlapsVmbusMmio { .. })) )); } diff --git a/support/fdt/src/parser.rs b/support/fdt/src/parser.rs index 7ed3247a1a..914fbe48da 100644 --- a/support/fdt/src/parser.rs +++ b/support/fdt/src/parser.rs @@ -6,123 +6,154 @@ use super::spec; use super::spec::U32b; use super::spec::U64b; +use core::fmt::Display; use core::mem::size_of; -use thiserror::Error; use zerocopy::FromBytes; use zerocopy::Immutable; use zerocopy::KnownLayout; /// Errors returned when parsing a FDT. -#[derive(Debug, Error)] -pub enum Error<'a> { +#[derive(Debug)] +pub struct Error<'a>(ErrorKind<'a>); + +impl Display for Error<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + self.0.fmt(f) + } +} + +// TODO: Once core::error::Error is stablized, we can remove this feature gate. +impl core::error::Error for Error<'_> {} + +/// Types of errors when parsing a FDT. +#[derive(Debug)] +enum ErrorKind<'a> { /// Buffer is not aligned to u32 - #[error("Buffer is not aligned to u32")] BufferAlignment, /// Buffer too small for fixed header - #[error("Buffer too small for fixed FDT header")] NoHeader, /// Fixed header magic invalid - #[error("FDT header magic field invalid")] HeaderMagic, /// Total size described in the fixed header is greater than buffer provided - #[error("FDT header total size greater than provided buffer")] HeaderTotalSize, /// Header version is invalid - #[error("FDT header version invalid")] HeaderVersion, /// Structure block not contained within buffer - #[error("Structure block not contained within buffer")] StructureBlock, /// Structure block not aligned to u32 - #[error("Structure block offset is not aligned to u32")] StructureBlockAlignment, /// Memory reservation block not contained within buffer - #[error("Memory reservation block not contained within buffer")] MemoryReservationBlock, /// Memory reservation block did not end with an empty entry - #[error("Memory reservation block did not end with an empty entry")] MemoryReservationBlockEnd, /// Strings block not contained within buffer - #[error("Strings block not contained within buffer")] StringsBlock, /// No root node present - #[error("No root node present")] RootNode, /// More than one node at the root - #[error("More than one node at the root")] MultipleRootNodes, /// Unable to parse FDT token when parsing nodes - #[error("Unable to parse FDT token when parsing nodes {0}")] - NodeToken(#[from] ParseTokenError), + NodeToken(ParseTokenError), /// Unexpected token when parsing begin node - #[error("Unexpected token when parsing begin node {0}")] NodeBegin(u32), /// Unexpected token when parsing node properties - #[error("Unexpected token when parsing node properties {0}")] NodeProp(u32), /// Unexpected token when parsing children nodes - #[error("Unexpected token when parsing children nodes {0}")] NodeChildren(u32), /// Property data buffer len is not a multiple of requested type size - #[error( - "Property {prop_name} data buffer len is not multiple of type size for node {node_name}" - )] PropertyDataTypeBuffer { - /// Node name node_name: &'a str, - /// Property name prop_name: &'a str, }, /// Property requested at offset is larger than data buffer - #[error( - "Property {prop_name} requested at offset is larger than data buffer for node {node_name}" - )] PropertyOffset { - /// Node name node_name: &'a str, - /// Property name prop_name: &'a str, }, /// Property data is not a a valid string - #[error("Property data is not a a valid string for node {node_name}: {error}")] PropertyStr { - /// Node name node_name: &'a str, - /// String parsing error - #[source] error: StringError, }, /// Unable to parse FDT token when parsing properties - #[error("Unable to parse FDT token when parsing properties for node {node_name}: {error}")] PropertyTokenParse { - /// Node name node_name: &'a str, - /// Token parsing error - #[source] error: ParseTokenError, }, /// Unexpected FDT token when parsing properties - #[error("Unexpected FDT token when parsing properties for node {node_name}: {token}")] - PropertyToken { - /// Node name - node_name: &'a str, - /// Token value - token: u32, - }, + PropertyToken { node_name: &'a str, token: u32 }, /// Property name string is not a valid string - #[error("Property name string is not a valid string for node {node_name}: {error}")] PropertyNameStr { - /// Node name node_name: &'a str, - /// String parsing error - #[source] error: StringError, }, /// FDT end token not present at end of structure block - #[error("FDT end token not present at end of structure block")] FdtEnd, } +impl Display for ErrorKind<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + ErrorKind::BufferAlignment => f.write_str("Buffer is not aligned to u32"), + ErrorKind::NoHeader => f.write_str("Buffer too small for fixed FDT header"), + ErrorKind::HeaderMagic => f.write_str("FDT header magic field invalid"), + ErrorKind::HeaderTotalSize => { + f.write_str("FDT header total size greater than provided buffer") + } + ErrorKind::HeaderVersion => f.write_str("FDT header version invalid"), + ErrorKind::StructureBlock => f.write_str("Structure block not contained within buffer"), + ErrorKind::StructureBlockAlignment => { + f.write_str("Structure block offset is not aligned to u32") + } + ErrorKind::MemoryReservationBlock => { + f.write_str("Memory reservation block not contained within buffer") + } + ErrorKind::MemoryReservationBlockEnd => { + f.write_str("Memory reservation block did not end with an empty entry") + } + ErrorKind::StringsBlock => f.write_str("Strings block not contained within buffer"), + ErrorKind::RootNode => f.write_str("No root node present"), + ErrorKind::MultipleRootNodes => f.write_str("More than one node at the root"), + ErrorKind::NodeToken(e) => f.write_fmt(format_args!( + "Unable to parse FDT token when parsing nodes {}", + e + )), + ErrorKind::NodeBegin(token) => f.write_fmt(format_args!( + "Unexpected token when parsing begin node {}", + token + )), + ErrorKind::NodeProp(token) => f.write_fmt(format_args!( + "Unexpected token when parsing node properties {}", + token + )), + ErrorKind::NodeChildren(token) => f.write_fmt(format_args!( + "Unexpected token when parsing children nodes {}", + token + )), + ErrorKind::PropertyDataTypeBuffer { node_name, prop_name } => f.write_fmt(format_args!( + "Property {prop_name} data buffer len is not multiple of type size for node {node_name}" + )), + ErrorKind::PropertyOffset { node_name, prop_name } => f.write_fmt(format_args!( + "Property {prop_name} requested at offset is larger than data buffer for node {node_name}" + )), + ErrorKind::PropertyStr { node_name, error } => f.write_fmt(format_args!( + "Property data is not a a valid string for node {node_name}: {error}" + )), + ErrorKind::PropertyTokenParse { node_name, error } => f.write_fmt(format_args!( + "Unable to parse FDT token when parsing properties for node {node_name}: {error}", + )), + ErrorKind::PropertyToken { node_name, token } => f.write_fmt(format_args!( + "Unexpected FDT token when parsing properties for node {node_name}: {}", + token + )), + ErrorKind::PropertyNameStr { node_name, error } => f.write_fmt(format_args!( + "Property name string is not a valid string for node {node_name}: {error}", + )), + ErrorKind::FdtEnd => f.write_str("FDT end token not present at end of structure block"), + } + } +} + /// A parser used to parse a FDT. pub struct Parser<'a> { /// The total size used by the dt. @@ -142,11 +173,11 @@ impl<'a> Parser<'a> { /// attempting to determine the overall size of a device tree. pub fn read_total_size(buf: &[u8]) -> Result> { let header = spec::Header::read_from_prefix(buf) - .map_err(|_| Error::NoHeader)? + .map_err(|_| Error(ErrorKind::NoHeader))? .0; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u32::from(header.magic) != spec::MAGIC { - Err(Error::HeaderMagic) + Err(Error(ErrorKind::HeaderMagic)) } else { Ok(u32::from(header.totalsize) as usize) } @@ -155,27 +186,27 @@ impl<'a> Parser<'a> { /// Create a new instance of a FDT parser. pub fn new(buf: &'a [u8]) -> Result> { if buf.as_ptr() as usize % size_of::() != 0 { - return Err(Error::BufferAlignment); + return Err(Error(ErrorKind::BufferAlignment)); } let header = spec::Header::read_from_prefix(buf) - .map_err(|_| Error::NoHeader)? + .map_err(|_| Error(ErrorKind::NoHeader))? .0; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u32::from(header.magic) != spec::MAGIC { - return Err(Error::HeaderMagic); + return Err(Error(ErrorKind::HeaderMagic)); } // Validate total size within buf. let total_size = u32::from(header.totalsize) as usize; if total_size > buf.len() { - return Err(Error::HeaderTotalSize); + return Err(Error(ErrorKind::HeaderTotalSize)); } if u32::from(header.version) < spec::CURRENT_VERSION || u32::from(header.last_comp_version) > spec::COMPAT_VERSION { - return Err(Error::HeaderVersion); + return Err(Error(ErrorKind::HeaderVersion)); } // Validate the mem_rsvmap region ends with an empty entry. Currently @@ -184,10 +215,10 @@ impl<'a> Parser<'a> { let mut memory_reservations_len = 0; let mut mem_rsvmap = buf .get(mem_rsvmap_offset..) - .ok_or(Error::MemoryReservationBlock)?; + .ok_or(Error(ErrorKind::MemoryReservationBlock))?; loop { let (entry, rest) = spec::ReserveEntry::read_from_prefix(mem_rsvmap) - .map_err(|_| Error::MemoryReservationBlockEnd)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) + .map_err(|_| Error(ErrorKind::MemoryReservationBlockEnd))?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u64::from(entry.address) == 0 && u64::from(entry.size) == 0 { break; @@ -199,30 +230,30 @@ impl<'a> Parser<'a> { let memory_reservations = buf .get(mem_rsvmap_offset..(mem_rsvmap_offset + memory_reservations_len)) - .ok_or(Error::MemoryReservationBlock)?; + .ok_or(Error(ErrorKind::MemoryReservationBlock))?; let struct_offset = u32::from(header.off_dt_struct) as usize; let struct_len = u32::from(header.size_dt_struct) as usize; if struct_offset % size_of::() != 0 { - return Err(Error::StructureBlockAlignment); + return Err(Error(ErrorKind::StructureBlockAlignment)); } let structure_block = buf .get(struct_offset..(struct_offset + struct_len)) - .ok_or(Error::StructureBlock)?; + .ok_or(Error(ErrorKind::StructureBlock))?; // FDT_END must be the last token in the structure block. Ignore it once // checked. let structure_block = structure_block .strip_suffix(&spec::END.to_be_bytes()) - .ok_or(Error::FdtEnd)?; + .ok_or(Error(ErrorKind::FdtEnd))?; let strings_offset = u32::from(header.off_dt_strings) as usize; let strings_len = u32::from(header.size_dt_strings) as usize; let strings_block = buf .get(strings_offset..(strings_offset + strings_len)) - .ok_or(Error::StringsBlock)?; + .ok_or(Error(ErrorKind::StringsBlock))?; Ok(Self { total_size, @@ -240,10 +271,10 @@ impl<'a> Parser<'a> { nodes: self.structure_block, }; - let root = iter.next().ok_or(Error::RootNode)??; + let root = iter.next().ok_or(Error(ErrorKind::RootNode))??; if iter.next().is_some() { - Err(Error::MultipleRootNodes) + Err(Error(ErrorKind::MultipleRootNodes)) } else { Ok(root) } @@ -291,29 +322,43 @@ impl ParsedToken<'_> { } /// Errors returned when parsing FDT tokens. -/// Errors returned when parsing FDT tokens. -#[derive(Debug, Error)] -pub enum ParseTokenError { +#[derive(Debug)] +enum ParseTokenError { /// Unknown token - #[error("Unknown FDT token {0}")] Unknown(u32), /// Buf too small - #[error("Buffer too small to read token")] BufLen, /// Buf too small for prop header - #[error("Buffer too small to read property header")] PropHeader, /// Buf too small for prop data described in prop header - #[error("Buffer too small to read property data encoded in property header")] PropData, /// Begin node name is not valid - #[error("Node name is not valid {0}")] - BeginName(#[from] StringError), + BeginName(StringError), /// Buf too small for begin node name alignment - #[error("Buffer too small for begin node name alignment")] BeginNameAlignment, } +impl Display for ParseTokenError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + ParseTokenError::Unknown(token) => { + f.write_fmt(format_args!("Unknown FDT token {}", token)) + } + ParseTokenError::BufLen => f.write_str("Buffer too small to read token"), + ParseTokenError::PropHeader => f.write_str("Buffer too small to read property header"), + ParseTokenError::PropData => { + f.write_str("Buffer too small to read property data encoded in property header") + } + ParseTokenError::BeginName(e) => { + f.write_fmt(format_args!("Node name is not valid {}", e)) + } + ParseTokenError::BeginNameAlignment => { + f.write_str("Buffer is too small for begin node name alignment") + } + } + } +} + /// Read to the next token from `buf`, returning `(token, remaining_buffer)`. fn read_token(buf: &[u8]) -> Result<(ParsedToken<'_>, &[u8]), ParseTokenError> { let (token, rest) = U32b::read_from_prefix(buf).map_err(|_| ParseTokenError::BufLen)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) @@ -366,10 +411,10 @@ fn read_token(buf: &[u8]) -> Result<(ParsedToken<'_>, &[u8]), ParseTokenError> { } impl<'a> NodeIter<'a> { - fn parse(&mut self) -> Result>, Error<'a>> { + fn parse(&mut self) -> Result>, ErrorKind<'a>> { while !self.nodes.is_empty() { // Parse the next token. - let (token, rest) = read_token(self.nodes)?; + let (token, rest) = read_token(self.nodes).map_err(ErrorKind::NodeToken)?; debug_assert!(rest.len() % size_of::() == 0); let name = match token { @@ -378,7 +423,7 @@ impl<'a> NodeIter<'a> { continue; } ParsedToken::BeginNode { name } => name, - _ => return Err(Error::NodeBegin(token.raw())), + _ => return Err(ErrorKind::NodeBegin(token.raw())), }; self.nodes = rest; @@ -386,7 +431,7 @@ impl<'a> NodeIter<'a> { // Find if there is a properties section, which comes before children. let mut prop = self.nodes; 'prop: loop { - let (token, rest) = read_token(prop)?; + let (token, rest) = read_token(prop).map_err(ErrorKind::NodeToken)?; match token { ParsedToken::BeginNode { .. } => { // Begin node means move to parsing children nodes. @@ -397,7 +442,7 @@ impl<'a> NodeIter<'a> { break 'prop; } ParsedToken::Property { .. } | ParsedToken::Nop => {} - token => return Err(Error::NodeProp(token.raw())), + token => return Err(ErrorKind::NodeProp(token.raw())), }; prop = rest; @@ -411,7 +456,7 @@ impl<'a> NodeIter<'a> { let mut children = self.nodes; let mut begin_node_count = 0; 'children: loop { - let (token, rest) = read_token(children)?; + let (token, rest) = read_token(children).map_err(ErrorKind::NodeToken)?; match token { ParsedToken::EndNode => { if begin_node_count == 0 { @@ -426,7 +471,7 @@ impl<'a> NodeIter<'a> { begin_node_count += 1; } ParsedToken::Property { .. } | ParsedToken::Nop => {} - token => return Err(Error::NodeChildren(token.raw())), + token => return Err(ErrorKind::NodeChildren(token.raw())), }; children = rest; @@ -456,7 +501,7 @@ impl<'a> Iterator for NodeIter<'a> { type Item = Result, Error<'a>>; fn next(&mut self) -> Option { - self.parse().transpose() + self.parse().map_err(Error).transpose() } } @@ -516,11 +561,11 @@ pub struct PropertyIter<'a> { } impl<'a> PropertyIter<'a> { - fn parse(&mut self) -> Result>, Error<'a>> { + fn parse(&mut self) -> Result>, ErrorKind<'a>> { while !self.properties.is_empty() { // Parse the next token. let (token, rest) = - read_token(self.properties).map_err(|error| Error::PropertyTokenParse { + read_token(self.properties).map_err(|error| ErrorKind::PropertyTokenParse { node_name: self.node_name, error, })?; @@ -532,7 +577,7 @@ impl<'a> PropertyIter<'a> { } ParsedToken::Property { name_offset, data } => (name_offset, data, rest), _ => { - return Err(Error::PropertyToken { + return Err(ErrorKind::PropertyToken { node_name: self.node_name, token: token.raw(), }); @@ -541,7 +586,7 @@ impl<'a> PropertyIter<'a> { // Read the property name let name = string_from_offset(self.strings_block, name_off).map_err(|error| { - Error::PropertyNameStr { + ErrorKind::PropertyNameStr { node_name: self.node_name, error, } @@ -563,7 +608,7 @@ impl<'a> Iterator for PropertyIter<'a> { type Item = Result, Error<'a>>; fn next(&mut self) -> Option { - self.parse().transpose() + self.parse().map_err(Error).transpose() } } @@ -595,16 +640,16 @@ impl<'a> Property<'a> { <[T]>::ref_from_bytes(self.data) .map_err(|_| { // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) - Error::PropertyDataTypeBuffer { + Error(ErrorKind::PropertyDataTypeBuffer { node_name: self.node_name, prop_name: self.name, - } + }) })? .get(index) - .ok_or(Error::PropertyOffset { + .ok_or(Error(ErrorKind::PropertyOffset { node_name: self.node_name, prop_name: self.name, - }) + })) .copied() } @@ -624,9 +669,11 @@ impl<'a> Property<'a> { /// Read the data as a `&str`. pub fn read_str(&self) -> Result<&'a str, Error<'a>> { - extract_str_from_bytes(self.data).map_err(|error| Error::PropertyStr { - node_name: self.node_name, - error, + extract_str_from_bytes(self.data).map_err(|error| { + Error(ErrorKind::PropertyStr { + node_name: self.node_name, + error, + }) }) } @@ -635,28 +682,35 @@ impl<'a> Property<'a> { Ok(<[U64b]>::ref_from_bytes(self.data) .map_err(|_| { // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) - Error::PropertyDataTypeBuffer { + Error(ErrorKind::PropertyDataTypeBuffer { node_name: self.node_name, prop_name: self.name, - } + }) })? .iter() .map(|v| v.get())) } } -/// String parsing errors -#[derive(Debug, Error)] -pub enum StringError { +/// Errors when reading a string from the FDT. +#[derive(Debug)] +enum StringError { /// Invalid string block offset - #[error("Invalid string block offset")] Offset, /// No null terminator found - #[error("No null terminator found")] Null, /// String is not utf8 - #[error("String is not utf8 {0}")] - Utf8(#[from] core::str::Utf8Error), + Utf8(core::str::Utf8Error), +} + +impl Display for StringError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + StringError::Offset => f.write_str("Invalid string block offset"), + StringError::Null => f.write_str("No null terminator found"), + StringError::Utf8(e) => f.write_fmt(format_args!("String is not utf8 {}", e)), + } + } } /// An iterator to parse through memory reservations. @@ -665,13 +719,13 @@ pub struct MemoryReserveIter<'a> { } impl<'a> MemoryReserveIter<'a> { - fn parse(&mut self) -> Result, Error<'a>> { + fn parse(&mut self) -> Result, ErrorKind<'a>> { if self.memory_reservations.is_empty() { return Ok(None); } let (entry, rest) = spec::ReserveEntry::read_from_prefix(self.memory_reservations) - .map_err(|_| Error::MemoryReservationBlock)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) + .map_err(|_| ErrorKind::MemoryReservationBlock)?; // TODO: zerocopy: map_err (https://github.com/microsoft/openvmm/issues/759) if u64::from(entry.address) == 0 && u64::from(entry.size) == 0 { return Ok(None); @@ -687,10 +741,12 @@ impl<'a> Iterator for MemoryReserveIter<'a> { type Item = Result>; fn next(&mut self) -> Option { - self.parse().transpose() + self.parse().map_err(Error).transpose() } } +impl core::error::Error for StringError {} + /// Extract a string from bytes treated as a C String, stopping at the first null terminator. fn extract_str_from_bytes(bytes: &[u8]) -> Result<&str, StringError> { // Find the null terminator. diff --git a/vm/hv1/hvdef/Cargo.toml b/vm/hv1/hvdef/Cargo.toml index c7b0775155..db94e9b0d1 100644 --- a/vm/hv1/hvdef/Cargo.toml +++ b/vm/hv1/hvdef/Cargo.toml @@ -10,7 +10,6 @@ rust-version.workspace = true open_enum.workspace = true bitfield-struct.workspace = true static_assertions.workspace = true -thiserror.workspace = true zerocopy.workspace = true [lints] workspace = true diff --git a/vm/hv1/hvdef/src/lib.rs b/vm/hv1/hvdef/src/lib.rs index bf957a9639..65ec26f177 100644 --- a/vm/hv1/hvdef/src/lib.rs +++ b/vm/hv1/hvdef/src/lib.rs @@ -12,7 +12,6 @@ use core::fmt::Debug; use core::mem::size_of; use open_enum::open_enum; use static_assertions::const_assert; -use thiserror::Error; use zerocopy::FromBytes; use zerocopy::FromZeros; use zerocopy::Immutable; @@ -424,7 +423,7 @@ impl Debug for HvStatus { // // DEVNOTE: use `NonZeroU16` to get a niche optimization, since 0 is reserved // for success. -#[derive(Copy, Clone, PartialEq, Eq, IntoBytes, Immutable, KnownLayout, Error)] +#[derive(Copy, Clone, PartialEq, Eq, IntoBytes, Immutable, KnownLayout)] #[repr(transparent)] pub struct HvError(core::num::NonZeroU16); @@ -452,6 +451,8 @@ impl core::fmt::Display for HvError { } } +impl core::error::Error for HvError {} + macro_rules! hv_error { ($ty:ty, $(#[doc = $doc:expr] $ident:ident = $val:expr),* $(,)?) => {