-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
arch-x86: Fix TLB Assertion Error on CFLUSH #1080
Conversation
Change-Id: I6004662e7c99f637ba0ddb07d205d1657708e99f
I still think we need to have the assertion |
@@ -254,7 +254,7 @@ class DataTranslation : public BaseMMU::Translation | |||
BaseMMU::Mode mode) | |||
{ | |||
assert(state); | |||
assert(mode == state->mode); | |||
assert(mode == state->mode || req->isCacheClean()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a little uncomfortable with this change. However, if it is what makes the most sense (e.g., this can't be fixed by changing the original Mode
, then I'm good with this change.
Thanks for the contribution!
Is there a plan to merge this to v24.0 release? |
Hi, I got to see this PR randomly as it is tagged as x86 but it actually affects every ISA as it touches the generic src/cpu/translation.hh code. I believe the current solution, while it fixes the problem for X86 is a hack for all other ISAs and I don't see the reason why this couldn't be fixed properly in the x86 code. As far as I understood from a quick look at the issue, the problem arises as CFLUSH is treated as a read for permission checking:
Well it shouldn't be hard to amend the permission checking logic in the TLB to treat the CacheClean as a corner case for BaseMMU::Write requests. I believe we do something similar in the ArmMMU code. I propose to revert this PR until things are done properly |
@giactra, we apologize for the oversight. I have created the revert PR. |
No worries, thanks for handling this so quickly! |
This reverts commit dad5c7b. Change-Id: I91543358703cfd6dfb300c3e79582987c0714a37
Reverts #1080 as it is not a good fix.
Fixed the assertion statement in the cpu's translation.hh file so that it doesn't fail the assertion if the cache is clean.
I compile this c code to
test
And run it with this script
./build/X86/gem5.opt configs/learning_gem5/part1/two_level.py ./test
In order to verify that it no longer fails the assertion check.
GitHub Issue: #862
Change-Id: I6004662e7c99f637ba0ddb07d205d1657708e99f