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

target/riscv: Fix memory access when MMU is enabled and address couldn't be translated #945

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

kr-sc
Copy link
Contributor

@kr-sc kr-sc commented Oct 25, 2023

Now:

  1. If mmu is disabled, virt2phys succeeded and returns physical address
  2. If mmu is enabled, but translation fails, read/write_memory fails

Change-Id: I312309c660239014b3278cb77cadc5618de8e4de

@kr-sc kr-sc force-pushed the kr-sc/fix-mmu-access-upstream branch from 9687679 to 3122c69 Compare October 25, 2023 11:08
Copy link
Collaborator

@timsifive timsifive left a comment

Choose a reason for hiding this comment

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

Looks good overall. Please return result where that is now easily possible.

LOG_TARGET_ERROR(target, "Failed to read vsatp register.");
int result = riscv_get_register(target, &vsatp, GDB_REGNO_VSATP);
if (result != ERROR_OK) {
LOG_TARGET_ERROR(target, "Failed to read vsatp register. Error: %d", result);
return ERROR_FAIL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return ERROR_FAIL;
return result;

Copy link
Contributor Author

@kr-sc kr-sc Oct 26, 2023

Choose a reason for hiding this comment

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

After internal discussion we decided not to print/return error code here, because it looks like useless for users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is fine.

LOG_TARGET_ERROR(target, "Failed to read hgatp register.");
result = riscv_get_register(target, &hgatp, GDB_REGNO_HGATP);
if (result != ERROR_OK) {
LOG_TARGET_ERROR(target, "Failed to read hgatp register. Error: %d", result);
return ERROR_FAIL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return ERROR_FAIL;
return result;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above

src/target/riscv/riscv.c Show resolved Hide resolved
src/target/riscv/riscv.c Show resolved Hide resolved
@kr-sc kr-sc force-pushed the kr-sc/fix-mmu-access-upstream branch from 3122c69 to 24de36b Compare October 26, 2023 18:57
…n't be translated

Now:
1) If mmu is disabled, virt2phys succeeded and returns physical address
2) If mmu is enbaled, but translation fails, read/write_memory fails

Change-Id: I312309c660239014b3278cb77cadc5618de8e4de
Signed-off-by: Kirill Radkin <[email protected]>
@kr-sc kr-sc force-pushed the kr-sc/fix-mmu-access-upstream branch from 24de36b to 57c3f0d Compare October 30, 2023 13:00
@timsifive timsifive merged commit 20bcd83 into riscv-collab:riscv Nov 2, 2023
4 checks passed
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.

4 participants