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

Script to generate signature from test ELFs #466

Merged
merged 15 commits into from
Nov 2, 2023
Merged

Script to generate signature from test ELFs #466

merged 15 commits into from
Nov 2, 2023

Conversation

piotro888
Copy link
Member

This was is attempt to run https://github.com/riscv-non-isa/riscv-arch-test, with recommend RISCOF framework.

Provided script run_signature runs a compiled ELF file and dumps data from separate memory section called signature to text file (between .begin_signature and .end_signature elf sections, mapped to separate Coreblocks memory segment. Test ends after write to .to_host. Sections are set-up from macros in .h file). This format is used in both in RISCOF / https://github.com/riscv-non-isa/riscv-arch-test and https://github.com/ucb-bar/riscv-torture tests and seem standard. Signature from other simulator like https://github.com/riscv-software-src/riscv-isa-sim (spike) is used for test verification.

Unfortunately RISCOF seem to be a terrible tool to work with, and after few days of experimenting I finally managed to run test suite partially successfully, with many workarounds and for some reason with non-deterministic failing results. I give up and believe that task of compiling and running a test suite is simple enough so we could create our own simpler and better script for our needs.
If someone is interested I can give my configs for running RISCOF.

For now I think that this script for generating signature from ELF can be merged as a separate useful part, and script for running it on a test suite created later.

@piotro888 piotro888 added the tests Tests and testbenches (not infrastructure) label Oct 5, 2023
@piotro888 piotro888 marked this pull request as draft October 5, 2023 19:32
@piotro888
Copy link
Member Author

Regression tests are failing probably because of change in loading of memory segments.
Previously segments were aligned up and down with zeroes to align field in ELF. I removed it because I don't think that's the intended use case.
The problem is when we load exactly section range, then with instruction fetch we can easily go out-of-bounds because we constantly fetch next instructions. I added fixed offset (0x10, probably too small for some tests here), but I don't like it at all. Previous solution is also not fully correct, because if instruction section would end near aligned address, it could also overflow.

@tilk
Copy link
Member

tilk commented Oct 6, 2023

Unfortunately RISCOF seem to be a terrible tool to work with, and after few days of experimenting I finally managed to run test suite partially successfully, with many workarounds and for some reason with non-deterministic failing results. I give up and believe that task of compiling and running a test suite is simple enough so we could create our own simpler and better script for our needs.

I imagined that this could be the case, great that you tried and verified this.

The problem is when we load exactly section range, then with instruction fetch we can easily go out-of-bounds because we constantly fetch next instructions.

This is not true, we are not doing prefetching currently, and even if we did, a memory error should stop prefetching, and not kill the CPU. The issue is with instruction cache: we assume that instructions are always fetched in whole cache lines, and that the memory is laid out so that a memory region should not end in the middle of a cache line.

Edit: I agree that the align field is misused here, but we still need a way to avoid memory errors during instruction cache fetches. A simple hack would be to post-process the segments, and fill the holes so that boundaries between allocated and unallocated memory always occur on cache line boundaries. Or do you have a better idea? This should probably be done in a separate PR.

@piotro888
Copy link
Member Author

Never mind, it looks like test fail mainly because of this fix in flag settings

-   if flags_raw & P_FLAGS.PF_X == flags_raw & P_FLAGS.PF_X:
+   if flags_raw & P_FLAGS.PF_X:

Previously all flags were always set, and now some tests crash because of access to non-executable or read only sections. Needs more investigation, I think I will separate this change to another PR.

A simple hack would be to post-process the segments, and fill the holes so that boundaries between allocated and unallocated memory always occur on cache line boundaries. Or do you have a better idea?

I also think this is the best idea for now

@piotro888
Copy link
Member Author

On the second thought, I think that we can use RISCOF for selecting/generating/compiling tests and maybe even running them on Spike, because this part is already done and it works well. (and there is some configuration of tests based on yaml config files for target, that I'm not sure how is working)
RISCOF generates nice Makefile for running single test cases, so only simple script to run Makefile targets and compare signatures would be needed.

@piotro888 piotro888 marked this pull request as ready for review October 28, 2023 16:17
Comment on lines 163 to 167
def align_down(n: int) -> int:
return (n // alignment) * alignment

align_front = seg_start - align_down(seg_start)
align_back = align_down(seg_end + alignment - 1) - seg_end
Copy link
Member

Choose a reason for hiding this comment

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

There is a clumsily named align_to_power_of_two function in coreblocks.utils.utils, which rounds up, you could use that. Rounding down can be calculated by masking; but maybe this should get an utility function as well? The code you've written is fine, but it's not very readable.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this strategy might fail if multiple segments use the same cache line. As this is unlikely to occur (?), that's a fine workaround for now.

Copy link
Member

@tilk tilk left a comment

Choose a reason for hiding this comment

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

The scripts are somewhat copy-pastey in places, but I don't see a quick fix for that (also, they're just scripts). Left a minor nitpick.

@tilk tilk requested a review from lekcyjna123 November 1, 2023 15:44
arglist = [
"make",
"-C",
parent + "/" if parent else "" + "test/regression/cocotb",
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe os.path.join()?

@tilk tilk merged commit d6b59ee into master Nov 2, 2023
6 checks passed
@tilk tilk deleted the piotro888/riscof branch November 2, 2023 08:21
@tilk tilk mentioned this pull request Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests and testbenches (not infrastructure)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants