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.

Sign up for GitHub

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

Jump to bottom

target/riscv: cleanup trigger setup #878

Merged
merged 1 commit into from
Jul 18, 2023
Merged

target/riscv: cleanup trigger setup #878

merged 1 commit into from
Jul 18, 2023

Conversation

en-sc
Copy link
Collaborator

@en-sc en-sc commented Jul 5, 2023

  • Add a warning when eq trigger is setup and it's behavior is different from other triggers.

  • Make eq trigger's behavior consistent with other triggers in case of length == 1.

  • Fix a bug in setting chained triggers (LT, GT case).

  • Improve logging.

Change-Id: Id1ed0d11971b8ed875afbb979e6c8a8b51dd3818

timsifive
timsifive reviewed Jul 5, 2023
View reviewed changes
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 like you removed matching on 8-bit accesses for 8-bit triggers. Can you share the reasoning behind that? (It makes sense to me, because neither OpenOCD nor gdb support specifying access size on a watchpoint.)

src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Show resolved Hide resolved
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
@en-sc
Copy link
Collaborator Author

en-sc commented Jul 6, 2023

Looks like you removed matching on 8-bit accesses for 8-bit triggers. Can you share the reasoning behind that? (It makes sense to me, because neither OpenOCD nor gdb support specifying access size on a watchpoint.)

The reasoning for matching accesses with any size for 8-bit equality (mcontrol.match = 0) triggers is the following:

In other triggers (mcontrol.match = 1 (napot), or a chain of mcontrol.match = 2, 3 (lt, ge)) access with an address in the inclusive range [trigger->address, trigger->address + trigger->length] matches, even if access_address + access_size > trigger->address + trigger->length.

If s8bit is used (trigger->length == 1), this will not be the case for the resulting trigger.
It will not be triggered on access access_address = trigger->address, access_size = 2.

timsifive
timsifive reviewed Jul 7, 2023
View reviewed changes
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
@en-sc en-sc mentioned this pull request Jul 14, 2023
Merged
@en-sc
Copy link
Collaborator Author

en-sc commented Jul 14, 2023

Please, disregard the mention in riscv-tests/debug PR -- it was a typo.

timsifive
timsifive previously approved these changes Jul 14, 2023
View reviewed changes
JanMatCodasip
JanMatCodasip requested changes Jul 17, 2023
View reviewed changes
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
@en-sc en-sc force-pushed the en-sc/trigg-eq-check branch 2 times, most recently from b5730cc to 5ec632e Compare July 17, 2023 09:26
JanMatCodasip
JanMatCodasip requested changes Jul 17, 2023
View reviewed changes
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.

Thank you for the changes so far. I have one very last set of comments, otherwise this looks fine.

src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Show resolved Hide resolved
@en-sc
target/riscv: cleanup trigger setup
a8f28fd 
* Add a warning when eq trigger is setup and it's behavior is different
from other triggers.

* Make eq trigger's behavior consistent with other triggers in case of
length == 1.

* Fix a bug in setting chained triggers (LT, GT case).

* Improve logging.

Change-Id: Id1ed0d11971b8ed875afbb979e6c8a8b51dd3818
Signed-off-by: Evgeniy Naydanov <[email protected]>
@timsifive timsifive merged commit fb28447 into riscv-collab:riscv Jul 18, 2023
5 checks passed
erhankur
erhankur reviewed Jul 25, 2023
View reviewed changes
src/target/riscv/riscv.c
.tdata2 = trigger->address,
.tdata1_ignore_mask = fields.tdata1_ignore_mask
};
return try_setup_single_match_trigger(target, trigger, eq);
ret = try_setup_single_match_trigger(target, trigger, eq);
if (ret != ERROR_OK)
Copy link
Contributor

Choose a reason for hiding this comment

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

@en-sc don't we need to handle ERROR_TARGET_RESOURCE_NOT_AVAILABLE return code here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the catch! Addressed in #899

@en-sc en-sc deleted the en-sc/trigg-eq-check branch August 14, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erhankur erhankur erhankur left review comments

@JanMatCodasip JanMatCodasip JanMatCodasip approved these changes

@timsifive timsifive timsifive left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@en-sc @timsifive @erhankur @JanMatCodasip