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.

Sign up for GitHub

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

Jump to bottom

Resize ROM and FW_RAM, add RESETINFO partition inside FW_RAM. #320

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Resize ROM and FW_RAM, add RESETINFO partition inside FW_RAM. #320

merged 1 commit into from
Feb 21, 2025
+309 −77

Conversation

jthornblad
Copy link
Contributor

@jthornblad jthornblad commented Feb 19, 2025 •
edited
Loading

Description

  • Change size of ROM from 6 KB to 8 KB.
  • Change size of FW_RAM, from 2 KB to 4 KB.
  • Add RESETINFO memory partition inside FW_RAM.
  • Add generation of map file.
  • Change CFLAGS from using -O2 to using -Os.
  • Update address ranges for valid access to ROM and FW_RAM.
  • Move stack to be located before data+bss.

Type of change

Please tick any that are relevant to this PR and remove any that aren't.

  • Bugfix (non breaking change which resolve an issue)
  • Feature (non breaking change which adds functionality)
  • Breaking Change (a change which would cause existing functionality to not work as expected)
  • Documentation (a change to documentation)

Submission checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my changes
  • I have tested and verified my changes on target
  • My changes are well written and CI is passing
  • I have squashed my work to relevant commits and rebased on main for linear history
  • I have added a "Co-authored-by: x" if several people contributed, either pair programming or by squashing commits from different authors.
  • I have updated the documentation where relevant (readme, dev.tillitis.se etc.)
  • QEMU is updated to reflect changes

@jthornblad jthornblad force-pushed the mem-resize branch 5 times, most recently from f06b964 to 66e5877 Compare February 19, 2025 16:44
@jthornblad jthornblad marked this pull request as ready for review February 20, 2025 11:39
agren
agren reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@agren agren left a comment

Choose a reason for hiding this comment

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

The FW RAM test in testfw says Testing FW_RAM (takes 15s on hw).... This is no longer true since FW RAM size has increased. Please update the time estimate.

@jthornblad jthornblad force-pushed the mem-resize branch 2 times, most recently from ff4a312 to 96a499c Compare February 20, 2025 16:34
@mchack-work
Copy link
Member

I've tested on real hardware with both firmware and the testfw. I loaded the signer and did the usual: getting public key, signing a message, verifying. Everything works, all tests pass.

I've also tested the firmware.bin created with objcopy -R .stack... and it works, too.

@jthornblad @mchack-work
fpga/fw: Resize ROM and FW_RAM, add RESETINFO partition
3c74f93 
In order to be able to leave data for firmware signalling the
intention with a reset or to leave data for the next app in a chain of
apps, we introduce a part of FW_RAM that can be used to store this
data. In order to do this, we:

- Change size of ROM from 6 KB to 8 KB.
- Change size of FW_RAM, from 2 KB to 4 KB.
- Add RESETINFO memory partition inside FW_RAM.
- Add generation of map file.
- Change CFLAGS from using -O2 to using -Os.
- Update address ranges for valid access to ROM and FW_RAM.
- Move stack to be located before data+bss and the RESETINFO data
  above them. This also means we introduce hardware stack overflow
  protection through the Security Monitor.
- Revise firmware README to the new use of FW_RAM.
@mchack-work mchack-work self-requested a review February 21, 2025 09:46
mchack-work
mchack-work approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@mchack-work mchack-work left a comment

Choose a reason for hiding this comment

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

  • Added docs to firmware about the new use of FW_RAM.
  • Changed commit message (shorter title, with prefix and not ending in "."), added info about hardware-assisted stack overflow protection. Squashed doc updates.

I'm happy with these changes.

@mchack-work mchack-work merged commit 3c74f93 into main Feb 21, 2025
6 checks passed
@mchack-work mchack-work deleted the mem-resize branch February 21, 2025 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agren agren agren left review comments

@mchack-work mchack-work mchack-work approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jthornblad @mchack-work @agren