-
Notifications
You must be signed in to change notification settings - Fork 15
Fix system attribute addressing #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,7 +7,7 @@ use byteorder::{ByteOrder, LittleEndian}; | |||||||||||
| use crate::errors::{AttributeParseError, TockError, TockloaderError}; | ||||||||||||
| use crate::IO; | ||||||||||||
|
|
||||||||||||
| use super::decode::{bytes_to_string, decode_attribute}; | ||||||||||||
| use super::decode::decode_attribute; | ||||||||||||
|
|
||||||||||||
| /// This structure contains all relevant information about board that is stored | ||||||||||||
| /// in the bootloader ROM. | ||||||||||||
|
|
@@ -48,12 +48,28 @@ impl SystemAttributes { | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Read system attributes using a generalized connection. A bootloader must be | ||||||||||||
| /// present on this board for this function to work properly. | ||||||||||||
| /// | ||||||||||||
| /// Check if the bootloader is present on the version of Tock | ||||||||||||
| pub(crate) async fn bootloader_is_present( | ||||||||||||
| conn: &mut dyn IO, | ||||||||||||
| flash_address: u64, | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| ) -> Result<bool, TockloaderError> { | ||||||||||||
| // If a bootloader is present, the start of 0x400 will read | ||||||||||||
| // "TOCKBOOTLOADER", which is exactly 14 bytes long, so we'll read | ||||||||||||
| // 14 bytes starting from 0x400. | ||||||||||||
| let flag = conn.read(flash_address + 0x400, 14).await?; | ||||||||||||
| if flag == "TOCKBOOTLOADER".as_bytes() { | ||||||||||||
| Ok(true) | ||||||||||||
| } else { | ||||||||||||
| Ok(false) | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Add a space after |
||||||||||||
| /// Read system and kernel attributes. | ||||||||||||
| /// System attributes are read only if the board has | ||||||||||||
| /// a bootloader present. | ||||||||||||
| /// # Parameters | ||||||||||||
| /// - `conn` : Either a SerialConnection or a ProbeRSConnection | ||||||||||||
| /// | ||||||||||||
| /// - `conn` : Either a SerialConnection or a ProbeRSConnection. | ||||||||||||
| /// - `flash_address` : The start of flash memory. | ||||||||||||
| /// - `start_address` : The start of User Space Applications in flash memory. | ||||||||||||
| /// # Returns | ||||||||||||
| /// - Ok(result): if attributes were read successfully | ||||||||||||
| /// - Err(TockloaderError::MisconfiguredBoard): if no start address is found or valid | ||||||||||||
|
|
@@ -62,74 +78,91 @@ impl SystemAttributes { | |||||||||||
| /// - Err(TockloaderError::SerialReadError): if reading fails on Serial | ||||||||||||
| pub(crate) async fn read_system_attributes( | ||||||||||||
| conn: &mut dyn IO, | ||||||||||||
| flash_address: u64, | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| start_address: u64, | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| ) -> Result<SystemAttributes, TockloaderError> { | ||||||||||||
| let mut result = SystemAttributes::new(); | ||||||||||||
| // System attributes start at 0x600 and up to 0x9FF. See: | ||||||||||||
| // https://book.tockos.org/doc/memory_layout#flash-1 | ||||||||||||
| let address = 0x600; | ||||||||||||
| let address = flash_address + 0x600; | ||||||||||||
|
Comment on lines
67
to
+87
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continue this comment. It start at an offset of 0x600, an example being visible at . Furthermore ,this is also evidenced by the linker script in tock upstream |
||||||||||||
|
|
||||||||||||
| let buf = conn.read(address, 64 * 16).await?; | ||||||||||||
|
|
||||||||||||
| let mut data = buf.chunks(64); | ||||||||||||
|
|
||||||||||||
| for current_slot in 0..data.len() { | ||||||||||||
| let slot_data = match data.next() { | ||||||||||||
| Some(data) => data, | ||||||||||||
| None => break, | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| // If the attribute chunk was successfully decoded, assign its value | ||||||||||||
| // to the corresponding field in `result` based on the index: | ||||||||||||
| // - 0 = board name, | ||||||||||||
| // - 1 = architecture, | ||||||||||||
| // - 2 = application start address (parsed from hex string), | ||||||||||||
| // - 3 = boot hash, _ = invalid or missing data is skipped. | ||||||||||||
| // NOTE: this can also be done by looping directly through the key attributes. | ||||||||||||
| if let Some(decoded_attributes) = decode_attribute(slot_data) { | ||||||||||||
| match current_slot { | ||||||||||||
| 0 => { | ||||||||||||
| result.board = Some(decoded_attributes.value.to_string()); | ||||||||||||
| } | ||||||||||||
| 1 => { | ||||||||||||
| result.arch = Some(decoded_attributes.value.to_string()); | ||||||||||||
| } | ||||||||||||
| 2 => { | ||||||||||||
| // Parse hex string like "0x40000" into actual u64 value | ||||||||||||
| result.appaddr = Some( | ||||||||||||
| u64::from_str_radix( | ||||||||||||
| decoded_attributes | ||||||||||||
| .value | ||||||||||||
| .to_string() | ||||||||||||
| .trim_start_matches("0x"), | ||||||||||||
| 16, | ||||||||||||
| ) | ||||||||||||
| .map_err(|e| { | ||||||||||||
| TockError::AttributeParsing(AttributeParseError::InvalidNumber(e)) | ||||||||||||
| })?, | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
| 3 => { | ||||||||||||
| result.boothash = Some(decoded_attributes.value.to_string()); | ||||||||||||
| let has_bootloader = Self::bootloader_is_present(conn, flash_address) | ||||||||||||
| .await | ||||||||||||
| .unwrap_or(false); | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bubble up the error, don't unwrap_or |
||||||||||||
|
|
||||||||||||
| if !has_bootloader { | ||||||||||||
| // TODO: Chain: CLI arguments -> hardcoded start_address -> calculate from kernel app start address | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| result.appaddr = Some(start_address); | ||||||||||||
| } else { | ||||||||||||
| for current_slot in 0..data.len() { | ||||||||||||
|
Comment on lines
+100
to
+101
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
You can also do the cleanup in this PR, but you will need to test the new implementation. We can discuss what I don't like, if you want to cleanup |
||||||||||||
| let slot_data = match data.next() { | ||||||||||||
| Some(data) => data, | ||||||||||||
| None => break, | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| // If the attribute chunk was successfully decoded, assign its value | ||||||||||||
| // to the corresponding field in `result` based on the index: | ||||||||||||
| // - 0 = board name, | ||||||||||||
| // - 1 = architecture, | ||||||||||||
| // - 2 = application start address (parsed from hex string), | ||||||||||||
| // - 3 = boot hash, _ = invalid or missing data is skipped. | ||||||||||||
| // NOTE: this can also be done by looping directly through the key attributes. | ||||||||||||
| if let Some(decoded_attributes) = decode_attribute(slot_data) { | ||||||||||||
| match current_slot { | ||||||||||||
| 0 => { | ||||||||||||
| result.board = Some(decoded_attributes.value.to_string()); | ||||||||||||
| } | ||||||||||||
| 1 => { | ||||||||||||
| result.arch = Some(decoded_attributes.value.to_string()); | ||||||||||||
| } | ||||||||||||
| 2 => { | ||||||||||||
| // Parse hex string like "0x40000" into actual u64 value | ||||||||||||
| result.appaddr = Some( | ||||||||||||
| u64::from_str_radix( | ||||||||||||
| decoded_attributes | ||||||||||||
| .value | ||||||||||||
| .to_string() | ||||||||||||
| .trim_start_matches("0x"), | ||||||||||||
| 16, | ||||||||||||
| ) | ||||||||||||
| .map_err(|e| { | ||||||||||||
| TockError::AttributeParsing(AttributeParseError::InvalidNumber( | ||||||||||||
| e, | ||||||||||||
| )) | ||||||||||||
| })?, | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
| 3 => { | ||||||||||||
| result.boothash = Some(decoded_attributes.value.to_string()); | ||||||||||||
| } | ||||||||||||
| _ => {} | ||||||||||||
| } | ||||||||||||
| _ => {} | ||||||||||||
| } else { | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
| } else { | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // TODO(eva-cosma): separate kernel attributes from kernel flags. | ||||||||||||
|
|
||||||||||||
| let address = 0x40E; | ||||||||||||
| // At this address lives the version of the bootloader we are using. | ||||||||||||
| // Trying to read this when no bootloader is present will result in a panic. | ||||||||||||
| // So we first verify if we have a bootloader, if not we just skip this and | ||||||||||||
| // return None for the bootloader version. | ||||||||||||
| let address = flash_address + 0x40E; | ||||||||||||
|
|
||||||||||||
| let buf = conn.read(address, 8).await?; | ||||||||||||
| let buf = conn.read(address, 8).await?; | ||||||||||||
|
|
||||||||||||
| let string = String::from_utf8(buf.to_vec()) | ||||||||||||
| .map_err(|e| TockError::AttributeParsing(AttributeParseError::InvalidString(e)))?; | ||||||||||||
| let string = String::from_utf8(buf.to_vec()) | ||||||||||||
| .map_err(|e| TockError::AttributeParsing(AttributeParseError::InvalidString(e)))?; | ||||||||||||
|
|
||||||||||||
| let string = string.trim_matches(char::from(0)); | ||||||||||||
| let string = string.trim_matches(char::from(0)); | ||||||||||||
|
|
||||||||||||
| result.bootloader_version = Some(string.to_owned()); | ||||||||||||
| result.bootloader_version = Some(string.to_owned()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // TODO(eva-cosma): separate kernel attributes from kernel flags. | ||||||||||||
|
|
||||||||||||
| // The 100 bytes prior to the application start address are reserved for the kernel attributes and flags | ||||||||||||
| let kernel_attr_addr = result | ||||||||||||
|
|
@@ -138,7 +171,9 @@ impl SystemAttributes { | |||||||||||
| - 100; | ||||||||||||
| let kernel_attr_binary = conn.read(kernel_attr_addr, 100).await?; | ||||||||||||
|
|
||||||||||||
| let sentinel = bytes_to_string(&kernel_attr_binary[96..100]); | ||||||||||||
| let sentinel = std::str::from_utf8(&kernel_attr_binary[96..100]) | ||||||||||||
| .unwrap_or("unknown") | ||||||||||||
| .to_string(); | ||||||||||||
|
Comment on lines
+174
to
+176
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't have a sentinel, or we can't convert it, throw an error. A very specific one as well, INVALID_SENTINEL. something like that. Sentinels exist to be unchanged and always correct. |
||||||||||||
| let kernel_version = LittleEndian::read_uint(&kernel_attr_binary[95..96], 1); | ||||||||||||
|
|
||||||||||||
| let app_memory_len = LittleEndian::read_u32(&kernel_attr_binary[84..92]); | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| #[derive(Clone)] | ||
| pub struct BoardSettings { | ||
| pub arch: Option<String>, | ||
| pub flash_address: u64, | ||
| pub start_address: u64, | ||
|
Comment on lines
+4
to
5
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, but I've changed my mind, let's not have 1-to-1 parity with tockloader-python in the naming scheme here cause this is confusing. We can do /// Previously named 'flash_address' in the python version of tockloader
pub flash_start_address: u64,
/// Previously named `start_address` in the python version of tockloader
pub app_start_Address: u64, |
||
| pub page_size: u64, | ||
| pub ram_start_address: u64, | ||
|
|
@@ -12,6 +13,7 @@ impl Default for BoardSettings { | |
| fn default() -> Self { | ||
| Self { | ||
| arch: None, | ||
| flash_address: 0x00000, // this would be actually like -0x10000 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can flash be negative? |
||
| start_address: 0x30000, | ||
| page_size: 512, | ||
| ram_start_address: 0x20000000, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,8 @@ use crate::{CommandInfo, IOCommands}; | |
| #[async_trait] | ||
| impl CommandInfo for TockloaderConnection { | ||
| async fn info(&mut self) -> Result<GeneralAttributes, TockloaderError> { | ||
| let installed_apps = self.read_installed_apps().await.unwrap(); | ||
| let system_atributes = self.read_system_attributes().await.unwrap(); | ||
| let installed_apps = self.read_installed_apps().await?; | ||
| let system_atributes = self.read_system_attributes().await?; | ||
|
Comment on lines
+11
to
+12
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Nice! |
||
| Ok(GeneralAttributes::new(system_atributes, installed_apps)) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are changing this, can we change it directly to the std implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utf8_decode should be eradicated from this codebase