Skip to content

arch/riscv: support CONFIG_CODE_DATA_RELOCATION #49262

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
Merged
merged 4 commits into from
Aug 24, 2022
Merged

arch/riscv: support CONFIG_CODE_DATA_RELOCATION #49262

merged 4 commits into from
Aug 24, 2022

Conversation

@tari
Copy link
Contributor

@tari tari commented Aug 19, 2022 •
edited
Loading

This implements support for relocating code to chosen memory regions via
the zephyr_code_relocate CMake function for RISC-V, where it was
previously implemented only for ARM.

@tari tari requested review from kgugala and pgielda as code owners August 19, 2022 05:36
@zephyrbot zephyrbot added the area: RISCV RISCV Architecture (32-bit & 64-bit) label Aug 19, 2022
@tari tari requested a review from dcpleung as a code owner August 19, 2022 06:41
carlocaione
carlocaione requested changes Aug 19, 2022
View reviewed changes
Copy link
Contributor

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

We need also to make this test-able by tests/application_development/code_relocation/

@fkokosinski
Copy link
Member

fkokosinski commented Aug 19, 2022

Hi @tari, thanks for your contribution. Did you test these changes on any of the RISC-V boards?

@tari
Copy link
Contributor Author

tari commented Aug 22, 2022

Hi @tari, thanks for your contribution. Did you test these changes on any of the RISC-V boards?

Yes, I'm testing on it8xxx2.

@tari tari force-pushed the riscv-code-relocation branch from 4b4195f to a99a068 Compare August 22, 2022 01:30
@zephyrbot zephyrbot added area: ARM ARM (32-bit) Architecture platform: GD32 GigaDevice platform: ITE ITE labels Aug 22, 2022
@zephyrbot zephyrbot removed the request for review from soburi August 22, 2022 01:30
@zephyrbot zephyrbot added the area: Toolchains Toolchains label Aug 23, 2022
@zephyrbot zephyrbot requested review from dcpleung, fkokosinski, galak and jeremybettis and removed request for bbolen, enjiamai, stephanosio and tejlmand August 23, 2022 05:17
@tari
Copy link
Contributor Author

tari commented Aug 23, 2022

We need also to make this test-able by tests/application_development/code_relocation/

The test (and gen_relocate_app) had a few baked-in ARM assumptions that I've had to fix, but I've added a test case that passes for RISC-V. I had to make it a new test case since adding a new section requires some unportable configuration.

I'm not very happy with the way target_relocation.cmake currently assumes section names based on the target architecture, so ideas there are welcome. The best alternative I see is changing semantics slightly to drop the data-in-default-RAM optimization so we can remove any references to particular memory regions from the script.

tari added 3 commits August 23, 2022 16:27
@tari
build: don't assume .symtab section type
a874552 
Only the elftools SymbolTableSection section type provides an
iter_symbols() method, and compilers are free to emit sections that
include the substring '.symtab' in their name which causes errors if
gen_relocate_app examines only the section name. Instead check whether a
section is a symbol table to skip attempting to inspect sections that
are not actually symbol tables.

Signed-off-by: Peter Marheine <[email protected]>
@tari
build: don't assume FLASH is the ROM region
d0124cc 
Most ARM platforms name their ROM region `FLASH`, but this assumption is
not portable. Have gen_relocate_app refer to ROMABLE_REGION instead of
hard-coding the platform-specific memory region name.

Signed-off-by: Peter Marheine <[email protected]>
@tari
arch: move CODE_DATA_RELOCATION to top level
35df765 
Support for CODE_DATA_RELOCATION is not inherently limited to ARM, so
move the Kconfig definition to top-level so it can be used by other
architectures. Since support is opt-in (requiring linker script
support), add a helper symbol enabled by architecture config that gates
whether CODE_DATA_RELOCATION is available instead of listing all
supported systems inline.

Signed-off-by: Peter Marheine <[email protected]>
carlocaione
carlocaione reviewed Aug 23, 2022
View reviewed changes
carlocaione
carlocaione requested changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

The only blocking comment is the copyright. The other comment can be addressed here or in another PR.

@tari
arch/riscv: support CONFIG_CODE_DATA_RELOCATION
2ef7b3a 
This implements support for relocating code to chosen memory regions via
the `zephyr_code_relocate` CMake function for RISC-V SoCs. ARM-specific
assumptions that were made by gen_relocate_app.py need to be corrected,
in particular not assuming any particular name for the default RAM
section (which is 'SRAM' for most ARM pltaforms) and not assuming 32-bit
pointers (so the test works on RV64).

Signed-off-by: Peter Marheine <[email protected]>
@tari tari force-pushed the riscv-code-relocation branch from 1f73b44 to 2ef7b3a Compare August 23, 2022 23:26
@tari tari requested a review from tejlmand as a code owner August 23, 2022 23:26
@zephyrbot zephyrbot requested review from bbolen, enjiamai and stephanosio and removed request for dcpleung, fkokosinski, galak and jeremybettis August 23, 2022 23:27
@tari tari mentioned this pull request Aug 24, 2022
Merged
@carlescufi carlescufi merged commit d400b81 into zephyrproject-rtos:main Aug 24, 2022
edersondisouza
edersondisouza reviewed Aug 24, 2022
View reviewed changes
tests/application_development/code_relocation/linker_riscv_qemu_sram2.ld
* @file
* @brief Linker command/script file
*
* Linker script for the Cortex-M platforms.
Copy link
Contributor

@edersondisouza edersondisouza Aug 24, 2022

Choose a reason for hiding this comment

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

Cortex-M?

Copy link
Contributor Author

@tari tari Aug 24, 2022

Choose a reason for hiding this comment

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

Hah, oops. I've added a commit to #49438 that fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edersondisouza edersondisouza edersondisouza left review comments

@carlocaione carlocaione carlocaione approved these changes

@fkokosinski fkokosinski fkokosinski approved these changes

@mgielda mgielda Awaiting requested review from mgielda

@nashif nashif Awaiting requested review from nashif

@katsuster katsuster Awaiting requested review from katsuster

@kgugala kgugala Awaiting requested review from kgugala

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@pgielda pgielda Awaiting requested review from pgielda

@soburi soburi Awaiting requested review from soburi

@tgorochowik tgorochowik Awaiting requested review from tgorochowik

@microbuilder microbuilder Awaiting requested review from microbuilder

@povergoing povergoing Awaiting requested review from povergoing

@tejlmand tejlmand Awaiting requested review from tejlmand

@bbolen bbolen Awaiting requested review from bbolen

@enjiamai enjiamai Awaiting requested review from enjiamai

@stephanosio stephanosio Awaiting requested review from stephanosio

Assignees

@kgugala kgugala

Labels

area: ARM ARM (32-bit) Architecture area: Build System area: RISCV RISCV Architecture (32-bit & 64-bit) area: Toolchains Toolchains area: Userspace Userspace platform: GD32 GigaDevice platform: ITE ITE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tari @fkokosinski @carlocaione @edersondisouza @kgugala @carlescufi @zephyrbot