-
Notifications
You must be signed in to change notification settings - Fork 345
Access memory through the scope of current privilege level #386
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
Conversation
This seems like it would work for a lot of use cases. The main one where it would fall down is if there's no page table entry for a given virtual address. Fixing that gets really hairy though, so it makes sense to have something like this for now. I have a few comments so far:
|
Thanks for the feedback!
Is there anything to add? And is it okay to leave out targets with a smaller program buffer for now or how would you suggest to proceed? |
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.
Thanks for making these changes. This is looking much better. Having pondered the change some more, I have more requests, however.
I'm not comfortable with the user-visible behavior silently changing (whether addresses are interpreted as virtual/physical) depending on progbufsize which is something users have no control over, and most won't even be aware of.
We should decide which behavior we want by default, and then warn the user once per session (using LOG_INFO) if what they requested isn't possible. I'm also a bit concerned that there might be people who are unknowingly relying on the existing behavior. Can you add a configuration option that lets people pick what they want? Search riscv.c for "prefer_sba" to get an example on how to do that.
I've included your suggestions, in particular a configuration option to enable this feature. By default it is disabled now to prevent any confusion, but to me enabling it looks generally more intuitive. Perhaps changing the default behavior would be something for a release where people expect changes in the ui? Do you have further comments or suggestions? |
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.
Can you resolve the conflicts as well? (Sorry about that.)
Done. Anything else? |
Thanks for making this all nice and clean. |
Hi,
currently, all memory reads and writes using the program buffer are performed on physical addresses. This makes it almost impossible to debug code running in s- or u-mode with the MMU enabled.
I've considered many approaches and to me this seems to be the most reliable one.
Setting
mstatus.mpp = dcsr.priv
ensures that the scope is the same as in our current environment. i.e. if we are entering debug mode from m-mode,mstatus.mprv
won't have any effect and the MMU remains deactivated. If, however,mstatus.mprv
was already set before entering debug mode, virtualization might have been active even though we came from m-mode. Thus, don't interfere withmstatus.mpp
.Next,
dcsr.mprven
is set and cleared right before and after the actual access inside the program buffer. This is to ensure that the enabled MMU doesn't interfere with the functionality of debug modules that rely on memory mapped I/O with the hart.I'm not 100% certain whether the changes are placed properly or if there should be separate functions for this, but I'm open for suggestions!
With best regards,
Nils