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.

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

Fix memory protection in regression tests #481

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

piotro888
Copy link
Member

@piotro888 piotro888 commented Oct 22, 2023

I found a bug in #466 - all memory segments were set to all RWX flags. After fixing that, regression tests started to fail.

First bug is simple - wrong flag on data memory (only in cocotb backend)
Second bug is problematic. In rv32uc-rvc test there is a data symbol and not .data (!) section that is embedded into instruction memory. This section is written by rvc tests, but not flagged as writeable in ELF file. I don't have currently solution to it (ignore for this specific test, or maybe data section can be added to linker script?)

@piotro888
Copy link
Member Author

This data section is here. Does someone know how this can be added to linker script, so instruction LOAD segment would be also writeable?

@piotro888 piotro888 added bug Something isn't working tests Tests and testbenches (not infrastructure) labels Oct 22, 2023
@piotro888
Copy link
Member Author

I decided to manually exclude tests from write protection. It will be also useful for tests with self-modifying code, which can't be detected for linker script.

@piotro888 piotro888 marked this pull request as ready for review October 22, 2023 11:15
@piotro888
Copy link
Member Author

@tilk Can you change the https://github.com/orgs/kuznia-rdzeni/packages/container/package/riscv-toolchain package visibility from internal, please? If we are working on forks, it can't be pulled for manual run of CI benchmark workflow on forked branch.

@tilk
Copy link
Member

tilk commented Oct 22, 2023

Done.

@tilk
Copy link
Member

tilk commented Oct 22, 2023

Second bug is problematic. In rv32uc-rvc test there is a data symbol and not .data (!) section that is embedded into instruction memory.

If I understand correctly, this test has no .data section, and the data symbol is just for padding in the code section. I don't see why this should cause a problem. Unfortunately I was mistaken: there are store instructions to the region. This is not the only test with such behavior: there is also rv32ui-fence_i, which we don't run because we still don't support FENCEI. These two should probably be special-cased somewhere, as I believe a write-protected code section is a good sanity check for the other tests.

@Arusekk
Copy link
Contributor

Arusekk commented Oct 23, 2023

May be worth reporting upstream to the tests repo.

@tilk tilk merged commit 122ac31 into kuznia-rdzeni:master Oct 23, 2023
5 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 23, 2023
@piotro888 piotro888 deleted the mem-segments-fix branch October 28, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests Tests and testbenches (not infrastructure)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants