#13944: Redesign memory packing API #15980
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nathan-TT
requested review from
abhullar-tt,
pgkeller,
aliuTT,
tt-aho,
tt-dma,
tt-asaigal,
ubcheema,
davorchap and
cfjchu
as code owners
pgkeller
approved these changes
Dec 13, 2024
SeanNijjar
added a commit
that referenced
this pull request
Dec 17, 2024
nathan-TT
added a commit
that referenced
this pull request
Dec 17, 2024
This reverts commit 47cb524.
nathan-TT
added a commit
that referenced
this pull request
Dec 18, 2024
### Ticket n/A ### Problem description This assert was added when kernel data packing was implemented (data load address immediately after text). But that only worked for (non-idle) erisc kernels by accident due to another bug an obsolete workaround. I fixed this with ``` * 434bd8e 2024-12-13 | #13944: Redesign memory packing API (#15980) ``` By not packing such erisc kernels. This worked in production builds because asserts are disabled, so I didn't fall over this problem. ### What's changed Remove asserts, update comments to reflect reality. working on updating non-idle erisc to allow packing (perhaps CI optimized builds should enable asserts, Remember CMAKE_BUILD_TYPE=RelWithDebInfo doesn't do that) ### Checklist - [Yes] Post commit CI passes - [ ] Blackhole Post commit (if applicable) - [ ] Model regression CI testing passes (if applicable) - [ ] Device performance regression CI testing passes (if applicable) - [ ] New/Existing tests provide coverage for changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#13944
Problem description
Packing ELF segments together for deployment relied on HAL information. That information is present in the ELF itself and no HAL involvement is necessary.
In developing this patch I discovered a bug that was mitigated by a different bug and obsolete code predating the use of ELF segments. Specifically for erisc kernel deployments packing was requested, but text memory range was incorrect. This resulted in the data segment being considered text, and the obsolete code inserted padding between the real text and the data, resulting in a deployable image, but one that was larger than necessary and would clobber any memory objects placed in that padding area.
What's changed
*) Linker scripts augmented to record the Load Memory Address(LMA) of data segments -- this is used to ensure packing is consistent
*) ELF loader augmented to record the LMA. (Other cleanups also implemented)
*) Replacement of the separate
PackSpansandRelocateenums with a single Loading enum offering {DISCRETE, CONTIGUOUS, CONTIGUOUS_XIP} alternatives. (Removing an unrequired DISCRETE_XIP variant.). While the original enums are orthogonal, their use is not -- and the above mentioned bug occurred by not paying attention to how they are related.*) Removal of
core_type_idx, processor_class_idx, processor_type_idxarguments toget_risc_binary*) Removal of
Memory::pack_data_into_textand direct implementation of that functionality during the conversion from ELF. (This routine contained the obsolete workaround and was given incorrect text and data base addresses for wormhole_b0 N300)Checklist