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

Add sfence.vma in the disable_pmp function #537

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

lz-bro
Copy link
Contributor

@lz-bro lz-bro commented Feb 4, 2024

Change-Id: Ief659c0e68618f5b08a1eee98bc7df92817b8b51

Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

SFENCE.VMA is illegal on implementations without S-mode. I think this should be conditionalized.

@lz-bro
Copy link
Contributor Author

lz-bro commented Mar 1, 2024

SFENCE.VMA is illegal on implementations without S-mode. I think this should be conditionalized.

PMP changes require an SFENCE.VMA on any hart that implements page-based virtual memory, even if VM is not currently enabled. If the implementation without S-mode, does this mean that PMP registers are optional?

@aswaterman
Copy link
Collaborator

aswaterman commented Mar 2, 2024

As you said, "any hart that implements page-based virtual memory" needs to execute SFENCE.VMA. If S-mode is not implemented, then the hart doesn't implement page-based virtual memory. So there's no contradiction here.

But I did misspeak earlier. It is true that SFENCE.VMA is illegal on implementations without S-mode. But it is also illegal on implementations with S-mode but without virtual memory. So, it needs to be conditionalized on "is S-mode implemented" and "is page-based virtual memory implemented". You can determine the latter property by checking whether satp.MODE is hardwired to 0.

But note that writing satp is illegal unless S-mode is implemented. So you should first check if S-mode is implemented, then check if satp.MODE is writable, and only then should you execute SFENCE.VMA.

@lz-bro lz-bro force-pushed the fix_disable_pmp branch 3 times, most recently from c105f73 to 4407a9c Compare March 2, 2024 06:44
@lz-bro
Copy link
Contributor Author

lz-bro commented Mar 2, 2024

As you said, "any hart that implements page-based virtual memory" needs to execute SFENCE.VMA. If S-mode is not implemented, then the hart doesn't implement page-based virtual memory. So there's no contradiction here.

But I did misspeak earlier. It is true that SFENCE.VMA is illegal on implementations without S-mode. But it is also illegal on implementations with S-mode but without virtual memory. So, it needs to be conditionalized on "is S-mode implemented" and "is page-based virtual memory implemented". You can determine the latter property by checking whether satp.MODE is hardwired to 0.

But note that writing satp is illegal unless S-mode is implemented. So you should first check if S-mode is implemented, then check if satp.MODE is writable, and only then should you execute SFENCE.VMA.

Thank you for your explanation,I've introduced an implements_page_virtual_memory property for the target. But, There is a pipeline failure in Build Spike.

@aswaterman
Copy link
Collaborator

I think we need to do something similar to the following. Can you give it a try and see if you can make it work? https://github.com/riscv-software-src/riscv-isa-sim/pull/1584/files

@lz-bro
Copy link
Contributor Author

lz-bro commented Mar 5, 2024

I think we need to do something similar to the following. Can you give it a try and see if you can make it work? https://github.com/riscv-software-src/riscv-isa-sim/pull/1584/files

Yes, It works now.

@lz-bro
Copy link
Contributor Author

lz-bro commented Nov 12, 2024

@aswaterman Would you kindly review and merge it.

@aswaterman aswaterman merged commit 44a1d2e into riscv-software-src:master Nov 12, 2024
2 checks passed
@lz-bro lz-bro deleted the fix_disable_pmp branch November 12, 2024 03:34
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.

2 participants