From ebf32824e3e575ecf17bae529cbb101c288a7dcc Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Tue, 14 May 2024 11:36:05 +0300 Subject: [PATCH] Feature: new svd-parser based backend (#25) * Implement new svd-parser based frontend & filtering * CLI * Add parameter `--use-legacy` for old parser * Add parameter `--validate` for SVD validation alternatives which is supported by the new frontend --- README.md | 3 + keelhaul-cli/src/main.rs | 54 ++++- keelhaul/Cargo.toml | 1 + keelhaul/src/api.rs | 80 ++++++-- keelhaul/src/frontend/mod.rs | 1 + keelhaul/src/frontend/svd_parser.rs | 308 ++++++++++++++++++++++++++++ register-selftest/build/lib.rs | 1 + 7 files changed, 422 insertions(+), 26 deletions(-) create mode 100644 keelhaul/src/frontend/svd_parser.rs diff --git a/README.md b/README.md index a4a7e70..41d9c58 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,9 @@ cd keelhaul-cli cargo run -- --help ``` +N.b., SoC Hub chip verification so far has used the custom parser available for all subcommands via +`--use-legacy`. + ## SoC integration guide (register-selftest) See also. [register-selftest](./register-selftest/README.md). diff --git a/keelhaul-cli/src/main.rs b/keelhaul-cli/src/main.rs index 8fd4fd9..37f8eed 100644 --- a/keelhaul-cli/src/main.rs +++ b/keelhaul-cli/src/main.rs @@ -38,6 +38,10 @@ enum Command { #[arg(long = "validate", default_value = ValidateLevel(keelhaul::ValidateLevel::Disabled), requires = "svd")] validate_level: ValidateLevel, + + /// Use the original, custom SVD parser instead of the new `svd-parser` based parser + #[arg(long, action = clap::ArgAction::SetTrue, requires = "svd")] + use_legacy: bool, }, /// List all top level items (peripherals or subsystems) in the supplied sources /// @@ -53,6 +57,10 @@ enum Command { #[arg(long = "validate", default_value = ValidateLevel(keelhaul::ValidateLevel::Disabled), requires = "svd")] validate_level: ValidateLevel, + /// Use the original, custom SVD parser instead of the new `svd-parser` based parser + #[arg(long, action = clap::ArgAction::SetTrue, requires = "svd")] + use_legacy: bool, + /// Only list peripherals without register counts #[arg(long, action = clap::ArgAction::SetTrue)] no_count: bool, @@ -75,6 +83,10 @@ enum Command { #[arg(long = "validate", default_value = ValidateLevel(keelhaul::ValidateLevel::Disabled), requires = "svd")] validate_level: ValidateLevel, + + /// Use the original, custom SVD parser instead of the new `svd-parser` based parser + #[arg(long, action = clap::ArgAction::SetTrue, requires = "svd")] + use_legacy: bool, }, /// Count the number of readable registers with a known reset value in the inputs CountResetValues { @@ -87,6 +99,10 @@ enum Command { #[arg(long = "validate", default_value = ValidateLevel(keelhaul::ValidateLevel::Disabled), requires = "svd")] validate_level: ValidateLevel, + + /// Use the original, custom SVD parser instead of the new `svd-parser` based parser + #[arg(long, action = clap::ArgAction::SetTrue, requires = "svd")] + use_legacy: bool, }, /// Generate metadata tests #[command(name = "gen-regtest")] @@ -101,6 +117,10 @@ enum Command { #[arg(long = "validate", default_value = ValidateLevel(keelhaul::ValidateLevel::Disabled), requires = "svd")] validate_level: ValidateLevel, + /// Use the original, custom SVD parser instead of the new `svd-parser` based parser + #[arg(long, action = clap::ArgAction::SetTrue, requires = "svd")] + use_legacy: bool, + /// Type of test to be generated. Chain multiple for more kinds. #[arg(short = 't', long = "test", required = true, action = clap::ArgAction::Append)] tests_to_generate: Vec, @@ -434,14 +454,6 @@ fn main() -> anyhow::Result<()> { .command .as_ref() .and_then(|cmd| cmd.arch().map(|arch| arch.into())); - if let Some(cmd) = cli.command.as_ref() { - if cmd - .validate_level() - .is_some_and(|l| l.0 != keelhaul::ValidateLevel::Disabled) - { - anyhow::bail!("validate level not implemented"); - } - } if let Some(cmd) = &cli.command { match cmd { @@ -449,9 +461,10 @@ fn main() -> anyhow::Result<()> { input: _input, arch: _arch, validate_level: _, + use_legacy, } => { let sources = sources.unwrap(); - match keelhaul::dry_run(&sources, arch.unwrap()).with_context(|| { + match keelhaul::dry_run(&sources, arch.unwrap(), *use_legacy).with_context(|| { format!("could not execute dry run for arch: {arch:?}, sources: {sources:?}") }) { Ok(_) => println!("keelhaul: dry run completed successfully"), @@ -465,22 +478,26 @@ fn main() -> anyhow::Result<()> { sorting, validate_level: _, no_rubric, + use_legacy, } => ls_top( &sources.unwrap(), arch.unwrap(), *sorting, *no_count, *no_rubric, + *use_legacy, )?, Command::CountRegisters { input: _input, validate_level: _, arch: _arch, + use_legacy, } => { let output = keelhaul::count_registers_svd( &sources.unwrap(), arch.unwrap(), &keelhaul::Filters::all(), + *use_legacy, )?; println!("{output}"); } @@ -496,6 +513,7 @@ fn main() -> anyhow::Result<()> { validate_level: _, filter_top_regex, filter_path_regex, + use_legacy, } => { let mut config = keelhaul::CodegenConfig::default() .tests_to_generate(tests_to_generate.iter().cloned().map(|tk| tk.0).collect()) @@ -527,23 +545,27 @@ fn main() -> anyhow::Result<()> { sources.unwrap(), *no_format, *use_zero_as_default_reset, + *use_legacy, )? } Command::CountResetValues { input: _input, arch: _arch, validate_level: _, + use_legacy, } => { let sources = sources.unwrap(); let total = keelhaul::count_registers_svd( &sources, arch.unwrap(), &keelhaul::Filters::all(), + *use_legacy, )?; let with_reset = keelhaul::count_readable_registers_with_reset_value( &sources, arch.unwrap(), &keelhaul::Filters::all(), + *use_legacy, )?; let percent = (with_reset as f64 / total as f64) * 100.; println!( @@ -604,9 +626,17 @@ fn generate( sources: Vec, no_format: bool, use_zero_as_default_reset: bool, + use_legacy: bool, ) -> Result<(), anyhow::Error> { let output = if no_format { - keelhaul::generate_tests(&sources, arch, &config, &filters, use_zero_as_default_reset)? + keelhaul::generate_tests( + &sources, + arch, + &config, + &filters, + use_zero_as_default_reset, + use_legacy, + )? } else { keelhaul::generate_tests_with_format( &sources, @@ -614,6 +644,7 @@ fn generate( &config, &filters, use_zero_as_default_reset, + use_legacy, )? }; println!("{output}"); @@ -626,8 +657,9 @@ fn ls_top( sorting: Sorting, no_count: bool, no_rubric: bool, + use_legacy: bool, ) -> Result<(), anyhow::Error> { - let mut top_and_count = keelhaul::list_top(sources, arch)?; + let mut top_and_count = keelhaul::list_top(sources, arch, use_legacy)?; if top_and_count.is_empty() { println!("keelhaul: no peripherals found in input"); } diff --git a/keelhaul/Cargo.toml b/keelhaul/Cargo.toml index a7728c5..31ef8a7 100644 --- a/keelhaul/Cargo.toml +++ b/keelhaul/Cargo.toml @@ -32,3 +32,4 @@ svd = { package = "svd-rs", version = "0.14.8", features = [ rustfmt = { version = "0.10.0", optional = true } chrono = { version = "0.4.37", default-features = false, features = ["now"] } indoc = "2.0.5" +svd-parser = { version = "0.14.5", features = ["derive-from", "expand"] } diff --git a/keelhaul/src/api.rs b/keelhaul/src/api.rs index 682a0cd..5e03e2a 100644 --- a/keelhaul/src/api.rs +++ b/keelhaul/src/api.rs @@ -89,8 +89,13 @@ impl str::FromStr for TestKind { /// Run the parser on the inputs without doing anything /// /// Good for checking whether the input files can be parsed by Keelhaul. -pub fn dry_run(sources: &[ModelSource], arch: ArchWidth) -> Result<(), ApiError> { - parse_registers(sources, arch, &Filters::all(), false)?; +/// +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser +pub fn dry_run(sources: &[ModelSource], arch: ArchWidth, use_legacy: bool) -> Result<(), ApiError> { + parse_registers(sources, arch, &Filters::all(), false, use_legacy)?; Ok(()) } @@ -105,11 +110,14 @@ pub fn dry_run(sources: &[ModelSource], arch: ArchWidth) -> Result<(), ApiError> /// /// * `use_zero_as_default_reset` - Assume zero as the default reset value if not provided by the /// source file. Provided for convenience, as `0` is a very common reset value. +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser fn parse_registers( sources: &[ModelSource], arch: ArchWidth, filters: &Filters, use_zero_as_default_reset: bool, + use_legacy: bool, ) -> Result { for src in sources { match src.format { @@ -129,14 +137,9 @@ fn parse_registers( for src in sources { match src.format { SourceFormat::Svd(vlevel) => { - if vlevel != ValidateLevel::Disabled { - return Err( - NotImplementedError::UnsupportedOption(format!("{:?}", vlevel)).into(), - ); - } let default_reset_value = use_zero_as_default_reset.then_some(0); - let regs = + let regs = if use_legacy { // Safety: max 3 levels of hierarchy (periph + cluster + reg) unsafe { crate::frontend::svd_legacy::parse_svd_into_registers( @@ -146,8 +149,17 @@ fn parse_registers( default_reset_value, ) } - .map_err(Box::new)?; - registers.push(regs); + .map_err(Box::new)? + } else { + crate::frontend::svd_parser::parse_svd_into_registers( + src.path(), + arch.into(), + filters, + vlevel, + ) + .map_err(Box::new)? + }; + registers.push(regs) } SourceFormat::Ieee1685 => todo!(), } @@ -156,12 +168,17 @@ fn parse_registers( Ok(registers.into_iter().next().unwrap()) } +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser fn parse_registers_for_analysis( sources: &[ModelSource], filters: &Filters, arch: ArchWidth, + use_legacy: bool, ) -> Result>, ApiError> { - Ok(parse_registers(sources, arch, filters, false)? + Ok(parse_registers(sources, arch, filters, false, use_legacy)? .clone() .into_iter() .map(|reg| Box::new(reg) as Box) @@ -195,14 +212,25 @@ fn apply_fmt(input: String) -> String { } } +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser pub fn generate_tests( sources: &[ModelSource], arch_ptr_size: ArchWidth, test_cfg: &CodegenConfig, filters: &Filters, use_zero_as_default_reset: bool, + use_legacy: bool, ) -> Result { - let registers = parse_registers(sources, arch_ptr_size, filters, use_zero_as_default_reset)?; + let registers = parse_registers( + sources, + arch_ptr_size, + filters, + use_zero_as_default_reset, + use_legacy, + )?; let test_cases = codegen::RegTestCases::from_registers(®isters, test_cfg); // FIXME: it would be good to have this message prior to generation info!("Wrote {} test cases.", test_cases.test_case_count); @@ -210,6 +238,10 @@ pub fn generate_tests( Ok(test_cases.to_tokens().to_string()) } +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser #[cfg(feature = "rustfmt")] pub fn generate_tests_with_format( sources: &[ModelSource], @@ -217,6 +249,7 @@ pub fn generate_tests_with_format( test_cfg: &codegen::CodegenConfig, filters: &Filters, use_zero_as_default_reset: bool, + use_legacy: bool, ) -> Result { let s = generate_tests( sources, @@ -224,25 +257,36 @@ pub fn generate_tests_with_format( test_cfg, filters, use_zero_as_default_reset, + use_legacy, )?; Ok(apply_fmt(s)) } +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser pub fn count_registers_svd( sources: &[ModelSource], arch: ArchWidth, filters: &Filters, + use_legacy: bool, ) -> Result { - let registers = parse_registers_for_analysis(sources, filters, arch)?; + let registers = parse_registers_for_analysis(sources, filters, arch, use_legacy)?; Ok(registers.len()) } +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser pub fn count_readable_registers_with_reset_value( sources: &[ModelSource], arch: ArchWidth, filters: &Filters, + use_legacy: bool, ) -> Result { - let registers = parse_registers_for_analysis(sources, filters, arch)?; + let registers = parse_registers_for_analysis(sources, filters, arch, use_legacy)?; Ok(registers .iter() .filter(|reg| reg.is_readable() && reg.has_reset_value()) @@ -252,11 +296,17 @@ pub fn count_readable_registers_with_reset_value( /// Returns top level containers (peripherals or subsystems) and the number of registers in each /// /// `Vec<(container, register count)>` +/// +/// # Arguments +/// +/// * `use_legacy` - Use the original, custom SVD parser instead of the new `svd-parser` based +/// parser pub fn list_top( sources: &[ModelSource], arch: ArchWidth, + use_legacy: bool, ) -> Result, ApiError> { - let registers = parse_registers_for_analysis(sources, &Filters::all(), arch)?; + let registers = parse_registers_for_analysis(sources, &Filters::all(), arch, use_legacy)?; let tops = registers .iter() diff --git a/keelhaul/src/frontend/mod.rs b/keelhaul/src/frontend/mod.rs index 36d68d9..3e21925 100644 --- a/keelhaul/src/frontend/mod.rs +++ b/keelhaul/src/frontend/mod.rs @@ -1,3 +1,4 @@ //! The point of any frontend is to convert the given source file into a model of [Registers] pub(crate) mod svd_legacy; +pub(crate) mod svd_parser; diff --git a/keelhaul/src/frontend/svd_parser.rs b/keelhaul/src/frontend/svd_parser.rs new file mode 100644 index 0000000..82eabef --- /dev/null +++ b/keelhaul/src/frontend/svd_parser.rs @@ -0,0 +1,308 @@ +use std::{fmt, iter, path}; + +use crate::{ + error::Error, + model::{self, AddrRepr, RegPath, RegPathSegment, Register, Registers, UniquePath}, + util, Filters, PtrSize, +}; +use itertools::Itertools; +use log::info; +use svd::Name; + +// TODO: error handling: check for unwrap, expect + +/// Property chain element +/// +/// Helps with traversing the hierarchy for specific attributes. +trait PropChainElem: svd::Name + svd::Description + fmt::Debug { + /// Address offset, i.e., base address or offset + fn offset(&self) -> Option; + /// Size of the register + fn size(&self) -> Option; + fn access(&self) -> Option; + fn reset_value(&self) -> Option; + fn reset_mask(&self) -> Option; +} + +impl PropChainElem for svd::Device { + fn offset(&self) -> Option { + None + } + + fn size(&self) -> Option { + self.default_register_properties.size + } + + fn access(&self) -> Option { + self.default_register_properties.access + } + + fn reset_value(&self) -> Option { + self.default_register_properties.reset_value + } + + fn reset_mask(&self) -> Option { + self.default_register_properties.reset_mask + } +} + +impl PropChainElem for svd::Peripheral { + fn offset(&self) -> Option { + Some(self.base_address) + } + + fn size(&self) -> Option { + self.default_register_properties.size + } + + fn access(&self) -> Option { + self.default_register_properties.access + } + + fn reset_value(&self) -> Option { + self.default_register_properties.reset_value + } + + fn reset_mask(&self) -> Option { + self.default_register_properties.reset_mask + } +} + +impl PropChainElem for svd::Cluster { + fn offset(&self) -> Option { + Some(self.address_offset as u64) + } + + fn size(&self) -> Option { + self.default_register_properties.size + } + + fn access(&self) -> Option { + self.default_register_properties.access + } + + fn reset_value(&self) -> Option { + self.default_register_properties.reset_value + } + + fn reset_mask(&self) -> Option { + self.default_register_properties.reset_mask + } +} + +impl PropChainElem for svd::Register { + fn offset(&self) -> Option { + Some(self.address_offset as u64) + } + + fn size(&self) -> Option { + self.properties.size + } + + fn access(&self) -> Option { + self.properties.access + } + + fn reset_value(&self) -> Option { + self.properties.reset_value + } + + fn reset_mask(&self) -> Option { + self.properties.reset_mask + } +} + +fn to_reg( + arch_size: u32, + device: &svd::Device, + periph: &svd::Peripheral, + cluster_chain: &[svd::Cluster], + reg: &svd::Register, +) -> Register { + let parent_chain = iter::once(device as &dyn PropChainElem) + .chain(iter::once(periph as &dyn PropChainElem)) + .chain(cluster_chain.iter().map(|cl| cl as &dyn PropChainElem)) + .chain(iter::once(reg as &dyn PropChainElem)) + .collect_vec(); + let path = RegPath::new( + parent_chain + .iter() + // Skip the device for path resolution + .skip(1) + .map(|p| RegPathSegment { + name: p.name().replace("[%s]", "placeholder"), + }) + .collect(), + ); + let addr = AddrRepr::from_vec( + parent_chain + .iter() + // Skip the device for address formation + .skip(1) + .filter_map(|p| p.offset()) + .collect(), + arch_size, + ) + .expect("could not make address"); + let size = parent_chain + .iter() + .rev() + .find_map(|p| p.size()) + .unwrap_or_else(|| { + panic!( + "could not determine size for register at path {:?}, chain: {:#?}", + path.join("-"), + parent_chain + ) + }); + let access = parent_chain + .iter() + .rev() + .find_map(|p| p.access()) + .unwrap_or(svd::Access::ReadWrite); + let reset_value = parent_chain.iter().rev().find_map(|p| p.reset_value()); + let reset_mask = parent_chain.iter().rev().find_map(|p| p.reset_mask()); + let reset_value = match (reset_value, reset_mask) { + // CMSIS-SVD requires both value and mask to be present + (Some(val), Some(mask)) => Some(model::ValueOnReset::new(val, Some(mask), size)), + _ => None, + }; + Register::new(path, addr, size, access, reset_value) +} + +// Recursive +fn select_regs_from_cluster( + device: &svd::Device, + periph: &svd::Peripheral, + cluster_chain: &[svd::Cluster], + size: u32, +) -> Vec { + let last = cluster_chain + .last() + .expect("internal error: chain cannot be empty"); + last + // Direct child registers + .registers() + .map(|reg| to_reg(size, device, periph, cluster_chain, reg)) + .chain( + // Registers via clusters + last.clusters().flat_map(|cluster| { + let mut cluster_chain = cluster_chain.to_vec(); + cluster_chain.push(cluster.clone()); + select_regs_from_cluster(device, periph, &cluster_chain, size) + }), + ) + .collect::>() +} + +/// # Arguments +/// +/// * `size` - Address representation +fn select_regs_from_periphs( + device: &svd::Device, + periphs: &[svd::Peripheral], + size: u32, +) -> Vec { + info!( + "Scanning {} peripherals: {}", + periphs.len(), + periphs.iter().map(|p| p.name()).join(", ") + ); + + periphs + .iter() + .flat_map(|periph| { + periph + // Direct child registers + .registers() + .map(|reg| to_reg(size, device, periph, &[], reg)) + .chain( + // Registers via clusters + periph.clusters().flat_map(|cluster| { + select_regs_from_cluster(device, periph, &vec![cluster.clone()], size) + }), + ) + }) + .collect() +} + +/// Parse the file at `svd_path` into a list of registers with provided filters & constraints +/// +/// # Arguments +/// +/// * `svd_path` - The path to the SVD file +/// * `reg_filter` - What registers to include or exclude +/// * `periph_filter` - What peripherals to include or exclude +/// * `syms_filter` - - What symbols to include or exclude (applying to full register identifier) +pub(crate) fn parse_svd_into_registers( + svd_source: &path::Path, + arch: PtrSize, + filters: &Filters, + validate_level: crate::ValidateLevel, +) -> Result { + if filters.reg.is_some() { + log::warn!("register level filtering is not implemented, reg filter is ignored"); + } + + let svd_xml = util::read_file_or_panic(svd_source); + + let parse_config = svd_parser::Config::default(); + parse_config.validate_level(validate_level); + parse_config.ignore_enums(false); + + // Derive register properties from parents + parse_config.expand_properties(true); + + // Always expand `arrays` and `deriveFrom`'s as we are only interested in the final output form + parse_config.expand(true); + + // TODO: error handling + let device = svd_parser::parse_with_config(&svd_xml, &parse_config).unwrap(); + + // Filter out top-level elements + let mut top_elems = device.peripherals.clone(); + if let Some(f) = filters.top.as_ref() { + let total = top_elems.len(); + top_elems = top_elems + .into_iter() + .filter(|p| f.is_allowed(p.name())) + .collect_vec(); + let selected = top_elems.len(); + info!( + "Top filter selected {selected}/{total} of top elements: {}", + top_elems.iter().map(|p| p.name()).join(", "), + ); + } + + // TODO: error handling + let mut registers = select_regs_from_periphs(&device, &top_elems, arch.count_bits()); + + if let Some(f) = filters.path.as_ref() { + let remaining = registers.len(); + registers = registers + .into_iter() + .filter(|reg| { + f.is_allowed( + ®.path() + // Dash '-' is used as the path combiner + .join("-"), + ) + }) + .collect_vec(); + let selected = registers.len(); + info!( + "Path filter selected {selected}/{remaining} of remaining registers: {}", + top_elems.iter().map(|p| p.name()).join(", "), + ); + } + + // If zero registers were chosen for generation, this run is useless. + // Therefore we treat it as an error. + // TODO: allow ignoring this error for special cases with a suitable flag on Config-struct + if registers.is_empty() { + return Err(Error::ZeroEntries); + } + + info!("Discovered {} registers", registers.len()); + Ok(registers.into()) +} diff --git a/register-selftest/build/lib.rs b/register-selftest/build/lib.rs index c89def9..cdbf07d 100644 --- a/register-selftest/build/lib.rs +++ b/register-selftest/build/lib.rs @@ -129,6 +129,7 @@ fn main() -> anyhow::Result<()> { &test_cfg, &Filters::from_filters(None, Some(periph_filter), syms_filter), true, + true, )?; file_output.write_all(test_cases.as_bytes())?; let path = get_path_to_output();