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

Don't use rd_wdata of instructions that trapped in the reg check #26

Closed
wants to merge 1 commit into from
Closed

Conversation

mtvec
Copy link

@mtvec mtvec commented Jul 17, 2019

The reg check used rd_wdata of any valid instruction as the value for
register_shadow. However, for instructions that trapped, this value is
invalid. This commit ensures that rd_wdata of trapped instructions is
never used for register_shadow.

The reg check used rd_wdata of any valid instruction as the value for
register_shadow. However, for instructions that trapped, this value is
invalid. This commit ensures that rd_wdata of trapped instructions is
never used for register_shadow.
@cliffordwolf
Copy link
Collaborator

Quoting the RVFI spec:

rvfi_rd_addr [..] For an instruction that writes no rd register, this output must always be zero.
rvfi_rd_wdata [..] This output must be zero when rd is zero.

So if for any reason you'd output different (non-zero) values for a trapping instruction then you'd have to gate them accordingly on the core side.

But for most cores you'd just hook it into the pipeline in a way so that rvfi_rd_addr=0 and rvfi_rd_wdata=0 whenever there's no writeback, regardless of the reason. (For example, branch instruction also have no writeback.) Otherwise, if the check would test rvfi_trap, you'd tie the instruction decoder and a whole bunch of other stuff into that path, dramatically reducing the to-expected proof performance for what's already one of the most challenging formal checks in the riscv-formal suite.

@mtvec
Copy link
Author

mtvec commented Jul 18, 2019

So if for any reason you'd output different (non-zero) values for a trapping instruction then you'd have to gate them accordingly on the core side.

Ok, I'll solve it that way. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants