Skip to content

Implement mcontrol6.hit #1257

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

Merged

Conversation

YenHaoChen
Copy link
Collaborator

@YenHaoChen YenHaoChen commented Feb 21, 2023

The debug spec revised the mcontrol6.hit in riscv/riscv-debug-spec#795. This PR implements the revised behavior.

@YenHaoChen
Copy link
Collaborator Author

The hit is separated into hit0 and hit1, i.e., hit={hit1,hit0}. I need help finding a more elegant implementation for the LSB and MSB operation on the hit field.

tdata1 = set_field(tdata1, CSR_MCONTROL6_HIT1, hit & 2); // MSB of 2-bit field
...
tdata1 = set_field(tdata1, CSR_MCONTROL6_HIT0, hit & 1); // LSB of 2-bit field
...
hit = hit_t(2 * get_field(val, CSR_MCONTROL6_HIT1) + get_field(val, CSR_MCONTROL6_HIT0)); // 2-bit field {hit1,hit0}

@aswaterman aswaterman requested a review from timsifive February 21, 2023 06:33
@YenHaoChen YenHaoChen force-pushed the pr-mcontrol6-hit0-hit1 branch from b06a669 to 3f2e4f9 Compare February 21, 2023 06:36
@YenHaoChen
Copy link
Collaborator Author

The CI fails. I passed the compilation on my PC and need help analyzing the failure.

@aswaterman
Copy link
Collaborator

CI fails with:

../install/include/riscv/triggers.h:8:10: fatal error: debug_defines.h: No such file or directory
    8 | #include "debug_defines.h"
      |          ^~~~~~~~~~~~~~~~~
compilation terminated.
Error: Process completed with exit code 1.

@aswaterman
Copy link
Collaborator

This failure mode indicates that a public header is including a private header: triggers.h should not include debug_defines.h. (The alternative is to install debug_defines.h, but we really should not make that part of the API, so I suggest changing triggers.h so it does not need to rely on debug_defines.h.)

@YenHaoChen YenHaoChen force-pushed the pr-mcontrol6-hit0-hit1 branch from 3f2e4f9 to 62c845d Compare February 21, 2023 09:12
@YenHaoChen
Copy link
Collaborator Author

This failure mode indicates that a public header is including a private header: triggers.h should not include debug_defines.h. (The alternative is to install debug_defines.h, but we really should not make that part of the API, so I suggest changing triggers.h so it does not need to rely on debug_defines.h.)

There are two methods to avoid reliance on debug_defines.h.

  1. Define the macros in encoding.h.
  2. Hard-code the values in triggers.h.

I choose the second method for simplicity.

@timsifive
Copy link
Collaborator

Please hold off on merging this. The debug spec is tweaking this change a bit more. It should not have been merged into the spec yet. (The actual change relative to this PR will be minor, so this code will probably all apply.)

@YenHaoChen
Copy link
Collaborator Author

It seems the debug spec has finalized the mcontrol6.hit. I believe this code of the PR provides the desired behavior.

@YenHaoChen YenHaoChen force-pushed the pr-mcontrol6-hit0-hit1 branch from 62c845d to eca9c96 Compare June 9, 2023 04:33
@YenHaoChen YenHaoChen requested a review from timsifive June 9, 2023 04:33
Comment on lines +55 to +58
HIT_FALSE = 0,
HIT_BEFORE = 1,
HIT_AFTER = 2,
HIT_IMMEDIATELY_AFTER = 3
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
HIT_FALSE = 0,
HIT_BEFORE = 1,
HIT_AFTER = 2,
HIT_IMMEDIATELY_AFTER = 3
HIT_FALSE = CSR_MCONTROL6_HIT0_FALSE,
HIT_BEFORE = CSR_MCONTROL6_HIT0_BEFORE,
HIT_AFTER = CSR_MCONTROL6_HIT0_AFTER,
HIT_IMMEDIATELY_AFTER = CSR_MCONTROL6_HIT0_IMMEDIATELY_AFTER

Copy link
Collaborator Author

@YenHaoChen YenHaoChen Jun 12, 2023

Choose a reason for hiding this comment

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

This requires the debug_defines.h to the triggers.h. The #1257 (comment) wants the triggers.h need not to rely on the debug_defines.h.

@timsifive I personally believe the debug_defines.h is a public header. Should we consider installing the debug_defines.h to the CI platform?

@YenHaoChen YenHaoChen force-pushed the pr-mcontrol6-hit0-hit1 branch from eca9c96 to 0557e5e Compare June 12, 2023 00:54
@YenHaoChen YenHaoChen requested a review from timsifive June 12, 2023 01:07
@aswaterman
Copy link
Collaborator

@YenHaoChen Is this ready to merge? You might want to rebase on top of master since almost a year has elapsed.

@YenHaoChen YenHaoChen force-pushed the pr-mcontrol6-hit0-hit1 branch from 0557e5e to 9a1ad1f Compare May 21, 2024 01:51
@YenHaoChen
Copy link
Collaborator Author

@aswaterman
I do a review again today.

  • The mcontrol6 trigger (hit0/hit1) is finalized (frozen) before this commit.
  • The Spike repository has not updated the header file "debug_defines.h" since this PR. Thus, the header file in this PR would work, although it is not the newest one from the debug repository.
  • After automatic rebasing (without a conflict), I could successfully compile the Spike binary on my PC.

I believe this PR is ready to merge.

@rtwfroody
Copy link
Contributor

I don't see the update to tinfo.version to indicate that mcontrol6 has the new hit0/hit1 fields.

@YenHaoChen
Copy link
Collaborator Author

@rtwfroody Thanks for the note. I overlooked the tinfo.version.

Bump the header "debug_defines.h" for the tinfo.version.

Update CSR_MCONTROL6_HIT to CSR_MCONTROL6_HIT0

Include CSR_TINFO_VERSION* macros
…d mcontrol6_t::hit

Add mcontrol_common_t::set_hit()
Avoid using private headers, e.g., debug_defines.h, in triggers.h
@YenHaoChen YenHaoChen force-pushed the pr-mcontrol6-hit0-hit1 branch from a3d37d8 to 7657966 Compare May 22, 2024 01:04
@aswaterman aswaterman merged commit 2cfd539 into riscv-software-src:master May 22, 2024
3 checks passed
@YenHaoChen YenHaoChen deleted the pr-mcontrol6-hit0-hit1 branch May 22, 2024 08:36
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