8000 boards: add a new linker script that includes the .apps section by bradjc · Pull Request #4158 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

boards: add a new linker script that includes the .apps section #4158

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

Closed
wants to merge 5 commits into from

Conversation

bradjc
Copy link
Contributor
@bradjc bradjc commented Aug 29, 2024

Pull Request Overview

The .apps section is used as a placeholder for replacing with a compiled app binary to create a binary that includes both the kernel in the app. This is used by some boards and avoids needing to flash several times.

As a side effect, if the .elf with .apps is used to flash the kernel, this also (effectively) removes all installed apps.

This PR makes this optional rather than the default. Boards that want this can use the kernel_layout_apps.ld linker script.

For the stm32 discovery boards I removed the docs from the readme because those have tockloader support.

I also had to expand the build.rs code to handle relative imports outside or the board's directory. I don't think it's the prettiest code.

Testing Strategy

Installing the microbit kernel and verifying it does not erase my apps any more.

TODO or Help Wanted

Nothing

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

bradjc added 4 commits August 29, 2024 13:34
This gives boards the flexibility to choose whether they want a .apps
section to end up in the elf or not.

The downside to leaving the .apps section in the elf is that loading the
elf directly erases all apps.
No need for the old .apps documentation.
This requires using the kernel_layout_apps.ld linker script.
Now works on recursive relative imports.
@github-actions github-actions bot added the WG-OpenTitan In the purview of the OpenTitan working group. label Aug 29, 2024
brghena
brghena previously approved these changes Aug 30, 2024
Copy link
Contributor
@brghena brghena left a comment

Choose a reason for hiding this comment

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

This makes sense. It's exactly the kind of little quality-of-life thing that is easy to not notice, but reduces friction for using Tock.

The changes look good to me.

Comment on lines +29 to +40
/* Include placeholder bytes in this section so that the linker
* includes a segment for it. Otherwise the section will be empty and
* the linker will ignore it when defining the segments.
* If less then 4 bytes, some linkers set this section to size 0
* and openocd fails to write it.
*
* An issue has been submitted https://github.com/raspberrypi/openocd/issues/25
*/
BYTE(0xFF)
BYTE(0xFF)
BYTE(0xFF)
BYTE(0xFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexandruradovici Do you know whether this is still necessary?

It looks like the issue was closed a while ago: raspberrypi/openocd#25

I guess we could just leave these four bytes as-is. Previously there was a single BYTE(0) here, so this isn't worse.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just removing the link to the issue is the right thing to do at this point.

For relative imports to work in linker scripts we must include all of
the locations of the linker scripts in the linker search path. Since we
are already tracing through linker scripts we can do this fairly easily.
@ppannuto
Copy link
Member
ppannuto commented Sep 5, 2024

Instead of maintaining two linker files, I believe we can just modify the elf instead? i.e.,

arm-none-eabi-objcopy --set-section-flags .apps=NOLOAD test.elf

should prevent the section from being overwritten.

Or, perhaps more in line with the common case for tock, we mark the .apps section as NOLOAD in the linker script, and for the boards that integrate apps with the kernel image, add arm-none-eabi-objcopy --set-section-flags .apps=LOAD test.elf

@hudson-ayers
Copy link
Contributor

Instead of maintaining two linker files, I believe we can just modify the elf instead? i.e.,

arm-none-eabi-objcopy --set-section-flags .apps=NOLOAD test.elf

should prevent the section from being overwritten.

Or, perhaps more in line with the common case for tock, we mark the .apps section as NOLOAD in the linker script, and for the boards that integrate apps with the kernel image, add arm-none-eabi-objcopy --set-section-flags .apps=LOAD test.elf

Is there a good way to do this without making builds only work if you build using make? My understanding was that We want kernels compiled using cargo build to work the same way as kernels compiled using that boards make target

@bradjc
Copy link
Contributor Author
bradjc commented Sep 10, 2024

Or, perhaps more in line with the common case for tock, we mark the .apps section as NOLOAD in the linker script, and for the boards that integrate apps with the kernel image, add arm-none-eabi-objcopy --set-section-flags .apps=LOAD test.elf

Have you known about this the whole time!?!?! I feel like you've been holding out! Why didn't we do this from the beginning.

@ppannuto
Copy link
Member

Hah, no and yes? I was reviewing this, and I actually started by re-investigating the "how do to a conditional in a linker script" rabbit hole [where the consensus solution is apparently to run the C preprocessor over your linker script via make 😳 ]. Then I thought, wait, there has to be a way to just edit the flags of an elf section, a quick search or three later and here we are :)


Is there a good way to do this without making builds only work if you build using make?

Without reviewing the machinery, don't we already need other "extra commands" to set up the binary image for apps that will be linked for the unified flash case? That's already a bit of a hack that I think we'd rather purge from the tree wholly eventually generally...

@bradjc
Copy link
Contributor Author
bradjc commented Sep 12, 2024

Alternative in #4169.

@bradjc bradjc closed this Sep 19, 2024
@bradjc bradjc deleted the kernel-linker-apps branch October 10, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WG-OpenTitan In the purview of the OpenTitan working group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0