Skip to content
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

Startup/shutdown refactoring #4499

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Startup/shutdown refactoring #4499

wants to merge 14 commits into from

Conversation

cepetr
Copy link
Contributor

@cepetr cepetr commented Jan 14, 2025

This PR includes significant refactoring of the startup and shutdown code.

  1. The startup and shutdown code has been simplified and mostly unified across all platforms.
  2. Many linker script symbols have been renamed for greater consistency.
  3. A large portion of assembler code has been rewritten in C.
  4. The RSOD screen is now displayed after all SRAM and other sensitive data are cleared.
  5. An option to wait and reboot after an RSOD is displayed has been added.
  6. Some peripherals (DMA, NVIC) are now reset during handovers - this new behavior must be carefully tested

@cepetr cepetr self-assigned this Jan 14, 2025
Copy link

github-actions bot commented Jan 14, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@cepetr cepetr force-pushed the cepetr/shutdown-refactoring branch 7 times, most recently from 13b756d to e3542c3 Compare January 16, 2025 13:28
@cepetr cepetr added bootloader core Bootloader for Trezor core T2B1 Trezor Safe 3 (F4) T3W1 T2T1 Trezor Model T T3T1 Trezor Safe 5 T3B1 Trezor Safe 3 (U5) labels Jan 16, 2025
@cepetr cepetr force-pushed the cepetr/shutdown-refactoring branch from e3542c3 to d4cd1a3 Compare January 16, 2025 15:27
@cepetr cepetr requested a review from TychoVrahe January 16, 2025 15:30
@cepetr cepetr marked this pull request as ready for review January 16, 2025 15:30
@cepetr cepetr requested a review from prusnak as a code owner January 16, 2025 15:30
@TychoVrahe TychoVrahe removed the request for review from prusnak January 17, 2025 13:30
@TychoVrahe TychoVrahe linked an issue Jan 17, 2025 that may be closed by this pull request

// Adds a new address range into the memory region.
//
// The rabge start and end pointers must be aligned to the 4 bytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo rabge

@@ -41,6 +42,10 @@
#include "zkp_context.h"
#endif

// symbols defined in the linker script
extern uint8_t _stack_section_start;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can move the extens to linker_utils?

// Wait for the user to manually power off the device
secure_shutdown();
// Wait for the user to read the RSOD and then reboots
// (or enters an infinite loop if RSOD_INFINITE_LOOP is defined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the need for this comment indicates that the function name is not ideal

// recommended to call it only during the startup sequence.
void clear_unused_stack(void);

//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfinished comment

void* start;
// block end address (exclusive)
void* end;
} memregion_block_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have similar memory_area_t used in applet code (thought it is start+size instead of end, but its equivalent). Would it make sense to use same structure there, and perhaps the supporting functions for clearing?

#endif

#ifdef BOARDLOADER
memregion_fill(&region, 0xFFFFFFFF); // do we really need this???
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was originally value from TRNG. Whether its actually useful is unclear to me

#endif

#ifdef BOARDLOADER
memregion_fill(&region, 0xFFFFFFFF); // !@# do we really need this???
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was originally value from TRNG

// Fill memory region with a value 32-bit value
void memregion_fill(memregion_t* region, uint32_t value);

#define MEMREGION_ADD_SECTION(region, section_name) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused, along with the memregion_add_range section. Intentionally? If so, do we need to keep it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bootloader core Bootloader for Trezor core T2B1 Trezor Safe 3 (F4) T2T1 Trezor Model T T3B1 Trezor Safe 3 (U5) T3T1 Trezor Safe 5 T3W1
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

consider renaming stack variables
2 participants