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: update some macro #883

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Conversation

zqb-all
Copy link
Contributor

@zqb-all zqb-all commented Jul 12, 2023

  1. update RISCV_MAX_HARTS to 2^20 according to SPEC
  2. remove RISCV_MAX_REGISTERS, it's not used anywhere anymore

Change-Id: Iadf0fa1ba3bbe5b9420b8430883e140db87f4f9e

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.

Have you checked that we correctly modify hartselhi where needed, and that the code to select multiple harts will also work with >1024 harts? (I mean by reading the code, making a test case for this would be hard.

@zqb-all
Copy link
Contributor Author

zqb-all commented Jul 13, 2023

Have you checked that we correctly modify hartselhi where needed

All the places that use hartsello now have hartselhi, so I think there's no problem

and that the code to select multiple harts will also work with >1024 harts?

https://github.com/riscv/riscv-openocd/blob/21d21408aa36ccb458ccfaad871f7dd1bb1f731a/src/target/riscv/riscv-013.c#L4450-L4453
The length of this array is variable and I think it need space from stack, if hart count is huge, maybe we should change it to get space from calloc.

@JanMatCodasip
Copy link
Collaborator

zqb-all: The length of this array is variable and I think it need space from stack, if hart count is huge, maybe we should change it to get space from calloc.

Agreed, I think using dynamically allocated memory (malloc, calloc) makes sense in that case.

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.

Overall, this looks all right to me.

As a last suggestion, I'd recommend to put assert(dm->hart_count > 0) to places where hart_count is used and expected to be already filled in.

That's because hart_count is set to -1 initially before the real count is known, but we do not wish to see the negative value anytime after the harts have been examined.

@zqb-all
Copy link
Contributor Author

zqb-all commented Jul 13, 2023

That's because hart_count is set to -1 initially before the real count is known, but we do not wish to see the negative value anytime after the harts have been examined.

There is a check in examine

https://github.com/riscv/riscv-openocd/blob/21d21408aa36ccb458ccfaad871f7dd1bb1f731a/src/target/riscv/riscv-013.c#L1840-L1843

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.

src/target/riscv/riscv.h Outdated Show resolved Hide resolved
src/target/riscv/riscv.h Outdated Show resolved Hide resolved
1. update RISCV_MAX_HARTS to 2^20 according to SPEC
2. remove RISCV_MAX_REGISTERS, it's not used anywhere anymore
3. add parentheses

Change-Id: Iadf0fa1ba3bbe5b9420b8430883e140db87f4f9e
Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>
Change-Id: Id2f1a2568a39eec0a9dd4fe0f155619b11f9d6ba
Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>
@timsifive timsifive merged commit 8032b78 into riscv-collab:riscv Jul 14, 2023
5 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.

None yet

3 participants