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: Select hart in update_dcsr() #856

Merged
merged 1 commit into from
Jun 6, 2023
Merged

target/riscv: Select hart in update_dcsr() #856

merged 1 commit into from
Jun 6, 2023

Conversation

timsifive
Copy link
Collaborator

Otherwise we may end up modifying DCSR of a different hart.

Fixes a bug introduced in f089815.

Change-Id: I39bde21a1444623ed150f2b3d504b9318b9d6191

GregSavin
GregSavin previously approved these changes Jun 2, 2023
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

I have checked this only visually. Several notes:

1
I think this is a wider issue, which I'd like to address in the future: Virtually all functions accept the target as the argument but it is not very clear

  • a) which of them will select the corresponding hart
  • b) which of them simply assume that the caller (or some grand-caller) has selected the target already, and it can be directly used

For the functions in the b) category, my preference would be to add an assertion that checks the number of the last selected hart. I assume that would have caught this bug as well.

2
The commit message says ... in update_dcsr() but the change is actually made in register_read_direct().

3
register_write_direct() does not contain the call to dm013_select_target but register_read_direct() now has it. This asymmetry does not look right. It seems to me that adding the select_target call to update_dcsr() would make more sense.

@timsifive
Copy link
Collaborator Author

  1. Agreed, but I don't think it's that big a problem. Just adding the occasional call to dm013_select_target() fixes it, and that seems better than an assert().
  2. I'll move the change back to update_dcsr() then.

Otherwise we may end up modifying DCSR of a different hart than
intended.

Change-Id: I39bde21a1444623ed150f2b3d504b9318b9d6191
Signed-off-by: Tim Newsome <tim@sifive.com>
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

Re-checked visually and looks all right. Thanks.

@timsifive timsifive merged commit e0dd44a into riscv Jun 6, 2023
5 checks passed
@timsifive timsifive deleted the select_hart branch June 6, 2023 15:56
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

3 participants