Skip to content

Use tbf_protected_region_size to indicate intended load address#476

Merged
bradjc merged 4 commits intotock:masterfrom
lschuermann:otdev/elf2tab-tbf-header-sym
Aug 1, 2023
Merged

Use tbf_protected_region_size to indicate intended load address#476
bradjc merged 4 commits intotock:masterfrom
lschuermann:otdev/elf2tab-tbf-header-sym

Conversation

@lschuermann
Copy link
Copy Markdown
Member

This removes the special .tbf_header section previously created by libtock-rs, in favor of setting the tbf_protected_region_size symbol and shifting the application's FLASH section by TBF_HEADER_SIZE.

While adding a .tbf_header section is a convenient way to reserve space for elf2tab to insert a TBF header, it causes issues with recent versions of elf2tab and certain Rust toolchains:

  • depending on factors such as the Rust toolchain version and the precise FLASH address, the linker may choose to place the RAM section before the FLASH section in the generated ELF file. This is perfectly legal for the linker to do: the intended load address for those sections are still maintained and included in the appropriate section headers. Furthermore, internal references in the ELF file (such as the addresses in the rt_header) will rely on this ordering of sections -- meaning that elf2tab is not allowed to rearrange them. However, when including a .tbf_header section at the start of the FLASH section, this may then not actually be located at the start of the ELF binary itself.

  • it seems that elf2tab no longer considers the .tbf_header region to not be included in the actual TBF binary (I assume this wasn't always the case, otherwise this would have never worked?). Instead, the .tbf_header section is included and loaded in the final TBF, and as a result the application is offset by an incorrect amount (twice the TBF_HEADER_SIZE)

    I'm not sure whether this issue is actually an elf2tab bug in that it seemingly considers NOBITS sections for loading, but those sections are also included in the ELF file, so I assume that elf2tab works correctly here.

This change tries to retain the previous semantics (developers only specify the intended load address in their platform linker scripts), while using a more reliable mechanism for elf2tab to prefix the generated binary with TBF headers and a protected region that, when prepended, aligns to this intended load address.

Most importantly, we exclude the TBF header from any of the app's MEMORY sections, giving the linker free reign over their layout. A special symbol, tbf_protected_region_size, communicates to elf2tab the intended protected region size, which matches the FLASH-section's ORIGIN of $INTENDED_LOAD_ADDRESS + TBF_HEADER.

In essence, this adjusts libtock-rs to behave like libtock-c (where the TBF header is also not reserved in the binary itself), while retaining libtock-rs' more elegant semantics. With this change, as long as elf2tab can reliably detect the app's fixed flash address, RAM address and .start symbol offset, the ELF's particular layout should now be irrelevant for elf2tab.

Relevant elf2tab PR: tock/elf2tab#70

@lschuermann lschuermann requested a review from jrvanwhy June 28, 2023 14:33
@lschuermann lschuermann force-pushed the otdev/elf2tab-tbf-header-sym branch from 7b4ab7c to d9a304f Compare June 28, 2023 14:57
Copy link
Copy Markdown
Collaborator

@jrvanwhy jrvanwhy left a comment

Choose a reason for hiding this comment

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

There is a lot of redundant calculation/specification in this PR. Can we move the MEMORY declaration into runtime/libtock_layout.ld, so that the per-board linker scripts aren't all doing the TBF_HEADER_SIZE math?

In case my previous paragraph was hard to understand, I'm suggesting that runtime/layouts/apollo3.ld should look something like:

TBF_HEADER_SIZE = 0x60;
FLASH_START = 0x40000;
FLASH_LENGTH = 0x60000;
RAM_START = 0x10002000;
RAM_LENGTH = 0x2000;

INCLUDE libtock_layout.ld

and MEMORY {} would be in libtock_layout.ld.

@lschuermann
Copy link
Copy Markdown
Member Author

Yes, that's most certainly possible and I agree that it's probably cleaner. I just didn't want the initial changeset to contain any more changes than strictly necessary, and I guess one could bring the concern that not having the MEMORY declaration in the board-specific linker files makes this infrastructure slightly less transparent. Will update accordingly.

Comment thread runtime/libtock_layout.ld
Comment thread runtime/layouts/apollo3.ld Outdated
@lschuermann lschuermann force-pushed the otdev/elf2tab-tbf-header-sym branch from ca21867 to 984e238 Compare July 3, 2023 14:15
jrvanwhy
jrvanwhy previously approved these changes Jul 10, 2023
@jrvanwhy jrvanwhy added the upkeep Indicates a PR is upkeep as defined by the code review policy. label Jul 10, 2023
@jrvanwhy
Copy link
Copy Markdown
Collaborator

bors r+

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jul 10, 2023

Merge conflict.

This removes the special `.tbf_header` section previously created by
`libtock-rs`, in favor of setting the `tbf_protected_region_size`
symbol and shifting the application's FLASH section by
`TBF_HEADER_SIZE`.

While adding a `.tbf_header` section is a convenient way to reserve
space for elf2tab to insert a TBF header, it causes issues with recent
versions of elf2tab and certain Rust toolchains:

- it seems that elf2tab no longer considers this region to not be
  included in the actual TBF binary. Instead, it is included and loaded
  in the final TBF, and as a result the application is offset by an
  incorrect amount (twice the `TBF_HEADER_SIZE`).

- depending on factors such as the Rust toolchain version and the
  precise FLASH address, the linker may choose to place the RAM
  section before the FLASH section in the generated ELF file. This is
  perfectly legal for the linker to do: the intended load address for
  those sections are still maintained and included in the appropriate
  section headers. Furthermore, internal references in the ELF
  file (such as the addresses in the `rt_header`) will rely on this
  ordering of sections -- meaning that elf2tab is not allowed to
  rearrange them. However, when including a `.tbf_header` section at
  the start of the FLASH section, this may then not actually be
  located at the start of the ELF binary itself.

This change tries to retain the previous semantics (developers only
specify the intended load address in their platform linker scripts),
while using a more reliable mechanism for elf2tab to prefix the
generated binary with TBF headers and a protected region that, when
prepended, aligns to this intended load address.

Most importantly, we exclude the TBF header from any of the app's
MEMORY sections, giving the linker free reign over their layout. A
special symbol, `tbf_protected_region_size`, communicates to elf2tab
the intended protected region size, which matches the FLASH-section's
ORIGIN of `$INTENDED_LOAD_ADDRESS + TBF_HEADER`.

With this change, as long as elf2tab can reliably detect the app's
fixed flash address, RAM address and `.start` symbol offset, the ELF's
particular layout should now be irrelevant for elf2tab.
@lschuermann lschuermann force-pushed the otdev/elf2tab-tbf-header-sym branch from c282804 to 6c3e727 Compare July 13, 2023 15:21
@lschuermann
Copy link
Copy Markdown
Member Author

Rebased. This might be a good test for the GitHub merge queue integration, so let me set that up first and perhaps then we can try to merge this?

@jrvanwhy
Copy link
Copy Markdown
Collaborator

Looks like one (or more) of our boards don't pass the new alignment check. I think you should just fix them manually until CI passes.

Rebased. This might be a good test for the GitHub merge queue integration, so let me set that up first and perhaps then we can try to merge this?

SGTM

Leon Schuermann added 2 commits July 13, 2023 12:52
This is to remove the requirement for all board-specific layouts to
perform the FLASH start address TBF_HEADER_SIZE offset calculation.
@lschuermann lschuermann force-pushed the otdev/elf2tab-tbf-header-sym branch from 6c3e727 to 1554a07 Compare July 13, 2023 16:52
@lschuermann
Copy link
Copy Markdown
Member Author

Looks like one (or more) of our boards don't pass the new alignment check. I think you should just fix them manually until CI passes.

Right, thanks for the heads up. The alignment checks still used the ORIGIN(SRAM) function whereas this PR now changes this to be SRAM_START. Hopefully CI is more happy now. 🤞

@lschuermann
Copy link
Copy Markdown
Member Author

Fixed the remaining incorrect RAM start offsets. Coincidentally, this issues does not seem to affect ARM binaries (the ELFs look good), but must've been present in quite some RISC-V boards, as they had start addresses not aligned to a 4kB boundary, and I confirmed the generated ELFs to be broken. Maybe this used to work on some older llvm-ld linker version or with the previous elf2tab version (before the ELF parsing rewrite).

Copy link
Copy Markdown
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

bors d+

I think this one should just go in via the old bors route, as the update to CI infrastructure seems to have some cascading effects that are more complicated (#481) and unrelated to this PR.

I don't track libtock-rs thoroughly enough to just do the merge here, but @lschuermann I think it would be appropriate for you to merge this at this stage looking at history here.

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jul 31, 2023

✌️ lschuermann can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lschuermann
Copy link
Copy Markdown
Member Author

Don't think that Bors will work given the elf2tab issues on the current Rust tool chain, but here goes nothing:

bors r+

If this fails, will merge manually.

bors Bot added a commit that referenced this pull request Jul 31, 2023
476: Use `tbf_protected_region_size` to indicate intended load address r=lschuermann a=lschuermann

This removes the special `.tbf_header` section previously created by `libtock-rs`, in favor of setting the `tbf_protected_region_size` symbol and shifting the application's FLASH section by `TBF_HEADER_SIZE`.

While adding a `.tbf_header` section is a convenient way to reserve space for elf2tab to insert a TBF header, it causes issues with recent versions of elf2tab and certain Rust toolchains:

- depending on factors such as the Rust toolchain version and the precise FLASH address, the linker may choose to place the RAM section before the FLASH section in the generated ELF file. This is perfectly legal for the linker to do: the intended load address for those sections are still maintained and included in the appropriate section headers. Furthermore, internal references in the ELF file (such as the addresses in the `rt_header`) will rely on this ordering of sections -- meaning that elf2tab is not allowed to rearrange them. However, when including a `.tbf_header` section at the start of the FLASH section, this may then not actually be located at the start of the ELF binary itself.

- it seems that elf2tab no longer considers the `.tbf_header` region to not be included in the actual TBF binary (I assume this wasn't always the case, otherwise this would have never worked?). Instead, the `.tbf_header` section is included and loaded in the final TBF, and as a result the application is offset by an incorrect amount (twice the `TBF_HEADER_SIZE`)

  I'm not sure whether this issue is actually an elf2tab bug in that it seemingly considers `NOBITS` sections for loading, but those sections are also included in the ELF file, so I assume that elf2tab works correctly here.

This change tries to retain the previous semantics (developers only specify the intended load address in their platform linker scripts), while using a more reliable mechanism for elf2tab to prefix the generated binary with TBF headers and a protected region that, when prepended, aligns to this intended load address.

Most importantly, we exclude the TBF header from any of the app's MEMORY sections, giving the linker free reign over their layout. A special symbol, `tbf_protected_region_size`, communicates to elf2tab the intended protected region size, which matches the FLASH-section's ORIGIN of `$INTENDED_LOAD_ADDRESS + TBF_HEADER`.

In essence, this adjusts `libtock-rs` to behave like `libtock-c` (where the TBF header is also not reserved in the binary itself), while retaining `libtock-rs`' more elegant semantics. With this change, as long as elf2tab can reliably detect the app's fixed flash address, RAM address and `.start` symbol offset, the ELF's particular layout should now be irrelevant for elf2tab.

Relevant elf2tab PR: tock/elf2tab#70

Co-authored-by: Leon Schuermann <leons@opentitan.org>
@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jul 31, 2023

Build failed:

@bradjc bradjc merged commit bdce67b into tock:master Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upkeep Indicates a PR is upkeep as defined by the code review policy.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants