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: Mismatch napot when mcontrol.maskmax is not expected #1127

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

sunnyzhu-learning
Copy link

From #1124,
1.Rename trigger_request_info::tdata1_ignore_mask -> trigger_request_info::maskmax.
2.Add the something like the following to set_trigger():

const uint32_t type = get_field(tdata1, CSR_TDATA1_TYPE(riscv_xlen(target)));
if (type == CSR_TDATA1_TYPE_MCONTROL &&
(get_field(tdata1, CSR_MCONTROL_MATCH) == CSR_MCONTROL_MATCH_NAPOT))
tdata1_config_denied |= get_field(tdata1_rb, CSR_MCONTROL_MASKMAX) < (get_least_significant_zero_index(tdata2) + 1).
}

addressed
@en-sc Thanks for your suggestion, internal discussion a lot, and first push code to github,delay....please review~

@en-sc
Copy link
Collaborator

en-sc commented Sep 5, 2024

Checkpatch fails:

> Run ./tools/scripts/checkpatch.pl --no-signoff --git FETCH_HEAD..HEAD
WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one
...

Please, add an empty line between commit title and description.
See https://git-scm.com/docs/git-commit#_discussion

Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

Only minor comments, nothing significant.

@JanMatCodasip, @MarekVCodasip, could you please take a look?

On a side note:
@sunnyzhu-learning, have you tested the change? What was the simulator/HW used?

src/target/riscv/riscv.c Outdated Show resolved Hide resolved
@sunnyzhu-learning
Copy link
Author

Q:have you tested the change? What was the simulator/HW used?
A:tested in HW env,
following is gdb + OpenOCD test:

$ riscv64-unknown-elf-gdb hello_world.elf
Reading symbols from hello_world.elf...
(gdb) target remote:3333
Remote debugging using :3333
_start () at ../****/SoC/****/Common/Source/GCC/startup_***.S:111
111         csrc MSTATUS, MSTATUS_MIE
(gdb) load
Loading section .init, size 0x18e lma 0x80000000
Loading section .text, size 0xe38c lma 0x80000400
Loading section .rodata, size 0xd00 lma 0x8000e790
Loading section .init_array, size 0x4 lma 0x8000f490
Loading section .data, size 0x728 lma 0x800104a0
Loading section .amo, size 0x90 lma 0x80011f58
Start address 0x80000000, load size 63958
Transfer rate: 34 KB/sec, 7106 
(gdb) monitor wp 0x800104a0 0x2 r
[riscv.cpu] Found 4 triggers
[riscv.cpu] Could not set a trigger that will match a whole address range. As a fallback, this trigger (and maybe others) will only match against the first address of the range.
(gdb) monitor reg tselect 0
tselect (/32): 0x00000000
(gdb) monitor reg tdata1
tdata1 (/32): 0x28001049
(gdb) monitor reg tdata2
tdata2 (/32): 0x800104a0

en-sc
en-sc previously approved these changes Sep 12, 2024
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Hi @sunnyzhu-learning,

thanks for your contribution! Overall, I like the improvement.

I do have several review comments, though. If you can please look at them.

Thank you.

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
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
@en-sc
Copy link
Collaborator

en-sc commented Sep 18, 2024

@sunnyzhu-learning, what is the reason for closing the PR?
Please, consider reopening. I believe the patch to be very valuable.

@sunnyzhu-learning
Copy link
Author

@sunnyzhu-learning, what is the reason for closing the PR? Please, consider reopening. I believe the patch to be very valuable.

reopen this PR,solved code merge conflict,I'm very sorry,poor use of PR,@en-sc @JanMatCodasip please code review,thanks....

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.

@sunnyzhu-learning Thank you for addressing our feedback so far.

I have several more comments. If you can please look at them.

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 Outdated Show resolved Hide resolved
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 Outdated Show resolved Hide resolved
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
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.

@sunnyzhu-learning Thank you for addressing the review feedback so far.

I have posted two last comments (and that should be all from me).

src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
1.Remove trigger_request_info::tdata1_ignore_mask
2.Adding ignore napot matching condition

Signed-off-by: Songhe Zhu <[email protected]>
Reviewed-by: Evgeniy Naydanov <[email protected]>
Reviewed-by: Jan Matyas <[email protected]>
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.

LGTM.

@sunnyzhu-learning, thank you for your contribution.

Comment on lines +626 to +627
assert(sizeof(riscv_reg_t) * 8 == 64);
for (unsigned int i = 0; i < 64; i++) {
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
assert(sizeof(riscv_reg_t) * 8 == 64);
for (unsigned int i = 0; i < 64; i++) {
const unsigned int riscv_reg_bits = sizeof(riscv_reg_t) * CHAR_BIT;
for (unsigned int i = 0; i < riscv_reg_bits; i++) {

if ((1 & (reg >> i)) == 0)
return i;
}
return 64;
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 64;
return riscv_reg_bits;

@en-sc en-sc merged commit e4f5489 into riscv-collab:riscv Oct 4, 2024
4 checks passed
@sunnyzhu-learning
Copy link
Author

@en-sc Sorry,No timely feedback,I took a seven-day vacation,next PR will modify above changes~

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.

3 participants