Expand SDRAM code space to 4MB.#288
Conversation
WalkthroughThe pull request implements changes to firmware validation and memory layout across several modules. It adds stricter firmware file size checks and updates function signatures by adding a boolean parameter. The firmware header is modified by replacing reserved bytes with a declared hash block. Memory regions in the linker script and SDRAM definitions are doubled. The Trezor utility removes progress tracking and simplifies firmware hashing. Python modules now update hash assignments, include a new constant, and change the return type for hash calculations. Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
python/src/trezorlib/firmware.py (1)
74-74: Fix typo in constant nameThe constant name should be "FIRMWARE_SIZE_LIMIT" not "FIREMWARE_SIZE_LIMIT".
-FIREMWARE_SIZE_LIMIT = V2_CHUNK_SIZE * 16 +FIRMWARE_SIZE_LIMIT = V2_CHUNK_SIZE * 16core/embed/extmod/modtrezorutils/modtrezorutils.c (1)
49-55: Comment removal needs attentionThe ui_progress function is commented out rather than properly removed.
-// static void ui_progress(mp_obj_t ui_wait_callback, uint32_t current, -// uint32_t total) { -// if (mp_obj_is_callable(ui_wait_callback)) { -// mp_call_function_2_protected(ui_wait_callback, mp_obj_new_int(current), -// mp_obj_new_int(total)); -// } -// }core/embed/trezorhal/image.h (1)
190-191: Fix typo in parameter nameParameter name has a spelling error: "chcek_remain" should be "check_remain".
- bool chcek_remain); + bool check_remain);core/embed/trezorhal/image.c (3)
334-334: Fix typo in parameter name.Parameter
chcek_remainhas a typo. Should becheck_remain.- const size_t code_len_check, - bool chcek_remain) { + const size_t code_len_check, + bool check_remain) {
377-387: Use shorter variable name in loop and fix typo references.The
iloop variable is fine butchcek_remaintypo appears again.- if (chcek_remain) { + if (check_remain) {
766-775: Change misleading error message.Error says "invalid" when the problem is read failure.
ExecuteCheck_ADV( emmc_fs_file_read("0:data/fw_p2.bin", 0, (uint32_t*)FMC_SDRAM_FIRMWARE_P2_ADDRESS, MAX(path_info.size, fw_external_size), &processed_len), true, { if (error_msg != NULL) - strncpy(error_msg, "Firmware code invalid! (P2_EMMC_2)", + strncpy(error_msg, "Failed to read firmware P2 file!", error_msg_len); return secfalse; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/embed/emmc_wrapper/emmc_commands.c(3 hunks)core/embed/extmod/modtrezorutils/modtrezorutils.c(2 hunks)core/embed/firmware/header.S(1 hunks)core/embed/firmware/memory_H.ld(1 hunks)core/embed/trezorhal/image.c(9 hunks)core/embed/trezorhal/image.h(4 hunks)core/embed/trezorhal/sdram.h(1 hunks)python/src/trezorlib/_internal/firmware_headers.py(2 hunks)python/src/trezorlib/firmware.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
core/embed/trezorhal/image.c (4)
core/embed/emmc_wrapper/emmc_fs.h (5)
emmc_fs_dir_make(150-150)emmc_fs_file_delete(138-138)emmc_fs_file_write(133-136)emmc_fs_path_info(124-124)emmc_fs_file_read(130-132)core/embed/trezorhal/common.c (1)
hal_delay(292-292)core/embed/trezorhal/common.h (1)
hal_delay(107-107)core/embed/trezorhal/image.h (1)
check_image_contents_ADV(186-191)
core/embed/emmc_wrapper/emmc_commands.c (4)
core/embed/bootloader/messages.c (1)
send_failure(312-317)core/embed/bootloader/messages.h (1)
send_failure(41-41)core/embed/trezorhal/image.h (1)
check_image_contents_ADV(186-191)core/embed/trezorhal/image.c (1)
check_image_contents_ADV(329-399)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (18)
core/embed/trezorhal/sdram.h (1)
65-65: Code space doubled to 4MB.The firmware memory allocation is now doubled from 2MB to 4MB, matching PR objective.
core/embed/firmware/header.S (1)
30-30: Reserved field repurposed for hash block size.Previously unused reserved field now stores hash block size.
core/embed/firmware/memory_H.ld (2)
7-7: FLASH2 memory region doubled.FLASH2 length increased from 2048K to 4096K.
14-14: EXRAM memory region doubled.EXRAM length increased from 2048K to 4096K.
python/src/trezorlib/_internal/firmware_headers.py (3)
256-258: Updated code hash calculation to return hash block.Now captures hash_block from calculate_code_hashes.
261-261: Added hash_block to digest header.Ensures hash_block is included in digest calculations.
273-273: Added hash_block to rehash method.Maintains hash_block consistency during rehashing.
python/src/trezorlib/firmware.py (3)
197-198: Appropriate replacement of reserved fieldReplacing the reserved field with a meaningful hash_block parameter aligns with the firmware header changes in the core.
357-380: Return type and logic changes for chunk size calculationThe function now returns a tuple with hash list and chunk size flag. The logic correctly doubles the chunk size when needed.
396-398: Call site updated correctlyThe call to calculate_code_hashes properly unpacks the tuple return value.
core/embed/extmod/modtrezorutils/modtrezorutils.c (1)
163-169: Function implementation significantly simplifiedThe function now returns empty data without computing a hash. Comment doesn't explain the rationale.
What happens when a caller expects actual hash data from this function? Is this intentional?
core/embed/emmc_wrapper/emmc_commands.c (4)
603-608: Added size verification for P2 segmentNew check prevents oversized firmware segments. Good defensive programming.
614-614: Updated function call with new parameterAdded boolean parameter to check_image_contents_ADV call.
625-625: Updated size check against new macroSize verification now uses the updated FIRMWARE_IMAGE_MAXSIZE constant.
853-853: Added parameter to function callConsistent update of check_image_contents_ADV call with the boolean parameter.
core/embed/trezorhal/image.h (3)
26-26: Added necessary includeIncluding sdram.h provides access to FMC_SDRAM_FIRMWARE_P2_LEN used below.
43-47: Improved size definition macrosSplitting into FIRMWARE_IMAGE_INNER_SIZE and FIRMWARE_IMAGE_MAXSIZE improves clarity.
64-64: Replaced reserved field with meaningful variableChanged from reserved bytes to a typed hash_block field, matching firmware.py changes.
3 participants
Summary by CodeRabbit
Bug Fixes
New Features
Refactor